lkml.org 
[lkml]   [2012]   [Feb]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 19/20] pinctrl: API changes to support multiple states per device
    On Sun, Feb 19, 2012 at 11:45:59PM -0700, Stephen Warren wrote:
    > The API model is changed from:
    >
    > p = pinctrl_get(dev, "state1");
    > pinctrl_enable(p);
    > ...
    > pinctrl_disable(p);
    > pinctrl_put(p);
    > p = pinctrl_get(dev, "state2");
    > pinctrl_enable(p);
    > ...
    > pinctrl_disable(p);
    > pinctrl_put(p);
    >
    > to this:
    >
    > p = pinctrl_get(dev);
    > s1 = pinctrl_lookup_state(p, "state1");
    > s2 = pinctrl_lookup_state(p, "state2");
    > pinctrl_select_state(p, s1);
    > ...
    > pinctrl_select_state(p, s2);
    > ...
    > pinctrl_put(p);
    >
    > This allows devices to directly transition between states without
    > disabling the pin controller programming and put()/get()ing the
    > configuration data each time. This model will also better suit pinconf
    > programming, which doesn't have a concept of "disable".
    >
    It's exactly what we discussed and wanted.
    Thank for the great and hard work.

    > The special-case hogging feature of pin controllers is re-written to use
    > the regular APIs instead of special-case code. Hence, the pinmux-hogs
    > debugfs file is removed; see the top-level pinctrl-handles files for
    > equivalent data.
    >
    > Signed-off-by: Stephen Warren <swarren@nvidia.com>
    > ---
    > Documentation/pinctrl.txt | 153 ++++++++-----
    > arch/arm/mach-u300/core.c | 28 +--
    > drivers/pinctrl/core.c | 491 +++++++++++++++++--------------------
    > drivers/pinctrl/core.h | 24 ++-
    > drivers/tty/serial/sirfsoc_uart.c | 12 +-
    > include/linux/pinctrl/consumer.h | 55 ++++-
    > include/linux/pinctrl/machine.h | 21 +-
    > 7 files changed, 408 insertions(+), 376 deletions(-)
    >

    >
    > @@ -814,7 +817,7 @@ it even more compact which assumes you want to use pinctrl-foo and position
    > 0 for mapping, for example:
    >
    > static struct pinctrl_map __initdata mapping[] = {
    > - PIN_MAP("I2CMAP", "pinctrl-foo", "i2c0", "foo-i2c.0"),
    > + PIN_MAP("default", "pinctrl-foo", "i2c0", "foo-i2c.0"),
    I remember you said you will also change the PIN_MAP sequence that
    place the device name at first.
    Will see your further patch.

    > The result of grabbing this mapping from the device with something like
    > this (see next paragraph):
    >
    > - p = pinctrl_get(&device, "8bit");
    > + p = pinctrl_get(dev);
    > + s = pinctrl_lookup_state(p, "8bit");
    > + ret = pinctrl_select_state(p, s);
    > +
    > +or more simply:
    > +
    > + p = pinctrl_get_select(dev, "8bit");
    >
    Very glad to see this simply one. :-)

    > foo_probe()
    > {
    > - /* Allocate a state holder named "state" etc */
    > - struct pinctrl p;

    > + /* Allocate a state holder named "foo" etc */
    > + struct foo_state *foo = ...;
    >
    > - p = pinctrl_get(&device, "default");
    > - if IS_ERR(p)
    > - return PTR_ERR(p);
    > - pinctrl_enable(p);
    > + foo->p = pinctrl_get(&device);
    > + if (IS_ERR(foo->p)) {
    > + /* FIXME: clean up "foo" here */
    > + return PTR_ERR(foo->p);
    > + }
    > +
    > + foo->s = pinctrl_lookup_state(foo->p, "default");
    > + if (IS_ERR(foo->s)) {
    > + pinctrl_put(foo->p);
    > + /* FIXME: clean up "foo" here */
    > + return PTR_ERR(s);
    > + }
    >
    > - state->p = p;
    > + ret = pinctrl_select_state(s);
    s/s/foo->s

    > @@ -976,25 +1004,25 @@ System pin control hogging
    > ==========================
    >
    > Pin control map entries can be hogged by the core when the pin controller
    > -is registered. This means that the core will attempt to call pinctrl_get() and
    > -pinctrl_enable() on it immediately after the pin control device has been
    > -registered.
    > +is registered. This means that the core will attempt to call pinctrl_get(),
    > +lookup_state() and select_state() on it immediately after the pin control
    > +device has been registered.
    >
    > -This is enabled by simply setting the .dev_name field in the map to the name
    > -of the pin controller itself, like this:
    > +This occurs for mapping table entries where the client device name is equal
    > +to the pin controller device name, and the state name is "active".
    >
    > {
    > - .name = "POWERMAP"
    > + .dev_name = "pinctrl-foo",
    > + .name = "active",
    I guess if we really decide to use a fixed state name for hog functions,
    we'd better not let users to write the state name, at least provide a macro.

    > .ctrl_dev_name = "pinctrl-foo",
    > .function = "power_func",
    > - .dev_name = "pinctrl-foo",
    > },
    >
    > Since it may be common to request the core to hog a few always-applicable
    > mux settings on the primary pin controller, there is a convenience macro for
    > this:
    >
    > -PIN_MAP_SYS_HOG("POWERMAP", "pinctrl-foo", "power_func")
    > +PIN_MAP_SYS_HOG("active", "pinctrl-foo", "power_func")
    I remember you had a patch changing this macro a bit.
    So here may also need change.

    > +static struct pinctrl *find_pinctrl(struct device *dev)
    > +{
    > + struct pinctrl *p;
    >
    > - dev_dbg(dev, "pinctrl_get() for state %s\n", name);
    > + list_for_each_entry(p, &pinctrldev_list, node)
    s/pinctrldev_list/pinctrl_list ?

    > +static struct pinctrl *pinctrl_get_locked(struct device *dev)
    > +{
    > + struct pinctrl *p;
    >
    > -error:
    > - list_for_each_entry(setting, &p->settings, node)
    > - pinmux_free_setting(setting);
    > + if (WARN_ON(!dev))
    > + return ERR_PTR(-EINVAL);
    >
    > - kfree(p);
    > + p = find_pinctrl(dev);
    > + if (p == NULL)
    > + p = create_pinctrl(dev);
    > + if (IS_ERR(p))
    > + return p;
    > +
    > + p->usecount++;
    I still can not understand what's the purpose of p->usecount?
    For allowing multi times calling of pinctrl_get for on the same device?

    > +static inline struct pinctrl * __must_check pinctrl_get_select(
    > + struct device *dev, const char *name)
    > +{
    > + struct pinctrl *p;
    > + struct pinctrl_state *s;
    > + int ret;
    > +
    > + p = pinctrl_get(dev);
    > + if (IS_ERR(p))
    > + return p;
    > +
    > + s = pinctrl_lookup_state(p, name);
    > + if (IS_ERR(s)) {
    > + pinctrl_put(p);
    > + return ERR_PTR(PTR_ERR(s));
    > + }
    > +
    > + ret = pinctrl_select_state(p, s);
    > + if (ret < 0) {
    > + pinctrl_put(p);
    > + return ERR_PTR(ret);
    s/ERR_PTR(ret)/ret ?

    Regards
    Dong Aisheng



    \
     
     \ /
      Last update: 2012-02-27 10:03    [W:0.050 / U:31.600 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site