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@xxxxxx>
>> ---
>> 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:
signature.asc
Description: OpenPGP digital signature
