Re: [PATCH 06/10] iio: frequency: ad9910: add axi backend driver

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3839 bytes --]

Hello,

On Thu, Oct 15, 2026 at 01:49:46PM -0300, Rodrigo Alencar via B4 Relay wrote:
> +static int ad9910_axi_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +				const struct pwm_state *state)
> +{
> +	struct ad9910_axi_state *st = pwmchip_get_drvdata(chip);
> +	u64 period, duty;
> +	int ret;
> +
> +	guard(mutex)(&st->lock);
> +
> +	period = mul_u64_u64_div_u64(state->period, st->pd_clk_freq_uhz,
> +				     1ULL * NANO * MICROHZ_PER_HZ);

Does st->lock protect st->pd_clk_freq_uhz? If not, the lock can be taken
later.

> +	period = min(period, U32_MAX);
> +	duty = mul_u64_u64_div_u64(state->duty_cycle, st->pd_clk_freq_uhz,
> +				   1ULL * NANO * MICROHZ_PER_HZ);
> +	duty = min(duty, period);
> +
> +	if (state->polarity == PWM_POLARITY_INVERSED)
> +		duty = period - duty;
> +
> +	ret = regmap_write(st->regmap, AD9910_AXI_REG_DRCTL_PERIOD, period);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(st->regmap, AD9910_AXI_REG_DRCTL_WIDTH, duty);
> +	if (ret)
> +		return ret;
> +
> +	st->pwm_polarity = state->polarity;

An inversed polarity waveform isn't the same as just inverting the
duty length. So this is cheating. The right thing to implement here
might be inconvenient, but you shouldn't implement .apply() anyhow and
the waveform stuff has saner semantics.

> +	if (state->enabled)
> +		return regmap_set_bits(st->regmap, AD9910_AXI_REG_DRG,
> +				       AD9910_AXI_REG_DRG_TOGGLE_EN_MSK);
> +
> +	return regmap_clear_bits(st->regmap, AD9910_AXI_REG_DRG,
> +				 AD9910_AXI_REG_DRG_TOGGLE_EN_MSK);
> +}
> +
> [...]
> +static const struct pwm_ops ad9910_axi_pwm_ops = {
> +	.apply = ad9910_axi_pwm_apply,
> +	.get_state = ad9910_axi_pwm_get_state,

Please use the waveform callbacks instead of .apply() and .get_state().

> +};
> +
> +static int ad9910_axi_setup(struct ad9910_axi_state *st)
> +{
> +	int ret;
> +
> +	st->pd_clk_freq_uhz = AD9910_AXI_PD_CLK_MAX_FREQ_HZ * MICROHZ_PER_HZ;
> +
> +	ret = regmap_set_bits(st->regmap, AD9910_AXI_REG_RESET,
> +			      AD9910_AXI_REG_RESET_CORE_MSK);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_clear_bits(st->regmap, AD9910_AXI_REG_RESET,
> +				 AD9910_AXI_REG_RESET_CORE_MSK);
> +}
> +
> +static int ad9910_axi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ad9910_axi_state *st;
> +	struct pwm_chip *pwm;

the name `pwm` is usually only used for variables of type struct
pwm_chip *. Please call this `chip` as it is done in the rest of the
driver.

> +	void __iomem *base;
> +	struct clk *clk;
> +	int ret;
> +
> +	pwm = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*st));
> +	if (IS_ERR(pwm))
> +		return PTR_ERR(pwm);
> +
> +	st = pwmchip_get_drvdata(pwm);
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	clk = devm_clk_get_enabled(dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "failed to get clock\n");
> +
> +	st->dev = dev;
> +	st->regmap = devm_regmap_init_mmio(dev, base,
> +					   &ad9910_axi_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "failed to init register map\n");
> +
> +	ret = devm_mutex_init(dev, &st->lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = ad9910_axi_setup(st);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to setup device\n");
> +
> +	pwm->ops = &ad9910_axi_pwm_ops;
> +
> +	ret = devm_pwmchip_add(dev, pwm);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Could not register PWM chip\n");
> +
> +	return devm_iio_backend_register(dev, &ad9910_axi_back_info, st);

It's unclear to me what is the relation between that iio backend and the
PWM. If .set_sample_rate() is called for the iio device, does that
change the output of the PWM? If yes, that's bad and needs fixing.

> +}

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]