Re: [PATCH 7/7] i2c: pxa: support to parse property

[Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>]

On Tue, Jul 19, 2011 at 06:17:21PM +0800, Eric Miao wrote:
> On Tue, Jul 19, 2011 at 10:24 AM, Haojian Zhuang
> <haojian.zhuang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org> wrote:
> > Support to parse some optional properties. These three properties are
> > i2c-polling, i2c-frequency, i2c-class.
> >
> > After supporting these property, i2c-pxa driver can avoid to use platform
> > data except for slave mode.
> >
> > Signed-off-by: Haojian Zhuang <haojian.zhuang-eYqpPyKDWXRBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  .../devicetree/bindings/i2c/pxa255-i2c.txt         |   36 ++++++++++++++++++++
> >  drivers/i2c/busses/i2c-pxa.c                       |   22 ++++++++----
> >  2 files changed, 51 insertions(+), 7 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/i2c/pxa255-i2c.txt
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/pxa255-i2c.txt b/Documentation/devicetree/bindings/i2c/pxa255-i2c.txt
> > new file mode 100644
> > index 0000000..bf34236
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/pxa255-i2c.txt
> > @@ -0,0 +1,36 @@
> > +PXA255 I2C
> > +
> > +The I2C-Controller is first used in PXA255. It's widely used in Intel/Marvell
> > +silicons.
> > +
> > +Optional Property:
> > +       - i2c-polling: Specifies whether I2C-Controller is used in polling
> > +         mode or interrupt mode. The type of property should be <u32>.
> > +
> > +       - i2c-frequency: Specifies the frequency that the I2C-Controller
> > +         is working. The type of property should be <string>.
> > +
> > +       - i2c-class: Specifies the class of I2C-Controller. The type of
> > +         property should be <u32>.
> > +
> > +Example:
> > +       i2c0: i2c@d4011000 {
> > +               compatible = "pxa2xx-i2c";
> > +               #address-cells = <1>;
> > +               #size-cells = <0>;
> > +               reg = <0xd4011000 0x60>;
> > +               /* I2C-Controller works in interrupt mode. */
> > +               i2c-polling = <0>;
> > +               /* I2C-Controller's frequency is FAST. */
> > +               i2c-frequency = "fast";
> > +               /* interrupt of I2C-Controller */
> > +               interrupts = <7>;
> > +               interrupt-parent = <&mmp_intc>;
> > +
> > +               pm860x: pmic@34 {
> > +                       interrupt-controller;
> > +                       /* interrupt of pm860x */
> > +                       interrupts = <4>;
> > +                       interrupt-parent = <&mmp_intc>;
> > +               };
> > +       };
> > diff --git a/drivers/i2c/busses/i2c-pxa.c b/drivers/i2c/busses/i2c-pxa.c
> > index adac74a..4b9fa71 100644
> > --- a/drivers/i2c/busses/i2c-pxa.c
> > +++ b/drivers/i2c/busses/i2c-pxa.c
> > @@ -1060,7 +1060,8 @@ static int i2c_pxa_probe(struct platform_device *dev)
> >        const struct platform_device_id *id = platform_get_device_id(dev);
> >        enum pxa_i2c_types i2c_type;
> >        struct resource *res;
> > -       int irq, ret;
> > +       int irq, ret, poll;
> > +       char *p = NULL;
> >        static int idx = 0;
> >
> >        if (np) {
> > @@ -1093,11 +1094,24 @@ static int i2c_pxa_probe(struct platform_device *dev)
> >
> >
> >        if (np) {
> > +               of_property_read_u32(np, "i2c-polling", &poll);
> > +               i2c->use_pio = (poll) ? 1 : 0;
> > +               of_property_read_string(np, "i2c-frequency", &p);
> > +               if (p && !strncmp(p, "fast", 4))
> > +                       i2c->fast_mode = 1;
> > +               of_property_read_u32(np, "i2c-class", &i2c->adap.class);
> > +
> >                i2c->adap.nr = idx++;
> >                snprintf(i2c->adap.name, sizeof(i2c->adap.name),
> >                        "pxa2xx-i2c.%u", i2c->adap.nr);
> >                i2c->clk = clk_get_sys(i2c->adap.name, NULL);
> >        } else {
> > +               if (plat) {
> > +                       i2c->adap.class = plat->class;
> > +                       i2c->use_pio = plat->use_pio;
> > +                       i2c->fast_mode = plat->fast_mode;
> > +               }
> > +
> 
> One concern of DT's impact to existing drivers is _every_ driver has to be
> modified. While I'm not sure if it's a right way to go in a long run. Or can
> we have a generic way to automatically map the DT properties to the
> specific platform data structure?

Not really, since there is no such thing as a generic platform_data
structure.  We can certainly have helper functions, but any
driver-specific data must by definition have driver-specific code to
decode it from the device tree.

It does help though when similar drivers use the same binding.

g.

> 
> Grant, any idea on this?
> 
> >                /*
> >                 * If "dev->id" is negative we consider it as zero.
> >                 * The reason to do so is to avoid sysfs names that only make
> > @@ -1142,12 +1156,6 @@ static int i2c_pxa_probe(struct platform_device *dev)
> >
> >        clk_enable(i2c->clk);
> >
> > -       if (plat) {
> > -               i2c->adap.class = plat->class;
> > -               i2c->use_pio = plat->use_pio;
> > -               i2c->fast_mode = plat->fast_mode;
> > -       }
> > -
> >        if (i2c->use_pio) {
> >                i2c->adap.algo = &i2c_pxa_pio_algorithm;
> >        } else {
> > --
> > 1.5.6.5
> >
> >