[PATCH 0/4] i2c: small updates to the slave framework

[PATCH 0/4] i2c: small updates to the slave framework

[Wolfram Sang]

Before nailing down the DT representation problem, here are some smaller
updates for the slave framework I have lying around.

Wolfram Sang (4):
  i2c: add FUNC flag for slave capabilities
  i2c: rcar: report slave capabilities to users
  i2c: slave: docs: be more precise about the prerequsites
  i2c: slave: add error messages to slave core

 Documentation/i2c/slave-interface | 25 +++++++++++++++----------
 drivers/i2c/busses/i2c-rcar.c     |  3 ++-
 drivers/i2c/i2c-core.c            | 22 +++++++++++++++++-----
 include/uapi/linux/i2c.h          |  1 +
 4 files changed, 35 insertions(+), 16 deletions(-)

-- 
2.1.4

[PATCH 1/4] i2c: add FUNC flag for slave capabilities

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

So users can check in advance if there is slave support.

Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
---
 include/uapi/linux/i2c.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/i2c.h b/include/uapi/linux/i2c.h
index 0e949cbfd33338..b0a7dd61eb353a 100644
--- a/include/uapi/linux/i2c.h
+++ b/include/uapi/linux/i2c.h
@@ -87,6 +87,7 @@ struct i2c_msg {
 #define I2C_FUNC_PROTOCOL_MANGLING	0x00000004 /* I2C_M_IGNORE_NAK etc. */
 #define I2C_FUNC_SMBUS_PEC		0x00000008
 #define I2C_FUNC_NOSTART		0x00000010 /* I2C_M_NOSTART */
+#define I2C_FUNC_SLAVE			0x00000020
 #define I2C_FUNC_SMBUS_BLOCK_PROC_CALL	0x00008000 /* SMBus 2.0 */
 #define I2C_FUNC_SMBUS_QUICK		0x00010000
 #define I2C_FUNC_SMBUS_READ_BYTE	0x00020000
-- 
2.1.4

Re: [PATCH 1/4] i2c: add FUNC flag for slave capabilities

[Uwe Kleine-König]

Hello Wolfram,

On Thu, May 14, 2015 at 02:40:02PM +0200, Wolfram Sang wrote:
> From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> 
> So users can check in advance if there is slave support.
I wonder if the core should check if .{,un}reg_slave is != NULL and
automatically set this bit accordingly.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/4] i2c: add FUNC flag for slave capabilities

[Wolfram Sang]

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

On Fri, May 15, 2015 at 01:37:44PM +0200, Uwe Kleine-König wrote:
> Hello Wolfram,
> 
> On Thu, May 14, 2015 at 02:40:02PM +0200, Wolfram Sang wrote:
> > From: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > 
> > So users can check in advance if there is slave support.
> I wonder if the core should check if .{,un}reg_slave is != NULL and
> automatically set this bit accordingly.

I thought about this, too, but decided against it. Maybe some driver
doesn't want to make its slave interface public because it is
specialized for one use-case only (thinking ECs here)? Also, doing it
would be a little inconsistent, since we could do the same for
I2C_FUNC_I2C when master_xfer is populated. So, let's start simple I'd
say.


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

Re: [PATCH 1/4] i2c: add FUNC flag for slave capabilities

[Uwe Kleine-König]

Hello Wolfram,

On Sat, May 16, 2015 at 10:22:16AM +0200, Wolfram Sang wrote:
> On Fri, May 15, 2015 at 01:37:44PM +0200, Uwe Kleine-König wrote:
> > On Thu, May 14, 2015 at 02:40:02PM +0200, Wolfram Sang wrote:
> > > From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
> > > 
> > > So users can check in advance if there is slave support.
> > I wonder if the core should check if .{,un}reg_slave is != NULL and
> > automatically set this bit accordingly.
> 
> I thought about this, too, but decided against it. Maybe some driver
> doesn't want to make its slave interface public because it is
> specialized for one use-case only (thinking ECs here)? Also, doing it
Hmm, I'd not say this is a valid reason. Sounds like policy in the
kernel?!

Does the framework check for the slave support flag before binding a
slave to the controler? I assume that not, but wonder if it should.

> would be a little inconsistent, since we could do the same for
> I2C_FUNC_I2C when master_xfer is populated. So, let's start simple I'd
> say.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/4] i2c: add FUNC flag for slave capabilities

[Wolfram Sang]

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


> Does the framework check for the slave support flag before binding a
> slave to the controler? I assume that not, but wonder if it should.

I think this nails it: Currently, the core doesn't use any I2C_FUNC_*
flags for checking anything. It is solely for exposing information to
users. I'd like to have FUNC_SLAVE in line with that for now.

If we are going to change that, it should be a seperate thread and not
limited to FUNC_SLAVE. I might note that for me this is not a priority
issue, so don't expect patches from me ;)


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

[PATCH 2/4] i2c: rcar: report slave capabilities to users

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
---
 drivers/i2c/busses/i2c-rcar.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 5a84bea5b84514..ae8c9c16ee9b08 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -604,7 +604,8 @@ static int rcar_unreg_slave(struct i2c_client *slave)
 static u32 rcar_i2c_func(struct i2c_adapter *adap)
 {
 	/* This HW can't do SMBUS_QUICK and NOSTART */
-	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+	return I2C_FUNC_I2C | I2C_FUNC_SLAVE |
+		(I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
 }
 
 static const struct i2c_algorithm rcar_i2c_algo = {
-- 
2.1.4

[PATCH 3/4] i2c: slave: docs: be more precise about the prerequsites

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

There was some confusion what was needed to utilize the slave support,
so let's be more precise about this. Add an introductory paragraph to
the development section while we are here.

Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
Cc: Peter Huewe <peterhuewe-Mmb7MZpHnFY@public.gmane.org>
---
 Documentation/i2c/slave-interface | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/Documentation/i2c/slave-interface b/Documentation/i2c/slave-interface
index 389bb5d618549e..02b086510b6c10 100644
--- a/Documentation/i2c/slave-interface
+++ b/Documentation/i2c/slave-interface
@@ -3,16 +3,16 @@ Linux I2C slave interface description
 
 by Wolfram Sang <wsa-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org> in 2014-15
 
-Linux can also be an I2C slave in case I2C controllers have slave support.
-Besides this HW requirement, one also needs a software backend providing the
-actual functionality. An example for this is the slave-eeprom driver, which
-acts as a dual memory driver. While another I2C master on the bus can access it
-like a regular EEPROM, the Linux I2C slave can access the content via sysfs and
-retrieve/provide information as needed. The software backend driver and the I2C
-bus driver communicate via events. Here is a small graph visualizing the data
-flow and the means by which data is transported. The dotted line marks only one
-example. The backend could also use e.g. a character device, be in-kernel
-only, or something completely different:
+Linux can also be an I2C slave if the I2C controller in use has slave
+functionality. For that to work, one needs slave support in the bus driver plus
+a hardware independent software backend providing the actual functionality. An
+example for the latter is the slave-eeprom driver, which acts as a dual memory
+driver. While another I2C master on the bus can access it like a regular
+EEPROM, the Linux I2C slave can access the content via sysfs and handle data as
+needed. The backend driver and the I2C bus driver communicate via events. Here
+is a small graph visualizing the data flow and the means by which data is
+transported. The dotted line marks only one example. The backend could also
+use a character device, be in-kernel only, or something completely different:
 
 
               e.g. sysfs        I2C slave events        I/O registers
@@ -43,6 +43,11 @@ behaviour and setup.
 Developer manual
 ================
 
+First, the events which are used by the bus driver and the backend will be
+described in detail. After that, some implementation hints for extending bus
+drivers and writing backends will be given.
+
+
 I2C slave events
 ----------------
 
-- 
2.1.4

[PATCH 4/4] i2c: slave: add error messages to slave core

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>

Inform users what went wrong from the core, so drivers don't have to do
it.

Signed-off-by: Wolfram Sang <wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/@public.gmane.org>
Cc: Peter Huewe <peterhuewe-Mmb7MZpHnFY@public.gmane.org>
---
 drivers/i2c/i2c-core.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 987c124432c501..e4dfdde832aa5f 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2914,18 +2914,24 @@ int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
 {
 	int ret;
 
-	if (!client || !slave_cb)
+	if (!client || !slave_cb) {
+		dev_err(&client->dev, "%s: insufficent data\n", __func__);
 		return -EINVAL;
+	}
 
 	if (!(client->flags & I2C_CLIENT_TEN)) {
 		/* Enforce stricter address checking */
 		ret = i2c_check_addr_validity(client->addr);
-		if (ret)
+		if (ret) {
+			dev_err(&client->dev, "%s: invalid address\n", __func__);
 			return ret;
+		}
 	}
 
-	if (!client->adapter->algo->reg_slave)
+	if (!client->adapter->algo->reg_slave) {
+		dev_err(&client->dev, "%s: not supported by adapter\n", __func__);
 		return -EOPNOTSUPP;
+	}
 
 	client->slave_cb = slave_cb;
 
@@ -2933,8 +2939,10 @@ int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
 	ret = client->adapter->algo->reg_slave(client);
 	i2c_unlock_adapter(client->adapter);
 
-	if (ret)
+	if (ret) {
 		client->slave_cb = NULL;
+		dev_err(&client->dev, "%s: adapter returned error %d\n", __func__, ret);
+	}
 
 	return ret;
 }
@@ -2944,8 +2952,10 @@ int i2c_slave_unregister(struct i2c_client *client)
 {
 	int ret;
 
-	if (!client->adapter->algo->unreg_slave)
+	if (!client->adapter->algo->unreg_slave) {
+		dev_err(&client->dev, "%s: not supported by adapter\n", __func__);
 		return -EOPNOTSUPP;
+	}
 
 	i2c_lock_adapter(client->adapter);
 	ret = client->adapter->algo->unreg_slave(client);
@@ -2953,6 +2963,8 @@ int i2c_slave_unregister(struct i2c_client *client)
 
 	if (ret == 0)
 		client->slave_cb = NULL;
+	else
+		dev_err(&client->dev, "%s: adapter returned error %d\n", __func__, ret);
 
 	return ret;
 }
-- 
2.1.4

Re: [PATCH 4/4] i2c: slave: add error messages to slave core

[Wolfram Sang]

> -	if (!client || !slave_cb)
> +	if (!client || !slave_cb) {
> +		dev_err(&client->dev, "%s: insufficent data\n", __func__);

Replaced this with WARN. Using client->dev after potential !client is bad.

Re: [PATCH 0/4] i2c: small updates to the slave framework

[Geert Uytterhoeven]

Hi Wolfram,

On Thu, May 14, 2015 at 2:40 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
> Before nailing down the DT representation problem, here are some smaller
> updates for the slave framework I have lying around.
>
> Wolfram Sang (4):
>   i2c: add FUNC flag for slave capabilities
>   i2c: rcar: report slave capabilities to users
>   i2c: slave: docs: be more precise about the prerequsites
>   i2c: slave: add error messages to slave core

This series looks fine to me

Acked-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 0/4] i2c: small updates to the slave framework

[Wolfram Sang]

On Thu, May 14, 2015 at 02:40:01PM +0200, Wolfram Sang wrote:
> Before nailing down the DT representation problem, here are some smaller
> updates for the slave framework I have lying around.
> 
> Wolfram Sang (4):
>   i2c: add FUNC flag for slave capabilities
>   i2c: rcar: report slave capabilities to users
>   i2c: slave: docs: be more precise about the prerequsites
>   i2c: slave: add error messages to slave core
> 

Applied to for-next with one small update to patch 4.