linux-kernel - Re: [RFC PATCH 2/3] iio: adc: Add support for TI ADS1120

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] [day] [month] [year] [list]

Message-ID: <de424e9d-95cd-4a42-8f1a-97ad04f5f9ef@baylibre.com>
Date: Fri, 7 Nov 2025 10:18:41 -0600
From: David Lechner <dlechner@...libre.com>
To: Ajith Anandhan <ajithanandhan0406@...il.com>, linux-iio@...r.kernel.org
Cc: jic23@...nel.org, nuno.sa@...log.com, andy@...nel.org, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 2/3] iio: adc: Add support for TI ADS1120

On 11/7/25 9:50 AM, Ajith Anandhan wrote:
> On 10/31/25 2:43 AM, David Lechner wrote:
>> On 10/30/25 11:34 AM, Ajith Anandhan wrote:
>>> Add driver for the Texas Instruments ADS1120, a precision 16-bit
>>> analog-to-digital converter with an SPI interface.
>>>

One note about the review process. Any suggestions you agree with, you
don't need to reply to specifically. You can trim out those parts in
your reply. It saves time for those reading the replies.

>>> +struct ads1120_state {
>>> +    struct spi_device    *spi;
>>> +    struct mutex        lock;
>>> +    /*
>>> +     * Used to correctly align data.
>>> +     * Ensure natural alignment for potential future timestamp support.
>>> +     */
>>> +    u8 data[4] __aligned(IIO_DMA_MINALIGN);
>>> +
>>> +    u8 config[4];
>>> +    int current_channel;
>>> +    int gain;
>> Since inputs are multiplexed, we can make this gain a per-channel value, no?
> 
> Yes we can, However i want  to keep the initial version simple so would it be
> 
> fine to support per-channel gain configurationin upcoming patches?

Absolutely. I really appreciate splitting things up like that as it makes
it much easier to review.

> 
>>
>> It also sounds like that certain mux input have to have the PGA bypassed
>> which means they are limited to only 3 gain values. So these would have
>> a different scale_available attribute.
> 
>  Since, I'm gonna enable all the 15 channels. I believe we have to have all
> 
> gains(for differential channels). Correct me if i'm wrong.

Yes, that is how I understood the datasheet. Differential channels have all
gains. Single-ended channels and diagnostic channels only get the low gains
(1, 2, 4).


>>> +static int ads1120_write_reg(struct ads1120_state *st, u8 reg, u8 value)
>>> +{
>>> +    u8 buf[2];
>>> +
>>> +    if (reg > ADS1120_REG_CONFIG3)
>>> +        return -EINVAL;
>>> +
>>> +    buf[0] = ADS1120_CMD_WREG | (reg << 2);
>>> +    buf[1] = value;
>>> +
>>> +    return spi_write(st->spi, buf, 2);
>>> +}
>> Can we use the regmap framework instead of writing our own?
> 
> I’d like to keep the first version simple so i will add the regmap support in the
> 
> later patch since the single ended has less spi transaction to handle.

It would be less churn to implement the regmap right away. Typically
we try to avoid churn if we can help it. So this would be an exception
to the general suggestion that splitting things up is better.