Re: [PATCH V2 1/2] pinctrl: Introduce PINCTRL_STATE_DEFAULT, define hogs as that state

[Dong Aisheng <aisheng.dong@freescale.com>]

On Tue, Feb 28, 2012 at 09:26:22AM -0800, Stephen Warren wrote:
> Dong Aisheng wrote at Tuesday, February 28, 2012 2:30 AM:
> > On Mon, Feb 27, 2012 at 04:55:08PM -0700, 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
> 
> > >  static struct pinctrl_map __initdata mapping[] = {
> > > -       PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
> > > +	PIN_MAP(PINCTRL_STATE_DEFAULT, "pinctrl-foo", "i2c0", "foo-i2c.0"),
> >
> > It seemed Linus already applied your "re-order struct pinctrl_map".
> > This may need change a bit according to that patch.
> 
> Yes, that patch is applied.
> 
> However, note that I deliberately did not change PIN_MAP()'s parameter
> order in that patch. I deferred as much rework of those macros to the
> later patch "pinctrl: Enhance mapping table to support pin config
> operations" to avoid repeatedly changing those macros where possible.
> 
> For reference, the definition of PIN_MAP() at this point in the series
> is:
> 
> #define PIN_MAP(a, b, c, d) \
>         { .name = a, .ctrl_dev_name = b, .function = c, .dev_name = d }
> 
Ok, i see.

> (.group is still allowed to be NULL)
> 
> > > @@ -1078,8 +944,6 @@ static void pinctrl_init_device_debugfs(struct pinctrl_dev *pctldev)
> > >  			    device_root, pctldev, &pinctrl_groups_ops);
> > >  	debugfs_create_file("gpio-ranges", S_IFREG | S_IRUGO,
> > >  			    device_root, pctldev, &pinctrl_gpioranges_ops);
> > > -	debugfs_create_file("pinmux-hogs", S_IFREG | S_IRUGO,
> > > -			    device_root, pctldev, &pinmux_hogs_ops);
> >
> > I see you remove the pinnmux-hogs sysfs file here.
> > I guess you may want to merge it into pinctrl-maps sysfs file since
> > they're almost same(only difference is device name).
> > 
> > Do you think if we can add a special flag for that type of map in sysfs
> > (e.g. a "Hog" flag behind the regular map debug info)?
> > Then users do not need to check device's name to see if it's a hogged function.
> > 
> > (Anyway, it should be in another patch)
> 
> I'd rather not myself; I don't see why "hog" should be a special-case;
> it's just that the pin controller driver's mapping table entries have
> it set up certain pin mux/config settings.
> 
> That said, feel free to submit a patch for this. I'd prefer that any
> text you add to the debugfs files to indicate this "hogging" be in
> addition to anything that's already present rather than replacing it
> (as has unfortunately happened already in "pinmux-pins"), otherwise
> it obscures information.
> 
Yes, correct.

> > > diff --git a/include/linux/pinctrl/machine.h b/include/linux/pinctrl/machine.h
> > > index 73fbb27..20e9735 100644
> > > --- a/include/linux/pinctrl/machine.h
> > > +++ b/include/linux/pinctrl/machine.h
> > > @@ -12,6 +12,8 @@
> > >  #ifndef __LINUX_PINCTRL_MACHINE_H
> > >  #define __LINUX_PINCTRL_MACHINE_H
> > >
> > > +#include "pinctrl.h"
> > > +
> > >  /**
> > >   * struct pinctrl_map - boards/machines shall provide this map for devices
> > >   * @dev_name: the name of the device using this specific mapping, the name
> > > @@ -49,17 +51,18 @@ struct pinctrl_map {
> > >   * Convenience macro to map a system function onto a certain pinctrl device,
> > >   * to be hogged by the pin control core until the system shuts down.
> > >   */
> > > -#define PIN_MAP_SYS_HOG(a, b, c) \
> > > -	{ .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, }
> > > +#define PIN_MAP_SYS_HOG(a, b) \
> > > +	{ .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
> > > +	  .function = b, }
> > >
> > >  /*
> > >   * Convenience macro to map a system function onto a certain pinctrl device
> > >   * using a specified group, to be hogged by the pin control core until the
> > >   * system shuts down.
> > >   */
> > > -#define PIN_MAP_SYS_HOG_GROUP(a, b, c, d)		\
> > > -	{ .name = a, .ctrl_dev_name = b, .dev_name = b, .function = c, \
> > > -	  .group = d, }
> > > +#define PIN_MAP_SYS_HOG_GROUP(a, b, c) \
> > > +	{ .name = PINCTRL_STATE_DEFAULT, .ctrl_dev_name = a, .dev_name = a, \
> > > +	  .function = b, .group = c, }
> > >
> > 
> > In your v1 patch, we discussed about the possible requirement of different state
> > support for hog functions(it seemed still no conclusion, right?).
> 
> I agreed to fully support that. However as I mentioned, the support will be
Great.

> actually implemented in patch "pinctrl: Enhance mapping table to support
> pin config operations" since I was already reworking all the mapping
> table macros there, it felt better to do the rework once in that patch.
> 
Reasonable to me.

> I'm waiting on resolution of the locking patch issue before I repost an
> updated version of the mapping table rework patch that includes this
> support.
> 
Sounds good.

> > Acked-by: Dong Aisheng <dong.aisheng@linaro.org>
> 

Regards
Dong Aisheng