Re: [PATCH v3 2/4] pwm: sun50i: Add H616 PWM support

[Philipp Zabel <p.zabel@pengutronix.de>]

On Fr, 2026-01-23 at 10:33 +0100, 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.
> 
> Signed-off-by: Richard Genoud <richard.genoud@bootlin.com>
> ---
>  drivers/pwm/Kconfig           |  12 +
>  drivers/pwm/Makefile          |   1 +
>  drivers/pwm/pwm-sun50i-h616.c | 959 ++++++++++++++++++++++++++++++++++
>  3 files changed, 972 insertions(+)
>  create mode 100644 drivers/pwm/pwm-sun50i-h616.c
> 
[...]
> diff --git a/drivers/pwm/pwm-sun50i-h616.c b/drivers/pwm/pwm-sun50i-h616.c
> new file mode 100644
> index 000000000000..02a8e2d39f86
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun50i-h616.c
> @@ -0,0 +1,959 @@
[...]
> +static int h616_pwm_init_clocks(struct platform_device *pdev,
> +				struct h616_pwm_chip *h616chip)
> +{
> +	struct clk_pwm_pdata *pdata;
> +	struct device *dev = &pdev->dev;
> +	int num_clocks = 0;
> +	int ret;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "Failed to allocate clk_pwm_pdata\n");
> +
> +	while (pwmcc_data[num_clocks].name)
> +		num_clocks++;
> +
> +	pdata->hw_data = devm_kzalloc(dev, struct_size(pdata->hw_data, hws, num_clocks),
> +				      GFP_KERNEL);
> +	if (!pdata->hw_data)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "Failed to allocate hw clocks\n");
> +
> +	pdata->hw_data->num = num_clocks;
> +	pdata->reg = h616chip->base;
> +
> +	spin_lock_init(&pdata->lock);
> +
> +	for (int i = 0; i < num_clocks; i++) {
> +		struct clk_hw **hw = &pdata->hw_data->hws[i];
> +
> +		ret = h616_add_composite_clk(&pwmcc_data[i], pdata->reg,
> +					     &pdata->lock, dev, hw);
> +		if (ret) {
> +			dev_err_probe(dev, ret,
> +				      "Failed to register hw clock %s\n",
> +				      pwmcc_data[i].name);
> +			for (i--; i >= 0; i--)
> +				clk_hw_unregister_composite(pdata->hw_data->hws[i]);
> +			return ret;
> +		}
> +	}
> +
> +	h616chip->clk_pdata = pdata;
> +
> +	return 0;
> +}
> +
> +static int h616_pwm_probe(struct platform_device *pdev)
> +{
> +	const struct h616_pwm_data *data;
> +	struct device *dev = &pdev->dev;
> +	struct h616_pwm_chip *h616chip;
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	data = of_device_get_match_data(dev);
> +	if (!data)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "Missing specific data structure\n");
> +
> +	chip = devm_pwmchip_alloc(dev, data->npwm, sizeof(*h616chip));
> +	if (IS_ERR(chip))
> +		return dev_err_probe(dev, PTR_ERR(chip),
> +				     "Failed to allocate pwmchip\n");
> +
> +	h616chip = h616_pwm_from_chip(chip);
> +	h616chip->data = data;
> +	h616chip->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(h616chip->base))
> +		return dev_err_probe(dev, PTR_ERR(h616chip->base),
> +				     "Failed to get PWM base address\n");
> +
> +	h616chip->bus_clk = devm_clk_get_enabled(dev, "bus");
> +	if (IS_ERR(h616chip->bus_clk))
> +		return dev_err_probe(dev, PTR_ERR(h616chip->bus_clk),
> +				     "Failed to get bus clock\n");
> +
> +	h616chip->channels = devm_kmalloc_array(dev, data->npwm,
> +						sizeof(*(h616chip->channels)),
> +						GFP_KERNEL);
> +	if (!h616chip->channels)
> +		return dev_err_probe(dev, -ENOMEM,
> +				     "Failed to allocate %d channels array\n",
> +				     data->npwm);
> +
> +	h616chip->rst = devm_reset_control_get_shared(dev, NULL);
> +	if (IS_ERR(h616chip->rst))
> +		return dev_err_probe(dev, PTR_ERR(h616chip->rst),
> +				     "Failed to get reset control\n");
> +
> +	chip->ops = &h616_pwm_ops;
> +
> +	ret = h616_pwm_init_clocks(pdev, h616chip);
> +	if (ret)
> +		return ret;
> +
> +	for (unsigned int i = 0; i < data->npwm; i++) {
> +		struct h616_pwm_channel *chan = &h616chip->channels[i];
> +		struct clk_hw **hw = &h616chip->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);
> +			goto err_get_clk;
> +		}
> +		chan->mode = H616_PWM_MODE_NONE;
> +	}
> +
> +	ret = devm_of_clk_add_hw_provider(dev, h616_pwm_of_clk_get, h616chip);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to add HW clock provider\n");
> +		goto err_add_clk_provider;
> +	}
> +
> +	/* Deassert reset */
> +	ret = reset_control_deassert(h616chip->rst);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Cannot deassert reset control\n");
> +		goto err_ctrl_deassert;
> +	}
> +
> +	ret = pwmchip_add(chip);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "Failed to add PWM chip\n");
> +		goto err_pwm_add;
> +	}
> +
> +	platform_set_drvdata(pdev, chip);
> +
> +	return 0;
> +
> +err_pwm_add:
> +	reset_control_assert(h616chip->rst);
> +
> +err_ctrl_deassert:
> +err_add_clk_provider:
> +err_get_clk:
> +	for (unsigned int i = 0; i < h616chip->clk_pdata->hw_data->num; i++)
> +		clk_hw_unregister_composite(h616chip->clk_pdata->hw_data->hws[i]);

Won't this try to unregister the clk_hw before the pwm_clk derived from
it? You could place this in a devres action to correct the cleanup
order and get rid of the duplicated cleanup in h616_pwm_remove().

I think you could even use devm_pwmchip_add() and
devm_reset_control_get_shared_deasserted() and remove h616_pwm_remove
entirely.

regards
Philipp