Regulators as sensors?

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Regulators as sensors?

Mark Kettenis
The diff below exposes voltage regulators as sensors.  This makes it
easy to look at the current settings of these regulators.  The
downside is that these aren't really sensors as the voltages are not
actually measured.

The functionality is optional; callers can pass NULL in the
regulator_register() if the regulators aren't particularly
interesting.

This is what it looks like on the rk3399-firefly:

milhaud$ sysctl hw.sensors
hw.sensors.rktemp0.temp0=23.89 degC (CPU)
hw.sensors.rktemp0.temp1=28.75 degC (GPU)
hw.sensors.rkpmic0.volt0=0.90 VDC (vdd_cpu_l)
hw.sensors.rkpmic0.volt1=1.80 VDC (vcc1v8_dvp)
hw.sensors.rkpmic0.volt2=1.80 VDC (vcc1v8_pmu)
hw.sensors.rkpmic0.volt3=3.00 VDC (vcc_sd)
hw.sensors.rkpmic0.volt4=1.80 VDC (vcca1v8_codec)
hw.sensors.rkpmic0.volt5=3.00 VDC (vcc_3v0)

thoughts?


Index: sys/dev/ofw/ofw_regulator.c
===================================================================
RCS file: /cvs/src/sys/dev/ofw/ofw_regulator.c,v
retrieving revision 1.2
diff -u -p -r1.2 ofw_regulator.c
--- sys/dev/ofw/ofw_regulator.c 18 Nov 2017 13:48:50 -0000 1.2
+++ sys/dev/ofw/ofw_regulator.c 21 Nov 2017 21:19:59 -0000
@@ -15,9 +15,10 @@
  * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
-#include <sys/types.h>
+#include <sys/param.h>
 #include <sys/systm.h>
 #include <sys/malloc.h>
+#include <sys/sensors.h>
 
 #include <dev/ofw/openfirm.h>
 #include <dev/ofw/ofw_gpio.h>
@@ -28,13 +29,24 @@ LIST_HEAD(, regulator_device) regulator_
  LIST_HEAD_INITIALIZER(regulator_devices);
 
 void
-regulator_register(struct regulator_device *rd)
+regulator_register(struct regulator_device *rd, struct ksensordev *sensdev)
 {
  rd->rd_phandle = OF_getpropint(rd->rd_node, "phandle", 0);
  if (rd->rd_phandle == 0)
  return;
 
  LIST_INSERT_HEAD(&regulator_devices, rd, rd_list);
+
+ if (sensdev) {
+ rd->rd_sens = malloc(sizeof(struct ksensor), M_DEVBUF,
+    M_WAITOK | M_ZERO);
+ OF_getprop(rd->rd_node, "regulator-name",
+    rd->rd_sens->desc, sizeof(rd->rd_sens->desc));
+ rd->rd_sens->desc[sizeof(rd->rd_sens->desc) - 1] = 0;
+ rd->rd_sens->type = SENSOR_VOLTS_DC;
+ rd->rd_sens->value = rd->rd_get_voltage(rd->rd_cookie);
+ sensor_attach(sensdev, rd->rd_sens);
+ }
 }
 
 int
@@ -121,14 +133,20 @@ int
 regulator_set_voltage(uint32_t phandle, uint32_t voltage)
 {
  struct regulator_device *rd;
+ int ret;
 
  LIST_FOREACH(rd, &regulator_devices, rd_list) {
  if (rd->rd_phandle == phandle)
  break;
  }
 
- if (rd && rd->rd_set_voltage)
- return rd->rd_set_voltage(rd->rd_cookie, voltage);
+ if (rd && rd->rd_set_voltage) {
+ ret = rd->rd_set_voltage(rd->rd_cookie, voltage);
+ if (ret == 0 && rd->rd_sens)
+ rd->rd_sens->value = rd->rd_get_voltage(rd->rd_cookie);
+
+ return ret;
+ }
 
  return -1;
 }
Index: sys/dev/ofw/ofw_regulator.h
===================================================================
RCS file: /cvs/src/sys/dev/ofw/ofw_regulator.h,v
retrieving revision 1.3
diff -u -p -r1.3 ofw_regulator.h
--- sys/dev/ofw/ofw_regulator.h 18 Nov 2017 21:03:23 -0000 1.3
+++ sys/dev/ofw/ofw_regulator.h 21 Nov 2017 21:19:59 -0000
@@ -18,6 +18,9 @@
 #ifndef _DEV_OFW_REGULATOR_H_
 #define _DEV_OFW_REGULATOR_H_
 
+struct ksensor;
+struct ksensordev;
+
 struct regulator_device {
  int rd_node;
  void *rd_cookie;
@@ -26,9 +29,11 @@ struct regulator_device {
 
  LIST_ENTRY(regulator_device) rd_list;
  uint32_t rd_phandle;
+
+ struct ksensor *rd_sens;
 };
 
-void regulator_register(struct regulator_device *);
+void regulator_register(struct regulator_device *, struct ksensordev *);
 
 int regulator_enable(uint32_t);
 int regulator_disable(uint32_t);
Index: sys/dev/fdt/rkpmic.c
===================================================================
RCS file: /cvs/src/sys/dev/fdt/rkpmic.c,v
retrieving revision 1.3
diff -u -p -r1.3 rkpmic.c
--- sys/dev/fdt/rkpmic.c 18 Nov 2017 20:29:51 -0000 1.3
+++ sys/dev/fdt/rkpmic.c 21 Nov 2017 21:19:59 -0000
@@ -19,6 +19,7 @@
 #include <sys/systm.h>
 #include <sys/device.h>
 #include <sys/malloc.h>
+#include <sys/sensors.h>
 
 #include <dev/ofw/openfirm.h>
 #include <dev/ofw/ofw_regulator.h>
@@ -70,6 +71,7 @@ struct rkpmic_softc {
  i2c_addr_t sc_addr;
 
  struct todr_chip_handle sc_todr;
+ struct ksensordev sc_sensordev;
 };
 
 int rkpmic_match(struct device *, void *, void *);
@@ -122,6 +124,11 @@ rkpmic_attach(struct device *parent, str
  return;
  for (node = OF_child(node); node; node = OF_peer(node))
  rkpmic_attach_regulator(sc, node);
+
+ /* Register sensors. */
+ strlcpy(sc->sc_sensordev.xname, sc->sc_dev.dv_xname,
+    sizeof(sc->sc_sensordev.xname));
+ sensordev_install(&sc->sc_sensordev);
 }
 
 struct rkpmic_regulator {
@@ -163,7 +170,7 @@ rkpmic_attach_regulator(struct rkpmic_so
  rr->rr_rd.rd_node = node;
  rr->rr_rd.rd_cookie = rr;
  rr->rr_rd.rd_get_voltage = rkpmic_get_voltage;
- regulator_register(&rr->rr_rd);
+ regulator_register(&rr->rr_rd, &sc->sc_sensordev);
 }
 
 uint32_t

Reply | Threaded
Open this post in threaded view
|

Re: Regulators as sensors?

STeve Andre'


On 11/21/17 16:31, Mark Kettenis wrote:

> The diff below exposes voltage regulators as sensors.  This makes it
> easy to look at the current settings of these regulators.  The
> downside is that these aren't really sensors as the voltages are not
> actually measured.
>
> The functionality is optional; callers can pass NULL in the
> regulator_register() if the regulators aren't particularly
> interesting.
>
> This is what it looks like on the rk3399-firefly:
>
> milhaud$ sysctl hw.sensors
> hw.sensors.rktemp0.temp0=23.89 degC (CPU)
> hw.sensors.rktemp0.temp1=28.75 degC (GPU)
> hw.sensors.rkpmic0.volt0=0.90 VDC (vdd_cpu_l)
> hw.sensors.rkpmic0.volt1=1.80 VDC (vcc1v8_dvp)
> hw.sensors.rkpmic0.volt2=1.80 VDC (vcc1v8_pmu)
> hw.sensors.rkpmic0.volt3=3.00 VDC (vcc_sd)
> hw.sensors.rkpmic0.volt4=1.80 VDC (vcca1v8_codec)
> hw.sensors.rkpmic0.volt5=3.00 VDC (vcc_3v0)
>
> thoughts?

As someone who does hardware stuff, having easy access to these sensorts
can't hurt, and might be useful in some situations.  I've measured
voltages before and found during extreme temperature conditions things
changed. So it's possibly useful and doesn't cost much.

--STeve Andre'

Reply | Threaded
Open this post in threaded view
|

Re: Regulators as sensors?

Anders Andersson
On Tue, Nov 21, 2017 at 11:19 PM, STeve Andre' <[hidden email]> wrote:

> On 11/21/17 16:31, Mark Kettenis wrote:
>>
>> The diff below exposes voltage regulators as sensors.  This makes it
>> easy to look at the current settings of these regulators.  The
>> downside is that these aren't really sensors as the voltages are not
>> actually measured.
>>
>> The functionality is optional; callers can pass NULL in the
>> regulator_register() if the regulators aren't particularly
>> interesting.
>>
>> This is what it looks like on the rk3399-firefly:
>>
>> milhaud$ sysctl hw.sensors
>> hw.sensors.rktemp0.temp0=23.89 degC (CPU)
>> hw.sensors.rktemp0.temp1=28.75 degC (GPU)
>> hw.sensors.rkpmic0.volt0=0.90 VDC (vdd_cpu_l)
>> hw.sensors.rkpmic0.volt1=1.80 VDC (vcc1v8_dvp)
>> hw.sensors.rkpmic0.volt2=1.80 VDC (vcc1v8_pmu)
>> hw.sensors.rkpmic0.volt3=3.00 VDC (vcc_sd)
>> hw.sensors.rkpmic0.volt4=1.80 VDC (vcca1v8_codec)
>> hw.sensors.rkpmic0.volt5=3.00 VDC (vcc_3v0)
>>
>> thoughts?
>
>
> As someone who does hardware stuff, having easy access to these sensorts
> can't hurt, and might be useful in some situations.  I've measured voltages
> before and found during extreme temperature conditions things changed. So
> it's possibly useful and doesn't cost much.

This reply illustrates the problem, and I think it won't be the last
time someone misunderstands the feature.

They are *not* sensors, so they will not vary under load. They don't
reflect the actual voltage, so they are useless for checking if the
hardware works as it should. I bet that a lot of people will still
assume that they can be used as such, leading people to believe that
everything is OK with their hardware when it's not.

Reply | Threaded
Open this post in threaded view
|

Re: Regulators as sensors?

STeve Andre'


On 11/22/17 13:06, Anders Andersson wrote:

> On Tue, Nov 21, 2017 at 11:19 PM, STeve Andre' <[hidden email]> wrote:
>> On 11/21/17 16:31, Mark Kettenis wrote:
>>>
>>> The diff below exposes voltage regulators as sensors.  This makes it
>>> easy to look at the current settings of these regulators.  The
>>> downside is that these aren't really sensors as the voltages are not
>>> actually measured.
>>>
>>> The functionality is optional; callers can pass NULL in the
>>> regulator_register() if the regulators aren't particularly
>>> interesting.
>>>
>>> This is what it looks like on the rk3399-firefly:
>>>
>>> milhaud$ sysctl hw.sensors
>>> hw.sensors.rktemp0.temp0=23.89 degC (CPU)
>>> hw.sensors.rktemp0.temp1=28.75 degC (GPU)
>>> hw.sensors.rkpmic0.volt0=0.90 VDC (vdd_cpu_l)
>>> hw.sensors.rkpmic0.volt1=1.80 VDC (vcc1v8_dvp)
>>> hw.sensors.rkpmic0.volt2=1.80 VDC (vcc1v8_pmu)
>>> hw.sensors.rkpmic0.volt3=3.00 VDC (vcc_sd)
>>> hw.sensors.rkpmic0.volt4=1.80 VDC (vcca1v8_codec)
>>> hw.sensors.rkpmic0.volt5=3.00 VDC (vcc_3v0)
>>>
>>> thoughts?
>>
>>
>> As someone who does hardware stuff, having easy access to these sensorts
>> can't hurt, and might be useful in some situations.  I've measured voltages
>> before and found during extreme temperature conditions things changed. So
>> it's possibly useful and doesn't cost much.
>
> This reply illustrates the problem, and I think it won't be the last
> time someone misunderstands the feature.
>
> They are *not* sensors, so they will not vary under load. They don't
> reflect the actual voltage, so they are useless for checking if the
> hardware works as it should. I bet that a lot of people will still
> assume that they can be used as such, leading people to believe that
> everything is OK with their hardware when it's not.
>
>

That's true.  Hardware often lies.  That's why I like the ability to
monitor stuff.

--STeve Andre'

Reply | Threaded
Open this post in threaded view
|

Re: Regulators as sensors?

Artturi Alm
In reply to this post by Mark Kettenis
On Tue, Nov 21, 2017 at 10:31:47PM +0100, Mark Kettenis wrote:

> The diff below exposes voltage regulators as sensors.  This makes it
> easy to look at the current settings of these regulators.  The
> downside is that these aren't really sensors as the voltages are not
> actually measured.
>
> The functionality is optional; callers can pass NULL in the
> regulator_register() if the regulators aren't particularly
> interesting.
>
> This is what it looks like on the rk3399-firefly:
>
> milhaud$ sysctl hw.sensors
> hw.sensors.rktemp0.temp0=23.89 degC (CPU)
> hw.sensors.rktemp0.temp1=28.75 degC (GPU)
> hw.sensors.rkpmic0.volt0=0.90 VDC (vdd_cpu_l)
> hw.sensors.rkpmic0.volt1=1.80 VDC (vcc1v8_dvp)
> hw.sensors.rkpmic0.volt2=1.80 VDC (vcc1v8_pmu)
> hw.sensors.rkpmic0.volt3=3.00 VDC (vcc_sd)
> hw.sensors.rkpmic0.volt4=1.80 VDC (vcca1v8_codec)
> hw.sensors.rkpmic0.volt5=3.00 VDC (vcc_3v0)
>
> thoughts?
>

How about increasing usefulness by skipping those where:
        regulator-min == -max && regulator-always-on ?

"eeprom -p" does cover those as is.
-Artturi

>
> Index: sys/dev/ofw/ofw_regulator.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/ofw/ofw_regulator.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 ofw_regulator.c
> --- sys/dev/ofw/ofw_regulator.c 18 Nov 2017 13:48:50 -0000 1.2
> +++ sys/dev/ofw/ofw_regulator.c 21 Nov 2017 21:19:59 -0000
> @@ -15,9 +15,10 @@
>   * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
>   */
>  
> -#include <sys/types.h>
> +#include <sys/param.h>
>  #include <sys/systm.h>
>  #include <sys/malloc.h>
> +#include <sys/sensors.h>
>  
>  #include <dev/ofw/openfirm.h>
>  #include <dev/ofw/ofw_gpio.h>
> @@ -28,13 +29,24 @@ LIST_HEAD(, regulator_device) regulator_
>   LIST_HEAD_INITIALIZER(regulator_devices);
>  
>  void
> -regulator_register(struct regulator_device *rd)
> +regulator_register(struct regulator_device *rd, struct ksensordev *sensdev)
>  {
>   rd->rd_phandle = OF_getpropint(rd->rd_node, "phandle", 0);
>   if (rd->rd_phandle == 0)
>   return;
>  
>   LIST_INSERT_HEAD(&regulator_devices, rd, rd_list);
> +
> + if (sensdev) {
> + rd->rd_sens = malloc(sizeof(struct ksensor), M_DEVBUF,
> +    M_WAITOK | M_ZERO);
> + OF_getprop(rd->rd_node, "regulator-name",
> +    rd->rd_sens->desc, sizeof(rd->rd_sens->desc));
> + rd->rd_sens->desc[sizeof(rd->rd_sens->desc) - 1] = 0;
> + rd->rd_sens->type = SENSOR_VOLTS_DC;
> + rd->rd_sens->value = rd->rd_get_voltage(rd->rd_cookie);
> + sensor_attach(sensdev, rd->rd_sens);
> + }
>  }
>  
>  int
> @@ -121,14 +133,20 @@ int
>  regulator_set_voltage(uint32_t phandle, uint32_t voltage)
>  {
>   struct regulator_device *rd;
> + int ret;
>  
>   LIST_FOREACH(rd, &regulator_devices, rd_list) {
>   if (rd->rd_phandle == phandle)
>   break;
>   }
>  
> - if (rd && rd->rd_set_voltage)
> - return rd->rd_set_voltage(rd->rd_cookie, voltage);
> + if (rd && rd->rd_set_voltage) {
> + ret = rd->rd_set_voltage(rd->rd_cookie, voltage);
> + if (ret == 0 && rd->rd_sens)
> + rd->rd_sens->value = rd->rd_get_voltage(rd->rd_cookie);
> +
> + return ret;
> + }
>  
>   return -1;
>  }
> Index: sys/dev/ofw/ofw_regulator.h
> ===================================================================
> RCS file: /cvs/src/sys/dev/ofw/ofw_regulator.h,v
> retrieving revision 1.3
> diff -u -p -r1.3 ofw_regulator.h
> --- sys/dev/ofw/ofw_regulator.h 18 Nov 2017 21:03:23 -0000 1.3
> +++ sys/dev/ofw/ofw_regulator.h 21 Nov 2017 21:19:59 -0000
> @@ -18,6 +18,9 @@
>  #ifndef _DEV_OFW_REGULATOR_H_
>  #define _DEV_OFW_REGULATOR_H_
>  
> +struct ksensor;
> +struct ksensordev;
> +
>  struct regulator_device {
>   int rd_node;
>   void *rd_cookie;
> @@ -26,9 +29,11 @@ struct regulator_device {
>  
>   LIST_ENTRY(regulator_device) rd_list;
>   uint32_t rd_phandle;
> +
> + struct ksensor *rd_sens;
>  };
>  
> -void regulator_register(struct regulator_device *);
> +void regulator_register(struct regulator_device *, struct ksensordev *);
>  
>  int regulator_enable(uint32_t);
>  int regulator_disable(uint32_t);
> Index: sys/dev/fdt/rkpmic.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/fdt/rkpmic.c,v
> retrieving revision 1.3
> diff -u -p -r1.3 rkpmic.c
> --- sys/dev/fdt/rkpmic.c 18 Nov 2017 20:29:51 -0000 1.3
> +++ sys/dev/fdt/rkpmic.c 21 Nov 2017 21:19:59 -0000
> @@ -19,6 +19,7 @@
>  #include <sys/systm.h>
>  #include <sys/device.h>
>  #include <sys/malloc.h>
> +#include <sys/sensors.h>
>  
>  #include <dev/ofw/openfirm.h>
>  #include <dev/ofw/ofw_regulator.h>
> @@ -70,6 +71,7 @@ struct rkpmic_softc {
>   i2c_addr_t sc_addr;
>  
>   struct todr_chip_handle sc_todr;
> + struct ksensordev sc_sensordev;
>  };
>  
>  int rkpmic_match(struct device *, void *, void *);
> @@ -122,6 +124,11 @@ rkpmic_attach(struct device *parent, str
>   return;
>   for (node = OF_child(node); node; node = OF_peer(node))
>   rkpmic_attach_regulator(sc, node);
> +
> + /* Register sensors. */
> + strlcpy(sc->sc_sensordev.xname, sc->sc_dev.dv_xname,
> +    sizeof(sc->sc_sensordev.xname));
> + sensordev_install(&sc->sc_sensordev);
>  }
>  
>  struct rkpmic_regulator {
> @@ -163,7 +170,7 @@ rkpmic_attach_regulator(struct rkpmic_so
>   rr->rr_rd.rd_node = node;
>   rr->rr_rd.rd_cookie = rr;
>   rr->rr_rd.rd_get_voltage = rkpmic_get_voltage;
> - regulator_register(&rr->rr_rd);
> + regulator_register(&rr->rr_rd, &sc->sc_sensordev);
>  }
>  
>  uint32_t
>