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

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

On Thu, May 03, 2012 at 01:50:59PM +0200, Johan Hovold wrote:
> On Thu, May 03, 2012 at 11:43:44AM +0100, Mark Brown wrote:

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

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

> 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?  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.

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

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

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

> 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).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]