* [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-16 13:37 ` Stefan Wahren
0 siblings, 0 replies; 50+ messages in thread
From: Stefan Wahren @ 2018-05-16 13:37 UTC (permalink / raw)
To: linux-arm-kernel
Currently there is no easy way to detect under-voltage conditions on a remote
Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
via mailbox interface. The handling based on Noralf's modifications to the
downstream firmware driver. In case of an under-voltage condition only an entry
is written to the kernel log.
CC: "Noralf Tr?nnes" <noralf@tronnes.org>
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/hwmon/Kconfig | 10 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
3 files changed, 218 insertions(+)
create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 768aed5..7f935cf 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
This driver can also be built as a module. If so, the module
will be called pwm-fan.
+config SENSORS_RASPBERRYPI_HWMON
+ tristate "Raspberry Pi voltage monitor"
+ depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
+ help
+ If you say yes here you get support for voltage sensor on the
+ Raspberry Pi.
+
+ This driver can also be built as a module. If so, the module
+ will be called raspberrypi-hwmon.
+
config SENSORS_SHT15
tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
depends on GPIOLIB || COMPILE_TEST
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index e7d52a3..a929770 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
+obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
new file mode 100644
index 0000000..2003f6c
--- /dev/null
+++ b/drivers/hwmon/raspberrypi-hwmon.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Raspberry Pi voltage sensor driver
+ *
+ * Based on firmware/raspberrypi.c by Noralf Tr?nnes
+ *
+ * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
+#define UNDERVOLTAGE_STICKY_BIT BIT(16)
+
+struct rpi_hwmon_data {
+ struct device *hwmon_dev;
+ struct rpi_firmware *fw;
+ u32 last_throttled;
+ struct delayed_work get_values_poll_work;
+};
+
+static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
+{
+ u32 new_uv, old_uv, value;
+ int ret;
+
+ /* Clear sticky bits */
+ value = 0xffff;
+
+ ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
+ &value, sizeof(value));
+ if (ret) {
+ dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
+ __func__, ret);
+ return;
+ }
+
+ new_uv = value & UNDERVOLTAGE_STICKY_BIT;
+ old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
+ data->last_throttled = value;
+
+ if (new_uv == old_uv)
+ return;
+
+ if (new_uv)
+ dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
+ value);
+ else
+ dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
+ value);
+
+ sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
+}
+
+static void get_values_poll(struct work_struct *work)
+{
+ struct rpi_hwmon_data *data;
+
+ data = container_of(work, struct rpi_hwmon_data,
+ get_values_poll_work.work);
+
+ rpi_firmware_get_throttled(data);
+
+ /*
+ * We can't run faster than the sticky shift (100ms) since we get
+ * flipping in the sticky bits that are cleared.
+ */
+ schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
+}
+
+static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct rpi_hwmon_data *data = dev_get_drvdata(dev);
+
+ if (type != hwmon_in)
+ return -EOPNOTSUPP;
+
+ if (attr != hwmon_in_lcrit_alarm)
+ return -EOPNOTSUPP;
+
+ if (channel)
+ return -EOPNOTSUPP;
+
+ *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
+ return 0;
+}
+
+static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ if (type != hwmon_in)
+ return 0;
+
+ if (attr != hwmon_in_lcrit_alarm)
+ return 0;
+
+ if (channel)
+ return 0;
+
+ return 0444;
+}
+
+static const u32 rpi_in_config[] = {
+ HWMON_I_LCRIT_ALARM,
+ 0
+};
+
+static const struct hwmon_channel_info rpi_in = {
+ .type = hwmon_in,
+ .config = rpi_in_config,
+};
+
+static const struct hwmon_channel_info *rpi_info[] = {
+ &rpi_in,
+ NULL
+};
+
+static const struct hwmon_ops rpi_hwmon_ops = {
+ .is_visible = rpi_is_visible,
+ .read = rpi_read,
+};
+
+static const struct hwmon_chip_info rpi_chip_info = {
+ .ops = &rpi_hwmon_ops,
+ .info = rpi_info,
+};
+
+static int rpi_hwmon_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *fw_node;
+ struct rpi_hwmon_data *data;
+ int ret;
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ fw_node = of_get_parent(dev->of_node);
+ if (!fw_node) {
+ dev_err(dev, "Missing firmware node\n");
+ return -ENOENT;
+ }
+
+ data->fw = rpi_firmware_get(fw_node);
+ of_node_put(fw_node);
+ if (!data->fw)
+ return -EPROBE_DEFER;
+
+ ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
+ &data->last_throttled,
+ sizeof(data->last_throttled));
+ if (ret) {
+ dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
+ return -EOPNOTSUPP;
+ }
+
+ data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
+ data,
+ &rpi_chip_info,
+ NULL);
+
+ INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
+ platform_set_drvdata(pdev, data);
+
+ if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
+ schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
+
+ return PTR_ERR_OR_ZERO(data->hwmon_dev);
+}
+
+static int rpi_hwmon_remove(struct platform_device *pdev)
+{
+ struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
+
+ cancel_delayed_work_sync(&data->get_values_poll_work);
+
+ return 0;
+}
+
+static const struct of_device_id rpi_hwmon_of_match[] = {
+ { .compatible = "raspberrypi,bcm2835-hwmon", },
+ { /* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
+
+static struct platform_driver rpi_hwmon_driver = {
+ .probe = rpi_hwmon_probe,
+ .remove = rpi_hwmon_remove,
+ .driver = {
+ .name = "raspberrypi-hwmon",
+ .of_match_table = rpi_hwmon_of_match,
+ },
+};
+module_platform_driver(rpi_hwmon_driver);
+
+MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
+MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related [flat|nested] 50+ messages in thread* [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-16 13:37 ` Stefan Wahren
0 siblings, 0 replies; 50+ messages in thread
From: Stefan Wahren @ 2018-05-16 13:37 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Jean Delvare, Guenter Roeck, Eric Anholt
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
Ray Jui, Stefan Wahren, Phil Elwell, Noralf Trønnes,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
Currently there is no easy way to detect under-voltage conditions on a remote
Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
via mailbox interface. The handling based on Noralf's modifications to the
downstream firmware driver. In case of an under-voltage condition only an entry
is written to the kernel log.
CC: "Noralf Trønnes" <noralf@tronnes.org>
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/hwmon/Kconfig | 10 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
3 files changed, 218 insertions(+)
create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 768aed5..7f935cf 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
This driver can also be built as a module. If so, the module
will be called pwm-fan.
+config SENSORS_RASPBERRYPI_HWMON
+ tristate "Raspberry Pi voltage monitor"
+ depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
+ help
+ If you say yes here you get support for voltage sensor on the
+ Raspberry Pi.
+
+ This driver can also be built as a module. If so, the module
+ will be called raspberrypi-hwmon.
+
config SENSORS_SHT15
tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
depends on GPIOLIB || COMPILE_TEST
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index e7d52a3..a929770 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
+obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
new file mode 100644
index 0000000..2003f6c
--- /dev/null
+++ b/drivers/hwmon/raspberrypi-hwmon.c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Raspberry Pi voltage sensor driver
+ *
+ * Based on firmware/raspberrypi.c by Noralf Trønnes
+ *
+ * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/workqueue.h>
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
+#define UNDERVOLTAGE_STICKY_BIT BIT(16)
+
+struct rpi_hwmon_data {
+ struct device *hwmon_dev;
+ struct rpi_firmware *fw;
+ u32 last_throttled;
+ struct delayed_work get_values_poll_work;
+};
+
+static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
+{
+ u32 new_uv, old_uv, value;
+ int ret;
+
+ /* Clear sticky bits */
+ value = 0xffff;
+
+ ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
+ &value, sizeof(value));
+ if (ret) {
+ dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
+ __func__, ret);
+ return;
+ }
+
+ new_uv = value & UNDERVOLTAGE_STICKY_BIT;
+ old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
+ data->last_throttled = value;
+
+ if (new_uv == old_uv)
+ return;
+
+ if (new_uv)
+ dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
+ value);
+ else
+ dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
+ value);
+
+ sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
+}
+
+static void get_values_poll(struct work_struct *work)
+{
+ struct rpi_hwmon_data *data;
+
+ data = container_of(work, struct rpi_hwmon_data,
+ get_values_poll_work.work);
+
+ rpi_firmware_get_throttled(data);
+
+ /*
+ * We can't run faster than the sticky shift (100ms) since we get
+ * flipping in the sticky bits that are cleared.
+ */
+ schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
+}
+
+static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ struct rpi_hwmon_data *data = dev_get_drvdata(dev);
+
+ if (type != hwmon_in)
+ return -EOPNOTSUPP;
+
+ if (attr != hwmon_in_lcrit_alarm)
+ return -EOPNOTSUPP;
+
+ if (channel)
+ return -EOPNOTSUPP;
+
+ *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
+ return 0;
+}
+
+static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ if (type != hwmon_in)
+ return 0;
+
+ if (attr != hwmon_in_lcrit_alarm)
+ return 0;
+
+ if (channel)
+ return 0;
+
+ return 0444;
+}
+
+static const u32 rpi_in_config[] = {
+ HWMON_I_LCRIT_ALARM,
+ 0
+};
+
+static const struct hwmon_channel_info rpi_in = {
+ .type = hwmon_in,
+ .config = rpi_in_config,
+};
+
+static const struct hwmon_channel_info *rpi_info[] = {
+ &rpi_in,
+ NULL
+};
+
+static const struct hwmon_ops rpi_hwmon_ops = {
+ .is_visible = rpi_is_visible,
+ .read = rpi_read,
+};
+
+static const struct hwmon_chip_info rpi_chip_info = {
+ .ops = &rpi_hwmon_ops,
+ .info = rpi_info,
+};
+
+static int rpi_hwmon_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *fw_node;
+ struct rpi_hwmon_data *data;
+ int ret;
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ fw_node = of_get_parent(dev->of_node);
+ if (!fw_node) {
+ dev_err(dev, "Missing firmware node\n");
+ return -ENOENT;
+ }
+
+ data->fw = rpi_firmware_get(fw_node);
+ of_node_put(fw_node);
+ if (!data->fw)
+ return -EPROBE_DEFER;
+
+ ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
+ &data->last_throttled,
+ sizeof(data->last_throttled));
+ if (ret) {
+ dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
+ return -EOPNOTSUPP;
+ }
+
+ data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
+ data,
+ &rpi_chip_info,
+ NULL);
+
+ INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
+ platform_set_drvdata(pdev, data);
+
+ if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
+ schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
+
+ return PTR_ERR_OR_ZERO(data->hwmon_dev);
+}
+
+static int rpi_hwmon_remove(struct platform_device *pdev)
+{
+ struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
+
+ cancel_delayed_work_sync(&data->get_values_poll_work);
+
+ return 0;
+}
+
+static const struct of_device_id rpi_hwmon_of_match[] = {
+ { .compatible = "raspberrypi,bcm2835-hwmon", },
+ { /* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
+
+static struct platform_driver rpi_hwmon_driver = {
+ .probe = rpi_hwmon_probe,
+ .remove = rpi_hwmon_remove,
+ .driver = {
+ .name = "raspberrypi-hwmon",
+ .of_match_table = rpi_hwmon_of_match,
+ },
+};
+module_platform_driver(rpi_hwmon_driver);
+
+MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
+MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
+MODULE_LICENSE("GPL v2");
--
2.7.4
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 50+ messages in thread* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
2018-05-16 13:37 ` Stefan Wahren
(?)
@ 2018-05-16 13:51 ` Robin Murphy
-1 siblings, 0 replies; 50+ messages in thread
From: Robin Murphy @ 2018-05-16 13:51 UTC (permalink / raw)
To: Stefan Wahren, Rob Herring, Mark Rutland, Jean Delvare,
Guenter Roeck, Eric Anholt
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
Ray Jui, Phil Elwell, Noralf Trønnes,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
Hi Stefan,
On 16/05/18 14:37, Stefan Wahren wrote:
> Currently there is no easy way to detect under-voltage conditions on a remote
> Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> via mailbox interface. The handling based on Noralf's modifications to the
> downstream firmware driver. In case of an under-voltage condition only an entry
> is written to the kernel log.
>
> CC: "Noralf Trønnes" <noralf@tronnes.org>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 218 insertions(+)
> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 768aed5..7f935cf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> This driver can also be built as a module. If so, the module
> will be called pwm-fan.
>
> +config SENSORS_RASPBERRYPI_HWMON
> + tristate "Raspberry Pi voltage monitor"
> + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
Since RASPBERRYPI_FIRMWARE already implies ARCH_BCM2835 (via
BCM2835_MBOX), this is just a very roundabout way to say:
depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
Robin.
> + help
> + If you say yes here you get support for voltage sensor on the
> + Raspberry Pi.
> +
> + This driver can also be built as a module. If so, the module
> + will be called raspberrypi-hwmon.
> +
> config SENSORS_SHT15
> tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e7d52a3..a929770 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> new file mode 100644
> index 0000000..2003f6c
> --- /dev/null
> +++ b/drivers/hwmon/raspberrypi-hwmon.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Raspberry Pi voltage sensor driver
> + *
> + * Based on firmware/raspberrypi.c by Noralf Trønnes
> + *
> + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> + */
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
> +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> +
> +struct rpi_hwmon_data {
> + struct device *hwmon_dev;
> + struct rpi_firmware *fw;
> + u32 last_throttled;
> + struct delayed_work get_values_poll_work;
> +};
> +
> +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> +{
> + u32 new_uv, old_uv, value;
> + int ret;
> +
> + /* Clear sticky bits */
> + value = 0xffff;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &value, sizeof(value));
> + if (ret) {
> + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> + __func__, ret);
> + return;
> + }
> +
> + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> + data->last_throttled = value;
> +
> + if (new_uv == old_uv)
> + return;
> +
> + if (new_uv)
> + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> + value);
> + else
> + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> + value);
> +
> + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> +}
> +
> +static void get_values_poll(struct work_struct *work)
> +{
> + struct rpi_hwmon_data *data;
> +
> + data = container_of(work, struct rpi_hwmon_data,
> + get_values_poll_work.work);
> +
> + rpi_firmware_get_throttled(data);
> +
> + /*
> + * We can't run faster than the sticky shift (100ms) since we get
> + * flipping in the sticky bits that are cleared.
> + */
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +}
> +
> +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> +
> + if (type != hwmon_in)
> + return -EOPNOTSUPP;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return -EOPNOTSUPP;
> +
> + if (channel)
> + return -EOPNOTSUPP;
> +
> + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> + return 0;
> +}
> +
> +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + if (type != hwmon_in)
> + return 0;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return 0;
> +
> + if (channel)
> + return 0;
> +
> + return 0444;
> +}
> +
> +static const u32 rpi_in_config[] = {
> + HWMON_I_LCRIT_ALARM,
> + 0
> +};
> +
> +static const struct hwmon_channel_info rpi_in = {
> + .type = hwmon_in,
> + .config = rpi_in_config,
> +};
> +
> +static const struct hwmon_channel_info *rpi_info[] = {
> + &rpi_in,
> + NULL
> +};
> +
> +static const struct hwmon_ops rpi_hwmon_ops = {
> + .is_visible = rpi_is_visible,
> + .read = rpi_read,
> +};
> +
> +static const struct hwmon_chip_info rpi_chip_info = {
> + .ops = &rpi_hwmon_ops,
> + .info = rpi_info,
> +};
> +
> +static int rpi_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *fw_node;
> + struct rpi_hwmon_data *data;
> + int ret;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + fw_node = of_get_parent(dev->of_node);
> + if (!fw_node) {
> + dev_err(dev, "Missing firmware node\n");
> + return -ENOENT;
> + }
> +
> + data->fw = rpi_firmware_get(fw_node);
> + of_node_put(fw_node);
> + if (!data->fw)
> + return -EPROBE_DEFER;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &data->last_throttled,
> + sizeof(data->last_throttled));
> + if (ret) {
> + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
> + return -EOPNOTSUPP;
> + }
> +
> + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> + data,
> + &rpi_chip_info,
> + NULL);
> +
> + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> + platform_set_drvdata(pdev, data);
> +
> + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +
> + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> +}
> +
> +static int rpi_hwmon_remove(struct platform_device *pdev)
> +{
> + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> +
> + cancel_delayed_work_sync(&data->get_values_poll_work);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rpi_hwmon_of_match[] = {
> + { .compatible = "raspberrypi,bcm2835-hwmon", },
> + { /* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> +
> +static struct platform_driver rpi_hwmon_driver = {
> + .probe = rpi_hwmon_probe,
> + .remove = rpi_hwmon_remove,
> + .driver = {
> + .name = "raspberrypi-hwmon",
> + .of_match_table = rpi_hwmon_of_match,
> + },
> +};
> +module_platform_driver(rpi_hwmon_driver);
> +
> +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 50+ messages in thread* [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-16 13:51 ` Robin Murphy
0 siblings, 0 replies; 50+ messages in thread
From: Robin Murphy @ 2018-05-16 13:51 UTC (permalink / raw)
To: linux-arm-kernel
Hi Stefan,
On 16/05/18 14:37, Stefan Wahren wrote:
> Currently there is no easy way to detect under-voltage conditions on a remote
> Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> via mailbox interface. The handling based on Noralf's modifications to the
> downstream firmware driver. In case of an under-voltage condition only an entry
> is written to the kernel log.
>
> CC: "Noralf Tr?nnes" <noralf@tronnes.org>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 218 insertions(+)
> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 768aed5..7f935cf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> This driver can also be built as a module. If so, the module
> will be called pwm-fan.
>
> +config SENSORS_RASPBERRYPI_HWMON
> + tristate "Raspberry Pi voltage monitor"
> + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
Since RASPBERRYPI_FIRMWARE already implies ARCH_BCM2835 (via
BCM2835_MBOX), this is just a very roundabout way to say:
depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
Robin.
> + help
> + If you say yes here you get support for voltage sensor on the
> + Raspberry Pi.
> +
> + This driver can also be built as a module. If so, the module
> + will be called raspberrypi-hwmon.
> +
> config SENSORS_SHT15
> tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e7d52a3..a929770 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> new file mode 100644
> index 0000000..2003f6c
> --- /dev/null
> +++ b/drivers/hwmon/raspberrypi-hwmon.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Raspberry Pi voltage sensor driver
> + *
> + * Based on firmware/raspberrypi.c by Noralf Tr?nnes
> + *
> + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> + */
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
> +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> +
> +struct rpi_hwmon_data {
> + struct device *hwmon_dev;
> + struct rpi_firmware *fw;
> + u32 last_throttled;
> + struct delayed_work get_values_poll_work;
> +};
> +
> +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> +{
> + u32 new_uv, old_uv, value;
> + int ret;
> +
> + /* Clear sticky bits */
> + value = 0xffff;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &value, sizeof(value));
> + if (ret) {
> + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> + __func__, ret);
> + return;
> + }
> +
> + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> + data->last_throttled = value;
> +
> + if (new_uv == old_uv)
> + return;
> +
> + if (new_uv)
> + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> + value);
> + else
> + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> + value);
> +
> + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> +}
> +
> +static void get_values_poll(struct work_struct *work)
> +{
> + struct rpi_hwmon_data *data;
> +
> + data = container_of(work, struct rpi_hwmon_data,
> + get_values_poll_work.work);
> +
> + rpi_firmware_get_throttled(data);
> +
> + /*
> + * We can't run faster than the sticky shift (100ms) since we get
> + * flipping in the sticky bits that are cleared.
> + */
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +}
> +
> +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> +
> + if (type != hwmon_in)
> + return -EOPNOTSUPP;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return -EOPNOTSUPP;
> +
> + if (channel)
> + return -EOPNOTSUPP;
> +
> + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> + return 0;
> +}
> +
> +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + if (type != hwmon_in)
> + return 0;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return 0;
> +
> + if (channel)
> + return 0;
> +
> + return 0444;
> +}
> +
> +static const u32 rpi_in_config[] = {
> + HWMON_I_LCRIT_ALARM,
> + 0
> +};
> +
> +static const struct hwmon_channel_info rpi_in = {
> + .type = hwmon_in,
> + .config = rpi_in_config,
> +};
> +
> +static const struct hwmon_channel_info *rpi_info[] = {
> + &rpi_in,
> + NULL
> +};
> +
> +static const struct hwmon_ops rpi_hwmon_ops = {
> + .is_visible = rpi_is_visible,
> + .read = rpi_read,
> +};
> +
> +static const struct hwmon_chip_info rpi_chip_info = {
> + .ops = &rpi_hwmon_ops,
> + .info = rpi_info,
> +};
> +
> +static int rpi_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *fw_node;
> + struct rpi_hwmon_data *data;
> + int ret;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + fw_node = of_get_parent(dev->of_node);
> + if (!fw_node) {
> + dev_err(dev, "Missing firmware node\n");
> + return -ENOENT;
> + }
> +
> + data->fw = rpi_firmware_get(fw_node);
> + of_node_put(fw_node);
> + if (!data->fw)
> + return -EPROBE_DEFER;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &data->last_throttled,
> + sizeof(data->last_throttled));
> + if (ret) {
> + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
> + return -EOPNOTSUPP;
> + }
> +
> + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> + data,
> + &rpi_chip_info,
> + NULL);
> +
> + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> + platform_set_drvdata(pdev, data);
> +
> + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +
> + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> +}
> +
> +static int rpi_hwmon_remove(struct platform_device *pdev)
> +{
> + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> +
> + cancel_delayed_work_sync(&data->get_values_poll_work);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rpi_hwmon_of_match[] = {
> + { .compatible = "raspberrypi,bcm2835-hwmon", },
> + { /* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> +
> +static struct platform_driver rpi_hwmon_driver = {
> + .probe = rpi_hwmon_probe,
> + .remove = rpi_hwmon_remove,
> + .driver = {
> + .name = "raspberrypi-hwmon",
> + .of_match_table = rpi_hwmon_of_match,
> + },
> +};
> +module_platform_driver(rpi_hwmon_driver);
> +
> +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-16 13:51 ` Robin Murphy
0 siblings, 0 replies; 50+ messages in thread
From: Robin Murphy @ 2018-05-16 13:51 UTC (permalink / raw)
To: Stefan Wahren, Rob Herring, Mark Rutland, Jean Delvare,
Guenter Roeck, Eric Anholt
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
Ray Jui, Phil Elwell, Noralf Trønnes,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
Hi Stefan,
On 16/05/18 14:37, Stefan Wahren wrote:
> Currently there is no easy way to detect under-voltage conditions on a remote
> Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> via mailbox interface. The handling based on Noralf's modifications to the
> downstream firmware driver. In case of an under-voltage condition only an entry
> is written to the kernel log.
>
> CC: "Noralf Trønnes" <noralf@tronnes.org>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 218 insertions(+)
> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 768aed5..7f935cf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> This driver can also be built as a module. If so, the module
> will be called pwm-fan.
>
> +config SENSORS_RASPBERRYPI_HWMON
> + tristate "Raspberry Pi voltage monitor"
> + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
Since RASPBERRYPI_FIRMWARE already implies ARCH_BCM2835 (via
BCM2835_MBOX), this is just a very roundabout way to say:
depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
Robin.
> + help
> + If you say yes here you get support for voltage sensor on the
> + Raspberry Pi.
> +
> + This driver can also be built as a module. If so, the module
> + will be called raspberrypi-hwmon.
> +
> config SENSORS_SHT15
> tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e7d52a3..a929770 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> new file mode 100644
> index 0000000..2003f6c
> --- /dev/null
> +++ b/drivers/hwmon/raspberrypi-hwmon.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Raspberry Pi voltage sensor driver
> + *
> + * Based on firmware/raspberrypi.c by Noralf Trønnes
> + *
> + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> + */
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
> +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> +
> +struct rpi_hwmon_data {
> + struct device *hwmon_dev;
> + struct rpi_firmware *fw;
> + u32 last_throttled;
> + struct delayed_work get_values_poll_work;
> +};
> +
> +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> +{
> + u32 new_uv, old_uv, value;
> + int ret;
> +
> + /* Clear sticky bits */
> + value = 0xffff;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &value, sizeof(value));
> + if (ret) {
> + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> + __func__, ret);
> + return;
> + }
> +
> + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> + data->last_throttled = value;
> +
> + if (new_uv == old_uv)
> + return;
> +
> + if (new_uv)
> + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> + value);
> + else
> + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> + value);
> +
> + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> +}
> +
> +static void get_values_poll(struct work_struct *work)
> +{
> + struct rpi_hwmon_data *data;
> +
> + data = container_of(work, struct rpi_hwmon_data,
> + get_values_poll_work.work);
> +
> + rpi_firmware_get_throttled(data);
> +
> + /*
> + * We can't run faster than the sticky shift (100ms) since we get
> + * flipping in the sticky bits that are cleared.
> + */
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +}
> +
> +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> +
> + if (type != hwmon_in)
> + return -EOPNOTSUPP;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return -EOPNOTSUPP;
> +
> + if (channel)
> + return -EOPNOTSUPP;
> +
> + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> + return 0;
> +}
> +
> +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + if (type != hwmon_in)
> + return 0;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return 0;
> +
> + if (channel)
> + return 0;
> +
> + return 0444;
> +}
> +
> +static const u32 rpi_in_config[] = {
> + HWMON_I_LCRIT_ALARM,
> + 0
> +};
> +
> +static const struct hwmon_channel_info rpi_in = {
> + .type = hwmon_in,
> + .config = rpi_in_config,
> +};
> +
> +static const struct hwmon_channel_info *rpi_info[] = {
> + &rpi_in,
> + NULL
> +};
> +
> +static const struct hwmon_ops rpi_hwmon_ops = {
> + .is_visible = rpi_is_visible,
> + .read = rpi_read,
> +};
> +
> +static const struct hwmon_chip_info rpi_chip_info = {
> + .ops = &rpi_hwmon_ops,
> + .info = rpi_info,
> +};
> +
> +static int rpi_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *fw_node;
> + struct rpi_hwmon_data *data;
> + int ret;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + fw_node = of_get_parent(dev->of_node);
> + if (!fw_node) {
> + dev_err(dev, "Missing firmware node\n");
> + return -ENOENT;
> + }
> +
> + data->fw = rpi_firmware_get(fw_node);
> + of_node_put(fw_node);
> + if (!data->fw)
> + return -EPROBE_DEFER;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &data->last_throttled,
> + sizeof(data->last_throttled));
> + if (ret) {
> + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
> + return -EOPNOTSUPP;
> + }
> +
> + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> + data,
> + &rpi_chip_info,
> + NULL);
> +
> + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> + platform_set_drvdata(pdev, data);
> +
> + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +
> + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> +}
> +
> +static int rpi_hwmon_remove(struct platform_device *pdev)
> +{
> + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> +
> + cancel_delayed_work_sync(&data->get_values_poll_work);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rpi_hwmon_of_match[] = {
> + { .compatible = "raspberrypi,bcm2835-hwmon", },
> + { /* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> +
> +static struct platform_driver rpi_hwmon_driver = {
> + .probe = rpi_hwmon_probe,
> + .remove = rpi_hwmon_remove,
> + .driver = {
> + .name = "raspberrypi-hwmon",
> + .of_match_table = rpi_hwmon_of_match,
> + },
> +};
> +module_platform_driver(rpi_hwmon_driver);
> +
> +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> +MODULE_LICENSE("GPL v2");
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
2018-05-16 13:51 ` Robin Murphy
(?)
@ 2018-05-16 18:05 ` Guenter Roeck
-1 siblings, 0 replies; 50+ messages in thread
From: Guenter Roeck @ 2018-05-16 18:05 UTC (permalink / raw)
To: Robin Murphy
Cc: Stefan Wahren, Rob Herring, Mark Rutland, Jean Delvare,
Eric Anholt, linux-hwmon, devicetree, Florian Fainelli,
Scott Branden, Ray Jui, Phil Elwell, Noralf Trønnes,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
On Wed, May 16, 2018 at 02:51:49PM +0100, Robin Murphy wrote:
> Hi Stefan,
>
> On 16/05/18 14:37, Stefan Wahren wrote:
> >Currently there is no easy way to detect under-voltage conditions on a remote
> >Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> >via mailbox interface. The handling based on Noralf's modifications to the
> >downstream firmware driver. In case of an under-voltage condition only an entry
> >is written to the kernel log.
> >
> >CC: "Noralf Trønnes" <noralf@tronnes.org>
> >Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> >---
> > drivers/hwmon/Kconfig | 10 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 218 insertions(+)
> > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> >
> >diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >index 768aed5..7f935cf 100644
> >--- a/drivers/hwmon/Kconfig
> >+++ b/drivers/hwmon/Kconfig
> >@@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> > This driver can also be built as a module. If so, the module
> > will be called pwm-fan.
> >+config SENSORS_RASPBERRYPI_HWMON
> >+ tristate "Raspberry Pi voltage monitor"
> >+ depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
>
> Since RASPBERRYPI_FIRMWARE already implies ARCH_BCM2835 (via BCM2835_MBOX),
> this is just a very roundabout way to say:
>
> depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
>
That would permit SENSORS_RASPBERRYPI_HWMON=y combined with
RASPBERRYPI_FIRMWARE=m, which AFAICS would result in a build error
because include/soc/bcm2835/raspberrypi-firmware.h uses IS_ENABLED()
and not IS_REACHABLE().
Guenter
> Robin.
>
> >+ help
> >+ If you say yes here you get support for voltage sensor on the
> >+ Raspberry Pi.
> >+
> >+ This driver can also be built as a module. If so, the module
> >+ will be called raspberrypi-hwmon.
> >+
> > config SENSORS_SHT15
> > tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> > depends on GPIOLIB || COMPILE_TEST
> >diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> >index e7d52a3..a929770 100644
> >--- a/drivers/hwmon/Makefile
> >+++ b/drivers/hwmon/Makefile
> >@@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> >+obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> >diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> >new file mode 100644
> >index 0000000..2003f6c
> >--- /dev/null
> >+++ b/drivers/hwmon/raspberrypi-hwmon.c
> >@@ -0,0 +1,207 @@
> >+// SPDX-License-Identifier: GPL-2.0+
> >+/*
> >+ * Raspberry Pi voltage sensor driver
> >+ *
> >+ * Based on firmware/raspberrypi.c by Noralf Trønnes
> >+ *
> >+ * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> >+ */
> >+#include <linux/device.h>
> >+#include <linux/err.h>
> >+#include <linux/hwmon.h>
> >+#include <linux/hwmon-sysfs.h>
> >+#include <linux/module.h>
> >+#include <linux/of_device.h>
> >+#include <linux/platform_device.h>
> >+#include <linux/slab.h>
> >+#include <linux/workqueue.h>
> >+#include <soc/bcm2835/raspberrypi-firmware.h>
> >+
> >+#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> >+
> >+struct rpi_hwmon_data {
> >+ struct device *hwmon_dev;
> >+ struct rpi_firmware *fw;
> >+ u32 last_throttled;
> >+ struct delayed_work get_values_poll_work;
> >+};
> >+
> >+static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> >+{
> >+ u32 new_uv, old_uv, value;
> >+ int ret;
> >+
> >+ /* Clear sticky bits */
> >+ value = 0xffff;
> >+
> >+ ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> >+ &value, sizeof(value));
> >+ if (ret) {
> >+ dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> >+ __func__, ret);
> >+ return;
> >+ }
> >+
> >+ new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> >+ old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> >+ data->last_throttled = value;
> >+
> >+ if (new_uv == old_uv)
> >+ return;
> >+
> >+ if (new_uv)
> >+ dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> >+ value);
> >+ else
> >+ dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> >+ value);
> >+
> >+ sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> >+}
> >+
> >+static void get_values_poll(struct work_struct *work)
> >+{
> >+ struct rpi_hwmon_data *data;
> >+
> >+ data = container_of(work, struct rpi_hwmon_data,
> >+ get_values_poll_work.work);
> >+
> >+ rpi_firmware_get_throttled(data);
> >+
> >+ /*
> >+ * We can't run faster than the sticky shift (100ms) since we get
> >+ * flipping in the sticky bits that are cleared.
> >+ */
> >+ schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> >+}
> >+
> >+static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> >+ u32 attr, int channel, long *val)
> >+{
> >+ struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> >+
> >+ if (type != hwmon_in)
> >+ return -EOPNOTSUPP;
> >+
> >+ if (attr != hwmon_in_lcrit_alarm)
> >+ return -EOPNOTSUPP;
> >+
> >+ if (channel)
> >+ return -EOPNOTSUPP;
> >+
> >+ *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> >+ return 0;
> >+}
> >+
> >+static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> >+ u32 attr, int channel)
> >+{
> >+ if (type != hwmon_in)
> >+ return 0;
> >+
> >+ if (attr != hwmon_in_lcrit_alarm)
> >+ return 0;
> >+
> >+ if (channel)
> >+ return 0;
> >+
> >+ return 0444;
> >+}
> >+
> >+static const u32 rpi_in_config[] = {
> >+ HWMON_I_LCRIT_ALARM,
> >+ 0
> >+};
> >+
> >+static const struct hwmon_channel_info rpi_in = {
> >+ .type = hwmon_in,
> >+ .config = rpi_in_config,
> >+};
> >+
> >+static const struct hwmon_channel_info *rpi_info[] = {
> >+ &rpi_in,
> >+ NULL
> >+};
> >+
> >+static const struct hwmon_ops rpi_hwmon_ops = {
> >+ .is_visible = rpi_is_visible,
> >+ .read = rpi_read,
> >+};
> >+
> >+static const struct hwmon_chip_info rpi_chip_info = {
> >+ .ops = &rpi_hwmon_ops,
> >+ .info = rpi_info,
> >+};
> >+
> >+static int rpi_hwmon_probe(struct platform_device *pdev)
> >+{
> >+ struct device *dev = &pdev->dev;
> >+ struct device_node *fw_node;
> >+ struct rpi_hwmon_data *data;
> >+ int ret;
> >+
> >+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >+ if (!data)
> >+ return -ENOMEM;
> >+
> >+ fw_node = of_get_parent(dev->of_node);
> >+ if (!fw_node) {
> >+ dev_err(dev, "Missing firmware node\n");
> >+ return -ENOENT;
> >+ }
> >+
> >+ data->fw = rpi_firmware_get(fw_node);
> >+ of_node_put(fw_node);
> >+ if (!data->fw)
> >+ return -EPROBE_DEFER;
> >+
> >+ ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> >+ &data->last_throttled,
> >+ sizeof(data->last_throttled));
> >+ if (ret) {
> >+ dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
> >+ return -EOPNOTSUPP;
> >+ }
> >+
> >+ data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> >+ data,
> >+ &rpi_chip_info,
> >+ NULL);
> >+
> >+ INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> >+ platform_set_drvdata(pdev, data);
> >+
> >+ if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> >+ schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> >+
> >+ return PTR_ERR_OR_ZERO(data->hwmon_dev);
> >+}
> >+
> >+static int rpi_hwmon_remove(struct platform_device *pdev)
> >+{
> >+ struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> >+
> >+ cancel_delayed_work_sync(&data->get_values_poll_work);
> >+
> >+ return 0;
> >+}
> >+
> >+static const struct of_device_id rpi_hwmon_of_match[] = {
> >+ { .compatible = "raspberrypi,bcm2835-hwmon", },
> >+ { /* sentinel */},
> >+};
> >+MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> >+
> >+static struct platform_driver rpi_hwmon_driver = {
> >+ .probe = rpi_hwmon_probe,
> >+ .remove = rpi_hwmon_remove,
> >+ .driver = {
> >+ .name = "raspberrypi-hwmon",
> >+ .of_match_table = rpi_hwmon_of_match,
> >+ },
> >+};
> >+module_platform_driver(rpi_hwmon_driver);
> >+
> >+MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> >+MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> >+MODULE_LICENSE("GPL v2");
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 50+ messages in thread* [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-16 18:05 ` Guenter Roeck
0 siblings, 0 replies; 50+ messages in thread
From: Guenter Roeck @ 2018-05-16 18:05 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, May 16, 2018 at 02:51:49PM +0100, Robin Murphy wrote:
> Hi Stefan,
>
> On 16/05/18 14:37, Stefan Wahren wrote:
> >Currently there is no easy way to detect under-voltage conditions on a remote
> >Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> >via mailbox interface. The handling based on Noralf's modifications to the
> >downstream firmware driver. In case of an under-voltage condition only an entry
> >is written to the kernel log.
> >
> >CC: "Noralf Tr?nnes" <noralf@tronnes.org>
> >Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> >---
> > drivers/hwmon/Kconfig | 10 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 218 insertions(+)
> > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> >
> >diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >index 768aed5..7f935cf 100644
> >--- a/drivers/hwmon/Kconfig
> >+++ b/drivers/hwmon/Kconfig
> >@@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> > This driver can also be built as a module. If so, the module
> > will be called pwm-fan.
> >+config SENSORS_RASPBERRYPI_HWMON
> >+ tristate "Raspberry Pi voltage monitor"
> >+ depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
>
> Since RASPBERRYPI_FIRMWARE already implies ARCH_BCM2835 (via BCM2835_MBOX),
> this is just a very roundabout way to say:
>
> depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
>
That would permit SENSORS_RASPBERRYPI_HWMON=y combined with
RASPBERRYPI_FIRMWARE=m, which AFAICS would result in a build error
because include/soc/bcm2835/raspberrypi-firmware.h uses IS_ENABLED()
and not IS_REACHABLE().
Guenter
> Robin.
>
> >+ help
> >+ If you say yes here you get support for voltage sensor on the
> >+ Raspberry Pi.
> >+
> >+ This driver can also be built as a module. If so, the module
> >+ will be called raspberrypi-hwmon.
> >+
> > config SENSORS_SHT15
> > tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> > depends on GPIOLIB || COMPILE_TEST
> >diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> >index e7d52a3..a929770 100644
> >--- a/drivers/hwmon/Makefile
> >+++ b/drivers/hwmon/Makefile
> >@@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> >+obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> >diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> >new file mode 100644
> >index 0000000..2003f6c
> >--- /dev/null
> >+++ b/drivers/hwmon/raspberrypi-hwmon.c
> >@@ -0,0 +1,207 @@
> >+// SPDX-License-Identifier: GPL-2.0+
> >+/*
> >+ * Raspberry Pi voltage sensor driver
> >+ *
> >+ * Based on firmware/raspberrypi.c by Noralf Tr?nnes
> >+ *
> >+ * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> >+ */
> >+#include <linux/device.h>
> >+#include <linux/err.h>
> >+#include <linux/hwmon.h>
> >+#include <linux/hwmon-sysfs.h>
> >+#include <linux/module.h>
> >+#include <linux/of_device.h>
> >+#include <linux/platform_device.h>
> >+#include <linux/slab.h>
> >+#include <linux/workqueue.h>
> >+#include <soc/bcm2835/raspberrypi-firmware.h>
> >+
> >+#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> >+
> >+struct rpi_hwmon_data {
> >+ struct device *hwmon_dev;
> >+ struct rpi_firmware *fw;
> >+ u32 last_throttled;
> >+ struct delayed_work get_values_poll_work;
> >+};
> >+
> >+static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> >+{
> >+ u32 new_uv, old_uv, value;
> >+ int ret;
> >+
> >+ /* Clear sticky bits */
> >+ value = 0xffff;
> >+
> >+ ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> >+ &value, sizeof(value));
> >+ if (ret) {
> >+ dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> >+ __func__, ret);
> >+ return;
> >+ }
> >+
> >+ new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> >+ old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> >+ data->last_throttled = value;
> >+
> >+ if (new_uv == old_uv)
> >+ return;
> >+
> >+ if (new_uv)
> >+ dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> >+ value);
> >+ else
> >+ dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> >+ value);
> >+
> >+ sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> >+}
> >+
> >+static void get_values_poll(struct work_struct *work)
> >+{
> >+ struct rpi_hwmon_data *data;
> >+
> >+ data = container_of(work, struct rpi_hwmon_data,
> >+ get_values_poll_work.work);
> >+
> >+ rpi_firmware_get_throttled(data);
> >+
> >+ /*
> >+ * We can't run faster than the sticky shift (100ms) since we get
> >+ * flipping in the sticky bits that are cleared.
> >+ */
> >+ schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> >+}
> >+
> >+static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> >+ u32 attr, int channel, long *val)
> >+{
> >+ struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> >+
> >+ if (type != hwmon_in)
> >+ return -EOPNOTSUPP;
> >+
> >+ if (attr != hwmon_in_lcrit_alarm)
> >+ return -EOPNOTSUPP;
> >+
> >+ if (channel)
> >+ return -EOPNOTSUPP;
> >+
> >+ *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> >+ return 0;
> >+}
> >+
> >+static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> >+ u32 attr, int channel)
> >+{
> >+ if (type != hwmon_in)
> >+ return 0;
> >+
> >+ if (attr != hwmon_in_lcrit_alarm)
> >+ return 0;
> >+
> >+ if (channel)
> >+ return 0;
> >+
> >+ return 0444;
> >+}
> >+
> >+static const u32 rpi_in_config[] = {
> >+ HWMON_I_LCRIT_ALARM,
> >+ 0
> >+};
> >+
> >+static const struct hwmon_channel_info rpi_in = {
> >+ .type = hwmon_in,
> >+ .config = rpi_in_config,
> >+};
> >+
> >+static const struct hwmon_channel_info *rpi_info[] = {
> >+ &rpi_in,
> >+ NULL
> >+};
> >+
> >+static const struct hwmon_ops rpi_hwmon_ops = {
> >+ .is_visible = rpi_is_visible,
> >+ .read = rpi_read,
> >+};
> >+
> >+static const struct hwmon_chip_info rpi_chip_info = {
> >+ .ops = &rpi_hwmon_ops,
> >+ .info = rpi_info,
> >+};
> >+
> >+static int rpi_hwmon_probe(struct platform_device *pdev)
> >+{
> >+ struct device *dev = &pdev->dev;
> >+ struct device_node *fw_node;
> >+ struct rpi_hwmon_data *data;
> >+ int ret;
> >+
> >+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >+ if (!data)
> >+ return -ENOMEM;
> >+
> >+ fw_node = of_get_parent(dev->of_node);
> >+ if (!fw_node) {
> >+ dev_err(dev, "Missing firmware node\n");
> >+ return -ENOENT;
> >+ }
> >+
> >+ data->fw = rpi_firmware_get(fw_node);
> >+ of_node_put(fw_node);
> >+ if (!data->fw)
> >+ return -EPROBE_DEFER;
> >+
> >+ ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> >+ &data->last_throttled,
> >+ sizeof(data->last_throttled));
> >+ if (ret) {
> >+ dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
> >+ return -EOPNOTSUPP;
> >+ }
> >+
> >+ data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> >+ data,
> >+ &rpi_chip_info,
> >+ NULL);
> >+
> >+ INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> >+ platform_set_drvdata(pdev, data);
> >+
> >+ if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> >+ schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> >+
> >+ return PTR_ERR_OR_ZERO(data->hwmon_dev);
> >+}
> >+
> >+static int rpi_hwmon_remove(struct platform_device *pdev)
> >+{
> >+ struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> >+
> >+ cancel_delayed_work_sync(&data->get_values_poll_work);
> >+
> >+ return 0;
> >+}
> >+
> >+static const struct of_device_id rpi_hwmon_of_match[] = {
> >+ { .compatible = "raspberrypi,bcm2835-hwmon", },
> >+ { /* sentinel */},
> >+};
> >+MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> >+
> >+static struct platform_driver rpi_hwmon_driver = {
> >+ .probe = rpi_hwmon_probe,
> >+ .remove = rpi_hwmon_remove,
> >+ .driver = {
> >+ .name = "raspberrypi-hwmon",
> >+ .of_match_table = rpi_hwmon_of_match,
> >+ },
> >+};
> >+module_platform_driver(rpi_hwmon_driver);
> >+
> >+MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> >+MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> >+MODULE_LICENSE("GPL v2");
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-16 18:05 ` Guenter Roeck
0 siblings, 0 replies; 50+ messages in thread
From: Guenter Roeck @ 2018-05-16 18:05 UTC (permalink / raw)
To: Robin Murphy
Cc: Stefan Wahren, Mark Rutland, Jean Delvare, Scott Branden,
devicetree, Ray Jui, Phil Elwell, Eric Anholt, Rob Herring,
bcm-kernel-feedback-list, linux-rpi-kernel, Florian Fainelli,
linux-hwmon, linux-arm-kernel, Noralf Trønnes
On Wed, May 16, 2018 at 02:51:49PM +0100, Robin Murphy wrote:
> Hi Stefan,
>
> On 16/05/18 14:37, Stefan Wahren wrote:
> >Currently there is no easy way to detect under-voltage conditions on a remote
> >Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> >via mailbox interface. The handling based on Noralf's modifications to the
> >downstream firmware driver. In case of an under-voltage condition only an entry
> >is written to the kernel log.
> >
> >CC: "Noralf Trønnes" <noralf@tronnes.org>
> >Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> >---
> > drivers/hwmon/Kconfig | 10 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 218 insertions(+)
> > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> >
> >diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >index 768aed5..7f935cf 100644
> >--- a/drivers/hwmon/Kconfig
> >+++ b/drivers/hwmon/Kconfig
> >@@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> > This driver can also be built as a module. If so, the module
> > will be called pwm-fan.
> >+config SENSORS_RASPBERRYPI_HWMON
> >+ tristate "Raspberry Pi voltage monitor"
> >+ depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
>
> Since RASPBERRYPI_FIRMWARE already implies ARCH_BCM2835 (via BCM2835_MBOX),
> this is just a very roundabout way to say:
>
> depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
>
That would permit SENSORS_RASPBERRYPI_HWMON=y combined with
RASPBERRYPI_FIRMWARE=m, which AFAICS would result in a build error
because include/soc/bcm2835/raspberrypi-firmware.h uses IS_ENABLED()
and not IS_REACHABLE().
Guenter
> Robin.
>
> >+ help
> >+ If you say yes here you get support for voltage sensor on the
> >+ Raspberry Pi.
> >+
> >+ This driver can also be built as a module. If so, the module
> >+ will be called raspberrypi-hwmon.
> >+
> > config SENSORS_SHT15
> > tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> > depends on GPIOLIB || COMPILE_TEST
> >diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> >index e7d52a3..a929770 100644
> >--- a/drivers/hwmon/Makefile
> >+++ b/drivers/hwmon/Makefile
> >@@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> >+obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> >diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> >new file mode 100644
> >index 0000000..2003f6c
> >--- /dev/null
> >+++ b/drivers/hwmon/raspberrypi-hwmon.c
> >@@ -0,0 +1,207 @@
> >+// SPDX-License-Identifier: GPL-2.0+
> >+/*
> >+ * Raspberry Pi voltage sensor driver
> >+ *
> >+ * Based on firmware/raspberrypi.c by Noralf Trønnes
> >+ *
> >+ * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> >+ */
> >+#include <linux/device.h>
> >+#include <linux/err.h>
> >+#include <linux/hwmon.h>
> >+#include <linux/hwmon-sysfs.h>
> >+#include <linux/module.h>
> >+#include <linux/of_device.h>
> >+#include <linux/platform_device.h>
> >+#include <linux/slab.h>
> >+#include <linux/workqueue.h>
> >+#include <soc/bcm2835/raspberrypi-firmware.h>
> >+
> >+#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> >+
> >+struct rpi_hwmon_data {
> >+ struct device *hwmon_dev;
> >+ struct rpi_firmware *fw;
> >+ u32 last_throttled;
> >+ struct delayed_work get_values_poll_work;
> >+};
> >+
> >+static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> >+{
> >+ u32 new_uv, old_uv, value;
> >+ int ret;
> >+
> >+ /* Clear sticky bits */
> >+ value = 0xffff;
> >+
> >+ ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> >+ &value, sizeof(value));
> >+ if (ret) {
> >+ dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> >+ __func__, ret);
> >+ return;
> >+ }
> >+
> >+ new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> >+ old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> >+ data->last_throttled = value;
> >+
> >+ if (new_uv == old_uv)
> >+ return;
> >+
> >+ if (new_uv)
> >+ dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> >+ value);
> >+ else
> >+ dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> >+ value);
> >+
> >+ sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> >+}
> >+
> >+static void get_values_poll(struct work_struct *work)
> >+{
> >+ struct rpi_hwmon_data *data;
> >+
> >+ data = container_of(work, struct rpi_hwmon_data,
> >+ get_values_poll_work.work);
> >+
> >+ rpi_firmware_get_throttled(data);
> >+
> >+ /*
> >+ * We can't run faster than the sticky shift (100ms) since we get
> >+ * flipping in the sticky bits that are cleared.
> >+ */
> >+ schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> >+}
> >+
> >+static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> >+ u32 attr, int channel, long *val)
> >+{
> >+ struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> >+
> >+ if (type != hwmon_in)
> >+ return -EOPNOTSUPP;
> >+
> >+ if (attr != hwmon_in_lcrit_alarm)
> >+ return -EOPNOTSUPP;
> >+
> >+ if (channel)
> >+ return -EOPNOTSUPP;
> >+
> >+ *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> >+ return 0;
> >+}
> >+
> >+static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> >+ u32 attr, int channel)
> >+{
> >+ if (type != hwmon_in)
> >+ return 0;
> >+
> >+ if (attr != hwmon_in_lcrit_alarm)
> >+ return 0;
> >+
> >+ if (channel)
> >+ return 0;
> >+
> >+ return 0444;
> >+}
> >+
> >+static const u32 rpi_in_config[] = {
> >+ HWMON_I_LCRIT_ALARM,
> >+ 0
> >+};
> >+
> >+static const struct hwmon_channel_info rpi_in = {
> >+ .type = hwmon_in,
> >+ .config = rpi_in_config,
> >+};
> >+
> >+static const struct hwmon_channel_info *rpi_info[] = {
> >+ &rpi_in,
> >+ NULL
> >+};
> >+
> >+static const struct hwmon_ops rpi_hwmon_ops = {
> >+ .is_visible = rpi_is_visible,
> >+ .read = rpi_read,
> >+};
> >+
> >+static const struct hwmon_chip_info rpi_chip_info = {
> >+ .ops = &rpi_hwmon_ops,
> >+ .info = rpi_info,
> >+};
> >+
> >+static int rpi_hwmon_probe(struct platform_device *pdev)
> >+{
> >+ struct device *dev = &pdev->dev;
> >+ struct device_node *fw_node;
> >+ struct rpi_hwmon_data *data;
> >+ int ret;
> >+
> >+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >+ if (!data)
> >+ return -ENOMEM;
> >+
> >+ fw_node = of_get_parent(dev->of_node);
> >+ if (!fw_node) {
> >+ dev_err(dev, "Missing firmware node\n");
> >+ return -ENOENT;
> >+ }
> >+
> >+ data->fw = rpi_firmware_get(fw_node);
> >+ of_node_put(fw_node);
> >+ if (!data->fw)
> >+ return -EPROBE_DEFER;
> >+
> >+ ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> >+ &data->last_throttled,
> >+ sizeof(data->last_throttled));
> >+ if (ret) {
> >+ dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
> >+ return -EOPNOTSUPP;
> >+ }
> >+
> >+ data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> >+ data,
> >+ &rpi_chip_info,
> >+ NULL);
> >+
> >+ INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> >+ platform_set_drvdata(pdev, data);
> >+
> >+ if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> >+ schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> >+
> >+ return PTR_ERR_OR_ZERO(data->hwmon_dev);
> >+}
> >+
> >+static int rpi_hwmon_remove(struct platform_device *pdev)
> >+{
> >+ struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> >+
> >+ cancel_delayed_work_sync(&data->get_values_poll_work);
> >+
> >+ return 0;
> >+}
> >+
> >+static const struct of_device_id rpi_hwmon_of_match[] = {
> >+ { .compatible = "raspberrypi,bcm2835-hwmon", },
> >+ { /* sentinel */},
> >+};
> >+MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> >+
> >+static struct platform_driver rpi_hwmon_driver = {
> >+ .probe = rpi_hwmon_probe,
> >+ .remove = rpi_hwmon_remove,
> >+ .driver = {
> >+ .name = "raspberrypi-hwmon",
> >+ .of_match_table = rpi_hwmon_of_match,
> >+ },
> >+};
> >+module_platform_driver(rpi_hwmon_driver);
> >+
> >+MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> >+MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> >+MODULE_LICENSE("GPL v2");
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
2018-05-16 18:05 ` Guenter Roeck
(?)
@ 2018-05-16 18:23 ` Robin Murphy
-1 siblings, 0 replies; 50+ messages in thread
From: Robin Murphy @ 2018-05-16 18:23 UTC (permalink / raw)
To: Guenter Roeck
Cc: Stefan Wahren, Rob Herring, Mark Rutland, Jean Delvare,
Eric Anholt, linux-hwmon, devicetree, Florian Fainelli,
Scott Branden, Ray Jui, Phil Elwell, Noralf Trønnes,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
On 16/05/18 19:05, Guenter Roeck wrote:
> On Wed, May 16, 2018 at 02:51:49PM +0100, Robin Murphy wrote:
>> Hi Stefan,
>>
>> On 16/05/18 14:37, Stefan Wahren wrote:
>>> Currently there is no easy way to detect under-voltage conditions on a remote
>>> Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
>>> via mailbox interface. The handling based on Noralf's modifications to the
>>> downstream firmware driver. In case of an under-voltage condition only an entry
>>> is written to the kernel log.
>>>
>>> CC: "Noralf Trønnes" <noralf@tronnes.org>
>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> ---
>>> drivers/hwmon/Kconfig | 10 ++
>>> drivers/hwmon/Makefile | 1 +
>>> drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 218 insertions(+)
>>> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 768aed5..7f935cf 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
>>> This driver can also be built as a module. If so, the module
>>> will be called pwm-fan.
>>> +config SENSORS_RASPBERRYPI_HWMON
>>> + tristate "Raspberry Pi voltage monitor"
>>> + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
>>
>> Since RASPBERRYPI_FIRMWARE already implies ARCH_BCM2835 (via BCM2835_MBOX),
>> this is just a very roundabout way to say:
>>
>> depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
>>
> That would permit SENSORS_RASPBERRYPI_HWMON=y combined with
> RASPBERRYPI_FIRMWARE=m, which AFAICS would result in a build error
> because include/soc/bcm2835/raspberrypi-firmware.h uses IS_ENABLED()
> and not IS_REACHABLE().
But that's only possible when COMPILE_TEST=y, where in any case there
are stub definitions for the #else case in that header which should
still be enough to build with, right? (and if not, that's probably its
own bug)
Nobody's expecting COMPILE_TEST configs to actually boot and work
perfectly, are they?
Robin.
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-16 18:23 ` Robin Murphy
0 siblings, 0 replies; 50+ messages in thread
From: Robin Murphy @ 2018-05-16 18:23 UTC (permalink / raw)
To: linux-arm-kernel
On 16/05/18 19:05, Guenter Roeck wrote:
> On Wed, May 16, 2018 at 02:51:49PM +0100, Robin Murphy wrote:
>> Hi Stefan,
>>
>> On 16/05/18 14:37, Stefan Wahren wrote:
>>> Currently there is no easy way to detect under-voltage conditions on a remote
>>> Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
>>> via mailbox interface. The handling based on Noralf's modifications to the
>>> downstream firmware driver. In case of an under-voltage condition only an entry
>>> is written to the kernel log.
>>>
>>> CC: "Noralf Tr?nnes" <noralf@tronnes.org>
>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> ---
>>> drivers/hwmon/Kconfig | 10 ++
>>> drivers/hwmon/Makefile | 1 +
>>> drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 218 insertions(+)
>>> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 768aed5..7f935cf 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
>>> This driver can also be built as a module. If so, the module
>>> will be called pwm-fan.
>>> +config SENSORS_RASPBERRYPI_HWMON
>>> + tristate "Raspberry Pi voltage monitor"
>>> + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
>>
>> Since RASPBERRYPI_FIRMWARE already implies ARCH_BCM2835 (via BCM2835_MBOX),
>> this is just a very roundabout way to say:
>>
>> depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
>>
> That would permit SENSORS_RASPBERRYPI_HWMON=y combined with
> RASPBERRYPI_FIRMWARE=m, which AFAICS would result in a build error
> because include/soc/bcm2835/raspberrypi-firmware.h uses IS_ENABLED()
> and not IS_REACHABLE().
But that's only possible when COMPILE_TEST=y, where in any case there
are stub definitions for the #else case in that header which should
still be enough to build with, right? (and if not, that's probably its
own bug)
Nobody's expecting COMPILE_TEST configs to actually boot and work
perfectly, are they?
Robin.
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-16 18:23 ` Robin Murphy
0 siblings, 0 replies; 50+ messages in thread
From: Robin Murphy @ 2018-05-16 18:23 UTC (permalink / raw)
To: Guenter Roeck
Cc: Stefan Wahren, Mark Rutland, Jean Delvare, Scott Branden,
devicetree, Ray Jui, Phil Elwell, Eric Anholt, Rob Herring,
bcm-kernel-feedback-list, linux-rpi-kernel, Florian Fainelli,
linux-hwmon, linux-arm-kernel, Noralf Trønnes
On 16/05/18 19:05, Guenter Roeck wrote:
> On Wed, May 16, 2018 at 02:51:49PM +0100, Robin Murphy wrote:
>> Hi Stefan,
>>
>> On 16/05/18 14:37, Stefan Wahren wrote:
>>> Currently there is no easy way to detect under-voltage conditions on a remote
>>> Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
>>> via mailbox interface. The handling based on Noralf's modifications to the
>>> downstream firmware driver. In case of an under-voltage condition only an entry
>>> is written to the kernel log.
>>>
>>> CC: "Noralf Trønnes" <noralf@tronnes.org>
>>> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
>>> ---
>>> drivers/hwmon/Kconfig | 10 ++
>>> drivers/hwmon/Makefile | 1 +
>>> drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 218 insertions(+)
>>> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 768aed5..7f935cf 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
>>> This driver can also be built as a module. If so, the module
>>> will be called pwm-fan.
>>> +config SENSORS_RASPBERRYPI_HWMON
>>> + tristate "Raspberry Pi voltage monitor"
>>> + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
>>
>> Since RASPBERRYPI_FIRMWARE already implies ARCH_BCM2835 (via BCM2835_MBOX),
>> this is just a very roundabout way to say:
>>
>> depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST
>>
> That would permit SENSORS_RASPBERRYPI_HWMON=y combined with
> RASPBERRYPI_FIRMWARE=m, which AFAICS would result in a build error
> because include/soc/bcm2835/raspberrypi-firmware.h uses IS_ENABLED()
> and not IS_REACHABLE().
But that's only possible when COMPILE_TEST=y, where in any case there
are stub definitions for the #else case in that header which should
still be enough to build with, right? (and if not, that's probably its
own bug)
Nobody's expecting COMPILE_TEST configs to actually boot and work
perfectly, are they?
Robin.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
2018-05-16 13:37 ` Stefan Wahren
(?)
@ 2018-05-16 18:21 ` Guenter Roeck
-1 siblings, 0 replies; 50+ messages in thread
From: Guenter Roeck @ 2018-05-16 18:21 UTC (permalink / raw)
To: Stefan Wahren
Cc: Rob Herring, Mark Rutland, Jean Delvare, Eric Anholt,
Florian Fainelli, Ray Jui, Scott Branden, Phil Elwell,
bcm-kernel-feedback-list, devicetree, linux-arm-kernel,
linux-rpi-kernel, linux-hwmon, Noralf Trønnes
On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
> Currently there is no easy way to detect under-voltage conditions on a remote
> Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> via mailbox interface. The handling based on Noralf's modifications to the
> downstream firmware driver. In case of an under-voltage condition only an entry
> is written to the kernel log.
>
My major concern is how this is displayed with the 'sensors' command.
Can you test and report ?
Of course, it would be much better if the firmware would also report
the actual voltage, but I guess we can't have everything.
More comments inline.
Thanks,
Guenter
> CC: "Noralf Trønnes" <noralf@tronnes.org>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
Please also provide Documentation/hwmon/raspberrypi-hwmon.
> 3 files changed, 218 insertions(+)
> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 768aed5..7f935cf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> This driver can also be built as a module. If so, the module
> will be called pwm-fan.
>
> +config SENSORS_RASPBERRYPI_HWMON
> + tristate "Raspberry Pi voltage monitor"
> + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> + help
> + If you say yes here you get support for voltage sensor on the
> + Raspberry Pi.
> +
> + This driver can also be built as a module. If so, the module
> + will be called raspberrypi-hwmon.
> +
> config SENSORS_SHT15
> tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e7d52a3..a929770 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> new file mode 100644
> index 0000000..2003f6c
> --- /dev/null
> +++ b/drivers/hwmon/raspberrypi-hwmon.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Raspberry Pi voltage sensor driver
> + *
> + * Based on firmware/raspberrypi.c by Noralf Trønnes
> + *
> + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> + */
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
Unnecessary include
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
> +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> +
> +struct rpi_hwmon_data {
> + struct device *hwmon_dev;
> + struct rpi_firmware *fw;
> + u32 last_throttled;
> + struct delayed_work get_values_poll_work;
> +};
> +
> +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> +{
> + u32 new_uv, old_uv, value;
> + int ret;
> +
> + /* Clear sticky bits */
Please make more explicit that this is a request/command sent to the firmware.
> + value = 0xffff;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &value, sizeof(value));
> + if (ret) {
> + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> + __func__, ret);
The function name seems unnecessary.
> + return;
> + }
> +
> + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> + data->last_throttled = value;
> +
> + if (new_uv == old_uv)
> + return;
> +
> + if (new_uv)
> + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> + value);
> + else
> + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> + value);
What value do those hex values provide to the user ?
> +
> + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> +}
> +
> +static void get_values_poll(struct work_struct *work)
> +{
> + struct rpi_hwmon_data *data;
> +
> + data = container_of(work, struct rpi_hwmon_data,
> + get_values_poll_work.work);
> +
> + rpi_firmware_get_throttled(data);
> +
> + /*
> + * We can't run faster than the sticky shift (100ms) since we get
> + * flipping in the sticky bits that are cleared.
> + */
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +}
> +
> +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> +
> + if (type != hwmon_in)
> + return -EOPNOTSUPP;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return -EOPNOTSUPP;
> +
> + if (channel)
> + return -EOPNOTSUPP;
> +
There is only one channel, one attribute, and one type supported.
As such, the checks are unnecessary.
> + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> + return 0;
> +}
> +
> +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + if (type != hwmon_in)
> + return 0;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return 0;
> +
> + if (channel)
> + return 0;
Same as above. In the list below, there is not a single conditional attribute.
Given that, the checks should be unnecessary, and it should be sufficient to
just return 0444.
> +
> + return 0444;
> +}
> +
> +static const u32 rpi_in_config[] = {
> + HWMON_I_LCRIT_ALARM,
> + 0
> +};
> +
> +static const struct hwmon_channel_info rpi_in = {
> + .type = hwmon_in,
> + .config = rpi_in_config,
> +};
> +
> +static const struct hwmon_channel_info *rpi_info[] = {
> + &rpi_in,
> + NULL
> +};
> +
> +static const struct hwmon_ops rpi_hwmon_ops = {
> + .is_visible = rpi_is_visible,
> + .read = rpi_read,
> +};
> +
> +static const struct hwmon_chip_info rpi_chip_info = {
> + .ops = &rpi_hwmon_ops,
> + .info = rpi_info,
> +};
> +
> +static int rpi_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *fw_node;
> + struct rpi_hwmon_data *data;
> + int ret;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + fw_node = of_get_parent(dev->of_node);
> + if (!fw_node) {
> + dev_err(dev, "Missing firmware node\n");
> + return -ENOENT;
> + }
> +
> + data->fw = rpi_firmware_get(fw_node);
> + of_node_put(fw_node);
> + if (!data->fw)
> + return -EPROBE_DEFER;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &data->last_throttled,
> + sizeof(data->last_throttled));
> + if (ret) {
> + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
If this is an error -> dev_err().
> + return -EOPNOTSUPP;
or return -ENODEV.
> + }
> +
> + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> + data,
> + &rpi_chip_info,
> + NULL);
> +
> + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> + platform_set_drvdata(pdev, data);
> +
> + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +
> + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> +}
> +
> +static int rpi_hwmon_remove(struct platform_device *pdev)
> +{
> + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> +
> + cancel_delayed_work_sync(&data->get_values_poll_work);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rpi_hwmon_of_match[] = {
> + { .compatible = "raspberrypi,bcm2835-hwmon", },
> + { /* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> +
> +static struct platform_driver rpi_hwmon_driver = {
> + .probe = rpi_hwmon_probe,
> + .remove = rpi_hwmon_remove,
> + .driver = {
> + .name = "raspberrypi-hwmon",
> + .of_match_table = rpi_hwmon_of_match,
> + },
> +};
> +module_platform_driver(rpi_hwmon_driver);
> +
> +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 50+ messages in thread* [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-16 18:21 ` Guenter Roeck
0 siblings, 0 replies; 50+ messages in thread
From: Guenter Roeck @ 2018-05-16 18:21 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
> Currently there is no easy way to detect under-voltage conditions on a remote
> Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> via mailbox interface. The handling based on Noralf's modifications to the
> downstream firmware driver. In case of an under-voltage condition only an entry
> is written to the kernel log.
>
My major concern is how this is displayed with the 'sensors' command.
Can you test and report ?
Of course, it would be much better if the firmware would also report
the actual voltage, but I guess we can't have everything.
More comments inline.
Thanks,
Guenter
> CC: "Noralf Tr?nnes" <noralf@tronnes.org>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
Please also provide Documentation/hwmon/raspberrypi-hwmon.
> 3 files changed, 218 insertions(+)
> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 768aed5..7f935cf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> This driver can also be built as a module. If so, the module
> will be called pwm-fan.
>
> +config SENSORS_RASPBERRYPI_HWMON
> + tristate "Raspberry Pi voltage monitor"
> + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> + help
> + If you say yes here you get support for voltage sensor on the
> + Raspberry Pi.
> +
> + This driver can also be built as a module. If so, the module
> + will be called raspberrypi-hwmon.
> +
> config SENSORS_SHT15
> tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e7d52a3..a929770 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> new file mode 100644
> index 0000000..2003f6c
> --- /dev/null
> +++ b/drivers/hwmon/raspberrypi-hwmon.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Raspberry Pi voltage sensor driver
> + *
> + * Based on firmware/raspberrypi.c by Noralf Tr?nnes
> + *
> + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> + */
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
Unnecessary include
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
> +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> +
> +struct rpi_hwmon_data {
> + struct device *hwmon_dev;
> + struct rpi_firmware *fw;
> + u32 last_throttled;
> + struct delayed_work get_values_poll_work;
> +};
> +
> +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> +{
> + u32 new_uv, old_uv, value;
> + int ret;
> +
> + /* Clear sticky bits */
Please make more explicit that this is a request/command sent to the firmware.
> + value = 0xffff;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &value, sizeof(value));
> + if (ret) {
> + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> + __func__, ret);
The function name seems unnecessary.
> + return;
> + }
> +
> + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> + data->last_throttled = value;
> +
> + if (new_uv == old_uv)
> + return;
> +
> + if (new_uv)
> + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> + value);
> + else
> + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> + value);
What value do those hex values provide to the user ?
> +
> + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> +}
> +
> +static void get_values_poll(struct work_struct *work)
> +{
> + struct rpi_hwmon_data *data;
> +
> + data = container_of(work, struct rpi_hwmon_data,
> + get_values_poll_work.work);
> +
> + rpi_firmware_get_throttled(data);
> +
> + /*
> + * We can't run faster than the sticky shift (100ms) since we get
> + * flipping in the sticky bits that are cleared.
> + */
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +}
> +
> +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> +
> + if (type != hwmon_in)
> + return -EOPNOTSUPP;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return -EOPNOTSUPP;
> +
> + if (channel)
> + return -EOPNOTSUPP;
> +
There is only one channel, one attribute, and one type supported.
As such, the checks are unnecessary.
> + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> + return 0;
> +}
> +
> +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + if (type != hwmon_in)
> + return 0;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return 0;
> +
> + if (channel)
> + return 0;
Same as above. In the list below, there is not a single conditional attribute.
Given that, the checks should be unnecessary, and it should be sufficient to
just return 0444.
> +
> + return 0444;
> +}
> +
> +static const u32 rpi_in_config[] = {
> + HWMON_I_LCRIT_ALARM,
> + 0
> +};
> +
> +static const struct hwmon_channel_info rpi_in = {
> + .type = hwmon_in,
> + .config = rpi_in_config,
> +};
> +
> +static const struct hwmon_channel_info *rpi_info[] = {
> + &rpi_in,
> + NULL
> +};
> +
> +static const struct hwmon_ops rpi_hwmon_ops = {
> + .is_visible = rpi_is_visible,
> + .read = rpi_read,
> +};
> +
> +static const struct hwmon_chip_info rpi_chip_info = {
> + .ops = &rpi_hwmon_ops,
> + .info = rpi_info,
> +};
> +
> +static int rpi_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *fw_node;
> + struct rpi_hwmon_data *data;
> + int ret;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + fw_node = of_get_parent(dev->of_node);
> + if (!fw_node) {
> + dev_err(dev, "Missing firmware node\n");
> + return -ENOENT;
> + }
> +
> + data->fw = rpi_firmware_get(fw_node);
> + of_node_put(fw_node);
> + if (!data->fw)
> + return -EPROBE_DEFER;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &data->last_throttled,
> + sizeof(data->last_throttled));
> + if (ret) {
> + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
If this is an error -> dev_err().
> + return -EOPNOTSUPP;
or return -ENODEV.
> + }
> +
> + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> + data,
> + &rpi_chip_info,
> + NULL);
> +
> + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> + platform_set_drvdata(pdev, data);
> +
> + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +
> + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> +}
> +
> +static int rpi_hwmon_remove(struct platform_device *pdev)
> +{
> + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> +
> + cancel_delayed_work_sync(&data->get_values_poll_work);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rpi_hwmon_of_match[] = {
> + { .compatible = "raspberrypi,bcm2835-hwmon", },
> + { /* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> +
> +static struct platform_driver rpi_hwmon_driver = {
> + .probe = rpi_hwmon_probe,
> + .remove = rpi_hwmon_remove,
> + .driver = {
> + .name = "raspberrypi-hwmon",
> + .of_match_table = rpi_hwmon_of_match,
> + },
> +};
> +module_platform_driver(rpi_hwmon_driver);
> +
> +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-16 18:21 ` Guenter Roeck
0 siblings, 0 replies; 50+ messages in thread
From: Guenter Roeck @ 2018-05-16 18:21 UTC (permalink / raw)
To: Stefan Wahren
Cc: Mark Rutland, devicetree, Jean Delvare, Scott Branden, Ray Jui,
Phil Elwell, Eric Anholt, Rob Herring, bcm-kernel-feedback-list,
linux-rpi-kernel, Florian Fainelli, linux-hwmon,
linux-arm-kernel, Noralf Trønnes
On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
> Currently there is no easy way to detect under-voltage conditions on a remote
> Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> via mailbox interface. The handling based on Noralf's modifications to the
> downstream firmware driver. In case of an under-voltage condition only an entry
> is written to the kernel log.
>
My major concern is how this is displayed with the 'sensors' command.
Can you test and report ?
Of course, it would be much better if the firmware would also report
the actual voltage, but I guess we can't have everything.
More comments inline.
Thanks,
Guenter
> CC: "Noralf Trønnes" <noralf@tronnes.org>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
Please also provide Documentation/hwmon/raspberrypi-hwmon.
> 3 files changed, 218 insertions(+)
> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 768aed5..7f935cf 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> This driver can also be built as a module. If so, the module
> will be called pwm-fan.
>
> +config SENSORS_RASPBERRYPI_HWMON
> + tristate "Raspberry Pi voltage monitor"
> + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> + help
> + If you say yes here you get support for voltage sensor on the
> + Raspberry Pi.
> +
> + This driver can also be built as a module. If so, the module
> + will be called raspberrypi-hwmon.
> +
> config SENSORS_SHT15
> tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index e7d52a3..a929770 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> new file mode 100644
> index 0000000..2003f6c
> --- /dev/null
> +++ b/drivers/hwmon/raspberrypi-hwmon.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Raspberry Pi voltage sensor driver
> + *
> + * Based on firmware/raspberrypi.c by Noralf Trønnes
> + *
> + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> + */
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
Unnecessary include
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
> +#include <soc/bcm2835/raspberrypi-firmware.h>
> +
> +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> +
> +struct rpi_hwmon_data {
> + struct device *hwmon_dev;
> + struct rpi_firmware *fw;
> + u32 last_throttled;
> + struct delayed_work get_values_poll_work;
> +};
> +
> +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> +{
> + u32 new_uv, old_uv, value;
> + int ret;
> +
> + /* Clear sticky bits */
Please make more explicit that this is a request/command sent to the firmware.
> + value = 0xffff;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &value, sizeof(value));
> + if (ret) {
> + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> + __func__, ret);
The function name seems unnecessary.
> + return;
> + }
> +
> + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> + data->last_throttled = value;
> +
> + if (new_uv == old_uv)
> + return;
> +
> + if (new_uv)
> + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> + value);
> + else
> + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> + value);
What value do those hex values provide to the user ?
> +
> + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> +}
> +
> +static void get_values_poll(struct work_struct *work)
> +{
> + struct rpi_hwmon_data *data;
> +
> + data = container_of(work, struct rpi_hwmon_data,
> + get_values_poll_work.work);
> +
> + rpi_firmware_get_throttled(data);
> +
> + /*
> + * We can't run faster than the sticky shift (100ms) since we get
> + * flipping in the sticky bits that are cleared.
> + */
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +}
> +
> +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> +
> + if (type != hwmon_in)
> + return -EOPNOTSUPP;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return -EOPNOTSUPP;
> +
> + if (channel)
> + return -EOPNOTSUPP;
> +
There is only one channel, one attribute, and one type supported.
As such, the checks are unnecessary.
> + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> + return 0;
> +}
> +
> +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + if (type != hwmon_in)
> + return 0;
> +
> + if (attr != hwmon_in_lcrit_alarm)
> + return 0;
> +
> + if (channel)
> + return 0;
Same as above. In the list below, there is not a single conditional attribute.
Given that, the checks should be unnecessary, and it should be sufficient to
just return 0444.
> +
> + return 0444;
> +}
> +
> +static const u32 rpi_in_config[] = {
> + HWMON_I_LCRIT_ALARM,
> + 0
> +};
> +
> +static const struct hwmon_channel_info rpi_in = {
> + .type = hwmon_in,
> + .config = rpi_in_config,
> +};
> +
> +static const struct hwmon_channel_info *rpi_info[] = {
> + &rpi_in,
> + NULL
> +};
> +
> +static const struct hwmon_ops rpi_hwmon_ops = {
> + .is_visible = rpi_is_visible,
> + .read = rpi_read,
> +};
> +
> +static const struct hwmon_chip_info rpi_chip_info = {
> + .ops = &rpi_hwmon_ops,
> + .info = rpi_info,
> +};
> +
> +static int rpi_hwmon_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *fw_node;
> + struct rpi_hwmon_data *data;
> + int ret;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + fw_node = of_get_parent(dev->of_node);
> + if (!fw_node) {
> + dev_err(dev, "Missing firmware node\n");
> + return -ENOENT;
> + }
> +
> + data->fw = rpi_firmware_get(fw_node);
> + of_node_put(fw_node);
> + if (!data->fw)
> + return -EPROBE_DEFER;
> +
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &data->last_throttled,
> + sizeof(data->last_throttled));
> + if (ret) {
> + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
If this is an error -> dev_err().
> + return -EOPNOTSUPP;
or return -ENODEV.
> + }
> +
> + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> + data,
> + &rpi_chip_info,
> + NULL);
> +
> + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> + platform_set_drvdata(pdev, data);
> +
> + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> +
> + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> +}
> +
> +static int rpi_hwmon_remove(struct platform_device *pdev)
> +{
> + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> +
> + cancel_delayed_work_sync(&data->get_values_poll_work);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id rpi_hwmon_of_match[] = {
> + { .compatible = "raspberrypi,bcm2835-hwmon", },
> + { /* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> +
> +static struct platform_driver rpi_hwmon_driver = {
> + .probe = rpi_hwmon_probe,
> + .remove = rpi_hwmon_remove,
> + .driver = {
> + .name = "raspberrypi-hwmon",
> + .of_match_table = rpi_hwmon_of_match,
> + },
> +};
> +module_platform_driver(rpi_hwmon_driver);
> +
> +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
2018-05-16 18:21 ` Guenter Roeck
(?)
@ 2018-05-16 19:59 ` Stefan Wahren
-1 siblings, 0 replies; 50+ messages in thread
From: Stefan Wahren @ 2018-05-16 19:59 UTC (permalink / raw)
To: Guenter Roeck
Cc: Rob Herring, Eric Anholt, bcm-kernel-feedback-list, devicetree,
Noralf Trønnes, Ray Jui, Scott Branden, linux-rpi-kernel,
Florian Fainelli, Mark Rutland, Phil Elwell, linux-hwmon,
Jean Delvare, linux-arm-kernel
Hi Guenter,
> Guenter Roeck <linux@roeck-us.net> hat am 16. Mai 2018 um 20:21 geschrieben:
>
>
> On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
> > Currently there is no easy way to detect under-voltage conditions on a remote
> > Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> > via mailbox interface. The handling based on Noralf's modifications to the
> > downstream firmware driver. In case of an under-voltage condition only an entry
> > is written to the kernel log.
> >
>
> My major concern is how this is displayed with the 'sensors' command.
> Can you test and report ?
I get the following output:
rpi_volt-isa-0000
Adapter: ISA adapter
in0: N/A
>
> Of course, it would be much better if the firmware would also report
> the actual voltage, but I guess we can't have everything.
I think this isn't possible because the hardware only provide a binary value (GPIO).
>
> More comments inline.
>
> Thanks,
> Guenter
>
> > CC: "Noralf Trønnes" <noralf@tronnes.org>
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > ---
> > drivers/hwmon/Kconfig | 10 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
>
> Please also provide Documentation/hwmon/raspberrypi-hwmon.
>
> > 3 files changed, 218 insertions(+)
> > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 768aed5..7f935cf 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> > This driver can also be built as a module. If so, the module
> > will be called pwm-fan.
> >
> > +config SENSORS_RASPBERRYPI_HWMON
> > + tristate "Raspberry Pi voltage monitor"
> > + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> > + help
> > + If you say yes here you get support for voltage sensor on the
> > + Raspberry Pi.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called raspberrypi-hwmon.
> > +
> > config SENSORS_SHT15
> > tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> > depends on GPIOLIB || COMPILE_TEST
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index e7d52a3..a929770 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> > +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> > diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> > new file mode 100644
> > index 0000000..2003f6c
> > --- /dev/null
> > +++ b/drivers/hwmon/raspberrypi-hwmon.c
> > @@ -0,0 +1,207 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Raspberry Pi voltage sensor driver
> > + *
> > + * Based on firmware/raspberrypi.c by Noralf Trønnes
> > + *
> > + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> > + */
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
>
> Unnecessary include
>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/workqueue.h>
> > +#include <soc/bcm2835/raspberrypi-firmware.h>
> > +
> > +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> > +
> > +struct rpi_hwmon_data {
> > + struct device *hwmon_dev;
> > + struct rpi_firmware *fw;
> > + u32 last_throttled;
> > + struct delayed_work get_values_poll_work;
> > +};
> > +
> > +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> > +{
> > + u32 new_uv, old_uv, value;
> > + int ret;
> > +
> > + /* Clear sticky bits */
>
> Please make more explicit that this is a request/command sent to the firmware.
>
> > + value = 0xffff;
> > +
> > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > + &value, sizeof(value));
> > + if (ret) {
> > + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> > + __func__, ret);
>
> The function name seems unnecessary.
>
> > + return;
> > + }
> > +
> > + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> > + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> > + data->last_throttled = value;
> > +
> > + if (new_uv == old_uv)
> > + return;
> > +
> > + if (new_uv)
> > + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> > + value);
> > + else
> > + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> > + value);
>
> What value do those hex values provide to the user ?
The actual definition of the bits can be found in the commit log of patch #1. But this isn't very helpful for an end user.
>
> > +
> > + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> > +}
> > +
> > +static void get_values_poll(struct work_struct *work)
> > +{
> > + struct rpi_hwmon_data *data;
> > +
> > + data = container_of(work, struct rpi_hwmon_data,
> > + get_values_poll_work.work);
> > +
> > + rpi_firmware_get_throttled(data);
> > +
> > + /*
> > + * We can't run faster than the sticky shift (100ms) since we get
> > + * flipping in the sticky bits that are cleared.
> > + */
> > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > +}
> > +
> > +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
> > +{
> > + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> > +
> > + if (type != hwmon_in)
> > + return -EOPNOTSUPP;
> > +
> > + if (attr != hwmon_in_lcrit_alarm)
> > + return -EOPNOTSUPP;
> > +
> > + if (channel)
> > + return -EOPNOTSUPP;
> > +
> There is only one channel, one attribute, and one type supported.
> As such, the checks are unnecessary.
>
> > + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> > + return 0;
> > +}
> > +
> > +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> > + u32 attr, int channel)
> > +{
> > + if (type != hwmon_in)
> > + return 0;
> > +
> > + if (attr != hwmon_in_lcrit_alarm)
> > + return 0;
> > +
> > + if (channel)
> > + return 0;
>
> Same as above. In the list below, there is not a single conditional attribute.
> Given that, the checks should be unnecessary, and it should be sufficient to
> just return 0444.
>
> > +
> > + return 0444;
> > +}
> > +
> > +static const u32 rpi_in_config[] = {
> > + HWMON_I_LCRIT_ALARM,
> > + 0
> > +};
> > +
> > +static const struct hwmon_channel_info rpi_in = {
> > + .type = hwmon_in,
> > + .config = rpi_in_config,
> > +};
> > +
> > +static const struct hwmon_channel_info *rpi_info[] = {
> > + &rpi_in,
> > + NULL
> > +};
> > +
> > +static const struct hwmon_ops rpi_hwmon_ops = {
> > + .is_visible = rpi_is_visible,
> > + .read = rpi_read,
> > +};
> > +
> > +static const struct hwmon_chip_info rpi_chip_info = {
> > + .ops = &rpi_hwmon_ops,
> > + .info = rpi_info,
> > +};
> > +
> > +static int rpi_hwmon_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *fw_node;
> > + struct rpi_hwmon_data *data;
> > + int ret;
> > +
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + fw_node = of_get_parent(dev->of_node);
> > + if (!fw_node) {
> > + dev_err(dev, "Missing firmware node\n");
> > + return -ENOENT;
> > + }
> > +
> > + data->fw = rpi_firmware_get(fw_node);
> > + of_node_put(fw_node);
> > + if (!data->fw)
> > + return -EPROBE_DEFER;
> > +
> > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > + &data->last_throttled,
> > + sizeof(data->last_throttled));
> > + if (ret) {
> > + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
>
> If this is an error -> dev_err().
I wasn't sure. If the firmware is too old, we cannot provide this feature and it's a waste of resources to load this driver. On the other side i don't want to confuse people this is something bad.
Stefan
>
> > + return -EOPNOTSUPP;
>
> or return -ENODEV.
>
> > + }
> > +
> > + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> > + data,
> > + &rpi_chip_info,
> > + NULL);
> > +
> > + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> > + platform_set_drvdata(pdev, data);
> > +
> > + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > +
> > + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> > +}
> > +
> > +static int rpi_hwmon_remove(struct platform_device *pdev)
> > +{
> > + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> > +
> > + cancel_delayed_work_sync(&data->get_values_poll_work);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id rpi_hwmon_of_match[] = {
> > + { .compatible = "raspberrypi,bcm2835-hwmon", },
> > + { /* sentinel */},
> > +};
> > +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> > +
> > +static struct platform_driver rpi_hwmon_driver = {
> > + .probe = rpi_hwmon_probe,
> > + .remove = rpi_hwmon_remove,
> > + .driver = {
> > + .name = "raspberrypi-hwmon",
> > + .of_match_table = rpi_hwmon_of_match,
> > + },
> > +};
> > +module_platform_driver(rpi_hwmon_driver);
> > +
> > +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> > +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-16 19:59 ` Stefan Wahren
0 siblings, 0 replies; 50+ messages in thread
From: Stefan Wahren @ 2018-05-16 19:59 UTC (permalink / raw)
To: linux-arm-kernel
Hi Guenter,
> Guenter Roeck <linux@roeck-us.net> hat am 16. Mai 2018 um 20:21 geschrieben:
>
>
> On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
> > Currently there is no easy way to detect under-voltage conditions on a remote
> > Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> > via mailbox interface. The handling based on Noralf's modifications to the
> > downstream firmware driver. In case of an under-voltage condition only an entry
> > is written to the kernel log.
> >
>
> My major concern is how this is displayed with the 'sensors' command.
> Can you test and report ?
I get the following output:
rpi_volt-isa-0000
Adapter: ISA adapter
in0: N/A
>
> Of course, it would be much better if the firmware would also report
> the actual voltage, but I guess we can't have everything.
I think this isn't possible because the hardware only provide a binary value (GPIO).
>
> More comments inline.
>
> Thanks,
> Guenter
>
> > CC: "Noralf Tr?nnes" <noralf@tronnes.org>
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > ---
> > drivers/hwmon/Kconfig | 10 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
>
> Please also provide Documentation/hwmon/raspberrypi-hwmon.
>
> > 3 files changed, 218 insertions(+)
> > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 768aed5..7f935cf 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> > This driver can also be built as a module. If so, the module
> > will be called pwm-fan.
> >
> > +config SENSORS_RASPBERRYPI_HWMON
> > + tristate "Raspberry Pi voltage monitor"
> > + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> > + help
> > + If you say yes here you get support for voltage sensor on the
> > + Raspberry Pi.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called raspberrypi-hwmon.
> > +
> > config SENSORS_SHT15
> > tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> > depends on GPIOLIB || COMPILE_TEST
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index e7d52a3..a929770 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> > +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> > diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> > new file mode 100644
> > index 0000000..2003f6c
> > --- /dev/null
> > +++ b/drivers/hwmon/raspberrypi-hwmon.c
> > @@ -0,0 +1,207 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Raspberry Pi voltage sensor driver
> > + *
> > + * Based on firmware/raspberrypi.c by Noralf Tr?nnes
> > + *
> > + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> > + */
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
>
> Unnecessary include
>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/workqueue.h>
> > +#include <soc/bcm2835/raspberrypi-firmware.h>
> > +
> > +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> > +
> > +struct rpi_hwmon_data {
> > + struct device *hwmon_dev;
> > + struct rpi_firmware *fw;
> > + u32 last_throttled;
> > + struct delayed_work get_values_poll_work;
> > +};
> > +
> > +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> > +{
> > + u32 new_uv, old_uv, value;
> > + int ret;
> > +
> > + /* Clear sticky bits */
>
> Please make more explicit that this is a request/command sent to the firmware.
>
> > + value = 0xffff;
> > +
> > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > + &value, sizeof(value));
> > + if (ret) {
> > + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> > + __func__, ret);
>
> The function name seems unnecessary.
>
> > + return;
> > + }
> > +
> > + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> > + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> > + data->last_throttled = value;
> > +
> > + if (new_uv == old_uv)
> > + return;
> > +
> > + if (new_uv)
> > + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> > + value);
> > + else
> > + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> > + value);
>
> What value do those hex values provide to the user ?
The actual definition of the bits can be found in the commit log of patch #1. But this isn't very helpful for an end user.
>
> > +
> > + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> > +}
> > +
> > +static void get_values_poll(struct work_struct *work)
> > +{
> > + struct rpi_hwmon_data *data;
> > +
> > + data = container_of(work, struct rpi_hwmon_data,
> > + get_values_poll_work.work);
> > +
> > + rpi_firmware_get_throttled(data);
> > +
> > + /*
> > + * We can't run faster than the sticky shift (100ms) since we get
> > + * flipping in the sticky bits that are cleared.
> > + */
> > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > +}
> > +
> > +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
> > +{
> > + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> > +
> > + if (type != hwmon_in)
> > + return -EOPNOTSUPP;
> > +
> > + if (attr != hwmon_in_lcrit_alarm)
> > + return -EOPNOTSUPP;
> > +
> > + if (channel)
> > + return -EOPNOTSUPP;
> > +
> There is only one channel, one attribute, and one type supported.
> As such, the checks are unnecessary.
>
> > + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> > + return 0;
> > +}
> > +
> > +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> > + u32 attr, int channel)
> > +{
> > + if (type != hwmon_in)
> > + return 0;
> > +
> > + if (attr != hwmon_in_lcrit_alarm)
> > + return 0;
> > +
> > + if (channel)
> > + return 0;
>
> Same as above. In the list below, there is not a single conditional attribute.
> Given that, the checks should be unnecessary, and it should be sufficient to
> just return 0444.
>
> > +
> > + return 0444;
> > +}
> > +
> > +static const u32 rpi_in_config[] = {
> > + HWMON_I_LCRIT_ALARM,
> > + 0
> > +};
> > +
> > +static const struct hwmon_channel_info rpi_in = {
> > + .type = hwmon_in,
> > + .config = rpi_in_config,
> > +};
> > +
> > +static const struct hwmon_channel_info *rpi_info[] = {
> > + &rpi_in,
> > + NULL
> > +};
> > +
> > +static const struct hwmon_ops rpi_hwmon_ops = {
> > + .is_visible = rpi_is_visible,
> > + .read = rpi_read,
> > +};
> > +
> > +static const struct hwmon_chip_info rpi_chip_info = {
> > + .ops = &rpi_hwmon_ops,
> > + .info = rpi_info,
> > +};
> > +
> > +static int rpi_hwmon_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *fw_node;
> > + struct rpi_hwmon_data *data;
> > + int ret;
> > +
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + fw_node = of_get_parent(dev->of_node);
> > + if (!fw_node) {
> > + dev_err(dev, "Missing firmware node\n");
> > + return -ENOENT;
> > + }
> > +
> > + data->fw = rpi_firmware_get(fw_node);
> > + of_node_put(fw_node);
> > + if (!data->fw)
> > + return -EPROBE_DEFER;
> > +
> > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > + &data->last_throttled,
> > + sizeof(data->last_throttled));
> > + if (ret) {
> > + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
>
> If this is an error -> dev_err().
I wasn't sure. If the firmware is too old, we cannot provide this feature and it's a waste of resources to load this driver. On the other side i don't want to confuse people this is something bad.
Stefan
>
> > + return -EOPNOTSUPP;
>
> or return -ENODEV.
>
> > + }
> > +
> > + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> > + data,
> > + &rpi_chip_info,
> > + NULL);
> > +
> > + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> > + platform_set_drvdata(pdev, data);
> > +
> > + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > +
> > + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> > +}
> > +
> > +static int rpi_hwmon_remove(struct platform_device *pdev)
> > +{
> > + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> > +
> > + cancel_delayed_work_sync(&data->get_values_poll_work);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id rpi_hwmon_of_match[] = {
> > + { .compatible = "raspberrypi,bcm2835-hwmon", },
> > + { /* sentinel */},
> > +};
> > +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> > +
> > +static struct platform_driver rpi_hwmon_driver = {
> > + .probe = rpi_hwmon_probe,
> > + .remove = rpi_hwmon_remove,
> > + .driver = {
> > + .name = "raspberrypi-hwmon",
> > + .of_match_table = rpi_hwmon_of_match,
> > + },
> > +};
> > +module_platform_driver(rpi_hwmon_driver);
> > +
> > +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> > +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-16 19:59 ` Stefan Wahren
0 siblings, 0 replies; 50+ messages in thread
From: Stefan Wahren @ 2018-05-16 19:59 UTC (permalink / raw)
To: Guenter Roeck
Cc: Mark Rutland, devicetree, Jean Delvare, Florian Fainelli,
Scott Branden, Ray Jui, Phil Elwell, Rob Herring, Eric Anholt,
Noralf Trønnes, bcm-kernel-feedback-list, linux-rpi-kernel,
linux-hwmon, linux-arm-kernel
Hi Guenter,
> Guenter Roeck <linux@roeck-us.net> hat am 16. Mai 2018 um 20:21 geschrieben:
>
>
> On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
> > Currently there is no easy way to detect under-voltage conditions on a remote
> > Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> > via mailbox interface. The handling based on Noralf's modifications to the
> > downstream firmware driver. In case of an under-voltage condition only an entry
> > is written to the kernel log.
> >
>
> My major concern is how this is displayed with the 'sensors' command.
> Can you test and report ?
I get the following output:
rpi_volt-isa-0000
Adapter: ISA adapter
in0: N/A
>
> Of course, it would be much better if the firmware would also report
> the actual voltage, but I guess we can't have everything.
I think this isn't possible because the hardware only provide a binary value (GPIO).
>
> More comments inline.
>
> Thanks,
> Guenter
>
> > CC: "Noralf Trønnes" <noralf@tronnes.org>
> > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > ---
> > drivers/hwmon/Kconfig | 10 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
>
> Please also provide Documentation/hwmon/raspberrypi-hwmon.
>
> > 3 files changed, 218 insertions(+)
> > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> >
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 768aed5..7f935cf 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> > This driver can also be built as a module. If so, the module
> > will be called pwm-fan.
> >
> > +config SENSORS_RASPBERRYPI_HWMON
> > + tristate "Raspberry Pi voltage monitor"
> > + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> > + help
> > + If you say yes here you get support for voltage sensor on the
> > + Raspberry Pi.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called raspberrypi-hwmon.
> > +
> > config SENSORS_SHT15
> > tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> > depends on GPIOLIB || COMPILE_TEST
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index e7d52a3..a929770 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> > +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> > diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> > new file mode 100644
> > index 0000000..2003f6c
> > --- /dev/null
> > +++ b/drivers/hwmon/raspberrypi-hwmon.c
> > @@ -0,0 +1,207 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Raspberry Pi voltage sensor driver
> > + *
> > + * Based on firmware/raspberrypi.c by Noralf Trønnes
> > + *
> > + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> > + */
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/hwmon-sysfs.h>
>
> Unnecessary include
>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/workqueue.h>
> > +#include <soc/bcm2835/raspberrypi-firmware.h>
> > +
> > +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> > +
> > +struct rpi_hwmon_data {
> > + struct device *hwmon_dev;
> > + struct rpi_firmware *fw;
> > + u32 last_throttled;
> > + struct delayed_work get_values_poll_work;
> > +};
> > +
> > +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> > +{
> > + u32 new_uv, old_uv, value;
> > + int ret;
> > +
> > + /* Clear sticky bits */
>
> Please make more explicit that this is a request/command sent to the firmware.
>
> > + value = 0xffff;
> > +
> > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > + &value, sizeof(value));
> > + if (ret) {
> > + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> > + __func__, ret);
>
> The function name seems unnecessary.
>
> > + return;
> > + }
> > +
> > + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> > + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> > + data->last_throttled = value;
> > +
> > + if (new_uv == old_uv)
> > + return;
> > +
> > + if (new_uv)
> > + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> > + value);
> > + else
> > + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> > + value);
>
> What value do those hex values provide to the user ?
The actual definition of the bits can be found in the commit log of patch #1. But this isn't very helpful for an end user.
>
> > +
> > + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> > +}
> > +
> > +static void get_values_poll(struct work_struct *work)
> > +{
> > + struct rpi_hwmon_data *data;
> > +
> > + data = container_of(work, struct rpi_hwmon_data,
> > + get_values_poll_work.work);
> > +
> > + rpi_firmware_get_throttled(data);
> > +
> > + /*
> > + * We can't run faster than the sticky shift (100ms) since we get
> > + * flipping in the sticky bits that are cleared.
> > + */
> > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > +}
> > +
> > +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> > + u32 attr, int channel, long *val)
> > +{
> > + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> > +
> > + if (type != hwmon_in)
> > + return -EOPNOTSUPP;
> > +
> > + if (attr != hwmon_in_lcrit_alarm)
> > + return -EOPNOTSUPP;
> > +
> > + if (channel)
> > + return -EOPNOTSUPP;
> > +
> There is only one channel, one attribute, and one type supported.
> As such, the checks are unnecessary.
>
> > + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> > + return 0;
> > +}
> > +
> > +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> > + u32 attr, int channel)
> > +{
> > + if (type != hwmon_in)
> > + return 0;
> > +
> > + if (attr != hwmon_in_lcrit_alarm)
> > + return 0;
> > +
> > + if (channel)
> > + return 0;
>
> Same as above. In the list below, there is not a single conditional attribute.
> Given that, the checks should be unnecessary, and it should be sufficient to
> just return 0444.
>
> > +
> > + return 0444;
> > +}
> > +
> > +static const u32 rpi_in_config[] = {
> > + HWMON_I_LCRIT_ALARM,
> > + 0
> > +};
> > +
> > +static const struct hwmon_channel_info rpi_in = {
> > + .type = hwmon_in,
> > + .config = rpi_in_config,
> > +};
> > +
> > +static const struct hwmon_channel_info *rpi_info[] = {
> > + &rpi_in,
> > + NULL
> > +};
> > +
> > +static const struct hwmon_ops rpi_hwmon_ops = {
> > + .is_visible = rpi_is_visible,
> > + .read = rpi_read,
> > +};
> > +
> > +static const struct hwmon_chip_info rpi_chip_info = {
> > + .ops = &rpi_hwmon_ops,
> > + .info = rpi_info,
> > +};
> > +
> > +static int rpi_hwmon_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *fw_node;
> > + struct rpi_hwmon_data *data;
> > + int ret;
> > +
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + fw_node = of_get_parent(dev->of_node);
> > + if (!fw_node) {
> > + dev_err(dev, "Missing firmware node\n");
> > + return -ENOENT;
> > + }
> > +
> > + data->fw = rpi_firmware_get(fw_node);
> > + of_node_put(fw_node);
> > + if (!data->fw)
> > + return -EPROBE_DEFER;
> > +
> > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > + &data->last_throttled,
> > + sizeof(data->last_throttled));
> > + if (ret) {
> > + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
>
> If this is an error -> dev_err().
I wasn't sure. If the firmware is too old, we cannot provide this feature and it's a waste of resources to load this driver. On the other side i don't want to confuse people this is something bad.
Stefan
>
> > + return -EOPNOTSUPP;
>
> or return -ENODEV.
>
> > + }
> > +
> > + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> > + data,
> > + &rpi_chip_info,
> > + NULL);
> > +
> > + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> > + platform_set_drvdata(pdev, data);
> > +
> > + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > +
> > + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> > +}
> > +
> > +static int rpi_hwmon_remove(struct platform_device *pdev)
> > +{
> > + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> > +
> > + cancel_delayed_work_sync(&data->get_values_poll_work);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id rpi_hwmon_of_match[] = {
> > + { .compatible = "raspberrypi,bcm2835-hwmon", },
> > + { /* sentinel */},
> > +};
> > +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> > +
> > +static struct platform_driver rpi_hwmon_driver = {
> > + .probe = rpi_hwmon_probe,
> > + .remove = rpi_hwmon_remove,
> > + .driver = {
> > + .name = "raspberrypi-hwmon",
> > + .of_match_table = rpi_hwmon_of_match,
> > + },
> > +};
> > +module_platform_driver(rpi_hwmon_driver);
> > +
> > +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> > +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
2018-05-16 19:59 ` Stefan Wahren
(?)
@ 2018-05-17 16:43 ` Guenter Roeck
-1 siblings, 0 replies; 50+ messages in thread
From: Guenter Roeck @ 2018-05-17 16:43 UTC (permalink / raw)
To: Stefan Wahren
Cc: Rob Herring, Eric Anholt, bcm-kernel-feedback-list, devicetree,
Noralf Trønnes, Ray Jui, Scott Branden, linux-rpi-kernel,
Florian Fainelli, Mark Rutland, Phil Elwell, linux-hwmon,
Jean Delvare, linux-arm-kernel
On Wed, May 16, 2018 at 09:59:01PM +0200, Stefan Wahren wrote:
> Hi Guenter,
>
> > Guenter Roeck <linux@roeck-us.net> hat am 16. Mai 2018 um 20:21 geschrieben:
> >
> >
> > On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
> > > Currently there is no easy way to detect under-voltage conditions on a remote
> > > Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> > > via mailbox interface. The handling based on Noralf's modifications to the
> > > downstream firmware driver. In case of an under-voltage condition only an entry
> > > is written to the kernel log.
> > >
> >
> > My major concern is how this is displayed with the 'sensors' command.
> > Can you test and report ?
>
> I get the following output:
> rpi_volt-isa-0000
> Adapter: ISA adapter
> in0: N/A
>
Ok, that works.
> >
> > Of course, it would be much better if the firmware would also report
> > the actual voltage, but I guess we can't have everything.
>
> I think this isn't possible because the hardware only provide a binary value (GPIO).
>
> >
> > More comments inline.
> >
> > Thanks,
> > Guenter
> >
> > > CC: "Noralf Trønnes" <noralf@tronnes.org>
> > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > ---
> > > drivers/hwmon/Kconfig | 10 ++
> > > drivers/hwmon/Makefile | 1 +
> > > drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
> >
> > Please also provide Documentation/hwmon/raspberrypi-hwmon.
> >
> > > 3 files changed, 218 insertions(+)
> > > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> > >
> > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > index 768aed5..7f935cf 100644
> > > --- a/drivers/hwmon/Kconfig
> > > +++ b/drivers/hwmon/Kconfig
> > > @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> > > This driver can also be built as a module. If so, the module
> > > will be called pwm-fan.
> > >
> > > +config SENSORS_RASPBERRYPI_HWMON
> > > + tristate "Raspberry Pi voltage monitor"
> > > + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> > > + help
> > > + If you say yes here you get support for voltage sensor on the
> > > + Raspberry Pi.
> > > +
> > > + This driver can also be built as a module. If so, the module
> > > + will be called raspberrypi-hwmon.
> > > +
> > > config SENSORS_SHT15
> > > tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> > > depends on GPIOLIB || COMPILE_TEST
> > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > > index e7d52a3..a929770 100644
> > > --- a/drivers/hwmon/Makefile
> > > +++ b/drivers/hwmon/Makefile
> > > @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> > > +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> > > diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> > > new file mode 100644
> > > index 0000000..2003f6c
> > > --- /dev/null
> > > +++ b/drivers/hwmon/raspberrypi-hwmon.c
> > > @@ -0,0 +1,207 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Raspberry Pi voltage sensor driver
> > > + *
> > > + * Based on firmware/raspberrypi.c by Noralf Trønnes
> > > + *
> > > + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> > > + */
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/hwmon.h>
> > > +#include <linux/hwmon-sysfs.h>
> >
> > Unnecessary include
> >
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/workqueue.h>
> > > +#include <soc/bcm2835/raspberrypi-firmware.h>
> > > +
> > > +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> > > +
> > > +struct rpi_hwmon_data {
> > > + struct device *hwmon_dev;
> > > + struct rpi_firmware *fw;
> > > + u32 last_throttled;
> > > + struct delayed_work get_values_poll_work;
> > > +};
> > > +
> > > +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> > > +{
> > > + u32 new_uv, old_uv, value;
> > > + int ret;
> > > +
> > > + /* Clear sticky bits */
> >
> > Please make more explicit that this is a request/command sent to the firmware.
> >
> > > + value = 0xffff;
> > > +
> > > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > > + &value, sizeof(value));
> > > + if (ret) {
> > > + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> > > + __func__, ret);
> >
> > The function name seems unnecessary.
> >
> > > + return;
> > > + }
> > > +
> > > + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> > > + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> > > + data->last_throttled = value;
> > > +
> > > + if (new_uv == old_uv)
> > > + return;
> > > +
> > > + if (new_uv)
> > > + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> > > + value);
> > > + else
> > > + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> > > + value);
> >
> > What value do those hex values provide to the user ?
>
> The actual definition of the bits can be found in the commit log of patch #1. But this isn't very helpful for an end user.
>
I would suggest to drop the value from the log.
> >
> > > +
> > > + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> > > +}
> > > +
> > > +static void get_values_poll(struct work_struct *work)
> > > +{
> > > + struct rpi_hwmon_data *data;
> > > +
> > > + data = container_of(work, struct rpi_hwmon_data,
> > > + get_values_poll_work.work);
> > > +
> > > + rpi_firmware_get_throttled(data);
> > > +
> > > + /*
> > > + * We can't run faster than the sticky shift (100ms) since we get
> > > + * flipping in the sticky bits that are cleared.
> > > + */
> > > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > > +}
> > > +
> > > +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> > > + u32 attr, int channel, long *val)
> > > +{
> > > + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> > > +
> > > + if (type != hwmon_in)
> > > + return -EOPNOTSUPP;
> > > +
> > > + if (attr != hwmon_in_lcrit_alarm)
> > > + return -EOPNOTSUPP;
> > > +
> > > + if (channel)
> > > + return -EOPNOTSUPP;
> > > +
> > There is only one channel, one attribute, and one type supported.
> > As such, the checks are unnecessary.
> >
> > > + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> > > + return 0;
> > > +}
> > > +
> > > +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> > > + u32 attr, int channel)
> > > +{
> > > + if (type != hwmon_in)
> > > + return 0;
> > > +
> > > + if (attr != hwmon_in_lcrit_alarm)
> > > + return 0;
> > > +
> > > + if (channel)
> > > + return 0;
> >
> > Same as above. In the list below, there is not a single conditional attribute.
> > Given that, the checks should be unnecessary, and it should be sufficient to
> > just return 0444.
> >
> > > +
> > > + return 0444;
> > > +}
> > > +
> > > +static const u32 rpi_in_config[] = {
> > > + HWMON_I_LCRIT_ALARM,
> > > + 0
> > > +};
> > > +
> > > +static const struct hwmon_channel_info rpi_in = {
> > > + .type = hwmon_in,
> > > + .config = rpi_in_config,
> > > +};
> > > +
> > > +static const struct hwmon_channel_info *rpi_info[] = {
> > > + &rpi_in,
> > > + NULL
> > > +};
> > > +
> > > +static const struct hwmon_ops rpi_hwmon_ops = {
> > > + .is_visible = rpi_is_visible,
> > > + .read = rpi_read,
> > > +};
> > > +
> > > +static const struct hwmon_chip_info rpi_chip_info = {
> > > + .ops = &rpi_hwmon_ops,
> > > + .info = rpi_info,
> > > +};
> > > +
> > > +static int rpi_hwmon_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + struct device_node *fw_node;
> > > + struct rpi_hwmon_data *data;
> > > + int ret;
> > > +
> > > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > > + if (!data)
> > > + return -ENOMEM;
> > > +
> > > + fw_node = of_get_parent(dev->of_node);
> > > + if (!fw_node) {
> > > + dev_err(dev, "Missing firmware node\n");
> > > + return -ENOENT;
> > > + }
> > > +
> > > + data->fw = rpi_firmware_get(fw_node);
> > > + of_node_put(fw_node);
> > > + if (!data->fw)
> > > + return -EPROBE_DEFER;
> > > +
> > > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > > + &data->last_throttled,
> > > + sizeof(data->last_throttled));
> > > + if (ret) {
> > > + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
> >
> > If this is an error -> dev_err().
>
> I wasn't sure. If the firmware is too old, we cannot provide this feature and it's a waste of resources to load this driver. On the other side i don't want to confuse people this is something bad.
>
Then I would suggest to return -ENODEV.
Guenter
> Stefan
>
> >
> > > + return -EOPNOTSUPP;
> >
> > or return -ENODEV.
> >
> > > + }
> > > +
> > > + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> > > + data,
> > > + &rpi_chip_info,
> > > + NULL);
> > > +
> > > + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> > > + platform_set_drvdata(pdev, data);
> > > +
> > > + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> > > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > > +
> > > + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> > > +}
> > > +
> > > +static int rpi_hwmon_remove(struct platform_device *pdev)
> > > +{
> > > + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> > > +
> > > + cancel_delayed_work_sync(&data->get_values_poll_work);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct of_device_id rpi_hwmon_of_match[] = {
> > > + { .compatible = "raspberrypi,bcm2835-hwmon", },
> > > + { /* sentinel */},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> > > +
> > > +static struct platform_driver rpi_hwmon_driver = {
> > > + .probe = rpi_hwmon_probe,
> > > + .remove = rpi_hwmon_remove,
> > > + .driver = {
> > > + .name = "raspberrypi-hwmon",
> > > + .of_match_table = rpi_hwmon_of_match,
> > > + },
> > > +};
> > > +module_platform_driver(rpi_hwmon_driver);
> > > +
> > > +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> > > +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> > > +MODULE_LICENSE("GPL v2");
> > > --
> > > 2.7.4
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-17 16:43 ` Guenter Roeck
0 siblings, 0 replies; 50+ messages in thread
From: Guenter Roeck @ 2018-05-17 16:43 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, May 16, 2018 at 09:59:01PM +0200, Stefan Wahren wrote:
> Hi Guenter,
>
> > Guenter Roeck <linux@roeck-us.net> hat am 16. Mai 2018 um 20:21 geschrieben:
> >
> >
> > On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
> > > Currently there is no easy way to detect under-voltage conditions on a remote
> > > Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> > > via mailbox interface. The handling based on Noralf's modifications to the
> > > downstream firmware driver. In case of an under-voltage condition only an entry
> > > is written to the kernel log.
> > >
> >
> > My major concern is how this is displayed with the 'sensors' command.
> > Can you test and report ?
>
> I get the following output:
> rpi_volt-isa-0000
> Adapter: ISA adapter
> in0: N/A
>
Ok, that works.
> >
> > Of course, it would be much better if the firmware would also report
> > the actual voltage, but I guess we can't have everything.
>
> I think this isn't possible because the hardware only provide a binary value (GPIO).
>
> >
> > More comments inline.
> >
> > Thanks,
> > Guenter
> >
> > > CC: "Noralf Tr?nnes" <noralf@tronnes.org>
> > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > ---
> > > drivers/hwmon/Kconfig | 10 ++
> > > drivers/hwmon/Makefile | 1 +
> > > drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
> >
> > Please also provide Documentation/hwmon/raspberrypi-hwmon.
> >
> > > 3 files changed, 218 insertions(+)
> > > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> > >
> > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > index 768aed5..7f935cf 100644
> > > --- a/drivers/hwmon/Kconfig
> > > +++ b/drivers/hwmon/Kconfig
> > > @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> > > This driver can also be built as a module. If so, the module
> > > will be called pwm-fan.
> > >
> > > +config SENSORS_RASPBERRYPI_HWMON
> > > + tristate "Raspberry Pi voltage monitor"
> > > + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> > > + help
> > > + If you say yes here you get support for voltage sensor on the
> > > + Raspberry Pi.
> > > +
> > > + This driver can also be built as a module. If so, the module
> > > + will be called raspberrypi-hwmon.
> > > +
> > > config SENSORS_SHT15
> > > tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> > > depends on GPIOLIB || COMPILE_TEST
> > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > > index e7d52a3..a929770 100644
> > > --- a/drivers/hwmon/Makefile
> > > +++ b/drivers/hwmon/Makefile
> > > @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> > > +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> > > diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> > > new file mode 100644
> > > index 0000000..2003f6c
> > > --- /dev/null
> > > +++ b/drivers/hwmon/raspberrypi-hwmon.c
> > > @@ -0,0 +1,207 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Raspberry Pi voltage sensor driver
> > > + *
> > > + * Based on firmware/raspberrypi.c by Noralf Tr?nnes
> > > + *
> > > + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> > > + */
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/hwmon.h>
> > > +#include <linux/hwmon-sysfs.h>
> >
> > Unnecessary include
> >
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/workqueue.h>
> > > +#include <soc/bcm2835/raspberrypi-firmware.h>
> > > +
> > > +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> > > +
> > > +struct rpi_hwmon_data {
> > > + struct device *hwmon_dev;
> > > + struct rpi_firmware *fw;
> > > + u32 last_throttled;
> > > + struct delayed_work get_values_poll_work;
> > > +};
> > > +
> > > +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> > > +{
> > > + u32 new_uv, old_uv, value;
> > > + int ret;
> > > +
> > > + /* Clear sticky bits */
> >
> > Please make more explicit that this is a request/command sent to the firmware.
> >
> > > + value = 0xffff;
> > > +
> > > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > > + &value, sizeof(value));
> > > + if (ret) {
> > > + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> > > + __func__, ret);
> >
> > The function name seems unnecessary.
> >
> > > + return;
> > > + }
> > > +
> > > + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> > > + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> > > + data->last_throttled = value;
> > > +
> > > + if (new_uv == old_uv)
> > > + return;
> > > +
> > > + if (new_uv)
> > > + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> > > + value);
> > > + else
> > > + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> > > + value);
> >
> > What value do those hex values provide to the user ?
>
> The actual definition of the bits can be found in the commit log of patch #1. But this isn't very helpful for an end user.
>
I would suggest to drop the value from the log.
> >
> > > +
> > > + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> > > +}
> > > +
> > > +static void get_values_poll(struct work_struct *work)
> > > +{
> > > + struct rpi_hwmon_data *data;
> > > +
> > > + data = container_of(work, struct rpi_hwmon_data,
> > > + get_values_poll_work.work);
> > > +
> > > + rpi_firmware_get_throttled(data);
> > > +
> > > + /*
> > > + * We can't run faster than the sticky shift (100ms) since we get
> > > + * flipping in the sticky bits that are cleared.
> > > + */
> > > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > > +}
> > > +
> > > +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> > > + u32 attr, int channel, long *val)
> > > +{
> > > + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> > > +
> > > + if (type != hwmon_in)
> > > + return -EOPNOTSUPP;
> > > +
> > > + if (attr != hwmon_in_lcrit_alarm)
> > > + return -EOPNOTSUPP;
> > > +
> > > + if (channel)
> > > + return -EOPNOTSUPP;
> > > +
> > There is only one channel, one attribute, and one type supported.
> > As such, the checks are unnecessary.
> >
> > > + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> > > + return 0;
> > > +}
> > > +
> > > +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> > > + u32 attr, int channel)
> > > +{
> > > + if (type != hwmon_in)
> > > + return 0;
> > > +
> > > + if (attr != hwmon_in_lcrit_alarm)
> > > + return 0;
> > > +
> > > + if (channel)
> > > + return 0;
> >
> > Same as above. In the list below, there is not a single conditional attribute.
> > Given that, the checks should be unnecessary, and it should be sufficient to
> > just return 0444.
> >
> > > +
> > > + return 0444;
> > > +}
> > > +
> > > +static const u32 rpi_in_config[] = {
> > > + HWMON_I_LCRIT_ALARM,
> > > + 0
> > > +};
> > > +
> > > +static const struct hwmon_channel_info rpi_in = {
> > > + .type = hwmon_in,
> > > + .config = rpi_in_config,
> > > +};
> > > +
> > > +static const struct hwmon_channel_info *rpi_info[] = {
> > > + &rpi_in,
> > > + NULL
> > > +};
> > > +
> > > +static const struct hwmon_ops rpi_hwmon_ops = {
> > > + .is_visible = rpi_is_visible,
> > > + .read = rpi_read,
> > > +};
> > > +
> > > +static const struct hwmon_chip_info rpi_chip_info = {
> > > + .ops = &rpi_hwmon_ops,
> > > + .info = rpi_info,
> > > +};
> > > +
> > > +static int rpi_hwmon_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + struct device_node *fw_node;
> > > + struct rpi_hwmon_data *data;
> > > + int ret;
> > > +
> > > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > > + if (!data)
> > > + return -ENOMEM;
> > > +
> > > + fw_node = of_get_parent(dev->of_node);
> > > + if (!fw_node) {
> > > + dev_err(dev, "Missing firmware node\n");
> > > + return -ENOENT;
> > > + }
> > > +
> > > + data->fw = rpi_firmware_get(fw_node);
> > > + of_node_put(fw_node);
> > > + if (!data->fw)
> > > + return -EPROBE_DEFER;
> > > +
> > > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > > + &data->last_throttled,
> > > + sizeof(data->last_throttled));
> > > + if (ret) {
> > > + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
> >
> > If this is an error -> dev_err().
>
> I wasn't sure. If the firmware is too old, we cannot provide this feature and it's a waste of resources to load this driver. On the other side i don't want to confuse people this is something bad.
>
Then I would suggest to return -ENODEV.
Guenter
> Stefan
>
> >
> > > + return -EOPNOTSUPP;
> >
> > or return -ENODEV.
> >
> > > + }
> > > +
> > > + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> > > + data,
> > > + &rpi_chip_info,
> > > + NULL);
> > > +
> > > + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> > > + platform_set_drvdata(pdev, data);
> > > +
> > > + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> > > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > > +
> > > + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> > > +}
> > > +
> > > +static int rpi_hwmon_remove(struct platform_device *pdev)
> > > +{
> > > + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> > > +
> > > + cancel_delayed_work_sync(&data->get_values_poll_work);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct of_device_id rpi_hwmon_of_match[] = {
> > > + { .compatible = "raspberrypi,bcm2835-hwmon", },
> > > + { /* sentinel */},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> > > +
> > > +static struct platform_driver rpi_hwmon_driver = {
> > > + .probe = rpi_hwmon_probe,
> > > + .remove = rpi_hwmon_remove,
> > > + .driver = {
> > > + .name = "raspberrypi-hwmon",
> > > + .of_match_table = rpi_hwmon_of_match,
> > > + },
> > > +};
> > > +module_platform_driver(rpi_hwmon_driver);
> > > +
> > > +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> > > +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> > > +MODULE_LICENSE("GPL v2");
> > > --
> > > 2.7.4
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> > > the body of a message to majordomo at vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-17 16:43 ` Guenter Roeck
0 siblings, 0 replies; 50+ messages in thread
From: Guenter Roeck @ 2018-05-17 16:43 UTC (permalink / raw)
To: Stefan Wahren
Cc: Mark Rutland, devicetree, Jean Delvare, Florian Fainelli,
Scott Branden, Ray Jui, Phil Elwell, Rob Herring, Eric Anholt,
Noralf Trønnes, bcm-kernel-feedback-list, linux-rpi-kernel,
linux-hwmon, linux-arm-kernel
On Wed, May 16, 2018 at 09:59:01PM +0200, Stefan Wahren wrote:
> Hi Guenter,
>
> > Guenter Roeck <linux@roeck-us.net> hat am 16. Mai 2018 um 20:21 geschrieben:
> >
> >
> > On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
> > > Currently there is no easy way to detect under-voltage conditions on a remote
> > > Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
> > > via mailbox interface. The handling based on Noralf's modifications to the
> > > downstream firmware driver. In case of an under-voltage condition only an entry
> > > is written to the kernel log.
> > >
> >
> > My major concern is how this is displayed with the 'sensors' command.
> > Can you test and report ?
>
> I get the following output:
> rpi_volt-isa-0000
> Adapter: ISA adapter
> in0: N/A
>
Ok, that works.
> >
> > Of course, it would be much better if the firmware would also report
> > the actual voltage, but I guess we can't have everything.
>
> I think this isn't possible because the hardware only provide a binary value (GPIO).
>
> >
> > More comments inline.
> >
> > Thanks,
> > Guenter
> >
> > > CC: "Noralf Trønnes" <noralf@tronnes.org>
> > > Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> > > ---
> > > drivers/hwmon/Kconfig | 10 ++
> > > drivers/hwmon/Makefile | 1 +
> > > drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++
> >
> > Please also provide Documentation/hwmon/raspberrypi-hwmon.
> >
> > > 3 files changed, 218 insertions(+)
> > > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> > >
> > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > > index 768aed5..7f935cf 100644
> > > --- a/drivers/hwmon/Kconfig
> > > +++ b/drivers/hwmon/Kconfig
> > > @@ -1298,6 +1298,16 @@ config SENSORS_PWM_FAN
> > > This driver can also be built as a module. If so, the module
> > > will be called pwm-fan.
> > >
> > > +config SENSORS_RASPBERRYPI_HWMON
> > > + tristate "Raspberry Pi voltage monitor"
> > > + depends on (ARCH_BCM2835 && RASPBERRYPI_FIRMWARE) || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE)
> > > + help
> > > + If you say yes here you get support for voltage sensor on the
> > > + Raspberry Pi.
> > > +
> > > + This driver can also be built as a module. If so, the module
> > > + will be called raspberrypi-hwmon.
> > > +
> > > config SENSORS_SHT15
> > > tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
> > > depends on GPIOLIB || COMPILE_TEST
> > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > > index e7d52a3..a929770 100644
> > > --- a/drivers/hwmon/Makefile
> > > +++ b/drivers/hwmon/Makefile
> > > @@ -141,6 +141,7 @@ obj-$(CONFIG_SENSORS_PC87427) += pc87427.o
> > > obj-$(CONFIG_SENSORS_PCF8591) += pcf8591.o
> > > obj-$(CONFIG_SENSORS_POWR1220) += powr1220.o
> > > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o
> > > +obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o
> > > obj-$(CONFIG_SENSORS_S3C) += s3c-hwmon.o
> > > obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
> > > obj-$(CONFIG_SENSORS_SCH5627) += sch5627.o
> > > diff --git a/drivers/hwmon/raspberrypi-hwmon.c b/drivers/hwmon/raspberrypi-hwmon.c
> > > new file mode 100644
> > > index 0000000..2003f6c
> > > --- /dev/null
> > > +++ b/drivers/hwmon/raspberrypi-hwmon.c
> > > @@ -0,0 +1,207 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Raspberry Pi voltage sensor driver
> > > + *
> > > + * Based on firmware/raspberrypi.c by Noralf Trønnes
> > > + *
> > > + * Copyright (C) 2018 Stefan Wahren <stefan.wahren@i2se.com>
> > > + */
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/hwmon.h>
> > > +#include <linux/hwmon-sysfs.h>
> >
> > Unnecessary include
> >
> > > +#include <linux/module.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/workqueue.h>
> > > +#include <soc/bcm2835/raspberrypi-firmware.h>
> > > +
> > > +#define UNDERVOLTAGE_STICKY_BIT BIT(16)
> > > +
> > > +struct rpi_hwmon_data {
> > > + struct device *hwmon_dev;
> > > + struct rpi_firmware *fw;
> > > + u32 last_throttled;
> > > + struct delayed_work get_values_poll_work;
> > > +};
> > > +
> > > +static void rpi_firmware_get_throttled(struct rpi_hwmon_data *data)
> > > +{
> > > + u32 new_uv, old_uv, value;
> > > + int ret;
> > > +
> > > + /* Clear sticky bits */
> >
> > Please make more explicit that this is a request/command sent to the firmware.
> >
> > > + value = 0xffff;
> > > +
> > > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > > + &value, sizeof(value));
> > > + if (ret) {
> > > + dev_err_once(data->hwmon_dev, "%s: Failed to get throttled (%d)\n",
> > > + __func__, ret);
> >
> > The function name seems unnecessary.
> >
> > > + return;
> > > + }
> > > +
> > > + new_uv = value & UNDERVOLTAGE_STICKY_BIT;
> > > + old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
> > > + data->last_throttled = value;
> > > +
> > > + if (new_uv == old_uv)
> > > + return;
> > > +
> > > + if (new_uv)
> > > + dev_crit(data->hwmon_dev, "Under-voltage detected! (0x%08x)\n",
> > > + value);
> > > + else
> > > + dev_info(data->hwmon_dev, "Voltage normalised (0x%08x)\n",
> > > + value);
> >
> > What value do those hex values provide to the user ?
>
> The actual definition of the bits can be found in the commit log of patch #1. But this isn't very helpful for an end user.
>
I would suggest to drop the value from the log.
> >
> > > +
> > > + sysfs_notify(&data->hwmon_dev->kobj, NULL, "in0_lcrit_alarm");
> > > +}
> > > +
> > > +static void get_values_poll(struct work_struct *work)
> > > +{
> > > + struct rpi_hwmon_data *data;
> > > +
> > > + data = container_of(work, struct rpi_hwmon_data,
> > > + get_values_poll_work.work);
> > > +
> > > + rpi_firmware_get_throttled(data);
> > > +
> > > + /*
> > > + * We can't run faster than the sticky shift (100ms) since we get
> > > + * flipping in the sticky bits that are cleared.
> > > + */
> > > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > > +}
> > > +
> > > +static int rpi_read(struct device *dev, enum hwmon_sensor_types type,
> > > + u32 attr, int channel, long *val)
> > > +{
> > > + struct rpi_hwmon_data *data = dev_get_drvdata(dev);
> > > +
> > > + if (type != hwmon_in)
> > > + return -EOPNOTSUPP;
> > > +
> > > + if (attr != hwmon_in_lcrit_alarm)
> > > + return -EOPNOTSUPP;
> > > +
> > > + if (channel)
> > > + return -EOPNOTSUPP;
> > > +
> > There is only one channel, one attribute, and one type supported.
> > As such, the checks are unnecessary.
> >
> > > + *val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
> > > + return 0;
> > > +}
> > > +
> > > +static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,
> > > + u32 attr, int channel)
> > > +{
> > > + if (type != hwmon_in)
> > > + return 0;
> > > +
> > > + if (attr != hwmon_in_lcrit_alarm)
> > > + return 0;
> > > +
> > > + if (channel)
> > > + return 0;
> >
> > Same as above. In the list below, there is not a single conditional attribute.
> > Given that, the checks should be unnecessary, and it should be sufficient to
> > just return 0444.
> >
> > > +
> > > + return 0444;
> > > +}
> > > +
> > > +static const u32 rpi_in_config[] = {
> > > + HWMON_I_LCRIT_ALARM,
> > > + 0
> > > +};
> > > +
> > > +static const struct hwmon_channel_info rpi_in = {
> > > + .type = hwmon_in,
> > > + .config = rpi_in_config,
> > > +};
> > > +
> > > +static const struct hwmon_channel_info *rpi_info[] = {
> > > + &rpi_in,
> > > + NULL
> > > +};
> > > +
> > > +static const struct hwmon_ops rpi_hwmon_ops = {
> > > + .is_visible = rpi_is_visible,
> > > + .read = rpi_read,
> > > +};
> > > +
> > > +static const struct hwmon_chip_info rpi_chip_info = {
> > > + .ops = &rpi_hwmon_ops,
> > > + .info = rpi_info,
> > > +};
> > > +
> > > +static int rpi_hwmon_probe(struct platform_device *pdev)
> > > +{
> > > + struct device *dev = &pdev->dev;
> > > + struct device_node *fw_node;
> > > + struct rpi_hwmon_data *data;
> > > + int ret;
> > > +
> > > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > > + if (!data)
> > > + return -ENOMEM;
> > > +
> > > + fw_node = of_get_parent(dev->of_node);
> > > + if (!fw_node) {
> > > + dev_err(dev, "Missing firmware node\n");
> > > + return -ENOENT;
> > > + }
> > > +
> > > + data->fw = rpi_firmware_get(fw_node);
> > > + of_node_put(fw_node);
> > > + if (!data->fw)
> > > + return -EPROBE_DEFER;
> > > +
> > > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > > + &data->last_throttled,
> > > + sizeof(data->last_throttled));
> > > + if (ret) {
> > > + dev_info(dev, "Firmware doesn't support GET_THROTTLED\n");
> >
> > If this is an error -> dev_err().
>
> I wasn't sure. If the firmware is too old, we cannot provide this feature and it's a waste of resources to load this driver. On the other side i don't want to confuse people this is something bad.
>
Then I would suggest to return -ENODEV.
Guenter
> Stefan
>
> >
> > > + return -EOPNOTSUPP;
> >
> > or return -ENODEV.
> >
> > > + }
> > > +
> > > + data->hwmon_dev = devm_hwmon_device_register_with_info(dev, "rpi_volt",
> > > + data,
> > > + &rpi_chip_info,
> > > + NULL);
> > > +
> > > + INIT_DELAYED_WORK(&data->get_values_poll_work, get_values_poll);
> > > + platform_set_drvdata(pdev, data);
> > > +
> > > + if (!PTR_ERR_OR_ZERO(data->hwmon_dev))
> > > + schedule_delayed_work(&data->get_values_poll_work, 2 * HZ);
> > > +
> > > + return PTR_ERR_OR_ZERO(data->hwmon_dev);
> > > +}
> > > +
> > > +static int rpi_hwmon_remove(struct platform_device *pdev)
> > > +{
> > > + struct rpi_hwmon_data *data = platform_get_drvdata(pdev);
> > > +
> > > + cancel_delayed_work_sync(&data->get_values_poll_work);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static const struct of_device_id rpi_hwmon_of_match[] = {
> > > + { .compatible = "raspberrypi,bcm2835-hwmon", },
> > > + { /* sentinel */},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, rpi_hwmon_of_match);
> > > +
> > > +static struct platform_driver rpi_hwmon_driver = {
> > > + .probe = rpi_hwmon_probe,
> > > + .remove = rpi_hwmon_remove,
> > > + .driver = {
> > > + .name = "raspberrypi-hwmon",
> > > + .of_match_table = rpi_hwmon_of_match,
> > > + },
> > > +};
> > > +module_platform_driver(rpi_hwmon_driver);
> > > +
> > > +MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
> > > +MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
> > > +MODULE_LICENSE("GPL v2");
> > > --
> > > 2.7.4
> > >
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
2018-05-17 16:43 ` Guenter Roeck
(?)
@ 2018-05-17 17:25 ` Eric Anholt
-1 siblings, 0 replies; 50+ messages in thread
From: Eric Anholt @ 2018-05-17 17:25 UTC (permalink / raw)
To: Guenter Roeck, Stefan Wahren
Cc: Rob Herring, bcm-kernel-feedback-list, devicetree,
Noralf Trønnes, Ray Jui, Scott Branden, linux-rpi-kernel,
Florian Fainelli, Mark Rutland, Phil Elwell, linux-hwmon,
Jean Delvare, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 1090 bytes --]
Guenter Roeck <linux@roeck-us.net> writes:
> On Wed, May 16, 2018 at 09:59:01PM +0200, Stefan Wahren wrote:
>> Hi Guenter,
>>
>> > Guenter Roeck <linux@roeck-us.net> hat am 16. Mai 2018 um 20:21 geschrieben:
>> >
>> >
>> > On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
>> > > Currently there is no easy way to detect under-voltage conditions on a remote
>> > > Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
>> > > via mailbox interface. The handling based on Noralf's modifications to the
>> > > downstream firmware driver. In case of an under-voltage condition only an entry
>> > > is written to the kernel log.
>> > >
>> >
>> > My major concern is how this is displayed with the 'sensors' command.
>> > Can you test and report ?
>>
>> I get the following output:
>> rpi_volt-isa-0000
>> Adapter: ISA adapter
>> in0: N/A
>>
> Ok, that works.
Thanks for the quick review on this, Guenter! This driver is going to
help save a lot of people debugging time on rpis in the upstream kernel.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 50+ messages in thread
* [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-17 17:25 ` Eric Anholt
0 siblings, 0 replies; 50+ messages in thread
From: Eric Anholt @ 2018-05-17 17:25 UTC (permalink / raw)
To: linux-arm-kernel
Guenter Roeck <linux@roeck-us.net> writes:
> On Wed, May 16, 2018 at 09:59:01PM +0200, Stefan Wahren wrote:
>> Hi Guenter,
>>
>> > Guenter Roeck <linux@roeck-us.net> hat am 16. Mai 2018 um 20:21 geschrieben:
>> >
>> >
>> > On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
>> > > Currently there is no easy way to detect under-voltage conditions on a remote
>> > > Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
>> > > via mailbox interface. The handling based on Noralf's modifications to the
>> > > downstream firmware driver. In case of an under-voltage condition only an entry
>> > > is written to the kernel log.
>> > >
>> >
>> > My major concern is how this is displayed with the 'sensors' command.
>> > Can you test and report ?
>>
>> I get the following output:
>> rpi_volt-isa-0000
>> Adapter: ISA adapter
>> in0: N/A
>>
> Ok, that works.
Thanks for the quick review on this, Guenter! This driver is going to
help save a lot of people debugging time on rpis in the upstream kernel.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180517/f9ce6988/attachment.sig>
^ permalink raw reply [flat|nested] 50+ messages in thread
* Re: [PATCH RFC 3/6] hwmon: Add support for RPi voltage sensor
@ 2018-05-17 17:25 ` Eric Anholt
0 siblings, 0 replies; 50+ messages in thread
From: Eric Anholt @ 2018-05-17 17:25 UTC (permalink / raw)
To: Guenter Roeck, Stefan Wahren
Cc: Mark Rutland, devicetree, Jean Delvare, Noralf Trønnes,
Scott Branden, Ray Jui, Phil Elwell, Rob Herring,
bcm-kernel-feedback-list, linux-rpi-kernel, Florian Fainelli,
linux-hwmon, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 1090 bytes --]
Guenter Roeck <linux@roeck-us.net> writes:
> On Wed, May 16, 2018 at 09:59:01PM +0200, Stefan Wahren wrote:
>> Hi Guenter,
>>
>> > Guenter Roeck <linux@roeck-us.net> hat am 16. Mai 2018 um 20:21 geschrieben:
>> >
>> >
>> > On Wed, May 16, 2018 at 03:37:04PM +0200, Stefan Wahren wrote:
>> > > Currently there is no easy way to detect under-voltage conditions on a remote
>> > > Raspberry Pi. This hwmon driver retrieves the state of the under-voltage sensor
>> > > via mailbox interface. The handling based on Noralf's modifications to the
>> > > downstream firmware driver. In case of an under-voltage condition only an entry
>> > > is written to the kernel log.
>> > >
>> >
>> > My major concern is how this is displayed with the 'sensors' command.
>> > Can you test and report ?
>>
>> I get the following output:
>> rpi_volt-isa-0000
>> Adapter: ISA adapter
>> in0: N/A
>>
> Ok, that works.
Thanks for the quick review on this, Guenter! This driver is going to
help save a lot of people debugging time on rpis in the upstream kernel.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 50+ messages in thread