Re: [PATCH v6 2/4] leds: Add driver for Qualcomm LPG

[Bjorn Andersson]

On Thu 29 Apr 16:12 CDT 2021, Pavel Machek wrote:

> Hi!
> 
> > > > +static int lpg_add_pwm(struct lpg *lpg)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	lpg->pwm.base = -1;
> > > > +	lpg->pwm.dev = lpg->dev;
> > > > +	lpg->pwm.npwm = lpg->num_channels;
> > > > +	lpg->pwm.ops = &lpg_pwm_ops;
> > > > +
> > > > +	ret = pwmchip_add(&lpg->pwm);
> > > > +	if (ret)
> > > > +		dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret);
> > > > +
> > > > +	return ret;
> > > > +}
> > > 
> > > Do we need to do this? I'd rather have LED driver, than LED+PWM
> > > driver...
> > > 
> > 
> > Yes, I believe we need to do this.
> > 
> > Because each piece of hardware has N channels, which can be wired to
> > LEDs, grouped with other channels and wired to multicolor LEDs or be
> > used as PWM signals. And this configuration is board specific.
> > 
> > One such example is the laptop in front of me, which has 3 channels
> > wired to an RGB LED and 1 channel wired as a backlight control signal
> > (i.e. using pwm-backlight).  Another example is a devboard where the
> > 4 channels are wired to 4 LEDs.
> 
> Ok, so this is actually important. In this case you should have PWM
> layer, exporting PWMs, and then rgb-LED driver that takes three of
> those PWMs and turns them into LED, no?
> 
> And ... surprise ... that is likely to help other people, as LEDs
> connected to PWMs are quite common.
> 
> Hmm.?
> 
> If you can't do this for some reason, you should probably explain in
> the changelog, because this is going to be FAQ.
> 

This is exactly what the downstream implementation does and in the case
of a solid color LED this works fine.

But the hardware has a shared chunk of memory where you can write
duty-cycle values, then for each PWM channel you can specify the
start/stop index and pace for the PWM to read and update the configured
duty-cycle. This is how the hardware implements pattern support.


So downstream they have (last time I looked at the code) an addition in
the PWM API where the LED driver can inform the PWM driver part about
the indices to use. Naturally I don't think that's a good idea.


Additionally, representing this as individual PWM channels means we're
loosing the grouping that now comes from the description of multicolor
LEDs, which serves the basis for synchronizing the pattern traversal
between the involved channels.


I just posted v7, but I'd be happy to incorporate such reasoning in the
commit message (or do you really mean changelog in the email?) when we
agree on a solution.

Regards,
Bjorn