| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 30 Jan 2013 10:37:30 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Hongbo Zhang <hongbo.zhang@...aro.org>
Cc: khali@...ux-fr.org, lm-sensors@...sensors.org,
sameo@...ux.intel.com, linux-kernel@...r.kernel.org,
STEricsson_nomadik_linux@...t.st.com, linaro-dev@...ts.linaro.org,
patches@...aro.org, linus.walleij@...aro.org, lee.jones@...aro.org
Subject: Re: [PATCH 2/2] hwmon: add ST-Ericsson ABX500 hwmon driver
On Wed, Jan 30, 2013 at 06:21:28PM +0800, Hongbo Zhang wrote:
> Each of ST-Ericsson X500 chip set series consists of both ABX500 and DBX500
> chips. This is ABX500 hwmon driver, where the abx500.c is a common layer for
> all ABX500s, and the ab8500.c is specific for AB8500 chip. Under this designed
> structure, other chip specific files can be added simply using the same common
> layer abx500.c.
>
> Signed-off-by: Hongbo Zhang <hongbo.zhang@...aro.org>
> ---
> drivers/hwmon/Kconfig | 13 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/ab8500.c | 160 ++++++++++++
> drivers/hwmon/abx500.c | 681 +++++++++++++++++++++++++++++++++++++++++++++++++
> drivers/hwmon/abx500.h | 88 +++++++
> 5 files changed, 943 insertions(+)
> create mode 100644 drivers/hwmon/ab8500.c
> create mode 100644 drivers/hwmon/abx500.c
> create mode 100644 drivers/hwmon/abx500.h
>
Hi,
we'll also need Documentation/hwmon/ab8500 to describe the driver, and possibly
Documentation/hwmon/ab85xx to describe the common elements.
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 32f238f..0a6fd21 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -39,6 +39,19 @@ config HWMON_DEBUG_CHIP
>
> comment "Native drivers"
>
> +config SENSORS_AB8500
> + tristate "AB8500 thermal monitoring"
> + depends on AB8500_GPADC
> + default n
> + help
> + If you say yes here you get support for the thermal sensor part
> + of the AB8500 chip. The driver includes thermal management for
> + AB8500 die and two GPADC channels. The GPADC channel are preferably
> + used to access sensors outside the AB8500 chip.
> +
> + This driver can also be built as a module. If so, the module
> + will be called abx500-temp.
> +
> config SENSORS_ABITUGURU
> tristate "Abit uGuru (rev 1 & 2)"
> depends on X86 && DMI
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 5da2874..06dfe85 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_SENSORS_W83795) += w83795.o
> obj-$(CONFIG_SENSORS_W83781D) += w83781d.o
> obj-$(CONFIG_SENSORS_W83791D) += w83791d.o
>
> +obj-$(CONFIG_SENSORS_AB8500) += abx500.o ab8500.o
> obj-$(CONFIG_SENSORS_ABITUGURU) += abituguru.o
> obj-$(CONFIG_SENSORS_ABITUGURU3)+= abituguru3.o
> obj-$(CONFIG_SENSORS_AD7314) += ad7314.o
> diff --git a/drivers/hwmon/ab8500.c b/drivers/hwmon/ab8500.c
> new file mode 100644
> index 0000000..426872c
> --- /dev/null
> +++ b/drivers/hwmon/ab8500.c
> @@ -0,0 +1,160 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2010
> + * Author: Martin Persson <martin.persson@...ricsson.com> for
> + * ST-Ericsson.
> + * License Terms: GNU General Public License v2
> + *
> + * If/when the AB8500 thermal warning temperature is reached (threshold cannot
> + * be changed by SW), an interrupt is set and the driver notifies user space
> + * via a sysfs event. If a shut down is not triggered by user space within a
> + * certain time frame, pm_power off is called.
> + *
> + * If/when AB8500 thermal shutdown temperature is reached a hardware shutdown
> + * of the AB8500 will occur.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/mfd/abx500/ab8500-bm.h>
> +#include <linux/mfd/abx500/ab8500-gpadc.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include "abx500.h"
> +
> +#define DEFAULT_POWER_OFF_DELAY 10000
> +
> +/* The driver monitors GPADC - ADC_AUX1, ADC_AUX2, BTEMP_BALL and BAT_CTRL */
> +#define NUM_MONITORED_SENSORS 4
> +
> +static int ab8500_read_sensor(struct abx500_temp *data, u8 sensor)
> +{
> + int val;
> + /*
> + * Special treatment for the BAT_CTRL node, since this temperature
> + * measurement is more complex than just an ADC readout
> + */
> + if (sensor == BAT_CTRL)
> + val = ab8500_btemp_get_batctrl_temp(data->ab8500_btemp);
> + else
> + val = ab8500_gpadc_convert(data->ab8500_gpadc, sensor);
> +
> + return val;
> +}
> +
> +static void ab8500_thermal_power_off(struct work_struct *work)
> +{
> + struct abx500_temp *data = container_of(work, struct abx500_temp,
> + power_off_work.work);
> +
> + dev_warn(&data->pdev->dev,
> + "Power off due to AB8500 thermal warning.\n");
> + pm_power_off();
> +}
> +
> +static ssize_t ab8500_show_name(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + return sprintf(buf, "ab8500\n");
> +}
> +
> +static ssize_t ab8500_show_label(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + char *name;
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + int index = attr->index;
> +
> + switch (index) {
> + case 1:
> + name = "ext_rtc_xtal";
> + break;
> + case 2:
> + name = "ext_db8500";
> + break;
> + case 3:
> + name = "bat_temp";
> + break;
> + case 4:
> + name = "bat_ctrl";
> + break;
> + case 5:
> + name = "ab8500";
> + break;
> + default:
> + return -EINVAL;
> + }
> + return sprintf(buf, "%s\n", name);
> +}
> +
> +static int ab8500_is_visible(struct attribute *attr, int n)
> +{
> + if (!strcmp(attr->name, "temp5_input") ||
> + !strcmp(attr->name, "temp5_min") ||
> + !strcmp(attr->name, "temp5_max") ||
> + !strcmp(attr->name, "temp5_max_hyst") ||
> + !strcmp(attr->name, "temp5_min_alarm") ||
> + !strcmp(attr->name, "temp5_max_alarm") ||
> + !strcmp(attr->name, "temp5_max_hyst_alarm"))
> + return 0;
> +
If all temp5 attributes are not supported for this chip, a single strncmp
for "temp5" might do. But you still show temp5_name, which is odd.
Really, for thermal management, you should look into using
the thermal subsystem.
> + return attr->mode;
> +}
> +
> +static int ab8500_temp_irq_handler(int irq, struct abx500_temp *data)
> +{
> + unsigned long delay_in_jiffies;
> +
> + mutex_lock(&data->lock);
> + data->crit_alarm[4] = 1;
> + mutex_unlock(&data->lock);
> +
This locking is unnecessary.
> + sysfs_notify(&data->pdev->dev.kobj, NULL, "temp5_crit_alarm");
> + dev_info(&data->pdev->dev, "AB8500 warning, power off in %lu s\n",
> + data->power_off_delay);
> + delay_in_jiffies = msecs_to_jiffies(data->power_off_delay);
> + schedule_delayed_work(&data->power_off_work, delay_in_jiffies);
> + return 0;
> +}
> +
> +int abx500_hwmon_init(struct abx500_temp *data)
With that function naming, you'll have to make sure for later drivers that only
_one_ driver can be configured at any given time. Otherwise build options such
as "allconfig" or "allmodconfig" will fail. Just something to keep in mind.
> +{
> + data->ab8500_gpadc = ab8500_gpadc_get("ab8500-gpadc.0");
Wonder if/how that is going to work in the real world. Can you always
guarantee that the gpadc driver name and instance is "ab8500-gpadc.0" ?
> + if (IS_ERR(data->ab8500_gpadc))
> + return PTR_ERR(data->ab8500_gpadc);
> +
> + data->ab8500_btemp = ab8500_btemp_get();
> + if (IS_ERR(data->ab8500_btemp))
> + return PTR_ERR(data->ab8500_btemp);
> +
> + INIT_DELAYED_WORK(&data->power_off_work, ab8500_thermal_power_off);
> +
> + /*
> + * Reference hardware:
> + * GPADC - ADC_AUX1, connected to NTC R2148
> + * GPADC - ADC_AUX2, connected to NTC R2150
> + * Hence temp#_min/max/max_hyst refer to millivolts, not millidegrees
> + * This is not the case for BAT_CTRL where millidegrees is used
> + *
So you report milli-volt in tempX attributes ? That is unacceptable.
The ABI expects milli-degrees C. If you can not report a temperature,
don't do it.
> + * Reading AB8500 temperature is not supportted. BUT an AB8500 IRQ
/tt/t/
/BUT/But/
> + * will be launched if die crit temp limit is reached.
> + */
> + data->gpadc_addr[0] = ADC_AUX1;
> + data->gpadc_addr[1] = ADC_AUX2;
> + data->gpadc_addr[2] = BTEMP_BALL;
> + data->gpadc_addr[3] = BAT_CTRL;
> + data->gpadc_addr[4] = DIE_TEMP;
> + data->power_off_delay = DEFAULT_POWER_OFF_DELAY;
> + data->monitored_sensors = NUM_MONITORED_SENSORS;
> +
> + data->ops.read_sensor = ab8500_read_sensor;
> + data->ops.irq_handler = ab8500_temp_irq_handler;
> + data->ops.show_name = ab8500_show_name;
> + data->ops.show_label = ab8500_show_label;
> + data->ops.is_visible = ab8500_is_visible;
> +
> + return 0;
> +}
> diff --git a/drivers/hwmon/abx500.c b/drivers/hwmon/abx500.c
> new file mode 100644
> index 0000000..94cffd4
> --- /dev/null
> +++ b/drivers/hwmon/abx500.c
> @@ -0,0 +1,681 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2010
> + * Author: Martin Persson <martin.persson@...ricsson.com> for
> + * ST-Ericsson.
> + * License Terms: GNU General Public License v2
> + *
> + * ABX500 does not provide auto ADC, so to monitor the required temperatures,
> + * a periodic work is used. It is more important to not wake up the CPU than
> + * to perform this job, hence the use of a deferred delay.
> + *
> + * A deferred delay for thermal monitor is considered safe because:
> + * If the chip gets too hot during a sleep state it's most likely due to
> + * external factors, such as the surrounding temperature. I.e. no SW decisions
> + * will make any difference.
> + *
> + * If/when the ABX500 thermal warning temperature is reached (threshold cannot
> + * be changed by SW), an interrupt is set and the driver notifies user space
> + * via a sysfs event.
> + *
The above comments suggest that the thermal subsystem might be a more
appropriate place for this driver. Please check and let me know.
> + * If/when ABX500 thermal shutdown temperature is reached a hardware shutdown
> + * of the ABX500 will occur.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/interrupt.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/workqueue.h>
> +#include "abx500.h"
> +
> +#define DEFAULT_MONITOR_DELAY 1000
> +
> +/*
> + * Thresholds are considered inactive if set to 0. To avoid confusion for user
> + * space applications, the temp monitor delay is set with the default value if
> + * all thresholds are 0.
> + */
> +static bool find_active_thresholds(struct abx500_temp *data)
> +{
> + int i;
> + for (i = 0; i < data->monitored_sensors; i++)
> + if (data->max[i] != 0 || data->max_hyst[i] != 0
> + || data->min[i] != 0)
> + return true;
> +
> + dev_dbg(&data->pdev->dev, "No active thresholds.\n");
> + cancel_delayed_work_sync(&data->work);
> + data->work_active = false;
> + data->gpadc_monitor_delay = DEFAULT_MONITOR_DELAY;
> + return false;
Conceptually, this is an interesting function. Not only does it look for active
thresholds, it actually cancels active work if there are none.
Yet, on the other side it doesn't _activate_ any work if there are active
thresholds.
To be more consistent, it seems to me it would make sense if this
function would also call schedule_work if there is work to be done.
After all, the callers will do it anyway.
> +}
> +
> +static inline void schedule_monitor(struct abx500_temp *data)
> +{
> + unsigned long delay_in_jiffies;
> + delay_in_jiffies = msecs_to_jiffies(data->gpadc_monitor_delay);
> + data->work_active = true;
> + schedule_delayed_work(&data->work, delay_in_jiffies);
> +}
> +
> +static inline void gpadc_monitor_exit(struct abx500_temp *data)
> +{
> + cancel_delayed_work_sync(&data->work);
> + data->work_active = false;
That assignment seems to be quite unnecessary, as the data structure is
about to be freed anyway.
> +}
> +
> +static void gpadc_monitor(struct work_struct *work)
> +{
> + unsigned long delay_in_jiffies;
> + int val, i, ret;
> + char alarm_node[30];
> + bool updated_min_alarm = false;
> + bool updated_max_alarm = false;
> + bool updated_max_hyst_alarm = false;
> + struct abx500_temp *data = container_of(work, struct abx500_temp,
> + work.work);
> +
> + for (i = 0; i < data->monitored_sensors; i++) {
> + /* Thresholds are considered inactive if set to 0 */
> + if (data->max[i] == 0 && data->max_hyst[i] == 0
> + && data->min[i] == 0)
> + continue;
> +
> + val = data->ops.read_sensor(data, data->gpadc_addr[i]);
> + if (val < 0) {
> + dev_err(&data->pdev->dev, "GPADC read failed\n");
> + continue;
> + }
> +
> + mutex_lock(&data->lock);
> + if (data->min[i] != 0) {
> + if (val < data->min[i]) {
> + if (data->min_alarm[i] == 0) {
> + data->min_alarm[i] = 1;
> + updated_min_alarm = true;
> + }
> + } else {
> + if (data->min_alarm[i] == 1) {
> + data->min_alarm[i] = 0;
> + updated_min_alarm = true;
> + }
> + }
> +
> + }
> + if (data->max[i] != 0) {
> + if (val > data->max[i]) {
> + if (data->max_alarm[i] == 0) {
> + data->max_alarm[i] = 1;
> + updated_max_alarm = true;
> + }
> + } else {
> + if (data->max_alarm[i] == 1) {
> + data->max_alarm[i] = 0;
> + updated_max_alarm = true;
> + }
> + }
> +
> + }
> + if (data->max_hyst[i] != 0) {
> + if (val > data->max_hyst[i]) {
> + if (data->max_hyst_alarm[i] == 0) {
> + data->max_hyst_alarm[i] = 1;
> + updated_max_hyst_alarm = true;
> + }
> + } else {
> + if (data->max_hyst_alarm[i] == 1) {
> + data->max_hyst_alarm[i] = 0;
> + updated_max_hyst_alarm = true;
> + }
> + }
> + }
> + mutex_unlock(&data->lock);
> +
> + /* hwmon attr index starts at 1, thus "i+1" below */
> + if (updated_min_alarm) {
> + ret = sprintf(alarm_node, "temp%d_min_alarm", (i + 1));
Unnecessary ( ).
> + sysfs_notify(&data->pdev->dev.kobj, NULL, alarm_node);
> + }
> + if (updated_max_alarm) {
> + ret = sprintf(alarm_node, "temp%d_max_alarm", (i + 1));
Unnecessary ( )
> + sysfs_notify(&data->pdev->dev.kobj, NULL, alarm_node);
> + }
> + if (updated_max_hyst_alarm) {
> + ret = sprintf(alarm_node, "temp%d_max_hyst_alarm",
> + (i + 1));
Unnecessary ( )
> + sysfs_notify(&data->pdev->dev.kobj, NULL, alarm_node);
> + }
You never reset any of the booleans in the loop. That means that after the first
notification all subsequent attributes will get notified, even if nothing
happened with those. For example, after notifying temp1_max_alarm,
temp[2-5]_max_alarm will be notified as well. Is that on purpose ?
> + }
> +
> + delay_in_jiffies = msecs_to_jiffies(data->gpadc_monitor_delay);
> + data->work_active = true;
> + schedule_delayed_work(&data->work, delay_in_jiffies);
> +}
> +
> +static ssize_t set_temp_monitor_delay(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + int res;
> + unsigned long delay_in_s;
> + struct abx500_temp *data = dev_get_drvdata(dev);
> +
> + res = kstrtoul(buf, 10, &delay_in_s);
> + if (res < 0)
> + return res;
> +
> + mutex_lock(&data->lock);
> + data->gpadc_monitor_delay = delay_in_s * 1000;
> +
No concern about overflows ?
> + if (find_active_thresholds(data))
> + schedule_monitor(data);
> +
> + mutex_unlock(&data->lock);
> +
> + return count;
> +}
> +
> +static ssize_t set_temp_power_off_delay(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + int res;
> + unsigned long delay_in_s;
> + struct abx500_temp *data = dev_get_drvdata(dev);
> +
> + res = kstrtoul(buf, 10, &delay_in_s);
> + if (res < 0)
> + return res;
> +
> + mutex_lock(&data->lock);
> + data->power_off_delay = delay_in_s * 1000;
No concern about overflows ?
> + mutex_unlock(&data->lock);
> +
> + return count;
> +}
> +
> +static ssize_t show_temp_monitor_delay(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + /* return time in s, not ms */
> + return sprintf(buf, "%lu\n", (data->gpadc_monitor_delay) / 1000);
> +}
> +
> +static ssize_t show_temp_power_off_delay(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + /* return time in s, not ms */
> + return sprintf(buf, "%lu\n", (data->power_off_delay) / 1000);
> +}
> +
> +/*
> + * HWMON sysfs interfaces
> + * For all the below set and show functions, the hwmon attr index starts at 1,
> + * thus "attr->index-1" is used.
Even though the attribute _names_ start with 1 for most attributes, that doesn't
mean the index parameter to SENSOR_DEVICE_ATTR has to start with 1. Subtracting
1 from attr->index each time it is used is simply a waste of computing resources.
Just start the index from 0.
> + */
> +static ssize_t show_name(struct device *dev, struct device_attribute *devattr,
> + char *buf)
> +{
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + /* Show chip name */
> + return data->ops.show_name(dev, devattr, buf);
> +}
> +
> +static ssize_t show_label(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + /* Show each sensor label */
> + return data->ops.show_label(dev, devattr, buf);
> +}
> +
> +static ssize_t show_input(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + int val;
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + u8 gpadc_addr = data->gpadc_addr[attr->index - 1];
> +
> + val = data->ops.read_sensor(data, gpadc_addr);
> + if (val < 0)
> + dev_err(&data->pdev->dev, "GPADC read failed\n");
> +
> + return sprintf(buf, "%d\n", val);
> +}
> +
> +/*
> + * Set functions (RW nodes)
> + * For all the set functions, threshold is considered inactive if set to 0
> + */
> +static ssize_t set_min(struct device *dev, struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + unsigned long val;
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + int res = kstrtoul(buf, 10, &val);
> + if (res < 0)
> + return res;
> +
> + mutex_lock(&data->lock);
> +
> + if (val == 0)
> + data->min_alarm[attr->index - 1] = 0;
> +
> + data->min[attr->index - 1] = val;
> +
> + if (val == 0)
> + (void) find_active_thresholds(data);
> + else
> + schedule_monitor(data);
> +
> + mutex_unlock(&data->lock);
> +
> + return count;
> +}
> +
> +static ssize_t set_max(struct device *dev, struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + unsigned long val;
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + int res = kstrtoul(buf, 10, &val);
> + if (res < 0)
> + return res;
> +
> + mutex_lock(&data->lock);
> +
> + if (val == 0)
> + data->max_alarm[attr->index - 1] = 0;
> +
> + data->max[attr->index - 1] = val;
> +
> + if (val == 0)
> + (void) find_active_thresholds(data);
> + else
> + schedule_monitor(data);
> +
> + mutex_unlock(&data->lock);
> +
> + return count;
> +}
> +
> +static ssize_t set_max_hyst(struct device *dev,
> + struct device_attribute *devattr,
> + const char *buf, size_t count)
> +{
> + unsigned long val;
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + int res = kstrtoul(buf, 10, &val);
> + if (res < 0)
> + return res;
> +
> + mutex_lock(&data->lock);
> +
> + if (val == 0)
> + data->max_hyst_alarm[attr->index - 1] = 0;
> +
> + data->max_hyst[attr->index - 1] = val;
> +
> + if (val == 0)
> + (void) find_active_thresholds(data);
> + else
> + schedule_monitor(data);
> +
> + mutex_unlock(&data->lock);
> +
> + return count;
> +}
> +
> +/* Show functions (RO nodes) */
> +static ssize_t show_min(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + return sprintf(buf, "%ld\n", data->min[attr->index - 1]);
> +}
> +
> +static ssize_t show_max(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + return sprintf(buf, "%ld\n", data->max[attr->index - 1]);
> +}
> +
> +static ssize_t show_max_hyst(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + return sprintf(buf, "%ld\n", data->max_hyst[attr->index - 1]);
> +}
> +
> +static ssize_t show_min_alarm(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + return sprintf(buf, "%ld\n", data->min_alarm[attr->index - 1]);
> +}
> +
> +static ssize_t show_max_alarm(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + return sprintf(buf, "%ld\n", data->max_alarm[attr->index - 1]);
> +}
> +
> +static ssize_t show_max_hyst_alarm(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + return sprintf(buf, "%ld\n", data->max_hyst_alarm[attr->index - 1]);
> +}
The hysteresis is intended to be the temperature at which the associated alarm
is turned off (in this case tempX_max_alarm). An alarm related to that hysteresis
does not make sense.
This leads to the question what tempX_max_hyst reflects in the first place.
Is it really a hysteresis or something else ? Looking into the code, I don't see
it used as hysteresis but as additional limit. If so, you should use an
attribute name to reflect this. tempX_crit might be an option, or
tempX_emergency.
[ On a side note, you _could_ implement hysteresis attributes, since all
comparisons are done in software anyway, but those should reflect hysteresis
and not just be hidden additional sensors ]
> +
> +static ssize_t show_crit_alarm(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct abx500_temp *data = dev_get_drvdata(dev);
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +
> + return sprintf(buf, "%ld\n", data->crit_alarm[attr->index - 1]);
> +}
> +
> +static mode_t abx500_attrs_visible(struct kobject *kobj,
> + struct attribute *a, int n)
> +{
> + struct device *dev = container_of(kobj, struct device, kobj);
> + struct abx500_temp *data = dev_get_drvdata(dev);
> +
> + return data->ops.is_visible(a, n);
> +}
> +
> +static SENSOR_DEVICE_ATTR(temp_monitor_delay, S_IRUGO | S_IWUSR,
> + show_temp_monitor_delay, set_temp_monitor_delay, 0);
> +static SENSOR_DEVICE_ATTR(temp_power_off_delay, S_IRUGO | S_IWUSR,
> + show_temp_power_off_delay,
> + set_temp_power_off_delay, 0);
> +
> +/* Chip name, required by hwmon*/
space after hwmon
> +static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, 0);
> +
If you don't use the index, you can use DEVICE_ATTR() instead of
SENSOR_DEVICE_ATTR().
> +/* GPADC - SENSOR1 */
> +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_label, NULL, 1);
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 1);
> +static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_min, set_min, 1);
> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_max, set_max, 1);
> +static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO,
> + show_max_hyst, set_max_hyst, 1);
> +static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_min_alarm, NULL, 1);
> +static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_max_alarm, NULL, 1);
> +static SENSOR_DEVICE_ATTR(temp1_max_hyst_alarm, S_IRUGO,
> + show_max_hyst_alarm, NULL, 1);
> +
> +/* GPADC - SENSOR2 */
> +static SENSOR_DEVICE_ATTR(temp2_label, S_IRUGO, show_label, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_min, set_min, 2);
> +static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_max, set_max, 2);
> +static SENSOR_DEVICE_ATTR(temp2_max_hyst, S_IWUSR | S_IRUGO,
> + show_max_hyst, set_max_hyst, 2);
> +static SENSOR_DEVICE_ATTR(temp2_min_alarm, S_IRUGO, show_min_alarm, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp2_max_alarm, S_IRUGO, show_max_alarm, NULL, 2);
> +static SENSOR_DEVICE_ATTR(temp2_max_hyst_alarm, S_IRUGO,
> + show_max_hyst_alarm, NULL, 2);
> +
> +/* GPADC - SENSOR3 */
> +static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, show_label, NULL, 3);
> +static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_input, NULL, 3);
> +static SENSOR_DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_min, set_min, 3);
> +static SENSOR_DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_max, set_max, 3);
> +static SENSOR_DEVICE_ATTR(temp3_max_hyst, S_IWUSR | S_IRUGO,
> + show_max_hyst, set_max_hyst, 3);
> +static SENSOR_DEVICE_ATTR(temp3_min_alarm, S_IRUGO, show_min_alarm, NULL, 3);
> +static SENSOR_DEVICE_ATTR(temp3_max_alarm, S_IRUGO, show_max_alarm, NULL, 3);
> +static SENSOR_DEVICE_ATTR(temp3_max_hyst_alarm, S_IRUGO,
> + show_max_hyst_alarm, NULL, 3);
> +
> +/* GPADC - SENSOR4 */
> +static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, show_label, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, show_input, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp4_min, S_IWUSR | S_IRUGO, show_min, set_min, 4);
> +static SENSOR_DEVICE_ATTR(temp4_max, S_IWUSR | S_IRUGO, show_max, set_max, 4);
> +static SENSOR_DEVICE_ATTR(temp4_max_hyst, S_IWUSR | S_IRUGO,
> + show_max_hyst, set_max_hyst, 4);
> +static SENSOR_DEVICE_ATTR(temp4_min_alarm, S_IRUGO, show_min_alarm, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp4_max_alarm, S_IRUGO, show_max_alarm, NULL, 4);
> +static SENSOR_DEVICE_ATTR(temp4_max_hyst_alarm, S_IRUGO,
> + show_max_hyst_alarm, NULL, 4);
> +
> +/* GPADC - SENSOR5 */
> +static SENSOR_DEVICE_ATTR(temp5_label, S_IRUGO, show_label, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp5_input, S_IRUGO, show_input, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp5_min, S_IWUSR | S_IRUGO, show_min, set_min, 5);
> +static SENSOR_DEVICE_ATTR(temp5_max, S_IWUSR | S_IRUGO, show_max, set_max, 5);
> +static SENSOR_DEVICE_ATTR(temp5_max_hyst, S_IWUSR | S_IRUGO,
> + show_max_hyst, set_max_hyst, 5);
> +static SENSOR_DEVICE_ATTR(temp5_min_alarm, S_IRUGO, show_min_alarm, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp5_max_alarm, S_IRUGO, show_max_alarm, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp5_max_hyst_alarm, S_IRUGO,
> + show_max_hyst_alarm, NULL, 5);
> +static SENSOR_DEVICE_ATTR(temp5_crit_alarm, S_IRUGO,
> + show_crit_alarm, NULL, 5);
> +
crit_alarm would be expected to be set if temp5_crit is exceeded ... which you
don't have in the first place.
Besides, it looks like temp5_crit_alarm is always visible even if all other
temp5 attributes are hidden. This makes no sense - you would report an alarm on
a non-existing sensor. The alarm should be tied to an existing sensor, and to an
existing sensor attribute.
> +struct attribute *abx500_temp_attributes[] = {
> + &sensor_dev_attr_name.dev_attr.attr,
> + &sensor_dev_attr_temp_monitor_delay.dev_attr.attr,
> + &sensor_dev_attr_temp_power_off_delay.dev_attr.attr,
We don't accept non-standard attributes unless you provide a really good reason
why those are needed. For the above, it seems to me that making those values
configurable might actually be risky, especially since you don't even check the
value range.
Do those parameters really have to be configurable at runtime ?
Why don't you use constants ? If that is not feasible, how about module
parameters or platform data / devicetree based initialization ?
> + /* GPADC SENSOR1 */
> + &sensor_dev_attr_temp1_label.dev_attr.attr,
> + &sensor_dev_attr_temp1_input.dev_attr.attr,
> + &sensor_dev_attr_temp1_min.dev_attr.attr,
> + &sensor_dev_attr_temp1_max.dev_attr.attr,
> + &sensor_dev_attr_temp1_max_hyst.dev_attr.attr,
> + &sensor_dev_attr_temp1_min_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp1_max_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp1_max_hyst_alarm.dev_attr.attr,
> + /* GPADC SENSOR2 */
> + &sensor_dev_attr_temp2_label.dev_attr.attr,
> + &sensor_dev_attr_temp2_input.dev_attr.attr,
> + &sensor_dev_attr_temp2_min.dev_attr.attr,
> + &sensor_dev_attr_temp2_max.dev_attr.attr,
> + &sensor_dev_attr_temp2_max_hyst.dev_attr.attr,
> + &sensor_dev_attr_temp2_min_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp2_max_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp2_max_hyst_alarm.dev_attr.attr,
> + /* GPADC SENSOR3 */
> + &sensor_dev_attr_temp3_label.dev_attr.attr,
> + &sensor_dev_attr_temp3_input.dev_attr.attr,
> + &sensor_dev_attr_temp3_min.dev_attr.attr,
> + &sensor_dev_attr_temp3_max.dev_attr.attr,
> + &sensor_dev_attr_temp3_max_hyst.dev_attr.attr,
> + &sensor_dev_attr_temp3_min_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp3_max_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp3_max_hyst_alarm.dev_attr.attr,
> + /* GPADC SENSOR4 */
> + &sensor_dev_attr_temp4_label.dev_attr.attr,
> + &sensor_dev_attr_temp4_input.dev_attr.attr,
> + &sensor_dev_attr_temp4_min.dev_attr.attr,
> + &sensor_dev_attr_temp4_max.dev_attr.attr,
> + &sensor_dev_attr_temp4_max_hyst.dev_attr.attr,
> + &sensor_dev_attr_temp4_min_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp4_max_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp4_max_hyst_alarm.dev_attr.attr,
> + /* GPADC SENSOR5*/
space after SENSOR5
> + &sensor_dev_attr_temp5_label.dev_attr.attr,
> + &sensor_dev_attr_temp5_input.dev_attr.attr,
> + &sensor_dev_attr_temp5_min.dev_attr.attr,
> + &sensor_dev_attr_temp5_max.dev_attr.attr,
> + &sensor_dev_attr_temp5_max_hyst.dev_attr.attr,
> + &sensor_dev_attr_temp5_min_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp5_max_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp5_max_hyst_alarm.dev_attr.attr,
> + &sensor_dev_attr_temp5_crit_alarm.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group abx500_temp_group = {
> + .attrs = abx500_temp_attributes,
> + .is_visible = abx500_attrs_visible,
> +};
> +
> +static irqreturn_t abx500_temp_irq_handler(int irq, void *irq_data)
> +{
> + struct platform_device *pdev = irq_data;
> + struct abx500_temp *data = platform_get_drvdata(pdev);
> +
> + data->ops.irq_handler(irq, data);
> + return IRQ_HANDLED;
> +}
> +
> +static int setup_irqs(struct platform_device *pdev)
> +{
> + int ret;
> + int irq = platform_get_irq_byname(pdev, "ABX500_TEMP_WARM");
> +
> + if (irq < 0) {
> + dev_err(&pdev->dev, "Get irq by name failed\n");
> + return irq;
> + }
> +
> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> + abx500_temp_irq_handler, IRQF_NO_SUSPEND, "abx500-temp", pdev);
> + if (ret < 0)
> + dev_err(&pdev->dev, "Request threaded irq failed (%d)\n", ret);
> +
> + return ret;
> +}
> +
> +static int abx500_temp_probe(struct platform_device *pdev)
> +{
> + struct abx500_temp *data;
> + int err;
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->pdev = pdev;
> + mutex_init(&data->lock);
> +
> + /* Chip specific initialization */
> + err = abx500_hwmon_init(data);
> + if (err < 0 || !data->ops.read_sensor || !data->ops.irq_handler
> + || !data->ops.show_name || !data->ops.show_label
> + || !data->ops.is_visible) {
> + dev_err(&pdev->dev, "ABx500 hwmon init failed");
If err == 0 but any of the ops is not set, you'll return 0
(no error).
> + return err;
> + }
> +
> + data->hwmon_dev = hwmon_device_register(&pdev->dev);
> + if (IS_ERR(data->hwmon_dev)) {
> + err = PTR_ERR(data->hwmon_dev);
> + dev_err(&pdev->dev, "Class registration failed (%d)\n", err);
> + return err;
> + }
This has to be the last call after everything else is set up. The ABI expects
all attributes to exist when this call is made.
> +
> + INIT_DEFERRABLE_WORK(&data->work, gpadc_monitor);
> + data->gpadc_monitor_delay = DEFAULT_MONITOR_DELAY;
> +
> + platform_set_drvdata(pdev, data);
> +
> + err = sysfs_create_group(&pdev->dev.kobj, &abx500_temp_group);
> + if (err < 0) {
> + dev_err(&pdev->dev, "Create sysfs group failed (%d)\n", err);
> + goto exit_platform_data;
> + }
> +
> + err = setup_irqs(pdev);
> + if (err < 0) {
> + dev_err(&pdev->dev, "irq setup failed (%d)\n", err);
> + goto exit_sysfs_group;
> + }
> + return 0;
> +
> +exit_sysfs_group:
> + sysfs_remove_group(&pdev->dev.kobj, &abx500_temp_group);
> +exit_platform_data:
> + platform_set_drvdata(pdev, NULL);
Unnecessary.
> + hwmon_device_unregister(data->hwmon_dev);
> + return err;
> +}
> +
> +static int abx500_temp_remove(struct platform_device *pdev)
> +{
> + struct abx500_temp *data = platform_get_drvdata(pdev);
> +
> + gpadc_monitor_exit(data);
Question here is if someone can access attribute files between those two calls.
If so, the monitor might be restarted. Might be an interesting test - add a
sleep here for,say, 10 seconds, then try to set a limit during that time.
If it crashes, you'll know that you have a race condition.
> + sysfs_remove_group(&pdev->dev.kobj, &abx500_temp_group);
> + platform_set_drvdata(pdev, NULL);
Unnecessary.
> + hwmon_device_unregister(data->hwmon_dev);
Has to come first, prior to removing the attributes.
> + return 0;
> +}
> +
> +static int abx500_temp_suspend(struct platform_device *pdev,
> + pm_message_t state)
> +{
> + struct abx500_temp *data = platform_get_drvdata(pdev);
> +
> + if (data->work_active)
> + cancel_delayed_work_sync(&data->work);
> + return 0;
> +}
> +
> +static int abx500_temp_resume(struct platform_device *pdev)
> +{
> + struct abx500_temp *data = platform_get_drvdata(pdev);
> +
> + if (data->work_active)
> + schedule_monitor(data);
Is the suspend/remove code guaranteed to be synchronized,
or should the above be mutex protected ?
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id abx500_temp_match[] = {
> + { .compatible = "stericsson,abx500-temp" },
> + {},
> +};
> +#endif
> +
> +static struct platform_driver abx500_temp_driver = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "abx500-temp",
> + .of_match_table = of_match_ptr(abx500_temp_match),
> + },
> + .suspend = abx500_temp_suspend,
> + .resume = abx500_temp_resume,
> + .probe = abx500_temp_probe,
> + .remove = abx500_temp_remove,
> +};
> +
> +module_platform_driver(abx500_temp_driver);
> +
> +MODULE_AUTHOR("Martin Persson <martin.persson@...ricsson.com>");
> +MODULE_DESCRIPTION("ABX500 temperature driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/hwmon/abx500.h b/drivers/hwmon/abx500.h
> new file mode 100644
> index 0000000..660a38e
> --- /dev/null
> +++ b/drivers/hwmon/abx500.h
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright (C) ST-Ericsson SA 2010
> + * License terms: GNU General Public License v2
> + * Author: Martin Persson <martin.persson@...ricsson.com>
> + */
> +
> +#ifndef _ABX500_H
> +#define _ABX500_H
> +
> +#define NUM_SENSORS 5
> +
> +struct ab8500_gpadc;
> +struct ab8500_btemp;
> +struct abx500_temp;
> +
> +/*
> + * struct abx500_temp_ops - abx500 chip specific ops
> + * @read_sensor: reads gpadc output
> + * @irq_handler: irq handler
> + * @show_name: hwmon device name
> + * @show_label: hwmon attribute label
> + * @is_visible: is attribute visible
> + */
> +struct abx500_temp_ops {
> + int (*read_sensor)(struct abx500_temp *, u8);
> + int (*irq_handler)(int, struct abx500_temp *);
> + ssize_t (*show_name)(struct device *,
> + struct device_attribute *, char *);
> + ssize_t (*show_label) (struct device *,
> + struct device_attribute *, char *);
> + int (*is_visible)(struct attribute *, int);
> +};
> +
> +/*
> + * struct abx500_temp - representation of temp mon device
> + * @pdev: platform device
> + * @hwmon_dev: hwmon device
> + * @ab8500_gpadc: gpadc interface for ab8500
> + * @btemp: battery temperature interface for ab8500
> + * @gpadc_addr: gpadc channel address
> + * @temp: sensor temperature input value
> + * @min: sensor temperature min value
> + * @max: sensor temperature max value
> + * @max_hyst: sensor temperature hysteresis value for max limit
> + * @crit: sensor temperature critical value
> + * @min_alarm: sensor temperature min alarm
> + * @max_alarm: sensor temperature max alarm
> + * @max_hyst_alarm: sensor temperature hysteresis alarm
> + * @crit_alarm: sensor temperature critical value alarm
> + * @work: delayed work scheduled to monitor temperature periodically
> + * @work_active: True if work is active
> + * @power_off_work: delayed work scheduled to power off the system
> + * when critical temperature is reached
> + * @lock: mutex
> + * @gpadc_monitor_delay: delay between temperature readings in ms
> + * @power_off_delay: delay before power off in ms
> + * @monitored_sensors: number of monitored sensors
> + */
> +struct abx500_temp {
> + struct platform_device *pdev;
> + struct device *hwmon_dev;
> + struct ab8500_gpadc *ab8500_gpadc;
> + struct ab8500_btemp *ab8500_btemp;
> + struct abx500_temp_ops ops;
> + u8 gpadc_addr[NUM_SENSORS];
> + unsigned long temp[NUM_SENSORS];
> + unsigned long min[NUM_SENSORS];
> + unsigned long max[NUM_SENSORS];
> + unsigned long max_hyst[NUM_SENSORS];
> + unsigned long crit[NUM_SENSORS];
> + unsigned long min_alarm[NUM_SENSORS];
> + unsigned long max_alarm[NUM_SENSORS];
> + unsigned long max_hyst_alarm[NUM_SENSORS];
> + unsigned long crit_alarm[NUM_SENSORS];
Why are the alarms unsigned long and not bool ?
> + struct delayed_work work;
> + bool work_active;
> + struct delayed_work power_off_work;
> + struct mutex lock;
> + /* Delay (ms) between temperature readings */
> + unsigned long gpadc_monitor_delay;
> + /* Delay (ms) before power off */
> + unsigned long power_off_delay;
> + int monitored_sensors;
> +};
> +
> +int __init abx500_hwmon_init(struct abx500_temp *data);
> +
> +#endif /* _ABX500_H */
> --
> 1.8.0
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists