iio: adc: add support for mcp3911
Subsystems (3)
Marcus Folkesson
maintainerPATCH v3 1/3 iio: adc: add support for mcp3911
MCP3911 is a dual channel Analog Front End (AFE) containing two synchronous sampling delta-sigma Analog-to-Digital Converters (ADC).
---
Notes:
v3:
- rename adc_clk to clki
- add error handling/cleanup for clock
v2:
- cleanups and bugfixes (thanks Peter Meerwald-Stadler)
- drop hardware gain
- use the presence or lack of regulator to indicate if we go for internal or external voltage reference
- do not store device node in private struct
- drop support to set width in devicetree
- use the presence or lack of clock to indicate if we go for internal or external clock
drivers/iio/adc/Kconfig | 10 ++ drivers/iio/adc/Makefile | 1 + drivers/iio/adc/mcp3911.c | 367 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 378 insertions(+) create mode 100644 drivers/iio/adc/mcp3911.c
drivers/iio/adc/Kconfig +10 -0
@@ -501,6 +501,16 @@ config MCP3422 This driver can also be built as a module. If so, the module will be called mcp3422. +config MCP3911 + tristate "Microchip Technology MCP3911 driver" + depends on SPI + help + Say yes here to build support for Microchip Technology's MCP3911 + analog to digital converter. + + This driver can also be built as a module. If so, the module will be + called mcp3911. + config MEDIATEK_MT6577_AUXADC tristate "MediaTek AUXADC driver" depends on ARCH_MEDIATEK || COMPILE_TEST
drivers/iio/adc/Makefile +1 -0
@@ -47,6 +47,7 @@ obj-$(CONFIG_MAX1363) += max1363.o obj-$(CONFIG_MAX9611) += max9611.o obj-$(CONFIG_MCP320X) += mcp320x.o obj-$(CONFIG_MCP3422) += mcp3422.o +obj-$(CONFIG_MCP3911) += mcp3911.o obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o obj-$(CONFIG_MESON_SARADC) += meson_saradc.o
drivers/iio/adc/mcp3911.c new file +367 -0
367 lines not on this page · raw message
Signature · 1 line
2.11.0.rc2
Marcus Folkesson
maintainerPATCH v3 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).
---
Notes:
v3:
- add bindings for interrupt
- prefix device-addr with microchip
- drop clock-names
v2:
- drop channel width
- drop external_vref
- replace external-clock with proper clock bindings
.../devicetree/bindings/iio/adc/mcp3911.txt | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txt
Documentation/devicetree/bindings/iio/adc/mcp3911.txt new file +32 -0
@@ -0,0 +1,32 @@ +* 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: + - clocks: Phandle and clock identifier for sampling clock + - interrupt-parent: Phandle to the parent interrupt controller + see: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt + - interrupts: IRQ line for the ADC + see: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt + - microchip,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. + +Example: +adc@0 { + compatible = "microchip,mcp3911"; + reg = <0>; + interrupt-parent = <&gpio5>; + interrupts = <15 IRQ_TYPE_EDGE_RISING>; + spi-max-frequency = <20000000>; + microchip,device-addr = <0>; + vref-supply = <&vref_reg>; + clocks = <&xtal>; +};
Signature · 1 line
2.11.0.rc2
Re: [PATCH v3 2/3] dt-bindings: iio: adc: add bindings for mcp3911
Quoted 32 lines · Marcus Folkesson
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 <[email protected]>Signed-off-by Kent Gustavsson <[email protected]>---
Notes:
v3:
- add bindings for interrupt
- prefix device-addr withmicrochip
- dropclock-names
v2:
- drop channel width
- dropexternal_vref
- replaceexternal-clockwith proper clock bindings.../devicetree/bindings/iio/adc/mcp3911.txt | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/mcp3911.txtDocumentation/devicetree/bindings/iio/adc/mcp3911.txt new file +5
-0@@ -0,0 +1,32 @@ +* Microchip MCP3911 Dual channel analog front end (ADC) + +Required properties: + - compatible: Should be "microchip,mcp3911" + - reg: SPI chip select number for the device
Documentation/devicetree/bindings/iio/adc/mcp3911.txt line 15
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/mcp3911.txt new file +10
-0+ +Recommended properties: + - spi-max-frequency: Definition as per + Documentation/devicetree/bindings/spi/spi-bus.txt. + Max frequency for this chip is 20MHz. + +Optional properties: + - clocks: Phandle and clock identifier for sampling clock + - interrupt-parent: Phandle to the parent interrupt controller + see: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
Drop this. It is implied.
With that,
Quoted 20 lines
Documentation/devicetree/bindings/iio/adc/mcp3911.txt new file +17
-0+ - interrupts: IRQ line for the ADC + see: Documentation/devicetree/bindings/interrupt-controller/interrupts.txt + - microchip,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. + +Example: +adc@0 { + compatible = "microchip,mcp3911"; + reg = <0>; + interrupt-parent = <&gpio5>; + interrupts = <15 IRQ_TYPE_EDGE_RISING>; + spi-max-frequency = <20000000>; + microchip,device-addr = <0>; + vref-supply = <&vref_reg>; + clocks = <&xtal>; +};Signature · 1 line
2.11.0.rc2
Marcus Folkesson
maintainerPATCH v3 3/3 MAINTAINERS: Add entry for mcp3911 ADC driver
In reply to Marcus FolkessonAdd an entry for mcp3911 ADC driver and add myself and Kent Gustavsson as maintainers of this driver.
---
Notes:
v3:
- no changes
v2:
- no changes
MAINTAINERS | 8 ++++++++ 1 file changed, 8 insertions(+)
MAINTAINERS +8 -0
@@ -9271,6 +9271,14 @@ L: [email protected] S: Maintained F: drivers/net/ethernet/microchip/lan743x_* +MICROCHIP / ATMEL MCP3911 ADC DRIVER +M: Marcus Folkesson <[email protected]> +M: Kent Gustavsson <[email protected]> +L: [email protected] +S: Supported +F: drivers/iio/adc/mcp3911.c +F: Documentation/devicetree/bindings/iio/adc/mcp3911.txt + MICROCHIP USB251XB DRIVER M: Richard Leitner <[email protected]> L: [email protected]
Signature · 1 line
2.11.0.rc2
Quoted 6 lines · Marcus Folkesson
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 <[email protected]>Signed-off-by Kent Gustavsson <[email protected]>
What is this? Why it's here (presense and location in the message)?
Quoted 3 lines
+ * Copyright (C) 2018 Kent Gustavsson <[email protected]>
+ *
Redundant.
Quoted 16 lines
drivers/iio/adc/mcp3911.c new file +16
-0+static int mcp3911_read (struct mcp3911 *adc,u8 reg,u32 *val,u8 len) +{ +int ret; + + reg =MCP3911_REG_READ (reg, adc->dev_addr); + ret =spi_write_then_read (adc->spi, ®,1 , val, len); +if (ret <0 ) +return ret; + +be32_to_cpus (val); + *val >>= ((4 - len) *8 ); +dev_dbg (&adc->spi->dev,"reading 0x%x from register 0x%x\n" , *val, + reg>>1 ); +return ret; +} +
drivers/iio/adc/mcp3911.c line 83
Quoted 10 lines
drivers/iio/adc/mcp3911.c new file +10
-0+static int mcp3911_write (struct mcp3911 *adc,u8 reg,u32 val,u8 len) +{ +dev_dbg (&adc->spi->dev,"writing 0x%x to register 0x%x\n" , val, reg); + + val <<= (3 - len) *8 ; +cpu_to_be32s (&val); + val |=MCP3911_REG_WRITE (reg, adc->dev_addr); + +return spi_write (adc->spi, &val, len +1 ); +}
Can't you use REGMAP_SPI ?
drivers/iio/adc/mcp3911.c line 216
Quoted 8 lines
drivers/iio/adc/mcp3911.c new file +8
-0+of_property_read_u32 (of_node,"device-addr" , &adc->dev_addr); +if (adc->dev_addr >3 ) { +dev_err (&adc->spi->dev, +"invalid device address (%i). Must be in range 0-3.\n" , + adc->dev_addr); +return -EINVAL ; + } +dev_dbg (&adc->spi->dev,"use device address %i\n" , adc->dev_addr);
Isn't what we called CS (chip select)?
drivers/iio/adc/mcp3911.c line 256
Quoted 3 lines
drivers/iio/adc/mcp3911.c new file +2
-0+if (IS_ERR (adc->vref)) { +
Redundant.
Quoted 1 line
+ if (
adc->clki)
Seems to be redundant if clock is optional (it should be NULL and API is NULL-aware).
drivers/iio/adc/mcp3911.c line 334
Quoted 4 lines
drivers/iio/adc/mcp3911.c new file +3
-0+clk_disable_unprepare (adc->clki); +if (adc->clki) +clk_disable_unprepare (adc->clki);
Ditto.
Quoted 1 line
+#if
defined(CONFIG_OF)
This prevents playing with the device on ACPI enabled platforms.
Quoted 1 line
+ .of_match_table =
of_match_ptr(mcp3911_dt_ids),
Ditto for macro.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerQuoted 10 lines · On Thu, 2 Aug 2018 22:52:00 +0300 Andy Shevchenko <[email protected]>…
Quoted 6 lines · Marcus Folkesson
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 <[email protected]>Signed-off-by Kent Gustavsson <[email protected]>What is this? Why it's here (presense and location in the message)?
To clarify... If Kent wrote the patch and you are simply acting as gatekeeper / upstreamer you should set the mail to be from Kent and put your own Signed-off after his to basically act as a submaintainer certifying you believe his sign off and all that entails.
If it is a bit of a joint effort then that's fine but for copyright purposes there should be some indication of the split.
Quoted 27 lines
Quoted 4 lines
+ * Copyright (C) 2018 Kent Gustavsson <[email protected]>
+ *
Redundant.
Quoted 20 lines
drivers/iio/adc/mcp3911.c new file +20
-0+static int mcp3911_read (struct mcp3911 *adc,u8 reg,u32 *val,u8 len) +{ +int ret; + + reg =MCP3911_REG_READ (reg, adc->dev_addr); + ret =spi_write_then_read (adc->spi, ®,1 , val, len); +if (ret <0 ) +return ret; + +be32_to_cpus (val); + *val >>= ((4 - len) *8 ); +dev_dbg (&adc->spi->dev,"reading 0x%x from register 0x%x\n" , *val, + reg>>1 ); +return ret; +} + +static int mcp3911_write (struct mcp3911 *adc,u8 reg,u32 val,u8 len) +{ +dev_dbg (&adc->spi->dev,"writing 0x%x to register 0x%x\n" , val, reg); +
Quoted 8 lines
drivers/iio/adc/mcp3911.c line 83
Quoted 6 lines
drivers/iio/adc/mcp3911.c new file +6
-0+ val <<= (3 - len) *8 ; +cpu_to_be32s (&val); + val |=MCP3911_REG_WRITE (reg, adc->dev_addr); + +return spi_write (adc->spi, &val, len +1 ); +}Can't you use
REGMAP_SPI?
My instinct is not really. The magic device-addr doesn't help. You could work around it but it's not that nice and you'd have to create the regmap mapping on the fly or carry static ones for each value of htat.
Quoted 11 lines
drivers/iio/adc/mcp3911.c line 216
Quoted 9 lines
drivers/iio/adc/mcp3911.c new file +8
-0+of_property_read_u32 (of_node,"device-addr" , &adc->dev_addr); +if (adc->dev_addr >3 ) { +dev_err (&adc->spi->dev, +"invalid device address (%i). Must be in range 0-3.\n" , + adc->dev_addr); +return -EINVAL ; + } +dev_dbg (&adc->spi->dev,"use device address %i\n" , adc->dev_addr);Isn't what we called CS (chip select)?
Nope. We went around this in an earlier revision. It's an address transmitted in the control byte to allow you to 'share' a chip select line between multiple chips (crazy but true).
Quoted 14 lines
drivers/iio/adc/mcp3911.c line 256
Quoted 4 lines
drivers/iio/adc/mcp3911.c new file +2
-0+if (IS_ERR (adc->vref)) { +Redundant.
Quoted 1 line
+ if (
adc->clki)Seems to be redundant if clock is optional (it should be NULL and API is NULL-aware).
Quoted 1 line
+
clk_disable_unprepare(adc->clki);
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ if (adc->clki) + clk_disable_unprepare(adc->clki);Ditto.
Quoted 1 line
+#if
defined(CONFIG_OF)This prevents playing with the device on ACPI enabled platforms.
Yeah, that trickery is still little known (and I forget about it from time to time).
The upshot for those who have missed this is you can use a magic acpi device to instantiate based on device tree bindings :)
So we need to strip all this 'obvious' protection stuff out of drivers.
Quoted 5 lines
Quoted 2 lines
+ .of_match_table =
of_match_ptr(mcp3911_dt_ids),Ditto for macro.
Marcus Folkesson
maintainerHi Andy and Jonathan,
Quoted 12 lines · Jonathan Cameron
Quoted 9 lines · On Thu, 2 Aug 2018 22:52:00 +0300 Andy Shevchenko <[email protected]>…
Quoted 6 lines · Marcus Folkesson
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 <[email protected]>Signed-off-by Kent Gustavsson <[email protected]>
Quoted 8 lines
Quoted 1 line
What is this? Why it's here (presense and location in the message)?
To clarify... If Kent wrote the patch and you are simply acting as gatekeeper / upstreamer you should set the mail to be from Kent and put your own Signed-off after his to basically act as a submaintainer certifying you believe his sign off and all that entails.
If it is a bit of a joint effort then that's fine but for copyright purposes there should be some indication of the split.
First, thank you Andy for noticing.
I actually intended to use Co-Developed-by (a pretty new tag) in combination with Signed-off-by.
But the tag must have disappeared in some preparation stage..
From Documentation/process/submitting-patches.rst ::
A Co-Developed-by: states that the patch was also created by another developer along with the original author. This is useful at times when multiple people work on a single patch. Note, this person also needs to have a Signed-off-by:
line in the patch as well.
I will switch order and add the Co-Developed-by-tag. Is this correct?
Quoted 54 lines
Quoted 29 lines
Quoted 6 lines
+ * Copyright (C) 2018 Kent Gustavsson <[email protected]>
+ *
Redundant.
Quoted 20 lines
drivers/iio/adc/mcp3911.c new file +20
-0+static int mcp3911_read (struct mcp3911 *adc,u8 reg,u32 *val,u8 len) +{ +int ret; + + reg =MCP3911_REG_READ (reg, adc->dev_addr); + ret =spi_write_then_read (adc->spi, ®,1 , val, len); +if (ret <0 ) +return ret; + +be32_to_cpus (val); + *val >>= ((4 - len) *8 ); +dev_dbg (&adc->spi->dev,"reading 0x%x from register 0x%x\n" , *val, + reg>>1 ); +return ret; +} + +static int mcp3911_write (struct mcp3911 *adc,u8 reg,u32 val,u8 len) +{ +dev_dbg (&adc->spi->dev,"writing 0x%x to register 0x%x\n" , val, reg); +Quoted 8 lines
drivers/iio/adc/mcp3911.c line 83
Quoted 6 lines
drivers/iio/adc/mcp3911.c new file +6
-0+ val <<= (3 - len) *8 ; +cpu_to_be32s (&val); + val |=MCP3911_REG_WRITE (reg, adc->dev_addr); + +return spi_write (adc->spi, &val, len +1 ); +}Can't you use
REGMAP_SPI?My instinct is not really. The magic device-addr doesn't help. You could work around it but it's not that nice and you'd have to create the regmap mapping on the fly or carry static ones for each value of htat.
Quoted 10 lines
Quoted 9 lines
drivers/iio/adc/mcp3911.c new file +8
-0+of_property_read_u32 (of_node,"device-addr" , &adc->dev_addr); +if (adc->dev_addr >3 ) { +dev_err (&adc->spi->dev, +"invalid device address (%i). Must be in range 0-3.\n" , + adc->dev_addr); +return -EINVAL ; + } +dev_dbg (&adc->spi->dev,"use device address %i\n" , adc->dev_addr);
Quoted 27 lines
Quoted 1 line
Isn't what we called CS (chip select)?
Nope. We went around this in an earlier revision. It's an address transmitted in the control byte to allow you to 'share' a chip select line between multiple chips (crazy but true).
Quoted 22 lines
drivers/iio/adc/mcp3911.c line 256
Quoted 4 lines
drivers/iio/adc/mcp3911.c new file +2
-0+if (IS_ERR (adc->vref)) { +Redundant.
Quoted 1 line
+ if (
adc->clki)Seems to be redundant if clock is optional (it should be NULL and API is NULL-aware).
drivers/iio/adc/mcp3911.c line 334
Quoted 4 lines
drivers/iio/adc/mcp3911.c new file +3
-0+clk_disable_unprepare (adc->clki); +if (adc->clki) +clk_disable_unprepare (adc->clki);Ditto.
Quoted 1 line
+#if
defined(CONFIG_OF)This prevents playing with the device on ACPI enabled platforms.
I will remove the defined(CONFIG_OF) and not use the of_match_ptr() macro.
Quoted 7 lines
Yeah, that trickery is still little known (and I forget about it from time to time).
The upshot for those who have missed this is you can use a magic acpi device to instantiate based on device tree bindings :)
So we need to strip all this 'obvious' protection stuff out of drivers.
Jonathan,
Do you want me to do the same changes for drivers in iio/ ? I'm probably not the only one looking at other drivers when writing my own, so I guess this is a rather frequent issue for new drivers?
Quoted 7 lines
Quoted 6 lines
Quoted 3 lines
+ .of_match_table =
of_match_ptr(mcp3911_dt_ids),Ditto for macro.
Best regards,
Marcus Folkesson
Quoted 11 lines · Marcus Folkesson
Quoted 10 lines · Jonathan Cameron
Quoted 7 lines · On Thu, 2 Aug 2018 22:52:00 +0300 Andy Shevchenko <[email protected]>…
Quoted 4 lines · Marcus Folkesson
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 <[email protected]>
Quoted 7 lines
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Kent Gustavsson <[email protected]>What is this? Why it's here (presense and location in the message)?
To clarify... If Kent wrote the patch and you are simply acting as gatekeeper / upstreamer you should set the mail to be from Kent and put your own Signed-off after his to basically act as a submaintainer certifying you believe his sign off and all that entails.
Yep. And here the ordering is incorrect, right?
Quoted 16 lines
Quoted 2 lines
If it is a bit of a joint effort then that's fine but for copyright purposes there should be some indication of the split.
First, thank you Andy for noticing.
I actually intended to use Co-Developed-by (a pretty new tag) in combination with Signed-off-by.
But the tag must have disappeared in some preparation stage..From
Documentation/process/submitting-patches.rst::A Co-Developed-by: states that the patch was also created by another developer along with the original author. This is useful at times when multiple people work on a single patch. Note, this person also needs to have a Signed-off-by:
line in the patch as well.
Quoted 6 lines
I will switch order and add the Co-Developed-by-tag. Is this correct?
Co-Developed-by Kent Gustavsson <[email protected]>Signed-off-by Kent Gustavsson <[email protected]>Signed-off-by Marcus Folkesson <[email protected]>
Yep, this looks good!
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
+
of_property_read_u32(of_node, "device-addr", &adc->dev_addr);Isn't what we called CS (chip select)?
Nope. We went around this in an earlier revision. It's an address transmitted in the control byte to allow you to 'share' a chip select line between multiple chips (crazy but true).
OK.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerQuoted 136 lines · On Sat, 4 Aug 2018 08:55:06 +0200 Marcus Folkesson…
Hi Andy and Jonathan,
Quoted 12 lines · Jonathan Cameron
Quoted 9 lines · On Thu, 2 Aug 2018 22:52:00 +0300 Andy Shevchenko <[email protected]>…
Quoted 6 lines · Marcus Folkesson
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 <[email protected]>Signed-off-by Kent Gustavsson <[email protected]>Quoted 8 lines
Quoted 1 line
What is this? Why it's here (presense and location in the message)?
To clarify... If Kent wrote the patch and you are simply acting as gatekeeper / upstreamer you should set the mail to be from Kent and put your own Signed-off after his to basically act as a submaintainer certifying you believe his sign off and all that entails.
If it is a bit of a joint effort then that's fine but for copyright purposes there should be some indication of the split.
First, thank you Andy for noticing.
I actually intended to use Co-Developed-by (a pretty new tag) in combination with Signed-off-by.
But the tag must have disappeared in some preparation stage..From
Documentation/process/submitting-patches.rst::A Co-Developed-by: states that the patch was also created by another developer along with the original author. This is useful at times when multiple people work on a single patch. Note, this person also needs to have a Signed-off-by:
line in the patch as well.I will switch order and add the Co-Developed-by-tag. Is this correct?
Co-Developed-by Kent Gustavsson <[email protected]>Signed-off-by Kent Gustavsson <[email protected]>Signed-off-by Marcus Folkesson <[email protected]>Quoted 54 lines
Quoted 29 lines
Quoted 6 lines
+ * Copyright (C) 2018 Kent Gustavsson <[email protected]>
+ *
Redundant.
Quoted 20 lines
drivers/iio/adc/mcp3911.c new file +20
-0+static int mcp3911_read (struct mcp3911 *adc,u8 reg,u32 *val,u8 len) +{ +int ret; + + reg =MCP3911_REG_READ (reg, adc->dev_addr); + ret =spi_write_then_read (adc->spi, ®,1 , val, len); +if (ret <0 ) +return ret; + +be32_to_cpus (val); + *val >>= ((4 - len) *8 ); +dev_dbg (&adc->spi->dev,"reading 0x%x from register 0x%x\n" , *val, + reg>>1 ); +return ret; +} + +static int mcp3911_write (struct mcp3911 *adc,u8 reg,u32 val,u8 len) +{ +dev_dbg (&adc->spi->dev,"writing 0x%x to register 0x%x\n" , val, reg); +Quoted 8 lines
drivers/iio/adc/mcp3911.c line 83
Quoted 6 lines
drivers/iio/adc/mcp3911.c new file +6
-0+ val <<= (3 - len) *8 ; +cpu_to_be32s (&val); + val |=MCP3911_REG_WRITE (reg, adc->dev_addr); + +return spi_write (adc->spi, &val, len +1 ); +}Can't you use
REGMAP_SPI?My instinct is not really. The magic device-addr doesn't help. You could work around it but it's not that nice and you'd have to create the regmap mapping on the fly or carry static ones for each value of htat.
Quoted 10 lines
Quoted 9 lines
drivers/iio/adc/mcp3911.c new file +8
-0+of_property_read_u32 (of_node,"device-addr" , &adc->dev_addr); +if (adc->dev_addr >3 ) { +dev_err (&adc->spi->dev, +"invalid device address (%i). Must be in range 0-3.\n" , + adc->dev_addr); +return -EINVAL ; + } +dev_dbg (&adc->spi->dev,"use device address %i\n" , adc->dev_addr);Quoted 27 lines
Quoted 1 line
Isn't what we called CS (chip select)?
Nope. We went around this in an earlier revision. It's an address transmitted in the control byte to allow you to 'share' a chip select line between multiple chips (crazy but true).
Quoted 22 lines
drivers/iio/adc/mcp3911.c line 256
Quoted 4 lines
drivers/iio/adc/mcp3911.c new file +2
-0+if (IS_ERR (adc->vref)) { +Redundant.
Quoted 1 line
+ if (
adc->clki)Seems to be redundant if clock is optional (it should be NULL and API is NULL-aware).
drivers/iio/adc/mcp3911.c line 334
Quoted 4 lines
drivers/iio/adc/mcp3911.c new file +3
-0+clk_disable_unprepare (adc->clki); +if (adc->clki) +clk_disable_unprepare (adc->clki);Ditto.
Quoted 1 line
+#if
defined(CONFIG_OF)This prevents playing with the device on ACPI enabled platforms.
I will remove the
defined(CONFIG_OF)and not use theof_match_ptr()macro.Quoted 6 lines
Yeah, that trickery is still little known (and I forget about it from time to time).
The upshot for those who have missed this is you can use a magic acpi device to instantiate based on device tree bindings :)
Quoted 6 lines
Quoted 1 line
So we need to strip all this 'obvious' protection stuff out of drivers.
Jonathan,
Do you want me to do the same changes for drivers in iio/ ? I'm probably not the only one looking at other drivers when writing my own, so I guess this is a rather frequent issue for new drivers?
A good question. Hmm. I'm a bit in two minds. It creates a lot
of churn for something that I don't think is actually very heavily used. As you say, the biggest reason to change it will be to stop others from copying the existing drivers and getting it 'wrong' going forward.
I don't have strong feelings either way, but don't really want to see a huge number of patches around this (I haven't checked how many drivers are effected). If you want to look at this, then perhaps do a small set at a time rather than a mass fixup to keep things manageable?
Thanks,
Jonathan
Quoted 12 lines
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines
+ .of_match_table =
of_match_ptr(mcp3911_dt_ids),Ditto for macro.
Best regards,
Marcus Folkesson