Re: [PATCH v11 2/3] iio: adc: max14001: New driver

[David Lechner <dlechner@baylibre.com>]

On 9/22/25 7:56 PM, Marilene Andrade Garcia wrote:
> On 16/09/2025 15:04, David Lechner wrote:
>> On 9/15/25 5:16 PM, Marilene Andrade Garcia wrote:

...


In general, please trim out extra stuff like I've done here when you
reply. It makes it easier to find the important parts. I hope I didn't
miss any of your questions.

>>> +    /*
>>> +     * The following buffers will be bit-reversed during device
>>> +     * communication, because the device transmits and receives data
>>> +     * LSB-first.
>>> +     * DMA (thus cache coherency maintenance) requires the transfer
>>> +     * buffers to live in their own cache lines.
>>> +     */
>>> +    __be16 spi_tx_buffer __aligned(IIO_DMA_MINALIGN);
>>> +    __be16 spi_rx_buffer;
>>
>> These would no longer be strictly big-endian, so we could
>> just make them u16 with a note in the comments.
> 
> Hello David, I have some doubts that I would like to clarify before sending v12. Since I am not able to test the case using SPI_LSB_FIRST, I noticed that you suggest saving the data as __le in this case. Just out of curiosity, if I use SPI_LSB_FIRST, would saving the data as __be lead to errors?

My thinking is that since we are sending things out 1 byte at a time, in order
for the least significant bit of 16 bits to be sent first, the least significant
byte has to be sent first. So will little-endian, the byte containing the least
significant bit of the 16 bits will be first in memory.

__be is just a naming convention and doesn't actually cause any bytes to
be swapped in memory. It just lets readers of the code know that the
value stored there has to be handled carefully because it may not be
cpu-endian. It would be confusing to readers to store a little-endian
value in a __be16 variable, but technically, no, it would not cause any
errors.

This is why I suggested to make it u16. It is still wrong but it is
equally wrong in both cases. If you still want to use __be16 though,
you could make a union instead.

union {
	__be16 be;
	__le16 le;
} spi_tx_buffer;
union {
	__be16 be;
	__le16 le;
} spi_rx_buffer;

>>
>> The scoped_guard() looks a bit odd with the extra indent. I would write
>> it like this instead:
>>
>>
>>
>>     case IIO_CHAN_INFO_RAW: {
>>         guard(mutex)(&st->lock);
>>
>>         ret = regmap_read(st->regmap, MAX14001_REG_ADC, val);
>>         if (ret)
>>             return ret;
>>
>>         return IIO_VAL_INT;
>>     }
>>
> 
> Ok, thank you. But since I removed the mutex, as it was mentioned in the first comments, I should not use the guard, right? At least not for now.
> 

Correct. The regmap_read() has something similar internally already.