Re: [PATCH v2] i2c: Driver to expose PowerNV platform i2c busses

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

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

On Sun, Nov 16, 2014 at 10:47:46PM +0530, Neelesh Gupta wrote:
> The patch exposes the available i2c busses on the PowerNV platform
> to the kernel and implements the bus driver to support i2c and
> smbus commands.
> The driver uses the platform device infrastructure to probe the busses
> on the platform and registers them with the i2c driver framework.
> 
> Signed-off-by: Neelesh Gupta <neelegup-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Signed-off-by: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>

...

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 917c358..71ad6e1 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1044,4 +1044,15 @@ config SCx200_ACB
>  	  This support is also available as a module.  If so, the module
>  	  will be called scx200_acb.
>  
> +config I2C_OPAL
> +	tristate "IBM OPAL I2C driver"
> +	depends on PPC_POWERNV
> +	default y
> +	help
> +	  This exposes the PowerNV platform i2c busses to the linux i2c layer,
> +	  the driver is based on the OPAL interfaces.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called as i2c-opal.
> +
>  endmenu
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 78d56c5..350aa86 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -102,5 +102,6 @@ obj-$(CONFIG_I2C_ELEKTOR)	+= i2c-elektor.o
>  obj-$(CONFIG_I2C_PCA_ISA)	+= i2c-pca-isa.o
>  obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
>  obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
> +obj-$(CONFIG_I2C_OPAL)		+= i2c-opal.o

Please keep it proprly sorted.

> +	rc = of_property_read_u32(pdev->dev.of_node, "ibm,opal-id", &opal_id);
> +	if (rc) {
> +		dev_err(&pdev->dev, "Missing ibm,opal-id property !\n");
> +		return -EIO;
> +	}

You introduce new bindings which need to be documented in
Docuemntation/devicetree/bindings/i2c.

They should be posted as a seperate patch with
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org CCed, so they can comment on it. This is
required these days and especially important..

> +	adapter = devm_kzalloc(&pdev->dev, sizeof(*adapter), GFP_KERNEL);
> +	if (!adapter)
> +		return -ENOMEM;
> +
> +	adapter->algo = &i2c_opal_algo;
> +	adapter->algo_data = (void *)(unsigned long)opal_id;
> +	adapter->dev.parent = &pdev->dev;
> +	adapter->dev.of_node = of_node_get(pdev->dev.of_node);
> +	pname = of_get_property(pdev->dev.of_node, "ibm,port-name", NULL);
> +	if (pname)
> +		strlcpy(adapter->name, pname, sizeof(adapter->name));
> +	else
> +		strlcpy(adapter->name, "opal", sizeof(adapter->name));

... because I'd like to get an ack from them because of this binding. I
don't know if we can just say "this comes from firmware, so we must
support it" (although you wrote the firmware IIUC) or if we have to
judge if this is a HW description which should go into DT? I am open
meanwhile that the adapter name does not need to be static anymore.
However, I don't know much about server world and FW, so maybe they can
assist.

An example binding in that document would also be very helpful.


> +static struct platform_driver i2c_opal_driver = {
> +	.probe	= i2c_opal_probe,
> +	.remove	= i2c_opal_remove,
> +	.driver	= {
> +		.name		= "i2c-opal",
> +		.owner		= THIS_MODULE,

owner not needed.

Thanks,

   Wolfram

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]