[PATCH 1/2] leds/tca6507: Add support for devicetree.

[Belisko Marek]

Hi,

On Tue, Nov 6, 2012 at 7:24 PM, Bryan Wu <cooloney at gmail.com> wrote:

> On Tue, Oct 30, 2012 at 2:45 PM, Marek Belisko
> <marek.belisko at open-nandra.com> wrote:
> > Support added only for leds (not for gpio's).
> >
> > Signed-off-by: Marek Belisko <marek.belisko at open-nandra.com>
> > ---
> >  drivers/leds/leds-tca6507.c |   73
> +++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 70 insertions(+), 3 deletions(-)
> >
>
> Overall, this looks good to me. Maybe some question as below,
>
> -Bryan
>
> > diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c
> > index dabcf7a..496dd98 100644
> > --- a/drivers/leds/leds-tca6507.c
> > +++ b/drivers/leds/leds-tca6507.c
> > @@ -667,6 +667,69 @@ static void tca6507_remove_gpio(struct tca6507_chip
> *tca)
> >  }
> >  #endif /* CONFIG_GPIOLIB */
> >
> > +#ifdef CONFIG_OF
> > +static struct tca6507_platform_data * __devinit
> tca6507_led_dt_init(struct i2c_client *client)
> > +{
> > +       struct device_node *np = client->dev.of_node, *child;
> > +       struct tca6507_platform_data *pdata;
> > +       struct led_info *tca_leds;
> > +       int count = 0;
> > +
> > +       for_each_child_of_node(np, child)
> > +               count++;
> > +       if (!count)
> > +               return NULL;
> > +
>
> I saw many 'return NULL' here, why not return some error code in ERR_PTR?
>
Well this function is only only used to correctly parse DT and set proper
platform data which was previously passed from board file. I think we don't
need to return (and check) specific error code just return NULL and use
some general advice to user that something is wrong. Same way it's done in
leds-gpio in gpio_leds_create_of() function.

>
> > +       if (count > NUM_LEDS)
> > +               return NULL;
> > +
> > +       tca_leds = devm_kzalloc(&client->dev, sizeof(struct led_info) *
> NUM_LEDS, GFP_KERNEL);
> > +
>
> useless empty line.
>
> > +       if (!tca_leds)
> > +               return NULL;
> > +
> > +
>
> useless empty line.
>
> > +       for_each_child_of_node(np, child) {
> > +               struct led_info led;
> > +               u32 reg;
> > +               int ret;
> > +
> > +               led.name = of_get_property(child, "label", NULL) ? :
> child->name;
> > +               led.default_trigger =
> > +                       of_get_property(child, "linux,default-trigger",
> NULL);
> > +
> > +               ret = of_property_read_u32(child, "reg", &reg);
> > +
> > +               if (ret != 0)
> > +                       continue;
> > +               tca_leds[reg] = led;
> > +       }
> > +       pdata = devm_kzalloc(&client->dev, sizeof(struct
> tca6507_platform_data), GFP_KERNEL);
> > +       if (!pdata) {
> > +               kfree(tca_leds);
>
> Do we need to kfree here? I think devm_zalloc() will take care of this
> if the driver failed to register.
>
You're right. I'll fix comments are send updated version of patch. Are you
OK with binding documentation patch?

>
> > +               return NULL;
> > +       }
> > +
> > +       pdata->leds.leds = tca_leds;
> > +       pdata->leds.num_leds = NUM_LEDS;
> > +
> > +       return pdata;
> > +}
> > +
> > +static const struct of_device_id of_tca6507_leds_match[] = {
> > +       { .compatible = "leds-tca6507", },
> > +       {},
> > +};
> > +
> > +#else
> > +static int __devinit tca6507_led_dt_init(struct i2c_client *client,
> struct tca6507_platform_data *data)
> > +{
> > +       return -1;
> > +}
> > +
> > +#define of_tca6507_leds_match NULL
> > +#endif
> > +
> >  static int __devinit tca6507_probe(struct i2c_client *client,
> >                                    const struct i2c_device_id *id)
> >  {
> > @@ -683,9 +746,12 @@ static int __devinit tca6507_probe(struct
> i2c_client *client,
> >                 return -EIO;
> >
> >         if (!pdata || pdata->leds.num_leds != NUM_LEDS) {
> > -               dev_err(&client->dev, "Need %d entries in platform-data
> list\n",
> > -                       NUM_LEDS);
> > -               return -ENODEV;
> > +               pdata = tca6507_led_dt_init(client);
> > +               if (!pdata) {
> > +                       dev_err(&client->dev, "Need %d entries in
> platform-data list\n",
> > +                               NUM_LEDS);
> > +                       return -ENODEV;
> > +               }
> >         }
> >         tca = devm_kzalloc(&client->dev, sizeof(*tca), GFP_KERNEL);
> >         if (!tca)
> > @@ -750,6 +816,7 @@ static struct i2c_driver tca6507_driver = {
> >         .driver   = {
> >                 .name    = "leds-tca6507",
> >                 .owner   = THIS_MODULE,
> > +               .of_match_table = of_tca6507_leds_match,
> >         },
> >         .probe    = tca6507_probe,
> >         .remove   = __devexit_p(tca6507_remove),
> > --
> > 1.7.9.5
> >
>

mbe

-- 
as simple and primitive as possible
-------------------------------------------------
Marek Belisko - OPEN-NANDRA
Freelance Developer

Ruska Nova Ves 219 | Presov, 08005 Slovak Republic
Tel: +421 915 052 184
skype: marekwhite
twitter: #opennandra
web: http://open-nandra.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/devicetree-discuss/attachments/20121106/5cfd8ddf/attachment.html>