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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
