Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754345AbbGCB1D (ORCPT ); Thu, 2 Jul 2015 21:27:03 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:60873 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754442AbbGCB04 (ORCPT ); Thu, 2 Jul 2015 21:26:56 -0400 Message-ID: <5595E4DB.6080403@roeck-us.net> Date: Thu, 02 Jul 2015 18:26:51 -0700 From: Guenter Roeck User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Constantine Shulyupin , jdelvare@suse.de, lm-sensors@lm-sensors.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] hwmon: (nct7802) Add fan output control References: <1435876843-1216-1-git-send-email-const@MakeLinux.com> In-Reply-To: <1435876843-1216-1-git-send-email-const@MakeLinux.com> 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-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: authenticated_id: linux@roeck-us.net 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: 4430 Lines: 147 Hi Constantine, On 07/02/2015 03:40 PM, Constantine Shulyupin wrote: > Notes: > > Some lines intentionally more than 80 characters because wrapping > this lines decreases readability. > The patch was checked with scripts/checkpatch.pl --max-line-length=88 > > Following code is not typecasting. > It is inline struct attribute_group definition using compound literal. > (See https://gcc.gnu.org/onlinedocs/gcc-3.2.2/gcc/Compound-Literals.html) > > static const struct attribute_group *nct7802_groups[] = { > ... > &(const struct attribute_group){ .attrs = fan_output_attributes }, > ... > I still don't like it. See below. > Reference: > > Nuvoton Hardware Monitoring IC NCT7802Y > 7.2.91 Fan Control 1 Output Value > Location : Index 60h > > 7.2.92 Fan Control 2 Output Value > Location : Index 61h > > 7.2.93 Fan Control 3 Output Value > Location : Index 62h > This all doesn't really belong into the change log. > Signed-off-by: Constantine Shulyupin > --- > drivers/hwmon/nct7802.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 45 insertions(+) > > diff --git a/drivers/hwmon/nct7802.c b/drivers/hwmon/nct7802.c > index afa242d..56d3547 100644 > --- a/drivers/hwmon/nct7802.c > +++ b/drivers/hwmon/nct7802.c > @@ -102,6 +102,38 @@ static ssize_t store_temp_type(struct device *dev, > return err ? : count; > } > > +static ssize_t show_dec(struct device *dev, struct device_attribute *devattr, char *buf) Please make this show_pwm. > +{ > + int ret = -1; > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > + struct nct7802_data *data = dev_get_drvdata(dev); > + unsigned int val = 0; > + > + ret = regmap_read(data->regmap, attr->index, &val); > + if (ret < 0) > + return ret; > + > + ret = sprintf(buf, "%d\n", val); > + return ret; return sprintf(buf, "%d\n", val); > +} > + > +static ssize_t store_u8(struct device *dev, struct device_attribute *devattr, > + const char *buf, size_t count) store_pwm, and please align the continuation line with '('. > +{ > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > + struct nct7802_data *data = dev_get_drvdata(dev); > + int err; > + unsigned int val; > + > + err = kstrtouint(buf, 0, &val); > + if (err < 0) > + return err; > + if (val > 0xFF) > + return -EINVAL; You might consider using kstrtou8 instead. > + > + err = regmap_write(data->regmap, attr->index, val); > + return err ? : count; > +} > > static int nct7802_read_temp(struct nct7802_data *data, > u8 reg_temp, u8 reg_temp_low, int *temp) > @@ -735,6 +767,11 @@ static SENSOR_DEVICE_ATTR_2(fan3_alarm, S_IRUGO, show_alarm, NULL, 0x1a, 2); > static SENSOR_DEVICE_ATTR_2(fan3_beep, S_IRUGO | S_IWUSR, show_beep, store_beep, > 0x5b, 2); > > +/* 7.2.91... Fan Control Output Value */ > +static SENSOR_DEVICE_ATTR(fan1_output, S_IRUGO | S_IWUSR, show_dec, store_u8, 0x60); > +static SENSOR_DEVICE_ATTR(fan2_output, S_IRUGO | S_IWUSR, show_dec, store_u8, 0x61); > +static SENSOR_DEVICE_ATTR(fan3_output, S_IRUGO | S_IWUSR, show_dec, store_u8, 0x62); > + Please use pwm[1-3] for the attribute names. > static struct attribute *nct7802_fan_attrs[] = { > &sensor_dev_attr_fan1_input.dev_attr.attr, > &sensor_dev_attr_fan1_min.dev_attr.attr, > @@ -773,10 +810,18 @@ static struct attribute_group nct7802_fan_group = { > .is_visible = nct7802_fan_is_visible, > }; > > +static struct attribute *fan_output_attributes[] = { > + &sensor_dev_attr_fan1_output.dev_attr.attr, > + &sensor_dev_attr_fan2_output.dev_attr.attr, > + &sensor_dev_attr_fan3_output.dev_attr.attr, > + NULL > +}; > + Please use an explicit variable for the group, similar to the other groups. I would suggest to use nct7802_pwm_attrs and nct7802_pwm_group. _attrs is a naming convention used by macros such as ATTRIBUTE_GROUPS, so it makes sense to stick with it. > static const struct attribute_group *nct7802_groups[] = { > &nct7802_temp_group, > &nct7802_in_group, > &nct7802_fan_group, > + &(const struct attribute_group){ .attrs = fan_output_attributes }, ... as I am not in favor of such constructs. Thanks, Guenter > NULL > }; > > -- 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/