Hello Thierry,
On Fri, Dec 04, 2020 at 01:41:16PM +0100, Thierry Reding wrote:
> On Thu, Dec 03, 2020 at 09:41:42AM +0100, Uwe Kleine-König wrote:
> > Currently .get_state() and .apply() use dev_get_drvdata() on the struct
> > device related to the pwm chip. This only works after .probe() called
> > platform_set_drvdata() which in this driver happens only after
> > pwmchip_add() and so comes possibly too late.
> >
> > Instead of setting the driver data earlier use the traditional
> > container_of approach as this way the driver data is conceptually and
> > computational nearer.
> >
> > Fixes: 9db33d221efc ("pwm: Add support for sl28cpld PWM controller")
> > Tested-by: Michael Walle <mich...@walle.cc>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koe...@pengutronix.de>
> > ---
> > Hello Linus,
> >
> > Thierry (who usually sends PWM patches to you) didn't react to this
> > patch sent to the pwm Mailinglist last week
> > (https://lore.kernel.org/r/20201124212432.3117322-1-u.kleine-koe...@pengutronix.de)
> > yet.
> >
> > Given v5.10 isn't far away any more and I don't know when Thierry will
> > take a look and act, I'm sending this directly to you. The affected
> > driver was new in 5.10-rc1 and at least once the unpatched driver
> > created an oops:
> >
> > https://lavalab.kontron.com/scheduler/job/108#L950
> >
> > Michael Walle who tested this patch is the original author of the
> > driver. IMHO it would be good to have this fixed before 5.10.
> >
> > If you prefer a pull request, I can setup something (but I don't have
> > access to Thierry's tree, so it will be for a repository that's new to
> > you).
> >
> > Best regards
> > Uwe
> >
> > drivers/pwm/pwm-sl28cpld.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
>
> I thought I had seen you discuss this with Lee and gotten the impression
> that you were going to respin this to move the platform_set_drvdata() to
> an earlier point, which I think is the more correct approach.Lee asked on irc why I didn't move the platform_set_drvdata to an earlier stage and I told him why. Then the conversation was over. > container_of() isn't exactly wrong, but it's really just papering over > the fact that platform_set_drvdata() is in the wrong place, so I'd > prefer a patch that does that instead. platfrom_set_drvdata is in a perfectly fine position if you only rely on it in the platform_driver's remove callback which is the case with my patch. I wrote in my commit log | Instead of setting the driver data earlier use the traditional | container_of approach as this way the driver data is conceptually and | computational nearer. which is still think to be true. The main thing I don't like about the platform_set_drvdata approach is that you have to rely on dev_get_drvdata() returning the value set with platform_set_drvdata() which IMHO is an implementation detail of the platform driver code. > Now, I can no longer find a link to the discussion that I recall, so it > was either on IRC (where I don't have any logs) or I'm loosing my mind. It was on IRC but I thought to have written an email about this, too. But I don't find it either. > I'll prepare a patch that moves platform_set_drvdata() for Michael to > test. If that works I'll send a PR with fixes to Linus early next week. You're late, Linus already merged my patch. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
signature.asc
Description: PGP signature
