Support added only for leds (not for gpio's).
Signed-off-by: Marek Belisko <[email protected]>
---
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
Signed-off-by: Marek Belisko <[email protected]>
---
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
On Tue, Oct 30, 2012 at 2:45 PM, Marek Belisko
<[email protected]> wrote:
> Support added only for leds (not for gpio's).
>
> Signed-off-by: Marek Belisko <[email protected]>
> ---
> 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
>
On Tue, Nov 6, 2012 at 12:32 PM, Belisko Marek <[email protected]> wrote:
> Hi,
>
> On Tue, Nov 6, 2012 at 7:24 PM, Bryan Wu <[email protected]> wrote:
>>
>> On Tue, Oct 30, 2012 at 2:45 PM, Marek Belisko
>> <[email protected]> wrote:
>> > Support added only for leds (not for gpio's).
>> >
>> > Signed-off-by: Marek Belisko <[email protected]>
>> > ---
>> > 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
On Wednesday, November 07, 2012 3:25 AM Bryan Wu wrote
>
> On Tue, Oct 30, 2012 at 2:45 PM, Marek Belisko
> <[email protected]> wrote:
> > Support added only for leds (not for gpio's).
> >
> > Signed-off-by: Marek Belisko <[email protected]>
> > ---
> > 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html