On Mon, Dec 7, 2020 at 11:13 PM Krzysztof Kozlowski <k...@kernel.org> wrote: > > On Mon, Dec 07, 2020 at 10:39:01PM +0900, Bongsu Jeon wrote: > > On Mon, Dec 7, 2020 at 8:51 PM Krzysztof Kozlowski <k...@kernel.org> wrote: > > > > > > On Mon, Dec 07, 2020 at 08:38:27PM +0900, Bongsu Jeon wrote: > > > > From: Bongsu Jeon <bongsu.j...@samsung.com> > > > > > > > > change irqflags from IRQF_TRIGGER_HIGH to IRQF_TRIGGER_RISING for stable > > > > Samsung's nfc interrupt handling. > > > > > > 1. Describe in commit title/subject the change. Just a word "change > > > irqflags" is > > > not enough. > > > > > Ok. I'll update it. > > > > > 2. Describe in commit message what you are trying to fix. Before was not > > > stable? The "for stable interrupt handling" is a little bit vauge. > > > > > Usually, Samsung's NFC Firmware sends an i2c frame as below. > > > > 1. NFC Firmware sets the gpio(interrupt pin) high when there is an i2c > > frame to send. > > 2. If the CPU's I2C master has received the i2c frame, NFC F/W sets > > the gpio low. > > > > NFC driver's i2c interrupt handler would be called in the abnormal case > > as the NFC F/W task of number 2 is delayed because of other high > > priority tasks. > > In that case, NFC driver will try to receive the i2c frame but there > > isn't any i2c frame > > to send in NFC. It would cause an I2C communication problem. > > This case would hardly happen. > > But, I changed the interrupt as a defense code. > > If Driver uses the TRIGGER_RISING not LEVEL trigger, there would be no > > problem > > even if the NFC F/W task is delayed. > > All this should be explained in commit message, not in the email. > Okay. I will
> > > > > 3. This is contradictory to the bindings and current DTS. I think the > > > driver should not force the specific trigger type because I could > > > imagine some configuration that the actual interrupt to the CPU is > > > routed differently. > > > > > > Instead, how about removing the trigger flags here and fixing the DTS > > > and bindings example? > > > > > > > As I mentioned before, > > I changed this code because of Samsung NFC's I2C Communication way. > > So, I think that it is okay for the nfc driver to force the specific > > trigger type( EDGE_RISING). > > > > What do you think about it? > > Some different chip or some different hardware implementation could have > the signal inverted, e.g. edge falling, not rising. This is rather > a theoretical scenario but still such change makes the code more > generic, configurable with DTS. Therefore trigger mode should be > configured via DTS, not enforced by the driver. > Okay. I understand it. > Best regards, > Krzysztof
