[PATCH 1/2] drivers: create a pin control subsystem v7

[PATCH 1/2] drivers: create a pin control subsystem v7

[Stephen Warren]

Linus Walleij wrote at Friday, September 16, 2011 6:13 AM:
> This creates a subsystem for handling of pin control devices.
> These are devices that control different aspects of package
> pins.

I've read through the documentation and header files, but not the .c files,
and this looks almost perfect as far as I can tell right now. I'll try to
review the .c files sometime too.

I just have one comment:

> diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
...
> +/* External interface to pinmux */
> +extern int pinmux_request_gpio(unsigned gpio);
> +extern void pinmux_free_gpio(unsigned gpio);
> +extern struct pinmux * __must_check pinmux_get(struct device *dev, const char *name);
> +extern void pinmux_put(struct pinmux *pmx);
> +extern int pinmux_enable(struct pinmux *pmx);
> +extern void pinmux_disable(struct pinmux *pmx);
> +extern int pinmux_config(struct pinmux *pmx, u16 param, unsigned long *data);

That definition of pinmux_config doesn't seem as useful as it could be;
I'd like the ability to execute pinmux_config on a /named/ group, and I
can certainly see a use-case for applying it to /named/ pins too.

The issues with applying pinmux_config to a mapping table entry are:

* When there are multiple mapping table entries referenced by one
pinmux_get, you don't necessarily want to apply the same configuration
to all of the groups; think of a bus with a combination of low-speed
output control signals and high-speed input data signals or something
like that.

* When muxing works in groups, you may want to apply the configuration
to individual pins rather than the whole groups using in the mapping
table.

-- 
nvpublic

[PATCH 1/2] drivers: create a pin control subsystem v7

[Linus Walleij]

On Tue, Sep 20, 2011 at 11:58 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Friday, September 16, 2011 6:13 AM:
>> This creates a subsystem for handling of pin control devices.
>> These are devices that control different aspects of package
>> pins.
>
> I've read through the documentation and header files, but not the .c files,
> and this looks almost perfect as far as I can tell right now. I'll try to
> review the .c files sometime too.

Great, I'm hunting your Acked-by/Reviewed-by ...

I will likely request inclusion into linux-next soon-ish.

> I just have one comment:
>
>> diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
> ...
>> +/* External interface to pinmux */
>> +extern int pinmux_request_gpio(unsigned gpio);
>> +extern void pinmux_free_gpio(unsigned gpio);
>> +extern struct pinmux * __must_check pinmux_get(struct device *dev, const char *name);
>> +extern void pinmux_put(struct pinmux *pmx);
>> +extern int pinmux_enable(struct pinmux *pmx);
>> +extern void pinmux_disable(struct pinmux *pmx);
>> +extern int pinmux_config(struct pinmux *pmx, u16 param, unsigned long *data);
>
> That definition of pinmux_config doesn't seem as useful as it could be;

It should be removed. It's just there in the header file, I killed off
the implementation because specific control of a mux doesn't make
sense. We want to do stuff to pin groups directly, not related to
muxing, so that kind of thing needs to be in the generic pinctrl
interface.

> I'd like the ability to execute pinmux_config on a /named/ group, and I
> can certainly see a use-case for applying it to /named/ pins too.

That sounds correct to me.

To abstract things the stuff we can do with the group should be
something enumerated too. So:

pinctrl_config_group(const char *pinctrl_device, const char *group,
const char *mode);
pinctrl_config_pin(const char *pinctrl_device, int pin, const char *mode);

So the driver need an API to enumerate pin and group modes.

I might want to save this thing for post-merge of the basic API and
pinmux stuff though so we don't try to push too much upfront
design at once.

> The issues with applying pinmux_config to a mapping table entry are:
>
> * When there are multiple mapping table entries referenced by one
> pinmux_get, you don't necessarily want to apply the same configuration
> to all of the groups; think of a bus with a combination of low-speed
> output control signals and high-speed input data signals or something
> like that.
>
> * When muxing works in groups, you may want to apply the configuration
> to individual pins rather than the whole groups using in the mapping
> table.

Yeah, we kill this old interface.

Thanks,
Linus Walleij

[PATCH 1/2] drivers: create a pin control subsystem v7

[Stephen Warren]

Linus Walleij wrote at Wednesday, September 21, 2011 3:17 AM:
> On Tue, Sep 20, 2011 at 11:58 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Linus Walleij wrote at Friday, September 16, 2011 6:13 AM:
> >> This creates a subsystem for handling of pin control devices.
> >> These are devices that control different aspects of package
> >> pins.
> >
> > I've read through the documentation and header files, but not the .c files,
> > and this looks almost perfect as far as I can tell right now. I'll try to
> > review the .c files sometime too.
> 
> Great, I'm hunting your Acked-by/Reviewed-by ...

Very close; I was just holding off until we resolved the discussion on
hog'd non-system mapping table entries, and I figured I'd wait until v8
which presumably would delete the pinmux_config function from the header?

> I will likely request inclusion into linux-next soon-ish.
> 
> > I just have one comment:
> >
> >> diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
> > ...
> >> +/* External interface to pinmux */
> >> +extern int pinmux_request_gpio(unsigned gpio);
> >> +extern void pinmux_free_gpio(unsigned gpio);
> >> +extern struct pinmux * __must_check pinmux_get(struct device *dev, const char *name);
> >> +extern void pinmux_put(struct pinmux *pmx);
> >> +extern int pinmux_enable(struct pinmux *pmx);
> >> +extern void pinmux_disable(struct pinmux *pmx);
> >> +extern int pinmux_config(struct pinmux *pmx, u16 param, unsigned long *data);
> >
> > That definition of pinmux_config doesn't seem as useful as it could be;
> 
> It should be removed. It's just there in the header file, I killed off
> the implementation because specific control of a mux doesn't make
> sense. We want to do stuff to pin groups directly, not related to
> muxing, so that kind of thing needs to be in the generic pinctrl
> interface.

OK.

> > I'd like the ability to execute pinmux_config on a /named/ group, and I
> > can certainly see a use-case for applying it to /named/ pins too.
> 
> That sounds correct to me.
> 
> To abstract things the stuff we can do with the group should be
> something enumerated too. So:
> 
> pinctrl_config_group(const char *pinctrl_device, const char *group,
> const char *mode);
> pinctrl_config_pin(const char *pinctrl_device, int pin, const char *mode);
> 
> So the driver need an API to enumerate pin and group modes.
> 
> I might want to save this thing for post-merge of the basic API and
> pinmux stuff though so we don't try to push too much upfront
> design at once.

Yes, adding in a replacement _config API can certainly be a later patch.

One comment on the API above: I think you want "mode" (or "setting"?)
/and/ a value parameter; Tegra's pull strength definition for example
is:

enum tegra_pull_strength {
        TEGRA_PULL_0 = 0,
        TEGRA_PULL_1,
// ... (every value in between)
        TEGRA_PULL_30,
        TEGRA_PULL_31,
        TEGRA_MAX_PULL,
};

And it seems better to represent that as ("pull", 0) ... ("pull", 31) than
"pull0" .. "pull31" such that the value needs to be parsed out of the "mode"
string somehow by the driver.

-- 
nvpublic

[PATCH 1/2] drivers: create a pin control subsystem v7

[Linus Walleij]

On Wed, Sep 21, 2011 at 9:45 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Wednesday, September 21, 2011 3:17 AM:
>>
>> Great, I'm hunting your Acked-by/Reviewed-by ...
>
> Very close; I was just holding off until we resolved the discussion on
> hog'd non-system mapping table entries, and I figured I'd wait until v8
> which presumably would delete the pinmux_config function from the header?

Yep, I will submit that soon. After this I plan to request its inclusion
into linux-next.

Yours,
Linus Walleij

[PATCH 1/2] drivers: create a pin control subsystem v7

[Linus Walleij]

On Wed, Sep 21, 2011 at 9:45 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Wednesday, September 21, 2011 3:17 AM:
>> To abstract things the stuff we can do with the group should be
>> something enumerated too. So:
>>
>> pinctrl_config_group(const char *pinctrl_device, const char *group,
>> const char *mode);
>> pinctrl_config_pin(const char *pinctrl_device, int pin, const char *mode);
>>
>> So the driver need an API to enumerate pin and group modes.
>>
>> I might want to save this thing for post-merge of the basic API and
>> pinmux stuff though so we don't try to push too much upfront
>> design at once.
>
> Yes, adding in a replacement _config API can certainly be a later patch.

If I'm efficient enough we might get that add-on before the v3.2
merge window, I'll iterate that patch separately though.

> One comment on the API above: I think you want "mode" (or "setting"?)
> /and/ a value parameter; Tegra's pull strength definition for example
> is:
>
> enum tegra_pull_strength {
> ? ? ? ?TEGRA_PULL_0 = 0,
> ? ? ? ?TEGRA_PULL_1,
> // ... (every value in between)
> ? ? ? ?TEGRA_PULL_30,
> ? ? ? ?TEGRA_PULL_31,
> ? ? ? ?TEGRA_MAX_PULL,
> };
>
> And it seems better to represent that as ("pull", 0) ... ("pull", 31) than
> "pull0" .. "pull31" such that the value needs to be parsed out of the "mode"
> string somehow by the driver.

OK so we might want some public defintion of "things you can do"
with pins, then an opaque parameter.

like:

#define PINCTRL_PULL "pull"
#define PINCTRL_BIAS "bias"
#define PINCTRL_LOADCAP "load-capacitance"
#define PINCTRL_DRIVE "drive"

...but then it's just simple enumerators, and we might be better off
with a simple enum after all:

enum pinctrl_pin_ops {
    PINCTRL_PULL,
    PINCTRL_BIAS,
    PINCTRL_LOADCAP,
    PINCTRL_DRIVE,
}

Surely the device tree will have to translate that enum into strings
(I guess? Sorry for my low DT competence) but that is more of a
DT pecularity and can be kept in the DT pin control parser?

Yours,
Linus Walleij

[PATCH 1/2] drivers: create a pin control subsystem v7

[Stephen Warren]

Linus Walleij wrote at Tuesday, September 27, 2011 1:51 AM:
> On Wed, Sep 21, 2011 at 9:45 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Linus Walleij wrote at Wednesday, September 21, 2011 3:17 AM:
> >> To abstract things the stuff we can do with the group should be
> >> something enumerated too. So:
> >>
> >> pinctrl_config_group(const char *pinctrl_device, const char *group,
> >> const char *mode);
> >> pinctrl_config_pin(const char *pinctrl_device, int pin, const char *mode);
> >>
> >> So the driver need an API to enumerate pin and group modes.
> >>
> >> I might want to save this thing for post-merge of the basic API and
> >> pinmux stuff though so we don't try to push too much upfront
> >> design at once.
> >
> > Yes, adding in a replacement _config API can certainly be a later patch.
> 
> If I'm efficient enough we might get that add-on before the v3.2
> merge window, I'll iterate that patch separately though.
> 
> > One comment on the API above: I think you want "mode" (or "setting"?)
> > /and/ a value parameter; Tegra's pull strength definition for example
> > is:
> >
> > enum tegra_pull_strength {
> > ? ? ? ?TEGRA_PULL_0 = 0,
> > ? ? ? ?TEGRA_PULL_1,
> > // ... (every value in between)
> > ? ? ? ?TEGRA_PULL_30,
> > ? ? ? ?TEGRA_PULL_31,
> > ? ? ? ?TEGRA_MAX_PULL,
> > };
> >
> > And it seems better to represent that as ("pull", 0) ... ("pull", 31) than
> > "pull0" .. "pull31" such that the value needs to be parsed out of the "mode"
> > string somehow by the driver.
> 
> OK so we might want some public defintion of "things you can do"
> with pins, then an opaque parameter.
> 
> like:
> 
> #define PINCTRL_PULL "pull"
> #define PINCTRL_BIAS "bias"
> #define PINCTRL_LOADCAP "load-capacitance"
> #define PINCTRL_DRIVE "drive"

Yes although Tegra has quite a few more weird options than that; I'm not
sure exactly how many of the options that Tegra supports would be useful
for other SoCs, or what the best way to represent all the device-specific
knobs is. I guess we need another survey of a bunch of SoCs like you did
when designing the pinmux API.

> ...but then it's just simple enumerators, and we might be better off
> with a simple enum after all:
> 
> enum pinctrl_pin_ops {
>     PINCTRL_PULL,
>     PINCTRL_BIAS,
>     PINCTRL_LOADCAP,
>     PINCTRL_DRIVE,
> }
> 
> Surely the device tree will have to translate that enum into strings
> (I guess? Sorry for my low DT competence) but that is more of a
> DT pecularity and can be kept in the DT pin control parser?

DT as a data format can handle strings or integers just fine. However,
there's currently no support for named constants (i.e. #defines) in the
device-tree source format, so strings end up being a lot more self-
documenting. Whatever the DT format ends up being, we can certainly
map from whatever-dt-contains to whatever-Linux-APIs-need inside 
whatever kernel function is written to extract the data from DT.

-- 
nvpublic

[PATCH 1/2] drivers: create a pin control subsystem v7

[Stijn Devriendt]

> +static int pin_request(struct pinctrl_dev *pctldev,
> + ? ? ? ? ? ? ? ? ? ? ?int pin, const char *function, bool gpio,
> + ? ? ? ? ? ? ? ? ? ? ?struct pinctrl_gpio_range *gpio_range)
> +{
> + ? ? ? struct pin_desc *desc;
> + ? ? ? const struct pinmux_ops *ops = pctldev->desc->pmxops;
> + ? ? ? int status = -EINVAL;
> +
> + ? ? ? dev_dbg(&pctldev->dev, "request pin %d for %s\n", pin, function);
> +
> + ? ? ? if (!pin_is_valid(pctldev, pin)) {
> + ? ? ? ? ? ? ? dev_err(&pctldev->dev, "pin is invalid\n");
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> +
> + ? ? ? if (!function) {
> + ? ? ? ? ? ? ? dev_err(&pctldev->dev, "no function name given\n");
> + ? ? ? ? ? ? ? return -EINVAL;
> + ? ? ? }
> +
> + ? ? ? desc = pin_desc_get(pctldev, pin);
> + ? ? ? if (desc == NULL) {
> + ? ? ? ? ? ? ? dev_err(&pctldev->dev,
> + ? ? ? ? ? ? ? ? ? ? ? "pin is not registered so it cannot be requested\n");
> + ? ? ? ? ? ? ? goto out;
> + ? ? ? }
> +
> + ? ? ? spin_lock(&desc->lock);
> + ? ? ? if (desc->mux_requested) {
> + ? ? ? ? ? ? ? spin_unlock(&desc->lock);
> + ? ? ? ? ? ? ? dev_err(&pctldev->dev,
> + ? ? ? ? ? ? ? ? ? ? ? "pin already requested\n");
> + ? ? ? ? ? ? ? goto out;
> + ? ? ? }
> + ? ? ? spin_unlock(&desc->lock);

Now this is racing with...

> +
> + ? ? ? /* Let each pin increase references to this module */
> + ? ? ? if (!try_module_get(pctldev->owner)) {
> + ? ? ? ? ? ? ? dev_err(&pctldev->dev,
> + ? ? ? ? ? ? ? ? ? ? ? "could not increase module refcount for pin %d\n",
> + ? ? ? ? ? ? ? ? ? ? ? pin);
> + ? ? ? ? ? ? ? status = -EINVAL;
> + ? ? ? ? ? ? ? goto out;
> + ? ? ? }
> +
> + ? ? ? /*
> + ? ? ? ?* If there is no kind of request function for the pin we just assume
> + ? ? ? ?* we got it by default and proceed.
> + ? ? ? ?*/
> + ? ? ? if (gpio && ops->gpio_request_enable)
> + ? ? ? ? ? ? ? /* This requests and enables a single GPIO pin */
> + ? ? ? ? ? ? ? status = ops->gpio_request_enable(pctldev, gpio_range, pin);
> + ? ? ? else if (ops->request)
> + ? ? ? ? ? ? ? status = ops->request(pctldev, pin);
> + ? ? ? else
> + ? ? ? ? ? ? ? status = 0;
> +
> + ? ? ? if (status) {
> + ? ? ? ? ? ? ? dev_err(&pctldev->dev, "->request on device %s failed "
> + ? ? ? ? ? ? ? ? ? ? ?"for pin %d\n",
> + ? ? ? ? ? ? ? ? ? ? ?pctldev->desc->name, pin);
> + ? ? ? ? ? ? ? goto out;
> + ? ? ? }
> +

... this:

> + ? ? ? spin_lock(&desc->lock);
> + ? ? ? desc->mux_requested = true;
> + ? ? ? strncpy(desc->mux_function, function, sizeof(desc->mux_function));
> + ? ? ? spin_unlock(&desc->lock);
> +

2 threads can pass through when mux_requested is false and they might
both acquire
the pin, depending on specific driver implementation.

Regards,
Stijn

[PATCH 1/2] drivers: create a pin control subsystem v7

[Linus Walleij]

On Tue, Sep 27, 2011 at 11:30 AM, Stijn Devriendt <highguy@gmail.com> wrote:
>> + ? ? ? spin_unlock(&desc->lock);
>
> Now this is racing with...
> (...)
> ... this:
>
>> + ? ? ? spin_lock(&desc->lock);
>> + ? ? ? desc->mux_requested = true;
>> + ? ? ? strncpy(desc->mux_function, function, sizeof(desc->mux_function));
>> + ? ? ? spin_unlock(&desc->lock);

Good catch!

I've fixed it by setting ->mux_requested immediatelty and then
later release it (inside a lock) if something fails.

Yours,
Linus Walleij