| Message ID | 20180325110747.8852-1-ard.biesheuvel@linaro.org |
|---|---|
| Headers | show |
| Series | add support for Socionext SynQuacer I2C controller | expand |
On 25 March 2018 at 12:07, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > Add a binding and a driver for the I2C IP found in the Socionext SynQuacer > SoC, which is essentially a rebranded version of the Fujitsu F_I2C controller. > > v6: > - use i2c_8bit_addr_from_msg() instead of open coding the address generation > - switch to generic recovery using minimal helpers to drive the SDA/SCL lines > directly > - use reinit_completion() and move init_completion() to probe function > - replace bus free detection at the end of a transfer with a simple udelay() > for 2 clock periods > - don't recover on every error > - don't call synquacer_i2c_hw_init() from synquacer_i2c_hw_reset(), since it > will be always called twice in that case > - add patch to sanity check i2c_transfer() arguments in core code (#3) > Hello Wolfram, Any comments on this v6? Time is running out quickly for v4.17. Thanks, Ard. > v5: > - add Rob's ack to #1 > - drop unnecessary 'platform_set_drvdata(pdev, NULL)' in remove path (#2) > > v4: > - clarify binding that only a single interrupt specifier is expected (#1) > - check return value of clk_prepare_enable() on probe path (#2) > - add Andy's R-b to patch #2 > > v3: > - incorporate more of Andy's review comments (#2), especially regarding the > bus speed and clock source handling for ACPI > - patch #1 unchanged. > > v2: > - incorporate Andy's review comments (#2) > - patch #1 unchanged. > > Ard Biesheuvel (3): > dt-bindings: i2c: add binding for Socionext SynQuacer I2C > i2c: add support for Socionext SynQuacer I2C controller > i2c: add param sanity check to i2c_transfer() > > Documentation/devicetree/bindings/i2c/i2c-synquacer.txt | 29 + > drivers/i2c/busses/Kconfig | 10 + > drivers/i2c/busses/Makefile | 1 + > drivers/i2c/busses/i2c-synquacer.c | 739 ++++++++++++++++++++ > drivers/i2c/i2c-core-base.c | 3 + > 5 files changed, 782 insertions(+) > create mode 100644 Documentation/devicetree/bindings/i2c/i2c-synquacer.txt > create mode 100644 drivers/i2c/busses/i2c-synquacer.c > > -- > 2.15.1 >
> - switch to generic recovery using minimal helpers to drive the SDA/SCL lines > directly If you had added this incrementally, reviewing would have been easier (= faster). The callbacks look okay... > - don't recover on every error ... but you are now never recovering. You don't call i2c_recover_bus(). > - add patch to sanity check i2c_transfer() arguments in core code (#3) See small comment there.
On 3 April 2018 at 17:01, Wolfram Sang <wsa@the-dreams.de> wrote: > >> - switch to generic recovery using minimal helpers to drive the SDA/SCL lines >> directly > > If you had added this incrementally, reviewing would have been easier (= > faster). The callbacks look okay... > >> - don't recover on every error > > ... but you are now never recovering. You don't call i2c_recover_bus(). > Yeah, good point. So when exactly should this be called? Only on error conditions that could be caused by a misbehaving slave? >> - add patch to sanity check i2c_transfer() arguments in core code (#3) > > See small comment there. >
> Yeah, good point. So when exactly should this be called? Only on error > conditions that could be caused by a misbehaving slave? Chapter 3.1.16 of the I2C specification, only when a slave keeps SDA low. Usually you detect that before you start transmitting. Really, please remove it for now and add it later, properly tested! BTW what kind of tests did you do with this driver so far?
On 3 April 2018 at 20:12, Wolfram Sang <wsa@the-dreams.de> wrote: > >> Yeah, good point. So when exactly should this be called? Only on error >> conditions that could be caused by a misbehaving slave? > > Chapter 3.1.16 of the I2C specification, only when a slave keeps SDA > low. Usually you detect that before you start transmitting. > > Really, please remove it for now and add it later, properly tested! > OK, I will remove it for now. > BTW what kind of tests did you do with this driver so far? > I have used it with a ATSHA204A crypto chip, using a userland driver and i2c-dev, and with a NXP PCF8563 RTC using the kernel driver.
> I have used it with a ATSHA204A crypto chip, using a userland driver > and i2c-dev, and with a NXP PCF8563 RTC using the kernel driver. Sounds good. Suspend/resume?
x`On 3 April 2018 at 20:32, Wolfram Sang <wsa@the-dreams.de> wrote: > >> I have used it with a ATSHA204A crypto chip, using a userland driver >> and i2c-dev, and with a NXP PCF8563 RTC using the kernel driver. > > Sounds good. Suspend/resume? > We don't have suspend/resume support yet for this SoC, so I haven't been able to try that. Also, the clocks are fixed in this particular implementation, so the clk_xxx() calls don't actually do anything.
> We don't have suspend/resume support yet for this SoC, so I haven't > been able to try that. Also, the clocks are fixed in this particular > implementation, so the clk_xxx() calls don't actually do anything. Then please remove that, too! Good that I asked... Frankly, not nice to make me review untested code without mentioning that fact somewhere :( BTW are you willing to maintain this driver? Then an entry for MAINTAINERS would be needed.
On 3 April 2018 at 20:45, Wolfram Sang <wsa@the-dreams.de> wrote: > >> We don't have suspend/resume support yet for this SoC, so I haven't >> been able to try that. Also, the clocks are fixed in this particular >> implementation, so the clk_xxx() calls don't actually do anything. > > Then please remove that, too! Good that I asked... > OK > Frankly, not nice to make me review untested code without mentioning > that fact somewhere :( > My apologies. I simply didn't think to remove it - I didn't write the code, I am just the one upstreaming it. > BTW are you willing to maintain this driver? Then an entry for > MAINTAINERS would be needed. > OK, I will add that as well.