* [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; 6+ 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 related [flat|nested] 6+ 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; 6+ 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 related [flat|nested] 6+ 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] ` <CAK5ve-+fE7T3U9KZ-C5-sZN9PUPq6RybCu+nv9UDb2x0bnarzw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-11-07 9:30 ` Jingoo Han
1 sibling, 2 replies; 6+ 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] 6+ messages in thread
* Re: [PATCH 1/2] leds/tca6507: Add support for devicetree.
[not found] ` <CAK5ve-+fE7T3U9KZ-C5-sZN9PUPq6RybCu+nv9UDb2x0bnarzw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2012-11-06 20:32 ` Belisko Marek
2012-11-06 23:27 ` Bryan Wu
0 siblings, 1 reply; 6+ messages in thread
From: Belisko Marek @ 2012-11-06 20:32 UTC (permalink / raw)
To: Bryan Wu
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Richard Purdie, LKML,
linux-leds-u79uwXL29TY76Z2rM5mHXA
[-- Attachment #1.1: Type: text/plain, Size: 5173 bytes --]
Hi,
On Tue, Nov 6, 2012 at 7:24 PM, Bryan Wu <cooloney-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Tue, Oct 30, 2012 at 2:45 PM, Marek Belisko
> <marek.belisko-e6Ot0yJZRx5zTq7rFmK8cA@public.gmane.org> wrote:
> > Support added only for leds (not for gpio's).
> >
> > Signed-off-by: Marek Belisko <marek.belisko-e6Ot0yJZRx5zTq7rFmK8cA@public.gmane.org>
> > ---
> > 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", ®);
> > +
> > + 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
[-- Attachment #1.2: Type: text/html, Size: 7519 bytes --]
[-- Attachment #2: Type: text/plain, Size: 192 bytes --]
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] leds/tca6507: Add support for devicetree.
2012-11-06 20:32 ` Belisko Marek
@ 2012-11-06 23:27 ` Bryan Wu
0 siblings, 0 replies; 6+ 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] 6+ 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] ` <CAK5ve-+fE7T3U9KZ-C5-sZN9PUPq6RybCu+nv9UDb2x0bnarzw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2012-11-07 9:30 ` Jingoo Han
1 sibling, 0 replies; 6+ 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] 6+ messages in thread
end of thread, other threads:[~2012-11-07 9:30 UTC | newest]
Thread overview: 6+ 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] ` <CAK5ve-+fE7T3U9KZ-C5-sZN9PUPq6RybCu+nv9UDb2x0bnarzw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2012-11-06 20:32 ` Belisko Marek
2012-11-06 23:27 ` Bryan Wu
2012-11-07 9:30 ` Jingoo Han
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.