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@xxxxxxxxxxxxxx) > > 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? > 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. Anyhow, not a hard requirement to split from my side. > > > +/* > > > + * 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. > > > +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(). > > > +} Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
