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@xxxxxxxxxxxxxxx> 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
