Support ROHM BD79112 ADC
Subsystems (4)
Matti Vaittinen
maintainerv2 cover Support ROHM BD79112 ADC
Support ROHM BD79112 ADC/GPIO
The ROHM BD79112 is a 12-bit, 32 channel SAR ADC / GPIO IC. Or, a "Signal Monitor Hub IC" as data-sheet describes it.
Data sheet states the maximum sampling rate to be 1 MSPS, but achieving this would probably require the SPI and samples to be processed by something else but the CPU running Linux. This could work with the "SPI offloading" which has recently landed upstream - but I have no HW to test this so nothing fancy is implemented here. It's still worth mentioning if someone needs the speed and wants to try implementing it :)
The SPI protocol is slightly peculiar. Accesses are done in 16-bit sequences, separated by releasing and re-aquiring the chip-select.
Register write takes 1 such sequence. The 8-bit register data to write, is stored in the last 8 bits. The high 8 bits contain register address and an I/O-bit which needs to be set for register accesses.
Register read consists of two 16-bit sequences (separated by chip-select). First sequence has again the register address and an IO bit in the high byte. Additionally, reads must have a 'read bit' set. The last 8 bits must be zero. The register data will be carried in the last 8 bits of the next 16-bit sequence while high bits in reply are zero.
ADC data reading is similar to register reading except:
- No R/W bit or I/O bit should be set.
- Register address is replaced by channel number (0 - 31).
- Reply data is carried in the 12 low bits (instead of 8 bits) of the reply sequence.
The protocol is implemented using custom regmap read() and write() operations.
Other than that, pretty standard device and driver.
Revision history:
v1 => v2:
- Plenty of fixes to the driver (thanks to reviewers, Andy and David)
- Add gpio-controller information to the device-tree bindings See individual patches for more accurate changelog
---
Matti Vaittinen (3):
dt-bindings: iio: adc: ROHM BD79112 ADC/GPIO
iio: adc: Support ROHM BD79112 ADC/GPIO
MAINTAINERS: Support ROHM BD79112 ADC
.../bindings/iio/adc/rohm,bd79112.yaml | 116 ++++ MAINTAINERS | 3 +- drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/rohm-bd79112.c | 551 ++++++++++++++++++ 5 files changed, 680 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml create mode 100644 drivers/iio/adc/rohm-bd79112.c
base-commit: d1487b0b78720b86ec2a2ac7acc683ec90627e5b
Signature · 1 line
2.51.0
Matti Vaittinen
maintainerPATCH v2 1/3 dt-bindings: iio: adc: ROHM BD79112 ADC/GPIO
The ROHM BD79112 is an ADC/GPIO with 32 channels. The channel inputs can be used as ADC or GPIO. Using the GPIOs as IRQ sources isn't supported.
The ADC is 12-bit, supporting input voltages up to 5.7V, and separate I/O voltage supply. Maximum SPI clock rate is 20 MHz (10 MHz with daisy-chain configuration) and maximum sampling rate is 1MSPS.
Add a device tree binding document for the ROHM BD79112.
---
Revision history:
v1 => v2:
- BD79112 can act as a GPIO controller.
--- .../bindings/iio/adc/rohm,bd79112.yaml | 116 ++++++++++++++++++ 1 file changed, 116 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml
Documentation/devicetree/bindings/iio/adc/rohm,bd79112.yaml new file +116 -0
@@ -0,0 +1,116 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/rohm,bd79112.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ROHM BD79112 ADC/GPO + +maintainers: + - Matti Vaittinen <[email protected]> + +description: | + The ROHM BD79112 is a 12-bit, 32-channel, SAR ADC. ADC input pins can be + also configured as general purpose inputs/outputs. SPI should use MODE 3. + +properties: + compatible: + const: rohm,bd79112 + + reg: + maxItems: 1 + + spi-cpha: true + spi-cpol: true + + gpio-controller: true + "#gpio-cells": + const: 2 + + vdd-supply: true + + iovdd-supply: true + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + +patternProperties: + "^channel@([0-9]|[12][0-9]|3[01])$": + type: object + $ref: /schemas/iio/adc/adc.yaml# + description: Represents ADC channel. Omitted channels' inputs are GPIOs. + + properties: + reg: + description: AIN pin number + minimum: 0 + maximum: 31 + + required: + - reg + + additionalProperties: false + +required: + - compatible + - reg + - iovdd-supply + - vdd-supply + - spi-cpha + - spi-cpol + +additionalProperties: false + +examples: + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + adc: adc@0 { + compatible = "rohm,bd79112"; + reg = <0x0>; + + spi-cpha; + spi-cpol; + + vdd-supply = <&dummyreg>; + iovdd-supply = <&dummyreg>; + + #address-cells = <1>; + #size-cells = <0>; + + gpio-controller; + #gpio-cells = <2>; + + channel@0 { + reg = <0>; + }; + channel@1 { + reg = <1>; + }; + channel@2 { + reg = <2>; + }; + channel@3 { + reg = <3>; + }; + channel@4 { + reg = <4>; + }; + channel@5 { + reg = <5>; + }; + channel@6 { + reg = <6>; + }; + channel@16 { + reg = <16>; + }; + channel@20 { + reg = <20>; + }; + }; + };
Signature · 1 line
2.51.0
Re: [PATCH v2 1/3] dt-bindings: iio: adc: ROHM BD79112 ADC/GPIO
Quoted 10 lines · Matti Vaittinen
The ROHM BD79112 is an ADC/GPIO with 32 channels. The channel inputs can be used as ADC or GPIO. Using the GPIOs as IRQ sources isn't supported.
The ADC is 12-bit, supporting input voltages up to 5.7V, and separate I/O voltage supply. Maximum SPI clock rate is 20 MHz (10 MHz with daisy-chain configuration) and maximum sampling rate is 1MSPS.
Add a device tree binding document for the ROHM BD79112.
Signed-off-by Matti Vaittinen <[email protected]>
Matti Vaittinen
maintainerPATCH v2 2/3 iio: adc: Support ROHM BD79112 ADC/GPIO
In reply to Matti VaittinenThe ROHM BD79112 is an ADC/GPIO with 32 channels. The channel inputs can be used as ADC or GPIO. Using the GPIOs as IRQ sources isn't supported.
The ADC is 12-bit, supporting input voltages up to 5.7V, and separate I/O voltage supply. Maximum SPI clock rate is 20 MHz (10 MHz with daisy-chain configuration) and maximum sampling rate is 1MSPS.
The IC does also support CRC but it is not implemented in the driver.
---
Revision history:
v1 => v2 (mainly based on reviews by Andy and David, thanks!)
- Fix Kconfig dependency to REGMAP_SPI instead of REGMAP_I2C
- Add a few header includes
- Drop unnecessary alignments
- plenty of styling
- use for_each_set_clump8 instead of open-coding it
- change order of direction setting writes to avoid receiving 'event' when direction is changed from input to output.
- fix data-sheet names and assigning of them to iio_dev
--- drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/rohm-bd79112.c | 551 +++++++++++++++++++++++++++++++++ 3 files changed, 562 insertions(+) create mode 100644 drivers/iio/adc/rohm-bd79112.c
drivers/iio/adc/Kconfig +10 -0
@@ -1309,6 +1309,16 @@ config RN5T618_ADC This driver can also be built as a module. If so, the module will be called rn5t618-adc. +config ROHM_BD79112 + tristate "Rohm BD79112 ADC driver" + depends on I2C && GPIOLIB + select REGMAP_SPI + select IIO_ADC_HELPER + help + Say yes here to build support for the ROHM BD79112 ADC. The + ROHM BD79112 is a 12-bit, 32-channel, SAR ADC, which analog + inputs can also be used for GPIO. + config ROHM_BD79124 tristate "Rohm BD79124 ADC driver" depends on I2C
drivers/iio/adc/Makefile +1 -0
@@ -117,6 +117,7 @@ obj-$(CONFIG_QCOM_VADC_COMMON) += qcom-vadc-common.o obj-$(CONFIG_RCAR_GYRO_ADC) += rcar-gyroadc.o obj-$(CONFIG_RICHTEK_RTQ6056) += rtq6056.o obj-$(CONFIG_RN5T618_ADC) += rn5t618-adc.o +obj-$(CONFIG_ROHM_BD79112) += rohm-bd79112.o obj-$(CONFIG_ROHM_BD79124) += rohm-bd79124.o obj-$(CONFIG_ROCKCHIP_SARADC) += rockchip_saradc.o obj-$(CONFIG_RZG2L_ADC) += rzg2l_adc.o
drivers/iio/adc/rohm-bd79112.c new file +551 -0
551 lines not on this page · raw message
Signature · 1 line
2.51.0
Re: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 8 lines · Matti Vaittinen
The ROHM BD79112 is an ADC/GPIO with 32 channels. The channel inputs can be used as ADC or GPIO. Using the GPIOs as IRQ sources isn't supported.
The ADC is 12-bit, supporting input voltages up to 5.7V, and separate I/O voltage supply. Maximum SPI clock rate is 20 MHz (10 MHz with daisy-chain configuration) and maximum sampling rate is 1MSPS.
The IC does also support CRC but it is not implemented in the driver.
...
Quoted 17 lines
+/*
+ * The data-sheet explains register I/O communication as follows:
+ *
+ * Read, two 16-bit sequences separated by CSB:
+ * MOSI:+ * SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | 9 .. 16 | + * data:| 0 | 0 |IOSET| RW (1) | ADDR | 8'b0 | + * + * MISO: + * SCK: | 1 .. 8 | 9 .. 16 | + * data:| 8'b0 | data |+ *
+ * Note, CSB is shown to be released between writing the address (MOSI) and
+ * reading the register data (MISO).
+ *
+ * Write, single 16-bit sequence:
+ * MOSI:
Quoted 7 lines
+ * SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | + * data:| 0 | 0 |IOSET| RW(0) | ADDR | + * + * MISO: + * SCK: | 1 .. 8 | + * data:| data |+ */
I don't know how to read this comment. In the monospace font the whole block looks like a mess.
...
drivers/iio/adc/rohm-bd79112.c line 124
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +5
-0+static int _get_gpio_reg (unsigned int offset,unsigned int base) +{ +int regoffset = offset /8 ; + +if (offset >31 || offset <0 )
So, < 0 is now unneeded and offset > 31 can be rewritten as
if (regoffset >= 4)
which is more clear to me (like we have 4 banks and here is the check for the bank. Maybe you can even call the variable 'bank'.
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+return -EINVAL ; + +return base - regoffset; +}
...
Quoted 1 line
+#define
GET_GPIO_BIT(offset)BIT((offset) % 8)
I suggest to make it to be a returned parameter of _get_gpio_reg(). This will give better code generation on some architectures, see, for example, this
commit: 9b3cd5c7099f regmap: place foo / 8 and foo % 8 closer to each other.
...
drivers/iio/adc/rohm-bd79112.c line 150
Quoted 3 lines
drivers/iio/adc/rohm-bd79112.c new file +3
-0+static const struct regmap_access_table bd79112_volatile_regs = { + .yes_ranges = &bd71815_volatile_ro_ranges[0 ], + .n_yes_ranges =ARRAY_SIZE (bd71815_volatile_ro_ranges),
+ array_size.h
(and btw we put generic asm/* _after_ generic linux/*, just noticed that).
Quoted 1 line
+};
...
Quoted 15 lines
drivers/iio/adc/rohm-bd79112.c new file +15
-0+static int bd79112_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan,int *val, +int *val2,long m) +{ +struct bd79112_data *data =iio_priv (indio_dev); +int ret; + +switch (m) { +case IIO_CHAN_INFO_RAW : + ret =regmap_read (data->map, chan->channel, val); +if (ret <0 ) +return ret; + +return IIO_VAL_INT ; +
drivers/iio/adc/rohm-bd79112.c line 220
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+case IIO_CHAN_INFO_SCALE : + *val = data->vref_mv; + *val2 =12 ; + +return IIO_VAL_FRACTIONAL_LOG2 ; +default : +return -EINVAL ; + } +
Unneeded blank line.
Quoted 1 line
+}
...
drivers/iio/adc/rohm-bd79112.c line 314
Quoted 7 lines
drivers/iio/adc/rohm-bd79112.c new file +7
-0+static int bd79112_gpio_set_multiple (struct gpio_chip *gc,unsigned long *mask, +unsigned long *bits) +{ +struct bd79112_data *data =gpiochip_get_data (gc); +unsigned long i, bank_mask; + +for_each_set_clump8 (i, bank_mask, mask,/* gc->ngpio */ 32 ) {
Hmm... Why constant and not gc->ngpio?
drivers/iio/adc/rohm-bd79112.c line 319
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+unsigned long bank_bits; +unsigned int reg; +int ret; +if (bank_mask) {
This is a duplication, the iterator only gives non-zero "clumps".
Quoted 11 lines
drivers/iio/adc/rohm-bd79112.c new file +11
-0+ bank_bits =bitmap_get_value8 (bits, i); + reg =BD79112_REG_GPO_VALUE_A0_A7 - i /8 ; + ret =regmap_update_bits (data->map, reg, bank_mask, + bank_bits); +if (ret) +return ret; + } + } + +return 0 ; +}
...
drivers/iio/adc/rohm-bd79112.c line 400
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +10
-0+static int bd79112_get_gpio_pins (const struct iio_chan_spec *cs,int num_channels) +{ +int i, gpio_channels; + +/* +* Let's initialize the mux config to say that all 32 channels are +* GPIOs. Then we can just loop through the iio_chan_spec and clear the +* bits for found ADC channels. +*/ + gpio_channels =GENMASK (31 ,0 );
This is negative number, it might bait one at a surprising time. Hence once again, why not make them unsigned?
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +5
-0+for (i =0 ; i < num_channels; i++) + gpio_channels &= ~BIT (cs[i].channel); + +return gpio_channels; +}
...
drivers/iio/adc/rohm-bd79112.c line 415
Quoted 9 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+/* ADC channels as named in the data-sheet */ +static const char *const bd79112_chan_names[] = { +"AGIO0A" ,"AGIO1A" ,"AGIO2A" ,"AGIO3A" ,"AGIO4A" ,/* 0 - 4 */ +"AGIO5A" ,"AGIO6A" ,"AGIO7A" ,"AGIO8A" ,"AGIO9A" ,/* 5 - 9 */ +"AGIO10A" ,"AGIO11A" ,"AGIO12A" ,"AGIO13A" ,"AGIO14A" ,/* 10 - 14 */ +"AGIO15A" ,"AGIO0B" ,"AGIO1B" ,"AGIO2B" ,"AGIO3B" ,/* 15 - 19 */ +"AGIO4B" ,"AGIO5B" ,"AGIO6B" ,"AGIO7B" ,"AGIO8B" ,/* 20 - 24 */ +"AGIO9B" ,"AGIO10B" ,"AGIO11B" ,"AGIO12B" ,"AGIO13B" ,/* 25 - 29 */ +"AGIO14B" ,"AGIO15B" ,/* 30 - 31 */
O-o-key, but why not power-of-two per line (esp. taking into account the whole size)? (Whatever, it's not something I would fight for.)
Quoted 1 line
+};
...
drivers/iio/adc/rohm-bd79112.c line 444
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+ data->vref_mv = ret /1000 ;
Yeah, mV, (MICRO / MILLI) and other things I leave to other people to discuss.
...
drivers/iio/adc/rohm-bd79112.c line 467
Quoted 8 lines
drivers/iio/adc/rohm-bd79112.c new file +7
-0+ ret =devm_iio_adc_device_alloc_chaninfo_se (dev, &bd79112_chan_template, +BD79112_MAX_NUM_CHANNELS -1 , + &cs); +if (ret <0 ) { +/* Register all pins as GPIOs if there are no ADC channels */ +if (ret == -ENOENT ) + goto register_gpios;
As I showed this can be checked before other case, but I kinda have an idea why you are liking to do it this way.
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }
...
drivers/iio/adc/rohm-bd79112.c line 507
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +5
-0+register_gpios: + gpio_pins =bd79112_get_gpio_pins (iio_dev->channels, + iio_dev->num_channels); + +/* If all channels are reserved for ADC, then we're done. */
I still consider the assignment to be located here is a better place, but I leave it to maintainers.
Quoted 2 lines
drivers/iio/adc/rohm-bd79112.c new file +2
-0+if (!gpio_pins) +return 0 ;
Signature · 2 lines
With Best Regards, Andy Shevchenko
Matti Vaittinen
maintainerRe: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Hi dee Ho Andy!
Thanks again. I really appreciate the effort you put in these reviews! :)
Quoted 37 lines · Andy Shevchenko
Quoted 8 lines · Matti Vaittinen
The ROHM BD79112 is an ADC/GPIO with 32 channels. The channel inputs can be used as ADC or GPIO. Using the GPIOs as IRQ sources isn't supported.
The ADC is 12-bit, supporting input voltages up to 5.7V, and separate I/O voltage supply. Maximum SPI clock rate is 20 MHz (10 MHz with daisy-chain configuration) and maximum sampling rate is 1MSPS.
The IC does also support CRC but it is not implemented in the driver.
...
Quoted 17 lines
+/*
+ * The data-sheet explains register I/O communication as follows:
+ *
+ * Read, two 16-bit sequences separated by CSB:
+ * MOSI:+ * SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | 9 .. 16 | + * data:| 0 | 0 |IOSET| RW (1) | ADDR | 8'b0 | + * + * MISO: + * SCK: | 1 .. 8 | 9 .. 16 | + * data:| 8'b0 | data |+ *
+ * Note, CSB is shown to be released between writing the address (MOSI) and
+ * reading the register data (MISO).
+ *
+ * Write, single 16-bit sequence:
+ * MOSI:Quoted 7 lines
+ * SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | + * data:| 0 | 0 |IOSET| RW(0) | ADDR | + * + * MISO: + * SCK: | 1 .. 8 | + * data:| data |+ */
Quoted 2 lines
I don't know how to read this comment. In the monospace font the whole block looks like a mess.
What do you mean by a mess? Don't you have the '|' -characters aligned? That's very odd because they are aligned for me. Or, is this otherwise unclear?
Quoted 11 lines
...
drivers/iio/adc/rohm-bd79112.c line 124
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +5
-0+static int _get_gpio_reg (unsigned int offset,unsigned int base) +{ +int regoffset = offset /8 ; + +if (offset >31 || offset <0 )So, < 0 is now unneeded and offset > 31 can be rewritten as
Quoted 4 lines
if (regoffset >= 4)which is more clear to me (like we have 4 banks and here is the check for the bank. Maybe you can even call the variable 'bank'.
Ah, thanks for pointing out the pointlessness of the < 0! Will fix this.
I still prefer checking that the offset doesn't exceed the pin count. The amount of pins is clear for anyone using the device, where as amount of 'banks' or even 'registers' isn't quite as obvious but requires one to know the internals of the IC.
Quoted 8 lines
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+return -EINVAL ; + +return base - regoffset; +}...
Quoted 5 lines
Quoted 1 line
+#define
GET_GPIO_BIT(offset)BIT((offset) % 8)I suggest to make it to be a returned parameter of
_get_gpio_reg(). This will give better code generation on some architectures, see, for example, this
commit:9b3cd5c7099fregmap: place foo / 8 and foo % 8 closer to each other.
Interesting micro optimization. I had no idea about this - thanks for sharing it :) It's always good to learn something!
I don't like it here though. There are a few call sites where we only need to get the offset once, but more than one register. (Like the direction setting). Hence, I like to keep these operations decoupled. I think the benefit from this optimization is quite small. Furthermore, architecture specific optimizations aren't really something we should add in generic drivers. It may be this will never get used on the architecture we are optimizing for. [Sure it can make sense for something like regmap, which is heavily used (probably) on all architectures, especially if cost is just placing some lines closer to each others. So, nice change there!]
Quoted 9 lines
...
drivers/iio/adc/rohm-bd79112.c line 150
Quoted 3 lines
drivers/iio/adc/rohm-bd79112.c new file +3
-0+static const struct regmap_access_table bd79112_volatile_regs = { + .yes_ranges = &bd71815_volatile_ro_ranges[0 ], + .n_yes_ranges =ARRAY_SIZE (bd71815_volatile_ro_ranges),+
array_size.h
(and btw we put generic asm/* _after_ generic linux/*, just noticed that).
Thanks!
Quoted 36 lines
Quoted 1 line
+};
...
Quoted 15 lines
drivers/iio/adc/rohm-bd79112.c new file +15
-0+static int bd79112_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan,int *val, +int *val2,long m) +{ +struct bd79112_data *data =iio_priv (indio_dev); +int ret; + +switch (m) { +case IIO_CHAN_INFO_RAW : + ret =regmap_read (data->map, chan->channel, val); +if (ret <0 ) +return ret; + +return IIO_VAL_INT ; +drivers/iio/adc/rohm-bd79112.c line 220
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+case IIO_CHAN_INFO_SCALE : + *val = data->vref_mv; + *val2 =12 ; + +return IIO_VAL_FRACTIONAL_LOG2 ; +default : +return -EINVAL ; + } +Unneeded blank line.
Quoted 1 line
+}
...
Quoted 9 lines
drivers/iio/adc/rohm-bd79112.c line 314
Quoted 7 lines
drivers/iio/adc/rohm-bd79112.c new file +7
-0+static int bd79112_gpio_set_multiple (struct gpio_chip *gc,unsigned long *mask, +unsigned long *bits) +{ +struct bd79112_data *data =gpiochip_get_data (gc); +unsigned long i, bank_mask; + +for_each_set_clump8 (i, bank_mask, mask,/* gc->ngpio */ 32 ) {Hmm... Why constant and not
gc->ngpio?
Oh, well spotted! That's just a leftover from my testing where I called this without the gpio-chip! It should indeed be the gc->ngpio! Thanks!
Quoted 8 lines
drivers/iio/adc/rohm-bd79112.c line 319
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+unsigned long bank_bits; +unsigned int reg; +int ret; +if (bank_mask) {This is a duplication, the iterator only gives non-zero "clumps".
Indeed. Thanks!
Quoted 14 lines
Quoted 11 lines
drivers/iio/adc/rohm-bd79112.c new file +11
-0+ bank_bits =bitmap_get_value8 (bits, i); + reg =BD79112_REG_GPO_VALUE_A0_A7 - i /8 ; + ret =regmap_update_bits (data->map, reg, bank_mask, + bank_bits); +if (ret) +return ret; + } + } + +return 0 ; +}...
Quoted 13 lines
drivers/iio/adc/rohm-bd79112.c line 400
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +10
-0+static int bd79112_get_gpio_pins (const struct iio_chan_spec *cs,int num_channels) +{ +int i, gpio_channels; + +/* +* Let's initialize the mux config to say that all 32 channels are +* GPIOs. Then we can just loop through the iio_chan_spec and clear the +* bits for found ADC channels. +*/ + gpio_channels =GENMASK (31 ,0 );This is negative number, it might bait one at a surprising time. Hence once again, why not make them unsigned?
I think I already explained why it is signed. I don't see how this bites us here? Can you please elaborate what's the problem _here_?
Quoted 9 lines
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c new file +5
-0+for (i =0 ; i < num_channels; i++) + gpio_channels &= ~BIT (cs[i].channel); + +return gpio_channels; +}...
Quoted 12 lines
drivers/iio/adc/rohm-bd79112.c line 415
Quoted 9 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+/* ADC channels as named in the data-sheet */ +static const char *const bd79112_chan_names[] = { +"AGIO0A" ,"AGIO1A" ,"AGIO2A" ,"AGIO3A" ,"AGIO4A" ,/* 0 - 4 */ +"AGIO5A" ,"AGIO6A" ,"AGIO7A" ,"AGIO8A" ,"AGIO9A" ,/* 5 - 9 */ +"AGIO10A" ,"AGIO11A" ,"AGIO12A" ,"AGIO13A" ,"AGIO14A" ,/* 10 - 14 */ +"AGIO15A" ,"AGIO0B" ,"AGIO1B" ,"AGIO2B" ,"AGIO3B" ,/* 15 - 19 */ +"AGIO4B" ,"AGIO5B" ,"AGIO6B" ,"AGIO7B" ,"AGIO8B" ,/* 20 - 24 */ +"AGIO9B" ,"AGIO10B" ,"AGIO11B" ,"AGIO12B" ,"AGIO13B" ,/* 25 - 29 */ +"AGIO14B" ,"AGIO15B" ,/* 30 - 31 */ O-o-key, but why not power-of-two per line (esp. taking into account the whole size)? (Whatever, it's not something I would fight for.)
I just filled the rows to the maximum width, while keeping the item count same for each row and adding the comment.
I'm not really sure having 4 items / row, and adding one row more would be much better, but not much worse either. I can do this if you think it's better. (No need to even fight for that).
Quoted 7 lines
Quoted 1 line
+};
...
drivers/iio/adc/rohm-bd79112.c line 444
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+ data->vref_mv = ret /1000 ;Yeah, mV, (MICRO / MILLI) and other things I leave to other people to discuss.
Thanks Andy. I've a feeling we were somewhat stuck on this discussion anyways. I suppose it's nice to hear other's opinions if someone else really cares. This was approaching bikeshedding.
Quoted 7 lines
...
Quoted 4 lines
drivers/iio/adc/rohm-bd79112.c new file +4
-0+ ret =devm_iio_adc_device_alloc_chaninfo_se (dev, &bd79112_chan_template, +BD79112_MAX_NUM_CHANNELS -1 , + &cs); +if (ret <0 ) {
Quoted 6 lines
drivers/iio/adc/rohm-bd79112.c line 467
Quoted 3 lines
drivers/iio/adc/rohm-bd79112.c new file +3
-0+/* Register all pins as GPIOs if there are no ADC channels */ +if (ret == -ENOENT ) + goto register_gpios;As I showed this can be checked before other case, but I kinda have an idea why you are liking to do it this way.
Sorry Andy. I think I didn't consider your suggestion thoroughly as I just assumed it was related to the question if the value returned by the devm_iio_adc_device_alloc_chaninfo_se() can be positive. I'll re-read your suggestion, thanks for pinging me on this.
Quoted 5 lines
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }...
Quoted 8 lines
drivers/iio/adc/rohm-bd79112.c line 507
Quoted 5 lines
drivers/iio/adc/rohm-bd79112.c new file +5
-0+register_gpios: + gpio_pins =bd79112_get_gpio_pins (iio_dev->channels, + iio_dev->num_channels); + +/* If all channels are reserved for ADC, then we're done. */ I still consider the assignment to be located here is a better place, but I leave it to maintainers.
I do also still think placing the comment before assignment can give reader an idea that the amount of pins is fetched for this check, which it isn't. We fetch it for other purposes, but optimize things bailing out if there are no GPIOs. So, we comment to explain early exit. But yeah, let's see what others think of it as well.
Quoted 3 lines
drivers/iio/adc/rohm-bd79112.c line 509
Quoted 2 lines
drivers/iio/adc/rohm-bd79112.c new file +2
-0+if (!gpio_pins) +return 0 ;
Yours,
-- Matti
David Lechner
reviewerRe: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 42 lines · Matti Vaittinen
Hi dee Ho Andy!
Thanks again. I really appreciate the effort you put in these reviews! :)
Quoted 37 lines · Andy Shevchenko
Quoted 8 lines · Matti Vaittinen
The ROHM BD79112 is an ADC/GPIO with 32 channels. The channel inputs can be used as ADC or GPIO. Using the GPIOs as IRQ sources isn't supported.
The ADC is 12-bit, supporting input voltages up to 5.7V, and separate I/O voltage supply. Maximum SPI clock rate is 20 MHz (10 MHz with daisy-chain configuration) and maximum sampling rate is 1MSPS.
The IC does also support CRC but it is not implemented in the driver.
...
Quoted 24 lines
+/*
+ * The data-sheet explains register I/O communication as follows:
+ *
+ * Read, two 16-bit sequences separated by CSB:
+ * MOSI:
+ * SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | 9 .. 16 |
+ * data:| 0 | 0 |IOSET| RW (1) | ADDR | 8'b0 |
+ *
+ * MISO:
+ * SCK: | 1 .. 8 | 9 .. 16 |
+ * data:| 8'b0 | data |
+ *
+ * Note, CSB is shown to be released between writing the address (MOSI) and
+ * reading the register data (MISO).
+ *
+ * Write, single 16-bit sequence:
+ * MOSI:+ * SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | + * data:| 0 | 0 |IOSET| RW(0) | ADDR |+ *
+ * MISO:
+ * SCK: | 1 .. 8 |
+ * data:| data |
+ */
Quoted 4 lines
Quoted 2 lines
I don't know how to read this comment. In the monospace font the whole block looks like a mess.
What do you mean by a mess? Don't you have the '|' -characters aligned? That's very odd because they are aligned for me. Or, is this otherwise unclear?
I find these diagrams very hard to read as well. I would just drop this part and let people look it up in the datasheet. I don't think it adds anything essential to understanding how the driver works.
Matti Vaittinen
maintainerRe: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 43 lines · David Lechner
Quoted 42 lines · Matti Vaittinen
Hi dee Ho Andy!
Thanks again. I really appreciate the effort you put in these reviews! :)
Quoted 37 lines · Andy Shevchenko
Quoted 8 lines · Matti Vaittinen
The ROHM BD79112 is an ADC/GPIO with 32 channels. The channel inputs can be used as ADC or GPIO. Using the GPIOs as IRQ sources isn't supported.
The ADC is 12-bit, supporting input voltages up to 5.7V, and separate I/O voltage supply. Maximum SPI clock rate is 20 MHz (10 MHz with daisy-chain configuration) and maximum sampling rate is 1MSPS.
The IC does also support CRC but it is not implemented in the driver.
...
Quoted 24 lines
+/*
+ * The data-sheet explains register I/O communication as follows:
+ *
+ * Read, two 16-bit sequences separated by CSB:
+ * MOSI:
+ * SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | 9 .. 16 |
+ * data:| 0 | 0 |IOSET| RW (1) | ADDR | 8'b0 |
+ *
+ * MISO:
+ * SCK: | 1 .. 8 | 9 .. 16 |
+ * data:| 8'b0 | data |
+ *
+ * Note, CSB is shown to be released between writing the address (MOSI) and
+ * reading the register data (MISO).
+ *
+ * Write, single 16-bit sequence:
+ * MOSI:+ * SCK: | 1 | 2 | 3 | 4 | 5 .. 8 | + * data:| 0 | 0 |IOSET| RW(0) | ADDR |+ *
+ * MISO:
+ * SCK: | 1 .. 8 |
+ * data:| data |
+ */
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
I don't know how to read this comment. In the monospace font the whole block looks like a mess.
What do you mean by a mess? Don't you have the '|' -characters aligned? That's very odd because they are aligned for me. Or, is this otherwise unclear?
I find these diagrams very hard to read as well. I would just drop this part and let people look it up in the datasheet. I don't think it adds anything essential to understanding how the driver works.
I am a bit reluctant to drop this because I don't see public data-sheet anywhere... :/ I think explaining the protocol is somewhat useful so readers can understand why we have custom regmap read/write functions, and hopefully also the I/O and RW bits.
I am all for improving this documentation though! Starting by dropping those tabs, and seeing if I can think of something else too :)
Yours,
-- Matti
Jonathan Cameron
maintainerRe: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
In reply to Matti VaittinenQuoted 14 lines
Quoted 13 lines
drivers/iio/adc/rohm-bd79112.c line 415
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+/* ADC channels as named in the data-sheet */ +static const char *const bd79112_chan_names[] = { +"AGIO0A" ,"AGIO1A" ,"AGIO2A" ,"AGIO3A" ,"AGIO4A" ,/* 0 - 4 */ +"AGIO5A" ,"AGIO6A" ,"AGIO7A" ,"AGIO8A" ,"AGIO9A" ,/* 5 - 9 */ +"AGIO10A" ,"AGIO11A" ,"AGIO12A" ,"AGIO13A" ,"AGIO14A" ,/* 10 - 14 */ +"AGIO15A" ,"AGIO0B" ,"AGIO1B" ,"AGIO2B" ,"AGIO3B" ,/* 15 - 19 */ +"AGIO4B" ,"AGIO5B" ,"AGIO6B" ,"AGIO7B" ,"AGIO8B" ,/* 20 - 24 */ +"AGIO9B" ,"AGIO10B" ,"AGIO11B" ,"AGIO12B" ,"AGIO13B" ,/* 25 - 29 */ +"AGIO14B" ,"AGIO15B" ,/* 30 - 31 */ O-o-key, but why not power-of-two per line (esp. taking into account the whole size)? (Whatever, it's not something I would fight for.)
Quoted 6 lines
I just filled the rows to the maximum width, while keeping the item count same for each row and adding the comment.
I'm not really sure having 4 items / row, and adding one row more would be much better, but not much worse either. I can do this if you think it's better. (No need to even fight for that).
In this case I'd do it in 4s purely because then the B ones start on a new line and that looks nicer ;)
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
+};
...
Quoted 8 lines
Quoted 3 lines
drivers/iio/adc/rohm-bd79112.c line 444
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+ data->vref_mv = ret /1000 ;Yeah, mV, (MICRO / MILLI) and other things I leave to other people to discuss.
Thanks Andy. I've a feeling we were somewhat stuck on this discussion anyways. I suppose it's nice to hear other's opinions if someone else really cares. This was approaching bikeshedding.
I'll just throw a quick comment in here.
I'm absolutely in favour of the defines as they help in several ways.
- Stop counting 0s when there are lots of them.
- Provide information on the unit conversion not otherwise visible where they happen to be used (often avoids need for local variables etc).
- Consistency across a code base.
In this particular case none apply strongly enough to force the issue. Even so I'd prefer the units.h macros are used for the consistency reason but I won't refuse to merge the driver over this.
One of those perfect (to me) being the enemy of good cases.
p.s. Matti, 3 versions in 4 days? Perhaps slow down a little.
Matti Vaittinen
maintainerRe: [PATCH v2 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO
Quoted 17 lines · Jonathan Cameron
Quoted 17 lines
Quoted 13 lines
drivers/iio/adc/rohm-bd79112.c line 415
Quoted 10 lines
drivers/iio/adc/rohm-bd79112.c new file +9
-0+/* ADC channels as named in the data-sheet */ +static const char *const bd79112_chan_names[] = { +"AGIO0A" ,"AGIO1A" ,"AGIO2A" ,"AGIO3A" ,"AGIO4A" ,/* 0 - 4 */ +"AGIO5A" ,"AGIO6A" ,"AGIO7A" ,"AGIO8A" ,"AGIO9A" ,/* 5 - 9 */ +"AGIO10A" ,"AGIO11A" ,"AGIO12A" ,"AGIO13A" ,"AGIO14A" ,/* 10 - 14 */ +"AGIO15A" ,"AGIO0B" ,"AGIO1B" ,"AGIO2B" ,"AGIO3B" ,/* 15 - 19 */ +"AGIO4B" ,"AGIO5B" ,"AGIO6B" ,"AGIO7B" ,"AGIO8B" ,/* 20 - 24 */ +"AGIO9B" ,"AGIO10B" ,"AGIO11B" ,"AGIO12B" ,"AGIO13B" ,/* 25 - 29 */ +"AGIO14B" ,"AGIO15B" ,/* 30 - 31 */ O-o-key, but why not power-of-two per line (esp. taking into account the whole size)? (Whatever, it's not something I would fight for.)
I just filled the rows to the maximum width, while keeping the item count same for each row and adding the comment.
Quoted 6 lines
Quoted 3 lines
I'm not really sure having 4 items / row, and adding one row more would be much better, but not much worse either. I can do this if you think it's better. (No need to even fight for that).
In this case I'd do it in 4s purely because then the B ones start on a new line and that looks nicer ;)
Ok.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
Quoted 2 lines
+};
...
drivers/iio/adc/rohm-bd79112.c line 444
Quoted 1 line
drivers/iio/adc/rohm-bd79112.c new file +1
-0+ data->vref_mv = ret /1000 ;Yeah, mV, (MICRO / MILLI) and other things I leave to other people to discuss.
Quoted 8 lines
Quoted 4 lines
Thanks Andy. I've a feeling we were somewhat stuck on this discussion anyways. I suppose it's nice to hear other's opinions if someone else really cares. This was approaching bikeshedding.
I'll just throw a quick comment in here.
I'm absolutely in favour of the defines as they help in several ways.
- Stop counting 0s when there are lots of them.
I do agree on this when there is more than 3 zeroes. Although, for example 100 * 1000 is still clear to me. 100 * MILLI is not quite as obvious (again, to me).
Quoted 2 lines
- Provide information on the unit conversion not otherwise visible where they happen to be used (often avoids need for local variables etc).
Speaking only on my behalf but I'd rather see the local variables instead of constructs like (MICRO / MILLI) - which does not really feel right to me.
Quoted 1 line
- Consistency across a code base.
This probably means much more to a subsystem maintainer than to me! :) I'd still claim that consistency could be also achieved by consistently using dividers like / 1000 :)
Quoted 7 lines
In this particular case none apply strongly enough to force the issue. Even so I'd prefer the
units.hmacros are used for the consistency reason but I won't refuse to merge the driver over this.One of those perfect (to me) being the enemy of good cases.
p.s. Matti, 3 versions in 4 days? Perhaps slow down a little.
I should've indeed throttled sending the versions a bit. I just did fixes suggested by Andy/David immediately - and then I thought I could just do a decent change-log for reviewers, and send them while at it.
I didn't consider someone like You who will probably skim through all the versions and discussion. Sorry!
Thanks for the input!
Yours,
-- Matti
Matti Vaittinen
maintainerPATCH v2 3/3 MAINTAINERS: Support ROHM BD79112 ADC
In reply to Matti VaittinenAdd the ROHM BD79112 ADC in the list of the BD791xx ADC drivers which are maintained by undersigned.
---
Revision history:
v1 => :
- no changes
--- MAINTAINERS | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
MAINTAINERS +2 -1
@@ -21864,9 +21864,10 @@ S: Supported F: drivers/power/supply/bd99954-charger.c F: drivers/power/supply/bd99954-charger.h -ROHM BD79124 ADC / GPO IC +ROHM BD791xx ADC / GPO IC M: Matti Vaittinen <[email protected]> S: Supported +F: drivers/iio/adc/rohm-bd79112.c F: drivers/iio/adc/rohm-bd79124.c ROHM BH1745 COLOUR SENSOR
Signature · 1 line
2.51.0