linux-kernel - RE: [PATCH 1/2 v4] pinctrl: add a pin config interface

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: <74CDBE0F657A3D45AFBB94109FB122FF174FDB01F1@HQMAIL01.nvidia.com>
Date:	Thu, 1 Dec 2011 09:12:14 -0800
From:	Stephen Warren <swarren@...dia.com>
To:	Linus Walleij <linus.walleij@...aro.org>
CC:	Linus Walleij <linus.walleij@...ricsson.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Grant Likely <grant.likely@...retlab.ca>,
	Barry Song <21cnbao@...il.com>,
	Shawn Guo <shawn.guo@...escale.com>,
	Thomas Abraham <thomas.abraham@...aro.org>,
	Dong Aisheng <dong.aisheng@...aro.org>,
	Rajendra Nayak <rajendra.nayak@...aro.org>
Subject: RE: [PATCH 1/2 v4] pinctrl: add a pin config interface

Linus Walleij wrote at Thursday, December 01, 2011 3:00 AM:
> On Wed, Nov 30, 2011 at 8:19 PM, Stephen Warren <swarren@...dia.com> wrote:
> > Linus Walleij wrote at Thursday, November 24, 2011 11:46 AM:

> >> +/**
> >> + * pin_config_get() - get the configuration of a single pin parameter
> >> + * @pctldev: pin controller device for this pin
> >> + * @pin: pin to get the config for
> >> + * @config: this config tuple will be filled in with the setting for
> >> + *   the requested parameter, so the .param part of this setting must
> >> + *   me set when calling the function
> >
> > I don't like muxing the param and value together in a single parameter;
> > it requires a bunch of bit-shifting to create them, modify the value
> > here, and extract values. Why not just have "param" and "value" function
> > parameters like the old patches?
> 
> Because since we move to the concept of the core not knowing what
> it is passing around, it is better to just have an unsigned long and let
> the driver and its board file/device tree decide what to do with it.
> 
> Since I don't have any enum for the param anymore, it would
> just be another unsigned long, so what's the point?

Well, there is an enum, it's just in the second patch set...

I think directly supporting param/value is a pretty common case, and
seems like it deserves first-class support.

Supporting it wouldn't complicated the "custom struct pointer" case at
all; you'd just hard-code one of the parameters to 0 when calling, and
ignore it when implementing the driver.

Conversely, explicitly supporting param/value would significantly simplify
drivers that do want to use this mechanism, since you wouldn't need any
custom struct types and casting, or all the pack and unpack macros, which
I think will complicated the common case right now.

> >> +int pin_config_get(struct pinctrl_dev *pctldev, int pin,
> >> +                       unsigned long *config)
> > ...
> >> +     ret = ops->pin_config_get(pctldev, pin, config);
> >> +     /*
> >> +      * -EINVAL is OK, it means the setting is not active right now
> >
> > That doesn't really make sense; given it's a param/value thing, a
> > particular param can't be "not active"; it has a particular value or
> > not.
> 
> Yeah that's generic pin config semantics, I'll take it out, remove the
> debug print and put it in the generic config documentation.
> 
> > There's no pin_config_group_get()?
> 
> I've added it for symmetry, but will that work in practice?
> 
> Say you pin_config_group_get() and pins have wildly different
> settings, what do you return?

-ENOTSUPP.

Some parameters can actually be programmed on a per-pin basis. For these,
pin_config_get/set should implement them, and pin_config_group_get/set
should return -ENOTSUPP.

Other parameters are actually set at the group level (with a HW register
that affects n pins at once). For these, pin_config_get/set should return
-ENOTSUPP, and pin_config_group_get/set should return implement them.

> >> +static inline int pin_config_get(struct pinctrl_dev *pctldev, int pin,
> >> +                              unsigned long *config)
> >> +{
> >> +     return 0;
> >> +}
> >
> > Shouldn't at least pin_config_get() return an error. It seems like both
> > pin_config_set() and pin_config_group() don't do what they're asked, so
> > they should also return an error. gpiolib takes that approach; e.g.
> > gpio_direction_input() fails in the dummy case.
> 
> We already allow say pinmux_enable() to succeed if pinmux
> is compiled out.
> 
> My idea was that it would be good for debugging to say disable
> the pinctrl altogether, then leave pins in their power-on state
> and no calls into the framework will even touch them, so you
> can see what happens. (This I think is partly the intention of the
> clk and regulator stubs.)
> 
> Having this return errors will make such binaries with pinconf
> compiled-out just not boot if the error codes are handled
> correctly, which is not very helpful.
> 
> Maybe I'm getting this all wrong?

It seems better to plug in a dummy/stub pinctrl driver for testing
purposes that allow the pinctrl APIs to succeed by default in cases
where there's no implementation, which might end up hiding problems.

-- 
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/