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 <[email protected]> >> --- >> 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
signature.asc
Description: OpenPGP digital signature
