linux-kernel - Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240914124527.1b89a660@jic23-huawei>
Date: Sat, 14 Sep 2024 12:45:27 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: "Tinaco, Mariel" <Mariel.Tinaco@...log.com>
Cc: David Lechner <dlechner@...libre.com>, "linux-iio@...r.kernel.org"
 <linux-iio@...r.kernel.org>, "devicetree@...r.kernel.org"
 <devicetree@...r.kernel.org>, "linux-kernel@...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>,
 "Hennerich, Michael" <Michael.Hennerich@...log.com>, Conor Dooley
 <conor+dt@...nel.org>, Marcelo Schmitt <marcelo.schmitt1@...il.com>,
 Dimitri Fedrau <dima.fedrau@...il.com>, Nuno Sá
 <noname.nuno@...il.com>
Subject: Re: [PATCH v3 2/2] iio: dac: support the ad8460 Waveform DAC

Hi Mariel,

You get to be the person I moan at today (though not the only person
doing it!).

Please crop to only the relevant information for continuing the 
discussion.  It is not necessarily easy to find where you reply if you
keep too much context.


...

> > > +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);
> > > +	mutex_init(&state->lock);
> > > +
> > > +	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");
> > > +
> > > +	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");
> > 
> > Should we check for specific errors here? For example what if we get -
> > EPROBE_DEFER? Also, it doesn't like we could ever get NULL, so IS_ERR()
> > should be sufficient.
> >   
> 
> It says in the docs that, the intended channel might return -EPROBE_DEFER
> If the driver associated with that channel depends on resources that are not
> Yet available. For this specific case, should I create a loop that waits for
> That channel to be available before proceeding with the probe function?

Normally I'd say fail the probe with EPROBE_DEFER but in this case
it's awkward because this is far from a 'required' feature and whilst
DT providing the channel would indicate that the board supports using
it, that doesn't mean a given system has the driver for the ADC.

I don't want to suggest we make this a CONFIG_XXX option but I can't
immediately see an alternative that lets people intentionally not build
the driver support for the ADC.

> 
> How would this be implemented?
> 
> Originally, this channel was intended to be optional. If any error results from
> Obtaining it, it will not be included in the channels.
> 
Given IIO drivers will probe in an unknown order this will fail perhaps half the
time.  Normally deferral deals with that, because the consumer isn't useful
without the channel.  Here it is... 

I'm open to other suggestions on this but right now it looks like only way
to definitely handle it is a config option.

Maybe for now we don't provide one and see if anyone cares?  That is
effectively make if required to provide an ADC driver if the DT describes
the channel we are getting. If that driver isn't loaded yet -EPROBE_DEFER and
wait for it to show up.

Jonathan