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