Re: [lm-sensors] hwmon/f75375s.c: buggy if()

[Jean Delvare <khali@linux-fr.org>]

Hi Riku,

On Thu, 25 Oct 2007 14:48:14 +0300, Riku Voipio wrote:
> On Wed, Oct 24, 2007 at 10:25:29PM -0400, Mark M. Hoffman wrote:
> > * Riku Voipio <riku.voipio@iki.fi> [2007-10-24 14:50:34 +0300]:
> > > On Fri, Oct 19, 2007 at 02:37:54PM +0200, Jean Delvare wrote:
> > > > Riku, can you please submit a patch fixing this? The attribute should
> > > > be declared read-only, and then you can use sysfs_chmod_file() to
> > > > change it to read-write where supported. 
> 
> > > Thanks, this was good suggestion. Patch attached.
> 
> > No patch?
> 
> Let's try again..

> Fix value check in set_pwm_mode(). Instead of checking for
> chip variant there, make pwmX_mode sysfs nodes only writable
> on f75375 variant.
> 
> Signed-off-by: Riku Voipio <riku.voipio@movial.fi>
> ---
>  drivers/hwmon/f75375s.c |   19 ++++++++++++++++---
>  1 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/f75375s.c b/drivers/hwmon/f75375s.c
> index 13a0413..d3b7932 100644
> --- a/drivers/hwmon/f75375s.c
> +++ b/drivers/hwmon/f75375s.c
> @@ -323,7 +323,7 @@ static ssize_t set_pwm_mode(struct device *dev, struct device_attribute *attr,
>  	int val = simple_strtoul(buf, NULL, 10);
>  	u8 conf = 0;
>  
> -	if (val != 0 || val != 1 || data->kind == f75373)
> +	if (!(val == 0 || val == 1))
>  		return -EINVAL;
>  
>  	mutex_lock(&data->update_lock);
> @@ -529,13 +529,13 @@ static SENSOR_DEVICE_ATTR(pwm1, S_IRUGO|S_IWUSR,
>  	show_pwm, set_pwm, 0);
>  static SENSOR_DEVICE_ATTR(pwm1_enable, S_IRUGO|S_IWUSR,
>  	show_pwm_enable, set_pwm_enable, 0);
> -static SENSOR_DEVICE_ATTR(pwm1_mode, S_IRUGO|S_IWUSR,
> +static SENSOR_DEVICE_ATTR(pwm1_mode, S_IRUGO,
>  	show_pwm_mode, set_pwm_mode, 0);
>  static SENSOR_DEVICE_ATTR(pwm2, S_IRUGO | S_IWUSR,
>  	show_pwm, set_pwm, 1);
>  static SENSOR_DEVICE_ATTR(pwm2_enable, S_IRUGO|S_IWUSR,
>  	show_pwm_enable, set_pwm_enable, 1);
> -static SENSOR_DEVICE_ATTR(pwm2_mode, S_IRUGO|S_IWUSR,
> +static SENSOR_DEVICE_ATTR(pwm2_mode, S_IRUGO,
>  	show_pwm_mode, set_pwm_mode, 1);
>  
>  static struct attribute *f75375_attributes[] = {
> @@ -655,6 +655,19 @@ static int f75375_detect(struct i2c_adapter *adapter, int address, int kind)
>  	if ((err = sysfs_create_group(&client->dev.kobj, &f75375_group)))
>  		goto exit_detach;
>  
> +	if (kind == f75375) {
> +		err = sysfs_chmod_file(&client->dev.kobj,
> +			&sensor_dev_attr_pwm1_mode.dev_attr.attr,
> +			S_IRUGO | S_IWUSR);
> +		if (err)
> +			goto exit_detach;
> +		err = sysfs_chmod_file(&client->dev.kobj,
> +			&sensor_dev_attr_pwm2_mode.dev_attr.attr,
> +			S_IRUGO | S_IWUSR);
> +		if (err)
> +			goto exit_detach;
> +	}
> +
>  	data->hwmon_dev = hwmon_device_register(&client->dev);
>  	if (IS_ERR(data->hwmon_dev)) {
>  		err = PTR_ERR(data->hwmon_dev);

Patch looks correct, however it doesn't apply on top of Mark's tree. I
was able to get it to apply by reverting "(f75375s) fix pwm mode
setting" first, but then the build fails. Presumably the other f75375s
patches interact badly. Can you please respin this patch on top of
Mark's tree (i.e. on top the the 4 other f75375s patches you sent since
the -rc1 merge)? Thanks.

-- 
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/