* Re: [lm-sensors] [PATCH] hwmon/sis5595: Add individual alarm files
2007-10-11 17:59 [lm-sensors] [PATCH] hwmon/sis5595: Add individual alarm files Pinkel
@ 2007-10-12 14:01 ` Jean Delvare
2007-10-12 15:12 ` Ivo Manca
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2007-10-12 14:01 UTC (permalink / raw)
To: lm-sensors
Hi Ivo,
On Thu, 11 Oct 2007 19:59:48 +0200, Pinkel wrote:
> Add individual alarm files needed by the new libsensors.
>
> Signed-off-by: Ivo Manca <pinkel@gmail.com>
>
> ---
> drivers/hwmon/sis5595.c | 32 ++++++++++++++++++++++++++++++--
> 1 file changed, 30 insertions(+), 2 deletions(-)
>
> --- linux-2.6.22.9.orig/drivers/hwmon/sis5595.c 2007-10-11
> 19:38:11.000000000 +0200
> +++ linux-2.6.22.9/drivers/hwmon/sis5595.c 2007-10-11
> 19:41:56.000000000 +0200
> @@ -62,6 +62,7 @@
> #include <linux/jiffies.h>
> #include <linux/mutex.h>
> #include <linux/sysfs.h>
> +#include <linux/hwmon-sysfs.h>
> #include <asm/io.h>
>
>
> @@ -473,26 +474,47 @@
> }
> static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
>
> +static ssize_t show_alarm(struct device *dev, struct device_attribute
> *da, char *buf)
> +{
> + struct sis5595_data *data = sis5595_update_device(dev);
> + int nr = to_sensor_dev_attr(da)->index;
> + return sprintf(buf, "%u\n", (data->alarms >> nr) & 1);
> +}
> + static SENSOR_DEVICE_ATTR(in0_alarm, S_IRUGO, show_alarm, NULL, 0);
> + static SENSOR_DEVICE_ATTR(in1_alarm, S_IRUGO, show_alarm, NULL, 1);
> + static SENSOR_DEVICE_ATTR(in2_alarm, S_IRUGO, show_alarm, NULL, 2);
> + static SENSOR_DEVICE_ATTR(in3_alarm, S_IRUGO, show_alarm, NULL, 3);
> + static SENSOR_DEVICE_ATTR(in4_alarm, S_IRUGO, show_alarm, NULL, 15);
> + static SENSOR_DEVICE_ATTR(fan1_alarm, S_IRUGO, show_alarm, NULL, 6);
> + static SENSOR_DEVICE_ATTR(fan2_alarm, S_IRUGO, show_alarm, NULL, 7);
> + static SENSOR_DEVICE_ATTR(temp1_alarm, S_IRUGO, show_alarm, NULL, 15);
Your mailer destroyed the patch formatting :( Can you please fix it or
resend the patch as an attachment?
Thanks,
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH] hwmon/sis5595: Add individual alarm files
2007-10-11 17:59 [lm-sensors] [PATCH] hwmon/sis5595: Add individual alarm files Pinkel
2007-10-12 14:01 ` Jean Delvare
@ 2007-10-12 15:12 ` Ivo Manca
2007-10-12 20:26 ` Jean Delvare
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Ivo Manca @ 2007-10-12 15:12 UTC (permalink / raw)
To: lm-sensors
[-- Attachment #1: Type: text/plain, Size: 1878 bytes --]
Jean Delvare wrote:
> Hi Ivo,
>
> On Thu, 11 Oct 2007 19:59:48 +0200, Pinkel wrote:
>
>> Add individual alarm files needed by the new libsensors.
>>
>> Signed-off-by: Ivo Manca <pinkel@gmail.com>
>>
>> ---
>> drivers/hwmon/sis5595.c | 32 ++++++++++++++++++++++++++++++--
>> 1 file changed, 30 insertions(+), 2 deletions(-)
>>
>> --- linux-2.6.22.9.orig/drivers/hwmon/sis5595.c 2007-10-11
>> 19:38:11.000000000 +0200
>> +++ linux-2.6.22.9/drivers/hwmon/sis5595.c 2007-10-11
>> 19:41:56.000000000 +0200
>> @@ -62,6 +62,7 @@
>> #include <linux/jiffies.h>
>> #include <linux/mutex.h>
>> #include <linux/sysfs.h>
>> +#include <linux/hwmon-sysfs.h>
>> #include <asm/io.h>
>>
>>
>> @@ -473,26 +474,47 @@
>> }
>> static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
>>
>> +static ssize_t show_alarm(struct device *dev, struct device_attribute
>> *da, char *buf)
>> +{
>> + struct sis5595_data *data = sis5595_update_device(dev);
>> + int nr = to_sensor_dev_attr(da)->index;
>> + return sprintf(buf, "%u\n", (data->alarms >> nr) & 1);
>> +}
>> + static SENSOR_DEVICE_ATTR(in0_alarm, S_IRUGO, show_alarm, NULL, 0);
>> + static SENSOR_DEVICE_ATTR(in1_alarm, S_IRUGO, show_alarm, NULL, 1);
>> + static SENSOR_DEVICE_ATTR(in2_alarm, S_IRUGO, show_alarm, NULL, 2);
>> + static SENSOR_DEVICE_ATTR(in3_alarm, S_IRUGO, show_alarm, NULL, 3);
>> + static SENSOR_DEVICE_ATTR(in4_alarm, S_IRUGO, show_alarm, NULL, 15);
>> + static SENSOR_DEVICE_ATTR(fan1_alarm, S_IRUGO, show_alarm, NULL, 6);
>> + static SENSOR_DEVICE_ATTR(fan2_alarm, S_IRUGO, show_alarm, NULL, 7);
>> + static SENSOR_DEVICE_ATTR(temp1_alarm, S_IRUGO, show_alarm, NULL, 15);
>>
>
> Your mailer destroyed the patch formatting :( Can you please fix it or
> resend the patch as an attachment?
>
> Thanks,
>
Sure. It's attached now, hope that works better.
Ivo
[-- Attachment #2: sis5595.patch --]
[-- Type: text/plain, Size: 3235 bytes --]
diff -ubrN linux-2.6.22.9.orig/drivers/hwmon/sis5595.c linux-2.6.22.9/drivers/hwmon/sis5595.c
--- linux-2.6.22.9.orig/drivers/hwmon/sis5595.c 2007-10-11 19:38:11.000000000 +0200
+++ linux-2.6.22.9/drivers/hwmon/sis5595.c 2007-10-11 19:41:56.000000000 +0200
@@ -62,6 +62,7 @@
#include <linux/jiffies.h>
#include <linux/mutex.h>
#include <linux/sysfs.h>
+#include <linux/hwmon-sysfs.h>
#include <asm/io.h>
@@ -473,26 +474,47 @@
}
static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
+static ssize_t show_alarm(struct device *dev, struct device_attribute *da, char *buf)
+{
+ struct sis5595_data *data = sis5595_update_device(dev);
+ int nr = to_sensor_dev_attr(da)->index;
+ return sprintf(buf, "%u\n", (data->alarms >> nr) & 1);
+}
+ static SENSOR_DEVICE_ATTR(in0_alarm, S_IRUGO, show_alarm, NULL, 0);
+ static SENSOR_DEVICE_ATTR(in1_alarm, S_IRUGO, show_alarm, NULL, 1);
+ static SENSOR_DEVICE_ATTR(in2_alarm, S_IRUGO, show_alarm, NULL, 2);
+ static SENSOR_DEVICE_ATTR(in3_alarm, S_IRUGO, show_alarm, NULL, 3);
+ static SENSOR_DEVICE_ATTR(in4_alarm, S_IRUGO, show_alarm, NULL, 15);
+ static SENSOR_DEVICE_ATTR(fan1_alarm, S_IRUGO, show_alarm, NULL, 6);
+ static SENSOR_DEVICE_ATTR(fan2_alarm, S_IRUGO, show_alarm, NULL, 7);
+ static SENSOR_DEVICE_ATTR(temp1_alarm, S_IRUGO, show_alarm, NULL, 15);
+
static struct attribute *sis5595_attributes[] = {
&dev_attr_in0_input.attr,
&dev_attr_in0_min.attr,
&dev_attr_in0_max.attr,
+ &sensor_dev_attr_in0_alarm.dev_attr.attr,
&dev_attr_in1_input.attr,
&dev_attr_in1_min.attr,
&dev_attr_in1_max.attr,
+ &sensor_dev_attr_in1_alarm.dev_attr.attr,
&dev_attr_in2_input.attr,
&dev_attr_in2_min.attr,
&dev_attr_in2_max.attr,
+ &sensor_dev_attr_in2_alarm.dev_attr.attr,
&dev_attr_in3_input.attr,
&dev_attr_in3_min.attr,
&dev_attr_in3_max.attr,
+ &sensor_dev_attr_in3_alarm.dev_attr.attr,
&dev_attr_fan1_input.attr,
&dev_attr_fan1_min.attr,
&dev_attr_fan1_div.attr,
+ &sensor_dev_attr_fan1_alarm.dev_attr.attr,
&dev_attr_fan2_input.attr,
&dev_attr_fan2_min.attr,
&dev_attr_fan2_div.attr,
+ &sensor_dev_attr_fan2_alarm.dev_attr.attr,
&dev_attr_alarms.attr,
NULL
@@ -506,10 +528,12 @@
&dev_attr_in4_input.attr,
&dev_attr_in4_min.attr,
&dev_attr_in4_max.attr,
+ &sensor_dev_attr_in4_alarm.dev_attr.attr,
&dev_attr_temp1_input.attr,
&dev_attr_temp1_max.attr,
&dev_attr_temp1_max_hyst.attr,
+ &sensor_dev_attr_temp1_alarm.dev_attr.attr,
NULL
};
@@ -617,7 +641,9 @@
|| (err = device_create_file(&new_client->dev,
&dev_attr_in4_min))
|| (err = device_create_file(&new_client->dev,
- &dev_attr_in4_max)))
+ &dev_attr_in4_max))
+ || (err = device_create_file(&new_client->dev,
+ &sensor_dev_attr_in4_alarm.dev_attr)))
goto exit_remove_files;
} else {
if ((err = device_create_file(&new_client->dev,
@@ -625,7 +651,9 @@
|| (err = device_create_file(&new_client->dev,
&dev_attr_temp1_max))
|| (err = device_create_file(&new_client->dev,
- &dev_attr_temp1_max_hyst)))
+ &dev_attr_temp1_max_hyst))
+ || (err = device_create_file(&new_client->dev,
+ &sensor_dev_attr_temp1_alarm.dev_attr)))
goto exit_remove_files;
}
[-- Attachment #3: Type: text/plain, Size: 153 bytes --]
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH] hwmon/sis5595: Add individual alarm files
2007-10-11 17:59 [lm-sensors] [PATCH] hwmon/sis5595: Add individual alarm files Pinkel
2007-10-12 14:01 ` Jean Delvare
2007-10-12 15:12 ` Ivo Manca
@ 2007-10-12 20:26 ` Jean Delvare
2007-10-12 21:49 ` Ivo Manca
2007-10-13 12:58 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2007-10-12 20:26 UTC (permalink / raw)
To: lm-sensors
Hi Ivo,
On Fri, 12 Oct 2007 17:12:50 +0200, Ivo Manca wrote:
> Jean Delvare wrote:
> > Your mailer destroyed the patch formatting :( Can you please fix it or
> > resend the patch as an attachment?
> >
> > Thanks,
>
> Sure. It's attached now, hope that works better.
Yes, it's better but now there's another problem: you based your patch
on 2.6.22.9, but the sis5595 driver changed a lot in 2.6.23, so I still
can't apply it. Would you be so kind and send a new patch that would
apply on top of 2.6.23?
Still, I can review the patch:
> diff -ubrN linux-2.6.22.9.orig/drivers/hwmon/sis5595.c linux-2.6.22.9/drivers/hwmon/sis5595.c
> --- linux-2.6.22.9.orig/drivers/hwmon/sis5595.c 2007-10-11 19:38:11.000000000 +0200
> +++ linux-2.6.22.9/drivers/hwmon/sis5595.c 2007-10-11 19:41:56.000000000 +0200
> @@ -62,6 +62,7 @@
> #include <linux/jiffies.h>
> #include <linux/mutex.h>
> #include <linux/sysfs.h>
> +#include <linux/hwmon-sysfs.h>
> #include <asm/io.h>
>
>
> @@ -473,26 +474,47 @@
> }
> static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
>
> +static ssize_t show_alarm(struct device *dev, struct device_attribute *da, char *buf)
Remember that line length should never exceed 80 columns.
> +{
> + struct sis5595_data *data = sis5595_update_device(dev);
> + int nr = to_sensor_dev_attr(da)->index;
> + return sprintf(buf, "%u\n", (data->alarms >> nr) & 1);
> +}
> + static SENSOR_DEVICE_ATTR(in0_alarm, S_IRUGO, show_alarm, NULL, 0);
> + static SENSOR_DEVICE_ATTR(in1_alarm, S_IRUGO, show_alarm, NULL, 1);
> + static SENSOR_DEVICE_ATTR(in2_alarm, S_IRUGO, show_alarm, NULL, 2);
> + static SENSOR_DEVICE_ATTR(in3_alarm, S_IRUGO, show_alarm, NULL, 3);
> + static SENSOR_DEVICE_ATTR(in4_alarm, S_IRUGO, show_alarm, NULL, 15);
> + static SENSOR_DEVICE_ATTR(fan1_alarm, S_IRUGO, show_alarm, NULL, 6);
> + static SENSOR_DEVICE_ATTR(fan2_alarm, S_IRUGO, show_alarm, NULL, 7);
> + static SENSOR_DEVICE_ATTR(temp1_alarm, S_IRUGO, show_alarm, NULL, 15);
Bad indentation, all these "static SENSOR_DEVICE_ATTR" lines shouldn't
be indented.
> +
> static struct attribute *sis5595_attributes[] = {
> &dev_attr_in0_input.attr,
> &dev_attr_in0_min.attr,
> &dev_attr_in0_max.attr,
> + &sensor_dev_attr_in0_alarm.dev_attr.attr,
> &dev_attr_in1_input.attr,
> &dev_attr_in1_min.attr,
> &dev_attr_in1_max.attr,
> + &sensor_dev_attr_in1_alarm.dev_attr.attr,
> &dev_attr_in2_input.attr,
> &dev_attr_in2_min.attr,
> &dev_attr_in2_max.attr,
> + &sensor_dev_attr_in2_alarm.dev_attr.attr,
> &dev_attr_in3_input.attr,
> &dev_attr_in3_min.attr,
> &dev_attr_in3_max.attr,
> + &sensor_dev_attr_in3_alarm.dev_attr.attr,
>
> &dev_attr_fan1_input.attr,
> &dev_attr_fan1_min.attr,
> &dev_attr_fan1_div.attr,
> + &sensor_dev_attr_fan1_alarm.dev_attr.attr,
> &dev_attr_fan2_input.attr,
> &dev_attr_fan2_min.attr,
> &dev_attr_fan2_div.attr,
> + &sensor_dev_attr_fan2_alarm.dev_attr.attr,
>
> &dev_attr_alarms.attr,
> NULL
> @@ -506,10 +528,12 @@
> &dev_attr_in4_input.attr,
> &dev_attr_in4_min.attr,
> &dev_attr_in4_max.attr,
> + &sensor_dev_attr_in4_alarm.dev_attr.attr,
>
> &dev_attr_temp1_input.attr,
> &dev_attr_temp1_max.attr,
> &dev_attr_temp1_max_hyst.attr,
> + &sensor_dev_attr_temp1_alarm.dev_attr.attr,
> NULL
> };
>
> @@ -617,7 +641,9 @@
> || (err = device_create_file(&new_client->dev,
> &dev_attr_in4_min))
> || (err = device_create_file(&new_client->dev,
> - &dev_attr_in4_max)))
> + &dev_attr_in4_max))
> + || (err = device_create_file(&new_client->dev,
> + &sensor_dev_attr_in4_alarm.dev_attr)))
> goto exit_remove_files;
> } else {
> if ((err = device_create_file(&new_client->dev,
> @@ -625,7 +651,9 @@
> || (err = device_create_file(&new_client->dev,
> &dev_attr_temp1_max))
> || (err = device_create_file(&new_client->dev,
> - &dev_attr_temp1_max_hyst)))
> + &dev_attr_temp1_max_hyst))
> + || (err = device_create_file(&new_client->dev,
> + &sensor_dev_attr_temp1_alarm.dev_attr)))
> goto exit_remove_files;
> }
>
Other than that, your patch looks good.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH] hwmon/sis5595: Add individual alarm files
2007-10-11 17:59 [lm-sensors] [PATCH] hwmon/sis5595: Add individual alarm files Pinkel
` (2 preceding siblings ...)
2007-10-12 20:26 ` Jean Delvare
@ 2007-10-12 21:49 ` Ivo Manca
2007-10-13 12:58 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Ivo Manca @ 2007-10-12 21:49 UTC (permalink / raw)
To: lm-sensors
Hey Jean,
Thanks for the quick review. It's starting to show I'm new to this,
isn't it? ;)
Anyway, one question while making the patch for 2.6.23.1:
@@ -553,7 +578,9 @@
|| (err = device_create_file(&pdev->dev,
&dev_attr_temp1_max))
|| (err = device_create_file(&pdev->dev,
- &dev_attr_temp1_max_hyst)))
+ &dev_attr_temp1_max_hyst))
+ || (err = device_create_file(&pdev->dev,
+ &sensor_dev_attr_temp1_alarm.dev_attr)))
goto exit_remove_files;
(E-mail client will most likely mess this up, however, it's also in the
original patch you just reviewed...).
The line "&sensor_dev_attr_temp1_alarm.dev_attr)))" can not fit within
80 characters using the same indentation as the lines above it; it takes
87 characters. However, removing the spaces before it, will still make
it use 81 chars... I can't see any hint about it in the CodingStyles and
removing some tabs will make it look just plain ugly. Any hint how to
format it? Patch against 2.6.23.1 is ready, just waiting to be sure how
to do this correct :p
Anyway,
Jean Delvare wrote:
> Hi Ivo,
>
> On Fri, 12 Oct 2007 17:12:50 +0200, Ivo Manca wrote:
>
>> Jean Delvare wrote:
>>
>>> Your mailer destroyed the patch formatting :( Can you please fix it or
>>> resend the patch as an attachment?
>>>
>>> Thanks,
>>>
>> Sure. It's attached now, hope that works better.
>>
>
> Yes, it's better but now there's another problem: you based your patch
> on 2.6.22.9, but the sis5595 driver changed a lot in 2.6.23, so I still
> can't apply it. Would you be so kind and send a new patch that would
> apply on top of 2.6.23?
>
> Still, I can review the patch:
>
Ugh. I should have known that, silly me. Anyway, I'll apply my changes
to it. Patch on its way.
>> diff -ubrN linux-2.6.22.9.orig/drivers/hwmon/sis5595.c linux-2.6.22.9/drivers/hwmon/sis5595.c
>> --- linux-2.6.22.9.orig/drivers/hwmon/sis5595.c 2007-10-11 19:38:11.000000000 +0200
>> +++ linux-2.6.22.9/drivers/hwmon/sis5595.c 2007-10-11 19:41:56.000000000 +0200
>> @@ -62,6 +62,7 @@
>> #include <linux/jiffies.h>
>> #include <linux/mutex.h>
>> #include <linux/sysfs.h>
>> +#include <linux/hwmon-sysfs.h>
>> #include <asm/io.h>
>>
>>
>> @@ -473,26 +474,47 @@
>> }
>> static DEVICE_ATTR(alarms, S_IRUGO, show_alarms, NULL);
>>
>> +static ssize_t show_alarm(struct device *dev, struct device_attribute *da, char *buf)
>>
>
> Remember that line length should never exceed 80 columns.
>
Yup, fixed
>> +{
>> + struct sis5595_data *data = sis5595_update_device(dev);
>> + int nr = to_sensor_dev_attr(da)->index;
>> + return sprintf(buf, "%u\n", (data->alarms >> nr) & 1);
>> +}
>> + static SENSOR_DEVICE_ATTR(in0_alarm, S_IRUGO, show_alarm, NULL, 0);
>> + static SENSOR_DEVICE_ATTR(in1_alarm, S_IRUGO, show_alarm, NULL, 1);
>> + static SENSOR_DEVICE_ATTR(in2_alarm, S_IRUGO, show_alarm, NULL, 2);
>> + static SENSOR_DEVICE_ATTR(in3_alarm, S_IRUGO, show_alarm, NULL, 3);
>> + static SENSOR_DEVICE_ATTR(in4_alarm, S_IRUGO, show_alarm, NULL, 15);
>> + static SENSOR_DEVICE_ATTR(fan1_alarm, S_IRUGO, show_alarm, NULL, 6);
>> + static SENSOR_DEVICE_ATTR(fan2_alarm, S_IRUGO, show_alarm, NULL, 7);
>> + static SENSOR_DEVICE_ATTR(temp1_alarm, S_IRUGO, show_alarm, NULL, 15);
>>
>
> Bad indentation, all these "static SENSOR_DEVICE_ATTR" lines shouldn't
> be indented.
>
Doh, fixed
>> +
>> static struct attribute *sis5595_attributes[] = {
>> &dev_attr_in0_input.attr,
>> &dev_attr_in0_min.attr,
>> &dev_attr_in0_max.attr,
>> + &sensor_dev_attr_in0_alarm.dev_attr.attr,
>> &dev_attr_in1_input.attr,
>> &dev_attr_in1_min.attr,
>> &dev_attr_in1_max.attr,
>> + &sensor_dev_attr_in1_alarm.dev_attr.attr,
>> &dev_attr_in2_input.attr,
>> &dev_attr_in2_min.attr,
>> &dev_attr_in2_max.attr,
>> + &sensor_dev_attr_in2_alarm.dev_attr.attr,
>> &dev_attr_in3_input.attr,
>> &dev_attr_in3_min.attr,
>> &dev_attr_in3_max.attr,
>> + &sensor_dev_attr_in3_alarm.dev_attr.attr,
>>
>> &dev_attr_fan1_input.attr,
>> &dev_attr_fan1_min.attr,
>> &dev_attr_fan1_div.attr,
>> + &sensor_dev_attr_fan1_alarm.dev_attr.attr,
>> &dev_attr_fan2_input.attr,
>> &dev_attr_fan2_min.attr,
>> &dev_attr_fan2_div.attr,
>> + &sensor_dev_attr_fan2_alarm.dev_attr.attr,
>>
>> &dev_attr_alarms.attr,
>> NULL
>> @@ -506,10 +528,12 @@
>> &dev_attr_in4_input.attr,
>> &dev_attr_in4_min.attr,
>> &dev_attr_in4_max.attr,
>> + &sensor_dev_attr_in4_alarm.dev_attr.attr,
>>
>> &dev_attr_temp1_input.attr,
>> &dev_attr_temp1_max.attr,
>> &dev_attr_temp1_max_hyst.attr,
>> + &sensor_dev_attr_temp1_alarm.dev_attr.attr,
>> NULL
>> };
>>
>> @@ -617,7 +641,9 @@
>> || (err = device_create_file(&new_client->dev,
>> &dev_attr_in4_min))
>> || (err = device_create_file(&new_client->dev,
>> - &dev_attr_in4_max)))
>> + &dev_attr_in4_max))
>> + || (err = device_create_file(&new_client->dev,
>> + &sensor_dev_attr_in4_alarm.dev_attr)))
>> goto exit_remove_files;
>> } else {
>> if ((err = device_create_file(&new_client->dev,
>> @@ -625,7 +651,9 @@
>> || (err = device_create_file(&new_client->dev,
>> &dev_attr_temp1_max))
>> || (err = device_create_file(&new_client->dev,
>> - &dev_attr_temp1_max_hyst)))
>> + &dev_attr_temp1_max_hyst))
>> + || (err = device_create_file(&new_client->dev,
>> + &sensor_dev_attr_temp1_alarm.dev_attr)))
>> goto exit_remove_files;
>> }
>>
>>
>
> Other than that, your patch looks good.
>
>
Thanks,
Ivo
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [lm-sensors] [PATCH] hwmon/sis5595: Add individual alarm files
2007-10-11 17:59 [lm-sensors] [PATCH] hwmon/sis5595: Add individual alarm files Pinkel
` (3 preceding siblings ...)
2007-10-12 21:49 ` Ivo Manca
@ 2007-10-13 12:58 ` Jean Delvare
4 siblings, 0 replies; 6+ messages in thread
From: Jean Delvare @ 2007-10-13 12:58 UTC (permalink / raw)
To: lm-sensors
Hi Ivo,
On Fri, 12 Oct 2007 23:49:19 +0200, Ivo Manca wrote:
> Hey Jean,
>
> Thanks for the quick review. It's starting to show I'm new to this,
> isn't it? ;)
New contributors are always welcome :)
> Anyway, one question while making the patch for 2.6.23.1:
>
> @@ -553,7 +578,9 @@
> || (err = device_create_file(&pdev->dev,
> &dev_attr_temp1_max))
> || (err = device_create_file(&pdev->dev,
> - &dev_attr_temp1_max_hyst)))
> + &dev_attr_temp1_max_hyst))
> + || (err = device_create_file(&pdev->dev,
> + &sensor_dev_attr_temp1_alarm.dev_attr)))
> goto exit_remove_files;
> (E-mail client will most likely mess this up, however, it's also in the
> original patch you just reviewed...).
> The line "&sensor_dev_attr_temp1_alarm.dev_attr)))" can not fit within
> 80 characters using the same indentation as the lines above it; it takes
> 87 characters. However, removing the spaces before it, will still make
> it use 81 chars... I can't see any hint about it in the CodingStyles and
> removing some tabs will make it look just plain ugly. Any hint how to
> format it? Patch against 2.6.23.1 is ready, just waiting to be sure how
> to do this correct :p
The usual way to workaround this is indeed to reduce the level of
indentation. Not necessarily very nice, but the easiest. In this
particular case, if you don't want to do that, I see two alternatives
(probably for a different patch though):
1* Move the creation of these individual files to a separate function.
This saves one level of indentation, so it should fit.
2* Split sis5595_attributes_opt into sis5595_attributes_in4 and
sis5595_attributes_temp1, and use sysfs_create_group on these instead
of individual calls to device_create_file.
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
^ permalink raw reply [flat|nested] 6+ messages in thread