| 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: <20240928151930.0c22e5e9@jic23-huawei>
Date: Sat, 28 Sep 2024 15:19:30 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: Mariel Tinaco <Mariel.Tinaco@...log.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, Lars-Peter
Clausen <lars@...afoo.de>, Rob Herring <robh@...nel.org>, Krzysztof
Kozlowski <krzk+dt@...nel.org>, Michael Hennerich
<Michael.Hennerich@...log.com>, Conor Dooley <conor+dt@...nel.org>, Marcelo
Schmitt <marcelo.schmitt1@...il.com>, Dimitri Fedrau
<dima.fedrau@...il.com>, David Lechner <dlechner@...libre.com>, Nuno
Sá <noname.nuno@...il.com>
Subject: Re: [PATCH v4 2/2] iio: dac: support the ad8460 Waveform DAC
On Sat, 14 Sep 2024 20:21:56 +0200
Christophe JAILLET <christophe.jaillet@...adoo.fr> wrote:
> Le 12/09/2024 à 11:54, Mariel Tinaco a écrit :
> > The AD8460 is a “bits in, power out” high voltage, high-power,
> > high-speed driver optimized for large output current (up to ±1 A)
> > and high slew rate (up to ±1800 V/μs) at high voltage (up to ±40 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 ±55 V and a single low voltage
> > supply of 5 V.
> >
> > Signed-off-by: Mariel Tinaco <Mariel.Tinaco-OyLXuOCK7orQT0dZR+AlfA@...lic.gmane.org>
> > ---
Rather than go around again, I fixed up all the comments made
and the autobuilder issues then applied this.
Diff follows. The only bit I'm not 100% sure on was your intent
with the temperature channel. I've made it an input but shout if
I'm missing something.
With this diff applied on top, applied to the togreg branch of iio.git
which is only pushed out as testing for now as I'll rebase on rc1
once available.
Thanks,
Jonathan
diff --git a/drivers/iio/dac/ad8460.c b/drivers/iio/dac/ad8460.c
index 9ce3a0f288ba..dc8c76ba573d 100644
--- a/drivers/iio/dac/ad8460.c
+++ b/drivers/iio/dac/ad8460.c
@@ -533,7 +533,7 @@ static int ad8460_write_event_value(struct iio_dev *indio_dev,
enum iio_event_info info, int val, int val2)
{
struct ad8460_state *state = iio_priv(indio_dev);
- unsigned int fault;
+ int fault;
if (type != IIO_EV_TYPE_THRESH)
return -EINVAL;
@@ -555,7 +555,7 @@ static int ad8460_read_event_value(struct iio_dev *indio_dev,
enum iio_event_info info, int *val, int *val2)
{
struct ad8460_state *state = iio_priv(indio_dev);
- unsigned int fault;
+ int fault;
if (type != IIO_EV_TYPE_THRESH)
return -EINVAL;
@@ -576,7 +576,7 @@ static int ad8460_write_event_config(struct iio_dev *indio_dev,
enum iio_event_direction dir, int val)
{
struct ad8460_state *state = iio_priv(indio_dev);
- unsigned int fault;
+ int fault;
if (type != IIO_EV_TYPE_THRESH)
return -EINVAL;
@@ -594,9 +594,8 @@ static int ad8460_read_event_config(struct iio_dev *indio_dev,
enum iio_event_direction dir)
{
struct ad8460_state *state = iio_priv(indio_dev);
- unsigned int fault;
+ int fault, ret;
bool en;
- int ret;
if (type != IIO_EV_TYPE_THRESH)
return -EINVAL;
@@ -660,14 +659,14 @@ static const struct iio_enum ad8460_powerdown_mode_enum = {
};
#define AD8460_CHAN_EXT_INFO(_name, _what, _read, _write) { \
- .name = _name, \
+ .name = (_name), \
.read = (_read), \
.write = (_write), \
.private = (_what), \
.shared = IIO_SEPARATE, \
}
-static struct iio_chan_spec_ext_info ad8460_ext_info[] = {
+static const struct iio_chan_spec_ext_info ad8460_ext_info[] = {
AD8460_CHAN_EXT_INFO("raw0", 0, ad8460_dac_input_read,
ad8460_dac_input_write),
AD8460_CHAN_EXT_INFO("raw1", 1, ad8460_dac_input_read,
@@ -709,7 +708,7 @@ static struct iio_chan_spec_ext_info ad8460_ext_info[] = {
IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad8460_powerdown_mode_enum),
IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
&ad8460_powerdown_mode_enum),
- {}
+ { }
};
static const struct iio_event_spec ad8460_events[] = {
@@ -761,7 +760,6 @@ static const struct iio_event_spec ad8460_events[] = {
#define AD8460_TEMP_CHAN { \
.type = IIO_TEMP, \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
- .output = 1, \
.indexed = 1, \
.channel = 0, \
.scan_index = -1, \
@@ -792,9 +790,9 @@ static const char * const ad8460_supplies[] = {
static int ad8460_probe(struct spi_device *spi)
{
+ struct device *dev = &spi->dev;
struct ad8460_state *state;
struct iio_dev *indio_dev;
- struct device *dev;
u32 tmp[2], temp;
int ret;
@@ -808,14 +806,15 @@ static int ad8460_probe(struct spi_device *spi)
indio_dev->info = &ad8460_info;
state->spi = spi;
- dev = &spi->dev;
state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
if (IS_ERR(state->regmap))
return dev_err_probe(dev, PTR_ERR(state->regmap),
"Failed to initialize regmap");
- devm_mutex_init(dev, &state->lock);
+ ret = devm_mutex_init(dev, &state->lock);
+ if (ret)
+ return ret;
state->sync_clk = devm_clk_get_enabled(dev, NULL);
if (IS_ERR(state->sync_clk))
@@ -835,10 +834,9 @@ static int ad8460_probe(struct spi_device *spi)
ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ad8460_supplies),
ad8460_supplies);
- if (ret) {
- dev_err(dev, "Failed to enable power supplies\n");
- return ret;
- }
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed to enable power supplies\n");
ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v");
if (ret < 0 && ret != -ENODEV)
@@ -908,9 +906,8 @@ static int ad8460_probe(struct spi_device *spi)
return ret;
/* Enables DAC by default */
- ret = regmap_update_bits(state->regmap, AD8460_CTRL_REG(0x01),
- AD8460_HVDAC_SLEEP_MSK,
- FIELD_PREP(AD8460_HVDAC_SLEEP_MSK, 0));
+ ret = regmap_clear_bits(state->regmap, AD8460_CTRL_REG(0x01),
+ AD8460_HVDAC_SLEEP_MSK);
if (ret)
return ret;
>
> Hi,
>
> ...
>
> > +#define AD8460_CHAN_EXT_INFO(_name, _what, _read, _write) { \
> > + .name = _name, \
> > + .read = (_read), \
> > + .write = (_write), \
> > + .private = (_what), \
>
> Why () for _read, _write, _what?
> (or why no () for _name?)
>
> > + .shared = IIO_SEPARATE, \
> > +}
> > +
> > +static struct iio_chan_spec_ext_info ad8460_ext_info[] = {
>
> I think this can be static const struct.
>
> > + AD8460_CHAN_EXT_INFO("raw0", 0, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw1", 1, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw2", 2, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw3", 3, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw4", 4, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw5", 5, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw6", 6, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw7", 7, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw8", 8, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw9", 9, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw10", 10, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw11", 11, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw12", 12, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw13", 13, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw14", 14, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("raw15", 15, ad8460_dac_input_read,
> > + ad8460_dac_input_write),
> > + AD8460_CHAN_EXT_INFO("toggle_en", 0, ad8460_read_toggle_en,
> > + ad8460_write_toggle_en),
> > + AD8460_CHAN_EXT_INFO("symbol", 0, ad8460_read_symbol,
> > + ad8460_write_symbol),
> > + AD8460_CHAN_EXT_INFO("powerdown", 0, ad8460_read_powerdown,
> > + ad8460_write_powerdown),
> > + IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad8460_powerdown_mode_enum),
> > + IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> > + &ad8460_powerdown_mode_enum),
> > + {}
> > +};
>
> ...
>
> > +static int ad8460_probe(struct spi_device *spi)
> > +{
> > + struct ad8460_state *state;
> > + struct iio_dev *indio_dev;
> > + struct device *dev;
> > + u32 tmp[2], temp;
> > + int ret;
> > +
> > + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + state = iio_priv(indio_dev);
> > +
> > + indio_dev->name = "ad8460";
> > + indio_dev->info = &ad8460_info;
> > +
> > + state->spi = spi;
> > + dev = &spi->dev;
> > +
> > + state->regmap = devm_regmap_init_spi(spi, &ad8460_regmap_config);
> > + if (IS_ERR(state->regmap))
> > + return dev_err_probe(dev, PTR_ERR(state->regmap),
> > + "Failed to initialize regmap");
> > +
> > + devm_mutex_init(dev, &state->lock);
> > +
> > + state->sync_clk = devm_clk_get_enabled(dev, NULL);
> > + if (IS_ERR(state->sync_clk))
> > + return dev_err_probe(dev, PTR_ERR(state->sync_clk),
> > + "Failed to get sync clk\n");
> > +
> > + state->tmp_adc_channel = devm_iio_channel_get(dev, "ad8460-tmp");
> > + if (IS_ERR(state->tmp_adc_channel)) {
> > + if (PTR_ERR(state->tmp_adc_channel) == -EPROBE_DEFER)
> > + return -EPROBE_DEFER;
> > + indio_dev->channels = ad8460_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels);
> > + } else {
> > + indio_dev->channels = ad8460_channels_with_tmp_adc;
> > + indio_dev->num_channels = ARRAY_SIZE(ad8460_channels_with_tmp_adc);
> > + }
> > +
> > + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(ad8460_supplies),
> > + ad8460_supplies);
> > + if (ret) {
> > + dev_err(dev, "Failed to enable power supplies\n");
> > + return ret;
>
> Nitpick: return dev_err_probe() as done in other places?
>
> > + }
> > +
> > + ret = devm_regulator_get_enable_read_voltage(dev, "refio_1p2v");
> > + if (ret < 0 && ret != -ENODEV)
> > + return dev_err_probe(dev, ret, "Failed to get reference voltage\n");
> > +
> > + state->refio_1p2v_mv = ret == -ENODEV ? 1200 : ret / 1000;
>
> ...
>
> CJ
Powered by blists - more mailing lists