Messages in this thread Patch in this message | | | From | Stephen Warren <> | Date | Fri, 28 Oct 2011 09:13:28 -0700 | Subject | RE: [PATCHv2] drivers: pinctrl: add a pin_base for sparse gpio-ranges |
| |
Chanho Park wrote at Thursday, October 27, 2011 11:42 PM: > This patch enables mapping a base offset of gpio ranges with > a pin offset even if does'nt matched. A "base" of pinctrl_gpio_range > means a start offset of gpio. However, we cannot convert gpio to pin > number for sparse gpio ranges just only using a gpio base offset. > We can convert a gpio to real pin number using a new pin_base > (which means a base pin offset of requested gpio range). > If the pin_base is not specified explicitly, the controller sybsystem > makes to equal with gpio's base offset. Now, a pinctrl subsystem passes > the pin number to the driver instead a offset.
... > diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt ... > -For example: if a user issues pinctrl_gpio_set_foo(50), the pin control > -subsystem will find that the second range on this pin controller matches, > -subtract the base 48 and call the > -pinctrl_driver_gpio_set_foo(pinctrl, range, 2) where the latter function has > -this signature: > - > -int pinctrl_driver_gpio_set_foo(struct pinctrl_dev *pctldev, > - struct pinctrl_gpio_range *rangeid, > - unsigned offset); > - > -Now the driver knows that we want to do some GPIO-specific operation on the > -second GPIO range handled by "chip b", at offset 2 in that specific range. > - > -(If the GPIO subsystem is ever refactored to use a local per-GPIO controller > -pin space, this mapping will need to be augmented accordingly.) > - > -
struct pinmux_ops.gpio_request_enable's documentation states that:
* @gpio_request_enable: requests and enables GPIO on a certain pin. * Implement this only if you can mux every pin individually as GPIO. The * affected GPIO range is passed along with an offset into that * specific GPIO range - function selectors and pin groups are orthogonal * to this, the core will however make sure the pins do not collide
You'll need to update that documentation now that the final parameter is a pin number, not an offset into the range.
That said, I wonder why there's a need to change this function's parameters; you could modify pin_request() to perform the calculation, and then not need to change any of the drivers:
- if (gpio_range && ops->gpio_request_enable) - /* This requests and enables a single GPIO pin */ - status = ops->gpio_request_enable(pctldev, gpio_range, pin); + if (gpio_range && ops->gpio_request_enable) + /* This requests and enables a single GPIO pin */ + status = ops->gpio_request_enable(pctldev, gpio_range, + pin - range->pin_base);
... > diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c > index a13354e..4d9fc44 100644 > --- a/drivers/pinctrl/core.c > +++ b/drivers/pinctrl/core.c > @@ -261,11 +261,15 @@ int pinctrl_get_device_gpio_range(unsigned gpio, > * > * This adds a range of GPIOs to be handled by a certain pin controller. Call > * this to register handled ranges after registering your pin controller. > + * If a pin_base offset is not specified explicitly, > + * it is equal to a gpio base offset. > */ > void pinctrl_add_gpio_range(struct pinctrl_dev *pctldev, > struct pinctrl_gpio_range *range) > { > mutex_lock(&pctldev->gpio_ranges_lock); > + if (!range->pin_base) > + range->pin_base = range->base;
This doesn't seem right; what if we really want a range with a pin_base of zero, yet a non-zero "base". I think we'd be better off just requiring all GPIO ranges to specify both values.
-- nvpublic
| |