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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZsiWALpt1IpTHsKg@smile.fi.intel.com>
Date: Fri, 23 Aug 2024 17:00:32 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Chen-Yu Tsai <wenst@...omium.org>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>,
	Matthias Brugger <matthias.bgg@...il.com>,
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>,
	Wolfram Sang <wsa@...nel.org>, Benson Leung <bleung@...omium.org>,
	Tzung-Bi Shih <tzungbi@...nel.org>, Mark Brown <broonie@...nel.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	chrome-platform@...ts.linux.dev, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Douglas Anderson <dianders@...omium.org>,
	Johan Hovold <johan@...nel.org>, Jiri Kosina <jikos@...nel.org>,
	linux-i2c@...r.kernel.org
Subject: Re: [PATCH v5 08/10] i2c: of-prober: Add GPIO support

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@...ux.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