* [PATCH 1/2] leds/tca6507: Add support for devicetree. @ 2012-10-30 21:45 Marek Belisko 2012-10-30 21:45 ` [PATCH 2/2] Add documentation for tca6507 devicetree bindings Marek Belisko 2012-11-06 18:24 ` [PATCH 1/2] leds/tca6507: Add support for devicetree Bryan Wu 0 siblings, 2 replies; 5+ messages in thread From: Marek Belisko @ 2012-10-30 21:45 UTC (permalink / raw) To: cooloney, rpurdie Cc: linux-leds, linux-kernel, devicetree-discuss, Marek Belisko Support added only for leds (not for gpio's). Signed-off-by: Marek Belisko <marek.belisko@open-nandra.com> --- drivers/leds/leds-tca6507.c | 73 +++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 70 insertions(+), 3 deletions(-) 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; + + if (count > NUM_LEDS) + return NULL; + + tca_leds = devm_kzalloc(&client->dev, sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL); + + if (!tca_leds) + return NULL; + + + 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", ®); + + 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); + 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 2/2] Add documentation for tca6507 devicetree bindings. 2012-10-30 21:45 [PATCH 1/2] leds/tca6507: Add support for devicetree Marek Belisko @ 2012-10-30 21:45 ` Marek Belisko 2012-11-06 18:24 ` [PATCH 1/2] leds/tca6507: Add support for devicetree Bryan Wu 1 sibling, 0 replies; 5+ messages in thread From: Marek Belisko @ 2012-10-30 21:45 UTC (permalink / raw) To: cooloney, rpurdie Cc: linux-leds, linux-kernel, devicetree-discuss, Marek Belisko Signed-off-by: Marek Belisko <marek.belisko@open-nandra.com> --- Documentation/devicetree/bindings/leds/tca6507.txt | 33 ++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/tca6507.txt diff --git a/Documentation/devicetree/bindings/leds/tca6507.txt b/Documentation/devicetree/bindings/leds/tca6507.txt new file mode 100644 index 0000000..cb0015b9f --- /dev/null +++ b/Documentation/devicetree/bindings/leds/tca6507.txt @@ -0,0 +1,33 @@ +LEDs conected to tca6507 + +Required properties: +- compatible : should be : "leds-tca6507". + +Each led is represented as a sub-node of the leds-tca6507 device. + +LED sub-node properties: +- label : label for this LED +- reg : number of LED line (could be from 0 to 6) +- linux,default-trigger : led default trigger + +Examples: + +tca6507@45 { + compatible = "leds-tca6507"; + #address-cells = <1>; + #size-cells = <0>; + reg = <0x45>; + + led0: red_aux@0 { + label = "red:aux"; + reg = <0x0>; + }; + + led1: green_aux@1 { + label = "green:aux"; + reg = <0x5>; + linux,default-trigger = "default-on"; + }; + +}; + -- 1.7.9.5 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] leds/tca6507: Add support for devicetree. 2012-10-30 21:45 [PATCH 1/2] leds/tca6507: Add support for devicetree Marek Belisko 2012-10-30 21:45 ` [PATCH 2/2] Add documentation for tca6507 devicetree bindings Marek Belisko @ 2012-11-06 18:24 ` Bryan Wu [not found] ` <CAAfyv36WJZro4DsOy=S7X5YLPw93M00JY95kc9EHKRqCSMcBbQ@mail.gmail.com> 2012-11-07 9:30 ` Jingoo Han 1 sibling, 2 replies; 5+ messages in thread From: Bryan Wu @ 2012-11-06 18:24 UTC (permalink / raw) To: Marek Belisko; +Cc: rpurdie, linux-leds, linux-kernel, devicetree-discuss On Tue, Oct 30, 2012 at 2:45 PM, Marek Belisko <marek.belisko@open-nandra.com> wrote: > Support added only for leds (not for gpio's). > > Signed-off-by: Marek Belisko <marek.belisko@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? > + 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", ®); > + > + 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. > + 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <CAAfyv36WJZro4DsOy=S7X5YLPw93M00JY95kc9EHKRqCSMcBbQ@mail.gmail.com>]
* Re: [PATCH 1/2] leds/tca6507: Add support for devicetree. [not found] ` <CAAfyv36WJZro4DsOy=S7X5YLPw93M00JY95kc9EHKRqCSMcBbQ@mail.gmail.com> @ 2012-11-06 23:27 ` Bryan Wu 0 siblings, 0 replies; 5+ messages in thread From: Bryan Wu @ 2012-11-06 23:27 UTC (permalink / raw) To: Belisko Marek; +Cc: Richard Purdie, linux-leds, LKML, devicetree-discuss On Tue, Nov 6, 2012 at 12:32 PM, Belisko Marek <marek.belisko@gmail.com> wrote: > Hi, > > On Tue, Nov 6, 2012 at 7:24 PM, Bryan Wu <cooloney@gmail.com> wrote: >> >> On Tue, Oct 30, 2012 at 2:45 PM, Marek Belisko >> <marek.belisko@open-nandra.com> wrote: >> > Support added only for leds (not for gpio's). >> > >> > Signed-off-by: Marek Belisko <marek.belisko@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. Oh, really. In my kernel tree, gpio_leds_create_of() function of drivers/leds/leds-gpio.c always returns something like this ERR_PTR(-ENODEV); I think return some meaningful things is useful than NULL. -Bryan >> >> >> > + 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", ®); >> > + >> > + 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/2] leds/tca6507: Add support for devicetree. 2012-11-06 18:24 ` [PATCH 1/2] leds/tca6507: Add support for devicetree Bryan Wu [not found] ` <CAAfyv36WJZro4DsOy=S7X5YLPw93M00JY95kc9EHKRqCSMcBbQ@mail.gmail.com> @ 2012-11-07 9:30 ` Jingoo Han 1 sibling, 0 replies; 5+ messages in thread From: Jingoo Han @ 2012-11-07 9:30 UTC (permalink / raw) To: 'Marek Belisko' Cc: 'Bryan Wu', rpurdie, linux-leds, linux-kernel, devicetree-discuss, 'Jingoo Han' On Wednesday, November 07, 2012 3:25 AM Bryan Wu wrote > > On Tue, Oct 30, 2012 at 2:45 PM, Marek Belisko > <marek.belisko@open-nandra.com> wrote: > > Support added only for leds (not for gpio's). > > > > Signed-off-by: Marek Belisko <marek.belisko@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? > > > + 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", ®); > > + > > + 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. If devm_kzalloc() fails, tca6507_probe() will return '-ENODEV'. In this case, kfree() is unnecessary in this case, when devm_kzalloc() is used. Moreover, even if kfree() is necessary, devm_kfree() should be used instead of kfree(). Marek Belisko, please refer to this document. : http://www.kernel.org/doc/htmldocs/device-drivers/API-devm-kzalloc.html Thank you. Best regards, Jingoo Han > > > + 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 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-leds" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-11-07 9:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-30 21:45 [PATCH 1/2] leds/tca6507: Add support for devicetree Marek Belisko
2012-10-30 21:45 ` [PATCH 2/2] Add documentation for tca6507 devicetree bindings Marek Belisko
2012-11-06 18:24 ` [PATCH 1/2] leds/tca6507: Add support for devicetree Bryan Wu
[not found] ` <CAAfyv36WJZro4DsOy=S7X5YLPw93M00JY95kc9EHKRqCSMcBbQ@mail.gmail.com>
2012-11-06 23:27 ` Bryan Wu
2012-11-07 9:30 ` Jingoo Han
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®