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

[Marilene Andrade Garcia <marilene.agarcia@gmail.com>]

On 23/09/2025 11:27, David Lechner wrote:
> 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.
> 
Ok, Thank you for the answers.

Best Regards,
Marilene