On Tue, 2014-11-25 at 18:53 +0100, Wolfram Sang wrote:

On Sun, Nov 16, 2014 at 10:47:46PM +0530, Neelesh Gupta wrote:

quoted

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 <redacted>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

...

quoted

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.

quoted

+	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.

Ugh ... thanks god the powerpc maintainer (me) didn't request binding
document for everything that went into the device-tree on those
platforms or we would still be writing them....

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

Suuure, let's create more process and committees, and make sure nothing
gets done in any reasonable amount of time. Have we gone completely
insane ?

This is completely useless as an exercise. It's not like I'm going to
change the firmware interfaces anymore anyway so the "comments" are
going to be at best bike shed painting.

I'm getting close to just put that driver in powerpc.git ...
 
The point of publicly discussing bindings in the ARM world was in part
because we attacked a community with no understanding of the DT, but in
LARGE part because we had to define them for components and SoC that
would potentially be re-used in different platforms etc..

Here we are talking about a representation specific to a given FW
interface on PowerPC only which isn't going to be used by any other
platform that PowerNV (since the OPAL fw is what makes PowerNV) by the
people who invented the frigging flat device tree in the first place, so
give me a break.

It's getting quite tempting to just throw that driver into powerpc.git

quoted

+	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.

And I don't give a flying crap about what random ARM SOC vendor thinks
of my powerpc FW interface for a powerpc unique FW interface.

 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.

I doubt it, all we're going to do is waste a few more monthes and make
it more painful for us to get our support into distros in time with 0
benefit whatsoever.

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

quoted

+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