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

[Richard GENOUD <richard.genoud@bootlin.com>]

Le 26/02/2026 à 15:12, Philipp Zabel a écrit :
> 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().
Ah! You're right!

> 
> I think you could even use devm_pwmchip_add() and
> devm_reset_control_get_shared_deasserted() and remove h616_pwm_remove
> entirely.
Nice!
I'll do that.

Thanks!

Regards,
Richard

> 
> regards
> Philipp