linux-kernel - Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b2650751-2f23-4508-9959-c55bc0530dfa@gmail.com>
Date: Wed, 3 Sep 2025 10:03:24 +0300
From: Matti Vaittinen <mazziesaccount@...il.com>
To: David Lechner <dlechner@...libre.com>,
 Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Linus Walleij
 <linus.walleij@...aro.org>, Bartosz Golaszewski <brgl@...ev.pl>,
 Marcelo Schmitt <marcelo.schmitt@...log.com>,
 Javier Carrasco <javier.carrasco.cruz@...il.com>,
 Tobias Sperling <tobias.sperling@...ting.com>,
 Antoniu Miclaus <antoniu.miclaus@...log.com>,
 Trevor Gamblin <tgamblin@...libre.com>, Esteban Blanc <eblanc@...libre.com>,
 Ramona Alexandra Nechita <ramona.nechita@...log.com>,
 Thomas Bonnefille <thomas.bonnefille@...tlin.com>,
 Hans de Goede <hansg@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-gpio@...r.kernel.org
Subject: Re: [PATCH 2/3] iio: adc: Support ROHM BD79112 ADC/GPIO

On 02/09/2025 18:14, David Lechner wrote:
> On 9/2/25 7:24 AM, Matti Vaittinen wrote:
>> 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.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@...il.com>
>> ---
>>   drivers/iio/adc/Kconfig        |  10 +
>>   drivers/iio/adc/Makefile       |   1 +
>>   drivers/iio/adc/rohm-bd79112.c | 542 +++++++++++++++++++++++++++++++++
>>   3 files changed, 553 insertions(+)
>>   create mode 100644 drivers/iio/adc/rohm-bd79112.c
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index e3d3826c3357..4b78929bb257 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -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_I2C
> 
> I think you want SPI rather than I2C. :-)

Ouch! :) Well spotted! Thanks!

> 
>> +	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.
>> +
> 
> 
> 
>> +struct bd79112_data {
>> +	struct spi_device *spi;
>> +	struct regmap *map;
>> +	struct device *dev;
>> +	struct gpio_chip gc;
>> +	unsigned long gpio_valid_mask;
>> +	unsigned int vref_mv;
>> +	struct spi_transfer read_xfer[2];
>> +	struct spi_transfer write_xfer;
>> +	struct spi_message read_msg;
>> +	struct spi_message write_msg;
>> +	/* 16-bit TX, valid data in high byte */
>> +	u8 read_tx[2] __aligned(IIO_DMA_MINALIGN);
>> +	/* 8-bit address followed by 8-bit data */
>> +	u8 reg_write_tx[2] __aligned(IIO_DMA_MINALIGN);
>> +	/* 12-bit of ADC data or 8 bit of reg data */
>> +	__be16 read_rx __aligned(IIO_DMA_MINALIGN);
> 
> Usually, we only need one __aligned(IIO_DMA_MINALIGN) (on the first
> field). Since these are only used for SPI messages and we can only
> send one message at a time, there isn't a way for there to be a
> problem that would require them to each need to be in their own
> cache line.

I was wondering about this and hoping to get a good comment explaining 
it :) I noticed I don't really know how different SPI controllers handle 
DMA or cache. Hence I just went with what felt like safest option - and 
hoped to get a comment like yours if it wasn't needed ;) So, Thanks!

> 
>> +};
>> +
> 
> 
> 
>> +static int bd79112_probe(struct spi_device *spi)
>> +{
> 
> ...
> 
>> +	iio_dev->channels = cs;
>> +	iio_dev->num_channels = ret;
> 
> This is quite far from where it is assigned. Better to have a dedicated
> local variable for this.

Gah! I agree. Actually there is now a bug where the 
iio_dev->num_channels is used before it is set. So, datasheet names 
won't be assigned correctly. (I did some re-ordering of stuff in probe 
to cover the 'all ADCs and all GPIOs cases. I must've messed this at 
that point!).

Thanks! I feel like I owe you a beer :] Just remind me if we meet! ;)

Yours,
	-- Matti