Re: [PATCH v7 2/4] pwm: sun8i: Add H616 PWM support

On Fr, 2026-07-03 at 17:22 +0200, Richard Genoud wrote:
> Add driver for Allwinner H616 PWM controller, supporting up to 6
> channels.
> Those channels output can be either a PWM signal output or a clock
> output, thanks to the bypass.
> 
> The channels are paired (0/1, 2/3 and 4/5) and each pair has a
> prescaler/mux/gate.
> Moreover, each channel has its own prescaler and bypass.
> 
> The clock provider part of this driver is needed not only because the
> H616 PWM controller provides also clocks when bypass is enabled, but
> really because pwm-clock isn't fit to handle all cases here.
> pwm-clock would work if the 100MHz clock is requested, but if a lower
> clock is requested (like 24MHz), it will request a 42ns period to the
> PWM driver which will happily serve, with the 100MHz clock as input a
> 25MHz frequency and a duty cycle adjustable in the range [0-4]/4,
> because that is a sane thing to do for a PWM.
> The information missing is that a real clock is resquested, not a PWM.
> 
> Tested-by: John Stultz <[email protected]>
> Tested-by: Joao Schim <[email protected]>
> Signed-off-by: Richard Genoud <[email protected]>
> ---
>  drivers/pwm/Kconfig     |  12 +
>  drivers/pwm/Makefile    |   1 +
>  drivers/pwm/pwm-sun8i.c | 938 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 951 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sun8i.c
> 
[...]
> diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c
> new file mode 100644
> index 000000000000..8f1023e3a2e5
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun8i.c
> @@ -0,0 +1,938 @@
[...]
> +struct sun8i_pwm_chip {
> +	struct clk_pwm_pdata *clk_pdata;
> +	struct sun8i_pwm_channel *channels;
> +	struct clk *bus_clk;
> +	struct reset_control *rst;

The rst field is unused, I suggest you remove it.

> +	void __iomem *base;
> +	const struct sun8i_pwm_data *data;
> +};
[...]
> +static int sun8i_pwm_probe(struct platform_device *pdev)
> +{
[...]
> +	ret = sun8i_pwm_init_clocks(pdev, sun8i_chip);
> +	if (ret)
> +		return ret;
> +
> +	for (unsigned int i = 0; i < data->npwm; i++) {
> +		struct sun8i_pwm_channel *chan = &sun8i_chip->channels[i];
> +		struct clk_hw **hw = &sun8i_chip->clk_pdata->hw_data->hws[i];
> +
> +		chan->pwm_clk = devm_clk_hw_get_clk(dev, *hw, NULL);
> +		if (IS_ERR(chan->pwm_clk)) {
> +			ret = dev_err_probe(dev, PTR_ERR(chan->pwm_clk),
> +					    "Failed to register PWM clock %d\n", i);
> +			return ret;

If this returns ...

> +		}
> +		chan->mode = SUN8I_PWM_MODE_NONE;
> +	}
> +
> +	ret = devm_of_clk_add_hw_provider(dev, sun8i_pwm_of_clk_get, sun8i_chip);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add HW clock provider\n");

... or this returns ...

> +
> +	ret = devm_add_action_or_reset(dev, sun8i_pwm_unregister_clk,
> +				       sun8i_chip->clk_pdata->hw_data);

... the clk_hw registered in sun8i_pwm_init_clocks() are never cleaned
up, so this devres action should be set up right after
sun8i_pwm_init_clocks().

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add devm action\n");
> +
> +	/* Deassert reset */
> +	sun8i_chip->rst = devm_reset_control_get_shared_deasserted(dev, NULL);

rst is never used again. It should be a local variable.

> +	if (IS_ERR(sun8i_chip->rst))
> +		return dev_err_probe(dev, PTR_ERR(sun8i_chip->rst),
> +				     "Failed to get reset control\n");
> +
> +	ret = devm_pwmchip_add(dev, chip);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to add PWM chip\n");
> +
> +	platform_set_drvdata(pdev, chip);
> +
> +	return 0;
> +}

regards
Philipp