RE: [PATCH 1/3] pinctrl: Introduce PINCTRL_STATE_DEFAULT define,and use it

[Stephen Warren]

Dong Aisheng wrote at Friday, February 24, 2012 12:10 AM:
> On Thu, Feb 23, 2012 at 09:26:14PM -0800, Stephen Warren wrote:
> > Dong Aisheng wrote at Thursday, February 23, 2012 8:25 PM:
> > > On Fri, Feb 24, 2012 at 08:04:38AM +0800, Stephen Warren wrote:
> > > > This provides a single centralized name for the default state.
> > > >
> > > > Update PIN_MAP_* macros to use this state name, instead of requiring the
> > > > user to pass a state name in.
> > ...
> > > > diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
> > ...
> > > > -       PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
> > > > +	PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-foo", "i2c0", "foo-i2c.0"),
> > >
> > > To keep align with the following
> > > PIN_MAP_SYS_HOG("pinmux-u300", "power"),
> > > Maybe PIN_MAP("pinctrl-foo", "i2c0", "foo-i2c.0") is better, right?
> > >
> > > And we may want to add a PIN_MAP_STAT macro, as well as PIN_MAP_SYS_HOG_STAT.
> >
> > I don't think so.
> >
> > PIN_MAP_SYS_HOG hard-codes the state name inside the macro because its
> > sole purpose is to support drivers/pinctrl/core.c's pinctrl_get() call
> > for pin hogging purposes, so that state name is always "default", or
> > rather, PINCTRL_STATE_DEFAULT.
> 
> I'm considering that we may also need to support multi states for system hog
> functions, do you agree?

Well, I don't think that'd be hogging; to my mind, hogging is specifically 
setting up PINCTRL_STATE_DEFAULT when a pin controller is registered.

But yes, I can see that the pinctrl core might want to use other state
names for a pin controller, e.g. for suspend/resume, when unregistering
the pin controller driver, etc.

That's exactly why in the mapping table rework patch I posted, I wrote
a single all-encompassing table entry macro that'd work for all these
situations, without the need to create a ton of special-case macros.

Still, I suppose now I've already enhanced that patch to support a few
special case macros, I can add yet more. But, please can I defer this
to that later patch rather than this one, since I'm already reworking
all the macros there to support the enhanced mapping table format for
pin config too?

> > BTW, did you have any further thoughts on (not) allowing NULL state names?
> > I'd like to repost the final version of that patch, or rework it so that
> > the code actually does allow NULL state names ASAP (i.e. early Friday for
> > me), since it blocks some of the later patches in the series. Thanks.
>
> Personally i wish to support NULL state name.
> But if it really can't, i can also accept no NULL state names.

OK, can I take that as an ACK for my patch that completely removes the
partial support for them ("Assume map table entries can't have a NULL
name filed")?

> You mentioned it may cause many problems on DT side to support NULL state.
> I'm still thinking the problems we're facing for dt and if we can fix it.
> 
> Thinking the model we discussed in Linaro connect:
> sdhci@c8000200 {
> 	pinctrl-0 = <&pmx_sdhci &pmx_sdhci_active>;
> 	pinctrl-1 = <&pmx_sdhci &pmx_sdhci_standby>;
> 	/* An optional list of state names; depends on SDHCI driver */
> 	pinctrl-names = "active", "suspend";
> };
> The orginally purpose will convert the id of pinctrl-0 (eg. it's '0' here)
> to be the state name if no pincgtrl-names property defined.
> I'm wondering can we not use that id, just treat this case as no state names?
> If that we can allow dt to define null state names.
> And it's easy for map writers to write such map.
> 
> But i do not know how much troubles it will be caused in implementation since
> you're writing all these code.
> 
> What do you think of it?

That sure seems like adding a special-case to the device tree parsing
code solely to provide justification for a special case in the mapping
table processing code.

My root issue here: Why do we want a NULL name field? It serves no
benefit that I can see, slightly complicates the code, obscures information
in device drivers (since they aren't requesting a particular state name)
and the pinctrl debugfs files (since we have these unnamed states, so
people need to know what that means), etc.

Given I've agreed to make macros that set .name = PINCTRL_STATE_DEFAULT
so that no mapping table author will need to do this, I don't think that
having .name=PINCTRL_STATE_DEFAULT is going to save anything either.

-- 
nvpublic

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