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@xxxxxxxxxxxx>
> > ---
> > 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);
> >
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
