[PATCH 0/3] pwm: Alternative way to convert leds-qcom-lpg to devm_pwmchip_alloc()

[PATCH 0/3] pwm: Alternative way to convert leds-qcom-lpg to devm_pwmchip_alloc()

[Uwe Kleine-König]

Hello,

on my way home thinking about my pwm-lifetime series[1] it occurred to
me how the leds-qcom-lpg driver could be converted to use
pwmchip_alloc() much prettier. Instead of patch #102 it can just not use
the feature to allocate private data and use (a new)
pwmchip_{get,set}_drvdata() function pair.

The 2nd patch is just split out of the conversion. In my original series
the equivalent was contained in the converting patch.

I'd expect that this sorts out the concerns about the ugliness I added
in two of the "non-pure" PWM drivers. A similar approach can be used (with the
same changes in core.c and pwm.h) for adapting the ti-sn65dsi86 driver.

Best regards
Uwe

[1] https://lore.kernel.org/linux-pwm/20231121134901.208535-1-u.kleine-koenig@pengutronix.de

Uwe Kleine-König (3):
  pwm: Provide wrappers for storing and getting private data
  leds: qcom-lpg: Introduce a wrapper for getting driver data from a pwm
    chip
  leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

 drivers/leds/rgb/leds-qcom-lpg.c | 27 ++++++++++++++++++---------
 drivers/pwm/core.c               |  2 ++
 include/linux/pwm.h              | 19 +++++++++++++++++++
 3 files changed, 39 insertions(+), 9 deletions(-)

(This base commit isn't published to not annoy the list with reports about
failed builds because of missing changes.)

base-commit: e40bd269dc0aa05aaf5390d66428601dc7433433
-- 
2.42.0

[PATCH 1/3] pwm: Provide wrappers for storing and getting private data

[Uwe Kleine-König]

Also call pwmchip_set_drvdata() in pwmchip_alloc() to have a sane
default. Might replace pwmchip_priv()?!

After struct pwm_chip got its own struct device, this can make use of
dev_get_drvdata() and dev_set_drvdata() on that device.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c  |  2 ++
 include/linux/pwm.h | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 17577a1c4efc..0cbce704cc0b 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -216,6 +216,8 @@ struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, si
 	chip->dev = parent;
 	chip->npwm = npwm;
 
+	pwmchip_set_drvdata(chip, pwmchip_priv(chip));
+
 	return chip;
 }
 EXPORT_SYMBOL_GPL(devm_pwmchip_alloc);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 3c62cf329ee0..7a32ac687717 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -302,6 +302,7 @@ struct pwm_chip {
 
 	/* only used internally by the PWM framework */
 	struct pwm_device *pwms;
+	void *drvdata;
 };
 
 static inline struct device *pwmchip_parent(struct pwm_chip *chip)
@@ -309,6 +310,24 @@ static inline struct device *pwmchip_parent(struct pwm_chip *chip)
 	return chip->dev;
 }
 
+static inline void *pwmchip_get_drvdata(struct pwm_chip *chip)
+{
+	/*
+	 * After pwm_chip got a dedicated struct device, this can be replaced by
+	 * dev_get_drvdata(&chip->dev);
+	 */
+	return chip->drvdata;
+}
+
+static inline void pwmchip_set_drvdata(struct pwm_chip *chip, void *data)
+{
+	/*
+	 * After pwm_chip got a dedicated struct device, this can be replaced by
+	 * dev_set_drvdata(&chip->dev, data);
+	 */
+	chip->drvdata = data;
+}
+
 #if IS_ENABLED(CONFIG_PWM)
 /* PWM user APIs */
 int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
-- 
2.42.0

Re: [PATCH 1/3] pwm: Provide wrappers for storing and getting private data

[Lee Jones]

On Fri, 24 Nov 2023, Uwe Kleine-König wrote:

> Also call pwmchip_set_drvdata() in pwmchip_alloc() to have a sane
> default. Might replace pwmchip_priv()?!
> 
> After struct pwm_chip got its own struct device, this can make use of
> dev_get_drvdata() and dev_set_drvdata() on that device.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/core.c  |  2 ++
>  include/linux/pwm.h | 19 +++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 17577a1c4efc..0cbce704cc0b 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -216,6 +216,8 @@ struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, si
>  	chip->dev = parent;
>  	chip->npwm = npwm;
>  
> +	pwmchip_set_drvdata(chip, pwmchip_priv(chip));
> +
>  	return chip;
>  }
>  EXPORT_SYMBOL_GPL(devm_pwmchip_alloc);
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 3c62cf329ee0..7a32ac687717 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -302,6 +302,7 @@ struct pwm_chip {
>  
>  	/* only used internally by the PWM framework */
>  	struct pwm_device *pwms;
> +	void *drvdata;

I appreciate that this may be temporary, but why not use the precedent
already set by struct device?

  void            *driver_data;

>  };
>  
>  static inline struct device *pwmchip_parent(struct pwm_chip *chip)
> @@ -309,6 +310,24 @@ static inline struct device *pwmchip_parent(struct pwm_chip *chip)
>  	return chip->dev;
>  }
>  
> +static inline void *pwmchip_get_drvdata(struct pwm_chip *chip)
> +{
> +	/*
> +	 * After pwm_chip got a dedicated struct device, this can be replaced by
> +	 * dev_get_drvdata(&chip->dev);
> +	 */
> +	return chip->drvdata;
> +}
> +
> +static inline void pwmchip_set_drvdata(struct pwm_chip *chip, void *data)
> +{
> +	/*
> +	 * After pwm_chip got a dedicated struct device, this can be replaced by
> +	 * dev_set_drvdata(&chip->dev, data);
> +	 */
> +	chip->drvdata = data;
> +}
> +
>  #if IS_ENABLED(CONFIG_PWM)
>  /* PWM user APIs */
>  int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state);
> -- 
> 2.42.0
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/3] pwm: Provide wrappers for storing and getting private data

[Uwe Kleine-König]

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

On Thu, Nov 30, 2023 at 02:50:27PM +0000, Lee Jones wrote:
> On Fri, 24 Nov 2023, Uwe Kleine-König wrote:
> 
> > Also call pwmchip_set_drvdata() in pwmchip_alloc() to have a sane
> > default. Might replace pwmchip_priv()?!
> > 
> > After struct pwm_chip got its own struct device, this can make use of
> > dev_get_drvdata() and dev_set_drvdata() on that device.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/core.c  |  2 ++
> >  include/linux/pwm.h | 19 +++++++++++++++++++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 17577a1c4efc..0cbce704cc0b 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -216,6 +216,8 @@ struct pwm_chip *devm_pwmchip_alloc(struct device *parent, unsigned int npwm, si
> >  	chip->dev = parent;
> >  	chip->npwm = npwm;
> >  
> > +	pwmchip_set_drvdata(chip, pwmchip_priv(chip));
> > +
> >  	return chip;
> >  }
> >  EXPORT_SYMBOL_GPL(devm_pwmchip_alloc);
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index 3c62cf329ee0..7a32ac687717 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -302,6 +302,7 @@ struct pwm_chip {
> >  
> >  	/* only used internally by the PWM framework */
> >  	struct pwm_device *pwms;
> > +	void *drvdata;
> 
> I appreciate that this may be temporary, but why not use the precedent
> already set by struct device?
> 
>   void            *driver_data;

Not sure I understood your question right. You wonder why I named the
struct member drvdata and not driver_data, right?

That's easy: I didn't look at struct device and just picked a name I
considered suiteable. I'm fine with driver_data, too, and will use that
for the next iteration.

Best regards
Uwe

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

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

[PATCH 2/3] leds: qcom-lpg: Introduce a wrapper for getting driver data from a pwm chip

[Uwe Kleine-König]

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/leds/rgb/leds-qcom-lpg.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index 68d82a682bf6..c523f3c0cdb1 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -977,9 +977,14 @@ static int lpg_pattern_mc_clear(struct led_classdev *cdev)
 	return lpg_pattern_clear(led);
 }
 
+static inline struct lpg *lpg_pwm_from_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct lpg, pwm);
+}
+
 static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
-	struct lpg *lpg = container_of(chip, struct lpg, pwm);
+	struct lpg *lpg = lpg_pwm_from_chip(chip);
 	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
 
 	return chan->in_use ? -EBUSY : 0;
@@ -995,7 +1000,7 @@ static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			 const struct pwm_state *state)
 {
-	struct lpg *lpg = container_of(chip, struct lpg, pwm);
+	struct lpg *lpg = lpg_pwm_from_chip(chip);
 	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
 	int ret = 0;
 
@@ -1026,7 +1031,7 @@ static int lpg_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 static int lpg_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 			     struct pwm_state *state)
 {
-	struct lpg *lpg = container_of(chip, struct lpg, pwm);
+	struct lpg *lpg = lpg_pwm_from_chip(chip);
 	struct lpg_channel *chan = &lpg->channels[pwm->hwpwm];
 	unsigned int resolution;
 	unsigned int pre_div;
-- 
2.42.0

[PATCH 3/3] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

[Uwe Kleine-König]

This prepares the pwm sub-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/leds/rgb/leds-qcom-lpg.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
index c523f3c0cdb1..81ebc23fd2f4 100644
--- a/drivers/leds/rgb/leds-qcom-lpg.c
+++ b/drivers/leds/rgb/leds-qcom-lpg.c
@@ -77,7 +77,7 @@ struct lpg {
 
 	struct mutex lock;
 
-	struct pwm_chip pwm;
+	struct pwm_chip *pwm;
 
 	const struct lpg_data *data;
 
@@ -979,7 +979,7 @@ static int lpg_pattern_mc_clear(struct led_classdev *cdev)
 
 static inline struct lpg *lpg_pwm_from_chip(struct pwm_chip *chip)
 {
-	return container_of(chip, struct lpg, pwm);
+	return pwmchip_get_drvdata(chip);
 }
 
 static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -1094,13 +1094,17 @@ static const struct pwm_ops lpg_pwm_ops = {
 
 static int lpg_add_pwm(struct lpg *lpg)
 {
+	struct pwm_chip *chip;
 	int ret;
 
-	lpg->pwm.dev = lpg->dev;
-	lpg->pwm.npwm = lpg->num_channels;
-	lpg->pwm.ops = &lpg_pwm_ops;
+	lpg->pwm = chip = devm_pwmchip_alloc(lpg->dev, lpg->num_channels, 0);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
 
-	ret = pwmchip_add(&lpg->pwm);
+	chip->ops = &lpg_pwm_ops;
+	pwmchip_set_drvdata(chip, lpg);
+
+	ret = pwmchip_add(chip);
 	if (ret)
 		dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret);
 
@@ -1372,7 +1376,7 @@ static void lpg_remove(struct platform_device *pdev)
 {
 	struct lpg *lpg = platform_get_drvdata(pdev);
 
-	pwmchip_remove(&lpg->pwm);
+	pwmchip_remove(lpg->pwm);
 }
 
 static const struct lpg_data pm8916_pwm_data = {
-- 
2.42.0

Re: [PATCH 0/3] pwm: Alternative way to convert leds-qcom-lpg to devm_pwmchip_alloc()

[Lee Jones]

On Fri, 24 Nov 2023, Uwe Kleine-König wrote:

> Hello,
> 
> on my way home thinking about my pwm-lifetime series[1] it occurred to
> me how the leds-qcom-lpg driver could be converted to use
> pwmchip_alloc() much prettier. Instead of patch #102 it can just not use
> the feature to allocate private data and use (a new)
> pwmchip_{get,set}_drvdata() function pair.
> 
> The 2nd patch is just split out of the conversion. In my original series
> the equivalent was contained in the converting patch.
> 
> I'd expect that this sorts out the concerns about the ugliness I added
> in two of the "non-pure" PWM drivers. A similar approach can be used (with the
> same changes in core.c and pwm.h) for adapting the ti-sn65dsi86 driver.

This is much better, thank you.

How should this be merged?

> [1] https://lore.kernel.org/linux-pwm/20231121134901.208535-1-u.kleine-koenig@pengutronix.de
> 
> Uwe Kleine-König (3):
>   pwm: Provide wrappers for storing and getting private data
>   leds: qcom-lpg: Introduce a wrapper for getting driver data from a pwm
>     chip
>   leds: qcom-lpg: Make use of devm_pwmchip_alloc() function
> 
>  drivers/leds/rgb/leds-qcom-lpg.c | 27 ++++++++++++++++++---------
>  drivers/pwm/core.c               |  2 ++
>  include/linux/pwm.h              | 19 +++++++++++++++++++
>  3 files changed, 39 insertions(+), 9 deletions(-)
> 
> (This base commit isn't published to not annoy the list with reports about
> failed builds because of missing changes.)
> 
> base-commit: e40bd269dc0aa05aaf5390d66428601dc7433433
> -- 
> 2.42.0
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH 0/3] pwm: Alternative way to convert leds-qcom-lpg to devm_pwmchip_alloc()

[Uwe Kleine-König]

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

On Thu, Nov 30, 2023 at 02:53:08PM +0000, Lee Jones wrote:
> On Fri, 24 Nov 2023, Uwe Kleine-König wrote:
> 
> > Hello,
> > 
> > on my way home thinking about my pwm-lifetime series[1] it occurred to
> > me how the leds-qcom-lpg driver could be converted to use
> > pwmchip_alloc() much prettier. Instead of patch #102 it can just not use
> > the feature to allocate private data and use (a new)
> > pwmchip_{get,set}_drvdata() function pair.
> > 
> > The 2nd patch is just split out of the conversion. In my original series
> > the equivalent was contained in the converting patch.
> > 
> > I'd expect that this sorts out the concerns about the ugliness I added
> > in two of the "non-pure" PWM drivers. A similar approach can be used (with the
> > same changes in core.c and pwm.h) for adapting the ti-sn65dsi86 driver.
> 
> This is much better, thank you.
> 
> How should this be merged?

This will be included in my next iteration of the pwm lifetime series.
The question about which tree this will go in will get relevant once the
patches this change depends on are fleshed out.

If you like patch #2 this can go in already now. Feel free to pick it
up. If you won't, that's fine, too, then I will keep it in my series.

Best regards
Uwe

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

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