Re: [RFC PATCH] pwm: TLC591xx PWM driver

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

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


On 18/08/15 18:09, Andrew Lunn wrote:
>> On the other hand, there's pwm_bl.c which give us backlight device
>> with PWM,
> 
> Lets look at this. A backlight device seems to do most of its work in
> the update_status callback. It is given a brightness in
> bl->props.brightness, which takes a value between 0 and
> props.max_brightness.
> 
> What pwm_bl.c does it then turn this brightness value into an
> artificial PWM configuration. Your proposed PWM driver then turns this
> back into a brightness, since you don't actually implement the period
> part of the PWM interface.

Hmm, I'm not sure I follow. It's still PWM, even if the period is fixed.
It is programming the PWM of TLC591xx.

> From an architecture point of view, doesn't an LED class device, which
> takes a brightness value, seem much more naturally?

It does the same thing, takes a brightness value, and programs TLC's PWM
for particular PWM duty cycle.

I guess I get your point. You're saying that as TLC has a fixed period,
we can just consider it as a brightness value.

But in the end, it is still PWM in the HW level, and also, the
brightness isn't linear, or even anything like it. At least on my
backlight, the visible values range from ~230 to 255.

So at least in this case I think it makes more sense to consider the
value as PWM duty cycle, which then causes the LED to glow at some
brightness.

If the value would be brightness, I'd presume that more or less the
whole 0-255 range would be usable, and 128 would be somewhere near
mid-brightness.

> It seems like implementing a generic led_bl.c driver would make sense.
> It would also allow some of the code in drivers/video/backlight/ to be
> eliminated. There seems to be both an LED class driver for lp55xx and
> a blacklight driver lp855x_bl.c. There are duplicate lp8788, 88pm860x,
> adp5520, da903x, da9052, hp6xx, and lm3533 drivers which might all be
> removed if a led_bl.c generic driver existed.

Yes, I think this should be looked at. I'll probably have a look at some
point if I find time.

>> and a GPIO over PWM sounds more sane to me than GPIO over LED.
> 
> Currently two LED class drivers are calling gpiochip_add:
> 
> ~/linux/drivers/leds$ grep gpiochip_add *.c
> leds-pca9532.c:	      	   err = gpiochip_add(&data->gpio);
> leds-tca6507.c:		   err = gpiochip_add(&tca->gpio);
> 
> The pca9532 has full GPIO capabilities, in as well as out. But it
> seems like tca6507 is output only. The TLC59108/TLC59116 is also
> output only. So a generic GPO driver on top of LED would make sense
> for these two, and save some code/bugs.
> 
> From a stand back, lets take a look at the architecture point of view,
> generic led_bl and gpio-led drivers seem to make sense.

Yes, led_bl makes sense. I don't think all LEDs are PWM/GPIO driven, so
in the minimum those LEDs might need a LED class driver.

For this particular chip I still think PWM driver would do just fine.
But as the LED framework offers features like blinking, and it's
possible to implement backlight and gpios on top LED (I hope), I do
agree that we should go with your driver.

That said, I feel that these frameworks, GPIO, PWM and LED are somewhat
overlapping. Not really my area of expertise, but I imagine a single
framework containing all those functionalities could be possible.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]