* [PATCH RFC V2 0/6] hwmon: Add support for Raspberry Pi voltage sensor
@ 2018-05-22 11:21 Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 1/6] ARM: bcm2835: Add GET_THROTTLED firmware property Stefan Wahren
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: Stefan Wahren @ 2018-05-22 11:21 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Jean Delvare, Guenter Roeck,
Jonathan Corbet, Eric Anholt
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Stefan Wahren, Phil Elwell,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
A common issue for the Raspberry Pi is an inadequate power supply.
Noralf Trønnes started a discussion [1] about writing such undervoltage
conditions into the kernel log.
This series is a draft to upstream the resulting kernel patch and is not
intended for 4.18.
Changes in V2:
- simplified Kconfig dependency suggested by Robin Murphy
- replace dt-binding by probing from firmware driver
- add hwmon documentation
- minor improvements suggested by Guenter Roeck
[1] - https://github.com/raspberrypi/linux/issues/2367
Stefan Wahren (6):
ARM: bcm2835: Add GET_THROTTLED firmware property
hwmon: Add support for RPi voltage sensor
firmware: raspberrypi: Register hwmon driver
ARM: bcm2835_defconfig: Enable RPi voltage sensor
ARM: multi_v7_defconfig: Enable RPi voltage sensor
arm64: defconfig: Enable RPi voltage sensor
Documentation/hwmon/raspberrypi-hwmon | 22 ++++
arch/arm/configs/bcm2835_defconfig | 2 +-
arch/arm/configs/multi_v7_defconfig | 1 +
arch/arm64/configs/defconfig | 1 +
drivers/firmware/raspberrypi.c | 19 ++++
drivers/hwmon/Kconfig | 10 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/raspberrypi-hwmon.c | 168 +++++++++++++++++++++++++++++
include/soc/bcm2835/raspberrypi-firmware.h | 1 +
9 files changed, 224 insertions(+), 1 deletion(-)
create mode 100644 Documentation/hwmon/raspberrypi-hwmon
create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
--
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 [flat|nested] 13+ messages in thread
* [PATCH RFC V2 1/6] ARM: bcm2835: Add GET_THROTTLED firmware property
2018-05-22 11:21 [PATCH RFC V2 0/6] hwmon: Add support for Raspberry Pi voltage sensor Stefan Wahren
@ 2018-05-22 11:21 ` Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor Stefan Wahren
` (4 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Stefan Wahren @ 2018-05-22 11:21 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Jean Delvare, Guenter Roeck,
Jonathan Corbet, Eric Anholt
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Stefan Wahren, Phil Elwell,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
Recent Raspberry Pi firmware provides a mailbox property to detect
under-voltage conditions. Here is the current definition.
The u32 value returned by the firmware is divided into 2 parts:
- lower 16-bits are the live value
- upper 16-bits are the history or sticky value
Bits:
0: undervoltage
1: arm frequency capped
2: currently throttled
16: undervoltage has occurred
17: arm frequency capped has occurred
18: throttling has occurred
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
include/soc/bcm2835/raspberrypi-firmware.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
index 8ee8991..c4a5c9e 100644
--- a/include/soc/bcm2835/raspberrypi-firmware.h
+++ b/include/soc/bcm2835/raspberrypi-firmware.h
@@ -75,6 +75,7 @@ enum rpi_firmware_property_tag {
RPI_FIRMWARE_GET_EDID_BLOCK = 0x00030020,
RPI_FIRMWARE_GET_CUSTOMER_OTP = 0x00030021,
RPI_FIRMWARE_GET_DOMAIN_STATE = 0x00030030,
+ RPI_FIRMWARE_GET_THROTTLED = 0x00030046,
RPI_FIRMWARE_SET_CLOCK_STATE = 0x00038001,
RPI_FIRMWARE_SET_CLOCK_RATE = 0x00038002,
RPI_FIRMWARE_SET_VOLTAGE = 0x00038003,
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor
2018-05-22 11:21 [PATCH RFC V2 0/6] hwmon: Add support for Raspberry Pi voltage sensor Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 1/6] ARM: bcm2835: Add GET_THROTTLED firmware property Stefan Wahren
@ 2018-05-22 11:21 ` Stefan Wahren
2018-05-22 13:41 ` Guenter Roeck
2018-05-22 11:21 ` [PATCH RFC V2 3/6] firmware: raspberrypi: Register hwmon driver Stefan Wahren
` (3 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Stefan Wahren @ 2018-05-22 11:21 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Jean Delvare, Guenter Roeck,
Jonathan Corbet, Eric Anholt
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, 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 undervoltage conditions on a
remote Raspberry Pi. This hwmon driver retrieves the state of the
undervoltage sensor via mailbox interface. The handling based on
Noralf's modifications to the downstream firmware driver. In case of
an undervoltage 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>
---
Documentation/hwmon/raspberrypi-hwmon | 22 +++++
drivers/hwmon/Kconfig | 10 ++
drivers/hwmon/Makefile | 1 +
drivers/hwmon/raspberrypi-hwmon.c | 168 ++++++++++++++++++++++++++++++++++
4 files changed, 201 insertions(+)
create mode 100644 Documentation/hwmon/raspberrypi-hwmon
create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
diff --git a/Documentation/hwmon/raspberrypi-hwmon b/Documentation/hwmon/raspberrypi-hwmon
new file mode 100644
index 0000000..3c92e2c
--- /dev/null
+++ b/Documentation/hwmon/raspberrypi-hwmon
@@ -0,0 +1,22 @@
+Kernel driver raspberrypi-hwmon
+===============================
+
+Supported boards:
+ * Raspberry Pi A+ (via GPIO on SoC)
+ * Raspberry Pi B+ (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)
+
+Author: Stefan Wahren <stefan.wahren@i2se.com>
+
+Description
+-----------
+
+This driver periodically polls a mailbox property of the VC4 firmware to detect
+undervoltage conditions.
+
+Sysfs entries
+-------------
+
+in0_lcrit_alarm Undervoltage alarm
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 768aed5..9a5bdb0 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 RASPBERRYPI_FIRMWARE || COMPILE_TEST
+ 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..6233e84
--- /dev/null
+++ b/drivers/hwmon/raspberrypi-hwmon.c
@@ -0,0 +1,168 @@
+// 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/module.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;
+
+ /* Request firmware to 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, "Failed to get throttled (%d)\n",
+ 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, "Undervoltage detected!\n");
+ else
+ dev_info(data->hwmon_dev, "Voltage normalised\n");
+
+ 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);
+
+ *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)
+{
+ 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 rpi_hwmon_data *data;
+ int ret;
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->fw = platform_get_drvdata(to_platform_device(dev->parent));
+ 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)
+ 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 struct platform_driver rpi_hwmon_driver = {
+ .probe = rpi_hwmon_probe,
+ .remove = rpi_hwmon_remove,
+ .driver = {
+ .name = "raspberrypi-hwmon",
+ },
+};
+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] 13+ messages in thread
* [PATCH RFC V2 3/6] firmware: raspberrypi: Register hwmon driver
2018-05-22 11:21 [PATCH RFC V2 0/6] hwmon: Add support for Raspberry Pi voltage sensor Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 1/6] ARM: bcm2835: Add GET_THROTTLED firmware property Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor Stefan Wahren
@ 2018-05-22 11:21 ` Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 4/6] ARM: bcm2835_defconfig: Enable RPi voltage sensor Stefan Wahren
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: Stefan Wahren @ 2018-05-22 11:21 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Jean Delvare, Guenter Roeck,
Jonathan Corbet, Eric Anholt
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Stefan Wahren, Phil Elwell,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
Since the raspberrypi-hwmon driver is tied to the VC4 firmware instead of
particular hardware its registration should be in the firmware driver.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
drivers/firmware/raspberrypi.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
index 6692888f..0602626 100644
--- a/drivers/firmware/raspberrypi.c
+++ b/drivers/firmware/raspberrypi.c
@@ -21,6 +21,8 @@
#define MBOX_DATA28(msg) ((msg) & ~0xf)
#define MBOX_CHAN_PROPERTY 8
+static struct platform_device *rpi_hwmon;
+
struct rpi_firmware {
struct mbox_client cl;
struct mbox_chan *chan; /* The property channel. */
@@ -183,6 +185,20 @@ rpi_firmware_print_firmware_revision(struct rpi_firmware *fw)
}
}
+static void
+rpi_register_hwmon_driver(struct device *dev, struct rpi_firmware *fw)
+{
+ u32 packet;
+ int ret = rpi_firmware_property(fw, RPI_FIRMWARE_GET_THROTTLED,
+ &packet, sizeof(packet));
+
+ if (ret)
+ return;
+
+ rpi_hwmon = platform_device_register_data(dev, "raspberrypi-hwmon",
+ -1, NULL, 0);
+}
+
static int rpi_firmware_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -209,6 +225,7 @@ static int rpi_firmware_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, fw);
rpi_firmware_print_firmware_revision(fw);
+ rpi_register_hwmon_driver(dev, fw);
return 0;
}
@@ -217,6 +234,8 @@ static int rpi_firmware_remove(struct platform_device *pdev)
{
struct rpi_firmware *fw = platform_get_drvdata(pdev);
+ platform_device_unregister(rpi_hwmon);
+ rpi_hwmon = NULL;
mbox_free_channel(fw->chan);
return 0;
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH RFC V2 4/6] ARM: bcm2835_defconfig: Enable RPi voltage sensor
2018-05-22 11:21 [PATCH RFC V2 0/6] hwmon: Add support for Raspberry Pi voltage sensor Stefan Wahren
` (2 preceding siblings ...)
2018-05-22 11:21 ` [PATCH RFC V2 3/6] firmware: raspberrypi: Register hwmon driver Stefan Wahren
@ 2018-05-22 11:21 ` Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 5/6] ARM: multi_v7_defconfig: " Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 6/6] arm64: defconfig: " Stefan Wahren
5 siblings, 0 replies; 13+ messages in thread
From: Stefan Wahren @ 2018-05-22 11:21 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Jean Delvare, Guenter Roeck,
Jonathan Corbet, Eric Anholt
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Stefan Wahren, Phil Elwell,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
The patch enables the hwmon driver for the Raspberry Pi.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
arch/arm/configs/bcm2835_defconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/configs/bcm2835_defconfig b/arch/arm/configs/bcm2835_defconfig
index e4d188f..e9bc889 100644
--- a/arch/arm/configs/bcm2835_defconfig
+++ b/arch/arm/configs/bcm2835_defconfig
@@ -86,7 +86,7 @@ CONFIG_SPI=y
CONFIG_SPI_BCM2835=y
CONFIG_SPI_BCM2835AUX=y
CONFIG_GPIO_SYSFS=y
-# CONFIG_HWMON is not set
+CONFIG_SENSORS_RASPBERRYPI_HWMON=m
CONFIG_THERMAL=y
CONFIG_BCM2835_THERMAL=y
CONFIG_WATCHDOG=y
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH RFC V2 5/6] ARM: multi_v7_defconfig: Enable RPi voltage sensor
2018-05-22 11:21 [PATCH RFC V2 0/6] hwmon: Add support for Raspberry Pi voltage sensor Stefan Wahren
` (3 preceding siblings ...)
2018-05-22 11:21 ` [PATCH RFC V2 4/6] ARM: bcm2835_defconfig: Enable RPi voltage sensor Stefan Wahren
@ 2018-05-22 11:21 ` Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 6/6] arm64: defconfig: " Stefan Wahren
5 siblings, 0 replies; 13+ messages in thread
From: Stefan Wahren @ 2018-05-22 11:21 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Jean Delvare, Guenter Roeck,
Jonathan Corbet, Eric Anholt
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Stefan Wahren, Phil Elwell,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
The patch enables the hwmon driver for the Raspberry Pi.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
arch/arm/configs/multi_v7_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 720461b..5c9dc00 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -477,6 +477,7 @@ CONFIG_SENSORS_LM90=y
CONFIG_SENSORS_LM95245=y
CONFIG_SENSORS_NTC_THERMISTOR=m
CONFIG_SENSORS_PWM_FAN=m
+CONFIG_SENSORS_RASPBERRYPI_HWMON=m
CONFIG_SENSORS_INA2XX=m
CONFIG_CPU_THERMAL=y
CONFIG_BCM2835_THERMAL=m
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH RFC V2 6/6] arm64: defconfig: Enable RPi voltage sensor
2018-05-22 11:21 [PATCH RFC V2 0/6] hwmon: Add support for Raspberry Pi voltage sensor Stefan Wahren
` (4 preceding siblings ...)
2018-05-22 11:21 ` [PATCH RFC V2 5/6] ARM: multi_v7_defconfig: " Stefan Wahren
@ 2018-05-22 11:21 ` Stefan Wahren
5 siblings, 0 replies; 13+ messages in thread
From: Stefan Wahren @ 2018-05-22 11:21 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, Jean Delvare, Guenter Roeck,
Jonathan Corbet, Eric Anholt
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Stefan Wahren, Phil Elwell,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
The patch enables the hwmon driver for the Raspberry Pi.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
arch/arm64/configs/defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index d25121b..5cdecef 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -352,6 +352,7 @@ CONFIG_BATTERY_BQ27XXX=y
CONFIG_SENSORS_ARM_SCPI=y
CONFIG_SENSORS_LM90=m
CONFIG_SENSORS_INA2XX=m
+CONFIG_SENSORS_RASPBERRYPI_HWMON=m
CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y
CONFIG_CPU_THERMAL=y
CONFIG_THERMAL_EMULATION=y
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor
2018-05-22 11:21 ` [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor Stefan Wahren
@ 2018-05-22 13:41 ` Guenter Roeck
2018-05-22 13:51 ` Stefan Wahren
0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2018-05-22 13:41 UTC (permalink / raw)
To: Stefan Wahren, Rob Herring, Mark Rutland, Jean Delvare,
Jonathan Corbet, Eric Anholt
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Phil Elwell, Noralf Trønnes,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
On 05/22/2018 04:21 AM, Stefan Wahren wrote:
> Currently there is no easy way to detect undervoltage conditions on a
> remote Raspberry Pi. This hwmon driver retrieves the state of the
> undervoltage sensor via mailbox interface. The handling based on
> Noralf's modifications to the downstream firmware driver. In case of
> an undervoltage 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>
> ---
> Documentation/hwmon/raspberrypi-hwmon | 22 +++++
> drivers/hwmon/Kconfig | 10 ++
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/raspberrypi-hwmon.c | 168 ++++++++++++++++++++++++++++++++++
> 4 files changed, 201 insertions(+)
> create mode 100644 Documentation/hwmon/raspberrypi-hwmon
> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>
> diff --git a/Documentation/hwmon/raspberrypi-hwmon b/Documentation/hwmon/raspberrypi-hwmon
> new file mode 100644
> index 0000000..3c92e2c
> --- /dev/null
> +++ b/Documentation/hwmon/raspberrypi-hwmon
> @@ -0,0 +1,22 @@
> +Kernel driver raspberrypi-hwmon
> +===============================
> +
> +Supported boards:
> + * Raspberry Pi A+ (via GPIO on SoC)
> + * Raspberry Pi B+ (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)
> +
> +Author: Stefan Wahren <stefan.wahren@i2se.com>
> +
> +Description
> +-----------
> +
> +This driver periodically polls a mailbox property of the VC4 firmware to detect
> +undervoltage conditions.
> +
> +Sysfs entries
> +-------------
> +
> +in0_lcrit_alarm Undervoltage alarm
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 768aed5..9a5bdb0 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 RASPBERRYPI_FIRMWARE || COMPILE_TEST
> + 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..6233e84
> --- /dev/null
> +++ b/drivers/hwmon/raspberrypi-hwmon.c
> @@ -0,0 +1,168 @@
> +// 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/module.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;
> +
> + /* Request firmware to 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, "Failed to get throttled (%d)\n",
> + 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, "Undervoltage detected!\n");
> + else
> + dev_info(data->hwmon_dev, "Voltage normalised\n");
> +
> + 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);
> +
> + *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)
> +{
> + 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 rpi_hwmon_data *data;
> + int ret;
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->fw = platform_get_drvdata(to_platform_device(dev->parent));
> + if (!data->fw)
> + return -EPROBE_DEFER;
> +
I am a bit at loss here (and sorry I didn't bring this up before).
How would this ever be possible, given that the driver is registered
from the firmware driver ?
> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> + &data->last_throttled,
> + sizeof(data->last_throttled));
> + if (ret)
> + 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 struct platform_driver rpi_hwmon_driver = {
> + .probe = rpi_hwmon_probe,
> + .remove = rpi_hwmon_remove,
> + .driver = {
> + .name = "raspberrypi-hwmon",
> + },
> +};
> +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] 13+ messages in thread
* Re: [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor
2018-05-22 13:41 ` Guenter Roeck
@ 2018-05-22 13:51 ` Stefan Wahren
2018-05-22 14:10 ` Guenter Roeck
0 siblings, 1 reply; 13+ messages in thread
From: Stefan Wahren @ 2018-05-22 13:51 UTC (permalink / raw)
To: Rob Herring, Guenter Roeck, Eric Anholt, Mark Rutland,
Jonathan Corbet, Jean Delvare
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Phil Elwell, Noralf Trønnes,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
Hi Guenter,
> Guenter Roeck <linux@roeck-us.net> hat am 22. Mai 2018 um 15:41 geschrieben:
>
>
> On 05/22/2018 04:21 AM, Stefan Wahren wrote:
> > Currently there is no easy way to detect undervoltage conditions on a
> > remote Raspberry Pi. This hwmon driver retrieves the state of the
> > undervoltage sensor via mailbox interface. The handling based on
> > Noralf's modifications to the downstream firmware driver. In case of
> > an undervoltage 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>
> > ---
> > Documentation/hwmon/raspberrypi-hwmon | 22 +++++
> > drivers/hwmon/Kconfig | 10 ++
> > drivers/hwmon/Makefile | 1 +
> > drivers/hwmon/raspberrypi-hwmon.c | 168 ++++++++++++++++++++++++++++++++++
> > 4 files changed, 201 insertions(+)
> > create mode 100644 Documentation/hwmon/raspberrypi-hwmon
> > create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> >
> > diff --git a/Documentation/hwmon/raspberrypi-hwmon b/Documentation/hwmon/raspberrypi-hwmon
> > new file mode 100644
> > index 0000000..3c92e2c
> > --- /dev/null
> > +++ b/Documentation/hwmon/raspberrypi-hwmon
> > @@ -0,0 +1,22 @@
> > +Kernel driver raspberrypi-hwmon
> > +===============================
> > +
> > +Supported boards:
> > + * Raspberry Pi A+ (via GPIO on SoC)
> > + * Raspberry Pi B+ (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)
> > +
> > +Author: Stefan Wahren <stefan.wahren@i2se.com>
> > +
> > +Description
> > +-----------
> > +
> > +This driver periodically polls a mailbox property of the VC4 firmware to detect
> > +undervoltage conditions.
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +in0_lcrit_alarm Undervoltage alarm
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 768aed5..9a5bdb0 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 RASPBERRYPI_FIRMWARE || COMPILE_TEST
> > + 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..6233e84
> > --- /dev/null
> > +++ b/drivers/hwmon/raspberrypi-hwmon.c
> > @@ -0,0 +1,168 @@
> > +// 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/module.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;
> > +
> > + /* Request firmware to 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, "Failed to get throttled (%d)\n",
> > + 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, "Undervoltage detected!\n");
> > + else
> > + dev_info(data->hwmon_dev, "Voltage normalised\n");
> > +
> > + 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);
> > +
> > + *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)
> > +{
> > + 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 rpi_hwmon_data *data;
> > + int ret;
> > +
> > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > + if (!data)
> > + return -ENOMEM;
> > +
> > + data->fw = platform_get_drvdata(to_platform_device(dev->parent));
> > + if (!data->fw)
> > + return -EPROBE_DEFER;
> > +
>
> I am a bit at loss here (and sorry I didn't bring this up before).
> How would this ever be possible, given that the driver is registered
> from the firmware driver ?
Do you refer to the (wrong) return code, the assumption that the parent must be a platform driver or a possible race?
>
> > + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
> > + &data->last_throttled,
> > + sizeof(data->last_throttled));
> > + if (ret)
> > + 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 struct platform_driver rpi_hwmon_driver = {
> > + .probe = rpi_hwmon_probe,
> > + .remove = rpi_hwmon_remove,
> > + .driver = {
> > + .name = "raspberrypi-hwmon",
> > + },
> > +};
> > +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] 13+ messages in thread
* Re: [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor
2018-05-22 13:51 ` Stefan Wahren
@ 2018-05-22 14:10 ` Guenter Roeck
2018-05-22 19:31 ` Stefan Wahren
0 siblings, 1 reply; 13+ messages in thread
From: Guenter Roeck @ 2018-05-22 14:10 UTC (permalink / raw)
To: Stefan Wahren, Rob Herring, Eric Anholt, Mark Rutland,
Jonathan Corbet, Jean Delvare
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Phil Elwell, Noralf Trønnes,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
On 05/22/2018 06:51 AM, Stefan Wahren wrote:
> Hi Guenter,
>
>> Guenter Roeck <linux@roeck-us.net> hat am 22. Mai 2018 um 15:41 geschrieben:
>>
>>
>> On 05/22/2018 04:21 AM, Stefan Wahren wrote:
>>> Currently there is no easy way to detect undervoltage conditions on a
>>> remote Raspberry Pi. This hwmon driver retrieves the state of the
>>> undervoltage sensor via mailbox interface. The handling based on
>>> Noralf's modifications to the downstream firmware driver. In case of
>>> an undervoltage 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>
>>> ---
>>> Documentation/hwmon/raspberrypi-hwmon | 22 +++++
>>> drivers/hwmon/Kconfig | 10 ++
>>> drivers/hwmon/Makefile | 1 +
>>> drivers/hwmon/raspberrypi-hwmon.c | 168 ++++++++++++++++++++++++++++++++++
>>> 4 files changed, 201 insertions(+)
>>> create mode 100644 Documentation/hwmon/raspberrypi-hwmon
>>> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
>>>
>>> diff --git a/Documentation/hwmon/raspberrypi-hwmon b/Documentation/hwmon/raspberrypi-hwmon
>>> new file mode 100644
>>> index 0000000..3c92e2c
>>> --- /dev/null
>>> +++ b/Documentation/hwmon/raspberrypi-hwmon
>>> @@ -0,0 +1,22 @@
>>> +Kernel driver raspberrypi-hwmon
>>> +===============================
>>> +
>>> +Supported boards:
>>> + * Raspberry Pi A+ (via GPIO on SoC)
>>> + * Raspberry Pi B+ (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)
>>> +
>>> +Author: Stefan Wahren <stefan.wahren@i2se.com>
>>> +
>>> +Description
>>> +-----------
>>> +
>>> +This driver periodically polls a mailbox property of the VC4 firmware to detect
>>> +undervoltage conditions.
>>> +
>>> +Sysfs entries
>>> +-------------
>>> +
>>> +in0_lcrit_alarm Undervoltage alarm
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 768aed5..9a5bdb0 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 RASPBERRYPI_FIRMWARE || COMPILE_TEST
>>> + 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..6233e84
>>> --- /dev/null
>>> +++ b/drivers/hwmon/raspberrypi-hwmon.c
>>> @@ -0,0 +1,168 @@
>>> +// 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/module.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;
>>> +
>>> + /* Request firmware to 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, "Failed to get throttled (%d)\n",
>>> + 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, "Undervoltage detected!\n");
>>> + else
>>> + dev_info(data->hwmon_dev, "Voltage normalised\n");
>>> +
>>> + 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);
>>> +
>>> + *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)
>>> +{
>>> + 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 rpi_hwmon_data *data;
>>> + int ret;
>>> +
>>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>> + if (!data)
>>> + return -ENOMEM;
>>> +
>>> + data->fw = platform_get_drvdata(to_platform_device(dev->parent));
>>> + if (!data->fw)
>>> + return -EPROBE_DEFER;
>>> +
>>
>> I am a bit at loss here (and sorry I didn't bring this up before).
>> How would this ever be possible, given that the driver is registered
>> from the firmware driver ?
>
> Do you refer to the (wrong) return code, the assumption that the parent must be a platform driver or a possible race?
>
The return code is one thing. My question was how the driver would ever be instantiated
with platform_get_drvdata(to_platform_device(dev->parent)) == NULL (but dev->parent != NULL),
so I referred to the race. But, sure, a second question would be how that would indicate
that the parent is not instantiated yet (which by itself seems like an odd question).
Yet another question, as you point out, is why to use platform_get_drvdata(to_platform_device(dev->parent))
instead of dev_get_drvdata(dev->parent).
Guenter
>>
>>> + ret = rpi_firmware_property(data->fw, RPI_FIRMWARE_GET_THROTTLED,
>>> + &data->last_throttled,
>>> + sizeof(data->last_throttled));
>>> + if (ret)
>>> + 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 struct platform_driver rpi_hwmon_driver = {
>>> + .probe = rpi_hwmon_probe,
>>> + .remove = rpi_hwmon_remove,
>>> + .driver = {
>>> + .name = "raspberrypi-hwmon",
>>> + },
>>> +};
>>> +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] 13+ messages in thread
* Re: [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor
2018-05-22 14:10 ` Guenter Roeck
@ 2018-05-22 19:31 ` Stefan Wahren
2018-05-23 12:12 ` Robin Murphy
0 siblings, 1 reply; 13+ messages in thread
From: Stefan Wahren @ 2018-05-22 19:31 UTC (permalink / raw)
To: Rob Herring, Guenter Roeck, Eric Anholt, Mark Rutland,
Jonathan Corbet, Jean Delvare
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Phil Elwell, Noralf Trønnes,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
> Guenter Roeck <linux@roeck-us.net> hat am 22. Mai 2018 um 16:10 geschrieben:
>
>
> On 05/22/2018 06:51 AM, Stefan Wahren wrote:
> > Hi Guenter,
> >
> >> Guenter Roeck <linux@roeck-us.net> hat am 22. Mai 2018 um 15:41 geschrieben:
> >>
> >>
> >> On 05/22/2018 04:21 AM, Stefan Wahren wrote:
> >>> Currently there is no easy way to detect undervoltage conditions on a
> >>> remote Raspberry Pi. This hwmon driver retrieves the state of the
> >>> undervoltage sensor via mailbox interface. The handling based on
> >>> Noralf's modifications to the downstream firmware driver. In case of
> >>> an undervoltage 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>
> >>> ---
> >>> Documentation/hwmon/raspberrypi-hwmon | 22 +++++
> >>> drivers/hwmon/Kconfig | 10 ++
> >>> drivers/hwmon/Makefile | 1 +
> >>> drivers/hwmon/raspberrypi-hwmon.c | 168 ++++++++++++++++++++++++++++++++++
> >>> 4 files changed, 201 insertions(+)
> >>> create mode 100644 Documentation/hwmon/raspberrypi-hwmon
> >>> create mode 100644 drivers/hwmon/raspberrypi-hwmon.c
> >>>
> >>> diff --git a/Documentation/hwmon/raspberrypi-hwmon b/Documentation/hwmon/raspberrypi-hwmon
> >>> new file mode 100644
> >>> index 0000000..3c92e2c
> >>> --- /dev/null
> >>> +++ b/Documentation/hwmon/raspberrypi-hwmon
> >>> @@ -0,0 +1,22 @@
> >>> +Kernel driver raspberrypi-hwmon
> >>> +===============================
> >>> +
> >>> +Supported boards:
> >>> + * Raspberry Pi A+ (via GPIO on SoC)
> >>> + * Raspberry Pi B+ (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)
> >>> +
> >>> +Author: Stefan Wahren <stefan.wahren@i2se.com>
> >>> +
> >>> +Description
> >>> +-----------
> >>> +
> >>> +This driver periodically polls a mailbox property of the VC4 firmware to detect
> >>> +undervoltage conditions.
> >>> +
> >>> +Sysfs entries
> >>> +-------------
> >>> +
> >>> +in0_lcrit_alarm Undervoltage alarm
> >>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> >>> index 768aed5..9a5bdb0 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 RASPBERRYPI_FIRMWARE || COMPILE_TEST
> >>> + 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..6233e84
> >>> --- /dev/null
> >>> +++ b/drivers/hwmon/raspberrypi-hwmon.c
> >>> @@ -0,0 +1,168 @@
> >>> +// 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/module.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;
> >>> +
> >>> + /* Request firmware to 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, "Failed to get throttled (%d)\n",
> >>> + 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, "Undervoltage detected!\n");
> >>> + else
> >>> + dev_info(data->hwmon_dev, "Voltage normalised\n");
> >>> +
> >>> + 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);
> >>> +
> >>> + *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)
> >>> +{
> >>> + 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 rpi_hwmon_data *data;
> >>> + int ret;
> >>> +
> >>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >>> + if (!data)
> >>> + return -ENOMEM;
> >>> +
> >>> + data->fw = platform_get_drvdata(to_platform_device(dev->parent));
> >>> + if (!data->fw)
> >>> + return -EPROBE_DEFER;
> >>> +
> >>
> >> I am a bit at loss here (and sorry I didn't bring this up before).
> >> How would this ever be possible, given that the driver is registered
> >> from the firmware driver ?
> >
> > Do you refer to the (wrong) return code, the assumption that the parent must be a platform driver or a possible race?
> >
>
> The return code is one thing. My question was how the driver would ever be instantiated
> with platform_get_drvdata(to_platform_device(dev->parent)) == NULL (but dev->parent != NULL),
> so I referred to the race. But, sure, a second question would be how that would indicate
> that the parent is not instantiated yet (which by itself seems like an odd question).
This shouldn't happen and worth a log error. In patch #3 the registration is called after the complete private data of the firmware driver is initialized. Did i missed something?
But i must confess that i didn't test all builtin/module combinations.
>
> Yet another question, as you point out, is why to use platform_get_drvdata(to_platform_device(dev->parent))
> instead of dev_get_drvdata(dev->parent).
Sure this is much simpler
Thanks
Stefan
_______________________________________________
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] 13+ messages in thread
* Re: [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor
2018-05-22 19:31 ` Stefan Wahren
@ 2018-05-23 12:12 ` Robin Murphy
2018-05-23 18:12 ` Guenter Roeck
0 siblings, 1 reply; 13+ messages in thread
From: Robin Murphy @ 2018-05-23 12:12 UTC (permalink / raw)
To: Stefan Wahren, Rob Herring, Guenter Roeck, Eric Anholt,
Mark Rutland, Jonathan Corbet, Jean Delvare
Cc: linux-hwmon, devicetree, Florian Fainelli, Scott Branden,
linux-doc, Ray Jui, Phil Elwell, Noralf Trønnes,
bcm-kernel-feedback-list, linux-rpi-kernel, linux-arm-kernel
On 22/05/18 20:31, Stefan Wahren wrote:
[...]
>>>>> +static int rpi_hwmon_probe(struct platform_device *pdev)
>>>>> +{
>>>>> + struct device *dev = &pdev->dev;
>>>>> + struct rpi_hwmon_data *data;
>>>>> + int ret;
>>>>> +
>>>>> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>>>> + if (!data)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + data->fw = platform_get_drvdata(to_platform_device(dev->parent));
>>>>> + if (!data->fw)
>>>>> + return -EPROBE_DEFER;
>>>>> +
>>>>
>>>> I am a bit at loss here (and sorry I didn't bring this up before).
>>>> How would this ever be possible, given that the driver is registered
>>>> from the firmware driver ?
>>>
>>> Do you refer to the (wrong) return code, the assumption that the parent must be a platform driver or a possible race?
>>>
>>
>> The return code is one thing. My question was how the driver would ever be instantiated
>> with platform_get_drvdata(to_platform_device(dev->parent)) == NULL (but dev->parent != NULL),
>> so I referred to the race. But, sure, a second question would be how that would indicate
>> that the parent is not instantiated yet (which by itself seems like an odd question).
>
> This shouldn't happen and worth a log error. In patch #3 the registration is called after the complete private data of the firmware driver is initialized. Did i missed something?
>
> But i must confess that i didn't test all builtin/module combinations.
The point is that, by construction, a "raspberrypi-hwmon" device will
only ever be created for this driver to bind to if the firmware device
is both fully initialised and known to support the GET_THROTTLED call
already. Thus trying to check those again from the hwmon driver is at
best pointless, and at worst misleading. If somebody *does* manage to
bind this driver to some random inappropriate device, you've still got
no guarantee that dev->parent is valid or that
dev_get_drvdata(dev->parent)) won't return something non-NULL that isn't
a struct rpi_firmware pointer, at which point you're liable to pass the
paranoid check yet still crash anyway.
IOW, you can't reasonably defend against incorrect operation, and under
correct operation there's nothing to defend against, so either way it's
pretty futile to waste effort trying.
Robin.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor
2018-05-23 12:12 ` Robin Murphy
@ 2018-05-23 18:12 ` Guenter Roeck
0 siblings, 0 replies; 13+ messages in thread
From: Guenter Roeck @ 2018-05-23 18:12 UTC (permalink / raw)
To: Robin Murphy
Cc: Stefan Wahren, Mark Rutland, Jean Delvare, Scott Branden,
Jonathan Corbet, Ray Jui, linux-doc, Phil Elwell, Eric Anholt,
devicetree, Rob Herring, bcm-kernel-feedback-list,
linux-rpi-kernel, Florian Fainelli, linux-hwmon,
linux-arm-kernel, Noralf Trønnes
On Wed, May 23, 2018 at 01:12:10PM +0100, Robin Murphy wrote:
> On 22/05/18 20:31, Stefan Wahren wrote:
> [...]
> >>>>>+static int rpi_hwmon_probe(struct platform_device *pdev)
> >>>>>+{
> >>>>>+ struct device *dev = &pdev->dev;
> >>>>>+ struct rpi_hwmon_data *data;
> >>>>>+ int ret;
> >>>>>+
> >>>>>+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >>>>>+ if (!data)
> >>>>>+ return -ENOMEM;
> >>>>>+
> >>>>>+ data->fw = platform_get_drvdata(to_platform_device(dev->parent));
> >>>>>+ if (!data->fw)
> >>>>>+ return -EPROBE_DEFER;
> >>>>>+
> >>>>
> >>>>I am a bit at loss here (and sorry I didn't bring this up before).
> >>>>How would this ever be possible, given that the driver is registered
> >>>>from the firmware driver ?
> >>>
> >>>Do you refer to the (wrong) return code, the assumption that the parent must be a platform driver or a possible race?
> >>>
> >>
> >>The return code is one thing. My question was how the driver would ever be instantiated
> >>with platform_get_drvdata(to_platform_device(dev->parent)) == NULL (but dev->parent != NULL),
> >>so I referred to the race. But, sure, a second question would be how that would indicate
> >>that the parent is not instantiated yet (which by itself seems like an odd question).
> >
> >This shouldn't happen and worth a log error. In patch #3 the registration is called after the complete private data of the firmware driver is initialized. Did i missed something?
> >
> >But i must confess that i didn't test all builtin/module combinations.
>
> The point is that, by construction, a "raspberrypi-hwmon" device will only
> ever be created for this driver to bind to if the firmware device is both
> fully initialised and known to support the GET_THROTTLED call already. Thus
> trying to check those again from the hwmon driver is at best pointless, and
> at worst misleading. If somebody *does* manage to bind this driver to some
> random inappropriate device, you've still got no guarantee that dev->parent
> is valid or that dev_get_drvdata(dev->parent)) won't return something
> non-NULL that isn't a struct rpi_firmware pointer, at which point you're
> liable to pass the paranoid check yet still crash anyway.
>
> IOW, you can't reasonably defend against incorrect operation, and under
> correct operation there's nothing to defend against, so either way it's
> pretty futile to waste effort trying.
>
Well said.
Guenter
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-05-23 18:12 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-22 11:21 [PATCH RFC V2 0/6] hwmon: Add support for Raspberry Pi voltage sensor Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 1/6] ARM: bcm2835: Add GET_THROTTLED firmware property Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 2/6] hwmon: Add support for RPi voltage sensor Stefan Wahren
2018-05-22 13:41 ` Guenter Roeck
2018-05-22 13:51 ` Stefan Wahren
2018-05-22 14:10 ` Guenter Roeck
2018-05-22 19:31 ` Stefan Wahren
2018-05-23 12:12 ` Robin Murphy
2018-05-23 18:12 ` Guenter Roeck
2018-05-22 11:21 ` [PATCH RFC V2 3/6] firmware: raspberrypi: Register hwmon driver Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 4/6] ARM: bcm2835_defconfig: Enable RPi voltage sensor Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 5/6] ARM: multi_v7_defconfig: " Stefan Wahren
2018-05-22 11:21 ` [PATCH RFC V2 6/6] arm64: defconfig: " Stefan Wahren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).