Hi,<div class="gmail_extra"><br><div class="gmail_quote">On Tue, Nov 6, 2012 at 7:24 PM, Bryan Wu <span dir="ltr"><<a href="mailto:cooloney@gmail.com" target="_blank">cooloney@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im">On Tue, Oct 30, 2012 at 2:45 PM, Marek Belisko<br>
<<a href="mailto:marek.belisko@open-nandra.com">marek.belisko@open-nandra.com</a>> wrote:<br>
> Support added only for leds (not for gpio's).<br>
><br>
> Signed-off-by: Marek Belisko <<a href="mailto:marek.belisko@open-nandra.com">marek.belisko@open-nandra.com</a>><br>
> ---<br>
> drivers/leds/leds-tca6507.c | 73 +++++++++++++++++++++++++++++++++++++++++--<br>
> 1 file changed, 70 insertions(+), 3 deletions(-)<br>
><br>
<br>
</div>Overall, this looks good to me. Maybe some question as below,<br>
<br>
-Bryan<br>
<div class="im"><br>
> diff --git a/drivers/leds/leds-tca6507.c b/drivers/leds/leds-tca6507.c<br>
> index dabcf7a..496dd98 100644<br>
> --- a/drivers/leds/leds-tca6507.c<br>
> +++ b/drivers/leds/leds-tca6507.c<br>
> @@ -667,6 +667,69 @@ static void tca6507_remove_gpio(struct tca6507_chip *tca)<br>
> }<br>
> #endif /* CONFIG_GPIOLIB */<br>
><br>
> +#ifdef CONFIG_OF<br>
> +static struct tca6507_platform_data * __devinit tca6507_led_dt_init(struct i2c_client *client)<br>
> +{<br>
> + struct device_node *np = client->dev.of_node, *child;<br>
> + struct tca6507_platform_data *pdata;<br>
> + struct led_info *tca_leds;<br>
> + int count = 0;<br>
> +<br>
> + for_each_child_of_node(np, child)<br>
> + count++;<br>
> + if (!count)<br>
> + return NULL;<br>
> +<br>
<br>
</div>I saw many 'return NULL' here, why not return some error code in ERR_PTR?<br></blockquote><div>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.</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="im"><br>
> + if (count > NUM_LEDS)<br>
> + return NULL;<br>
> +<br>
> + tca_leds = devm_kzalloc(&client->dev, sizeof(struct led_info) * NUM_LEDS, GFP_KERNEL);<br>
> +<br>
<br>
</div>useless empty line.<br>
<div class="im"><br>
> + if (!tca_leds)<br>
> + return NULL;<br>
> +<br>
> +<br>
<br>
</div>useless empty line.<br>
<div class="im"><br>
> + for_each_child_of_node(np, child) {<br>
> + struct led_info led;<br>
> + u32 reg;<br>
> + int ret;<br>
> +<br>
> + <a href="http://led.name" target="_blank">led.name</a> = of_get_property(child, "label", NULL) ? : child->name;<br>
> + led.default_trigger =<br>
> + of_get_property(child, "linux,default-trigger", NULL);<br>
> +<br>
> + ret = of_property_read_u32(child, "reg", ®);<br>
> +<br>
> + if (ret != 0)<br>
> + continue;<br>
> + tca_leds[reg] = led;<br>
> + }<br>
> + pdata = devm_kzalloc(&client->dev, sizeof(struct tca6507_platform_data), GFP_KERNEL);<br>
> + if (!pdata) {<br>
> + kfree(tca_leds);<br>
<br>
</div>Do we need to kfree here? I think devm_zalloc() will take care of this<br>
if the driver failed to register.<br></blockquote><div>You're right. I'll fix comments are send updated version of patch. Are you OK with binding documentation patch? </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><div class="h5"><br>
> + return NULL;<br>
> + }<br>
> +<br>
> + pdata->leds.leds = tca_leds;<br>
> + pdata->leds.num_leds = NUM_LEDS;<br>
> +<br>
> + return pdata;<br>
> +}<br>
> +<br>
> +static const struct of_device_id of_tca6507_leds_match[] = {<br>
> + { .compatible = "leds-tca6507", },<br>
> + {},<br>
> +};<br>
> +<br>
> +#else<br>
> +static int __devinit tca6507_led_dt_init(struct i2c_client *client, struct tca6507_platform_data *data)<br>
> +{<br>
> + return -1;<br>
> +}<br>
> +<br>
> +#define of_tca6507_leds_match NULL<br>
> +#endif<br>
> +<br>
> static int __devinit tca6507_probe(struct i2c_client *client,<br>
> const struct i2c_device_id *id)<br>
> {<br>
> @@ -683,9 +746,12 @@ static int __devinit tca6507_probe(struct i2c_client *client,<br>
> return -EIO;<br>
><br>
> if (!pdata || pdata->leds.num_leds != NUM_LEDS) {<br>
> - dev_err(&client->dev, "Need %d entries in platform-data list\n",<br>
> - NUM_LEDS);<br>
> - return -ENODEV;<br>
> + pdata = tca6507_led_dt_init(client);<br>
> + if (!pdata) {<br>
> + dev_err(&client->dev, "Need %d entries in platform-data list\n",<br>
> + NUM_LEDS);<br>
> + return -ENODEV;<br>
> + }<br>
> }<br>
> tca = devm_kzalloc(&client->dev, sizeof(*tca), GFP_KERNEL);<br>
> if (!tca)<br>
> @@ -750,6 +816,7 @@ static struct i2c_driver tca6507_driver = {<br>
> .driver = {<br>
> .name = "leds-tca6507",<br>
> .owner = THIS_MODULE,<br>
> + .of_match_table = of_tca6507_leds_match,<br>
> },<br>
> .probe = tca6507_probe,<br>
> .remove = __devexit_p(tca6507_remove),<br>
> --<br>
> 1.7.9.5<br>
><br>
</div></div></blockquote></div><br>mbe<br clear="all"><div><br></div>-- <br>as simple and primitive as possible<br>-------------------------------------------------<br>Marek Belisko - OPEN-NANDRA<br>Freelance Developer<br>
<br>Ruska Nova Ves 219 | Presov, 08005 Slovak Republic<br>Tel: +421 915 052 184<br>skype: marekwhite<br>twitter: #opennandra<br>web: <a href="http://open-nandra.com" target="_blank">http://open-nandra.com</a><br>
</div>