Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759251AbbBIEkk (ORCPT ); Sun, 8 Feb 2015 23:40:40 -0500 Received: from bh-25.webhostbox.net ([208.91.199.152]:56853 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759078AbbBIEki (ORCPT ); Sun, 8 Feb 2015 23:40:38 -0500 Message-ID: <54D83A2B.3070601@roeck-us.net> Date: Sun, 08 Feb 2015 20:40:11 -0800 From: Guenter Roeck User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Lukasz Majewski CC: Lukasz Majewski , Eduardo Valentin , Kamil Debski , Jean Delvare , Kukjin Kim , lm-sensors@lm-sensors.org, Linux PM list , "linux-samsung-soc@vger.kernel.org" , devicetree@vger.kernel.org, Kukjin Kim , linux-kernel@vger.kernel.org, Sjoerd Simons , Abhilash Kesavan , Abhilash Kesavan Subject: Re: [PATCH v3 7/8] hwmon: pwm-fan: Read PWM FAN configuration from device tree References: <1418897591-18332-1-git-send-email-l.majewski@samsung.com> <1423241948-31981-1-git-send-email-l.majewski@samsung.com> <1423241948-31981-8-git-send-email-l.majewski@samsung.com> <20150206183657.GB25410@roeck-us.net> <20150208223640.2bcab3f4@jawa> In-Reply-To: <20150208223640.2bcab3f4@jawa> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Authenticated_sender: linux@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-CTCH-PVer: 0000001 X-CTCH-Spam: Unknown X-CTCH-VOD: Unknown X-CTCH-Flags: 0 X-CTCH-RefID: str=0001.0A020204.54D83A45.0172,ss=1,re=0.001,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0 X-CTCH-Score: 0.001 X-CTCH-ScoreCust: 0.000 X-CTCH-Rules: C_4847, X-CTCH-SenderID: linux@roeck-us.net X-CTCH-SenderID-Flags: 0 X-CTCH-SenderID-TotalMessages: 4 X-CTCH-SenderID-TotalSpam: 0 X-CTCH-SenderID-TotalSuspected: 0 X-CTCH-SenderID-TotalConfirmed: 0 X-CTCH-SenderID-TotalBulk: 0 X-CTCH-SenderID-TotalVirus: 0 X-CTCH-SenderID-TotalRecipients: 0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: mailgid no entry from get_relayhosts_entry X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4216 Lines: 121 On 02/08/2015 01:36 PM, Lukasz Majewski wrote: > On Fri, 6 Feb 2015 10:36:57 -0800 > Guenter Roeck wrote: > >> On Fri, Feb 06, 2015 at 05:59:07PM +0100, Lukasz Majewski wrote: >>> This patch provides code for reading PWM FAN configuration data via >>> device tree. The pwm-fan can work with full speed when configuration >>> is not provided. However, errors are propagated when wrong DT >>> bindings are found. >>> Additionally the struct pwm_fan_ctx has been extended. >>> >>> Signed-off-by: Lukasz Majewski >>> --- >>> Changes for v2: >>> - Rename pwm_fan_max_states to pwm_fan_cooling_levels >>> - Moving pwm_fan_of_get_cooling_data() call after setting end >>> enabling PWM FAN >>> - pwm_fan_of_get_cooling_data() now can fail - preserving old >>> behaviour >>> - Remove unnecessary dev_err() call >>> Changes for v3: >>> - Patch's headline has been reedited >>> - pwm_fan_of_get_cooling_data() return code is now being checked. >>> - of_property_count_elems_of_size() is now used instead >>> of_find_property() >>> - More verbose patch description added >>> --- >>> drivers/hwmon/pwm-fan.c | 54 >>> ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, >>> 53 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c >>> index 870e100..f3f5843 100644 >>> --- a/drivers/hwmon/pwm-fan.c >>> +++ b/drivers/hwmon/pwm-fan.c >>> @@ -30,7 +30,10 @@ >>> struct pwm_fan_ctx { >>> struct mutex lock; >>> struct pwm_device *pwm; >>> - unsigned char pwm_value; >>> + unsigned int pwm_value; >>> + unsigned int pwm_fan_state; >>> + unsigned int pwm_fan_max_state; >>> + unsigned int *pwm_fan_cooling_levels; >>> }; >>> >>> static int __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm) >>> @@ -100,6 +103,50 @@ static struct attribute *pwm_fan_attrs[] = { >>> >>> ATTRIBUTE_GROUPS(pwm_fan); >>> >>> +int pwm_fan_of_get_cooling_data(struct device *dev, struct >>> pwm_fan_ctx *ctx) +{ >>> + struct device_node *np = dev->of_node; >>> + int num, i, ret; >>> + >>> + ret = of_property_count_elems_of_size(np, "cooling-levels", >>> + sizeof(u32)); >>> + >>> + if (ret == -EINVAL) { >>> + dev_err(dev, "Property 'cooling-levels' not >>> found\n"); >>> + return 0; >> >> Returning 0 is obviously not an error, otherwise you would not return >> 0 here. So dev_err is wrong. > > pr_info would be more appropriate here. > >> Also, the message itself is wrong; the >> property may well be there but have a wrong size. > > As fair as I remember the -EINVAL is set only when the property is not > defined. Such situation is correct and our pwm-fan driver should work > with or without it. > of_property_count_elems_of_size returns -EINVAL if np is NULL or if there is no peoperty or prop->length % elem_size != 0, and -ENODATA if there is no value associated with the property. If -EINVAL is not an error, please no message. Noisy drivers are just that, noisy. >> >>> + } >>> + >>> + if (ret <= 0) { >>> + dev_err(dev, "Wrong data!\n"); >>> + return ret; >>> + } >> >> This will result in the driver failing to load if devicetree is not >> configured (of_property_count_elems_of_size will return -ENOSYS). > > As you pointed out in the comment to v2: > > It is OK if the "cooling-levels" is not defined in DT. However, if it > has broken definition, then we should return error. This is what we do > here. > You don't return an error, you return 0, at least in some circumstances. >> This is not acceptable. Also, if the call returns 0 you don't return >> an error but display a "Wrong data!" error message. Either it is an >> error or it is not, but not both. > > This is an error. "cooling-levels" with zero elements is regarded as a > broken property. > It returns -ENOSYS if DT is not configured, which is still unacceptable. And, again, if ret == 0 is an error, you should return an error code, not display an error message and return 0. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/