Guenter Roeck <linux@xxxxxxxxxxxx> writes: > On 08/03/2015 08:22 AM, Punit Agrawal wrote: >> Create a driver to add support for SoC sensors exported by the System >> Control Processor (SCP) via the System Control and Power Interface >> (SCPI). The supported sensor types is one of voltage, temperature, >> current, and power. >> >> The sensor labels and values provided by the SCP are exported via the >> hwmon sysfs interface. >> >> Signed-off-by: Punit Agrawal <punit.agrawal@xxxxxxx> >> Cc: Jean Delvare <jdelvare@xxxxxxx> >> Cc: Guenter Roeck <linux@xxxxxxxxxxxx> >> Cc: Sudeep Holla <sudeep.holla@xxxxxxx> > > Looks good for the most part. Single comment below. > Assuming you fix it up, feel free to add > > Acked-by: Guenter Roeck <linux@xxxxxxxxxxxx> > > for v3. > > Thanks, > Guenter > >> --- >> Documentation/hwmon/scpi-hwmon | 33 ++++++++ >> drivers/hwmon/Kconfig | 8 ++ >> drivers/hwmon/Makefile | 1 + >> drivers/hwmon/scpi-hwmon.c | 183 +++++++++++++++++++++++++++++++++++++++++ >> 4 files changed, 225 insertions(+) >> create mode 100644 Documentation/hwmon/scpi-hwmon >> create mode 100644 drivers/hwmon/scpi-hwmon.c >> >> diff --git a/Documentation/hwmon/scpi-hwmon b/Documentation/hwmon/scpi-hwmon >> new file mode 100644 >> index 0000000..4cfcdf2d >> --- /dev/null >> +++ b/Documentation/hwmon/scpi-hwmon >> @@ -0,0 +1,33 @@ >> +Kernel driver scpi-hwmon >> +======================== >> + >> +Supported chips: >> + * Chips based on ARM System Control Processor Interface >> + Addresses scanned: - >> + Datasheet: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0922b/index.html >> + >> +Author: Punit Agrawal <punit.agrawal@xxxxxxx> >> + >> +Description >> +----------- >> + >> +This driver supports hardware monitoring for SoC's based on the ARM >> +System Control Processor (SCP) implementing the System Control >> +Processor Interface (SCPI). The following sensor types are supported >> +by the SCP - >> + >> + * temperature >> + * voltage >> + * current >> + * power >> + >> +The SCP interface provides an API to query the available sensors and >> +their values which are then exported to userspace by this driver. >> + >> +Usage Notes >> +----------- >> + >> +The driver relies on device tree node to indicate the presence of SCPI >> +support in the kernel. See >> +Documentation/devicetree/bindings/arm/arm,scpi.txt for details of the >> +devicetree node. >> \ No newline at end of file >> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig >> index 4943c3c..c9714b0 100644 >> --- a/drivers/hwmon/Kconfig >> +++ b/drivers/hwmon/Kconfig >> @@ -1551,6 +1551,14 @@ config SENSORS_VEXPRESS >> the ARM Ltd's Versatile Express platform. It can provide wide >> range of information like temperature, power, energy. >> >> +config SENSORS_ARM_SCPI >> + tristate "ARM SCPI Sensors" >> + depends on ARM_SCPI_PROTOCOL >> + help >> + This driver provides support for temperature, voltage, current >> + and power sensors available on ARM Ltd's SCP based platforms. The >> + actual number and type of sensors exported depend the platform. >> + >> config SENSORS_VIA_CPUTEMP >> tristate "VIA CPU temperature sensor" >> depends on X86 >> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile >> index 8aba87f..4961710 100644 >> --- a/drivers/hwmon/Makefile >> +++ b/drivers/hwmon/Makefile >> @@ -150,6 +150,7 @@ obj-$(CONFIG_SENSORS_TMP421) += tmp421.o >> obj-$(CONFIG_SENSORS_TWL4030_MADC)+= twl4030-madc-hwmon.o >> obj-$(CONFIG_SENSORS_V2M_JUNO) += v2m-juno.o >> obj-$(CONFIG_SENSORS_VEXPRESS) += vexpress.o >> +obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o >> obj-$(CONFIG_SENSORS_VIA_CPUTEMP)+= via-cputemp.o >> obj-$(CONFIG_SENSORS_VIA686A) += via686a.o >> obj-$(CONFIG_SENSORS_VT1211) += vt1211.o >> diff --git a/drivers/hwmon/scpi-hwmon.c b/drivers/hwmon/scpi-hwmon.c >> new file mode 100644 >> index 0000000..7e7e06b >> --- /dev/null >> +++ b/drivers/hwmon/scpi-hwmon.c >> @@ -0,0 +1,183 @@ >> +/* >> + * System Control and Power Interface(SCPI) based hwmon sensor driver >> + * >> + * Copyright (C) 2015 ARM Ltd. >> + * Punit Agrawal <punit.agrawal@xxxxxxx> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any >> + * kind, whether express or implied; without even the implied warranty >> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#include <linux/hwmon.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/scpi_protocol.h> >> +#include <linux/slab.h> >> +#include <linux/sysfs.h> >> + >> +struct sensor_data { >> + struct scpi_sensor_info info; >> + struct device_attribute dev_attr_input; >> + struct device_attribute dev_attr_label; >> + char input[20]; >> + char label[20]; >> +}; >> + >> +struct scpi_sensors { >> + struct scpi_ops *scpi_ops; >> + struct sensor_data *data; >> + struct attribute **attrs; >> + struct attribute_group group; >> + const struct attribute_group *groups[2]; >> +}; >> + >> +/* hwmon callback functions */ >> +static ssize_t >> +scpi_show_sensor(struct device *dev, struct device_attribute *attr, char *buf) >> +{ >> + struct scpi_sensors *scpi_sensors = dev_get_drvdata(dev); >> + struct scpi_ops *scpi_ops = scpi_sensors->scpi_ops; >> + struct sensor_data *sensor; >> + u32 value; >> + int ret; >> + >> + sensor = container_of(attr, struct sensor_data, dev_attr_input); >> + >> + ret = scpi_ops->sensor_get_value(sensor->info.sensor_id, &value); >> + if (ret) >> + return ret; >> + >> + return sprintf(buf, "%u\n", value); >> +} >> + >> +static ssize_t >> +scpi_show_label(struct device *dev, struct device_attribute *attr, char *buf) >> +{ >> + struct sensor_data *sensor; >> + >> + sensor = container_of(attr, struct sensor_data, dev_attr_label); >> + >> + return sprintf(buf, "%s\n", sensor->info.name); >> +} >> + >> +static int scpi_hwmon_probe(struct platform_device *pdev) >> +{ >> + u16 nr_sensors, i; >> + int num_temp = 0, num_volt = 0, num_current = 0, num_power = 0; >> + struct scpi_ops *scpi_ops; >> + struct device *hwdev, *dev = &pdev->dev; >> + struct scpi_sensors *scpi_sensors; >> + int ret; >> + >> + scpi_ops = get_scpi_ops(); >> + if (!scpi_ops) >> + return -EPROBE_DEFER; >> + >> + ret = scpi_ops->sensor_get_capability(&nr_sensors); >> + if (ret || !nr_sensors) >> + return ret; >> + > This will return 0 if nr_sensors == 0. Should it be -ENODEV ? > Good catch. I've fixed this up to return -ENODEV. Thanks for the ack. Punit >> + scpi_sensors = devm_kzalloc(dev, sizeof(*scpi_sensors), GFP_KERNEL); >> + if (!scpi_sensors) >> + return -ENOMEM; >> + >> + scpi_sensors->data = devm_kcalloc(dev, nr_sensors, >> + sizeof(*scpi_sensors->data), GFP_KERNEL); >> + if (!scpi_sensors->data) >> + return -ENOMEM; >> + >> + scpi_sensors->attrs = devm_kcalloc(dev, (nr_sensors * 2) + 1, >> + sizeof(*scpi_sensors->attrs), GFP_KERNEL); >> + if (!scpi_sensors->attrs) >> + return -ENOMEM; >> + >> + scpi_sensors->scpi_ops = scpi_ops; >> + >> + for (i = 0; i < nr_sensors; i++) { >> + struct sensor_data *sensor = &scpi_sensors->data[i]; >> + >> + ret = scpi_ops->sensor_get_info(i, &sensor->info); >> + if (ret) >> + return ret; >> + >> + switch (sensor->info.class) { >> + case TEMPERATURE: >> + snprintf(sensor->input, sizeof(sensor->input), >> + "temp%d_input", num_temp + 1); >> + snprintf(sensor->label, sizeof(sensor->input), >> + "temp%d_label", num_temp + 1); >> + num_temp++; >> + break; >> + case VOLTAGE: >> + snprintf(sensor->input, sizeof(sensor->input), >> + "in%d_input", num_volt); >> + snprintf(sensor->label, sizeof(sensor->input), >> + "in%d_label", num_volt); >> + num_volt++; >> + break; >> + case CURRENT: >> + snprintf(sensor->input, sizeof(sensor->input), >> + "curr%d_input", num_current + 1); >> + snprintf(sensor->label, sizeof(sensor->input), >> + "curr%d_label", num_current + 1); >> + num_current++; >> + break; >> + case POWER: >> + snprintf(sensor->input, sizeof(sensor->input), >> + "power%d_input", num_power + 1); >> + snprintf(sensor->label, sizeof(sensor->input), >> + "power%d_label", num_power + 1); >> + num_power++; >> + break; >> + default: >> + break; >> + } >> + >> + sensor->dev_attr_input.attr.mode = S_IRUGO; >> + sensor->dev_attr_input.show = scpi_show_sensor; >> + sensor->dev_attr_input.attr.name = sensor->input; >> + >> + sensor->dev_attr_label.attr.mode = S_IRUGO; >> + sensor->dev_attr_label.show = scpi_show_label; >> + sensor->dev_attr_label.attr.name = sensor->label; >> + >> + scpi_sensors->attrs[i << 1] = &sensor->dev_attr_input.attr; >> + scpi_sensors->attrs[(i << 1) + 1] = &sensor->dev_attr_label.attr; >> + >> + sysfs_attr_init(scpi_sensors->attrs[i << 1]); >> + sysfs_attr_init(scpi_sensors->attrs[(i << 1) + 1]); >> + } >> + >> + scpi_sensors->group.attrs = scpi_sensors->attrs; >> + scpi_sensors->groups[0] = &scpi_sensors->group; >> + >> + hwdev = devm_hwmon_device_register_with_groups(dev, >> + "scpi_sensors", scpi_sensors, scpi_sensors->groups); >> + >> + return PTR_ERR_OR_ZERO(hwdev); >> +} >> + >> +static const struct of_device_id scpi_of_match[] = { >> + {.compatible = "arm,scpi-sensors"}, >> + {}, >> +}; >> + >> +static struct platform_driver scpi_hwmon_platdrv = { >> + .driver = { >> + .name = "scpi-hwmon", >> + .owner = THIS_MODULE, >> + .of_match_table = scpi_of_match, >> + }, >> + .probe = scpi_hwmon_probe, >> +}; >> +module_platform_driver(scpi_hwmon_platdrv); >> + >> +MODULE_AUTHOR("Punit Agrawal <punit.agrawal@xxxxxxx>"); >> +MODULE_DESCRIPTION("ARM SCPI HWMON interface driver"); >> +MODULE_LICENSE("GPL v2"); >> > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
