Re: [PATCH v7 2/3] pwm: rp1: Add RP1 PWM controller driver

[Andrea della Porta <andrea.porta@suse.com>]

Hi Christophe,

On 22:36 Thu 03 Sep     , Christophe JAILLET wrote:
> Le 20/07/2026 à 11:44, Andrea della Porta a écrit :
> > From: Naushir Patuck <naush@raspberrypi.com>
> > 
> > The Raspberry Pi RP1 southbridge features an embedded PWM
> > controller with 4 output channels, alongside an RPM interface
> > to read the fan speed on the Raspberry Pi 5.
> > 
> > Add the supporting driver.
> > 
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Co-developed-by: Stanimir Varbanov <svarbanov@suse.de>
> > Signed-off-by: Stanimir Varbanov <svarbanov@suse.de>
> > Signed-off-by: Andrea della Porta <andrea.porta@suse.com>
> 
> Hi,
> 
> ...
> 
> > +static int rp1_pwm_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	unsigned long clk_rate;
> > +	struct pwm_chip *chip;
> > +	void __iomem	*base;
> > +	struct rp1_pwm *rp1;
> > +	int ret;
> > +
> > +	chip = devm_pwmchip_alloc(dev, RP1_PWM_NUM_PWMS, sizeof(*rp1));
> > +	if (IS_ERR(chip))
> > +		return PTR_ERR(chip);
> > +
> > +	rp1 = pwmchip_get_drvdata(chip);
> > +
> > +	base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> > +
> > +	rp1->regmap = devm_regmap_init_mmio(dev, base, &rp1_pwm_regmap_config);
> > +	if (IS_ERR(rp1->regmap))
> > +		return dev_err_probe(dev, PTR_ERR(rp1->regmap), "Cannot initialize regmap\n");
> > +
> > +	rp1->clk = devm_clk_get(dev, NULL);
> 
> Could it be devm_clk_get_enabled() to simplify the error handling path as
> done above with other devm function?

The very first version of this patches had devres everywhere, but Uwe has correctly
spotted that this could lead to clock ops imbalance, please see:
https://lore.kernel.org/all/adLTwOTbkJ0VQXy6@monoceros/
As a result, I turned devm_clk_get_enabled() into the corresponding non devres/single
component functions since now disengaging the clock depends on a conditional.
Of course this does not make much sense in case we don't need a .remove callback,
but it seems that I can reintroduce it again if we agree to use EXPORT_SYMBOL_NS.

> ...
> 
> > +	if (IS_ERR(rp1->clk))
> > +		return dev_err_probe(dev, PTR_ERR(rp1->clk), "Clock not found\n");
> > +
> > +	ret = clk_prepare_enable(rp1->clk);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to enable clock\n");
> 
> ... this also saves these 3 lines.

See above.

> 
> > +	rp1->clk_enabled = true;
> > +
> > +	ret = devm_clk_rate_exclusive_get(dev, rp1->clk);
> > +	if (ret) {
> > +		dev_err_probe(dev, ret, "Failed to get exclusive rate\n");
> > +		goto err_disable_clk;
> > +	}
> > +
> > +	clk_rate = clk_get_rate(rp1->clk);
> > +	if (!clk_rate) {
> > +		ret = dev_err_probe(dev, -EINVAL, "Failed to get clock rate\n");
> > +		goto err_disable_clk;
> > +	}
> > +	/*
> > +	 * To prevent u64 overflow in period calculations:
> > +	 * mul_u64_u64_div_u64(period_ns, clk_rate, NSEC_PER_SEC)
> > +	 * If clk_rate > 1 GHz, the result can overflow.
> > +	 */
> > +	if (clk_rate > HZ_PER_GHZ) {
> > +		ret = dev_err_probe(dev, -EINVAL, "Clock rate > 1 GHz is not supported\n");
> > +		goto err_disable_clk;
> > +	}
> > +	rp1->clk_rate = clk_rate;
> > +
> > +	chip->ops = &rp1_pwm_ops;
> > +	chip->atomic = true;
> > +
> > +	platform_set_drvdata(pdev, chip);
> > +
> > +	ret = pwmchip_add(chip);
> 
> Could it be devm_pwmchip_add() to simplify the error handling path as done
> above with other devm function?

Due to the above-mentioned scenario and since .remove is called before devres release
funtions, that would make the clock to be released before the pwm chip, causing
inconsistencies if the pwm is used in the meanwhile. 

Many thanks,
Andrea.

> 
> > +	if (ret) {
> > +		dev_err_probe(dev, ret, "Failed to register PWM chip\n");
> > +		goto err_disable_clk;
> > +	}
> > +
> > +	ret = of_syscon_register_regmap(np, rp1->regmap);
> > +	if (ret) {
> > +		dev_err_probe(dev, ret, "Failed to register syscon\n");
> > +		goto err_remove_chip;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_remove_chip:
> > +	pwmchip_remove(chip);
> > +err_disable_clk:
> > +	clk_disable_unprepare(rp1->clk);
> > +
> > +	return ret;
> > +}
> ...
> 
> CJ