Re: [PATCH v4 3/7] pwm: sun4i: Add an optional probe for bus clock

[Uwe Kleine-König]

On Fri, Nov 08, 2019 at 09:45:13AM +0100, Clément Péron wrote:
> From: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> 
> H6 PWM core needs bus clock to be enabled in order to work.
> 
> Add an optional probe for it and a fallback for previous
> bindings without name on module clock.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> Signed-off-by: Clément Péron <peron.clem@xxxxxxxxx>
> ---
>  drivers/pwm/pwm-sun4i.c | 48 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 2b9a2a78591f..a10022d6c0fd 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -78,6 +78,7 @@ struct sun4i_pwm_data {
>  
>  struct sun4i_pwm_chip {
>  	struct pwm_chip chip;
> +	struct clk *bus_clk;
>  	struct clk *clk;
>  	struct reset_control *rst;
>  	void __iomem *base;
> @@ -363,9 +364,38 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
>  	if (IS_ERR(pwm->base))
>  		return PTR_ERR(pwm->base);
>  
> -	pwm->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(pwm->clk))
> +	/* Get all clocks and reset line */
> +	pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
> +	if (IS_ERR(pwm->clk)) {
> +		if (PTR_ERR(pwm->rst) != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "get clock failed %pe\n",
> +				pwm->clk);
>  		return PTR_ERR(pwm->clk);
> +	}
> +
> +	/*
> +	 * Fallback for old dtbs with a single clock and no name.
> +	 * If a parent has a clock-name called "mod" whereas the
> +	 * current node is unnamed the clock reference will be
> +	 * incorrectly obtained and will not go into this fallback.

For me "old dtbs" suggests that today a device tree should have a "mod"
clock. Is this true also for machines other than H6? And I'd put the
comment before the acquisition of the "mod" clock. Something like:

	/*
	 * A clock called "mod" is only required on H6 (for now) and on
	 * other SoCs we expect an unnamed clock. So we request "mod"
	 * first (and ignore the corner case that a parent provides a
	 * "mod" clock) and if this is not found we fall back to the
	 * first clock of the PWM.
	 */

> +	 */
> +	if (!pwm->clk) {
> +		pwm->clk = devm_clk_get(&pdev->dev, NULL);
> +		if (IS_ERR(pwm->clk)) {
> +			if (PTR_ERR(pwm->rst) != -EPROBE_DEFER)
> +				dev_err(&pdev->dev, "get clock failed %pe\n",
> +					pwm->clk);
> +			return PTR_ERR(pwm->clk);
> +		}
> +	}
> +
> +	pwm->bus_clk = devm_clk_get_optional(&pdev->dev, "bus");
> +	if (IS_ERR(pwm->bus_clk)) {
> +		if (PTR_ERR(pwm->rst) != -EPROBE_DEFER)
> +			dev_err(&pdev->dev, "get bus_clock failed %pe\n",
> +				pwm->bus_clk);
> +		return PTR_ERR(pwm->bus_clk);
> +	}
>  
>  	pwm->rst = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
>  	if (IS_ERR(pwm->rst)) {
> @@ -382,6 +412,17 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	/*
> +	 * We're keeping the bus clock on for the sake of simplicity.
> +	 * Actually it only needs to be on for hardware register
> +	 * accesses.
> +	 */
> +	ret = clk_prepare_enable(pwm->bus_clk);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Cannot prepare and enable bus_clk\n");
> +		goto err_bus;
> +	}
> +

Would it make sense to split this patch into "Prefer "mod" clock to
(unnamed) clock" and "Introduce optional bus clock"?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |