[PATCH v3] Documentation: add Device tree bindings for hwmon/nct7802

[PATCH v3] Documentation: add Device tree bindings for hwmon/nct7802

[Constantine Shulyupin]

Add add Device tree bindings for registers
which are not covered by hwmon ABI and are required to
configure specific HW.
---

Changed in v3:
- Fixed vendor prefix
- Added short registers description,
  full registers description is available at
https://www.nuvoton.com/hq/products/cloud-computing/hardware-monitors/desktop-server-series/nct7802y/

Changed in v2:
- Removed nct7802,reg-init
- Added registers initialization by names

Introduced in v1:
 - nct7802,reg-init

Signed-off-by: Constantine Shulyupin <const@MakeLinux.com>
---
 .../devicetree/bindings/hwmon/nct7802.txt          | 28 ++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/nct7802.txt

diff --git a/Documentation/devicetree/bindings/hwmon/nct7802.txt b/Documentation/devicetree/bindings/hwmon/nct7802.txt
new file mode 100644
index 0000000..5b4c3fe
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/nct7802.txt
@@ -0,0 +1,28 @@
+Nuvoton NCT7802Y Hardware Monitoring IC
+
+Required node properties:
+
+ - "compatible": must be "nuvoton,nct7802"
+ - "reg": I2C bus address of the device
+
+Optional properties:
+
+One byte registers:
+ - nuvoton,start - Start register at index 0x21
+ - nuvoton,mode - Mode register at index 0x22
+ - nuvoton,en_peci - PECI enable register at index 0x23
+ - nuvoton,en_fan - Fan Enable Register at index 0x24
+ - nuvoton,en_v - voltage monitor enable register
+		  at index 0x25
+
+A detailed datasheet for registers and the device is
+available at Nuvoton web site.
+
+Example nct7802 node:
+
+nct7802 {
+	compatible = "nuvoton,nct7802";
+	reg = <0x2a>;
+	nuvoton,start = <0x01>;
+	nuvoton,mode = <0x7E>; // RTD1_MD = 2
+};
-- 
1.9.1

Re: [PATCH v3] Documentation: add Device tree bindings for hwmon/nct7802

[Mark Rutland]

On Mon, Sep 07, 2015 at 11:27:47PM +0100, Constantine Shulyupin wrote:
> Add add Device tree bindings for registers
> which are not covered by hwmon ABI and are required to
> configure specific HW.

This doesn't really answer my question [1] of why you need these.

We generally don't encode register values unless there's no other way to
describe the device. Why can we not describe the physical properties of
the device that make these values necessary?

Could you please Cc me on the entire series in future? The driver code
is helpful context.

> ---
> 
> Changed in v3:
> - Fixed vendor prefix
> - Added short registers description,
>   full registers description is available at
> https://www.nuvoton.com/hq/products/cloud-computing/hardware-monitors/desktop-server-series/nct7802y/

This seems to require registration, which is unfortuante (and
familiar)...

> 
> Changed in v2:
> - Removed nct7802,reg-init
> - Added registers initialization by names
> 
> Introduced in v1:
>  - nct7802,reg-init
> 
> Signed-off-by: Constantine Shulyupin <const-GkuDRZ1haSFDPfheJLI6IQ@public.gmane.org>
> ---
>  .../devicetree/bindings/hwmon/nct7802.txt          | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/nct7802.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/nct7802.txt b/Documentation/devicetree/bindings/hwmon/nct7802.txt
> new file mode 100644
> index 0000000..5b4c3fe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/nct7802.txt
> @@ -0,0 +1,28 @@
> +Nuvoton NCT7802Y Hardware Monitoring IC
> +
> +Required node properties:
> +
> + - "compatible": must be "nuvoton,nct7802"
> + - "reg": I2C bus address of the device
> +
> +Optional properties:
> +
> +One byte registers:
> + - nuvoton,start - Start register at index 0x21
> + - nuvoton,mode - Mode register at index 0x22
> + - nuvoton,en_peci - PECI enable register at index 0x23
> + - nuvoton,en_fan - Fan Enable Register at index 0x24
> + - nuvoton,en_v - voltage monitor enable register
> +		  at index 0x25

Minor nit I missed last time, but s/_/-/ in property names.

That said, as above I don't believe that these properties make sense
as they are. I think this needs properties that describe the HW, rather
than exposing the intricacies of the programming interface.

> +
> +A detailed datasheet for registers and the device is
> +available at Nuvoton web site.
> +
> +Example nct7802 node:
> +
> +nct7802 {
> +	compatible = "nuvoton,nct7802";
> +	reg = <0x2a>;
> +	nuvoton,start = <0x01>;
> +	nuvoton,mode = <0x7E>; // RTD1_MD = 2
> +};
> -- 
> 1.9.1

Thanks,
Mark.

[1] http://lkml.kernel.org/r/20150907123531.GA11540@leverpostej

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] Documentation: add Device tree bindings for hwmon/nct7802

[Constantine Shulyupin]

>> Add add Device tree bindings for registers
>> which are not covered by hwmon ABI and are required to
>> configure specific HW.
>
> This doesn't really answer my question [1] of why you need these.
>
> We generally don't encode register values unless there's no other way to
> describe the device. Why can we not describe the physical properties of
> the device that make these values necessary?
>
....
> That said, as above I don't believe that these properties make sense
> as they are. I think this needs properties that describe the HW, rather
> than exposing the intricacies of the programming interface.
....
> Thanks,
> Mark.
>
> [1] http://lkml.kernel.org/r/20150907123531.GA11540@leverpostej
>

Sources of the driver:

Let's see to register MODE.
Excerpt from datasheet:

7.2.32 Mode Selection Register
Location : Index 22h
Type : Read/Write
Power on default value : 7Fh
BIT; NAME; FUNC.:
0,1 RTD1_MD : 00=Closed , 01=Current mode , 10=Thermistor mode ,
11=Voltage sense
2,3 RTD2_MD : 00=Closed , 01=Current mode , 10=Thermistor mode ,
11=Voltage sense
4,5 RTD3_MD : 00=Closed , 01=Reserved , 10=Thermistor mode , 11=Voltage sense
6 EnLTD : 1 indicates the LTD function enabled

The register defines sensors types (Closed, Current mode, Thermistor
mode, Voltage sense).
Essential sensors will not work properly with wrong configuration.
It is board or platform depended configuration. It should be
configured on boot only.
Usually hwmon drivers are managed with utility lm-sensors via
Documentation/hwmon/sysfs-interface. By design sensor type should be
defined by platform data and lm-sensors only displays it.

Also MODE registers defines visibility temperature and input attributes:

https://git.kernel.org/cgit/linux/kernel/git/mpe/linux-next.git/tree/drivers/hwmon/nct7802.c#n532

https://git.kernel.org/cgit/linux/kernel/git/mpe/linux-next.git/tree/drivers/hwmon/nct7802.c#n641

Mark, does above answers your question?
What do you propose?

-- 
Thanks
Constantine
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] Documentation: add Device tree bindings for hwmon/nct7802

[Mark Rutland]

On Tue, Sep 08, 2015 at 01:07:30PM +0100, Constantine Shulyupin wrote:
> >> Add add Device tree bindings for registers
> >> which are not covered by hwmon ABI and are required to
> >> configure specific HW.
> >
> > This doesn't really answer my question [1] of why you need these.
> >
> > We generally don't encode register values unless there's no other way to
> > describe the device. Why can we not describe the physical properties of
> > the device that make these values necessary?
> >
> ....
> > That said, as above I don't believe that these properties make sense
> > as they are. I think this needs properties that describe the HW, rather
> > than exposing the intricacies of the programming interface.
> ....
> > Thanks,
> > Mark.
> >
> > [1] http://lkml.kernel.org/r/20150907123531.GA11540@leverpostej
> >
> 
> Sources of the driver:
> 
> Let's see to register MODE.
> Excerpt from datasheet:
> 
> 7.2.32 Mode Selection Register
> Location : Index 22h
> Type : Read/Write
> Power on default value : 7Fh
> BIT; NAME; FUNC.:
> 0,1 RTD1_MD : 00=Closed , 01=Current mode , 10=Thermistor mode ,
> 11=Voltage sense
> 2,3 RTD2_MD : 00=Closed , 01=Current mode , 10=Thermistor mode ,
> 11=Voltage sense
> 4,5 RTD3_MD : 00=Closed , 01=Reserved , 10=Thermistor mode , 11=Voltage sense
> 6 EnLTD : 1 indicates the LTD function enabled
> 
> The register defines sensors types (Closed, Current mode, Thermistor
> mode, Voltage sense).
> Essential sensors will not work properly with wrong configuration.

Sure, but why do you need _raw_ register values?

Why can you not encode this in the DT in a manner that describes what is
connected to this device, then have the kernel figure out any particular
values that need to be poked into registers?

That'll be easier to read, and it's far more likely people will get
things correct in a dts.

Mark.

> It is board or platform depended configuration. It should be
> configured on boot only.
> Usually hwmon drivers are managed with utility lm-sensors via
> Documentation/hwmon/sysfs-interface. By design sensor type should be
> defined by platform data and lm-sensors only displays it.
> 
> Also MODE registers defines visibility temperature and input attributes:
> 
> https://git.kernel.org/cgit/linux/kernel/git/mpe/linux-next.git/tree/drivers/hwmon/nct7802.c#n532
> 
> https://git.kernel.org/cgit/linux/kernel/git/mpe/linux-next.git/tree/drivers/hwmon/nct7802.c#n641
> 
> Mark, does above answers your question?
> What do you propose?
> 
> -- 
> Thanks
> Constantine
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] Documentation: add Device tree bindings for hwmon/nct7802

[Constantine Shulyupin]

> Sure, but why do you need _raw_ register values?
>
> Why can you not encode this in the DT in a manner that describes what is
> connected to this device, then have the kernel figure out any particular
> values that need to be poked into registers?
>
> That'll be easier to read, and it's far more likely people will get
> things correct in a dts.
>
> Mark.

Theoretically it is possible to use function store_temp_type
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/hwmon/nct7802.c#n87
to set sensors types.
With ABI codes:
 3 - thermal diode
 4 - thermistor

But:
- Sensor type could be also voltage and is mapped to in_input
attributes. Generic code would be over complicated.
- For low level programmers and PCB engineers is much more comfortable
and error safe
to works with datasheets, registers and bits than with fancy "human
readable" configuration.
- Mapping between raw bits and ABI is not intuitive. More simple and
directed configuration is preferable.

-- 
Thanks
Constantine

Re: [PATCH v3] Documentation: add Device tree bindings for hwmon/nct7802

[Guenter Roeck]

Constantine,

On 09/08/2015 06:49 AM, Constantine Shulyupin wrote:
>> Sure, but why do you need _raw_ register values?
>>
>> Why can you not encode this in the DT in a manner that describes what is
>> connected to this device, then have the kernel figure out any particular
>> values that need to be poked into registers?
>>
>> That'll be easier to read, and it's far more likely people will get
>> things correct in a dts.
>>
>> Mark.
>
> Theoretically it is possible to use function store_temp_type
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/hwmon/nct7802.c#n87
> to set sensors types.
> With ABI codes:
>   3 - thermal diode
>   4 - thermistor
>
> But:
> - Sensor type could be also voltage and is mapped to in_input
> attributes. Generic code would be over complicated.
> - For low level programmers and PCB engineers is much more comfortable
> and error safe
> to works with datasheets, registers and bits than with fancy "human
> readable" configuration.
> - Mapping between raw bits and ABI is not intuitive. More simple and
> directed configuration is preferable.
>

Mark is asking why you don't have a 'sensor-type' property (or similar).
This is not an ABI issue, since the sensor type is determined by the PCB,
not by user space configuration. The only reason to have the attribute
writable in the ABI is to be able to support hardware where for some
reason the hardware parameters configured by the BIOS or ROMMON
are wrong, not as primary means to set the sensor type. Actually,
in an automated system where the temperature is used for automatic
fan control, waiting for user space to configure the chip might be fatal.

As for your other arguments, pretty much all other devicetree properties
use logical values instead of raw register values. It appears that the
low level programmers and PCB engineers responsible for writing devicetree
files must be able to handle that level of abstraction. And I really
would content that it is easier to write
	sensor-type = "thermistor";
than forcing people to look up the datasheet when writing devicetree
files.

Thanks,
Guenter