| Message ID | 20231121134901.208535-103-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
| Series | pwm: Fix lifetime issues for pwm_chips | expand |
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(-) Does this need to be taken in with the other 107 patches?
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!
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
Hello Lee, 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; > > [...] > > @@ -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! This is indeed one of the uglier conversions. It gets a bit prettier with the following addon patch: diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c index 283227e02df6..e09eba823057 100644 --- a/drivers/leds/rgb/leds-qcom-lpg.c +++ b/drivers/leds/rgb/leds-qcom-lpg.c @@ -206,6 +206,10 @@ struct lpg_data { const struct lpg_channel_data *channels; }; +struct lpg_pwm_data { + struct lpg *lpg; +}; + static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable) { /* Skip if we don't have a triled block */ @@ -979,8 +983,9 @@ static int lpg_pattern_mc_clear(struct led_classdev *cdev) static inline struct lpg *lpg_pwm_from_chip(struct pwm_chip *chip) { - struct lpg **lpg = pwmchip_priv(chip); - return *lpg; + struct lpg_pwm_data *lpg_pwm_data = pwmchip_priv(chip); + + return lpg_pwm_data->lpg; } static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) @@ -1096,14 +1101,16 @@ static const struct pwm_ops lpg_pwm_ops = { static int lpg_add_pwm(struct lpg *lpg) { struct pwm_chip *chip; + struct lpg_pwm_data *lpg_pwm_data; int ret; lpg->pwm = chip = devm_pwmchip_alloc(lpg->dev, lpg->num_channels, - sizeof(&lpg)); + sizeof(*lpg_pwm_data)); if (IS_ERR(chip)) return PTR_ERR(chip); - *(struct lpg **)pwmchip_priv(chip) = lpg; + lpg_pwm_data = pwmchip_priv(chip); + lpg_pwm_data->lpg = lpg; chip->ops = &lpg_pwm_ops; Would you like it better then? Best regards Uwe
On Wed, 22 Nov 2023, Uwe Kleine-König wrote: > Hello Lee, > > 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; > > > [...] > > > @@ -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! > > This is indeed one of the uglier conversions. It gets a bit prettier > with the following addon patch: > > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c > index 283227e02df6..e09eba823057 100644 > --- a/drivers/leds/rgb/leds-qcom-lpg.c > +++ b/drivers/leds/rgb/leds-qcom-lpg.c > @@ -206,6 +206,10 @@ struct lpg_data { > const struct lpg_channel_data *channels; > }; > > +struct lpg_pwm_data { > + struct lpg *lpg; > +}; > + > static int triled_set(struct lpg *lpg, unsigned int mask, unsigned int enable) > { > /* Skip if we don't have a triled block */ > @@ -979,8 +983,9 @@ static int lpg_pattern_mc_clear(struct led_classdev *cdev) > > static inline struct lpg *lpg_pwm_from_chip(struct pwm_chip *chip) > { > - struct lpg **lpg = pwmchip_priv(chip); > - return *lpg; > + struct lpg_pwm_data *lpg_pwm_data = pwmchip_priv(chip); > + > + return lpg_pwm_data->lpg; > } > > static int lpg_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > @@ -1096,14 +1101,16 @@ static const struct pwm_ops lpg_pwm_ops = { > static int lpg_add_pwm(struct lpg *lpg) > { > struct pwm_chip *chip; > + struct lpg_pwm_data *lpg_pwm_data; > int ret; > > lpg->pwm = chip = devm_pwmchip_alloc(lpg->dev, lpg->num_channels, > - sizeof(&lpg)); > + sizeof(*lpg_pwm_data)); > if (IS_ERR(chip)) > return PTR_ERR(chip); > > - *(struct lpg **)pwmchip_priv(chip) = lpg; > + lpg_pwm_data = pwmchip_priv(chip); > + lpg_pwm_data->lpg = lpg; > > chip->ops = &lpg_pwm_ops; > > Would you like it better then? It's definitely nicer to read and more in-line with the style I expect, but the additional wrapper/abstraction layer is still bothersome.
Hello Lee, On Thu, Nov 23, 2023 at 10:21:11AM +0000, Lee Jones wrote: > On Wed, 22 Nov 2023, Uwe Kleine-König wrote: > > Would you like it better then? > > It's definitely nicer to read and more in-line with the style I expect, > but the additional wrapper/abstraction layer is still bothersome. I guess that's subjective because I think having a separate pwm private data struct is nice. I don't see an immediate advantage for the leds-qcom-lpg driver, but the ti-sn65dsi86 driver could move some members of the parent driver to the pwm specific struct. Best regards Uwe
On Thu, Nov 23, 2023 at 11:44:58AM +0100, Uwe Kleine-König wrote: > Hello Thierry, > > [adding Bartosz to Cc] > > On Wed, Nov 22, 2023 at 06:15:32PM +0100, Thierry Reding wrote: > > On Wed, Nov 22, 2023 at 11:56:21AM +0000, Lee Jones wrote: > > > On Tue, 21 Nov 2023, Uwe Kleine-König wrote: > > > > + *(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. > > With the patch I suggested in reply to Lee's mail this is IMHO much > nicer and with that squashed into the patch under discussion I'd not > call this a work around. > > Note that the thing you consider ugly here (I think) is that for > handling a combined "PWM + something else" device a separate allocation > is needed for stuff that embedded a struct pwm_chip before. With > Bartosz's approach you have that second allocation for all PWM devices > ---and so the downsides hurt all PWM implementations and not only those > combined devices. > > Also note that among the four external PWM drivers (i.e. > > drivers/staging/greybus/pwm.c > drivers/leds/rgb/leds-qcom-lpg.c > drivers/gpu/drm/bridge/ti-sn65dsi86.c > drivers/gpio/gpio-mvebu.c > > ) only two suffer from this complication, because the other two use a > pwm specific private data structure already which seems natural to me. That's true for now, but new drivers get added all the time, so anything we do here should be as future proof as we can make it. > > 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. > > Where do you see a midlayer and how would that be better with what > Bartosz suggests? I wasn't saying that this was a midlayer but rather that it reminds me of one and the restrictions that it comes with. Right now all of these drivers work just fine and we don't need any of these weird assignments due to the single allocation. They all neatly plug into whatever other drivers or subsystems do. > The relevant difference between my approach and Bartosz's is that I put > the driver specific private data in the same allocation as the struct > pwm_chip and thus reducing the number of allocations and pointer > traversals. This difference IMHO doesn't qualify my approach as a > midlayer without Bartosz's qualifying, too. The solution that Bartosz proposed in his talk has two big advantages: it can potentially be generalized to a number of subsystems, which means that eventually we may get an actual library that would allow this stuff to be unified across subsystems without everyone having to invent their own and fix the same bugs. Secondly it also puts the lifetime management where it belongs: in the subsystem. Drivers don't really have to care about lifetime management of whatever they expose. When they are unloaded, they should only need to let the subsystem know that they're gone and then the subsystem can take appropriate action. There are other advantages as well, mostly derived from the above: the patch series to implement this can probably be something like 5 patches, so we don't actually need to touch every driver, because the drivers themselves are not the issue. It's how the subsystem will expose them via chardev (or already exposes them via sysfs) that's really the problem. The only place where it makes sense to fix this is in the subsystem. Drivers don't need to be concerned about this. Thierry
Hello Thierry, On Fri, Nov 24, 2023 at 01:27:21PM +0100, Thierry Reding wrote: > On Thu, Nov 23, 2023 at 11:44:58AM +0100, Uwe Kleine-König wrote: > > Hello Thierry, > > > > [adding Bartosz to Cc] > > > > On Wed, Nov 22, 2023 at 06:15:32PM +0100, Thierry Reding wrote: > > > On Wed, Nov 22, 2023 at 11:56:21AM +0000, Lee Jones wrote: > > > > On Tue, 21 Nov 2023, Uwe Kleine-König wrote: > > > > > + *(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. > > > > With the patch I suggested in reply to Lee's mail this is IMHO much > > nicer and with that squashed into the patch under discussion I'd not > > call this a work around. > > > > Note that the thing you consider ugly here (I think) is that for > > handling a combined "PWM + something else" device a separate allocation > > is needed for stuff that embedded a struct pwm_chip before. With > > Bartosz's approach you have that second allocation for all PWM devices > > ---and so the downsides hurt all PWM implementations and not only those > > combined devices. > > > > Also note that among the four external PWM drivers (i.e. > > > > drivers/staging/greybus/pwm.c > > drivers/leds/rgb/leds-qcom-lpg.c > > drivers/gpu/drm/bridge/ti-sn65dsi86.c > > drivers/gpio/gpio-mvebu.c > > > > ) only two suffer from this complication, because the other two use a > > pwm specific private data structure already which seems natural to me. > > That's true for now, but new drivers get added all the time, so anything > we do here should be as future proof as we can make it. If drivers are added with a sane separation between their functionality, my approach doesn't result in these complications. See https://lore.kernel.org/dri-devel/20231123175425.496956-1-u.kleine-koenig@pengutronix.de for how this could look. With that applied, the ti-sn65dsi86 driver can be nicely adapted. So yes, if you have an ugly driver, the pwm support cannot be prettier. I can live with that. You could even sell this as an advantage. > > > 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. > > > > Where do you see a midlayer and how would that be better with what > > Bartosz suggests? > > I wasn't saying that this was a midlayer but rather that it reminds me > of one and the restrictions that it comes with. > > Right now all of these drivers work just fine and we don't need any of > these weird assignments due to the single allocation. They all neatly > plug into whatever other drivers or subsystems do. Do you see the advantages of my approach, too? > > The relevant difference between my approach and Bartosz's is that I put > > the driver specific private data in the same allocation as the struct > > pwm_chip and thus reducing the number of allocations and pointer > > traversals. This difference IMHO doesn't qualify my approach as a > > midlayer without Bartosz's qualifying, too. > > The solution that Bartosz proposed in his talk has two big advantages: > it can potentially be generalized to a number of subsystems, which means > that eventually we may get an actual library that would allow this stuff > to be unified across subsystems without everyone having to invent their > own and fix the same bugs. Can you please point out the relevant difference between Bartosz's and my approach that makes his generalizable but not mine? Also I don't see much in the pwm core that could still benefit from such a generalisation. > Secondly it also puts the lifetime management > where it belongs: in the subsystem. Drivers don't really have to care > about lifetime management of whatever they expose. When they are > unloaded, they should only need to let the subsystem know that they're > gone and then the subsystem can take appropriate action. I understand your words, but I don't see that critic to apply to my patch set. Handling consumers of unloaded drivers is completely in the core. If you don't agree, can you please point your finger on any of the drivers adapted here that I might understand what you mean? (OK, we need a one-time conversion of all drivers to an abstraction that allows the core to handle the lifetime management. That's something that my approach has in common with Bartosz's.) > There are other advantages as well, mostly derived from the above: the > patch series to implement this can probably be something like 5 patches, > so we don't actually need to touch every driver, because the drivers > themselves are not the issue. While I don't think that the number of patches to reach a goal is a good objective to judge the result of a patch set: We won't go down to 5. We would still need to adapt every driver as they all assign struct pwm_chip::dev. > It's how the subsystem will expose them > via chardev (or already exposes them via sysfs) that's really the > problem. The only place where it makes sense to fix this is in the > subsystem. Drivers don't need to be concerned about this. This is another critic I don't understand. I agree it would be a relevant issue if it applied. But the chardev stuff is completely in the core. I invested much thought, time and effort into this series. I'm convinced it is a good one improving the pwm framework. I aligned the implementation ideas to what several other frameworks do---I'm aware of counter, iio, net, rtc, siox and spi that all use this idiom. I grepped a bit around and found some more using the _alloc pattern: amba, drm, hid, infiniband, input, libata. I also found some that don't: - rpmsg: but that seems to rely on the lowlevel drivers to get the lifetime stuff right (look at mtk_rpmsg.c). - i2c/i3c: has lifetime issues (though I think they are all "properly" worked around) - gpio: See how both gpio_chip and gpio_device have base, ngpio, a parent pointer (gpio_device has it in .dev), an owner and a label. Do they all have the same semantic? Yes? -> that's bad. No? -> that's IMHO even worse. And now I'm supposed to rework my patch set to a different abstraction because of some vapour ware resource lib that probably involves some data duplication (see gpio above) and more overhead in the source *and* the binaries because we need more pointer dereferences? Honestly? That's really frustrating. Can I please invest some of the karma I earned by caring for the pwm subsystem to align it to how other major subsystems work? Best regards Uwe
On Fri, Nov 24, 2023 at 7:22 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > [snip] > - gpio: See how both gpio_chip and gpio_device have base, ngpio, a > parent pointer (gpio_device has it in .dev), an owner and a label. > Do they all have the same semantic? Yes? -> that's bad. No? -> > that's IMHO even worse. For the record: I am very well aware there are a lot of things wrong with GPIO at the moment. It's years of technical debt biting back. I've racked up ~100 patches last release cycle alone fixing various cases of abuse of GPIOLIB in the wildest places (OMAP1?!). I'm slowly doing cleanups all around the place but since GPIO is so ubiquitous across the kernel tree, it's going quite slow. I'm going to get to the duplication between gpio_chip and gpio_device but we still have users in the kernel who shamelessly access gpio_chip directly without being GPIO providers. This is not an argument against the general approach I presented - it's an argument against stacking up years' worth of cruft. Bart
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; +} + 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; + + chip->ops = &lpg_pwm_ops; + + ret = pwmchip_add(chip); if (ret) dev_err(lpg->dev, "failed to add PWM chip: ret %d\n", ret); @@ -1367,7 +1379,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 = {
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(-)