| 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
| ||
|
Message-ID: <2943205c-e7dc-4ca1-a174-15df2244c77b@baylibre.com>
Date: Thu, 16 May 2024 15:17:17 +0200
From: Angelo Dureghello <adureghello@...libre.com>
To: David Lechner <dlechner@...libre.com>
Cc: jic23@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, nuno.sa@...log.com, lars@...afoo.de,
Michael.Hennerich@...log.com, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] iio: dac: ad3552r: add support for ad3541r and
ad3551r
Hi David,
On 10/05/24 5:42 PM, David Lechner wrote:
> On Fri, May 10, 2024 at 9:19 AM Angelo Dureghello
> <adureghello@...libre.com> wrote:
>> From: Angelo Dureghello <adureghello@...libre.com>
>>
>> Add support for single-output dac variants.
>>
>> Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
>> ---
>> drivers/iio/dac/ad3552r.c | 39 +++++++++++++++++++++++++++++----------
>> 1 file changed, 29 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/iio/dac/ad3552r.c b/drivers/iio/dac/ad3552r.c
>> index a492e8f2fc0f..0dd6f995c3e2 100644
>> --- a/drivers/iio/dac/ad3552r.c
>> +++ b/drivers/iio/dac/ad3552r.c
>> @@ -140,7 +140,9 @@ enum ad3552r_ch_vref_select {
>> };
>>
>> enum ad3542r_id {
>> + AD3541R_ID = 0x400b,
>> AD3542R_ID = 0x4009,
>> + AD3551R_ID = 0x400a,
>> AD3552R_ID = 0x4008,
>> };
>>
>> @@ -745,7 +747,8 @@ static void ad3552r_calc_gain_and_offset(struct ad3552r_desc *dac, s32 ch)
>> } else {
>> /* Normal range */
>> idx = dac->ch_data[ch].range;
>> - if (dac->chip_id == AD3542R_ID) {
>> + if (dac->chip_id == AD3541R_ID ||
>> + dac->chip_id == AD3542R_ID) {
>> v_min = ad3542r_ch_ranges[idx][0];
>> v_max = ad3542r_ch_ranges[idx][1];
>> } else {
>> @@ -780,7 +783,7 @@ static int ad3552r_find_range(u16 id, s32 *vals)
>> int i, len;
>> const s32 (*ranges)[2];
>>
>> - if (id == AD3542R_ID) {
>> + if (id == AD3541R_ID || id == AD3542R_ID) {
>> len = ARRAY_SIZE(ad3542r_ch_ranges);
>> ranges = ad3542r_ch_ranges;
>> } else {
>> @@ -955,9 +958,10 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
>> dev_err(dev, "mandatory reg property missing\n");
>> goto put_child;
>> }
>> - if (ch >= AD3552R_NUM_CH) {
>> - dev_err(dev, "reg must be less than %d\n",
>> - AD3552R_NUM_CH);
>> + if (ch >= AD3552R_NUM_CH ||
>> + (dac->chip_id == AD3541R_ID && ch) ||
>> + (dac->chip_id == AD3551R_ID && ch)) {
>> + dev_err(dev, "channel %d is not supported\n", ch);
>> err = -EINVAL;
>> goto put_child;
>> }
>> @@ -987,9 +991,10 @@ static int ad3552r_configure_device(struct ad3552r_desc *dac)
>> goto put_child;
>>
>> dac->ch_data[ch].range = val;
>> - } else if (dac->chip_id == AD3542R_ID) {
>> + } else if (dac->chip_id == AD3541R_ID ||
>> + dac->chip_id == AD3542R_ID) {
>> dev_err(dev,
>> - "adi,output-range-microvolt is required for ad3542r\n");
>> + "adi,output-range-microvolt is required for ad354xr\n");
>> err = -EINVAL;
>> goto put_child;
>> } else {
>> @@ -1088,10 +1093,20 @@ static int ad3552r_probe(struct spi_device *spi)
>> return err;
>>
>> /* Config triggered buffer device */
>> - if (dac->chip_id == AD3552R_ID)
>> - indio_dev->name = "ad3552r";
>> - else
>> + switch (dac->chip_id) {
>> + case AD3541R_ID:
>> + indio_dev->name = "ad3541r";
>> + break;
>> + case AD3542R_ID:
>> indio_dev->name = "ad3542r";
>> + break;
>> + case AD3551R_ID:
>> + indio_dev->name = "ad3551r";
>> + break;
>> + case AD3552R_ID:
>> + indio_dev->name = "ad3552r";
>> + break;
>> + }
>> indio_dev->dev.parent = &spi->dev;
>> indio_dev->info = &ad3552r_iio_info;
>> indio_dev->num_channels = dac->num_ch;
>> @@ -1110,14 +1125,18 @@ static int ad3552r_probe(struct spi_device *spi)
>> }
>>
>> static const struct spi_device_id ad3552r_id[] = {
>> + { "ad3541r", AD3541R_ID },
>> { "ad3542r", AD3542R_ID },
>> + { "ad3551r", AD3551R_ID },
>> { "ad3552r", AD3552R_ID },
>> { }
>> };
>> MODULE_DEVICE_TABLE(spi, ad3552r_id);
>>
>> static const struct of_device_id ad3552r_of_match[] = {
>> + { .compatible = "adi,ad3541r"},
>> { .compatible = "adi,ad3542r"},
>> + { .compatible = "adi,ad3551r"},
>> { .compatible = "adi,ad3552r"},
>> { }
>> };
>> --
>> 2.45.0.rc1
>>
>>
> It looks like it is time for a chip_info struct here instead of the if
> and switch statements to get chip-specific data. Most other IIO
> drivers have this already and it is the preferred way to look up this
> kind of information in the IIO subsystem. I prefer the drivers that
> don't put all of the info structs in an array (that way the code is
> less verbose). So I would suggest looking at e.g. adc/aspeed_adc,
> starting with aspeed_adc_matches, to see what I mean and how to
> implement it. (So one patch to add the info structs and a second patch
> to add the single channel chips)
Ack, will change in that way.
Regards,
angelo
Powered by blists - more mailing lists