Re: [PATCH v3 102/108] leds: qcom-lpg: Make use of devm_pwmchip_alloc() function

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

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

On Wed, Nov 22, 2023 at 11:56:21AM +0000, Lee Jones wrote:
> On Tue, 21 Nov 2023, Uwe Kleine-König wrote:
> 
> > 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 | 30 +++++++++++++++++++++---------
> >  1 file changed, 21 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> > index 68d82a682bf6..283227e02df6 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;
> >  
> > @@ -977,9 +977,15 @@ 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)
> > +{
> > +	struct lpg **lpg = pwmchip_priv(chip);
> > +	return *lpg;
> > +}
> 
> I don't have easy-vis into the other patches, but if this is a common
> pattern, perhaps add a generic helper in <linux/pwm.h>?
> 
> >  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 +1001,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 +1032,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;
> > @@ -1089,13 +1095,19 @@ 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,
> > +					     sizeof(&lpg));
> > +	if (IS_ERR(chip))
> > +		return PTR_ERR(chip);
> >  
> > -	ret = pwmchip_add(&lpg->pwm);
> > +	*(struct lpg **)pwmchip_priv(chip) = lpg;
> 
> This is vile!

Indeed. This highlights one of the weaker parts of this whole design and
I really don't like it. The whole chip_alloc() construct works fine if
you have everything isolated nicely in a single driver and subsystem
(like you usually have in network land), but for cases like this where
things are spread throughout and a device is actually more than just a
PWM controller, it looks like we now have to work around this design
because it doesn't fit.

In fact, this reminds me about the "midlayer mistake" in many ways and
combined with what Bartosz said, I'm not sure this is going to hold up
very well the more special cases we get.

Thierry

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