[PATCH v2] i2c: Add generic support passing secondary devices addresses

[PATCH v2] i2c: Add generic support passing secondary devices addresses

[Jean-Michel Hautbois]

Some I2C devices have multiple addresses assigned, for example each address
corresponding to a different internal register map page of the device.
So far drivers which need support for this have handled this with a driver
specific and non-generic implementation, e.g. passing the additional address
via platform data.

This patch provides a new helper function called i2c_new_secondary_device()
which is intended to provide a generic way to get the secondary address
as well as instantiate a struct i2c_client for the secondary address.

The function expects a pointer to the primary i2c_client, a name
for the secondary address and an optional default address. The name is used
as a handle to specify which secondary address to get.

The default address is used as a fallback in case no secondary address
was explicitly specified. In case no secondary address and no default
address were specified the function returns NULL.

For now the function only supports look-up of the secondary address
from devicetree, but it can be extended in the future
to for example support board files and/or ACPI.

Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@veo-labs.com>
---
v2: adding some DT bindings documentation (more than one year later...)

 Documentation/devicetree/bindings/i2c/i2c.txt |  7 +++++
 drivers/i2c/i2c-core.c                        | 42 +++++++++++++++++++++++++++
 include/linux/i2c.h                           |  5 ++++
 3 files changed, 54 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index c8d977e..f31b2ad 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -62,6 +62,13 @@ wants to support one of the below features, it should adapt the bindings below.
 - wakeup-source
 	device can be used as a wakeup source.
 
+- reg
+	I2C slave addresses
+
+- reg-names
+	Names of map programmable addresses.
+	It can contain any map needing another address than default one.
+
 Binding may contain optional "interrupts" property, describing interrupts
 used by the device. I2C core will assign "irq" interrupt (or the very first
 interrupt if not using interrupt names) as primary interrupt for the slave.
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index ffe715d..da49fab 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1158,6 +1158,48 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
 }
 EXPORT_SYMBOL_GPL(i2c_new_dummy);
 
+/**
+ * i2c_new_secondary_device - Helper to get the instantiated secondary address
+ * and create the associated device
+ * @client: Handle to the primary client
+ * @name: Handle to specify which secondary address to get
+ * @default_addr: Used as a fallback if no secondary address was specified
+ * Context: can sleep
+ *
+ * I2C clients can be composed of multiple I2C slaves bound together in a single
+ * component. The I2C client driver then binds to the master I2C slave and needs
+ * to create I2C dummy clients to communicate with all the other slaves.
+ *
+ * This function creates and returns an I2C dummy client whose I2C address is
+ * retrieved from the platform firmware based on the given slave name. If no
+ * address is specified by the firmware default_addr is used.
+ *
+ * On DT-based platforms the address is retrieved from the "reg" property entry
+ * cell whose "reg-names" value matches the slave name.
+ *
+ * This returns the new i2c client, which should be saved for later use with
+ * i2c_unregister_device(); or NULL to indicate an error.
+ */
+struct i2c_client *i2c_new_secondary_device(struct i2c_client *client,
+						const char *name,
+						u16 default_addr)
+{
+	struct device_node *np = client->dev.of_node;
+	u32 addr = default_addr;
+	int i;
+
+	if (np) {
+		i = of_property_match_string(np, "reg-names", name);
+		if (i >= 0)
+			of_property_read_u32_index(np, "reg", i, &addr);
+	}
+
+	dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
+	return i2c_new_dummy(client->adapter, addr);
+}
+EXPORT_SYMBOL_GPL(i2c_new_secondary_device);
+
+
 /* ------------------------------------------------------------------------- */
 
 /* I2C bus adapters -- one roots each I2C or SMBUS segment */
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 200cf13b..9c90735 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -349,6 +349,11 @@ extern int i2c_probe_func_quick_read(struct i2c_adapter *, unsigned short addr);
 extern struct i2c_client *
 i2c_new_dummy(struct i2c_adapter *adap, u16 address);
 
+extern struct i2c_client *
+i2c_new_secondary_device(struct i2c_client *client,
+				const char *name,
+				u16 default_addr);
+
 extern void i2c_unregister_device(struct i2c_client *);
 #endif /* I2C */
 
-- 
2.7.0

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Rob Herring]

On Sun, Jan 31, 2016 at 04:33:00PM +0100, Jean-Michel Hautbois wrote:
> Some I2C devices have multiple addresses assigned, for example each address
> corresponding to a different internal register map page of the device.
> So far drivers which need support for this have handled this with a driver
> specific and non-generic implementation, e.g. passing the additional address
> via platform data.
> 
> This patch provides a new helper function called i2c_new_secondary_device()
> which is intended to provide a generic way to get the secondary address
> as well as instantiate a struct i2c_client for the secondary address.
> 
> The function expects a pointer to the primary i2c_client, a name
> for the secondary address and an optional default address. The name is used
> as a handle to specify which secondary address to get.
> 
> The default address is used as a fallback in case no secondary address
> was explicitly specified. In case no secondary address and no default
> address were specified the function returns NULL.
> 
> For now the function only supports look-up of the secondary address
> from devicetree, but it can be extended in the future
> to for example support board files and/or ACPI.
> 
> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@veo-labs.com>
> ---
> v2: adding some DT bindings documentation (more than one year later...)
> 
>  Documentation/devicetree/bindings/i2c/i2c.txt |  7 +++++

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/i2c/i2c-core.c                        | 42 +++++++++++++++++++++++++++
>  include/linux/i2c.h                           |  5 ++++
>  3 files changed, 54 insertions(+)

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Jean-Michel Hautbois]

Hi !

2016-02-01 15:46 GMT+01:00 Rob Herring <robh@kernel.org>:
> On Sun, Jan 31, 2016 at 04:33:00PM +0100, Jean-Michel Hautbois wrote:
>> Some I2C devices have multiple addresses assigned, for example each address
>> corresponding to a different internal register map page of the device.
>> So far drivers which need support for this have handled this with a driver
>> specific and non-generic implementation, e.g. passing the additional address
>> via platform data.
>>
>> This patch provides a new helper function called i2c_new_secondary_device()
>> which is intended to provide a generic way to get the secondary address
>> as well as instantiate a struct i2c_client for the secondary address.
>>
>> The function expects a pointer to the primary i2c_client, a name
>> for the secondary address and an optional default address. The name is used
>> as a handle to specify which secondary address to get.
>>
>> The default address is used as a fallback in case no secondary address
>> was explicitly specified. In case no secondary address and no default
>> address were specified the function returns NULL.
>>
>> For now the function only supports look-up of the secondary address
>> from devicetree, but it can be extended in the future
>> to for example support board files and/or ACPI.
>>
>> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@veo-labs.com>
>> ---
>> v2: adding some DT bindings documentation (more than one year later...)
>>
>>  Documentation/devicetree/bindings/i2c/i2c.txt |  7 +++++
>
> Acked-by: Rob Herring <robh@kernel.org>
>
>>  drivers/i2c/i2c-core.c                        | 42 +++++++++++++++++++++++++++
>>  include/linux/i2c.h                           |  5 ++++
>>  3 files changed, 54 insertions(+)

Thanks Rob for your ack. What is the future of this patch ?
I know the merge window is opened, so it will not be integrated now,
but could be in the next version ?

Thanks,
JM

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Rob Herring]

On Thu, Mar 24, 2016 at 5:11 AM, Jean-Michel Hautbois
<jean-michel.hautbois@veo-labs.com> wrote:
> Hi !
>
> 2016-02-01 15:46 GMT+01:00 Rob Herring <robh@kernel.org>:
>> On Sun, Jan 31, 2016 at 04:33:00PM +0100, Jean-Michel Hautbois wrote:
>>> Some I2C devices have multiple addresses assigned, for example each address
>>> corresponding to a different internal register map page of the device.
>>> So far drivers which need support for this have handled this with a driver
>>> specific and non-generic implementation, e.g. passing the additional address
>>> via platform data.
>>>
>>> This patch provides a new helper function called i2c_new_secondary_device()
>>> which is intended to provide a generic way to get the secondary address
>>> as well as instantiate a struct i2c_client for the secondary address.
>>>
>>> The function expects a pointer to the primary i2c_client, a name
>>> for the secondary address and an optional default address. The name is used
>>> as a handle to specify which secondary address to get.
>>>
>>> The default address is used as a fallback in case no secondary address
>>> was explicitly specified. In case no secondary address and no default
>>> address were specified the function returns NULL.
>>>
>>> For now the function only supports look-up of the secondary address
>>> from devicetree, but it can be extended in the future
>>> to for example support board files and/or ACPI.
>>>
>>> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@veo-labs.com>
>>> ---
>>> v2: adding some DT bindings documentation (more than one year later...)
>>>
>>>  Documentation/devicetree/bindings/i2c/i2c.txt |  7 +++++
>>
>> Acked-by: Rob Herring <robh@kernel.org>
>>
>>>  drivers/i2c/i2c-core.c                        | 42 +++++++++++++++++++++++++++
>>>  include/linux/i2c.h                           |  5 ++++
>>>  3 files changed, 54 insertions(+)
>
> Thanks Rob for your ack. What is the future of this patch ?

I'm expecting Wolfram to pick it up.

> I know the merge window is opened, so it will not be integrated now,
> but could be in the next version ?

I would say it is not too late for 4.6 if it was missed by accident.

Rob

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Wolfram Sang]

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

On Sun, Jan 31, 2016 at 04:33:00PM +0100, Jean-Michel Hautbois wrote:
> Some I2C devices have multiple addresses assigned, for example each address
> corresponding to a different internal register map page of the device.
> So far drivers which need support for this have handled this with a driver
> specific and non-generic implementation, e.g. passing the additional address
> via platform data.
> 
> This patch provides a new helper function called i2c_new_secondary_device()
> which is intended to provide a generic way to get the secondary address
> as well as instantiate a struct i2c_client for the secondary address.
> 
> The function expects a pointer to the primary i2c_client, a name
> for the secondary address and an optional default address. The name is used
> as a handle to specify which secondary address to get.
> 
> The default address is used as a fallback in case no secondary address
> was explicitly specified. In case no secondary address and no default
> address were specified the function returns NULL.

With "no default address" you mean the function parameter is 0? Yeah,
you get a NULL pointer from i2c_new_device then, but also a dev_err
message, or?

> For now the function only supports look-up of the secondary address
> from devicetree, but it can be extended in the future
> to for example support board files and/or ACPI.

Ccing Mika. I think it can be 1:1 converted to device_property_*
functions already giving ACPI support for free?

Thanks,

   Wolfram


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

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Mika Westerberg]

+Srinivas

On Sun, Jan 31, 2016 at 04:33:00PM +0100, Jean-Michel Hautbois wrote:
> Some I2C devices have multiple addresses assigned, for example each address
> corresponding to a different internal register map page of the device.
> So far drivers which need support for this have handled this with a driver
> specific and non-generic implementation, e.g. passing the additional address
> via platform data.
> 
> This patch provides a new helper function called i2c_new_secondary_device()
> which is intended to provide a generic way to get the secondary address
> as well as instantiate a struct i2c_client for the secondary address.
> 
> The function expects a pointer to the primary i2c_client, a name
> for the secondary address and an optional default address. The name is used
> as a handle to specify which secondary address to get.
> 
> The default address is used as a fallback in case no secondary address
> was explicitly specified. In case no secondary address and no default
> address were specified the function returns NULL.
> 
> For now the function only supports look-up of the secondary address
> from devicetree, but it can be extended in the future
> to for example support board files and/or ACPI.

It was not clear to me but does this support more than two addresses?
For example we might a device with 3 I2cSerialBus() connectors:

    Device (CAM1)
    {
        Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource Settings
        {
            Name (SBUF, ResourceTemplate ()
            {
                ...
                I2cSerialBus (0x0010, ControllerInitiated, 0x00061A80,
                    AddressingMode7Bit, "\\_SB.I2C4",
                    0x00, ResourceConsumer, ,)
                I2cSerialBus (0x000C, ControllerInitiated, 0x00061A80,
                    AddressingMode7Bit, "\\_SB.I2C4",
                    0x00, ResourceConsumer, ,)
                I2cSerialBus (0x0054, ControllerInitiated, 0x00061A80,
                    AddressingMode7Bit, "\\_SB.I2C4",
                    0x00, ResourceConsumer, ,)
            })
            Return (SBUF) /* \_SB_.I2C4.CAM1._CRS.SBUF */
        }
        ...

Furthermore those do not have names. At least the existing ones out
there, which is why I think we should instead refer them with integer
indexes. I think that works also with DT. Then provide an optional
"reg-names" or whatever that can be used to get secondary addresses with
certain name. For new stuff we can use names with ACPI _DSD method like:

    Name (_DSD, Package () {
        ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
        Package () {
            Package () {"reg-names", Package() {"primary", "secondary-1", "secondary-2"}}
        }
    })

Here "secondary-1" maps to second entry in _CRS. Although not sure how
useful the whole naming thing is.

> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@veo-labs.com>
> ---
> v2: adding some DT bindings documentation (more than one year later...)
> 
>  Documentation/devicetree/bindings/i2c/i2c.txt |  7 +++++
>  drivers/i2c/i2c-core.c                        | 42 +++++++++++++++++++++++++++
>  include/linux/i2c.h                           |  5 ++++
>  3 files changed, 54 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
> index c8d977e..f31b2ad 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c.txt
> @@ -62,6 +62,13 @@ wants to support one of the below features, it should adapt the bindings below.
>  - wakeup-source
>  	device can be used as a wakeup source.
>  
> +- reg
> +	I2C slave addresses
> +
> +- reg-names
> +	Names of map programmable addresses.
> +	It can contain any map needing another address than default one.
> +
>  Binding may contain optional "interrupts" property, describing interrupts
>  used by the device. I2C core will assign "irq" interrupt (or the very first
>  interrupt if not using interrupt names) as primary interrupt for the slave.
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index ffe715d..da49fab 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -1158,6 +1158,48 @@ struct i2c_client *i2c_new_dummy(struct i2c_adapter *adapter, u16 address)
>  }
>  EXPORT_SYMBOL_GPL(i2c_new_dummy);
>  
> +/**
> + * i2c_new_secondary_device - Helper to get the instantiated secondary address
> + * and create the associated device
> + * @client: Handle to the primary client
> + * @name: Handle to specify which secondary address to get
> + * @default_addr: Used as a fallback if no secondary address was specified
> + * Context: can sleep
> + *
> + * I2C clients can be composed of multiple I2C slaves bound together in a single
> + * component. The I2C client driver then binds to the master I2C slave and needs
> + * to create I2C dummy clients to communicate with all the other slaves.
> + *
> + * This function creates and returns an I2C dummy client whose I2C address is
> + * retrieved from the platform firmware based on the given slave name. If no
> + * address is specified by the firmware default_addr is used.
> + *
> + * On DT-based platforms the address is retrieved from the "reg" property entry
> + * cell whose "reg-names" value matches the slave name.
> + *
> + * This returns the new i2c client, which should be saved for later use with
> + * i2c_unregister_device(); or NULL to indicate an error.
> + */
> +struct i2c_client *i2c_new_secondary_device(struct i2c_client *client,
> +						const char *name,
> +						u16 default_addr)
> +{
> +	struct device_node *np = client->dev.of_node;
> +	u32 addr = default_addr;
> +	int i;
> +
> +	if (np) {
> +		i = of_property_match_string(np, "reg-names", name);
> +		if (i >= 0)
> +			of_property_read_u32_index(np, "reg", i, &addr);
> +	}
> +
> +	dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
> +	return i2c_new_dummy(client->adapter, addr);
> +}
> +EXPORT_SYMBOL_GPL(i2c_new_secondary_device);
> +
> +
>  /* ------------------------------------------------------------------------- */
>  
>  /* I2C bus adapters -- one roots each I2C or SMBUS segment */
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 200cf13b..9c90735 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -349,6 +349,11 @@ extern int i2c_probe_func_quick_read(struct i2c_adapter *, unsigned short addr);
>  extern struct i2c_client *
>  i2c_new_dummy(struct i2c_adapter *adap, u16 address);
>  
> +extern struct i2c_client *
> +i2c_new_secondary_device(struct i2c_client *client,
> +				const char *name,
> +				u16 default_addr);
> +
>  extern void i2c_unregister_device(struct i2c_client *);
>  #endif /* I2C */
>  
> -- 
> 2.7.0

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Srinivas Pandruvada]

On Fri, 2016-04-15 at 11:01 +0300, Mika Westerberg wrote:
> +Srinivas
> 
> On Sun, Jan 31, 2016 at 04:33:00PM +0100, Jean-Michel Hautbois wrote:
> > 
> > Some I2C devices have multiple addresses assigned, for example each
> > address
> > corresponding to a different internal register map page of the
> > device.
> > So far drivers which need support for this have handled this with a
> > driver
> > specific and non-generic implementation, e.g. passing the
> > additional address
> > via platform data.
> > 
> > This patch provides a new helper function called
> > i2c_new_secondary_device()
> > which is intended to provide a generic way to get the secondary
> > address
> > as well as instantiate a struct i2c_client for the secondary
> > address.
> > 
> > The function expects a pointer to the primary i2c_client, a name
> > for the secondary address and an optional default address. The name
> > is used
> > as a handle to specify which secondary address to get.
> > 
> > The default address is used as a fallback in case no secondary
> > address
> > was explicitly specified. In case no secondary address and no
> > default
> > address were specified the function returns NULL.
> > 
> > For now the function only supports look-up of the secondary address
> > from devicetree, but it can be extended in the future
> > to for example support board files and/or ACPI.
> It was not clear to me but does this support more than two addresses?

In past when we were looking at this for Invensese MPU60XX device, we
had two I2cSerialBus() entries, but they belong to two different
devices. The first one for the MPU60XX and second one for another
device which is a slave for MPU60XX. This slave device can be also be
reached from master i2c controller, by disabling i2c master capability
of MPU60XX device.
So ACPI spec is very vague in specifying what they really meant.

> For example we might a device with 3 I2cSerialBus() connectors:
> 
>     Device (CAM1)
>     {
>         Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource
> Settings
>         {
>             Name (SBUF, ResourceTemplate ()
>             {
>                 ...
>                 I2cSerialBus (0x0010, ControllerInitiated,
> 0x00061A80,
>                     AddressingMode7Bit, "\\_SB.I2C4",
>                     0x00, ResourceConsumer, ,)
>                 I2cSerialBus (0x000C, ControllerInitiated,
> 0x00061A80,
>                     AddressingMode7Bit, "\\_SB.I2C4",
>                     0x00, ResourceConsumer, ,)
>                 I2cSerialBus (0x0054, ControllerInitiated,
> 0x00061A80,
>                     AddressingMode7Bit, "\\_SB.I2C4",
>                     0x00, ResourceConsumer, ,)
>             })
>             Return (SBUF) /* \_SB_.I2C4.CAM1._CRS.SBUF */
>         }
>         ...
> 
> Furthermore those do not have names.
Correct. Better to use indexes. I think Mika also proposed similar API
in past.

Thanks,
Srinivas

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Lars-Peter Clausen]

On 04/18/2016 05:20 PM, Srinivas Pandruvada wrote:
> On Fri, 2016-04-15 at 11:01 +0300, Mika Westerberg wrote:
>> +Srinivas
>>
>> On Sun, Jan 31, 2016 at 04:33:00PM +0100, Jean-Michel Hautbois wrote:
>>>
>>> Some I2C devices have multiple addresses assigned, for example each
>>> address
>>> corresponding to a different internal register map page of the
>>> device.
>>> So far drivers which need support for this have handled this with a
>>> driver
>>> specific and non-generic implementation, e.g. passing the
>>> additional address
>>> via platform data.
>>>
>>> This patch provides a new helper function called
>>> i2c_new_secondary_device()
>>> which is intended to provide a generic way to get the secondary
>>> address
>>> as well as instantiate a struct i2c_client for the secondary
>>> address.
>>>
>>> The function expects a pointer to the primary i2c_client, a name
>>> for the secondary address and an optional default address. The name
>>> is used
>>> as a handle to specify which secondary address to get.
>>>
>>> The default address is used as a fallback in case no secondary
>>> address
>>> was explicitly specified. In case no secondary address and no
>>> default
>>> address were specified the function returns NULL.
>>>
>>> For now the function only supports look-up of the secondary address
>>> from devicetree, but it can be extended in the future
>>> to for example support board files and/or ACPI.
>> It was not clear to me but does this support more than two addresses?
> 
> In past when we were looking at this for Invensese MPU60XX device, we
> had two I2cSerialBus() entries, but they belong to two different
> devices. The first one for the MPU60XX and second one for another
> device which is a slave for MPU60XX. This slave device can be also be
> reached from master i2c controller, by disabling i2c master capability
> of MPU60XX device.
> So ACPI spec is very vague in specifying what they really meant.
> 
>> For example we might a device with 3 I2cSerialBus() connectors:
>>
>>     Device (CAM1)
>>     {
>>         Method (_CRS, 0, NotSerialized)  // _CRS: Current Resource
>> Settings
>>         {
>>             Name (SBUF, ResourceTemplate ()
>>             {
>>                 ...
>>                 I2cSerialBus (0x0010, ControllerInitiated,
>> 0x00061A80,
>>                     AddressingMode7Bit, "\\_SB.I2C4",
>>                     0x00, ResourceConsumer, ,)
>>                 I2cSerialBus (0x000C, ControllerInitiated,
>> 0x00061A80,
>>                     AddressingMode7Bit, "\\_SB.I2C4",
>>                     0x00, ResourceConsumer, ,)
>>                 I2cSerialBus (0x0054, ControllerInitiated,
>> 0x00061A80,
>>                     AddressingMode7Bit, "\\_SB.I2C4",
>>                     0x00, ResourceConsumer, ,)
>>             })
>>             Return (SBUF) /* \_SB_.I2C4.CAM1._CRS.SBUF */
>>         }
>>         ...
>>
>> Furthermore those do not have names.
> Correct. Better to use indexes. I think Mika also proposed similar API
> in past.

A generic API by indexes wont work. The order between DT and ACPI will most
likely be different. I'd even assume that the order will be different with
ACPI for the same device on different platforms.

If we want to support ACPI over the same interface drivers need to provide a
lookup table that maps a name to the index.

- Lars

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Mika Westerberg]

On Mon, Apr 18, 2016 at 05:26:54PM +0200, Lars-Peter Clausen wrote:
> A generic API by indexes wont work. The order between DT and ACPI will most
> likely be different. I'd even assume that the order will be different with
> ACPI for the same device on different platforms.

Yes, unfortunately that might be possible.

> If we want to support ACPI over the same interface drivers need to provide a
> lookup table that maps a name to the index.

Indeed something like we already have with GPIOs. The lookup table could
be filled from names in _DSD where it is available.

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Lars-Peter Clausen]

On 04/19/2016 02:40 PM, Mika Westerberg wrote:
> On Mon, Apr 18, 2016 at 05:26:54PM +0200, Lars-Peter Clausen wrote:
>> A generic API by indexes wont work. The order between DT and ACPI will most
>> likely be different. I'd even assume that the order will be different with
>> ACPI for the same device on different platforms.
> 
> Yes, unfortunately that might be possible.
> 
>> If we want to support ACPI over the same interface drivers need to provide a
>> lookup table that maps a name to the index.
> 
> Indeed something like we already have with GPIOs. The lookup table could
> be filled from names in _DSD where it is available.

Does that mean you are OK with the patch as it is?

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Mika Westerberg]

On Tue, Apr 19, 2016 at 03:02:06PM +0200, Lars-Peter Clausen wrote:
> On 04/19/2016 02:40 PM, Mika Westerberg wrote:
> > On Mon, Apr 18, 2016 at 05:26:54PM +0200, Lars-Peter Clausen wrote:
> >> A generic API by indexes wont work. The order between DT and ACPI will most
> >> likely be different. I'd even assume that the order will be different with
> >> ACPI for the same device on different platforms.
> > 
> > Yes, unfortunately that might be possible.
> > 
> >> If we want to support ACPI over the same interface drivers need to provide a
> >> lookup table that maps a name to the index.
> > 
> > Indeed something like we already have with GPIOs. The lookup table could
> > be filled from names in _DSD where it is available.
> 
> Does that mean you are OK with the patch as it is?

It is still not clear to me if this supports more than two addresses and
if it does, how those are represented in DT and how the function can be
used to fetch all those additional addresses.

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Lars-Peter Clausen]

On 04/19/2016 03:16 PM, Mika Westerberg wrote:
> On Tue, Apr 19, 2016 at 03:02:06PM +0200, Lars-Peter Clausen wrote:
>> On 04/19/2016 02:40 PM, Mika Westerberg wrote:
>>> On Mon, Apr 18, 2016 at 05:26:54PM +0200, Lars-Peter Clausen wrote:
>>>> A generic API by indexes wont work. The order between DT and ACPI will most
>>>> likely be different. I'd even assume that the order will be different with
>>>> ACPI for the same device on different platforms.
>>>
>>> Yes, unfortunately that might be possible.
>>>
>>>> If we want to support ACPI over the same interface drivers need to provide a
>>>> lookup table that maps a name to the index.
>>>
>>> Indeed something like we already have with GPIOs. The lookup table could
>>> be filled from names in _DSD where it is available.
>>
>> Does that mean you are OK with the patch as it is?
> 
> It is still not clear to me if this supports more than two addresses and
> if it does, how those are represented in DT and how the function can be
> used to fetch all those additional addresses.

It adds a standard API for dealing with devices that have more than one
address. It uses the normal way of specifying multiple (named) address in DT.

reg = <0xa>, <0xb>, <0xc>;
reg-names = "main", "aux1", "aux2";

The function the new i2c_new_secondary_device() function takes the name of
the address region and allocates a dummy I2C client for it, which can be
used to access that I2C part of the device that responds to that region. The
name is used to lookup the address from the name to address map.

- Lars

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Wolfram Sang]

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


> It is still not clear to me if this supports more than two addresses and

I stumbled over this as well: "Secondary" doesn't mean "as in second address",
but "as in not primary address". So, more than two are supported.


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

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Mika Westerberg]

On Tue, Apr 19, 2016 at 03:31:41PM +0200, Lars-Peter Clausen wrote:
> It adds a standard API for dealing with devices that have more than one
> address. It uses the normal way of specifying multiple (named) address in DT.
> 
> reg = <0xa>, <0xb>, <0xc>;
> reg-names = "main", "aux1", "aux2";
> 
> The function the new i2c_new_secondary_device() function takes the name of
> the address region and allocates a dummy I2C client for it, which can be
> used to access that I2C part of the device that responds to that region. The
> name is used to lookup the address from the name to address map.

Thanks for the explanation.

Yes, I think we can support this in ACPI using both _DSD and built-in
properties so we do not need to add new fields to struct acpi_device
like we did with GPIOs. If _DSD does not have "reg-names" property we
can provide built-in version in a driver:

	static const char *mydev_reg_names[] = { "main", "aux1", "aux2" };
		
	static struct property_entry mydev_properties[] = {
		PROPERTY_ENTRY_STRING_ARRAY("reg-names", mydev_reg_names),
		{ },
	};

Then call device_add_properties() for the "primary" I2C client to make
them available to the unified properties API.

The i2c_new_secondary_device() would then look like:

	struct i2c_client *i2c_new_secondary_device(struct i2c_client *client,
					    const char *name, u16 default_addr)
	{       
		struct device_node *np = client->dev.of_node;
		u32 addr = default_addr;
		int i;
		
		i = device_property_match_string(&client->dev, "reg-names", name);
		if (i >= 0) {
			if (np) {
				of_property_read_u32_index(np, "reg", i, &addr);
			} else if (has_acpi_companion(&client->dev)) {
				/* Look for the ith I2cSerialBus() in _CRS */
			}
		}

		dev_dbg(&client->adapter->dev, "Address for %s : 0x%x\n", name, addr);
		return i2c_new_dummy(client->adapter, addr);
	}
	EXPORT_SYMBOL_GPL(i2c_new_secondary_device);

The above should work with both DT and ACPI so I'm OK with the current
patch. We can add ACPI parts later when needed.

Srinivas, do you think this works with the sensor stuff?

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Mika Westerberg]

On Tue, Apr 19, 2016 at 03:49:57PM +0200, Wolfram Sang wrote:
> 
> > It is still not clear to me if this supports more than two addresses and
> 
> I stumbled over this as well: "Secondary" doesn't mean "as in second address",
> but "as in not primary address". So, more than two are supported.

Indeed, thanks for clearing that up for me.

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Srinivas Pandruvada]

On Tue, 2016-04-19 at 17:40 +0300, Mika Westerberg wrote:
> On Tue, Apr 19, 2016 at 03:31:41PM +0200, Lars-Peter Clausen wrote:
> > 
> > It adds a standard API for dealing with devices that have more than
> > one
> > address. It uses the normal way of specifying multiple (named)
> > address in DT.
> > 
> > reg = <0xa>, <0xb>, <0xc>;
> > reg-names = "main", "aux1", "aux2";
> > 
[...]

> Srinivas, do you think this works with the sensor stuff?
Yes, it will work.

Thanks,
Srinivas

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Wolfram Sang]

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


> The above should work with both DT and ACPI so I'm OK with the current
> patch. We can add ACPI parts later when needed.

Just to make sure, I read this as an ack for the original patch?


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

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Mika Westerberg]

On Sun, Apr 24, 2016 at 10:14:37PM +0200, Wolfram Sang wrote:
> 
> > The above should work with both DT and ACPI so I'm OK with the current
> > patch. We can add ACPI parts later when needed.
> 
> Just to make sure, I read this as an ack for the original patch?

Yes :)

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Wolfram Sang]

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

On Mon, Apr 25, 2016 at 10:25:53AM +0300, Mika Westerberg wrote:
> On Sun, Apr 24, 2016 at 10:14:37PM +0200, Wolfram Sang wrote:
> > 
> > > The above should work with both DT and ACPI so I'm OK with the current
> > > patch. We can add ACPI parts later when needed.
> > 
> > Just to make sure, I read this as an ack for the original patch?
> 
> Yes :)

Thanks! Could you use the formal tag for that in the future please?
Patchwork automatically collects them and that saves me some work. (I
even have a keyboard macro for that and use it a lot)


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

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Mika Westerberg]

On Mon, Apr 25, 2016 at 09:39:41AM +0200, Wolfram Sang wrote:
> On Mon, Apr 25, 2016 at 10:25:53AM +0300, Mika Westerberg wrote:
> > On Sun, Apr 24, 2016 at 10:14:37PM +0200, Wolfram Sang wrote:
> > > 
> > > > The above should work with both DT and ACPI so I'm OK with the current
> > > > patch. We can add ACPI parts later when needed.
> > > 
> > > Just to make sure, I read this as an ack for the original patch?
> > 
> > Yes :)
> 
> Thanks! Could you use the formal tag for that in the future please?
> Patchwork automatically collects them and that saves me some work. (I
> even have a keyboard macro for that and use it a lot)

Sure, sorry about that.

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Lars-Peter Clausen]

On 04/25/2016 09:41 AM, Mika Westerberg wrote:
> On Mon, Apr 25, 2016 at 09:39:41AM +0200, Wolfram Sang wrote:
>> On Mon, Apr 25, 2016 at 10:25:53AM +0300, Mika Westerberg wrote:
>>> On Sun, Apr 24, 2016 at 10:14:37PM +0200, Wolfram Sang wrote:
>>>>
>>>>> The above should work with both DT and ACPI so I'm OK with the current
>>>>> patch. We can add ACPI parts later when needed.
>>>>
>>>> Just to make sure, I read this as an ack for the original patch?
>>>
>>> Yes :)
>>
>> Thanks! Could you use the formal tag for that in the future please?
>> Patchwork automatically collects them and that saves me some work. (I
>> even have a keyboard macro for that and use it a lot)
> 
> Sure, sorry about that.
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Hi Wolfram,

It looks like everybody was OK now with this patch, but it got lost somehow.
Should it be resent, or can you still pick it up from patchwork?

Thanks,
- Lars

Re: [PATCH v2] i2c: Add generic support passing secondary devices addresses

[Wolfram Sang]

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

On Sun, Jan 31, 2016 at 04:33:00PM +0100, Jean-Michel Hautbois wrote:
> Some I2C devices have multiple addresses assigned, for example each address
> corresponding to a different internal register map page of the device.
> So far drivers which need support for this have handled this with a driver
> specific and non-generic implementation, e.g. passing the additional address
> via platform data.
> 
> This patch provides a new helper function called i2c_new_secondary_device()
> which is intended to provide a generic way to get the secondary address
> as well as instantiate a struct i2c_client for the secondary address.
> 
> The function expects a pointer to the primary i2c_client, a name
> for the secondary address and an optional default address. The name is used
> as a handle to specify which secondary address to get.
> 
> The default address is used as a fallback in case no secondary address
> was explicitly specified. In case no secondary address and no default
> address were specified the function returns NULL.
> 
> For now the function only supports look-up of the secondary address
> from devicetree, but it can be extended in the future
> to for example support board files and/or ACPI.
> 
> Signed-off-by: Jean-Michel Hautbois <jean-michel.hautbois@veo-labs.com>

Fixed a 'checkpatch --strict' warning and applied to for-next, thanks!


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