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

[Jonathan Cameron <jic23@kernel.org>]

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