From: Jean Delvare <khali@linux-fr.org>
To: lm-sensors@vger.kernel.org
Subject: Re: [lm-sensors] [PATCH 1/4] hwmon-f71882fg: Cleanup sysfs attr
Date: Wed, 28 Oct 2009 08:39:56 +0000 [thread overview]
Message-ID: <20091028093956.4a837e46@hyperion.delvare> (raw)
In-Reply-To: <4AE02EFB.9090309@redhat.com>
Hi Hans,
On Thu, 22 Oct 2009 12:07:55 +0200, Hans de Goede wrote:
> hwmon-f71882fg: Cleanup sysfs attr creation 1/2
>
> This patch makes a number of cleanups to the sysfs attr creation
> in the f71882fg driver, this is a preparation patch for adding f71889fg
> support:
>
> * Add some comments to explain why some models need separate sysfs attr
> arrays for in / temp / fan / pwm
> * Rename a number of sysfs attr arrays to make their function clearer
> * Move the pwm#_auto_channels_temp attribute from the common to all
> models fan attr array to the per model auto mode pwm attr arrays, so
> that all the auto mode pwm attr are grouped together, and thus can be
> left out on models where we don't support auto pwm mode
> * Put fan_beep attr in their own array, so that only auto mode pwm attr
> remain in the per model pwm sysfs attr arrays.
> * Put the 4th special fan input for the f8000 in its own array, so that only
> auto mode pwm attr remain in the per model pwm sysfs attr arrays.
Only minor comments (which I may fix myself if you have no objections):
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> --- linux/drivers/hwmon/f71882fg.c 2009-10-13 13:15:17.000000000 +0200
> +++ linux/drivers/hwmon/f71882fg.c 2009-10-21 11:53:34.000000000 +0200
> @@ -258,7 +258,9 @@
>
> static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
>
> -/* Temp and in attr for the f71858fg */
> +/* Temp and in attr for the f71858fg, the f71858fg is special as it
> + has its temperature indexes start at 0 (the others start at 1) and
> + it only has 3 volt inputs */
I suggest "3 voltage inputs", otherwise it can be confused with the
input range.
> static struct sensor_device_attribute_2 f71858fg_in_temp_attr[] = {
> SENSOR_ATTR_2(in0_input, S_IRUGO, show_in, NULL, 0, 0),
> SENSOR_ATTR_2(in1_input, S_IRUGO, show_in, NULL, 0, 1),
> @@ -302,8 +304,8 @@
> SENSOR_ATTR_2(temp3_fault, S_IRUGO, show_temp_fault, NULL, 0, 2),
> };
>
> -/* Temp and in attr common to both the f71862fg and f71882fg */
> -static struct sensor_device_attribute_2 f718x2fg_in_temp_attr[] = {
> +/* Temp and in attr common to the f71862fg, f71882fg and f71889fg */
> +static struct sensor_device_attribute_2 fxxxx_in_temp_attr[] = {
> SENSOR_ATTR_2(in0_input, S_IRUGO, show_in, NULL, 0, 0),
> SENSOR_ATTR_2(in1_input, S_IRUGO, show_in, NULL, 0, 1),
> SENSOR_ATTR_2(in2_input, S_IRUGO, show_in, NULL, 0, 2),
> @@ -371,8 +373,8 @@
> SENSOR_ATTR_2(temp3_fault, S_IRUGO, show_temp_fault, NULL, 0, 3),
> };
>
> -/* Temp and in attr found only on the f71882fg */
> -static struct sensor_device_attribute_2 f71882fg_in_temp_attr[] = {
> +/* For models with in1 alarm capability */
> +static struct sensor_device_attribute_2 fxxxx_in1_alarm_attr[] = {
> SENSOR_ATTR_2(in1_max, S_IRUGO|S_IWUSR, show_in_max, store_in_max,
> 0, 1),
> SENSOR_ATTR_2(in1_beep, S_IRUGO|S_IWUSR, show_in_beep, store_in_beep,
> @@ -383,6 +385,7 @@
> /* Temp and in attr for the f8000
> Note on the f8000 temp_ovt (crit) is used as max, and temp_high (max)
> is used as hysteresis value to clear alarms
> + Also like the f71858fg its temperature indexes start at 0
> */
> static struct sensor_device_attribute_2 f8000_in_temp_attr[] = {
> SENSOR_ATTR_2(in0_input, S_IRUGO, show_in, NULL, 0, 0),
> @@ -435,39 +438,36 @@
> store_pwm_enable, 0, 0),
> SENSOR_ATTR_2(pwm1_interpolate, S_IRUGO|S_IWUSR,
> show_pwm_interpolate, store_pwm_interpolate, 0, 0),
> - SENSOR_ATTR_2(pwm1_auto_channels_temp, S_IRUGO|S_IWUSR,
> - show_pwm_auto_point_channel,
> - store_pwm_auto_point_channel, 0, 0),
>
> SENSOR_ATTR_2(pwm2, S_IRUGO|S_IWUSR, show_pwm, store_pwm, 0, 1),
> SENSOR_ATTR_2(pwm2_enable, S_IRUGO|S_IWUSR, show_pwm_enable,
> store_pwm_enable, 0, 1),
> SENSOR_ATTR_2(pwm2_interpolate, S_IRUGO|S_IWUSR,
> show_pwm_interpolate, store_pwm_interpolate, 0, 1),
> - SENSOR_ATTR_2(pwm2_auto_channels_temp, S_IRUGO|S_IWUSR,
> - show_pwm_auto_point_channel,
> - store_pwm_auto_point_channel, 0, 1),
>
> SENSOR_ATTR_2(pwm3, S_IRUGO|S_IWUSR, show_pwm, store_pwm, 0, 2),
> SENSOR_ATTR_2(pwm3_enable, S_IRUGO|S_IWUSR, show_pwm_enable,
> store_pwm_enable, 0, 2),
> SENSOR_ATTR_2(pwm3_interpolate, S_IRUGO|S_IWUSR,
> show_pwm_interpolate, store_pwm_interpolate, 0, 2),
> - SENSOR_ATTR_2(pwm3_auto_channels_temp, S_IRUGO|S_IWUSR,
> - show_pwm_auto_point_channel,
> - store_pwm_auto_point_channel, 0, 2),
> };
>
> -/* Fan / PWM attr for the f71862fg, less pwms and less zones per pwm than the
> - f71882fg */
> -static struct sensor_device_attribute_2 f71862fg_fan_attr[] = {
> +/* Attr for models which can beep on Fan alarm */
> +static struct sensor_device_attribute_2 fxxxx_fan_beep_attr[] = {
> SENSOR_ATTR_2(fan1_beep, S_IRUGO|S_IWUSR, show_fan_beep,
> store_fan_beep, 0, 0),
> SENSOR_ATTR_2(fan2_beep, S_IRUGO|S_IWUSR, show_fan_beep,
> store_fan_beep, 0, 1),
> SENSOR_ATTR_2(fan3_beep, S_IRUGO|S_IWUSR, show_fan_beep,
> store_fan_beep, 0, 2),
> +};
>
> +/* PWM attr for the f71862fg, less pwms and less zones per pwm than the
fewer pwms and fewer zones
> + f71882fg / f71889fg */
> +static struct sensor_device_attribute_2 f71862fg_auto_pwm_attr[] = {
> + SENSOR_ATTR_2(pwm1_auto_channels_temp, S_IRUGO|S_IWUSR,
> + show_pwm_auto_point_channel,
> + store_pwm_auto_point_channel, 0, 0),
> SENSOR_ATTR_2(pwm1_auto_point1_pwm, S_IRUGO|S_IWUSR,
> show_pwm_auto_point_pwm, store_pwm_auto_point_pwm,
> 1, 0),
> @@ -487,6 +487,9 @@
> SENSOR_ATTR_2(pwm1_auto_point2_temp_hyst, S_IRUGO,
> show_pwm_auto_point_temp_hyst, NULL, 3, 0),
>
> + SENSOR_ATTR_2(pwm2_auto_channels_temp, S_IRUGO|S_IWUSR,
> + show_pwm_auto_point_channel,
> + store_pwm_auto_point_channel, 0, 1),
> SENSOR_ATTR_2(pwm2_auto_point1_pwm, S_IRUGO|S_IWUSR,
> show_pwm_auto_point_pwm, store_pwm_auto_point_pwm,
> 1, 1),
> @@ -506,6 +509,9 @@
> SENSOR_ATTR_2(pwm2_auto_point2_temp_hyst, S_IRUGO,
> show_pwm_auto_point_temp_hyst, NULL, 3, 1),
>
> + SENSOR_ATTR_2(pwm3_auto_channels_temp, S_IRUGO|S_IWUSR,
> + show_pwm_auto_point_channel,
> + store_pwm_auto_point_channel, 0, 2),
> SENSOR_ATTR_2(pwm3_auto_point1_pwm, S_IRUGO|S_IWUSR,
> show_pwm_auto_point_pwm, store_pwm_auto_point_pwm,
> 1, 2),
> @@ -526,8 +532,11 @@
> show_pwm_auto_point_temp_hyst, NULL, 3, 2),
> };
>
> -/* Fan / PWM attr common to both the f71882fg and f71858fg */
> -static struct sensor_device_attribute_2 f71882fg_f71858fg_fan_attr[] = {
> +/* PWM attr common to both the f71858fg, f71882fg and f71889fg */
I don't think you can say "both" followed by 3 items.
> +static struct sensor_device_attribute_2 fxxxx_auto_pwm_attr[] = {
> + SENSOR_ATTR_2(pwm1_auto_channels_temp, S_IRUGO|S_IWUSR,
> + show_pwm_auto_point_channel,
> + store_pwm_auto_point_channel, 0, 0),
> SENSOR_ATTR_2(pwm1_auto_point1_pwm, S_IRUGO|S_IWUSR,
> show_pwm_auto_point_pwm, store_pwm_auto_point_pwm,
> 0, 0),
> @@ -566,6 +575,9 @@
> SENSOR_ATTR_2(pwm1_auto_point4_temp_hyst, S_IRUGO,
> show_pwm_auto_point_temp_hyst, NULL, 3, 0),
>
> + SENSOR_ATTR_2(pwm2_auto_channels_temp, S_IRUGO|S_IWUSR,
> + show_pwm_auto_point_channel,
> + store_pwm_auto_point_channel, 0, 1),
> SENSOR_ATTR_2(pwm2_auto_point1_pwm, S_IRUGO|S_IWUSR,
> show_pwm_auto_point_pwm, store_pwm_auto_point_pwm,
> 0, 1),
> @@ -604,6 +616,9 @@
> SENSOR_ATTR_2(pwm2_auto_point4_temp_hyst, S_IRUGO,
> show_pwm_auto_point_temp_hyst, NULL, 3, 1),
>
> + SENSOR_ATTR_2(pwm3_auto_channels_temp, S_IRUGO|S_IWUSR,
> + show_pwm_auto_point_channel,
> + store_pwm_auto_point_channel, 0, 2),
> SENSOR_ATTR_2(pwm3_auto_point1_pwm, S_IRUGO|S_IWUSR,
> show_pwm_auto_point_pwm, store_pwm_auto_point_pwm,
> 0, 2),
> @@ -644,14 +659,7 @@
> };
>
> /* Fan / PWM attr found on the f71882fg but not on the f71858fg */
> -static struct sensor_device_attribute_2 f71882fg_fan_attr[] = {
> - SENSOR_ATTR_2(fan1_beep, S_IRUGO|S_IWUSR, show_fan_beep,
> - store_fan_beep, 0, 0),
> - SENSOR_ATTR_2(fan2_beep, S_IRUGO|S_IWUSR, show_fan_beep,
> - store_fan_beep, 0, 1),
> - SENSOR_ATTR_2(fan3_beep, S_IRUGO|S_IWUSR, show_fan_beep,
> - store_fan_beep, 0, 2),
> -
> +static struct sensor_device_attribute_2 f71882fg_auto_pwm_attr[] = {
> SENSOR_ATTR_2(fan4_input, S_IRUGO, show_fan, NULL, 0, 3),
> SENSOR_ATTR_2(fan4_full_speed, S_IRUGO|S_IWUSR,
> show_fan_full_speed,
> @@ -707,12 +715,18 @@
> show_pwm_auto_point_temp_hyst, NULL, 3, 3),
> };
>
> -/* Fan / PWM attr for the f8000, zones mapped to temp instead of to pwm!
> - Also the register block at offset A0 maps to TEMP1 (so our temp2, as the
> - F8000 starts counting temps at 0), B0 maps the TEMP2 and C0 maps to TEMP0 */
> +/* Fan attr specific to the f8000 (4th fan input can only measure speed) */
> static struct sensor_device_attribute_2 f8000_fan_attr[] = {
> SENSOR_ATTR_2(fan4_input, S_IRUGO, show_fan, NULL, 0, 3),
> +};
>
> +/* PWM attr for the f8000, zones mapped to temp instead of to pwm!
> + Also the register block at offset A0 maps to TEMP1 (so our temp2, as the
> + F8000 starts counting temps at 0), B0 maps the TEMP2 and C0 maps to TEMP0 */
> +static struct sensor_device_attribute_2 f8000_auto_pwm_attr[] = {
> + SENSOR_ATTR_2(pwm1_auto_channels_temp, S_IRUGO|S_IWUSR,
> + show_pwm_auto_point_channel,
> + store_pwm_auto_point_channel, 0, 0),
> SENSOR_ATTR_2(temp1_auto_point1_pwm, S_IRUGO|S_IWUSR,
> show_pwm_auto_point_pwm, store_pwm_auto_point_pwm,
> 0, 2),
> @@ -751,6 +765,9 @@
> SENSOR_ATTR_2(temp1_auto_point4_temp_hyst, S_IRUGO,
> show_pwm_auto_point_temp_hyst, NULL, 3, 2),
>
> + SENSOR_ATTR_2(pwm2_auto_channels_temp, S_IRUGO|S_IWUSR,
> + show_pwm_auto_point_channel,
> + store_pwm_auto_point_channel, 0, 1),
> SENSOR_ATTR_2(temp2_auto_point1_pwm, S_IRUGO|S_IWUSR,
> show_pwm_auto_point_pwm, store_pwm_auto_point_pwm,
> 0, 0),
> @@ -789,6 +806,9 @@
> SENSOR_ATTR_2(temp2_auto_point4_temp_hyst, S_IRUGO,
> show_pwm_auto_point_temp_hyst, NULL, 3, 0),
>
> + SENSOR_ATTR_2(pwm3_auto_channels_temp, S_IRUGO|S_IWUSR,
> + show_pwm_auto_point_channel,
> + store_pwm_auto_point_channel, 0, 2),
> SENSOR_ATTR_2(temp3_auto_point1_pwm, S_IRUGO|S_IWUSR,
> show_pwm_auto_point_pwm, store_pwm_auto_point_pwm,
> 0, 1),
> @@ -1847,15 +1867,15 @@
> break;
> case f71882fg:
> err = f71882fg_create_sysfs_files(pdev,
> - f71882fg_in_temp_attr,
> - ARRAY_SIZE(f71882fg_in_temp_attr));
> + fxxxx_in1_alarm_attr,
> + ARRAY_SIZE(fxxxx_in1_alarm_attr));
> if (err)
> goto exit_unregister_sysfs;
> /* fall through! */
> case f71862fg:
> err = f71882fg_create_sysfs_files(pdev,
> - f718x2fg_in_temp_attr,
> - ARRAY_SIZE(f718x2fg_in_temp_attr));
> + fxxxx_in_temp_attr,
> + ARRAY_SIZE(fxxxx_in_temp_attr));
> break;
> case f8000:
> err = f71882fg_create_sysfs_files(pdev,
> @@ -1905,25 +1925,40 @@
> switch (data->type) {
> case f71862fg:
> err = f71882fg_create_sysfs_files(pdev,
> - f71862fg_fan_attr,
> - ARRAY_SIZE(f71862fg_fan_attr));
> + fxxxx_fan_beep_attr,
> + ARRAY_SIZE(fxxxx_fan_beep_attr));
> + if (err)
> + goto exit_unregister_sysfs;
> + err = f71882fg_create_sysfs_files(pdev,
> + f71862fg_auto_pwm_attr,
> + ARRAY_SIZE(f71862fg_auto_pwm_attr));
> break;
> case f71882fg:
> err = f71882fg_create_sysfs_files(pdev,
> - f71882fg_fan_attr,
> - ARRAY_SIZE(f71882fg_fan_attr));
> + fxxxx_fan_beep_attr,
> + ARRAY_SIZE(fxxxx_fan_beep_attr));
> + if (err)
> + goto exit_unregister_sysfs;
> + err = f71882fg_create_sysfs_files(pdev,
> + f71882fg_auto_pwm_attr,
> + ARRAY_SIZE(f71882fg_auto_pwm_attr));
> if (err)
> goto exit_unregister_sysfs;
> /* fall through! */
> case f71858fg:
> err = f71882fg_create_sysfs_files(pdev,
> - f71882fg_f71858fg_fan_attr,
> - ARRAY_SIZE(f71882fg_f71858fg_fan_attr));
> + fxxxx_auto_pwm_attr,
> + ARRAY_SIZE(fxxxx_auto_pwm_attr));
> break;
> case f8000:
> err = f71882fg_create_sysfs_files(pdev,
> f8000_fan_attr,
> ARRAY_SIZE(f8000_fan_attr));
> + if (err)
> + goto exit_unregister_sysfs;
> + err = f71882fg_create_sysfs_files(pdev,
> + f8000_auto_pwm_attr,
> + ARRAY_SIZE(f8000_auto_pwm_attr));
> break;
> }
> if (err)
> @@ -1965,22 +2000,28 @@
> below are supersets of the ones skipped. */
> device_remove_file(&pdev->dev, &dev_attr_name);
>
> - for (i = 0; i < ARRAY_SIZE(f718x2fg_in_temp_attr); i++)
> + for (i = 0; i < ARRAY_SIZE(fxxxx_in_temp_attr); i++)
> device_remove_file(&pdev->dev,
> - &f718x2fg_in_temp_attr[i].dev_attr);
> + &fxxxx_in_temp_attr[i].dev_attr);
>
> - for (i = 0; i < ARRAY_SIZE(f71882fg_in_temp_attr); i++)
> + for (i = 0; i < ARRAY_SIZE(fxxxx_in1_alarm_attr); i++)
> device_remove_file(&pdev->dev,
> - &f71882fg_in_temp_attr[i].dev_attr);
> + &fxxxx_in1_alarm_attr[i].dev_attr);
>
> for (i = 0; i < ARRAY_SIZE(fxxxx_fan_attr); i++)
> device_remove_file(&pdev->dev, &fxxxx_fan_attr[i].dev_attr);
>
> - for (i = 0; i < ARRAY_SIZE(f71882fg_fan_attr); i++)
> - device_remove_file(&pdev->dev, &f71882fg_fan_attr[i].dev_attr);
> + for (i = 0; i < ARRAY_SIZE(fxxxx_fan_beep_attr); i++)
> + device_remove_file(&pdev->dev,
> + &fxxxx_fan_beep_attr[i].dev_attr);
>
> - for (i = 0; i < ARRAY_SIZE(f8000_fan_attr); i++)
> - device_remove_file(&pdev->dev, &f8000_fan_attr[i].dev_attr);
> + for (i = 0; i < ARRAY_SIZE(f71882fg_auto_pwm_attr); i++)
> + device_remove_file(&pdev->dev,
> + &f71882fg_auto_pwm_attr[i].dev_attr);
> +
> + for (i = 0; i < ARRAY_SIZE(f8000_auto_pwm_attr); i++)
> + device_remove_file(&pdev->dev,
> + &f8000_auto_pwm_attr[i].dev_attr);
>
> kfree(data);
>
--
Jean Delvare
_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
next prev parent reply other threads:[~2009-10-28 8:39 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-10-22 10:07 [lm-sensors] [PATCH 1/4] hwmon-f71882fg: Cleanup sysfs attr Hans de Goede
2009-10-28 8:39 ` Jean Delvare [this message]
2009-10-28 8:54 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20091028093956.4a837e46@hyperion.delvare \
--to=khali@linux-fr.org \
--cc=lm-sensors@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.