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

On Thu, May 03, 2012 at 03:51:08PM +0100, Mark Brown wrote:

On Thu, May 03, 2012 at 01:50:59PM +0200, Johan Hovold wrote:

quoted

On Thu, May 03, 2012 at 11:43:44AM +0100, Mark Brown wrote:

quoted

quoted

quoted

+		5 - 4.194 s
+		6 - 8.389 s
+		7 - 16.78 s

quoted

quoted

Shouldn't these be controlled by led_blink_set() rather than a custom
ABI?

quoted

led_blink_set controls the on/off times, but the LM3533 has the two
additional rise and fall-time settings which determine the transition
time between these states.

Hrm.  In that case these rise times are very large - I'd expect them to
cause issues with led_set_blink() users?

They are. The default settings (as fast a transition as possible) will
probably what most people use, and if they start fiddling with the
transition times they probably know what they're doing.

Though actually I suspect the
solution here is to pull these out into the framework later; we can
probably simulate reasonably in software with a lot of brightness
variable LEDs.

Ok.

quoted

quoted

quoted

+What:		/sys/class/leds/<led>/max_current

quoted

quoted

Shouldn't this be set by platform data, the maximum current you can push
through the LEDs seems like a board dependant thing which won't change
dynamically at runtime.  The brightness can already be varied.

quoted

I fully agree and it is possible to set via the platform data for that
reason. The end-customer, however, insisted that even this setting be
available through sysfs to facilitate their integration and testing.

quoted

I'd be willing drop this attribute if requested, as it would only be used
during integration and could easily be added back by the end-customer if
needed.

I'd strongly suggest removing this for mainline.  If it's present it
should at least be limited to the maximum specified in platform data
(just for safety if nothing else).

Agreed.

Thanks,
Johan