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: 4009 bytes --]

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

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