Hi Rob,

On Sat, Aug 26, 2017 at 1:35 AM, Rob Herring [off-list ref] wrote:

On Tue, Aug 22, 2017 at 01:26:11PM +0700, s.abhisit@gmail.com wrote:

quoted

From: Abhisit Sangjan <redacted>

TI LMP92001 Analog System Monitor and Controller

8-bit GPIOs.
12 DACs with 12-bit resolution.
The GPIOs and DACs are shared port function with Cy function pin to
take control the pin suddenly from external hardware.
DAC's referance voltage selectable for Internal/External.

16 + 1 ADCs with 12-bit resolution.
Built-in internal Temperature Sensor on channel 17.
Windows Comparator Function is supported on channel 1-3 and 9-11 for
monitoring with interrupt signal (pending to implement for interrupt).
ADC's referance voltage selectable for Internal/External.

Signed-off-by: Abhisit Sangjan <redacted>
---
 Documentation/ABI/testing/sysfs-bus-iio-lmp920001  |  92 ++++
 .../devicetree/bindings/gpio/gpio-lmp92001.txt     |  22 +
 .../bindings/iio/adc/ti-lmp92001-adc.txt           |  21 +
 .../bindings/iio/dac/ti-lmp92001-dac.txt           |  35 ++

quoted

diff --git a/Documentation/devicetree/bindings/gpio/gpio-lmp92001.txt

b/Documentation/devicetree/bindings/gpio/gpio-lmp92001.txt

quoted

new file mode 100644
index 0000000..c68784e

--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-lmp92001.txt

@@ -0,0 +1,22 @@
+* Texas Instruments' LMP92001 GPIOs
+
+Required properties:
+ - compatible: Must be set to "ti,lmp92001-gpio".
+ - reg: i2c chip address for the device.

No, you show this in the parent which needs to be described in
bindings/mtd/...

You could have reg here too if all the registers for each sub-block are
in a well defined range.

I am sorry, I do not understand.

quoted

+ - gpio-controller: Marks the device node as a gpio controller.
+ - #gpio-cells : Should be two.  The first cell is the pin number and

the

quoted

+  second cell is used to specify the gpio polarity:
+        0 = Active high
+        1 = Active low
+
+Example:
+lmp92001@20 {
+        compatible = "ti,lmp92001";
+        reg = <0x20>;
+
+        lmp92001_gpio: lmp92001-gpio {

gpio-controller {

I am sorry, I do not understand. I took this idea from some things
like Documentation/devicetree/bindings/gpio/gpio-lp3943.txt
------------------------------------------------------------------------------------------------------------------------------

TI/National Semiconductor LP3943 GPIO controller

Required properties:
  - compatible: "ti,lp3943-gpio"
  - gpio-controller: Marks the device node as a GPIO controller.
  - #gpio-cells: Should be 2. See gpio.txt in this directory for a
                 description of the cells format.

Example:
Simple LED controls with LP3943 GPIO controller

&i2c4 {
        *lp3943@60* {
                compatible = "ti,lp3943";
                reg = <0x60>;

                *gpioex: gpio* {
                        compatible = "ti,lp3943-gpio";
                        *gpio-controller;*
                        #gpio-cells = <2>;
                };
        };
};

leds {
        compatible = "gpio-leds";
        indicator1 {
                label = "indi1";
                gpios = <&gpioex 9 GPIO_ACTIVE_LOW>;
        };

        indicator2 {
                label = "indi2";
                gpios = <&gpioex 10 GPIO_ACTIVE_LOW>;
                default-state = "off";
        };
};

quoted

+                compatible = "ti,lmp92001-gpio";
+                gpio-controller;
+                #gpio-cells = <2>;
+        };
+};

diff --git a/Documentation/devicetree/bindings/iio/adc/ti-lmp92001-adc.txt

b/Documentation/devicetree/bindings/iio/adc/ti-lmp92001-adc.txt

quoted

new file mode 100644
index 0000000..34d7809

--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/ti-lmp92001-adc.txt

@@ -0,0 +1,21 @@
+* Texas Instruments' LMP92001 ADCs
+
+Required properties:
+ - compatible: Must be set to "ti,lmp92001-adc".
+ - reg: i2c chip address for the device.

Same comment here.

quoted

+ - ti,lmp92001-adc-mode: adc operation, either continuous or

single-shot.

No standard property for this?

I am removed this, regrading to Lukas Wunner (cc) and Adriana Reus
discussed (cc)
"it would be fine also as a sysfs property instead".
-------------------------------------------------------------------------------------------------------
On Sun, Aug 27, 2017 at 6:34 PM, Lukas Wunner [off-list ref] wrote:

On Fri, Aug 25, 2017 at 02:59:41PM -0500, Rob Herring wrote:

quoted

On Tue, Aug 22, 2017 at 03:33:00PM +0200, Lukas Wunner wrote:

quoted

--- a/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
+++ b/Documentation/devicetree/bindings/iio/adc/mcp320x.txt
+Optional properties:
+   - microchip,continuous-conversion (boolean):
+                   Only applicable to MCP3550/1/3:  These ADCs have

long

quoted

quoted

+                   conversion times and therefore support "continuous
+                   conversion mode" to allow retrieval of conversions
+                   at any time without observing a delay.  The mode is
+                   enabled by permanently driving CS low, e.g. by

wiring

quoted

quoted

+                   it to ground.

Second binding I have seen today with a continuous property. Make this
common (or maybe we already have one).

The other one was the TI LMP92001 ADC driver posted by Abhisit Sangjan
(cc), however looking at the datasheet of that chip reveals that
continuous versus one-shot mode is selected by flipping a bit in the
chip's register map.

So it is configurable at run-time.  It's not something that's hardwired.
(Which is the case with the MCP3550 in my patch.)

My understanding was that run-time configurable options should not be
listed in the device-tree at all, only hardware features.  If that is
correct then that device-tree property should be dropped from Abhisit
Sangjan's patch.  Configuring the feature via sysfs is fine I guess.

However we do have another driver supporting continuous versus one-shot
mode and that is drivers/iio/light/us5182d.c by Adriana Reus (cc).
The feature was added with commit c3304c212326.  I'm not sure if it's
hardwired or runtime-configurable, the datasheet is gone from the
manufacturer's website.

For the us5182 continuous versus one-shot is also a configuration in the
chip's registers. I think it would be fine also as a sysfs property instead.

I agree that a common "continuous" property makes sense.  We haven't
defined any common IIO properties so far and that has already led to
inconsistencies.  E.g. most ADC/DAC drivers name the reference regulator
"vref-supply", but e.g. drivers/iio/dac/ad7303.c calls it "REF-supply".

What do you think of this:

-- >8 --
Subject: [PATCH] dt-bindings: iio: Document common properties

It's about time we standardize on common names for frequently used IIO
properties.  For starters, document "vref-supply" and "continuous".

Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 Documentation/devicetree/bindings/iio/iio-bindings.txt | 15

+++++++++++++++

b/Documentation/devicetree/bindings/iio/iio-bindings.txt

device.

                io-channels = <&adc 10>, <&adc 11>;
                io-channel-names = "adc1", "adc2";
        };
+
+==Common IIO properties==
+
+Reference voltage:
+ADCs, DACs and several other IIO devices require a reference voltage.
+By convention the property specifying this regulator is named

"vref-supply".

+If the chip lacks a dedicated Vref pin and instead uses its own power

supply

+as reference, the property specifying the regulator is commonly named
+"vdd-supply" or "vcc-supply".
+
+Continuous mode:
+Some sensors can be configured to perform continuous (versus one-shot)
+measurements.  Continuous mode may require more energy in return for

faster

+or more reliable measurements.  A boolean property named "continuous"
+signifies that the device is configured for this mode.

quoted

+ - ti,lmp92001-adc-mask: bit mask for which channel is enable.
+        0 = Off
+        1 = On
+
+Example:
+lmp92001@20 {
+        compatible = "ti,lmp92001";
+        reg = <0x20>;
+
+        lmp92001-adc {
+                compatible = "ti,lmp92001-adc";
+                ti,lmp92001-adc-mode = "continuous";
+                ti,lmp92001-adc-mask = <0x00000079>;
+        };
+};
\ No newline at end of file

diff --git a/Documentation/devicetree/bindings/iio/dac/ti-lmp92001-dac.txt

b/Documentation/devicetree/bindings/iio/dac/ti-lmp92001-dac.txt

quoted

new file mode 100644
index 0000000..882db9c

--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/ti-lmp92001-dac.txt

@@ -0,0 +1,35 @@
+* Texas Instruments' LMP92001 DACs
+
+Required properties:
+ - compatible: Must be set to "ti,lmp92001-dac".
+ - reg: i2c chip address for the device.
+ - ti,lmp92001-dac-hiz: hi-impedance control,
+        1 = Forces all OUT[1:12] outputs to hi-z, 0 = normal

Just make this a boolean (i.e. no value).

Hi-Z will be or to cdac and it is unsigned int, u8 would be safe and work
fine for this.

lmp92001_dac_probe()
...
u8 gang = 0, outx = 0, hiz = 0;
unsigned int cdac = 0;
...
of_property_read_u8(np, "ti,lmp92001-dac-hiz", &hiz);
cdac = hiz;

quoted

+ - ti,lmp92001-dac-outx:
+        Cy = 0, 1 = will force associated OUTx outputs to VDD
+        Cy = 0, 0 = will force associated OUTx outputs to GND
+ - ti,lmp92001-dac-gang: What group of Cy is governed to.
+        -----------------------------------------
+        |  Cy   | CDAC:GANG = 0 | CDAC:GANG = 1 |
+        -----------------------------------------
+        |  C1   |   OUT[1:4]    |    OUT[1:3]   |
+        -----------------------------------------
+        |  C2   |   OUT[5:6]    |    OUT[4:6]   |
+        -----------------------------------------
+        |  C3   |   OUT[7:8]    |    OUT[7:9]   |
+        -----------------------------------------
+        |  C4   |   OUT[9:12]   |    OUT[10:12] |
+        -----------------------------------------
+
+Example:
+lmp92001@20 {
+        compatible = "ti,lmp92001";
+        reg = <0x20>;
+
+        lmp92001-dac {
+                compatible = "ti,lmp92001-dac";
+                ti,lmp92001-dac-hiz  = /bits/ 8 <0>;
+                ti,lmp92001-dac-outx = /bits/ 8 <0>;
+                ti,lmp92001-dac-gang = /bits/ 8 <0>;
+        };
+};

Thank you for your review,
Abhisit.