Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757477Ab0GDLYt (ORCPT ); Sun, 4 Jul 2010 07:24:49 -0400 Received: from poutre.nerim.net ([62.4.16.124]:61888 "EHLO poutre.nerim.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757306Ab0GDLYr (ORCPT ); Sun, 4 Jul 2010 07:24:47 -0400 Date: Sun, 4 Jul 2010 13:24:42 +0200 From: Jean Delvare To: Guenter Roeck Cc: Andrew Morton , Ethan Lawrence , Jim Cromie , H Hartley Sweeten , Iain , lm-sensors@lm-sensors.org, linux-kernel@vger.kernel.org, lars4910@hotmail.com, birdie@permonline.ru, jadcock@cox.net, binximeng@gmail.com Subject: Re: [PATCH v4 1/2] hwmon: w83627ehf driver cleanup Message-ID: <20100704132442.07a5ef40@hyperion.delvare> In-Reply-To: <1278195202-2683-2-git-send-email-guenter.roeck@ericsson.com> References: <1278195202-2683-1-git-send-email-guenter.roeck@ericsson.com> <1278195202-2683-2-git-send-email-guenter.roeck@ericsson.com> X-Mailer: Claws Mail 3.5.0 (GTK+ 2.14.4; i586-suse-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6289 Lines: 162 Hi Guenter, On Sat, 3 Jul 2010 15:13:21 -0700, Guenter Roeck wrote: > - Moved fan pwm register array pointers into per-instance data. > - Only read fan pwm data for installed/supported fans. > - Update fan max output and fan step output information from data in registers. > - Create max_output and step_output attribute files only if respective > fan pwm registers exist. > > Signed-off-by: Guenter Roeck > --- > drivers/hwmon/w83627ehf.c | 59 ++++++++++++++++++++++++++++++++++++++++++-- > 1 files changed, 56 insertions(+), 3 deletions(-) > > diff --git a/drivers/hwmon/w83627ehf.c b/drivers/hwmon/w83627ehf.c > index 0dcaba9..e01a3e9 100644 > --- a/drivers/hwmon/w83627ehf.c > +++ b/drivers/hwmon/w83627ehf.c > @@ -277,6 +277,11 @@ struct w83627ehf_data { > struct device *hwmon_dev; > struct mutex lock; > > + const u8 *REG_FAN_START_OUTPUT; > + const u8 *REG_FAN_STOP_OUTPUT; > + const u8 *REG_FAN_MAX_OUTPUT; > + const u8 *REG_FAN_STEP_OUTPUT; > + > struct mutex update_lock; > char valid; /* !=0 if following fields are valid */ > unsigned long last_updated; /* In jiffies */ > @@ -524,7 +529,10 @@ static struct w83627ehf_data *w83627ehf_update_device(struct device *dev) > } > } > > - for (i = 0; i < 4; i++) { > + for (i = 0; i < data->pwm_num; i++) { > + if (!(data->has_fan & (1 << i))) > + continue; I'm skeptical. data->has_fan refers to fan inputs, not fan outputs. There is no guarantee that pwm1 is mapped to fan1, etc., so you can't use data->has_fan to discard fan outputs. > + > /* pwmcfg, tolerance mapped for i=0, i=1 to same reg */ > if (i != 1) { > pwmcfg = w83627ehf_read_value(data, > @@ -546,6 +554,17 @@ static struct w83627ehf_data *w83627ehf_update_device(struct device *dev) > W83627EHF_REG_FAN_STOP_OUTPUT[i]); > data->fan_stop_time[i] = w83627ehf_read_value(data, > W83627EHF_REG_FAN_STOP_TIME[i]); > + > + if (data->REG_FAN_MAX_OUTPUT[i] != 0xff) > + data->fan_max_output[i] = > + w83627ehf_read_value(data, > + data->REG_FAN_MAX_OUTPUT[i]); > + > + if (data->REG_FAN_STEP_OUTPUT[i] != 0xff) > + data->fan_step_output[i] = > + w83627ehf_read_value(data, > + data->REG_FAN_STEP_OUTPUT[i]); > + Huuu, wait a minute, So these values were supposedly exposed to user-space, but were never read from their respective registers? So this is a plain bug you're fixing? Now I'm wondering, how useful is a feature that has been broken forever and nobody ever complained? > data->target_temp[i] = > w83627ehf_read_value(data, > W83627EHF_REG_TARGET[i]) & > @@ -1126,7 +1145,7 @@ store_##reg(struct device *dev, struct device_attribute *attr, \ > u32 val = SENSORS_LIMIT(simple_strtoul(buf, NULL, 10), 1, 255); \ > mutex_lock(&data->update_lock); \ > data->reg[nr] = val; \ > - w83627ehf_write_value(data, W83627EHF_REG_##REG[nr], val); \ > + w83627ehf_write_value(data, data->REG_##REG[nr], val); \ > mutex_unlock(&data->update_lock); \ > return count; \ > } > @@ -1206,12 +1225,26 @@ static struct sensor_device_attribute sda_sf3_arrays[] = { > store_fan_stop_output, 1), > SENSOR_ATTR(pwm3_stop_output, S_IWUSR | S_IRUGO, show_fan_stop_output, > store_fan_stop_output, 2), > +}; > > - /* pwm1 and pwm3 don't support max and step settings */ > + > +/* > + * pwm1 and pwm3 don't support max and step settings on all chips. > + * Need to check support while generating/removing attribute files. > + */ > +static struct sensor_device_attribute sda_sf3_max_step_arrays[] = { > + SENSOR_ATTR(pwm1_max_output, S_IWUSR | S_IRUGO, show_fan_max_output, > + store_fan_max_output, 0), > + SENSOR_ATTR(pwm1_step_output, S_IWUSR | S_IRUGO, show_fan_step_output, > + store_fan_step_output, 0), > SENSOR_ATTR(pwm2_max_output, S_IWUSR | S_IRUGO, show_fan_max_output, > store_fan_max_output, 1), > SENSOR_ATTR(pwm2_step_output, S_IWUSR | S_IRUGO, show_fan_step_output, > store_fan_step_output, 1), > + SENSOR_ATTR(pwm3_max_output, S_IWUSR | S_IRUGO, show_fan_max_output, > + store_fan_max_output, 2), > + SENSOR_ATTR(pwm3_step_output, S_IWUSR | S_IRUGO, show_fan_step_output, > + store_fan_step_output, 2), > }; > > static ssize_t > @@ -1235,6 +1268,12 @@ static void w83627ehf_device_remove_files(struct device *dev) > > for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays); i++) > device_remove_file(dev, &sda_sf3_arrays[i].dev_attr); > + for (i = 0; i < ARRAY_SIZE(sda_sf3_max_step_arrays); i++) { > + struct sensor_device_attribute *attr = > + &sda_sf3_max_step_arrays[i]; > + if (data->REG_FAN_STEP_OUTPUT[attr->index] != 0xff) > + device_remove_file(dev, &attr->dev_attr); > + } > for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++) > device_remove_file(dev, &sda_sf3_arrays_fan4[i].dev_attr); > for (i = 0; i < data->in_num; i++) { > @@ -1352,6 +1391,11 @@ static int __devinit w83627ehf_probe(struct platform_device *pdev) > data->in6_skip = !data->temp3_disable; > } > > + data->REG_FAN_START_OUTPUT = W83627EHF_REG_FAN_START_OUTPUT; > + data->REG_FAN_STOP_OUTPUT = W83627EHF_REG_FAN_STOP_OUTPUT; > + data->REG_FAN_MAX_OUTPUT = W83627EHF_REG_FAN_MAX_OUTPUT; > + data->REG_FAN_STEP_OUTPUT = W83627EHF_REG_FAN_STEP_OUTPUT; > + > /* Initialize the chip */ > w83627ehf_init_device(data); > > @@ -1440,6 +1484,15 @@ static int __devinit w83627ehf_probe(struct platform_device *pdev) > &sda_sf3_arrays[i].dev_attr))) > goto exit_remove; > > + for (i = 0; i < ARRAY_SIZE(sda_sf3_max_step_arrays); i++) { > + struct sensor_device_attribute *attr = > + &sda_sf3_max_step_arrays[i]; > + if (data->REG_FAN_STEP_OUTPUT[attr->index] != 0xff) { > + err = device_create_file(dev, &attr->dev_attr); > + if (err) > + goto exit_remove; > + } > + } > /* if fan4 is enabled create the sf3 files for it */ > if ((data->has_fan & (1 << 3)) && data->pwm_num >= 4) > for (i = 0; i < ARRAY_SIZE(sda_sf3_arrays_fan4); i++) { Nice cleanup overall. -- Jean Delvare -- 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/