Re: [PATCH v1 080/101] pwm: lpss: Store parent device in driver data

[Andy Shevchenko <andy@kernel.org>]

On Tue, Aug 08, 2023 at 07:19:10PM +0200, Uwe Kleine-König wrote:
> struct pwm_chip::dev is about to change. To not have to touch this
> driver in the same commit as struct pwm_chip::dev, store a pointer to
> the parent device in driver data.

I'm not against this change, so
Acked-by: Andy Shevchenko <andy@kernel.org>
bu see some comments below.

I think ideally pwm_chip should be an opaque to the driver
(or something near to it). OTOH it may be I understood that
wrong.

...

>  	if (state->enabled) {
>  		if (!pwm_is_enabled(pwm)) {
> -			pm_runtime_get_sync(chip->dev);
> +			pm_runtime_get_sync(lpwm->parent);
>  			ret = pwm_lpss_prepare_enable(lpwm, pwm, state);
>  			if (ret)
> -				pm_runtime_put(chip->dev);
> +				pm_runtime_put(lpwm->parent);
>  		} else {
>  			ret = pwm_lpss_prepare_enable(lpwm, pwm, state);
>  		}
>  	} else if (pwm_is_enabled(pwm)) {
>  		pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE);
> -		pm_runtime_put(chip->dev);
> +		pm_runtime_put(lpwm->parent);
>  	}

I'm wondering why PM runtime calls can't be part of PWM core?
We may cleanup a lot of code with it, no?

...

> -	pm_runtime_get_sync(chip->dev);
> +	pm_runtime_get_sync(lpwm->parent);


> -	pm_runtime_put(chip->dev);
> +	pm_runtime_put(lpwm->parent);

Ditto.

...

>  struct pwm_lpss_chip {
> +	struct device *parent;

Have you checked IIO approach with the public and private members
(under private the opaque pointer is meant)? Maybe something of that
can be applied to PWM code as well, dunno.

>  	void __iomem *regs;
>  	const struct pwm_lpss_boardinfo *info;
>  };

-- 
With Best Regards,
Andy Shevchenko