Re: [PATCH v3 6/6] iio: adc: Add support for AD4000

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

Sun, Jun 09, 2024 at 10:23:54AM +0100, Jonathan Cameron kirjoitti:

...

> > > +	/*
> > > +	 * In 4-wire mode, the CNV line is held high for the entire
> > > conversion
> > > +	 * and acquisition process. In other modes st->cnv_gpio is NULL and
> > > is
> > > +	 * ignored (CS is wired to CNV in those cases).
> > > +	 */
> > > +	gpiod_set_value_cansleep(st->cnv_gpio, 1);  
> > 
> > Not sure it's a good practise to assume internal details as you're going for
> > GPIO. I would prefer to have an explicit check for st->cnv_gpio being NULL or
> > not.
> 
> Hmm. I had it in my head that this was documented behaviour, but
> I can't find such in the docs, so agreed checking it makes sense.
> 
> I would be very surprised if this ever changed as it's one of the
> things that makes optional gpios easy to work with but who knows!

Not Linus and not Bart, but we have tons of drivers that call GPIO APIs
unconditionally as long as they want optional GPIO.

What I see here is the comment that should be rewritten to say something like

"if GPIO is defined blablabla, otherwise blablabla."

I.o.w. do not mention that implementation detail (being NULL, i.e. optional).

-- 
With Best Regards,
Andy Shevchenko