From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Date: Mon, 04 Apr 2016 15:20:48 +0000 Subject: Re: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate Message-Id: <20160404152048.GA17856@ulmo> MIME-Version: 1 Content-Type: multipart/mixed; boundary="PNTmBPCT7hxwcZjr" List-Id: References: <1459368249-13241-1-git-send-email-boris.brezillon@free-electrons.com> <1459368249-13241-9-git-send-email-boris.brezillon@free-electrons.com> <20160330225244.GA24044@roeck-us.net> <20160331090709.5c3bdf70@bbrezillon> In-Reply-To: <20160331090709.5c3bdf70@bbrezillon> To: Boris Brezillon Cc: Guenter Roeck , linux-pwm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mike Turquette , Stephen Boyd , linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mark Brown , Liam Girdwood , Kamil Debski , lm-sensors-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org, Jean Delvare , Dmitry Torokhov , linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Bryan Wu , Richard Purdie , Jacek Anaszewski , linux-leds-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Maxime Ripard , Chen-Yu Tsai , linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, Joachim Eastwood , Thomas Petazzoni , Heiko Stuebner , linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Jingoo Han , Lee Jones --PNTmBPCT7hxwcZjr Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 31, 2016 at 09:07:09AM +0200, Boris Brezillon wrote: > Hi Guenter, >=20 > On Wed, 30 Mar 2016 15:52:44 -0700 > Guenter Roeck wrote: >=20 > > On Wed, Mar 30, 2016 at 10:03:31PM +0200, Boris Brezillon wrote: > > > The PWM framework has clarified the concept of reference PWM config > > > (the platform dependent config retrieved from the DT or the PWM > > > lookup table) and real PWM state. > > >=20 > > > Use pwm_get_args() when the PWM user wants to retrieve this reference > > > config and not the current state. > > >=20 > > > This is part of the rework allowing the PWM framework to support > > > hardware readout and expose real PWM state even when the PWM has > > > just been requested (before the user calls pwm_config/enable/disable(= )). > > >=20 > > > Signed-off-by: Boris Brezillon > > > --- > > > drivers/hwmon/pwm-fan.c | 19 +++++++++++++------ > > > 1 file changed, 13 insertions(+), 6 deletions(-) > > >=20 > > > diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c > > > index 3e23003..82c5656 100644 > > > --- a/drivers/hwmon/pwm-fan.c > > > +++ b/drivers/hwmon/pwm-fan.c > > > @@ -40,15 +40,18 @@ struct pwm_fan_ctx { > > > =20 > > > static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) > > > { > > > + struct pwm_args pargs =3D { }; > >=20 > > Hi Boris, > >=20 > > I guess I am missing some context; sorry for that. Unfortunately, > > I did not easily find an explanation, so please bear with me. > >=20 > > Two questions: Why do we need a local copy of struct pwm_args instead > > of a pointer to it ? If it can change while being used, isn't it > > inconsistent anyway ? >=20 > It cannot change after pwm_get() is called. For the reason behind > prototype: I just followed the Thierry's proposal, but I'm perfectly > fine returning a const struct pwm_args pointer intead of passing > pwm_args as a parameter. >=20 > Thierry, what's your opinion? I do prefer the current variant because it is more consistent with the new atomic API, even if not strictly necessary because of the immutable data. > > Also, assuming the local copy is necessary, why initialize pargs ?=20 > > After all, pwm_get_args() just overwrites it. >=20 > It's a leftover from a previous version where pwm_get_args was > implemented this way: >=20 > static inline void pwm_get_args(pwm, args) > { > if (pwm) > *args =3D pwm->args > } >=20 > and this implementation was generating a lot of 'uninitialized > variable' warnings. >=20 > I just decided to drop the 'if (pwm)' test, because, IMO, this > should be checked way before calling pwm_get_args() is called. Sounds fine to me. Thierry --PNTmBPCT7hxwcZjr Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAABCAAGBQJXAoZLAAoJEN0jrNd/PrOhsEQQAKKy319NRaO6e/ciYlg6vDDN rdmFDoQgotIfLetr3KUFRP95Q3Cldg3cRW9oEg+OH8Uhf84Wg6Ug+glGDCmZ5jtl MNH8yavaO/eE0Ai1uV9rv7Nv6uJKoh+P11aQ7ht6HHr7+Iim8D6HGxdCEnqcxYz/ Np9mk3jc2pj4FYxY8t8B4xuQS9/sbxKpCRRcQUDXVLnun+88Q+hWPlYA6ypTtQZ3 deJzpGD3vWZIPtF1ZCm1U6gLetexRC84wyKmVGvV1yaeXaiXnr13MbUn2YbtBRFC oYRkNh59URh9bflVUnBORWi0Z9W7JBOt9f9pDUcOnY600rxhn0fXjt5aStdp6bDQ WHG/8pXEk5SdwU9PNDUPxhmfM3kO8a3v4KczSthjsUaMBd/76x1TroWTeSk4kuLD yEP3ZUP4fPb4hrE7Jvd5ruDnYgv0Vtk2MOa8IssefGYSO36pKTRyBCw7LKwmp+ww oFfZMR/KESTFCK59URYElJJHCUOWu3/u4TilXnimjRv2Co6HGdhZj5RTFhe1R3NE jO+k3ods1O9oVrVLmteHoePGnt6PCkPR/1szu5xW8XoFgL/58EWusji+XbMOlAlJ VJLrWoH2rOUW2/AEKvlasgeUOYFEszRM+vRyId/4awPJhhX5GVV3f+nY5UZLfW5I aPz78WqEqGpe2nzBw+67 =pF1x -----END PGP SIGNATURE----- --PNTmBPCT7hxwcZjr--