Re: [PATCH v3 2/7] pwm: axi-pwmgen: report the waveform step

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 9 Nov 2026 16:06:07 +0200
Nicolae-Daniel Deaconescu <Nicolae-daniel.Deaconescu@analog.com> wrote:

> Hi Jonathan,
> 
> On Sat, 7 Nov 2026 01:47:55 +0000, Jonathan Cameron wrote:
> >  
> > > index b40522f01002..6b15244efabf 100644
> > > --- a/drivers/pwm/pwm-axi-pwmgen.c
> > > +++ b/drivers/pwm/pwm-axi-pwmgen.c
> > > @@ -136,6 +136,18 @@ static int axi_pwmgen_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_de
> > >  	return 0;
> > >  }
> > > +static int axi_pwmgen_get_waveform_step(struct pwm_chip *chip,
> > > +					struct pwm_device *pwm,
> > > +					struct pwm_waveform_step *step)
> > > +{
> > > +	struct axi_pwmgen_ddata *ddata = axi_pwmgen_ddata_from_chip(chip);
> > > +
> > > +	step->clk_rate_Hz = ddata->clk_rate_hz;
> > > +	step->clk_cycles_per_step = 1;  
> >
> > Given the only known user sets cycles_per_step to 1, what evidence
> > do we have that part of the interface is actually useful?
> >
> > I can theorise that some device might need a couple of clocks
> > but can also theorise someone might use both edges of the clock as possible
> > state transition points for a PWM.
> >
> > Note my PWM knowledge is minimal so this may be entirely garbage!  
> 
> None yet. axi-pwmgen is the only provider and reports 1. The field is there
> because Conor's v1 review said the code would fail if someone used a PWM
> provider with a mandatory divider. It keeps the consumer from assuming
> one clock cycle per step.
> 
> If you prefer to add it when a provider needs it, I can drop it. struct
> pwm_waveform_step then keeps only clk_rate_Hz.

I'm fine with sticking with this as you have it.  Thanks for the extra info.

Jonathan

> 
> Thanks,
> Daniel