hwmon: Add support for Raspberry Pi voltage sensor

Recent VC4 firmware provides a hardware-independent way to retrieve the under-voltage sensor on the following Raspberry Pi boards:
- Raspberry Pi B+ (via GPIO on SoC)
- Raspberry Pi A+ (via GPIO on SoC)
- Raspberry Pi 2 B (via GPIO on SoC)
- Raspberry Pi 3 B (via GPIO on port expander)
- Raspberry Pi 3 B+ (via PMIC)

TODO:
- try to make bcm2835 firmware a bus driver

Signed-off-by Stefan Wahren <[email protected]>

---
  .../devicetree/bindings/hwmon/raspberrypi-hwmon.txt   | 19 +++++++++++++++++++
  1 file changed, 19 insertions(+)

create mode 100644 Documentation/devicetree/bindings/hwmon/raspberrypi-hwmon.txt

Documentation/devicetree/bindings/hwmon/raspberrypi-hwmon.txt new file +19 -0

new file mode 100644
index 0000000..ec2523f
@@ -0,0 +1,19 @@
+Raspberry Pi voltage sensor
+---------------------------
+
+The VC4 firmware exposes a mailbox interface that allows the ARM core
+to access the board's voltage sensor.
+
+Required properties :
+- compatible : Should be "raspberrypi,bcm2835-hwmon"
+
+Example:
+
+firmware: firmware-rpi {
+	compatible = "raspberrypi,bcm2835-firmware";
+	mboxes = <&mailbox>;
+
+	voltage-sensor {
+		compatible = "raspberrypi,bcm2835-hwmon";
+	};
+};

Quoted 8 lines · Stefan Wahren

Recent VC4 firmware provides a hardware-independent way to retrieve the under-voltage sensor on the following Raspberry Pi boards:
- Raspberry Pi B+ (via GPIO on SoC)
- Raspberry Pi A+ (via GPIO on SoC)
- Raspberry Pi 2 B (via GPIO on SoC)
- Raspberry Pi 3 B (via GPIO on port expander)
- Raspberry Pi 3 B+ (via PMIC)

Quoted 2 lines

TODO:
- try to make bcm2835 firmware a bus driver

Do you actually need a DT binding? From the probe routine in patch 2 it looks like this capability can be discovered dynamically by asking the firmware, therefore it might be more straightforward for the firmware driver itself to probe that and create a child device iff the feature is present, which the hwmon driver can then bind to as a pure platform driver.

Recent VC4 firmware provides a hardware-independent way to retrieve the under-voltage sensor on the following Raspberry Pi boards:
- Raspberry Pi B+ (via GPIO on SoC)
- Raspberry Pi A+ (via GPIO on SoC)
- Raspberry Pi 2 B (via GPIO on SoC)
- Raspberry Pi 3 B (via GPIO on port expander)
- Raspberry Pi 3 B+ (via PMIC)

TODO:
- try to make bcm2835 firmware a bus driver

Signed-off-by Stefan Wahren <[email protected]>

---
 .../devicetree/bindings/hwmon/raspberrypi-hwmon.txt   | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/raspberrypi-hwmon.txt

Documentation/devicetree/bindings/hwmon/raspberrypi-hwmon.txt new file +0 -0

new file mode 100644
index 0000000..ec2523f

Documentation/devicetree/bindings/hwmon/raspberrypi-hwmon.txt new file +11 -0

+
+Example:
+
+firmware: firmware-rpi {
+	compatible = "raspberrypi,bcm2835-firmware";
+	mboxes = <&mailbox>;
+
+	voltage-sensor {
+		compatible = "raspberrypi,bcm2835-hwmon";
+	};
+};

Signature · 6 lines

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

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" <[email protected]>

Signed-off-by Stefan Wahren <[email protected]>

---
  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

drivers/hwmon/Kconfig +0 -0

index 768aed5..7f935cf 100644

drivers/hwmon/Kconfig +7 -0

+	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

drivers/hwmon/Makefile +1 -0

index e7d52a3..a929770 100644
@@ -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

drivers/hwmon/raspberrypi-hwmon.c new file +207 -0

new file mode 100644
index 0000000..2003f6c
@@ -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 <[email protected]>
+ */
+#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 <[email protected]>");
+MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
+MODULE_LICENSE("GPL v2");

Hi Stefan,

Quoted 20 lines · Stefan Wahren

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" <[email protected]>

Signed-off-by Stefan Wahren <[email protected]>

---
 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

drivers/hwmon/Kconfig +0 -0

index 768aed5..7f935cf 100644

drivers/hwmon/Kconfig config SENSORS_PWM_FAN · line 1302

Quoted 5 lines

drivers/hwmon/Kconfig +3 -0

@@ -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.

Quoted 226 lines

drivers/hwmon/Kconfig +7 -0

+	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

drivers/hwmon/Makefile +1 -0

index e7d52a3..a929770 100644
@@ -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

drivers/hwmon/raspberrypi-hwmon.c new file +197 -0

new file mode 100644
index 0000000..2003f6c
@@ -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 <[email protected]>
+ */
+#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,

drivers/hwmon/raspberrypi-hwmon.c line 207

Quoted 10 lines

drivers/hwmon/raspberrypi-hwmon.c new file +10 -0

+	.driver = {
+		.name = "raspberrypi-hwmon",
+		.of_match_table = rpi_hwmon_of_match,
+	},
+};
+module_platform_driver(rpi_hwmon_driver);
+
+MODULE_AUTHOR("Stefan Wahren <[email protected]>");
+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

Quoted 32 lines · Robin Murphy

Hi Stefan,

Quoted 20 lines · Stefan Wahren

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" <[email protected]>

Signed-off-by Stefan Wahren <[email protected]>

---
  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

drivers/hwmon/Kconfig +0 -0

index 768aed5..7f935cf 100644

drivers/hwmon/Kconfig config SENSORS_PWM_FAN · line 1302

Quoted 5 lines

drivers/hwmon/Kconfig +3 -0

@@ -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:

Quoted 2 lines

	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().

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" <[email protected]>

Signed-off-by Stefan Wahren <[email protected]>

---
 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

drivers/hwmon/Kconfig +10 -0

index 768aed5..7f935cf 100644
@@ -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

drivers/hwmon/Makefile +1 -0

index e7d52a3..a929770 100644
@@ -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

drivers/hwmon/raspberrypi-hwmon.c new file +2 -0

new file mode 100644
index 0000000..2003f6c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*

drivers/hwmon/raspberrypi-hwmon.c new file +12 -0

+#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;

drivers/hwmon/raspberrypi-hwmon.c new file +6 -0

+		return;
+	}
+
+	new_uv = value & UNDERVOLTAGE_STICKY_BIT;
+	old_uv = data->last_throttled & UNDERVOLTAGE_STICKY_BIT;
+	data->last_throttled = value;

drivers/hwmon/raspberrypi-hwmon.c new file +24 -0

+
+	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);

drivers/hwmon/raspberrypi-hwmon.c new file +5 -0

+	*val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
+	return 0;
+}
+
+static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,

drivers/hwmon/raspberrypi-hwmon.c new file +46 -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;
+	}
+

drivers/hwmon/raspberrypi-hwmon.c new file +44 -0

+	}
+
+	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 <[email protected]>");
+MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
+MODULE_LICENSE("GPL v2");

Signature · 6 lines

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

Guenter Roeck <[email protected]> hat am 16. Mai 2018 um 20:21 geschrieben:

Quoted 6 lines · Stefan Wahren

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

Quoted 6 lines

CC "Noralf Tr?nnes" <[email protected]>

Signed-off-by Stefan Wahren <[email protected]>

---
 drivers/hwmon/Kconfig             |  10 ++
 drivers/hwmon/Makefile            |   1 +
 drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++

Please also provide Documentation/hwmon/raspberrypi-hwmon.

Quoted 44 lines

 3 files changed, 218 insertions(+)
 create mode 100644 drivers/hwmon/raspberrypi-hwmon.c

drivers/hwmon/Kconfig +10 -0

index 768aed5..7f935cf 100644
@@ -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

drivers/hwmon/Makefile +1 -0

index e7d52a3..a929770 100644
@@ -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

drivers/hwmon/raspberrypi-hwmon.c new file +2 -0

new file mode 100644
index 0000000..2003f6c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*

drivers/hwmon/raspberrypi-hwmon.c line 12

Quoted 10 lines

drivers/hwmon/raspberrypi-hwmon.c new file +10 -0

+ * Raspberry Pi voltage sensor driver
+ *
+ * Based on firmware/raspberrypi.c by Noralf Tr?nnes
+ *
+ * Copyright (C) 2018 Stefan Wahren <[email protected]>
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>

Unnecessary include

Quoted 12 lines

drivers/hwmon/raspberrypi-hwmon.c new file +12 -0

+#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;

drivers/hwmon/raspberrypi-hwmon.c line 34

Quoted 10 lines

drivers/hwmon/raspberrypi-hwmon.c new file +10 -0

+	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.

drivers/hwmon/raspberrypi-hwmon.c line 41

Quoted 7 lines

drivers/hwmon/raspberrypi-hwmon.c new file +7 -0

+	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.

Quoted 10 lines

drivers/hwmon/raspberrypi-hwmon.c new file +10 -0

+		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;
+

drivers/hwmon/raspberrypi-hwmon.c line 57

Quoted 6 lines

drivers/hwmon/raspberrypi-hwmon.c new file +6 -0

+	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 ?

Quoted 25 lines

drivers/hwmon/raspberrypi-hwmon.c new file +24 -0

+
+	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);

drivers/hwmon/raspberrypi-hwmon.c line 91

Quoted 10 lines

drivers/hwmon/raspberrypi-hwmon.c new file +10 -0

+
+	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.

Quoted 5 lines

drivers/hwmon/raspberrypi-hwmon.c new file +5 -0

+	*val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
+	return 0;
+}
+
+static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,

drivers/hwmon/raspberrypi-hwmon.c line 106

Quoted 10 lines

drivers/hwmon/raspberrypi-hwmon.c new file +10 -0

+			      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.

Quoted 50 lines

drivers/hwmon/raspberrypi-hwmon.c new file +50 -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;

drivers/hwmon/raspberrypi-hwmon.c line 162

Quoted 6 lines

drivers/hwmon/raspberrypi-hwmon.c new file +6 -0

+
+	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().

drivers/hwmon/raspberrypi-hwmon.c line 163

Quoted 2 lines

drivers/hwmon/raspberrypi-hwmon.c new file +1 -0

+		return -EOPNOTSUPP;

or return -ENODEV.

Quoted 51 lines

drivers/hwmon/raspberrypi-hwmon.c new file +44 -0

+	}
+
+	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 <[email protected]>");
+MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
+MODULE_LICENSE("GPL v2");

Signature · 6 lines

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

Hi Guenter,

Quoted 11 lines

Guenter Roeck <[email protected]> hat am 16. Mai 2018 um 20:21 geschrieben:

Quoted 6 lines · Stefan Wahren

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.

Quoted 2 lines

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

Quoted 3 lines

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).

Quoted 117 lines

More comments inline.

Thanks,
Guenter

Quoted 6 lines

CC "Noralf Tr?nnes" <[email protected]>

Signed-off-by Stefan Wahren <[email protected]>

---
 drivers/hwmon/Kconfig             |  10 ++
 drivers/hwmon/Makefile            |   1 +
 drivers/hwmon/raspberrypi-hwmon.c | 207 ++++++++++++++++++++++++++++++++++++++

Please also provide Documentation/hwmon/raspberrypi-hwmon.

Quoted 44 lines

 3 files changed, 218 insertions(+)
 create mode 100644 drivers/hwmon/raspberrypi-hwmon.c

drivers/hwmon/Kconfig +10 -0

index 768aed5..7f935cf 100644
@@ -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

drivers/hwmon/Makefile +1 -0

index e7d52a3..a929770 100644
@@ -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

drivers/hwmon/raspberrypi-hwmon.c new file +2 -0

new file mode 100644
index 0000000..2003f6c
@@ -0,0 +1,207 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*

drivers/hwmon/raspberrypi-hwmon.c line 12

Quoted 10 lines

drivers/hwmon/raspberrypi-hwmon.c new file +10 -0

+ * Raspberry Pi voltage sensor driver
+ *
+ * Based on firmware/raspberrypi.c by Noralf Tr?nnes
+ *
+ * Copyright (C) 2018 Stefan Wahren <[email protected]>
+ */
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>

Unnecessary include

Quoted 12 lines

drivers/hwmon/raspberrypi-hwmon.c new file +12 -0

+#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;

drivers/hwmon/raspberrypi-hwmon.c line 34

Quoted 10 lines

drivers/hwmon/raspberrypi-hwmon.c new file +10 -0

+	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.

drivers/hwmon/raspberrypi-hwmon.c line 41

Quoted 7 lines

drivers/hwmon/raspberrypi-hwmon.c new file +7 -0

+	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.

Quoted 10 lines

drivers/hwmon/raspberrypi-hwmon.c new file +10 -0

+		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;
+

Quoted 8 lines

drivers/hwmon/raspberrypi-hwmon.c line 57

Quoted 6 lines

drivers/hwmon/raspberrypi-hwmon.c new file +6 -0

+	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.

Quoted 108 lines

Quoted 25 lines

drivers/hwmon/raspberrypi-hwmon.c new file +24 -0

+
+	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);

drivers/hwmon/raspberrypi-hwmon.c line 91

Quoted 10 lines

drivers/hwmon/raspberrypi-hwmon.c new file +10 -0

+
+	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.

Quoted 5 lines

drivers/hwmon/raspberrypi-hwmon.c new file +5 -0

+	*val = !!(data->last_throttled & UNDERVOLTAGE_STICKY_BIT);
+	return 0;
+}
+
+static umode_t rpi_is_visible(const void *_data, enum hwmon_sensor_types type,

drivers/hwmon/raspberrypi-hwmon.c line 106

Quoted 10 lines

drivers/hwmon/raspberrypi-hwmon.c new file +10 -0

+			      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.

Quoted 50 lines

drivers/hwmon/raspberrypi-hwmon.c new file +50 -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;

Quoted 8 lines

drivers/hwmon/raspberrypi-hwmon.c line 162

Quoted 6 lines

drivers/hwmon/raspberrypi-hwmon.c new file +6 -0

+
+	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

Quoted 61 lines

drivers/hwmon/raspberrypi-hwmon.c line 163

Quoted 2 lines

drivers/hwmon/raspberrypi-hwmon.c new file +1 -0

+		return -EOPNOTSUPP;

or return -ENODEV.

Quoted 51 lines

drivers/hwmon/raspberrypi-hwmon.c new file +44 -0

+	}
+
+	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 <[email protected]>");
+MODULE_DESCRIPTION("Raspberry Pi voltage sensor driver");
+MODULE_LICENSE("GPL v2");

Signature · 6 lines

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

Quoted 16 lines · Stefan Wahren

Hi Guenter,

Quoted 13 lines

Guenter Roeck <[email protected]> hat am 16. Mai 2018 um 20:21 geschrieben:

Quoted 6 lines · Stefan Wahren

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 ?

Quoted 5 lines

I get the following output:
rpi_volt-isa-0000
Adapter: ISA adapter
in0: N/A

Ok, that works.