Re: [PATCH RESEND] i2c: add sc18is600 driver

[Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3252 bytes --]


> This is identical to the patch [0], that I send 3 months
> ago and got zero feedback from I2C maintainers.

I2C has not enough reviewers, this is a known issue. Pointing it out
alone does not really help. You could have reviewed your own patch;
after 3 months, one sometimes sees issues not noticed before. Or look at
other driver reviews to find out about common issues in new drivers
(like the suggested error codes on transmission failures)...

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-cp2120.txt b/Documentation/devicetree/bindings/i2c/i2c-cp2120.txt
> new file mode 100644
> index 000000000000..95e06e74f288
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-cp2120.txt
> @@ -0,0 +1 @@
> +Please see binding for i2c-sc18is600

I'll leave it to Rob, but since there is no driver 'i2c-cp2120', I'd
rather drop it.


> diff --git a/Documentation/devicetree/bindings/i2c/i2c-sc18is600.txt b/Documentation/devicetree/bindings/i2c/i2c-sc18is600.txt
> new file mode 100644
> index 000000000000..d0d9e680a5d6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-sc18is600.txt

And Rob will ask you to send bindings as seperate patches, so I'll do
already, too.


> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 144cbadc7c72..a6e776c1795e 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -210,6 +210,16 @@ config I2C_NFORCE2_S4985
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-nforce2-s4985.
>  
> +config I2C_SC18IS600
> +	tristate "NXP SC18IS600"
> +	depends on SPI && REGMAP
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  NXP SC18IS600 SPI to I2C-bus interface.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called i2c-sc18is600.
> +

It should rather go to the "External I2C/SMBus adapter drivers" section.

>  config I2C_SIS5595
>  	tristate "SiS 5595"
>  	depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 30b60855fbcd..29971aebd238 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_I2C_ISMT)		+= i2c-ismt.o
>  obj-$(CONFIG_I2C_NFORCE2)	+= i2c-nforce2.o
>  obj-$(CONFIG_I2C_NFORCE2_S4985)	+= i2c-nforce2-s4985.o
>  obj-$(CONFIG_I2C_PIIX4)		+= i2c-piix4.o
> +obj-$(CONFIG_I2C_SC18IS600)	+= i2c-sc18is600.o

Ditto.

>  obj-$(CONFIG_I2C_SIS5595)	+= i2c-sis5595.o
>  obj-$(CONFIG_I2C_SIS630)	+= i2c-sis630.o
>  obj-$(CONFIG_I2C_SIS96X)	+= i2c-sis96x.o
> +static int sc18is600_read(struct sc18is600dev *dev, struct i2c_msg *msg)
> +{
> +	u8 header[] = { SC18IS600_CMD_RD, msg->len, msg->addr << 1 };
> +	struct spi_transfer xfer[1] = { 0 };
> +
> +	xfer[0].tx_buf = header;
> +	xfer[0].len = sizeof(header);
> +
> +	dev_dbg(&dev->spi->dev, "r(addr=%x, len=%d)", msg->addr, msg->len);

Maybe you could drop these? We have tracing messages and core debug
messages for printing message destination addresses and lengths. Just
saying...

And what Andy said (thanks!). But looks quite good, in general.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]