Re: [PATCH v16 1/7] spi: pxa2xx: introduce clock enable and disable helper functions

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Jul 21, 2026 at 12:21:10AM +0800, Shih-Yuan Lee wrote:
> The driver disables the clock during PM runtime suspend, PM system
> suspend, and device unbinding (remove). It also disables the clock
> on various error unwinding paths in pxa2xx_spi_probe().
> 
> However, if the clock is already disabled (for example, if the device is
> already runtime-suspended during driver unbinding), calling the common
> clock framework's clk_disable_unprepare() again leads to clock prepare/enable
> count underflows, generating kernel warnings.

Any real life example here?

> Introduce pxa2xx_spi_clk_enable() and pxa2xx_spi_clk_disable() helper
> functions that track the clock enable state using a new 'clk_enabled'
> boolean flag in struct driver_data.
> 
> This ensures clk_disable_unprepare() is called only when the clock is
> active, preventing clock underflows during suspend transitions and unbind.
> It also allows the probe function to safely unwind resource allocations
> without triggering clock underflows.

...

Same issue, I have no cover letter in my mailbox. Please, slow down and check
your email setup. Something is wrong.

...

> +static int pxa2xx_spi_clk_enable(struct driver_data *drv_data)
> +{
> +	int ret;
> +
> +	if (drv_data->clk_enabled)
> +		return 0;
> +
> +	ret = clk_prepare_enable(drv_data->ssp->clk);

> +	if (ret == 0)
> +		drv_data->clk_enabled = true;
> +
> +	return ret;

Use usual pattern

	if (ret)
		return ret;

	...
	return 0;

> +}

> +static void pxa2xx_spi_clk_disable(struct driver_data *drv_data)
> +{
> +	if (drv_data->clk_enabled) {

	if (!drv_data->clk_enabled)
		return;

> +		clk_disable_unprepare(drv_data->ssp->clk);
> +		drv_data->clk_enabled = false;
> +	}
> +}

...

> struct driver_data {

>  	void __iomem *lpss_base;
>  
> +	bool clk_enabled;

How is this protected against simultaneously called enable/disable on different
CPUs?

>  	/* Optional slave FIFO ready signal */
>  	struct gpio_desc *gpiod_ready;
>  };

Whenever you add the field, check with `pahole` that the layout is optimal.

-- 
With Best Regards,
Andy Shevchenko