Received-SPF: pass (google.com: domain of linux-kernel+bounces-176051-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240510064053.278257-1-Mariel.Tinaco@analog.com> <20240510064053.278257-3-Mariel.Tinaco@analog.com>
In-Reply-To: <20240510064053.278257-3-Mariel.Tinaco@analog.com>
From: David Lechner <dlechner@baylibre.com>
Date: Fri, 10 May 2024 12:30:06 -0500
Message-ID: <CAMknhBFMrY7LSTUK+PxCkekEha7T_OyVyjcT8sKTQc+9zP7gHA@mail.gmail.com>
Subject: Re: [PATCH 2/2] iio: dac: support the ad8460 Waveform DAC
To: Mariel Tinaco <Mariel.Tinaco@analog.com>
Cc: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, 
	linux-kernel@vger.kernel.org, Jonathan Cameron <jic23@kernel.org>, 
	Lars-Peter Clausen <lars@metafoo.de>, Rob Herring <robh@kernel.org>, 
	Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>, 
	Liam Girdwood <lgirdwood@gmail.com>, Mark Brown <broonie@kernel.org>, 
	Michael Hennerich <Michael.Hennerich@analog.com>, Marcelo Schmitt <marcelo.schmitt1@gmail.com>, 
	Dimitri Fedrau <dima.fedrau@gmail.com>, Guenter Roeck <linux@roeck-us.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, May 10, 2024 at 1:42=E2=80=AFAM Mariel Tinaco <Mariel.Tinaco@analog=
com> wrote:
>
> The AD8460 is a =E2=80=9Cbits in, power out=E2=80=9D high voltage, high-p=
ower,
> highspeed driver optimized for large output current (up to =C2=B11 A)

high-speed

> and high slew rate (up to =C2=B11800 V/=CE=BCs) at high voltage (up to =
=C2=B140 V)
> into capacitive loads.
>
> A digital engine implements user-configurable features: modes for
> digital input, programmable supply current, and fault monitoring
> and programmable protection settings for output current,
> output voltage, and junction temperature. The AD8460 operates on
> high voltage dual supplies up to =C2=B155 V and a single low voltage
> supply of 5 V.
>
> Signed-off-by: Mariel Tinaco <Mariel.Tinaco@analog.com>
> ---

..

> diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c
> new file mode 100644
> index 000000000..4049be0f5
> --- /dev/null
> +++ b/drivers/iio/dac/ad8460.c

..

> +static int ad8460_probe(struct spi_device *spi)
> +{
> +       struct ad8460_state *state;
> +       struct iio_dev *indio_dev;
> +       struct regulator *vrefio;
> +       int ret;
> +
> +       indio_dev =3D devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +       if (!indio_dev)
> +               return -ENOMEM;
> +
> +       state =3D iio_priv(indio_dev);
> +       mutex_init(&state->lock);
> +
> +       state->spi =3D spi;
> +
> +       state->regmap =3D devm_regmap_init_spi(spi, &ad8460_regmap_config=
);
> +       if (IS_ERR(state->regmap))
> +               return dev_err_probe(&spi->dev, PTR_ERR(state->regmap),
> +                                    "Failed to initialize regmap");
> +
> +       ret =3D devm_iio_dmaengine_buffer_setup_ext(&spi->dev, indio_dev,=
 "tx",
> +                                                 IIO_BUFFER_DIRECTION_OU=
T);
> +       if (ret)
> +               return dev_err_probe(&spi->dev, ret,
> +                                    "Failed to get DMA buffer\n");

It looks like the dmas property is missing in the DT bindings.
However, as I suggested in my comments on that patch, it might make
more sense to implement the parallel data part as an IIO backend. I
assume this is using one of ADI's FPGA IP blocks to get the data?

> +
> +       state->sync_clk =3D devm_clk_get_enabled(&spi->dev, "sync_clk");

The DT bindings don't specify clock-names, which is perfectly fine and
perhaps even preferred since there is only one clock. But that means
the ID here should be NULL instead of "sync_clk".

> +       if (IS_ERR(state->sync_clk))
> +               return dev_err_probe(&spi->dev, PTR_ERR(state->sync_clk),
> +                                    "Failed to get sync clk\n");
> +


> +       vrefio =3D devm_regulator_get_optional(&spi->dev, "vrefio");
> +       if (IS_ERR(vrefio)) {
> +               if (PTR_ERR(vrefio) !=3D -ENODEV)
> +                       return dev_err_probe(&spi->dev, PTR_ERR(vrefio),
> +                                            "Failed to get vref regulato=
r\n");
> +
> +               state->vref_mv =3D 1200;
> +
> +       } else {
> +               ret =3D regulator_enable(vrefio);
> +               if (ret)
> +                       return dev_err_probe(&spi->dev, ret,
> +                                            "Failed to enable vrefio reg=
ulator\n");
> +
> +               ret =3D devm_add_action_or_reset(&spi->dev,
> +                                              ad8460_regulator_disable,
> +                                              vrefio);
> +               if (ret)
> +                       return ret;
> +
> +               ret =3D regulator_get_voltage(vrefio);
> +               if (ret < 0)
> +                       return dev_err_probe(&spi->dev, ret,
> +                                            "Failed to get ref voltage\n=
");
> +
> +               if (!in_range(ret, AD8460_MIN_VREFIO_UV, AD8460_MAX_VREFI=
O_UV))
> +                       return dev_err_probe(&spi->dev, -EINVAL,
> +                                            "Invalid ref voltage range(%=
u) [%u, %u]\n",
> +                                            ret, AD8460_MIN_VREFIO_UV,
> +                                            AD8460_MAX_VREFIO_UV);
> +
> +               state->vref_mv =3D ret / 1000;
> +       }

FYI, if all goes well, starting with kernel 6.10-rc1 (we'll have to
wait a few weeks for this), this regulator section can be simplified
to:

ret =3D devm_regulator_get_enable_read_voltage(&spi->dev, "vrefio");
if (ret =3D=3D -ENODEV) {
    /* no supply, using internal 1.2V reference */
    state->vref_mv =3D 1200;
} else if (ret < 0) {
    return  dev_err_probe(&spi->dev, ret, "Failed to get reference voltage\=
n");
} else {
    /* external reference */
    state->vref_mv =3D ret / 1000;
}

if (!in_range(...

We can always fix it up later though if you don't want to wait. :-)

> +
> +       ret =3D device_property_read_u32(&spi->dev, "adi,rset-ohms",
> +                                      &state->rset_ohms);

Since 0 isn't an allowable value for R_SET, it seems like we need to
return on error here or assign a default value if the property is
missing. If we do a default value, the DT bindings need to be updated
to reflect that as well.

> +       if (!ret) {
> +               if (!in_range(state->rset_ohms, AD8460_MIN_RSET_OHMS,
> +                             AD8460_MAX_RSET_OHMS))
> +                       return dev_err_probe(&spi->dev, -EINVAL,
> +                                            "Invalid resistor set range(=
%u) [%u, %u]\n",
> +                                            state->rset_ohms,
> +                                            AD8460_MIN_RSET_OHMS,
> +                                            AD8460_MAX_RSET_OHMS);
> +       }
> +
> +       ret =3D ad8460_reset(state);
> +       if (ret)
> +               return ret;
> +
> +       /* Enables DAC by default */
> +       ret =3D regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),

I was going to suggest giving names to the registers instead of using
the number, but the datasheet doesn't do that! :-(

Oh well, I guess this is the best we can do.

> +                                AD8460_HVDAC_SLEEP_MSK,
> +                                FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0));
> +       if (ret)
> +               return ret;
> +
> +       indio_dev->name =3D "ad8460";
> +       indio_dev->info =3D &ad8460_info;
> +       indio_dev->channels =3D ad8460_channels;
> +       indio_dev->num_channels =3D ARRAY_SIZE(ad8460_channels);
> +       indio_dev->modes =3D INDIO_DIRECT_MODE | INDIO_BUFFER_HARDWARE;

devm_iio_dmaengine_buffer_setup_ext() already sets the
INDIO_BUFFER_HARDWARE flag so this is redundant. Also,
devm_iio_dmaengine_buffer_setup_ext() is usually called here rather
than early in probe because it needs a few things in indio_dev
populated, IIRC.

> +       indio_dev->setup_ops =3D &ad8460_buffer_setup_ops;
> +
> +       ret =3D devm_iio_device_register(&spi->dev, indio_dev);
> +       if (ret)
> +               return ret;
> +
> +       ad8460_debugfs_init(indio_dev);
> +
> +       return 0;
> +}
> +

..