Re: [PATCH v5 08/10] i2c: of-prober: Add GPIO support

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Aug 23, 2024 at 06:32:16PM +0800, Chen-Yu Tsai wrote:
> On Thu, Aug 22, 2024 at 10:20 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Aug 22, 2024 at 05:20:01PM +0800, Chen-Yu Tsai wrote:

...

> > > +     if (!data->gpiods)
> > > +             return 0;
> >
> > If it comes a new code (something else besides GPIOs and regulators) this
> > will be a (small) impediment. Better to have a helper for each case and do
> >
> >         ret = ..._gpiods();
> >         if (ret)
> >                 ...
> >
> > Same for regulators and anything else in the future, if any.
> 
> I'm not sure I follow. Do you mean wrap each individual type in a wrapper
> and call those here, like the following?
> 
>     i2c_of_probe_enable_res(...)
>     {
>         ret = i2c_of_probe_enable_regulators(...)
>         if (ret)
>               return ret;
> 
>         ret = i2c_of_probe_enable_gpios(...)
>         if (ret)
>               goto error_disable_regulators;
> 
>         ...
>     }

Yes.

...

> > > +             /*
> > > +              * reset GPIOs normally have opposite polarity compared to
> >
> > "reset"
> >
> > > +              * enable GPIOs. Instead of parsing the flags again, simply
> >
> > "enable"
> >
> > > +              * set the raw value to high.
> >
> > This is quite a fragile assumption. Yes, it would work in 98% cases, but will
> > break if it's not true somewhere else.
> 
> Well, this seems to be the de facto standard. Or it would have to remember
> what each GPIO descriptor's name is, and try to classify those into either
> "enable" or "reset", and set their respective logical values to 1 or 0.
> And then you run into a peripheral with a broken binding that has its
> "reset" GPIO inverted, i.e. it's driver behavior needs to follow the
> "enable" GPIO style. The class of devices this prober targets are
> consumer electronics (laptops, tablets, phones) that at least have gone
> through some component selection where the options won't have conflicting
> requirements.

I'm talking from real life example(s) :-)

Recently I looked at the OV7251 sensor driver that expects "enable" GPIO while
all users supply "reset"-as-"enable" with the exact trouble I described.
Yet it's pure software / ABI issue in that case, but who knows what PCB
engineers may come up with.

> And if the polarities of the possible components don't line up, then this
> probe structure can't really do anything. One would need something that
> power sequences each component separately and probes it. I would really
> like to avoid that if possible, as it makes the boot time (to peripheral
> available) dependent on which component you have and how far down the
> list it is. We have Chromebooks that have 4 touchscreen components
> introduced over the years. In that case something more like Doug's
> original proposal would work better: something that forces mutual
> exclusivity among a class of devices.

Maybe. I just pointed out the potential problem.

> > > +              */

-- 
With Best Regards,
Andy Shevchenko