[v5,087/111] pwm: sl28cpld: Make use of devm_pwmchip_alloc() function

This prepares the pwm-sl28cpld driver to further changes of the pwm core
outlined in the commit introducing devm_pwmchip_alloc(). There is no
intended semantical change and the driver should behave as before.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-sl28cpld.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

> This prepares the pwm-sl28cpld driver to further changes of the pwm 
> core
> outlined in the commit introducing devm_pwmchip_alloc(). There is no
> intended semantical change and the driver should behave as before.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Reviewed-by: Michael Walle <mwalle@kernel.org>

With a small nit below.

> ---
>  drivers/pwm/pwm-sl28cpld.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
> index 88b01ff9e460..934378d6a002 100644
> --- a/drivers/pwm/pwm-sl28cpld.c
> +++ b/drivers/pwm/pwm-sl28cpld.c
> @@ -81,14 +81,13 @@
>  	regmap_write((priv)->regmap, (priv)->offset + (reg), (val))
> 
>  struct sl28cpld_pwm {
> -	struct pwm_chip chip;
>  	struct regmap *regmap;
>  	u32 offset;
>  };
> 
>  static inline struct sl28cpld_pwm *sl28cpld_pwm_from_chip(struct 
> pwm_chip *chip)
>  {
> -	return container_of(chip, struct sl28cpld_pwm, chip);
> +	return pwmchip_get_drvdata(chip);

This function now seems superfluous. Better use
pwmchip_get_drvdata(chip) directly.

If you don't respin or this is too much work, I can
send a patch once this is applied.

-michael

>  }
> 
>  static int sl28cpld_pwm_get_state(struct pwm_chip *chip,
> @@ -213,9 +212,10 @@ static int sl28cpld_pwm_probe(struct 
> platform_device *pdev)
>  		return -ENODEV;
>  	}
> 
> -	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> -	if (!priv)
> -		return -ENOMEM;
> +	chip = devm_pwmchip_alloc(&pdev->dev, 1, sizeof(*priv));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +	priv = sl28cpld_pwm_from_chip(chip);
> 
>  	priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>  	if (!priv->regmap) {
> @@ -231,10 +231,7 @@ static int sl28cpld_pwm_probe(struct 
> platform_device *pdev)
>  	}
> 
>  	/* Initialize the pwm_chip structure */
> -	chip = &priv->chip;
> -	chip->dev = &pdev->dev;
>  	chip->ops = &sl28cpld_pwm_ops;
> -	chip->npwm = 1;
> 
>  	ret = devm_pwmchip_add(&pdev->dev, chip);
>  	if (ret) {

On Thu, Jan 25, 2024 at 01:26:45PM +0100, Michael Walle wrote:
> > This prepares the pwm-sl28cpld driver to further changes of the pwm core
> > outlined in the commit introducing devm_pwmchip_alloc(). There is no
> > intended semantical change and the driver should behave as before.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> Reviewed-by: Michael Walle <mwalle@kernel.org>
> 
> With a small nit below.
> 
> > ---
> >  drivers/pwm/pwm-sl28cpld.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
> > index 88b01ff9e460..934378d6a002 100644
> > --- a/drivers/pwm/pwm-sl28cpld.c
> > +++ b/drivers/pwm/pwm-sl28cpld.c
> > @@ -81,14 +81,13 @@
> >  	regmap_write((priv)->regmap, (priv)->offset + (reg), (val))
> > 
> >  struct sl28cpld_pwm {
> > -	struct pwm_chip chip;
> >  	struct regmap *regmap;
> >  	u32 offset;
> >  };
> > 
> >  static inline struct sl28cpld_pwm *sl28cpld_pwm_from_chip(struct
> > pwm_chip *chip)
> >  {
> > -	return container_of(chip, struct sl28cpld_pwm, chip);
> > +	return pwmchip_get_drvdata(chip);
> 
> This function now seems superfluous. Better use
> pwmchip_get_drvdata(chip) directly.
> 
> If you don't respin or this is too much work, I can
> send a patch once this is applied.

I like it keeping around, becuase it returns the right type (instead of
pwmchip_get_drvdata() which returns void). Also it might be beneficial
to mark this function with the const attribute. If you want to care for
this, that would be very welcome.

Best regards
Uwe

>> >  static inline struct sl28cpld_pwm *sl28cpld_pwm_from_chip(struct
>> > pwm_chip *chip)
>> >  {
>> > -	return container_of(chip, struct sl28cpld_pwm, chip);
>> > +	return pwmchip_get_drvdata(chip);
>> 
>> This function now seems superfluous. Better use
>> pwmchip_get_drvdata(chip) directly.
>> 
>> If you don't respin or this is too much work, I can
>> send a patch once this is applied.
> 
> I like it keeping around, becuase it returns the right type (instead of
> pwmchip_get_drvdata() which returns void). Also it might be beneficial
> to mark this function with the const attribute. If you want to care for
> this, that would be very welcome.

It's a matter of taste I guess, so I'm fine with either. It's only used 
in
two places. If you return const you can't use it in the probe function. 
And
using pwmchip_get_drvdata() just there is worse.

-michael

Hello Michael,

On Thu, Jan 25, 2024 at 02:28:56PM +0100, Michael Walle wrote:
> > > >  static inline struct sl28cpld_pwm *sl28cpld_pwm_from_chip(struct
> > > > pwm_chip *chip)
> > > >  {
> > > > -	return container_of(chip, struct sl28cpld_pwm, chip);
> > > > +	return pwmchip_get_drvdata(chip);
> > > 
> > > This function now seems superfluous. Better use
> > > pwmchip_get_drvdata(chip) directly.
> > > 
> > > If you don't respin or this is too much work, I can
> > > send a patch once this is applied.
> > 
> > I like it keeping around, becuase it returns the right type (instead of
> > pwmchip_get_drvdata() which returns void). Also it might be beneficial
> > to mark this function with the const attribute. If you want to care for
> > this, that would be very welcome.
> 
> It's a matter of taste I guess, so I'm fine with either. It's only used in
> two places. If you return const you can't use it in the probe function.

Why's that?

My guess is, we're not talking about the same thing. I consider:

diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
index 934378d6a002..1dcdcdd03787 100644
--- a/drivers/pwm/pwm-sl28cpld.c
+++ b/drivers/pwm/pwm-sl28cpld.c
@@ -85,6 +85,7 @@ struct sl28cpld_pwm {
 	u32 offset;
 };
 
+static inline struct sl28cpld_pwm *sl28cpld_pwm_from_chip(struct pwm_chip *chip) __attribute__((const));
 static inline struct sl28cpld_pwm *sl28cpld_pwm_from_chip(struct pwm_chip *chip)
 {
 	return pwmchip_get_drvdata(chip);

. Now that I created that diff: It doesn't change anything. Other
drivers might benefit from a similar annotation if they call their
from_chip function in more places.

> And using pwmchip_get_drvdata() just there is worse.

Ack.

Best regards
Uwe

> My guess is, we're not talking about the same thing. I consider:
> 
> diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
> index 934378d6a002..1dcdcdd03787 100644
> --- a/drivers/pwm/pwm-sl28cpld.c
> +++ b/drivers/pwm/pwm-sl28cpld.c
> @@ -85,6 +85,7 @@ struct sl28cpld_pwm {
>  	u32 offset;
>  };
> 
> +static inline struct sl28cpld_pwm *sl28cpld_pwm_from_chip(struct 
> pwm_chip *chip) __attribute__((const));
>  static inline struct sl28cpld_pwm *sl28cpld_pwm_from_chip(struct 
> pwm_chip *chip)

You're right. I thought you mean returing pointer to a const struct.

Anyway, it might be correct in the pwm case, because the data is 
contained
within the pwm struct (at least for now), so for any given pointer the
returned value is just pointer + offset. But there is a huge warning 
that
const shouldn't be used with pointers (or rather to look at the data the
pointer points to). Therefore, I'm not sure this is a good idea.

OTOH I've never used this attribute, so I might be wrong. The kernel
doesn't seem to use it very often.

-michael