lkml.org 
[lkml]   [2014]   [Dec]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH/RFC v9 01/19] leds: Add LED Flash class extension to the LED subsystem
Hi Sakari,

On 12/09/2014 01:36 PM, Sakari Ailus wrote:
> Hi Jacek,
>
> On Thu, Dec 04, 2014 at 10:29:10AM +0100, Jacek Anaszewski wrote:
> ...
>>>> +static struct attribute *led_flash_strobe_attrs[] = {
>>>> + &dev_attr_flash_strobe.attr,
>>>> + NULL,
>>>> +};
>>>> +
>>>> +static struct attribute *led_flash_timeout_attrs[] = {
>>>> + &dev_attr_flash_timeout.attr,
>>>> + &dev_attr_max_flash_timeout.attr,
>>>> + NULL,
>>>> +};
>>>> +
>>>> +static struct attribute *led_flash_brightness_attrs[] = {
>>>> + &dev_attr_flash_brightness.attr,
>>>> + &dev_attr_max_flash_brightness.attr,
>>>> + NULL,
>>>> +};
>>>> +
>>>> +static struct attribute *led_flash_fault_attrs[] = {
>>>> + &dev_attr_flash_fault.attr,
>>>> + NULL,
>>>> +};
>>>> +
>>>> +static struct attribute *led_flash_sync_strobe_attrs[] = {
>>>> + &dev_attr_flash_sync_strobe.attr,
>>>> + NULL,
>>>> +};
>>>> +
>>>> +static const struct attribute_group led_flash_strobe_group = {
>>>> + .attrs = led_flash_strobe_attrs,
>>>> +};
>>>> +
>>>> +static const struct attribute_group led_flash_timeout_group = {
>>>> + .attrs = led_flash_timeout_attrs,
>>>> +};
>>>> +
>>>> +static const struct attribute_group led_flash_brightness_group = {
>>>> + .attrs = led_flash_brightness_attrs,
>>>> +};
>>>> +
>>>> +static const struct attribute_group led_flash_fault_group = {
>>>> + .attrs = led_flash_fault_attrs,
>>>> +};
>>>> +
>>>> +static const struct attribute_group led_flash_sync_strobe_group = {
>>>> + .attrs = led_flash_sync_strobe_attrs,
>>>> +};
>>>> +
>>>> +static const struct attribute_group *flash_groups[] = {
>>>> + &led_flash_strobe_group,
>>>> + NULL,
>>>> + NULL,
>>>> + NULL,
>>>> + NULL,
>>>> + NULL,
>>>> + NULL
>>>> +};
>>>> +
>>>> +static void led_flash_resume(struct led_classdev *led_cdev)
>>>> +{
>>>> + struct led_classdev_flash *flash = lcdev_to_flash(led_cdev);
>>>> +
>>>> + call_flash_op(flash, flash_brightness_set, flash->brightness.val);
>>>> + call_flash_op(flash, timeout_set, flash->timeout.val);
>>>> +}
>>>> +
>>>> +static void led_flash_init_sysfs_groups(struct led_classdev_flash *flash)
>>>> +{
>>>> + struct led_classdev *led_cdev = &flash->led_cdev;
>>>> + const struct led_flash_ops *ops = flash->ops;
>>>> + int num_sysfs_groups = 1;
>>>> +
>>>> + if (ops->flash_brightness_set)
>>>> + flash_groups[num_sysfs_groups++] = &led_flash_brightness_group;
>>>> +
>>>> + if (ops->timeout_set)
>>>> + flash_groups[num_sysfs_groups++] = &led_flash_timeout_group;
>>>> +
>>>> + if (ops->fault_get)
>>>> + flash_groups[num_sysfs_groups++] = &led_flash_fault_group;
>>>> +
>>>> + if (led_cdev->flags & LED_DEV_CAP_COMPOUND)
>>>> + flash_groups[num_sysfs_groups++] = &led_flash_sync_strobe_group;
>>>> +
>>>> + led_cdev->groups = flash_groups;
>>>
>>> Shouldn't you have groups local to the device instead? If you register
>>> another flash device bad things will happen if the ops the device supports
>>> are different.
>>
>> The groups are local to the device. A LED class device is registered
>> with device_create_with_groups called from led_classdev_register
>> function. It is passed led_cdev->groups in the fifth argument.
>
> The groups pointer will be stored in struct device. If you have another
> driver using different groups, it will affect the groups for all flash
> devices that use the same groups pointer. I'm not sure what exactly would
> follow from that but I'd rather not change them once the device is created.

I had to take another look at this to understand the problem.
I think that the best option will be making flash_groups array
a member of struct led_classdev_flash.

>>>> +}
>>>> +
>>>> +int led_classdev_flash_register(struct device *parent,
>>>> + struct led_classdev_flash *flash)
>>>> +{
>>>> + struct led_classdev *led_cdev;
>>>> + const struct led_flash_ops *ops;
>>>> + int ret;
>>>> +
>>>> + if (!flash)
>>>
>>> Do you have a use case for this?
>>
>> This is just a guard against NULL pointer dereference. Maybe it is
>> indeed redundant, as the driver developer can easily check its
>> origin during implementation.
>
> Fine for me.

Fine regarding my explanation or you agree that it is redundant?

Best Regards,
Jacek Anaszewski



\
 
 \ /
  Last update: 2014-12-09 14:21    [W:0.193 / U:0.036 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site