Date: Tue, 9 Dec 2014 14:36:08 +0200
From: Sakari Ailus <sakari.ailus@iki.fi>
To: Jacek Anaszewski <j.anaszewski@samsung.com>
Cc: linux-leds@vger.kernel.org, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org, kyungmin.park@samsung.com,
        b.zolnierkie@samsung.com, pavel@ucw.cz, cooloney@gmail.com,
        rpurdie@rpsys.net, s.nawrocki@samsung.com, robh+dt@kernel.org,
        pawel.moll@arm.com, mark.rutland@arm.com,
        ijc+devicetree@hellion.org.uk, galak@codeaurora.org
Subject: Re: [PATCH/RFC v9 01/19] leds: Add LED Flash class extension to the
 LED subsystem
Message-ID: <20141209123607.GI15559@valkosipuli.retiisi.org.uk>
References: <1417622814-10845-1-git-send-email-j.anaszewski@samsung.com>
 <1417622814-10845-2-git-send-email-j.anaszewski@samsung.com>
 <20141203165013.GM14746@valkosipuli.retiisi.org.uk>
 <54802966.8060405@samsung.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <54802966.8060405@samsung.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org

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.

> >>+}
> >>+
> >>+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.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/