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

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Thu 03 Feb 00:31 PST 2022, Uwe Kleine-K?nig wrote:

> Hello Bjorn,
> 
> On Wed, Feb 02, 2022 at 01:40:21PM -0800, Bjorn Andersson wrote:
> > On Wed 02 Feb 08:29 PST 2022, Uwe Kleine-K?nig wrote:
> > > did you consider my earlier feedback "It would also be good if the PWM
> > > code could live in drivers/pwm"?
> > > (https://lore.kernel.org/r/20210505051958.e5lvwfxuo2skdu2q@pengutronix.de)
> > 
> > Yes, I did consider this. Because the downstream driver is (at least was
> > when I looked at it originally) split like that.
> > 
> > We have a number of different Qualcomm PMICs containing the LPG modules,
> > which consists of N PWM channels, a pattern lookup table and a set of
> > current sinks.
> > 
> > Each PWM channel can either be used as a traditional PWM, a LED or be
> > grouped together with other channels to form a multicolor LED. So we
> > need a design that allows different boards using a particular PMIC to
> > freely use the N channels according to one of these three operational
> > modes.
> > 
> > The pattern lookup table is a shared resource containing duty cycle
> > values and each of the PWM channels can be configured to have their duty
> > cycle modified from the lookup table on some configured cadence.
> > 
> > In the even that multiple PWM channels are ganged together to form a
> > multicolor LED, which is driven by a pattern, the pattern generator for
> > the relevant channels needs to be synchronized.
> 
> Is this some material for the commit log to motivate the design
> decision?
> 

Sounds reasonable. I'll try to capture some of this background in the
commit message, for future reference.

> > If we consider the PWM channel to be the basic primitive we need some
> > mechanism to configure the pattern properties for each of the channels
> > and we need some mechanism to synchronize the pattern generators for
> > some subset of the PWM channels.
> > 
> > 
> > In other words we need some custom API between the LED driver part and
> > the PWM driver, to configure these properties. This was the design
> > of the downstream driver when I started looking at this driver.
> > 
> > 
> > Another alternative that has been considered is to create two
> > independent drivers - for the same hardware. This would allow the system
> > integrator to pick the right driver for each of the channels.
> > 
> > One problem with this strategy is that the DeviceTree description of the
> > LPG hardware will have to be modified depending on the use case. In
> > particular this prevents me from writing a platform dtsi describing the
> > LPG hardware and then describe the LEDs and pwm channels in a board dts.
> > 
> > And we can't express the individual channels, because the multicolor
> > definition needs to span multiple channels.
> > 
> > 
> > So among all the options, implementing the pwm_chip in the LED driver
> > makes it possible for us to describe the LPG as one entity, with
> > board-specific LEDs and a set of PWM channels.
> 
> ok.
> 
> > > At least splitting in two patches would be good IMHO.
> > 
> > I guess I can split out the parts related to the pwmchip in a separate
> > patch. Seems to be a rather small portion of the code though. Is that
> > what you have in mind?
> 
> I didn't try to understand the pattern part. I know that for PWMs there
> is no pattern support, wasn't aware it's a thing for LEDs.
> 

It allow you to have the hardware adjust the duty cycle over time, to
implement things such as fading or pulsing light effects - without
having to use the CPU.

> Anyhow, not a hard requirement to split from my side.
> 

Okay, cool.

> > > > +/*
> > > > + * Limitations:
> > > > + * - Updating both duty and period is not done atomically, so the output signal
> > > > + *   will momentarily be a mix of the settings.
> > > 
> > > Is the PWM well-behaved? (i.e. does it emit the inactive level when
> > > disabled?)
> > 
> > Yes, a disabled channel outputs a logical 0.
> 
> Please add this to the Limitations section. It's not actually a
> limitation, but still this is a good place to put this information.
> 
> > > Does it complete a period before switching to the new
> > > setting?
> > 
> > I see nothing indicating the answer to this, in either direction...
> 
> Can you test that? It's as easy as configuring a long period with 0%
> relative duty cycle and then immediately a 100% relative duty cycle.
> 

I will give it a try and see what I can deduce, and update the comment
accordingly.

> > > > +static void lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > +			      struct pwm_state *state)
> > > > +{
> > > > +	struct lpg *lpg = container_of(chip, struct lpg, pwm);
> > > > +	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
> > > > +	u64 duty = DIV_ROUND_UP_ULL(chan->pwm_value * chan->period, LPG_RESOLUTION - 1);
> > > > +
> > > > +	state->period = chan->period;
> > > > +	state->duty_cycle = duty;
> > > > +	state->polarity = PWM_POLARITY_NORMAL;
> > > > +	state->enabled = chan->enabled;
> > > 
> > > This doesn't work if .get_state() is called before .apply() was called,
> > > does it?
> > > 
> > 
> > You mean that I would return some bogus state and not the actual
> > hardware state?
> 
> Yes. At least I only found lpg_calc_freq() assigning chan->period and
> chan->enabled. And unless I missed something this isn't called before
> the pwm core calls .get_state().
> 

Right, with a freshly probed driver I would return period = 0,
duty_cycle = 0/511 and enabled = 0.

Am I expected to instead return the hardware state, e.g. to recover
hardware initialization provided by the bootloader?

Thanks,
Bjorn

> > > > +}
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |