Re: [PATCH 3/4] leds: add LM3533 LED driver

On Fri, Apr 20, 2012 at 04:10:15PM +0000, Arnd Bergmann wrote:

On Friday 20 April 2012, Johan Hovold wrote:

quoted

Add sub-driver for the LEDs in National Semiconductor / TI LM3533
lighting power chips.

The chip provides 256 brightness levels, hardware accelerated blinking
as well as ambient-light-sensor and pwm input control.

Signed-off-by: Johan Hovold <redacted>

I notice that there is already driver for lm3530, which sounds related.
Is there an opportunity to share code between these, or are they completely
different devices?

Unfortunately not. They are really very different devices despite
similar naming and terminology.

quoted

+
+#define show_ctrlbank_attr(_name)					\
+static ssize_t show_##_name(struct device *dev,				\
+				struct device_attribute *attr,		\
+				char *buf)				\
+{									\
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);		\
+	struct lm3533_led *led = to_lm3533_led(led_cdev);		\
+	u8 val;								\
+	int ret;							\
+									\
+	ret = lm3533_ctrlbank_get_##_name(&led->cb, &val);		\
+	if (ret)							\
+		return ret;						\
+									\
+	return scnprintf(buf, PAGE_SIZE, "%d\n", val);			\
+}

IMHO this macro adds more in terms of complexity than it saves in terms
of lines of code, and it would be better to open-code the two instances.
If you need more than two or three instances, I would recommend creating
keying the number off of the attribute pointer, either by comparing the
pointer or by adding a data structure derived from device_attribute and
using container_of to get at the other data.

Agreed. I'll simply open-code them for now, and do the same with the
equivalent instances in the backlight driver.

Thanks,
Johan