* [PATCH 1/2] drivers: create a pin control subsystem v7 [not found] <1316175203-10736-1-git-send-email-linus.walleij@stericsson.com> @ 2011-09-20 21:58 ` Stephen Warren 2011-09-21 9:17 ` Linus Walleij 2011-09-27 9:30 ` Stijn Devriendt 1 sibling, 1 reply; 8+ messages in thread From: Stephen Warren @ 2011-09-20 21:58 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] drivers: create a pin control subsystem v7 2011-09-20 21:58 ` [PATCH 1/2] drivers: create a pin control subsystem v7 Stephen Warren @ 2011-09-21 9:17 ` Linus Walleij 2011-09-21 19:45 ` Stephen Warren 0 siblings, 1 reply; 8+ messages in thread From: Linus Walleij @ 2011-09-21 9:17 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] drivers: create a pin control subsystem v7 2011-09-21 9:17 ` Linus Walleij @ 2011-09-21 19:45 ` Stephen Warren 2011-09-27 7:44 ` Linus Walleij 2011-09-27 7:51 ` Linus Walleij 0 siblings, 2 replies; 8+ messages in thread From: Stephen Warren @ 2011-09-21 19:45 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] drivers: create a pin control subsystem v7 2011-09-21 19:45 ` Stephen Warren @ 2011-09-27 7:44 ` Linus Walleij 2011-09-27 7:51 ` Linus Walleij 1 sibling, 0 replies; 8+ messages in thread From: Linus Walleij @ 2011-09-27 7:44 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] drivers: create a pin control subsystem v7 2011-09-21 19:45 ` Stephen Warren 2011-09-27 7:44 ` Linus Walleij @ 2011-09-27 7:51 ` Linus Walleij 2011-09-28 0:08 ` Stephen Warren 1 sibling, 1 reply; 8+ messages in thread From: Linus Walleij @ 2011-09-27 7:51 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] drivers: create a pin control subsystem v7 2011-09-27 7:51 ` Linus Walleij @ 2011-09-28 0:08 ` Stephen Warren 0 siblings, 0 replies; 8+ messages in thread From: Stephen Warren @ 2011-09-28 0:08 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] drivers: create a pin control subsystem v7 [not found] <1316175203-10736-1-git-send-email-linus.walleij@stericsson.com> 2011-09-20 21:58 ` [PATCH 1/2] drivers: create a pin control subsystem v7 Stephen Warren @ 2011-09-27 9:30 ` Stijn Devriendt 2011-09-28 9:18 ` Linus Walleij 1 sibling, 1 reply; 8+ messages in thread From: Stijn Devriendt @ 2011-09-27 9:30 UTC (permalink / raw) To: linux-arm-kernel > +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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] drivers: create a pin control subsystem v7 2011-09-27 9:30 ` Stijn Devriendt @ 2011-09-28 9:18 ` Linus Walleij 0 siblings, 0 replies; 8+ messages in thread From: Linus Walleij @ 2011-09-28 9:18 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-09-28 9:18 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1316175203-10736-1-git-send-email-linus.walleij@stericsson.com>
2011-09-20 21:58 ` [PATCH 1/2] drivers: create a pin control subsystem v7 Stephen Warren
2011-09-21 9:17 ` Linus Walleij
2011-09-21 19:45 ` Stephen Warren
2011-09-27 7:44 ` Linus Walleij
2011-09-27 7:51 ` Linus Walleij
2011-09-28 0:08 ` Stephen Warren
2011-09-27 9:30 ` Stijn Devriendt
2011-09-28 9:18 ` Linus Walleij
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).