Re: [PATCHv2 1/3] leds: Add of_led_get() and led_put()

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 2503 bytes --]


On 08/09/15 16:20, Jacek Anaszewski wrote:
> Hi Tomi,
> 
> Thanks for the update.
> 
> On 09/08/2015 01:19 PM, Tomi Valkeinen wrote:
>> This patch adds basic support for a kernel driver to get a LED device.
>> This will be used by the led-backlight driver.
>>
>> Only OF version is implemented for now, and the behavior is similar to
>> PWM's of_pwm_get() and pwm_put().
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>   drivers/leds/Makefile    |  6 +++-
>>   drivers/leds/led-class.c | 13 +++++++-
>>   drivers/leds/led-of.c    | 82
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/leds/leds.h      |  1 +
>>   include/linux/leds-of.h  | 26 +++++++++++++++
> 
> According to existing naming convention this should be "of_leds.h".

Right. I was thinking it's "leds" first, and "of" second, but I see
of_*.h is the convention.

>> +#include <linux/leds.h>
>> +#include <linux/of.h>
>> +#include <linux/leds-of.h>
> 
> Please keep alphabetical order.

Yep.

>> +#include <linux/module.h>
>> +
>> +#include "leds.h"
>> +
>> +/* find OF node for the given led_cdev */
>> +static struct device_node *find_led_of_node(struct led_classdev
>> *led_cdev)
>> +{
>> +    struct device *led_dev = led_cdev->dev;
>> +    struct device_node *child;
>> +
>> +    for_each_child_of_node(led_dev->parent->of_node, child) {
>> +        if (of_property_match_string(child, "label", led_cdev->name)
>> == 0)
> 
> Line over 80 characters.

I don't like to split lines to exact 80 chars, when it makes the code
more difficult to read. In this case it's 3 chars over 80, and splitting
the function call above to two lines doesn't look nice to me.

I'll do the func call separately, then it stays under 80 chars.

>> +            return child;
>> +    }
>> +
>> +    return NULL;
>> +}
>> +
>> +static int led_match_led_node(struct device *led_dev, const void *data)
>> +{
>> +    struct led_classdev *led_cdev = dev_get_drvdata(led_dev);
>> +    const struct device_node *target_node = data;
>> +    struct device_node *led_node;
>> +
>> +    led_node = find_led_of_node(led_cdev);
>> +    if (!led_node)
>> +        return 0;
>> +
>> +    of_node_put(led_node);
>> +
>> +    return led_node == target_node ? 1 : 0;
> 
> return led_node == target_node;

Again a matter of taste, but to me, == returns a bool, whereas the match
function here returns an int.

But I'm fine with plain == here.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]