Re: [PATCH v1 071/101] pwm: ab8500: Store parent device in driver data

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3955 bytes --]

On Tue, Aug 08, 2023 at 07:19:01PM +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.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-ab8500.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)

You're basically adding a parent device to all driver-private data
structures in this and the following patches, so why not keep in in
struct pwm_chip and simply rename chip->dev to chip->parent?

As Andy has commented, this would eventually allow the PWM core to
take care of certain things like runtime PM, or even only for stuff
like using the parent device name in info/debug/error messages.

Also, you could then just make this a single large patch that renames
dev to parent in one go rather than making this large set even larger
with this kind of trivial changes.

Thierry

> 
> diff --git a/drivers/pwm/pwm-ab8500.c b/drivers/pwm/pwm-ab8500.c
> index f64f3fd251e7..663fdfe90bb6 100644
> --- a/drivers/pwm/pwm-ab8500.c
> +++ b/drivers/pwm/pwm-ab8500.c
> @@ -24,6 +24,7 @@
>  #define AB8500_PWM_CLKRATE 9600000
>  
>  struct ab8500_pwm_chip {
> +	struct device *parent;
>  	unsigned int hwid;
>  };
>  
> @@ -91,7 +92,7 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * when disabled.
>  	 */
>  	if (!state->enabled || duty_steps == 0) {
> -		ret = abx500_mask_and_set_register_interruptible(chip->dev,
> +		ret = abx500_mask_and_set_register_interruptible(ab8500->parent,
>  					AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG,
>  					1 << ab8500->hwid, 0);
>  
> @@ -111,18 +112,18 @@ static int ab8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	reg = AB8500_PWM_OUT_CTRL1_REG + (ab8500->hwid * 2);
>  
> -	ret = abx500_set_register_interruptible(chip->dev, AB8500_MISC,
> +	ret = abx500_set_register_interruptible(ab8500->parent, AB8500_MISC,
>  			reg, lower_val);
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = abx500_set_register_interruptible(chip->dev, AB8500_MISC,
> +	ret = abx500_set_register_interruptible(ab8500->parent, AB8500_MISC,
>  			(reg + 1), higher_val);
>  	if (ret < 0)
>  		return ret;
>  
>  	/* enable */
> -	ret = abx500_mask_and_set_register_interruptible(chip->dev,
> +	ret = abx500_mask_and_set_register_interruptible(ab8500->parent,
>  				AB8500_MISC, AB8500_PWM_OUT_CTRL7_REG,
>  				1 << ab8500->hwid, 1 << ab8500->hwid);
>  
> @@ -137,7 +138,7 @@ static int ab8500_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct ab8500_pwm_chip *ab8500 = ab8500_pwm_from_chip(chip);
>  	unsigned int div, duty_steps;
>  
> -	ret = abx500_get_register_interruptible(chip->dev, AB8500_MISC,
> +	ret = abx500_get_register_interruptible(ab8500->parent, AB8500_MISC,
>  						AB8500_PWM_OUT_CTRL7_REG,
>  						&ctrl7);
>  	if (ret)
> @@ -150,13 +151,13 @@ static int ab8500_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  		return 0;
>  	}
>  
> -	ret = abx500_get_register_interruptible(chip->dev, AB8500_MISC,
> +	ret = abx500_get_register_interruptible(ab8500->parent, AB8500_MISC,
>  						AB8500_PWM_OUT_CTRL1_REG + (ab8500->hwid * 2),
>  						&lower_val);
>  	if (ret)
>  		return ret;
>  
> -	ret = abx500_get_register_interruptible(chip->dev, AB8500_MISC,
> +	ret = abx500_get_register_interruptible(ab8500->parent, AB8500_MISC,
>  						AB8500_PWM_OUT_CTRL2_REG + (ab8500->hwid * 2),
>  						&higher_val);
>  	if (ret)
> @@ -196,6 +197,7 @@ static int ab8500_pwm_probe(struct platform_device *pdev)
>  	ab8500 = pwmchip_priv(chip);
>  
>  	chip->ops = &ab8500_pwm_ops;
> +	ab8500->parent = &pdev->dev;
>  	ab8500->hwid = pdev->id - 1;
>  
>  	err = devm_pwmchip_add(&pdev->dev, chip);
> -- 
> 2.40.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]