Re: [PATCH 2/2] pwm: Add driver for AXI PWM generator

[Uwe Kleine-König]

Hello Trevor,

On Wed, Jan 17, 2024 at 07:51:26AM -0500, Trevor Gamblin wrote:
> On 2024-01-15 16:18, Uwe Kleine-König wrote:
> > On Mon, Jan 15, 2024 at 03:12:21PM -0500, Trevor Gamblin wrote:
> > > +#define AXI_PWMGEN_TEST_DATA		0x5A0F0081
> > Is this a documented constant, or just a random (as in xkcd#221) value?
> 
> This is just a random (as in xkcd#221) value to write to the scratch
> register.

Then I suggest to add a comment telling that.

> > > +	period_cnt = DIV_ROUND_UP_ULL(state->period * clk_rate_hz, NSEC_PER_SEC);
> > The multiplication might overflow. Please use mul_u64_u64_div_u64() (or
> > one of its variant) and error out on clk_rate_hz > NSEC_PER_SEC.
> > 
> > Also round-up is wrong. I would expect that enabling PWM_DEBUG and
> > enough testing should tell you that. .apply is supposed to implement the
> > biggest period not bigger than the requested one. So you have to round
> > down here.
> To be clear, I should use mul_u64_u64_div_u64 only, or should I round down
> afterwards with another function as well?

mul_u64_u64_div_u64() already rounds down.

> > > +	if (period_cnt > UINT_MAX)
> > > +		return -EINVAL;
> > That's wrong. Please continue with period_cnd = UINT_MAX here.
> > 
> > Instead you should probably error out on period_cnt == 0.
> > 
> > > +	pwm->ch_period[ch] = period_cnt;
> > > +	pwm->ch_enabled[ch] = state->enabled;
> > > +	ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), state->enabled ? period_cnt : 0);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	duty_cnt = DIV_ROUND_UP_ULL(state->duty_cycle * clk_rate_hz, NSEC_PER_SEC);
> > > +	ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), duty_cnt);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return regmap_write(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_LOAD_CONFIG);
> > I assume this means that the writes above are to shadow registers and on
> > this write they are latched into the hardware. So there is no glitch?!
> > 
> > Does this wait for the currently running period to complete before
> > switching to the new configuration?
> > 
> > Please document these two hardware properties in a "Limitations"
> > paragraph at the top of the driver. See other drivers for the format.
> 
> The registers are shadow registers and changes don't take effect right away.
> It also keeps the phase offset in sync between outputs.

I don't understand that. This only makes sense if the different PWMs
share the same period length, doesn't it?

Please add this to the Limitations paragraph, too.

> > > +static int axi_pwmgen_get_state(struct pwm_chip *chip, struct pwm_device *device,
> > > +				struct pwm_state *state)
> > > +{
> > > +	struct axi_pwmgen *pwm = to_axi_pwmgen(chip);
> > > +	unsigned long clk_rate_hz = clk_get_rate(pwm->clk);
> > > +	struct regmap *regmap = pwm->regmap;
> > > +	unsigned int ch = device->hwpwm;
> > > +	u32 cnt;
> > > +	int ret;
> > > +
> > > +	if (!clk_rate_hz) {
> > > +		dev_err(device->chip->dev, "axi pwm clock has no frequency\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	state->enabled = pwm->ch_enabled[ch];
> > > +
> > > +	if (state->enabled) {
> > > +		ret = regmap_read(regmap, AXI_PWMGEN_CHX_PERIOD(ch), &cnt);
> > > +		if (ret)
> > > +			return ret;
> > > +	} else {
> > > +		cnt = pwm->ch_period[ch];
> > > +	}
> > If state->enabled is false, state->period is (or should) be ignored by
> > the caller, so there shouldn't be a need to track ch_period.
> > 
> > Also ch_enabled shouldn't be needed. Just reporting
> > AXI_PWMGEN_CHX_PERIOD(ch) == 0 as disabled should work fine?!
> > 
> > I think then you also don't need to artificially limit npwm to four.
> The only concern I have with not tracking ch_period is that it might not be
> clear what period to restore after disabling and re-enabling the device,
> unless I'm missing something.

A consumer that reenables the device should use pwm_apply_* which gets
the complete state that should be configured. (And for the few other
cases, this is solved in the core.)

> > > +	pwm->clk = devm_clk_get_enabled(&pdev->dev, NULL);
> > > +	if (IS_ERR(pwm->clk))
> > > +		return dev_err_probe(&pdev->dev, PTR_ERR(pwm->clk), "failed to get clock\n");
> > Please call clk_rate_exclusive_get() on pwm->clk and cache the rate in
> > struct axi_pwmgen.
> > 
> > > +	pwm->chip.dev = &pdev->dev;
> > > +	pwm->chip.ops = &axi_pwmgen_pwm_ops;
> > > +	pwm->chip.base = -1;
> > Don't assign .base.
> Alright. I will set pwm->chip.atomic as per Sean's comment as well.

Sounds good.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature