linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Lee Jones <lee.jones@linaro.org>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org,
	kernel@pengutronix.de, "Michael Walle" <michael@walle.cc>
Subject: Re: [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks
Date: Fri, 4 Dec 2020 13:24:36 +0000	[thread overview]
Message-ID: <20201204132436.GO4801@dell> (raw)
In-Reply-To: <X8oubGP9CvoOQKtF@ulmo>

On Fri, 04 Dec 2020, 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 <michael@walle.cc>
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@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-koenig@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.
> 
> 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.
> 
> 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.

Don't worry, you are (probably!) still quite sane.

The discussion happened over IRC.  I highlighted my concerns, but Uwe
didn't respond to them.  This patch was the next time I saw anything
on the subject.

> 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.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

  reply	other threads:[~2020-12-04 13:25 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-03  8:41 Uwe Kleine-König
2020-12-04 12:41 ` Thierry Reding
2020-12-04 13:24   ` Lee Jones [this message]
2020-12-04 14:03     ` Uwe Kleine-König
2020-12-04 13:51   ` Uwe Kleine-König
2020-12-04 16:08     ` Thierry Reding
2020-12-04 20:06       ` Uwe Kleine-König

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201204132436.GO4801@dell \
    --to=lee.jones@linaro.org \
    --cc=kernel@pengutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=michael@walle.cc \
    --cc=thierry.reding@gmail.com \
    --cc=torvalds@linux-foundation.org \
    --cc=u.kleine-koenig@pengutronix.de \
    --subject='Re: [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).