Received-SPF: pass (google.com: domain of linux-kernel+bounces-175972-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240510141836.1624009-1-adureghello@baylibre.org> <20240510141836.1624009-2-adureghello@baylibre.org>
In-Reply-To: <20240510141836.1624009-2-adureghello@baylibre.org>
From: David Lechner <dlechner@baylibre.com>
Date: Fri, 10 May 2024 10:42:49 -0500
Message-ID: <CAMknhBFrOdzvo+aEFjMSf_3FGmbhVp42Oymt_DEF2L-CdWiSmQ@mail.gmail.com>
Subject: Re: [PATCH 2/3] iio: dac: ad3552r: add support for ad3541r and ad3551r
To: Angelo Dureghello <adureghello@baylibre.com>
Cc: jic23@kernel.org, robh@kernel.org, krzk+dt@kernel.org, conor+dt@kernel.org, 
	nuno.sa@analog.com, lars@metafoo.de, Michael.Hennerich@analog.com, 
	linux-iio@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, May 10, 2024 at 9:19=E2=80=AFAM Angelo Dureghello
<adureghello@baylibre.com> wrote:
>
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> Add support for single-output dac variants.
>
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.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 =3D 0x400b,
>         AD3542R_ID =3D 0x4009,
> +       AD3551R_ID =3D 0x400a,
>         AD3552R_ID =3D 0x4008,
>  };
>
> @@ -745,7 +747,8 @@ static void ad3552r_calc_gain_and_offset(struct ad355=
2r_desc *dac, s32 ch)
>         } else {
>                 /* Normal range */
>                 idx =3D dac->ch_data[ch].range;
> -               if (dac->chip_id =3D=3D AD3542R_ID) {
> +               if (dac->chip_id =3D=3D AD3541R_ID ||
> +                   dac->chip_id =3D=3D AD3542R_ID) {
>                         v_min =3D ad3542r_ch_ranges[idx][0];
>                         v_max =3D 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 =3D=3D AD3542R_ID) {
> +       if (id =3D=3D AD3541R_ID || id =3D=3D AD3542R_ID) {
>                 len =3D ARRAY_SIZE(ad3542r_ch_ranges);
>                 ranges =3D ad3542r_ch_ranges;
>         } else {
> @@ -955,9 +958,10 @@ static int ad3552r_configure_device(struct ad3552r_d=
esc *dac)
>                         dev_err(dev, "mandatory reg property missing\n");
>                         goto put_child;
>                 }
> -               if (ch >=3D AD3552R_NUM_CH) {
> -                       dev_err(dev, "reg must be less than %d\n",
> -                               AD3552R_NUM_CH);
> +               if (ch >=3D AD3552R_NUM_CH ||
> +                       (dac->chip_id =3D=3D AD3541R_ID && ch) ||
> +                       (dac->chip_id =3D=3D AD3551R_ID && ch)) {
> +                       dev_err(dev, "channel %d is not supported\n", ch)=
;
>                         err =3D -EINVAL;
>                         goto put_child;
>                 }
> @@ -987,9 +991,10 @@ static int ad3552r_configure_device(struct ad3552r_d=
esc *dac)
>                                 goto put_child;
>
>                         dac->ch_data[ch].range =3D val;
> -               } else if (dac->chip_id =3D=3D AD3542R_ID) {
> +               } else if (dac->chip_id =3D=3D AD3541R_ID ||
> +                          dac->chip_id =3D=3D AD3542R_ID) {
>                         dev_err(dev,
> -                               "adi,output-range-microvolt is required f=
or ad3542r\n");
> +                               "adi,output-range-microvolt is required f=
or ad354xr\n");
>                         err =3D -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 =3D=3D AD3552R_ID)
> -               indio_dev->name =3D "ad3552r";
> -       else
> +       switch (dac->chip_id) {
> +       case AD3541R_ID:
> +               indio_dev->name =3D "ad3541r";
> +               break;
> +       case AD3542R_ID:
>                 indio_dev->name =3D "ad3542r";
> +               break;
> +       case AD3551R_ID:
> +               indio_dev->name =3D "ad3551r";
> +               break;
> +       case AD3552R_ID:
> +               indio_dev->name =3D "ad3552r";
> +               break;
> +       }
>         indio_dev->dev.parent =3D &spi->dev;
>         indio_dev->info =3D &ad3552r_iio_info;
>         indio_dev->num_channels =3D dac->num_ch;
> @@ -1110,14 +1125,18 @@ static int ad3552r_probe(struct spi_device *spi)
>  }
>
>  static const struct spi_device_id ad3552r_id[] =3D {
> +       { "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[] =3D {
> +       { .compatible =3D "adi,ad3541r"},
>         { .compatible =3D "adi,ad3542r"},
> +       { .compatible =3D "adi,ad3551r"},
>         { .compatible =3D "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)