[PATCH RESEND] i2c: Add support for device-tree based chip initialization

[PATCH RESEND] i2c: Add support for device-tree based chip initialization

[Guenter Roeck]

Some I2C devices are not or not correctly initialized by the firmware.
Configuration would be possible via platform data, but that would require
per-driver platform data and a lot of code, and changing it would not be
possible without re-compiling the kernel. It is more elegant to do it
generically via devicetree properties.

Add a generic I2C devicetree property named "reg-init". This property provides
a sequence of device initialization commands to be executed prior to calling
the probe function for a given device.

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
Any comments / feedback ?

 .../devicetree/bindings/i2c/trivial-devices.txt    |   24 +++++++
 drivers/i2c/i2c-core.c                             |   68 ++++++++++++++++++++
 2 files changed, 92 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
index 2f5322b..33b694e 100644
--- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
@@ -2,6 +2,30 @@ This is a list of trivial i2c devices that have simple device tree
 bindings, consisting only of a compatible field, an address and
 possibly an interrupt line.
 
+Device initialization is supported with an optional reg-init property.
+This property contains a sequence of commands to be written into the chip.
+Each command consists of four values: Register, command type, mask, and data.
+	Register:
+		Register or command address
+	Command type:
+		0: SMBus write byte
+		1: SMBus write byte data
+		2: SMBus write word data
+	Mask:
+		If set, the register is read and masked with this value.
+	Data:
+		Data to be written (or with original data and mask if set)
+
+Example:
+	max6696@1a {
+		compatible = "maxim,max6696";
+		reg = <0x1a>;
+		reg-init = <
+			0x09 1 0x00 0x24
+			0x21 1 0x00 0x05
+		>;
+	};
+
 If a device needs more specific bindings, such as properties to
 describe some aspect of it, there needs to be a specific binding
 document for it just like any other devices.
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index a7edf98..49f8b74 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -104,6 +104,69 @@ static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 #define i2c_device_uevent	NULL
 #endif	/* CONFIG_HOTPLUG */
 
+static int i2c_dev_of_init(struct i2c_client *client, struct device *dev)
+{
+	const u32 *reg_init;
+	int rlen;
+	int val;
+	u32 reg, access, mask, data;
+
+	reg_init = of_get_property(dev->of_node, "reg-init", &rlen);
+	if (!reg_init)
+		return 0;
+
+	if (!rlen || rlen % 4)
+		return -EINVAL;
+
+	while (rlen >= 4) {
+		reg = reg_init[0];
+		access = reg_init[1];
+		mask = reg_init[2];
+		data = reg_init[3];
+
+		if (reg > 0xff)
+			return -EINVAL;
+
+		switch (access) {
+		default:
+			return -EINVAL;
+		case 0:
+			val = 0;
+			break;
+		case 1:
+			val = mask ? i2c_smbus_read_byte_data(client, reg) : 0;
+			break;
+		case 2:
+			val = mask ? i2c_smbus_read_word_data(client, reg) : 0;
+			break;
+		}
+		if (val < 0)
+			return val;
+
+		val &= mask;
+		val |= data;
+
+		switch (access) {
+		default:
+		case 0:
+			val = i2c_smbus_write_byte(client, reg);
+			break;
+		case 1:
+			val = i2c_smbus_write_byte_data(client, reg, val);
+			break;
+		case 2:
+			val = i2c_smbus_write_word_data(client, reg, val);
+			break;
+		}
+		if (val < 0)
+			return val;
+
+		reg_init += 4;
+		rlen -= 4 * sizeof(u32);
+	}
+	return 0;
+}
+
 static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
@@ -122,7 +185,12 @@ static int i2c_device_probe(struct device *dev)
 					client->flags & I2C_CLIENT_WAKE);
 	dev_dbg(dev, "probe\n");
 
+	status = i2c_dev_of_init(client, dev);
+	if (status)
+		goto error;
+
 	status = driver->probe(client, i2c_match_id(driver->id_table, client));
+error:
 	if (status) {
 		client->driver = NULL;
 		i2c_set_clientdata(client, NULL);
-- 
1.7.9.7

Re: [PATCH RESEND] i2c: Add support for device-tree based chip initialization

[Rob Herring]

On 11/25/2012 10:53 PM, Guenter Roeck wrote:
> Some I2C devices are not or not correctly initialized by the firmware.
> Configuration would be possible via platform data, but that would require
> per-driver platform data and a lot of code, and changing it would not be
> possible without re-compiling the kernel. It is more elegant to do it
> generically via devicetree properties.
> 
> Add a generic I2C devicetree property named "reg-init". This property provides
> a sequence of device initialization commands to be executed prior to calling
> the probe function for a given device.
> 
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> ---
> Any comments / feedback ?

This has been discussed before in terms of memory mapped registers. I
think this is of questionable use of DT and could easily be abused. Not
all systems use DT, so this needs to be solved without DT anyway.

Do you have examples of drivers that would use this?

Can this be handled in userspace using the i2c device interface before
loading the device's module?

Rob

> 
>  .../devicetree/bindings/i2c/trivial-devices.txt    |   24 +++++++
>  drivers/i2c/i2c-core.c                             |   68 ++++++++++++++++++++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> index 2f5322b..33b694e 100644
> --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> @@ -2,6 +2,30 @@ This is a list of trivial i2c devices that have simple device tree
>  bindings, consisting only of a compatible field, an address and
>  possibly an interrupt line.
>  
> +Device initialization is supported with an optional reg-init property.
> +This property contains a sequence of commands to be written into the chip.
> +Each command consists of four values: Register, command type, mask, and data.
> +	Register:
> +		Register or command address
> +	Command type:
> +		0: SMBus write byte
> +		1: SMBus write byte data
> +		2: SMBus write word data
> +	Mask:
> +		If set, the register is read and masked with this value.
> +	Data:
> +		Data to be written (or with original data and mask if set)
> +
> +Example:
> +	max6696@1a {
> +		compatible = "maxim,max6696";
> +		reg = <0x1a>;
> +		reg-init = <
> +			0x09 1 0x00 0x24
> +			0x21 1 0x00 0x05
> +		>;
> +	};
> +
>  If a device needs more specific bindings, such as properties to
>  describe some aspect of it, there needs to be a specific binding
>  document for it just like any other devices.
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index a7edf98..49f8b74 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -104,6 +104,69 @@ static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
>  #define i2c_device_uevent	NULL
>  #endif	/* CONFIG_HOTPLUG */
>  
> +static int i2c_dev_of_init(struct i2c_client *client, struct device *dev)
> +{
> +	const u32 *reg_init;
> +	int rlen;
> +	int val;
> +	u32 reg, access, mask, data;
> +
> +	reg_init = of_get_property(dev->of_node, "reg-init", &rlen);
> +	if (!reg_init)
> +		return 0;
> +
> +	if (!rlen || rlen % 4)
> +		return -EINVAL;
> +
> +	while (rlen >= 4) {
> +		reg = reg_init[0];
> +		access = reg_init[1];
> +		mask = reg_init[2];
> +		data = reg_init[3];
> +
> +		if (reg > 0xff)
> +			return -EINVAL;
> +
> +		switch (access) {
> +		default:
> +			return -EINVAL;
> +		case 0:
> +			val = 0;
> +			break;
> +		case 1:
> +			val = mask ? i2c_smbus_read_byte_data(client, reg) : 0;
> +			break;
> +		case 2:
> +			val = mask ? i2c_smbus_read_word_data(client, reg) : 0;
> +			break;
> +		}
> +		if (val < 0)
> +			return val;
> +
> +		val &= mask;
> +		val |= data;
> +
> +		switch (access) {
> +		default:
> +		case 0:
> +			val = i2c_smbus_write_byte(client, reg);
> +			break;
> +		case 1:
> +			val = i2c_smbus_write_byte_data(client, reg, val);
> +			break;
> +		case 2:
> +			val = i2c_smbus_write_word_data(client, reg, val);
> +			break;
> +		}
> +		if (val < 0)
> +			return val;
> +
> +		reg_init += 4;
> +		rlen -= 4 * sizeof(u32);
> +	}
> +	return 0;
> +}
> +
>  static int i2c_device_probe(struct device *dev)
>  {
>  	struct i2c_client	*client = i2c_verify_client(dev);
> @@ -122,7 +185,12 @@ static int i2c_device_probe(struct device *dev)
>  					client->flags & I2C_CLIENT_WAKE);
>  	dev_dbg(dev, "probe\n");
>  
> +	status = i2c_dev_of_init(client, dev);
> +	if (status)
> +		goto error;
> +
>  	status = driver->probe(client, i2c_match_id(driver->id_table, client));
> +error:
>  	if (status) {
>  		client->driver = NULL;
>  		i2c_set_clientdata(client, NULL);
> 

Re: [PATCH RESEND] i2c: Add support for device-tree based chip initialization

[Grant Likely]

On Sun, 25 Nov 2012 20:53:56 -0800, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> Some I2C devices are not or not correctly initialized by the firmware.
> Configuration would be possible via platform data, but that would require
> per-driver platform data and a lot of code, and changing it would not be
> possible without re-compiling the kernel. It is more elegant to do it
> generically via devicetree properties.
> 
> Add a generic I2C devicetree property named "reg-init". This property provides
> a sequence of device initialization commands to be executed prior to calling
> the probe function for a given device.
> 
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

This is a very similar problem to the power sequences series that was
recently posted to this list. I would like that feature and this
mechanism use the same approach.

g.

Re: [PATCH RESEND] i2c: Add support for device-tree based chip initialization

[Guenter Roeck]

On Mon, Nov 26, 2012 at 10:43:38AM -0600, Rob Herring wrote:
> On 11/25/2012 10:53 PM, Guenter Roeck wrote:
> > Some I2C devices are not or not correctly initialized by the firmware.
> > Configuration would be possible via platform data, but that would require
> > per-driver platform data and a lot of code, and changing it would not be
> > possible without re-compiling the kernel. It is more elegant to do it
> > generically via devicetree properties.
> > 
> > Add a generic I2C devicetree property named "reg-init". This property provides
> > a sequence of device initialization commands to be executed prior to calling
> > the probe function for a given device.
> > 
> > Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > ---
> > Any comments / feedback ?
> 
> This has been discussed before in terms of memory mapped registers. I
> think this is of questionable use of DT and could easily be abused. Not

Isn't that true for pretty much everything ?

Really, I don't think that "it can be abused" should be considered a valid
argument. It is almost as good (or bad) as "we have always done it that way"
or "this doesn't scale".

> all systems use DT, so this needs to be solved without DT anyway.
> 
> Do you have examples of drivers that would use this?
> 
I would need it for MAX6697 (for which I submitted a driver a week or so ago),
and possibly for others. I didn't check further because I don't want to go along
on this road too far if the idea is rejected.

I took the idea from Broadcom and Marvell PHY chip initialization, which uses
a similar approach, including the reg-init keyword. As far as I can see
both don't support platform data based initialization, so one question
to ask would be when it is mandatory to support platforma data based
initialization and when it isn't.

> Can this be handled in userspace using the i2c device interface before
> loading the device's module?
> 
Not in my use case.

Thanks,
Guenter

> Rob
> 
> > 
> >  .../devicetree/bindings/i2c/trivial-devices.txt    |   24 +++++++
> >  drivers/i2c/i2c-core.c                             |   68 ++++++++++++++++++++
> >  2 files changed, 92 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> > index 2f5322b..33b694e 100644
> > --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> > +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
> > @@ -2,6 +2,30 @@ This is a list of trivial i2c devices that have simple device tree
> >  bindings, consisting only of a compatible field, an address and
> >  possibly an interrupt line.
> >  
> > +Device initialization is supported with an optional reg-init property.
> > +This property contains a sequence of commands to be written into the chip.
> > +Each command consists of four values: Register, command type, mask, and data.
> > +	Register:
> > +		Register or command address
> > +	Command type:
> > +		0: SMBus write byte
> > +		1: SMBus write byte data
> > +		2: SMBus write word data
> > +	Mask:
> > +		If set, the register is read and masked with this value.
> > +	Data:
> > +		Data to be written (or with original data and mask if set)
> > +
> > +Example:
> > +	max6696@1a {
> > +		compatible = "maxim,max6696";
> > +		reg = <0x1a>;
> > +		reg-init = <
> > +			0x09 1 0x00 0x24
> > +			0x21 1 0x00 0x05
> > +		>;
> > +	};
> > +
> >  If a device needs more specific bindings, such as properties to
> >  describe some aspect of it, there needs to be a specific binding
> >  document for it just like any other devices.
> > diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> > index a7edf98..49f8b74 100644
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -104,6 +104,69 @@ static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
> >  #define i2c_device_uevent	NULL
> >  #endif	/* CONFIG_HOTPLUG */
> >  
> > +static int i2c_dev_of_init(struct i2c_client *client, struct device *dev)
> > +{
> > +	const u32 *reg_init;
> > +	int rlen;
> > +	int val;
> > +	u32 reg, access, mask, data;
> > +
> > +	reg_init = of_get_property(dev->of_node, "reg-init", &rlen);
> > +	if (!reg_init)
> > +		return 0;
> > +
> > +	if (!rlen || rlen % 4)
> > +		return -EINVAL;
> > +
> > +	while (rlen >= 4) {
> > +		reg = reg_init[0];
> > +		access = reg_init[1];
> > +		mask = reg_init[2];
> > +		data = reg_init[3];
> > +
> > +		if (reg > 0xff)
> > +			return -EINVAL;
> > +
> > +		switch (access) {
> > +		default:
> > +			return -EINVAL;
> > +		case 0:
> > +			val = 0;
> > +			break;
> > +		case 1:
> > +			val = mask ? i2c_smbus_read_byte_data(client, reg) : 0;
> > +			break;
> > +		case 2:
> > +			val = mask ? i2c_smbus_read_word_data(client, reg) : 0;
> > +			break;
> > +		}
> > +		if (val < 0)
> > +			return val;
> > +
> > +		val &= mask;
> > +		val |= data;
> > +
> > +		switch (access) {
> > +		default:
> > +		case 0:
> > +			val = i2c_smbus_write_byte(client, reg);
> > +			break;
> > +		case 1:
> > +			val = i2c_smbus_write_byte_data(client, reg, val);
> > +			break;
> > +		case 2:
> > +			val = i2c_smbus_write_word_data(client, reg, val);
> > +			break;
> > +		}
> > +		if (val < 0)
> > +			return val;
> > +
> > +		reg_init += 4;
> > +		rlen -= 4 * sizeof(u32);
> > +	}
> > +	return 0;
> > +}
> > +
> >  static int i2c_device_probe(struct device *dev)
> >  {
> >  	struct i2c_client	*client = i2c_verify_client(dev);
> > @@ -122,7 +185,12 @@ static int i2c_device_probe(struct device *dev)
> >  					client->flags & I2C_CLIENT_WAKE);
> >  	dev_dbg(dev, "probe\n");
> >  
> > +	status = i2c_dev_of_init(client, dev);
> > +	if (status)
> > +		goto error;
> > +
> >  	status = driver->probe(client, i2c_match_id(driver->id_table, client));
> > +error:
> >  	if (status) {
> >  		client->driver = NULL;
> >  		i2c_set_clientdata(client, NULL);
> > 
> 
> 

Re: [PATCH RESEND] i2c: Add support for device-tree based chip initialization

[Rob Herring]

On 11/26/2012 02:19 PM, Guenter Roeck wrote:
> On Mon, Nov 26, 2012 at 10:43:38AM -0600, Rob Herring wrote:
>> On 11/25/2012 10:53 PM, Guenter Roeck wrote:
>>> Some I2C devices are not or not correctly initialized by the firmware.
>>> Configuration would be possible via platform data, but that would require
>>> per-driver platform data and a lot of code, and changing it would not be
>>> possible without re-compiling the kernel. It is more elegant to do it
>>> generically via devicetree properties.
>>>
>>> Add a generic I2C devicetree property named "reg-init". This property provides
>>> a sequence of device initialization commands to be executed prior to calling
>>> the probe function for a given device.
>>>
>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>> ---
>>> Any comments / feedback ?
>>
>> This has been discussed before in terms of memory mapped registers. I
>> think this is of questionable use of DT and could easily be abused. Not
> 
> Isn't that true for pretty much everything ?
> 
> Really, I don't think that "it can be abused" should be considered a valid
> argument. It is almost as good (or bad) as "we have always done it that way"
> or "this doesn't scale".

DT is a description of the h/w. It is not a h/w configuration mechanism.
Although, you do lots of h/w configuration based on the h/w description.
That being said, you can find examples in bindings that are just
configuration data so it's not a clear line.

If we do want to support this, then there is no reason for it to be
specific to i2c devices or ethernet phys. It was on the pinmux/pinctrl
binding discussions the last time this came up IIRC. The first issue
will be the need to specify register sizes. Then you get into needing
masked writes. Then you need delays and polling reads of register
values. Then the discussion dies when it starts to look like a scripting
language and Forth is mentioned (happened just last week with runtime
interpreted power sequences thread).

Rob

>> all systems use DT, so this needs to be solved without DT anyway.
>>
>> Do you have examples of drivers that would use this?
>>
> I would need it for MAX6697 (for which I submitted a driver a week or so ago),
> and possibly for others. I didn't check further because I don't want to go along
> on this road too far if the idea is rejected.
> 
> I took the idea from Broadcom and Marvell PHY chip initialization, which uses
> a similar approach, including the reg-init keyword. As far as I can see
> both don't support platform data based initialization, so one question
> to ask would be when it is mandatory to support platforma data based
> initialization and when it isn't.
>
>> Can this be handled in userspace using the i2c device interface before
>> loading the device's module?
>>
> Not in my use case.
> 
> Thanks,
> Guenter
> 
>> Rob
>>
>>>
>>>  .../devicetree/bindings/i2c/trivial-devices.txt    |   24 +++++++
>>>  drivers/i2c/i2c-core.c                             |   68 ++++++++++++++++++++
>>>  2 files changed, 92 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
>>> index 2f5322b..33b694e 100644
>>> --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
>>> +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
>>> @@ -2,6 +2,30 @@ This is a list of trivial i2c devices that have simple device tree
>>>  bindings, consisting only of a compatible field, an address and
>>>  possibly an interrupt line.
>>>  
>>> +Device initialization is supported with an optional reg-init property.
>>> +This property contains a sequence of commands to be written into the chip.
>>> +Each command consists of four values: Register, command type, mask, and data.
>>> +	Register:
>>> +		Register or command address
>>> +	Command type:
>>> +		0: SMBus write byte
>>> +		1: SMBus write byte data
>>> +		2: SMBus write word data
>>> +	Mask:
>>> +		If set, the register is read and masked with this value.
>>> +	Data:
>>> +		Data to be written (or with original data and mask if set)
>>> +
>>> +Example:
>>> +	max6696@1a {
>>> +		compatible = "maxim,max6696";
>>> +		reg = <0x1a>;
>>> +		reg-init = <
>>> +			0x09 1 0x00 0x24
>>> +			0x21 1 0x00 0x05
>>> +		>;
>>> +	};
>>> +
>>>  If a device needs more specific bindings, such as properties to
>>>  describe some aspect of it, there needs to be a specific binding
>>>  document for it just like any other devices.
>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>>> index a7edf98..49f8b74 100644
>>> --- a/drivers/i2c/i2c-core.c
>>> +++ b/drivers/i2c/i2c-core.c
>>> @@ -104,6 +104,69 @@ static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
>>>  #define i2c_device_uevent	NULL
>>>  #endif	/* CONFIG_HOTPLUG */
>>>  
>>> +static int i2c_dev_of_init(struct i2c_client *client, struct device *dev)
>>> +{
>>> +	const u32 *reg_init;
>>> +	int rlen;
>>> +	int val;
>>> +	u32 reg, access, mask, data;
>>> +
>>> +	reg_init = of_get_property(dev->of_node, "reg-init", &rlen);
>>> +	if (!reg_init)
>>> +		return 0;
>>> +
>>> +	if (!rlen || rlen % 4)
>>> +		return -EINVAL;
>>> +
>>> +	while (rlen >= 4) {
>>> +		reg = reg_init[0];
>>> +		access = reg_init[1];
>>> +		mask = reg_init[2];
>>> +		data = reg_init[3];
>>> +
>>> +		if (reg > 0xff)
>>> +			return -EINVAL;
>>> +
>>> +		switch (access) {
>>> +		default:
>>> +			return -EINVAL;
>>> +		case 0:
>>> +			val = 0;
>>> +			break;
>>> +		case 1:
>>> +			val = mask ? i2c_smbus_read_byte_data(client, reg) : 0;
>>> +			break;
>>> +		case 2:
>>> +			val = mask ? i2c_smbus_read_word_data(client, reg) : 0;
>>> +			break;
>>> +		}
>>> +		if (val < 0)
>>> +			return val;
>>> +
>>> +		val &= mask;
>>> +		val |= data;
>>> +
>>> +		switch (access) {
>>> +		default:
>>> +		case 0:
>>> +			val = i2c_smbus_write_byte(client, reg);
>>> +			break;
>>> +		case 1:
>>> +			val = i2c_smbus_write_byte_data(client, reg, val);
>>> +			break;
>>> +		case 2:
>>> +			val = i2c_smbus_write_word_data(client, reg, val);
>>> +			break;
>>> +		}
>>> +		if (val < 0)
>>> +			return val;
>>> +
>>> +		reg_init += 4;
>>> +		rlen -= 4 * sizeof(u32);
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>>  static int i2c_device_probe(struct device *dev)
>>>  {
>>>  	struct i2c_client	*client = i2c_verify_client(dev);
>>> @@ -122,7 +185,12 @@ static int i2c_device_probe(struct device *dev)
>>>  					client->flags & I2C_CLIENT_WAKE);
>>>  	dev_dbg(dev, "probe\n");
>>>  
>>> +	status = i2c_dev_of_init(client, dev);
>>> +	if (status)
>>> +		goto error;
>>> +
>>>  	status = driver->probe(client, i2c_match_id(driver->id_table, client));
>>> +error:
>>>  	if (status) {
>>>  		client->driver = NULL;
>>>  		i2c_set_clientdata(client, NULL);
>>>
>>
>>

Re: [PATCH RESEND] i2c: Add support for device-tree based chip initialization

[Mitch Bradley]

On 11/26/2012 12:37 PM, Rob Herring wrote:
> On 11/26/2012 02:19 PM, Guenter Roeck wrote:
>> On Mon, Nov 26, 2012 at 10:43:38AM -0600, Rob Herring wrote:
>>> On 11/25/2012 10:53 PM, Guenter Roeck wrote:
>>>> Some I2C devices are not or not correctly initialized by the firmware.
>>>> Configuration would be possible via platform data, but that would require
>>>> per-driver platform data and a lot of code, and changing it would not be
>>>> possible without re-compiling the kernel. It is more elegant to do it
>>>> generically via devicetree properties.
>>>>
>>>> Add a generic I2C devicetree property named "reg-init". This property provides
>>>> a sequence of device initialization commands to be executed prior to calling
>>>> the probe function for a given device.
>>>>
>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>>> ---
>>>> Any comments / feedback ?
>>>
>>> This has been discussed before in terms of memory mapped registers. I
>>> think this is of questionable use of DT and could easily be abused. Not
>>
>> Isn't that true for pretty much everything ?
>>
>> Really, I don't think that "it can be abused" should be considered a valid
>> argument. It is almost as good (or bad) as "we have always done it that way"
>> or "this doesn't scale".
> 
> DT is a description of the h/w. It is not a h/w configuration mechanism.

And yet, the document that defined the device tree is entitled "IEEE
Standard for Boot (Initialization, Configuration) Firmware".
Initialization and configuration was part of the problem set, and part
of the solution set, from the outset.

> Although, you do lots of h/w configuration based on the h/w description.
> That being said, you can find examples in bindings that are just
> configuration data so it's not a clear line.
> 
> If we do want to support this, then there is no reason for it to be
> specific to i2c devices or ethernet phys. It was on the pinmux/pinctrl
> binding discussions the last time this came up IIRC. The first issue
> will be the need to specify register sizes. Then you get into needing
> masked writes. Then you need delays and polling reads of register
> values. Then the discussion dies when it starts to look like a scripting
> language and Forth is mentioned (happened just last week with runtime
> interpreted power sequences thread).

It would be nice if hardware were simple enough not to need the power of
a programming language to initialize it, or if hardware variants did not
proliferate to the point where building new kernels became unattractive.
 The world doesn't appear to be going in that "nice" direction...

The longer that one resists doing the right thing, the more likely that
one ends up saddled with a bad half-solution that has to be extended
into a truly awful hodgepodge.

> 
> Rob
> 
>>> all systems use DT, so this needs to be solved without DT anyway.
>>>
>>> Do you have examples of drivers that would use this?
>>>
>> I would need it for MAX6697 (for which I submitted a driver a week or so ago),
>> and possibly for others. I didn't check further because I don't want to go along
>> on this road too far if the idea is rejected.
>>
>> I took the idea from Broadcom and Marvell PHY chip initialization, which uses
>> a similar approach, including the reg-init keyword. As far as I can see
>> both don't support platform data based initialization, so one question
>> to ask would be when it is mandatory to support platforma data based
>> initialization and when it isn't.
>>
>>> Can this be handled in userspace using the i2c device interface before
>>> loading the device's module?
>>>
>> Not in my use case.
>>
>> Thanks,
>> Guenter
>>
>>> Rob
>>>
>>>>
>>>>  .../devicetree/bindings/i2c/trivial-devices.txt    |   24 +++++++
>>>>  drivers/i2c/i2c-core.c                             |   68 ++++++++++++++++++++
>>>>  2 files changed, 92 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
>>>> index 2f5322b..33b694e 100644
>>>> --- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
>>>> +++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
>>>> @@ -2,6 +2,30 @@ This is a list of trivial i2c devices that have simple device tree
>>>>  bindings, consisting only of a compatible field, an address and
>>>>  possibly an interrupt line.
>>>>  
>>>> +Device initialization is supported with an optional reg-init property.
>>>> +This property contains a sequence of commands to be written into the chip.
>>>> +Each command consists of four values: Register, command type, mask, and data.
>>>> +	Register:
>>>> +		Register or command address
>>>> +	Command type:
>>>> +		0: SMBus write byte
>>>> +		1: SMBus write byte data
>>>> +		2: SMBus write word data
>>>> +	Mask:
>>>> +		If set, the register is read and masked with this value.
>>>> +	Data:
>>>> +		Data to be written (or with original data and mask if set)
>>>> +
>>>> +Example:
>>>> +	max6696@1a {
>>>> +		compatible = "maxim,max6696";
>>>> +		reg = <0x1a>;
>>>> +		reg-init = <
>>>> +			0x09 1 0x00 0x24
>>>> +			0x21 1 0x00 0x05
>>>> +		>;
>>>> +	};
>>>> +
>>>>  If a device needs more specific bindings, such as properties to
>>>>  describe some aspect of it, there needs to be a specific binding
>>>>  document for it just like any other devices.
>>>> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
>>>> index a7edf98..49f8b74 100644
>>>> --- a/drivers/i2c/i2c-core.c
>>>> +++ b/drivers/i2c/i2c-core.c
>>>> @@ -104,6 +104,69 @@ static int i2c_device_uevent(struct device *dev, struct kobj_uevent_env *env)
>>>>  #define i2c_device_uevent	NULL
>>>>  #endif	/* CONFIG_HOTPLUG */
>>>>  
>>>> +static int i2c_dev_of_init(struct i2c_client *client, struct device *dev)
>>>> +{
>>>> +	const u32 *reg_init;
>>>> +	int rlen;
>>>> +	int val;
>>>> +	u32 reg, access, mask, data;
>>>> +
>>>> +	reg_init = of_get_property(dev->of_node, "reg-init", &rlen);
>>>> +	if (!reg_init)
>>>> +		return 0;
>>>> +
>>>> +	if (!rlen || rlen % 4)
>>>> +		return -EINVAL;
>>>> +
>>>> +	while (rlen >= 4) {
>>>> +		reg = reg_init[0];
>>>> +		access = reg_init[1];
>>>> +		mask = reg_init[2];
>>>> +		data = reg_init[3];
>>>> +
>>>> +		if (reg > 0xff)
>>>> +			return -EINVAL;
>>>> +
>>>> +		switch (access) {
>>>> +		default:
>>>> +			return -EINVAL;
>>>> +		case 0:
>>>> +			val = 0;
>>>> +			break;
>>>> +		case 1:
>>>> +			val = mask ? i2c_smbus_read_byte_data(client, reg) : 0;
>>>> +			break;
>>>> +		case 2:
>>>> +			val = mask ? i2c_smbus_read_word_data(client, reg) : 0;
>>>> +			break;
>>>> +		}
>>>> +		if (val < 0)
>>>> +			return val;
>>>> +
>>>> +		val &= mask;
>>>> +		val |= data;
>>>> +
>>>> +		switch (access) {
>>>> +		default:
>>>> +		case 0:
>>>> +			val = i2c_smbus_write_byte(client, reg);
>>>> +			break;
>>>> +		case 1:
>>>> +			val = i2c_smbus_write_byte_data(client, reg, val);
>>>> +			break;
>>>> +		case 2:
>>>> +			val = i2c_smbus_write_word_data(client, reg, val);
>>>> +			break;
>>>> +		}
>>>> +		if (val < 0)
>>>> +			return val;
>>>> +
>>>> +		reg_init += 4;
>>>> +		rlen -= 4 * sizeof(u32);
>>>> +	}
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  static int i2c_device_probe(struct device *dev)
>>>>  {
>>>>  	struct i2c_client	*client = i2c_verify_client(dev);
>>>> @@ -122,7 +185,12 @@ static int i2c_device_probe(struct device *dev)
>>>>  					client->flags & I2C_CLIENT_WAKE);
>>>>  	dev_dbg(dev, "probe\n");
>>>>  
>>>> +	status = i2c_dev_of_init(client, dev);
>>>> +	if (status)
>>>> +		goto error;
>>>> +
>>>>  	status = driver->probe(client, i2c_match_id(driver->id_table, client));
>>>> +error:
>>>>  	if (status) {
>>>>  		client->driver = NULL;
>>>>  		i2c_set_clientdata(client, NULL);
>>>>
>>>
>>>
> 
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
> 

Re: [PATCH RESEND] i2c: Add support for device-tree based chip initialization

[David Daney]

On 11/26/2012 12:13 PM, Grant Likely wrote:
> On Sun, 25 Nov 2012 20:53:56 -0800, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
>> Some I2C devices are not or not correctly initialized by the firmware.
>> Configuration would be possible via platform data, but that would require
>> per-driver platform data and a lot of code, and changing it would not be
>> possible without re-compiling the kernel. It is more elegant to do it
>> generically via devicetree properties.
>>
>> Add a generic I2C devicetree property named "reg-init". This property provides
>> a sequence of device initialization commands to be executed prior to calling
>> the probe function for a given device.
>>
>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>
> This is a very similar problem to the power sequences series that was
> recently posted to this list. I would like that feature and this
> mechanism use the same approach.


I haven't fully studied the power sequencing thing, but just to play 
Devil's Advocate, there is precedence for the "reg-init" style in some 
of the Ethernet PHY drivers.

Perhaps the "reg-init" should be used as one of the steps in the 
sequence, and if there was only a single step it would be functionally 
equivalent to the "reg-init" proposal.


David Daney


>
> g.
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
>

Re: [PATCH RESEND] i2c: Add support for device-tree based chip initialization

[Guenter Roeck]

On Mon, Nov 26, 2012 at 04:26:39PM -0800, David Daney wrote:
> On 11/26/2012 12:13 PM, Grant Likely wrote:
> >On Sun, 25 Nov 2012 20:53:56 -0800, Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org> wrote:
> >>Some I2C devices are not or not correctly initialized by the firmware.
> >>Configuration would be possible via platform data, but that would require
> >>per-driver platform data and a lot of code, and changing it would not be
> >>possible without re-compiling the kernel. It is more elegant to do it
> >>generically via devicetree properties.
> >>
> >>Add a generic I2C devicetree property named "reg-init". This property provides
> >>a sequence of device initialization commands to be executed prior to calling
> >>the probe function for a given device.
> >>
> >>Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> >
> >This is a very similar problem to the power sequences series that was
> >recently posted to this list. I would like that feature and this
> >mechanism use the same approach.
> 
> 
> I haven't fully studied the power sequencing thing, but just to play
> Devil's Advocate, there is precedence for the "reg-init" style in
> some of the Ethernet PHY drivers.
> 
That is where I took it from.

> Perhaps the "reg-init" should be used as one of the steps in the
> sequence, and if there was only a single step it would be
> functionally equivalent to the "reg-init" proposal.
> 
My thought is along the same line. I'll have to study the power sequencing
proposal a bit more to see if that would work.

Guenter

Re: [PATCH RESEND] i2c: Add support for device-tree based chip initialization

[Guenter Roeck]

On Mon, Nov 26, 2012 at 12:58:38PM -1000, Mitch Bradley wrote:
> On 11/26/2012 12:37 PM, Rob Herring wrote:
> > On 11/26/2012 02:19 PM, Guenter Roeck wrote:
> >> On Mon, Nov 26, 2012 at 10:43:38AM -0600, Rob Herring wrote:
> >>> On 11/25/2012 10:53 PM, Guenter Roeck wrote:
> >>>> Some I2C devices are not or not correctly initialized by the firmware.
> >>>> Configuration would be possible via platform data, but that would require
> >>>> per-driver platform data and a lot of code, and changing it would not be
> >>>> possible without re-compiling the kernel. It is more elegant to do it
> >>>> generically via devicetree properties.
> >>>>
> >>>> Add a generic I2C devicetree property named "reg-init". This property provides
> >>>> a sequence of device initialization commands to be executed prior to calling
> >>>> the probe function for a given device.
> >>>>
> >>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> >>>> ---
> >>>> Any comments / feedback ?
> >>>
> >>> This has been discussed before in terms of memory mapped registers. I
> >>> think this is of questionable use of DT and could easily be abused. Not
> >>
> >> Isn't that true for pretty much everything ?
> >>
> >> Really, I don't think that "it can be abused" should be considered a valid
> >> argument. It is almost as good (or bad) as "we have always done it that way"
> >> or "this doesn't scale".
> > 
> > DT is a description of the h/w. It is not a h/w configuration mechanism.
> 
> And yet, the document that defined the device tree is entitled "IEEE
> Standard for Boot (Initialization, Configuration) Firmware".
> Initialization and configuration was part of the problem set, and part
> of the solution set, from the outset.
> 
Many of the dt properties could be removed if it were only a HW description,
and it would be quite worthless. One of the arguments for dt was that it can be
used to replace platform data, which is widely used to provide configuration
data.


> > Although, you do lots of h/w configuration based on the h/w description.
> > That being said, you can find examples in bindings that are just
> > configuration data so it's not a clear line.
> > 
> > If we do want to support this, then there is no reason for it to be
> > specific to i2c devices or ethernet phys. It was on the pinmux/pinctrl
> > binding discussions the last time this came up IIRC. The first issue
> > will be the need to specify register sizes. Then you get into needing
> > masked writes. Then you need delays and polling reads of register
> > values. Then the discussion dies when it starts to look like a scripting
> > language and Forth is mentioned (happened just last week with runtime
> > interpreted power sequences thread).
> 
> It would be nice if hardware were simple enough not to need the power of
> a programming language to initialize it, or if hardware variants did not
> proliferate to the point where building new kernels became unattractive.
>  The world doesn't appear to be going in that "nice" direction...
> 
I didn't plan to solve all problems in the world - for i2c I'd be happy with 90%
or even 80%. That would help me to avoid having to write explicit code for those 80%.

> The longer that one resists doing the right thing, the more likely that
> one ends up saddled with a bad half-solution that has to be extended
> into a truly awful hodgepodge.
> 
At least, as David has pointed out, there is already a precedence for reg-init.

Personally I prefer a working solution for existing problems now over a
solution which is supposed to solve all problems in the world ... until the next
problem comes up and it turns out that the solution doesn't work for it. Which,
in my experience, happens all the time if one tries to come up with solutions
for future problems.

Guenter