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. > 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? 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. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
