From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Thu, 31 Mar 2016 09:07:09 +0200 From: Boris Brezillon To: Guenter Roeck Cc: Thierry Reding , linux-pwm@vger.kernel.org, Mike Turquette , Stephen Boyd , linux-clk@vger.kernel.org, Mark Brown , Liam Girdwood , Kamil Debski , lm-sensors@lm-sensors.org, Jean Delvare , Dmitry Torokhov , linux-input@vger.kernel.org, Bryan Wu , Richard Purdie , Jacek Anaszewski , linux-leds@vger.kernel.org, Maxime Ripard , Chen-Yu Tsai , linux-sunxi@googlegroups.com, Joachim Eastwood , Thomas Petazzoni , Heiko Stuebner , linux-rockchip@lists.infradead.org, Jingoo Han , Lee Jones , linux-fbdev@vger.kernel.org, Jean-Christophe Plagniol-Villard , Tomi Valkeinen , Robert Jarzmik , Alexandre Belloni , Kukjin Kim , Krzysztof Kozlowski , linux-samsung-soc@vger.kernel.org, intel-gfx@lists.freedesktop.org, Daniel Vetter , Jani Nikula , Jonathan Corbet , linux-doc@vger.kernel.org, David Airlie , Daniel Vetter , dri-devel@lists.freedesktop.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Hartley Sweeten , Ryan Mallon , Alexander Shiyan , Milo Kim Subject: Re: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate Message-ID: <20160331090709.5c3bdf70@bbrezillon> In-Reply-To: <20160330225244.GA24044@roeck-us.net> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII List-ID: Hi Guenter, On Wed, 30 Mar 2016 15:52:44 -0700 Guenter Roeck wrote: > 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. > > > > Use pwm_get_args() when the PWM user wants to retrieve this reference > > config and not the current state. > > > > 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()). > > > > Signed-off-by: Boris Brezillon > > --- > > drivers/hwmon/pwm-fan.c | 19 +++++++++++++------ > > 1 file changed, 13 insertions(+), 6 deletions(-) > > > > 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 { > > > > static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) > > { > > + struct pwm_args pargs = { }; > > Hi Boris, > > I guess I am missing some context; sorry for that. Unfortunately, > I did not easily find an explanation, so please bear with me. > > 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 ? 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. Thierry, what's your opinion? > > Also, assuming the local copy is necessary, why initialize pargs ? > After all, pwm_get_args() just overwrites it. It's a leftover from a previous version where pwm_get_args was implemented this way: static inline void pwm_get_args(pwm, args) { if (pwm) *args = pwm->args } and this implementation was generating a lot of 'uninitialized variable' warnings. I just decided to drop the 'if (pwm)' test, because, IMO, this should be checked way before calling pwm_get_args() is called. Anyway, apparently I forgot to modify a few patches after this modification. Thanks for the review. Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Date: Thu, 31 Mar 2016 07:07:09 +0000 Subject: Re: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate Message-Id: <20160331090709.5c3bdf70@bbrezillon> 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> In-Reply-To: <20160330225244.GA24044@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Guenter Roeck Cc: Milo Kim , Kamil Debski , Heiko Stuebner , linux-doc@vger.kernel.org, David Airlie , Mike Turquette , linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-sunxi@googlegroups.com, Thierry Reding , Alexandre Belloni , Daniel Vetter , Lee Jones , linux-clk@vger.kernel.org, linux-leds@vger.kernel.org, Krzysztof Kozlowski , linux-samsung-soc@vger.kernel.org, Alexander Shiyan , Jonathan Corbet , Robert Jarzmik , lm-sensors@lm-sensors.org, linux-rockchip@lists.infradead.org, Chen-Yu Tsai , Tomi Valkeinen Hi Guenter, On Wed, 30 Mar 2016 15:52:44 -0700 Guenter Roeck wrote: > 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. > > > > Use pwm_get_args() when the PWM user wants to retrieve this reference > > config and not the current state. > > > > 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()). > > > > Signed-off-by: Boris Brezillon > > --- > > drivers/hwmon/pwm-fan.c | 19 +++++++++++++------ > > 1 file changed, 13 insertions(+), 6 deletions(-) > > > > 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 { > > > > static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) > > { > > + struct pwm_args pargs = { }; > > Hi Boris, > > I guess I am missing some context; sorry for that. Unfortunately, > I did not easily find an explanation, so please bear with me. > > 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 ? 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. Thierry, what's your opinion? > > Also, assuming the local copy is necessary, why initialize pargs ? > After all, pwm_get_args() just overwrites it. It's a leftover from a previous version where pwm_get_args was implemented this way: static inline void pwm_get_args(pwm, args) { if (pwm) *args = pwm->args } and this implementation was generating a lot of 'uninitialized variable' warnings. I just decided to drop the 'if (pwm)' test, because, IMO, this should be checked way before calling pwm_get_args() is called. Anyway, apparently I forgot to modify a few patches after this modification. Thanks for the review. Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate Date: Thu, 31 Mar 2016 09:07:09 +0200 Message-ID: <20160331090709.5c3bdf70@bbrezillon> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160330225244.GA24044@roeck-us.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Guenter Roeck Cc: Milo Kim , Kamil Debski , Heiko Stuebner , linux-doc@vger.kernel.org, David Airlie , Mike Turquette , linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-sunxi@googlegroups.com, Thierry Reding , Alexandre Belloni , Daniel Vetter , Lee Jones , linux-clk@vger.kernel.org, linux-leds@vger.kernel.org, Krzysztof Kozlowski , linux-samsung-soc@vger.kernel.org, Alexander Shiyan , Jonathan Corbet , Robert Jarzmik , lm-sensors@lm-sensors.org, linux-rockchip@lists.infradead.org, Chen-Yu Tsai , Tomi Valkeinen l List-Id: linux-input@vger.kernel.org SGkgR3VlbnRlciwKCk9uIFdlZCwgMzAgTWFyIDIwMTYgMTU6NTI6NDQgLTA3MDAKR3VlbnRlciBS b2VjayA8bGludXhAcm9lY2stdXMubmV0PiB3cm90ZToKCj4gT24gV2VkLCBNYXIgMzAsIDIwMTYg YXQgMTA6MDM6MzFQTSArMDIwMCwgQm9yaXMgQnJlemlsbG9uIHdyb3RlOgo+ID4gVGhlIFBXTSBm cmFtZXdvcmsgaGFzIGNsYXJpZmllZCB0aGUgY29uY2VwdCBvZiByZWZlcmVuY2UgUFdNIGNvbmZp Zwo+ID4gKHRoZSBwbGF0Zm9ybSBkZXBlbmRlbnQgY29uZmlnIHJldHJpZXZlZCBmcm9tIHRoZSBE VCBvciB0aGUgUFdNCj4gPiBsb29rdXAgdGFibGUpIGFuZCByZWFsIFBXTSBzdGF0ZS4KPiA+IAo+ ID4gVXNlIHB3bV9nZXRfYXJncygpIHdoZW4gdGhlIFBXTSB1c2VyIHdhbnRzIHRvIHJldHJpZXZl IHRoaXMgcmVmZXJlbmNlCj4gPiBjb25maWcgYW5kIG5vdCB0aGUgY3VycmVudCBzdGF0ZS4KPiA+ IAo+ID4gVGhpcyBpcyBwYXJ0IG9mIHRoZSByZXdvcmsgYWxsb3dpbmcgdGhlIFBXTSBmcmFtZXdv cmsgdG8gc3VwcG9ydAo+ID4gaGFyZHdhcmUgcmVhZG91dCBhbmQgZXhwb3NlIHJlYWwgUFdNIHN0 YXRlIGV2ZW4gd2hlbiB0aGUgUFdNIGhhcwo+ID4ganVzdCBiZWVuIHJlcXVlc3RlZCAoYmVmb3Jl IHRoZSB1c2VyIGNhbGxzIHB3bV9jb25maWcvZW5hYmxlL2Rpc2FibGUoKSkuCj4gPiAKPiA+IFNp Z25lZC1vZmYtYnk6IEJvcmlzIEJyZXppbGxvbiA8Ym9yaXMuYnJlemlsbG9uQGZyZWUtZWxlY3Ry b25zLmNvbT4KPiA+IC0tLQo+ID4gIGRyaXZlcnMvaHdtb24vcHdtLWZhbi5jIHwgMTkgKysrKysr KysrKysrKy0tLS0tLQo+ID4gIDEgZmlsZSBjaGFuZ2VkLCAxMyBpbnNlcnRpb25zKCspLCA2IGRl bGV0aW9ucygtKQo+ID4gCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9od21vbi9wd20tZmFuLmMg Yi9kcml2ZXJzL2h3bW9uL3B3bS1mYW4uYwo+ID4gaW5kZXggM2UyMzAwMy4uODJjNTY1NiAxMDA2 NDQKPiA+IC0tLSBhL2RyaXZlcnMvaHdtb24vcHdtLWZhbi5jCj4gPiArKysgYi9kcml2ZXJzL2h3 bW9uL3B3bS1mYW4uYwo+ID4gQEAgLTQwLDE1ICs0MCwxOCBAQCBzdHJ1Y3QgcHdtX2Zhbl9jdHgg ewo+ID4gIAo+ID4gIHN0YXRpYyBpbnQgIF9fc2V0X3B3bShzdHJ1Y3QgcHdtX2Zhbl9jdHggKmN0 eCwgdW5zaWduZWQgbG9uZyBwd20pCj4gPiAgewo+ID4gKwlzdHJ1Y3QgcHdtX2FyZ3MgcGFyZ3Mg PSB7IH07Cj4gCj4gSGkgQm9yaXMsCj4gCj4gSSBndWVzcyBJIGFtIG1pc3Npbmcgc29tZSBjb250 ZXh0OyBzb3JyeSBmb3IgdGhhdC4gVW5mb3J0dW5hdGVseSwKPiBJIGRpZCBub3QgZWFzaWx5IGZp bmQgYW4gZXhwbGFuYXRpb24sIHNvIHBsZWFzZSBiZWFyIHdpdGggbWUuCj4gCj4gVHdvIHF1ZXN0 aW9uczogV2h5IGRvIHdlIG5lZWQgYSBsb2NhbCBjb3B5IG9mIHN0cnVjdCBwd21fYXJncyBpbnN0 ZWFkCj4gb2YgYSBwb2ludGVyIHRvIGl0ID8gSWYgaXQgY2FuIGNoYW5nZSB3aGlsZSBiZWluZyB1 c2VkLCBpc24ndCBpdAo+IGluY29uc2lzdGVudCBhbnl3YXkgPwoKSXQgY2Fubm90IGNoYW5nZSBh ZnRlciBwd21fZ2V0KCkgaXMgY2FsbGVkLiBGb3IgdGhlIHJlYXNvbiBiZWhpbmQKcHJvdG90eXBl OiBJIGp1c3QgZm9sbG93ZWQgdGhlIFRoaWVycnkncyBwcm9wb3NhbCwgYnV0IEknbSBwZXJmZWN0 bHkKZmluZSByZXR1cm5pbmcgYSBjb25zdCBzdHJ1Y3QgcHdtX2FyZ3MgcG9pbnRlciBpbnRlYWQg b2YgcGFzc2luZwpwd21fYXJncyBhcyBhIHBhcmFtZXRlci4KClRoaWVycnksIHdoYXQncyB5b3Vy IG9waW5pb24/CgoKPiAKPiBBbHNvLCBhc3N1bWluZyB0aGUgbG9jYWwgY29weSBpcyBuZWNlc3Nh cnksIHdoeSBpbml0aWFsaXplIHBhcmdzID8gCj4gQWZ0ZXIgYWxsLCBwd21fZ2V0X2FyZ3MoKSBq dXN0IG92ZXJ3cml0ZXMgaXQuCgpJdCdzIGEgbGVmdG92ZXIgZnJvbSBhIHByZXZpb3VzIHZlcnNp b24gd2hlcmUgcHdtX2dldF9hcmdzIHdhcwppbXBsZW1lbnRlZCB0aGlzIHdheToKCnN0YXRpYyBp bmxpbmUgdm9pZCBwd21fZ2V0X2FyZ3MocHdtLCBhcmdzKQp7CglpZiAocHdtKQoJCSphcmdzID0g cHdtLT5hcmdzCn0KCmFuZCB0aGlzIGltcGxlbWVudGF0aW9uIHdhcyBnZW5lcmF0aW5nIGEgbG90 IG9mICd1bmluaXRpYWxpemVkCnZhcmlhYmxlJyB3YXJuaW5ncy4KCkkganVzdCBkZWNpZGVkIHRv IGRyb3AgdGhlICdpZiAocHdtKScgdGVzdCwgYmVjYXVzZSwgSU1PLCB0aGlzCnNob3VsZCBiZSBj aGVja2VkIHdheSBiZWZvcmUgY2FsbGluZyBwd21fZ2V0X2FyZ3MoKSBpcyBjYWxsZWQuCgpBbnl3 YXksIGFwcGFyZW50bHkgSSBmb3Jnb3QgdG8gbW9kaWZ5IGEgZmV3IHBhdGNoZXMgYWZ0ZXIgdGhp cwptb2RpZmljYXRpb24uCgpUaGFua3MgZm9yIHRoZSByZXZpZXcuCgpCb3JpcwoKCi0tIApCb3Jp cyBCcmV6aWxsb24sIEZyZWUgRWxlY3Ryb25zCkVtYmVkZGVkIExpbnV4IGFuZCBLZXJuZWwgZW5n aW5lZXJpbmcKaHR0cDovL2ZyZWUtZWxlY3Ryb25zLmNvbQpfX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fXwpJbnRlbC1nZnggbWFpbGluZyBsaXN0CkludGVsLWdm eEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFp bG1hbi9saXN0aW5mby9pbnRlbC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 From: boris.brezillon@free-electrons.com (Boris Brezillon) Date: Thu, 31 Mar 2016 09:07:09 +0200 Subject: [PATCH v5 08/46] hwmon: pwm-fan: use pwm_get_args() where appropriate In-Reply-To: <20160330225244.GA24044@roeck-us.net> 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> Message-ID: <20160331090709.5c3bdf70@bbrezillon> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Guenter, On Wed, 30 Mar 2016 15:52:44 -0700 Guenter Roeck wrote: > 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. > > > > Use pwm_get_args() when the PWM user wants to retrieve this reference > > config and not the current state. > > > > 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()). > > > > Signed-off-by: Boris Brezillon > > --- > > drivers/hwmon/pwm-fan.c | 19 +++++++++++++------ > > 1 file changed, 13 insertions(+), 6 deletions(-) > > > > 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 { > > > > static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) > > { > > + struct pwm_args pargs = { }; > > Hi Boris, > > I guess I am missing some context; sorry for that. Unfortunately, > I did not easily find an explanation, so please bear with me. > > 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 ? 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. Thierry, what's your opinion? > > Also, assuming the local copy is necessary, why initialize pargs ? > After all, pwm_get_args() just overwrites it. It's a leftover from a previous version where pwm_get_args was implemented this way: static inline void pwm_get_args(pwm, args) { if (pwm) *args = pwm->args } and this implementation was generating a lot of 'uninitialized variable' warnings. I just decided to drop the 'if (pwm)' test, because, IMO, this should be checked way before calling pwm_get_args() is called. Anyway, apparently I forgot to modify a few patches after this modification. Thanks for the review. Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com