| 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
| ||
|
Date: Thu, 15 Feb 2024 15:22:08 +0200
From: Alisa-Dariana Roman <alisadariana@...il.com>
To: David Lechner <dlechner@...libre.com>
Cc: alexandru.tachici@...log.com, alisa.roman@...log.com,
conor+dt@...nel.org, devicetree@...r.kernel.org, jic23@...nel.org,
krzysztof.kozlowski+dt@...aro.org, krzysztof.kozlowski@...aro.org,
lars@...afoo.de, linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
michael.hennerich@...log.com, robh+dt@...nel.org,
Nuno Sa <nuno.sa@...log.com>
Subject: Re: [PATCH v3 5/5] iio: adc: ad7192: Add AD7194 support
Hello and thank you for the feedback!
On 09.02.2024 00:27, David Lechner wrote:
> On Thu, Feb 8, 2024 at 11:25 AM Alisa-Dariana Roman
> <alisadariana@...il.com> wrote:
>>
>> Unlike the other AD719Xs, AD7194 has configurable differential
>> channels. The default configuration for these channels can be changed
>> from the devicetree.
..
>>
>> +static const struct iio_info ad7194_info = {
>> + .read_raw = ad7192_read_raw,
>> + .write_raw = ad7192_write_raw,
>> + .write_raw_get_fmt = ad7192_write_raw_get_fmt,
>> + .read_avail = ad7192_read_avail,
>> + .validate_trigger = ad_sd_validate_trigger,
>> + .update_scan_mode = ad7192_update_scan_mode,
>> +};
>
> Isn't this identical to ad7192_info and ad7195_info now that .attrs is
> removed? It seems like we could consolidate here.
Those are not exactly identical since: 92 has bridge switch attribute,
95 has bridge switch and ac excitation attributes and 94 has no custom
attributes. I used a different info structure for 94 in order to avoid
showing extra attributes.
>
>> +
>> static const struct iio_info ad7195_info = {
>> .read_raw = ad7192_read_raw,
>> .write_raw = ad7192_write_raw,
>> @@ -1009,6 +1049,80 @@ static const struct iio_chan_spec ad7193_channels[] = {
>> IIO_CHAN_SOFT_TIMESTAMP(14),
>> };
>>
>> +static struct iio_chan_spec ad7194_channels[] = {
>> + AD7193_DIFF_CHANNEL(0, 1, 2, 0x001),
>> + AD7193_DIFF_CHANNEL(1, 3, 4, 0x023),
>> + AD7193_DIFF_CHANNEL(2, 5, 6, 0x045),
>> + AD7193_DIFF_CHANNEL(3, 7, 8, 0x067),
>> + AD7193_DIFF_CHANNEL(4, 9, 10, 0x089),
>> + AD7193_DIFF_CHANNEL(5, 11, 12, 0x0AB),
>> + AD7193_DIFF_CHANNEL(6, 13, 14, 0x0CD),
>> + AD7193_DIFF_CHANNEL(7, 15, 16, 0x0EF),
>> + AD719x_TEMP_CHANNEL(8, AD7194_CH_TEMP),
>> + AD7193_CHANNEL(9, 1, AD7194_CH_AIN1),
>> + AD7193_CHANNEL(10, 2, AD7194_CH_AIN2),
>> + AD7193_CHANNEL(11, 3, AD7194_CH_AIN3),
>> + AD7193_CHANNEL(12, 4, AD7194_CH_AIN4),
>> + AD7193_CHANNEL(13, 5, AD7194_CH_AIN5),
>> + AD7193_CHANNEL(14, 6, AD7194_CH_AIN6),
>> + AD7193_CHANNEL(15, 7, AD7194_CH_AIN7),
>> + AD7193_CHANNEL(16, 8, AD7194_CH_AIN8),
>> + AD7193_CHANNEL(17, 9, AD7194_CH_AIN9),
>> + AD7193_CHANNEL(18, 10, AD7194_CH_AIN10),
>> + AD7193_CHANNEL(19, 11, AD7194_CH_AIN11),
>> + AD7193_CHANNEL(20, 12, AD7194_CH_AIN12),
>> + AD7193_CHANNEL(21, 13, AD7194_CH_AIN13),
>> + AD7193_CHANNEL(22, 14, AD7194_CH_AIN14),
>> + AD7193_CHANNEL(23, 15, AD7194_CH_AIN15),
>> + AD7193_CHANNEL(24, 16, AD7194_CH_AIN16),
>
> Shouldn't these be differential channels since they are
> pseudo-differential inputs measuring the difference between AINx and
> AINCOM?
>
>> + IIO_CHAN_SOFT_TIMESTAMP(25),
>> +};
>
> i.e. like this (where AINCOM is voltage0 AINx is voltagex)
>
> static struct iio_chan_spec ad7194_channels[] = {
> AD7193_DIFF_CHANNEL(0, 1, 0, AD7194_CH_AIN1),
> AD7193_DIFF_CHANNEL(1, 2, 0, AD7194_CH_AIN2),
> AD7193_DIFF_CHANNEL(2, 3, 0, AD7194_CH_AIN3),
> AD7193_DIFF_CHANNEL(3, 4, 0, AD7194_CH_AIN4),
> AD7193_DIFF_CHANNEL(4, 5, 0, AD7194_CH_AIN5),
> AD7193_DIFF_CHANNEL(5, 6, 0, AD7194_CH_AIN6),
> AD7193_DIFF_CHANNEL(6, 7, 0, AD7194_CH_AIN7),
> AD7193_DIFF_CHANNEL(7, 8, 0, AD7194_CH_AIN8),
> AD7193_DIFF_CHANNEL(8, 9, 0, AD7194_CH_AIN9),
> AD7193_DIFF_CHANNEL(9, 10, 0, AD7194_CH_AIN10),
> AD7193_DIFF_CHANNEL(10, 11, 0, AD7194_CH_AIN11),
> AD7193_DIFF_CHANNEL(11, 12, 0, AD7194_CH_AIN12),
> AD7193_DIFF_CHANNEL(12, 13, 0, AD7194_CH_AIN13),
> AD7193_DIFF_CHANNEL(13, 14, 0, AD7194_CH_AIN14),
> AD7193_DIFF_CHANNEL(14, 15, 0, AD7194_CH_AIN15),
> AD7193_DIFF_CHANNEL(15, 16, 0, AD7194_CH_AIN16),
> AD719x_TEMP_CHANNEL(16, AD7194_CH_TEMP),
> IIO_CHAN_SOFT_TIMESTAMP(17),
> };
>
I tried to follow the existing style of the driver: for each
pseudo-differential channel(AINx - AINCOM) there is an iio channel like
this in_voltagex_raw; and for each differential channel(AINx - AINy)
there is an iio channel like this in_voltagex-in_voltagey_raw. AD7194
has 16 pseudo-differential channels/8 fully differential channels so I
thought the (AINx - AINCOM) channels should be static and only the 8
differential ones could be configured by the user from the devicetree by
choosing the input pins.
The existing style of the driver, AD7192 has 4 pseudo differential
channels and 2 (non configurable) differential channels:
static const struct iio_chan_spec ad7192_channels[] = {
AD719x_DIFF_CHANNEL(0, 1, 2, AD7192_CH_AIN1P_AIN2M),
AD719x_DIFF_CHANNEL(1, 3, 4, AD7192_CH_AIN3P_AIN4M),
AD719x_TEMP_CHANNEL(2, AD7192_CH_TEMP),
AD719x_DIFF_CHANNEL(3, 2, 2, AD7192_CH_AIN2P_AIN2M),
AD719x_CHANNEL(4, 1, AD7192_CH_AIN1),
AD719x_CHANNEL(5, 2, AD7192_CH_AIN2),
AD719x_CHANNEL(6, 3, AD7192_CH_AIN3),
AD719x_CHANNEL(7, 4, AD7192_CH_AIN4),
IIO_CHAN_SOFT_TIMESTAMP(8),
};
Would it be better to respect the existing style or to do like you
suggested and have a total of 16 differential channels that are
configurable from the device tree?
Kind regards,
Alisa-Dariana Roman
Powered by blists - more mailing lists