Guenter, Thanks for you thorough review very much, will adopt all your comments. On 27 February 2013 13:56, Guenter Roeck <linux@xxxxxxxxxxxx> wrote: > On Thu, Feb 21, 2013 at 06:32:41PM +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@xxxxxxxxxx> >> --- >> Documentation/hwmon/ab8500 | 22 ++ >> Documentation/hwmon/abx500 | 26 +++ >> drivers/hwmon/Kconfig | 13 ++ >> drivers/hwmon/Makefile | 1 + >> drivers/hwmon/ab8500.c | 178 ++++++++++++++++ >> drivers/hwmon/abx500.c | 501 +++++++++++++++++++++++++++++++++++++++++++++ >> drivers/hwmon/abx500.h | 87 ++++++++ >> 7 files changed, 828 insertions(+) >> create mode 100644 Documentation/hwmon/ab8500 >> create mode 100644 Documentation/hwmon/abx500 >> create mode 100644 drivers/hwmon/ab8500.c >> create mode 100644 drivers/hwmon/abx500.c >> create mode 100644 drivers/hwmon/abx500.h >> >> diff --git a/Documentation/hwmon/ab8500 b/Documentation/hwmon/ab8500 >> new file mode 100644 >> index 0000000..76c534d >> --- /dev/null >> +++ b/Documentation/hwmon/ab8500 >> @@ -0,0 +1,22 @@ >> +Kernel driver ab8500 >> +==================== >> + >> +Supported chips: >> + * ST-Ericsson AB8500 >> + Prefix: 'ab8500' >> + Addresses scanned: - >> + Datasheet: http://www.stericsson.com/developers/documentation.jsp >> + >> +Authors: >> + Martin Persson <martin.persson@xxxxxxxxxxxxxx> >> + Hongbo Zhang <hongbo.zhang@xxxxxxxxxx> >> + >> +Description >> +----------- >> + >> +See also Documentation/hwmon/abx500. This is ST-Ericsson AB8500 hwmon specific >> +initialization. >> + > "This is the ST-Ericsson AB8500 specific driver" or similar might be better. > >> +Currently only the AB8500 internal sensor and one external sensor for battery >> +temperature are monitored. Other GPADC channels can also be monitored if needed >> +in future. >> diff --git a/Documentation/hwmon/abx500 b/Documentation/hwmon/abx500 >> new file mode 100644 >> index 0000000..f60b73c >> --- /dev/null >> +++ b/Documentation/hwmon/abx500 >> @@ -0,0 +1,26 @@ >> +Kernel driver abx500 >> +==================== >> + >> +Supported chips: >> + * ST-Ericsson ABx500 series >> + Prefix: 'abx500' >> + Addresses scanned: - >> + Datasheet: http://www.stericsson.com/developers/documentation.jsp >> + >> +Authors: >> + Martin Persson <martin.persson@xxxxxxxxxxxxxx> >> + Hongbo Zhang <hongbo.zhang@xxxxxxxxxx> >> + >> +Description >> +----------- >> + >> +Every ST-Ericsson Ux500 SOC consists of both ABx500 and DBx500 physically, >> +this is kernel hwmon driver for ABx500. >> + >> +There are some GPADCs inside ABx500 which are designed for connecting to >> +thermal sensors, and there is also a thermal sensor inside ABx500 too, which >> +raises interrupt when critical temperature reached. >> + >> +This abx500 is a common layer which can monitor all of the sensors, every >> +specific abx500 chip has its special configurations in its own file, e.g. some >> +sensors can be configured invisible if they are not available on that chip. > > Given that limits are disabled if set to 0, you really should document that > here. > >> 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..33221e7 >> --- /dev/null >> +++ b/drivers/hwmon/ab8500.c >> @@ -0,0 +1,178 @@ >> +/* >> + * Copyright (C) ST-Ericsson SA 2010 > > 2010 - 1013 ? > >> + * Author: Martin Persson <martin.persson@xxxxxxxxxxxxxx> >> + * Hongbo Zhang <hongbo.zhang@xxxxxxxxxx> >> + * 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.h> >> +#include <linux/mfd/abx500/ab8500-bm.h> >> +#include <linux/mfd/abx500/ab8500-gpadc.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/slab.h> >> +#include <linux/sysfs.h> >> +#include "abx500.h" >> + >> +#define DEFAULT_POWER_OFF_DELAY 10000 > > I notice you define all delays in ms just to convert them to jiffies, yet the > delays are hardly ever reported to the user anywhere. I think it would make > more sense to just define the delays in HZ, use them directly, and use > msecs_to_jiffies() in the one case where a delay reported to the user. > This would have the added benefit that (HZ * 10) is easier to understand > than an unexplained 10000 when reading the code. > >> +#define NUM_MONITORED_SENSORS 4 >> +#define THERMAL_VCC 1800 >> +#define PULL_UP_RESISTOR 47000 >> + >> +/* >> + * The hardware connection is like this: >> + * VCC----[ R_up ]-----[ NTC ]----GND >> + * where R_up is pull-up resistance, and GPADC measures voltage on NTC. >> + * and res_to_temp table is strictly sorted by falling resistance values. >> + */ >> +static int voltage_to_temp(int vcc, int r_up, int v_ntc, >> + const struct abx500_res_to_temp *tbl, int tbl_sz, int *temp) >> +{ >> + int r_ntc, i = 0; >> + >> + if (vcc < 0 || v_ntc > vcc) > > Should be 'v_ntc >= vcc' to avoid the == case and resulting division by zero. > >> + return -EINVAL; >> + >> + r_ntc = v_ntc * r_up / (vcc - v_ntc); >> + if (r_ntc > tbl[0].resist || r_ntc < tbl[tbl_sz - 1].resist) >> + return -EINVAL; >> + >> + while (!(r_ntc <= tbl[i].resist && r_ntc > tbl[i + 1].resist) >> + && i < tbl_sz - 2) >> + i++; >> + >> + *temp = tbl[i].temp + ((tbl[i + 1].temp - tbl[i].temp) * >> + (r_ntc - tbl[i].resist)) / (tbl[i + 1].resist - tbl[i].resist); >> + >> + return 0; >> +} >> + >> +static int ab8500_read_sensor(struct abx500_temp *data, u8 sensor) >> +{ >> + int temp, voltage, ret; >> + >> + if (sensor == BAT_CTRL) >> + temp = ab8500_btemp_get_batctrl_temp(data->ab8500_btemp); > > That can return a valid number below zero unless I am missing something > (BTEMP_THERMAL_LOW_LIMIT is -10). Which means you can not return the error > and the return value and need to use a pointer for the return value after all. > >> + >> + else if (sensor == BTEMP_BALL) >> + temp = ab8500_btemp_get_temp(data->ab8500_btemp); > > Same here. > >> + >> + else { >> + voltage = ab8500_gpadc_convert(data->ab8500_gpadc, sensor); >> + if (voltage < 0) >> + return -EINVAL; > > return voltage; >> + >> + ret = voltage_to_temp(THERMAL_VCC, PULL_UP_RESISTOR, voltage, >> + temp_tbl_A_thermistor, temp_tbl_A_size, &temp); > > This can also return a negative temperature (the first temperature entry in > temp_tbl_A_thermistor is negative). > >> + if (ret < 0) >> + return -EINVAL; > > return ret; > >> + temp *= 1000; >> + } >> + >> + return temp; >> +} >> + >> +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; > > Nitpick: label, really. > >> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >> + int index = attr->index; >> + >> + switch (index) { >> + case 1: >> + name = "ext_adc1"; >> + break; >> + case 2: >> + name = "ext_adc2"; >> + break; >> + case 3: >> + name = "bat_temp"; >> + break; >> + case 4: >> + name = "bat_ctrl"; >> + break; >> + default: >> + return -EINVAL; >> + } >> + return sprintf(buf, "%s\n", name); >> +} >> + >> +static int ab8500_is_visible(struct attribute *attr, int n) >> +{ >> + return attr->mode; >> +} > > Instead of providing an empty function, you should make is_visible optional > and have the calling code return attr->mode directly if it is not defined > (ie NULL). > >> + >> +static int ab8500_temp_irq_handler(int irq, struct abx500_temp *data) >> +{ >> + unsigned long delay_in_jiffies; >> + >> + dev_info(&data->pdev->dev, "AB8500 warning, power off in %lu s\n", >> + data->power_off_delay); >> + > dev_warn, and "AB8500 warning" is redundant. > >> + 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) >> +{ >> + data->ab8500_gpadc = ab8500_gpadc_get("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); >> + >> + /* >> + * ADC_AUX1 and ADC_AUX2, connected to external NTC >> + * BTEMP_BALL and BAT_CTRL, fixed usage >> + */ >> + 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->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..69ea8cb >> --- /dev/null >> +++ b/drivers/hwmon/abx500.c >> @@ -0,0 +1,501 @@ >> +/* >> + * Copyright (C) ST-Ericsson SA 2010 > > 2010 - 1023 ? > >> + * Author: Martin Persson <martin.persson@xxxxxxxxxxxxxx> >> + * Hongbo Zhang <hongbo.zhang@xxxxxxxxxx> >> + * 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. >> + * >> + * 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 >> + >> +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 void threshold_updated(struct abx500_temp *data) >> +{ >> + int i; >> + for (i = 0; i < data->monitored_sensors; i++) >> + if (data->max[i] != 0 || data->min[i] != 0) { >> + schedule_monitor(data); >> + return ; > > Extra ' ' before ; > >> + } >> + >> + dev_dbg(&data->pdev->dev, "No active thresholds.\n"); >> + cancel_delayed_work_sync(&data->work); >> + data->work_active = false; >> +} >> + >> +static void gpadc_monitor(struct work_struct *work) >> +{ >> + int val, i, ret; >> + char alarm_node[30]; >> + bool updated_min_alarm = false; >> + bool updated_max_alarm = false; >> + struct abx500_temp *data = container_of(work, struct abx500_temp, >> + work.work); >> + >> + mutex_lock(&data->lock); >> + for (i = 0; i < data->monitored_sensors; i++) { >> + /* Thresholds are considered inactive if set to 0 */ >> + if (data->max[i] == 0 && data->min[i] == 0) >> + continue; >> + /* >> + * In case we are in the temporary state that one threshold >> + * has been changed, but the other hasn't yet. >> + */ >> + if (data->max[i] < data->min[i]) >> + continue; >> + >> + val = data->ops.read_sensor(data, data->gpadc_addr[i]); >> + if (val < 0) { >> + dev_err(&data->pdev->dev, "GPADC read failed\n"); >> + continue; >> + } >> + >> + if (data->min[i] != 0) { >> + if (val < data->min[i]) { >> + if (data->min_alarm[i] == false) { >> + data->min_alarm[i] = true; >> + updated_min_alarm = true; >> + } >> + } else { >> + if (data->min_alarm[i] == true) { >> + data->min_alarm[i] = false; >> + updated_min_alarm = true; >> + } >> + } >> + > Unnecessary empty line. > >> + } >> + if (data->max[i] != 0) { >> + if (val > data->max[i]) { >> + if (data->max_alarm[i] == false) { >> + data->max_alarm[i] = true; >> + updated_max_alarm = true; >> + } >> + } else if (val < data->max[i] - data->max_hyst[i]) { >> + if (data->max_alarm[i] == true) { >> + data->max_alarm[i] = false; >> + updated_max_alarm = true; >> + } >> + } >> + } >> + >> + if (updated_min_alarm) { >> + ret = sprintf(alarm_node, "temp%d_min_alarm", i); >> + sysfs_notify(&data->pdev->dev.kobj, NULL, alarm_node); >> + } >> + if (updated_max_alarm) { >> + ret = sprintf(alarm_node, "temp%d_max_alarm", i); >> + sysfs_notify(&data->pdev->dev.kobj, NULL, alarm_node); >> + } >> + >> + updated_min_alarm = false; >> + updated_max_alarm = false; >> + } >> + >> + schedule_monitor(data); >> + mutex_unlock(&data->lock); >> +} >> + >> +/* HWMON sysfs interfaces */ >> +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]; >> + >> + 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) */ >> +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; >> + > > You should use kstrtol, and use clamp_val() to limit the range. We don't expect > users to know the valid limits for this chip, and furthermore the limit could be > the result of a calculation. > > Sure you want to accept arbitrary upper limits ? 1,000,000 degrees C ? > OK, learned, thanks. >> + mutex_lock(&data->lock); >> + data->min[attr->index] = val; >> + threshold_updated(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); > > Same as above. > >> + if (res < 0) >> + return res; >> + >> + mutex_lock(&data->lock); >> + data->max[attr->index] = val; >> + threshold_updated(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); >> + data->max_hyst[attr->index] = val; >> + threshold_updated(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]); >> +} >> + >> +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]); >> +} >> + >> +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]); >> +} >> + >> +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, "%d\n", data->min_alarm[attr->index]); >> +} >> + >> +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, "%d\n", data->max_alarm[attr->index]); >> +} >> + >> +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); >> +} >> + >> +/* Chip name, required by hwmon */ >> +static SENSOR_DEVICE_ATTR(name, S_IRUGO, show_name, NULL, 0); >> + >> +/* GPADC - SENSOR1 */ >> +static SENSOR_DEVICE_ATTR(temp1_label, S_IRUGO, show_label, NULL, 0); >> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_input, NULL, 0); >> +static SENSOR_DEVICE_ATTR(temp1_min, S_IWUSR | S_IRUGO, show_min, set_min, 0); >> +static SENSOR_DEVICE_ATTR(temp1_max, S_IWUSR | S_IRUGO, show_max, set_max, 0); >> +static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IWUSR | S_IRUGO, >> + show_max_hyst, set_max_hyst, 0); >> +static SENSOR_DEVICE_ATTR(temp1_min_alarm, S_IRUGO, show_min_alarm, NULL, 0); >> +static SENSOR_DEVICE_ATTR(temp1_max_alarm, S_IRUGO, show_max_alarm, NULL, 0); >> + >> +/* GPADC - SENSOR2 */ >> +static SENSOR_DEVICE_ATTR(temp2_label, S_IRUGO, show_label, NULL, 1); >> +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_input, NULL, 1); >> +static SENSOR_DEVICE_ATTR(temp2_min, S_IWUSR | S_IRUGO, show_min, set_min, 1); >> +static SENSOR_DEVICE_ATTR(temp2_max, S_IWUSR | S_IRUGO, show_max, set_max, 1); >> +static SENSOR_DEVICE_ATTR(temp2_max_hyst, S_IWUSR | S_IRUGO, >> + show_max_hyst, set_max_hyst, 1); >> +static SENSOR_DEVICE_ATTR(temp2_min_alarm, S_IRUGO, show_min_alarm, NULL, 1); >> +static SENSOR_DEVICE_ATTR(temp2_max_alarm, S_IRUGO, show_max_alarm, NULL, 1); >> + >> +/* GPADC - SENSOR3 */ >> +static SENSOR_DEVICE_ATTR(temp3_label, S_IRUGO, show_label, NULL, 2); >> +static SENSOR_DEVICE_ATTR(temp3_input, S_IRUGO, show_input, NULL, 2); >> +static SENSOR_DEVICE_ATTR(temp3_min, S_IWUSR | S_IRUGO, show_min, set_min, 2); >> +static SENSOR_DEVICE_ATTR(temp3_max, S_IWUSR | S_IRUGO, show_max, set_max, 2); >> +static SENSOR_DEVICE_ATTR(temp3_max_hyst, S_IWUSR | S_IRUGO, >> + show_max_hyst, set_max_hyst, 2); >> +static SENSOR_DEVICE_ATTR(temp3_min_alarm, S_IRUGO, show_min_alarm, NULL, 2); >> +static SENSOR_DEVICE_ATTR(temp3_max_alarm, S_IRUGO, show_max_alarm, NULL, 2); >> + >> +/* GPADC - SENSOR4 */ >> +static SENSOR_DEVICE_ATTR(temp4_label, S_IRUGO, show_label, NULL, 3); >> +static SENSOR_DEVICE_ATTR(temp4_input, S_IRUGO, show_input, NULL, 3); >> +static SENSOR_DEVICE_ATTR(temp4_min, S_IWUSR | S_IRUGO, show_min, set_min, 3); >> +static SENSOR_DEVICE_ATTR(temp4_max, S_IWUSR | S_IRUGO, show_max, set_max, 3); >> +static SENSOR_DEVICE_ATTR(temp4_max_hyst, S_IWUSR | S_IRUGO, >> + show_max_hyst, set_max_hyst, 3); >> +static SENSOR_DEVICE_ATTR(temp4_min_alarm, S_IRUGO, show_min_alarm, NULL, 3); >> +static SENSOR_DEVICE_ATTR(temp4_max_alarm, S_IRUGO, show_max_alarm, NULL, 3); >> + >> +struct attribute *abx500_temp_attributes[] = { >> + &sensor_dev_attr_name.dev_attr.attr, >> + >> + &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_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_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_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, >> + 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.show_name >> + || !data->ops.show_label || !data->ops.is_visible) { > > Nitpick: Second line should be aligned with '('. > >> + dev_err(&pdev->dev, "ABx500 hwmon init failed"); >> + return -EINVAL; >> + } >> + >> + INIT_DEFERRABLE_WORK(&data->work, gpadc_monitor); >> + data->gpadc_monitor_delay = DEFAULT_MONITOR_DELAY; > > Any benefit of having this as variable in the first place ? Why not just use the > define ? > > Nitpick: Extra space after '='. > >> + 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); >> + 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); >> + goto exit_sysfs_group; >> + } >> + >> + if (data->ops.irq_handler) { >> + err = setup_irqs(pdev); >> + if (err < 0) { >> + dev_err(&pdev->dev, "irq setup failed (%d)\n", err); >> + goto exit_hwmon_reg; >> + } >> + } >> + return 0; >> + >> +exit_hwmon_reg: >> + hwmon_device_unregister(data->hwmon_dev); >> +exit_sysfs_group: >> + sysfs_remove_group(&pdev->dev.kobj, &abx500_temp_group); >> + return err; >> +} >> + >> +static int abx500_temp_remove(struct platform_device *pdev) >> +{ >> + struct abx500_temp *data = platform_get_drvdata(pdev); >> + >> + mutex_lock(&data->lock); >> + hwmon_device_unregister(data->hwmon_dev); >> + sysfs_remove_group(&pdev->dev.kobj, &abx500_temp_group); >> + cancel_delayed_work_sync(&data->work); > > Should come first. > >> + mutex_unlock(&data->lock); > > This mutex protection is definitely not needed. > >> + >> + return 0; >> +} >> + >> +static int abx500_temp_suspend(struct platform_device *pdev, >> + pm_message_t state) >> +{ >> + struct abx500_temp *data = platform_get_drvdata(pdev); >> + >> + mutex_lock(&data->lock); >> + if (data->work_active) >> + cancel_delayed_work_sync(&data->work); >> + mutex_unlock(&data->lock); > > I don't think this mutex protection is needed. Looking into other drivers, they > commonly cancel work outside mutex protection. > > What happens if poweroff due to overheating is pending ? > >> + >> + 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); >> + 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@xxxxxxxxxxxxxx>"); >> +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..c1dd41d >> --- /dev/null >> +++ b/drivers/hwmon/abx500.h >> @@ -0,0 +1,87 @@ >> +/* >> + * Copyright (C) ST-Ericsson SA 2010 > > 2010 - 2013 ? > >> + * License terms: GNU General Public License v2 >> + * Author: Martin Persson <martin.persson@xxxxxxxxxxxxxx> >> + */ >> + >> +#ifndef _ABX500_H >> +#define _ABX500_H >> + >> +#define NUM_SENSORS 5 >> + >> +struct ab8500_gpadc; >> +struct ab8500_btemp; >> +struct abx500_temp; >> + >> +extern struct abx500_res_to_temp temp_tbl_A_thermistor[]; >> +extern int temp_tbl_A_size; > > Those variables should be defined in the defining code in an exporting > include file, which this driver should include. Also, the name should be less > generic and refer to the introducing module (abx500_temp_tbl_a_thermistor, > maybe). > > Also, CamelCase variable names are discouraged nowadays and create a checkpatch > warning. Please take that into account when selecting a different name. > Talked to the owner of that part, we will move the data to proper place, and rename the variables. >> + >> +/* >> + * 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 >> + * @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 >> + * @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 min[NUM_SENSORS]; >> + unsigned long max[NUM_SENSORS]; >> + unsigned long max_hyst[NUM_SENSORS]; >> + unsigned long crit[NUM_SENSORS]; > > Not used anywhere. > >> + bool min_alarm[NUM_SENSORS]; >> + bool max_alarm[NUM_SENSORS]; >> + bool crit_alarm[NUM_SENSORS]; > > Not used anywhere. > >> + 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; > > Many of those variables are only used in the abx500 code. You should have two > structures, one defined inside the abx500 code for variables only used there, > and one exported to other drivers. The exported structure could be embedded in > the private one. > > Similar, you should have a private data structure in the ab8500 driver, for > variables only used there. > Yes, will consider this. (historic reason is that these two files was one file before, and then separated into two) >> +}; >> + >> +int abx500_hwmon_init(struct abx500_temp *data); >> + >> +#endif /* _ABX500_H */ >> -- >> 1.8.0 >> >> _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
