Re: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911
On Wed, Jul 25, 2018 at 1:04 PM Marcus Folkesson wrote: > > Hello Rob, > > Thank you for the review. > > On Wed, Jul 25, 2018 at 11:51:17AM -0600, Rob Herring wrote: > > On Tue, Jul 24, 2018 at 08:30:03PM +0200, Marcus Folkesson wrote: > > > MCP3911 is a dual channel Analog Front End (AFE) containing two > > > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC). > > > > > > Signed-off-by: Marcus Folkesson > > > Signed-off-by: Kent Gustavsson > > > --- > > > > > > Notes: > > > v2: > > > - drop channel width > > > - drop `external_vref` > > > - replace `external-clock` with proper clock bindings > > > > > > .../devicetree/bindings/iio/adc/mcp3911.txt| 28 > > > ++ > > > 1 file changed, 28 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > > b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > > new file mode 100644 > > > index ..af5472f51a84 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > > @@ -0,0 +1,28 @@ > > > +* Microchip MCP3911 Dual channel analog front end (ADC) > > > + > > > +Required properties: > > > + - compatible: Should be "microchip,mcp3911" > > > + - reg: SPI chip select number for the device > > > + > > > +Recommended properties: > > > + - spi-max-frequency: Definition as per > > > +Documentation/devicetree/bindings/spi/spi-bus.txt. > > > +Max frequency for this chip is 20MHz. > > > + > > > +Optional properties: > > > + - device-addr: Device address when multiple MCP3911 chips are present > > > on the > > > + same SPI bus. Valid values are 0-3. Defaults to 0. > > > > Isn't this the reg value? > > > > The reg value defines which chip select the chip hangs on. > The chip has support to connect up to four chips on the same SPI bus, > including the same chip select signal. > > The chips are separated by the device-addr as it is part of the > communication protocol. Okay, seems pretty specific to this chip. Just add a vendor prefix. > When reading the description for device-addr, I agree that it fits well > for the reg property as well.. Except that reg format is already defined for SPI devices, so it's going to have to be separate property. > > > + - vref-supply: Phandle to the external reference voltage supply. > > > + - clocks: Phandle and clock identifier (see clock-names) > > > + - clock-names: "adc_clk" for the ADC (sampling) clock > > > > Datasheet calls this clki (or mclk internally). Or just drop clock-names > > as it is pointless > > when there is only 1 clock. > > I will drop the clock-names, thanks! > > > > > Also DR handling as an IRQ is missing. > > I have considered using the DR signal, but as we not support buffering > yet I did not see any point in using it. For the binding it doesn't matter if a particular driver uses it or not. If it's connected in a h/w design it should be in DT. Rob
Re: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911
On Wed, Jul 25, 2018 at 1:04 PM Marcus Folkesson wrote: > > Hello Rob, > > Thank you for the review. > > On Wed, Jul 25, 2018 at 11:51:17AM -0600, Rob Herring wrote: > > On Tue, Jul 24, 2018 at 08:30:03PM +0200, Marcus Folkesson wrote: > > > MCP3911 is a dual channel Analog Front End (AFE) containing two > > > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC). > > > > > > Signed-off-by: Marcus Folkesson > > > Signed-off-by: Kent Gustavsson > > > --- > > > > > > Notes: > > > v2: > > > - drop channel width > > > - drop `external_vref` > > > - replace `external-clock` with proper clock bindings > > > > > > .../devicetree/bindings/iio/adc/mcp3911.txt| 28 > > > ++ > > > 1 file changed, 28 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > > b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > > new file mode 100644 > > > index ..af5472f51a84 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > > @@ -0,0 +1,28 @@ > > > +* Microchip MCP3911 Dual channel analog front end (ADC) > > > + > > > +Required properties: > > > + - compatible: Should be "microchip,mcp3911" > > > + - reg: SPI chip select number for the device > > > + > > > +Recommended properties: > > > + - spi-max-frequency: Definition as per > > > +Documentation/devicetree/bindings/spi/spi-bus.txt. > > > +Max frequency for this chip is 20MHz. > > > + > > > +Optional properties: > > > + - device-addr: Device address when multiple MCP3911 chips are present > > > on the > > > + same SPI bus. Valid values are 0-3. Defaults to 0. > > > > Isn't this the reg value? > > > > The reg value defines which chip select the chip hangs on. > The chip has support to connect up to four chips on the same SPI bus, > including the same chip select signal. > > The chips are separated by the device-addr as it is part of the > communication protocol. Okay, seems pretty specific to this chip. Just add a vendor prefix. > When reading the description for device-addr, I agree that it fits well > for the reg property as well.. Except that reg format is already defined for SPI devices, so it's going to have to be separate property. > > > + - vref-supply: Phandle to the external reference voltage supply. > > > + - clocks: Phandle and clock identifier (see clock-names) > > > + - clock-names: "adc_clk" for the ADC (sampling) clock > > > > Datasheet calls this clki (or mclk internally). Or just drop clock-names > > as it is pointless > > when there is only 1 clock. > > I will drop the clock-names, thanks! > > > > > Also DR handling as an IRQ is missing. > > I have considered using the DR signal, but as we not support buffering > yet I did not see any point in using it. For the binding it doesn't matter if a particular driver uses it or not. If it's connected in a h/w design it should be in DT. Rob
Re: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911
Hello Rob, Thank you for the review. On Wed, Jul 25, 2018 at 11:51:17AM -0600, Rob Herring wrote: > On Tue, Jul 24, 2018 at 08:30:03PM +0200, Marcus Folkesson wrote: > > MCP3911 is a dual channel Analog Front End (AFE) containing two > > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC). > > > > Signed-off-by: Marcus Folkesson > > Signed-off-by: Kent Gustavsson > > --- > > > > Notes: > > v2: > > - drop channel width > > - drop `external_vref` > > - replace `external-clock` with proper clock bindings > > > > .../devicetree/bindings/iio/adc/mcp3911.txt| 28 > > ++ > > 1 file changed, 28 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > new file mode 100644 > > index ..af5472f51a84 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > @@ -0,0 +1,28 @@ > > +* Microchip MCP3911 Dual channel analog front end (ADC) > > + > > +Required properties: > > + - compatible: Should be "microchip,mcp3911" > > + - reg: SPI chip select number for the device > > + > > +Recommended properties: > > + - spi-max-frequency: Definition as per > > +Documentation/devicetree/bindings/spi/spi-bus.txt. > > +Max frequency for this chip is 20MHz. > > + > > +Optional properties: > > + - device-addr: Device address when multiple MCP3911 chips are present on > > the > > + same SPI bus. Valid values are 0-3. Defaults to 0. > > Isn't this the reg value? > The reg value defines which chip select the chip hangs on. The chip has support to connect up to four chips on the same SPI bus, including the same chip select signal. The chips are separated by the device-addr as it is part of the communication protocol. When reading the description for device-addr, I agree that it fits well for the reg property as well.. > > + - vref-supply: Phandle to the external reference voltage supply. > > + - clocks: Phandle and clock identifier (see clock-names) > > + - clock-names: "adc_clk" for the ADC (sampling) clock > > Datasheet calls this clki (or mclk internally). Or just drop clock-names > as it is pointless > when there is only 1 clock. I will drop the clock-names, thanks! > > Also DR handling as an IRQ is missing. I have considered using the DR signal, but as we not support buffering yet I did not see any point in using it. From the datasheet, section 7.1 :: These registers [channel registers] are latched when an ADC read communication occurs. When a data ready event occurs during a read communication, the most current ADC data is also latched to avoid data corruption issues. The three bytes of each channel are updated synchronously at a DRCLK rate. The three bytes can be accessed separately if needed but are refreshed synchronously. So it should be safe to read the values independently of the DR signal. Or maybe I'm missing something. Best regards Marcus Folkesson signature.asc Description: PGP signature
Re: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911
Hello Rob, Thank you for the review. On Wed, Jul 25, 2018 at 11:51:17AM -0600, Rob Herring wrote: > On Tue, Jul 24, 2018 at 08:30:03PM +0200, Marcus Folkesson wrote: > > MCP3911 is a dual channel Analog Front End (AFE) containing two > > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC). > > > > Signed-off-by: Marcus Folkesson > > Signed-off-by: Kent Gustavsson > > --- > > > > Notes: > > v2: > > - drop channel width > > - drop `external_vref` > > - replace `external-clock` with proper clock bindings > > > > .../devicetree/bindings/iio/adc/mcp3911.txt| 28 > > ++ > > 1 file changed, 28 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > > > diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > new file mode 100644 > > index ..af5472f51a84 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > @@ -0,0 +1,28 @@ > > +* Microchip MCP3911 Dual channel analog front end (ADC) > > + > > +Required properties: > > + - compatible: Should be "microchip,mcp3911" > > + - reg: SPI chip select number for the device > > + > > +Recommended properties: > > + - spi-max-frequency: Definition as per > > +Documentation/devicetree/bindings/spi/spi-bus.txt. > > +Max frequency for this chip is 20MHz. > > + > > +Optional properties: > > + - device-addr: Device address when multiple MCP3911 chips are present on > > the > > + same SPI bus. Valid values are 0-3. Defaults to 0. > > Isn't this the reg value? > The reg value defines which chip select the chip hangs on. The chip has support to connect up to four chips on the same SPI bus, including the same chip select signal. The chips are separated by the device-addr as it is part of the communication protocol. When reading the description for device-addr, I agree that it fits well for the reg property as well.. > > + - vref-supply: Phandle to the external reference voltage supply. > > + - clocks: Phandle and clock identifier (see clock-names) > > + - clock-names: "adc_clk" for the ADC (sampling) clock > > Datasheet calls this clki (or mclk internally). Or just drop clock-names > as it is pointless > when there is only 1 clock. I will drop the clock-names, thanks! > > Also DR handling as an IRQ is missing. I have considered using the DR signal, but as we not support buffering yet I did not see any point in using it. From the datasheet, section 7.1 :: These registers [channel registers] are latched when an ADC read communication occurs. When a data ready event occurs during a read communication, the most current ADC data is also latched to avoid data corruption issues. The three bytes of each channel are updated synchronously at a DRCLK rate. The three bytes can be accessed separately if needed but are refreshed synchronously. So it should be safe to read the values independently of the DR signal. Or maybe I'm missing something. Best regards Marcus Folkesson signature.asc Description: PGP signature
Re: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911
On Tue, Jul 24, 2018 at 08:30:03PM +0200, Marcus Folkesson wrote: > MCP3911 is a dual channel Analog Front End (AFE) containing two > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC). > > Signed-off-by: Marcus Folkesson > Signed-off-by: Kent Gustavsson > --- > > Notes: > v2: > - drop channel width > - drop `external_vref` > - replace `external-clock` with proper clock bindings > > .../devicetree/bindings/iio/adc/mcp3911.txt| 28 > ++ > 1 file changed, 28 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > new file mode 100644 > index ..af5472f51a84 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > @@ -0,0 +1,28 @@ > +* Microchip MCP3911 Dual channel analog front end (ADC) > + > +Required properties: > + - compatible: Should be "microchip,mcp3911" > + - reg: SPI chip select number for the device > + > +Recommended properties: > + - spi-max-frequency: Definition as per > + Documentation/devicetree/bindings/spi/spi-bus.txt. > + Max frequency for this chip is 20MHz. > + > +Optional properties: > + - device-addr: Device address when multiple MCP3911 chips are present on the > + same SPI bus. Valid values are 0-3. Defaults to 0. Isn't this the reg value? > + - vref-supply: Phandle to the external reference voltage supply. > + - clocks: Phandle and clock identifier (see clock-names) > + - clock-names: "adc_clk" for the ADC (sampling) clock Datasheet calls this clki (or mclk internally). Or just drop clock-names as it is pointless when there is only 1 clock. Also DR handling as an IRQ is missing.
Re: [PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911
On Tue, Jul 24, 2018 at 08:30:03PM +0200, Marcus Folkesson wrote: > MCP3911 is a dual channel Analog Front End (AFE) containing two > synchronous sampling delta-sigma Analog-to-Digital Converters (ADC). > > Signed-off-by: Marcus Folkesson > Signed-off-by: Kent Gustavsson > --- > > Notes: > v2: > - drop channel width > - drop `external_vref` > - replace `external-clock` with proper clock bindings > > .../devicetree/bindings/iio/adc/mcp3911.txt| 28 > ++ > 1 file changed, 28 insertions(+) > create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt > > diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > new file mode 100644 > index ..af5472f51a84 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt > @@ -0,0 +1,28 @@ > +* Microchip MCP3911 Dual channel analog front end (ADC) > + > +Required properties: > + - compatible: Should be "microchip,mcp3911" > + - reg: SPI chip select number for the device > + > +Recommended properties: > + - spi-max-frequency: Definition as per > + Documentation/devicetree/bindings/spi/spi-bus.txt. > + Max frequency for this chip is 20MHz. > + > +Optional properties: > + - device-addr: Device address when multiple MCP3911 chips are present on the > + same SPI bus. Valid values are 0-3. Defaults to 0. Isn't this the reg value? > + - vref-supply: Phandle to the external reference voltage supply. > + - clocks: Phandle and clock identifier (see clock-names) > + - clock-names: "adc_clk" for the ADC (sampling) clock Datasheet calls this clki (or mclk internally). Or just drop clock-names as it is pointless when there is only 1 clock. Also DR handling as an IRQ is missing.
[PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911
MCP3911 is a dual channel Analog Front End (AFE) containing two
synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
Signed-off-by: Marcus Folkesson
Signed-off-by: Kent Gustavsson
---
Notes:
v2:
- drop channel width
- drop `external_vref`
- replace `external-clock` with proper clock bindings
.../devicetree/bindings/iio/adc/mcp3911.txt| 28 ++
1 file changed, 28 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt
diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
new file mode 100644
index ..af5472f51a84
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
@@ -0,0 +1,28 @@
+* Microchip MCP3911 Dual channel analog front end (ADC)
+
+Required properties:
+ - compatible: Should be "microchip,mcp3911"
+ - reg: SPI chip select number for the device
+
+Recommended properties:
+ - spi-max-frequency: Definition as per
+Documentation/devicetree/bindings/spi/spi-bus.txt.
+Max frequency for this chip is 20MHz.
+
+Optional properties:
+ - device-addr: Device address when multiple MCP3911 chips are present on the
+ same SPI bus. Valid values are 0-3. Defaults to 0.
+ - vref-supply: Phandle to the external reference voltage supply.
+ - clocks: Phandle and clock identifier (see clock-names)
+ - clock-names: "adc_clk" for the ADC (sampling) clock
+
+Example:
+adc@0 {
+ compatible = "microchip,mcp3911";
+ reg = <0>;
+ spi-max-frequency = <2000>;
+ device-addr = <0>;
+ vref-supply = <_reg>;
+ clocks = <>;
+ clock-names = "adc_clk";
+};
--
2.11.0.rc2
[PATCH v2 2/3] dt-bindings: iio: adc: add bindings for mcp3911
MCP3911 is a dual channel Analog Front End (AFE) containing two
synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
Signed-off-by: Marcus Folkesson
Signed-off-by: Kent Gustavsson
---
Notes:
v2:
- drop channel width
- drop `external_vref`
- replace `external-clock` with proper clock bindings
.../devicetree/bindings/iio/adc/mcp3911.txt| 28 ++
1 file changed, 28 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt
diff --git a/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
new file mode 100644
index ..af5472f51a84
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/mcp3911.txt
@@ -0,0 +1,28 @@
+* Microchip MCP3911 Dual channel analog front end (ADC)
+
+Required properties:
+ - compatible: Should be "microchip,mcp3911"
+ - reg: SPI chip select number for the device
+
+Recommended properties:
+ - spi-max-frequency: Definition as per
+Documentation/devicetree/bindings/spi/spi-bus.txt.
+Max frequency for this chip is 20MHz.
+
+Optional properties:
+ - device-addr: Device address when multiple MCP3911 chips are present on the
+ same SPI bus. Valid values are 0-3. Defaults to 0.
+ - vref-supply: Phandle to the external reference voltage supply.
+ - clocks: Phandle and clock identifier (see clock-names)
+ - clock-names: "adc_clk" for the ADC (sampling) clock
+
+Example:
+adc@0 {
+ compatible = "microchip,mcp3911";
+ reg = <0>;
+ spi-max-frequency = <2000>;
+ device-addr = <0>;
+ vref-supply = <_reg>;
+ clocks = <>;
+ clock-names = "adc_clk";
+};
--
2.11.0.rc2
