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

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 03/09/2025 14:23, Andy Shevchenko wrote:
> On Wed, Sep 03, 2025 at 09:52:02AM +0300, Matti Vaittinen wrote:
>> On 02/09/2025 17:15, Andy Shevchenko wrote:
>>> On Tue, Sep 02, 2025 at 03:24:31PM +0300, Matti Vaittinen wrote:
> 
> ...
> 
>>>> +	unsigned int vref_mv;
>>>
>>> Perhaps _mV to follow the actual unit spelling?
>>> (and yes, I know that both variants are present in the kernel)
>>
>> The 'mv' is a deliberate choise. I still remember our previous discussion
>> about this same topic [1].
> 
> Rings a bell. And I still think the proper spelling is more important when
> we talk about physics.
> 
> ...
> 
>> TLDR; I can combine the conditions in one if (), thanks.
> 
> Works for me.
> 
> ...
> 
>>>> +static int bd79112_gpio_dir_set(struct bd79112_data *data, unsigned int offset,
>>>> +				int dir)
>>>> +{
>>>> +	unsigned int set_reg, clear_reg, bit;
>>>> +	int ret;
>>>> +
>>>> +	bit = GET_GPIO_BIT(offset);
>>>> +
>>>> +	if (dir == GPIO_LINE_DIRECTION_IN) {
>>>> +		set_reg = GET_GPI_EN_REG(offset);
>>>> +		clear_reg = GET_GPO_EN_REG(offset);
>>>> +	} else {
>>>> +		set_reg = GET_GPO_EN_REG(offset);
>>>> +		clear_reg = GET_GPI_EN_REG(offset);
>>>> +	}
>>>
>>>> +	ret = regmap_set_bits(data->map, set_reg, bit);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	return regmap_clear_bits(data->map, clear_reg, bit);
>>>
>>> I believe the order depends on the out-in or in-out switch.
>>> Otherwise it might be potential glitches on input (hw) buffer.
>>> Right now when it's not an interrupt it may be okay to don't
>>> bother, but in general I see a potential issues with that.
>>
>> Can you please explain what you mean. I am not sure I can follow you here.
> 
> If we set out first with enabled input, input can get a value from the output.
> If the value before was different, it will be an event condition (of course
> it all depends on the HW and I haven't read datasheet for this one).

Ah. I guess I see what you mean. I have a feeling there is a problem at 
the GPIO user side, if it keeps listening for inputs after it calls 
direction to be switched to output.

But yes. I believe turning off the 'input listening' first, and toggling 
direction to output only afterwards makes sense. So, thanks. And thanks 
for explaining this further.

> ...
> 
>>>> +static int bd79112_probe(struct spi_device *spi)
>>>> +{
>>>> +	/* ADC channels as named in the data-sheet */
>>>> +	static const char * const chan_names[] = {
>>>> +		"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A", "AGIO4A", "AGIO5A",
>>>> +		"AGIO6A", "AGIO7A", "AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",
>>>> +		"AGIO11A", "AGIO12A", "AGIO13A", "AGIO14A", "AGIO15A",
>>>> +		"AGIO0B", "AGIO1B", "AGIO2B", "AGIO3B", "AGIO4B", "AGIO5B",
>>>> +		"AGIO6B", "AGIO7B", "AGIO8B", "AGIO9B", "AGIO10B", "AGIO11B",
>>>> +		"AGIO11B", "AGIO12B", "AGIO13B", "AGIO14B", "AGIO15B",
>>>
>>> Can you make all of the lines to be the same in terms of amount of entries?
>>
>> Maybe :) I would like to know why? As you know, I prefer to keep lines short
>> to fit multiple terminals in parallel, so this will probably make the entry
>> to consume more rows. Thus, I would like to have a solid reason.
> 
> Sure, the array above is unindexed. It's prone to errors and typos.

Ha. Thanks :) I see it now when I counted the entries :) Should be 32, 
was 34. I agree this would have been easier to spot!

> Moreover, it's really hard to follow in case one needs to debug such
> a typo and see which value needs to be fixed (imagine you typed twice
> the same name).

Or, if I typed twice the same name twice ;) Thanks!

> 
> Recommended way is to use power-of-two per line (and even add a comment
> at the end), like
> 
> static const char * const chan_names[] = {
> 	"AGIO0A", "AGIO1A", "AGIO2A", "AGIO3A",		/*  0 -  3 */
> 	"AGIO4A", "AGIO5A", "AGIO6A", "AGIO7A",		/*  4 -  7 */
> 	"AGIO8A", "AGIO9A", "AGIO10A", "AGIO11A",	/*  8 - 11 */
> 	...
> 
> (or hexadecimal offsets, whatever is better and more in accordance with
>   the SW / data sheet).

Ok, This makes sense now.

> 
>>>> +	};
> 
> ...
> 
>>>> +	data->vref_mv = ret / 1000;
>>>
>>> (MICRO / MILLI)
>>
>> I find this much more confusing than plain 1000. (I know we had this type of
>> discussion before. See [1] again).
> 
> Rings a bell, but that's what IIO reviewers suggest to do nowadays as a
> compromise between creating a new bunch of unit (V) related definitions.

I am sorry, but this just seems stupid to me. I'd say that it is very 
obvious for most of the readers dividing microvolts by 1000 results 
millivolts. And if it is not, then having this MICRO / MILLI is likely 
to just cause more confusion.

I _really_ dislike these defines. Why is MILLI 1000? Why it isn't 0.001? 
It makes no sense that KILO and MILLI are the same. Especially not when 
we are dealing with physics.

This is just an obfuscation compared to using plain 1000. (I kind of 
understand having a define for a value like 100000 - where counting the 
zeros gets cumbersome, although 100 * 1000 would be equally clear. But 
1000 _is_ really 100% clear, whereas MICRO / MILLI is not).

>>>> +	ret = devm_iio_adc_device_alloc_chaninfo_se(dev, &bd79112_chan_template,
>>>> +		BD79112_MAX_NUM_CHANNELS - 1, &cs);
>>>
>>> Hmm... Indentation can be amended.
>>
>> Sorry but I am not sure I understand what you mean by amended? Can you
>> please go an extra mile and explain :)
> 
> 	ret = devm_iio_adc_device_alloc_chaninfo_se(dev,
> 						    &bd79112_chan_template,
> 						    BD79112_MAX_NUM_CHANNELS - 1,
> 						    &cs);
> 

Ah, Ok. I'll do this, thanks.

> ...
> 
>>>> +	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
>>>> +					  iio_dev->num_channels);
>>>
>>>> +
>>>
>>> Instead of leaving this rather unneeded blank line I would move above...
>>>
>>>> +	/* We're done if all channels are reserved for ADC. */
>>>
>>> ...to be here
>>>
>>> 	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
>>> 					  iio_dev->num_channels);
>>
>> I suppose you mean something like:
>>
>> register_gpios:
>> 	/* We're done if all channels are reserved for ADC. */
>> 	gpio_pins = bd79112_get_gpio_pins(iio_dev->channels,
>>                                            iio_dev->num_channels);
>> 	if (!gpio_pins)
>> 		return 0;
>>
>> right?
> 
> Yes.
> 
>> I don't like this because now the comment suggests we do call
>> bd79112_get_gpio_pins() only to see if all channels were for ADCs. This,
>> however, is not THE reason for this call, only an optimization. I think:
>> having:
>>
>>          /* We're done if all channels are reserved for ADC. */
> 
> Then you can amend the comment
> 
>           /* If all channels are reserved for ADC, we are done. */
> 
>>          if (!gpio_pins)
>>                  return 0;
>>
>> is clearer.
> 
> Which makes my approach sustainable.

I like your wording better, but placing this comment before the call to 
bd79112_get_gpio_pins() is still more confusing that placing it before 
the actual check:
	if (!gpio_pins)
is still misleading. Comment applies to the check, not the retrieval.


Yours,
	-- Matti