| Message ID | 10321666.EILxzFPKZf@wasted.cogentembedded.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Sergei, Thank you for the patch. On Thursday 04 June 2015 16:25:01 Sergei Shtylyov wrote: > Calling sh_pfc_get_pin_index() to calculate a pin index based on the > collected pin range data is unnecessary when we're dealing with > 'pfc->info->pins' and 'chip->pins' arrays as those always reperesent the > pins starting from index 0 sequentially. Being a mere optimization at > this time, this change will become crucial when we'll allow the "holes" in > those arrays... Pin information is stored in per-SoC pins arrays (a.k.a. pfc->info->pins). The driver support two models to number pins: - The sequential model, in which pins are numbered sequentially starting at 0. Pin numbers are equal to the index in the array. - The explicit numbering model, in which each pin entry has an explicit number (stored in struct sh_pfc_pin.pin). Pins numbers are not necessarily equal to the index of the pin entry in the array. The sh_pfc_get_pin_index() function converts a pin number to the pin index in the pins array. Let's consider the sh_pfc_pinconf_validate() from which your patch removes the call to sh_pfc_get_pin_index() and uses the pin number directly. The function is called from the .pin_config_get() and .pin_config_set() handlers. One possible call path is pinconf_pins_show() -> pinconf_dump_pin() -> pinconf_dump_pins() -> pinconf_generic_dump_pins() -> pinconf_generic_dump_one() -> pin_config_get_for_pin() -> .pin_config_get() The pin value passed to the .pin_config_get() function is pctldev->desc- >pins[i].number, which is the pin number, not its index. It thus looks like this patch introduces a bug. There might be something obvious I'm not getting though, so please feel free to prove me wrong. > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > Changes in version 2: > - new patch. > > drivers/pinctrl/sh-pfc/gpio.c | 6 ++---- > drivers/pinctrl/sh-pfc/pinctrl.c | 7 +++---- > 2 files changed, 5 insertions(+), 8 deletions(-) > > Index: linux-pinctrl/drivers/pinctrl/sh-pfc/gpio.c > =================================================================== > --- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/gpio.c > +++ linux-pinctrl/drivers/pinctrl/sh-pfc/gpio.c > @@ -52,8 +52,7 @@ static void gpio_get_data_reg(struct sh_ > struct sh_pfc_gpio_data_reg **reg, > unsigned int *bit) > { > - int idx = sh_pfc_get_pin_index(chip->pfc, offset); > - struct sh_pfc_gpio_pin *gpio_pin = &chip->pins[idx]; > + struct sh_pfc_gpio_pin *gpio_pin = &chip->pins[offset]; > > *reg = &chip->regs[gpio_pin->dreg]; > *bit = gpio_pin->dbit; > @@ -138,9 +137,8 @@ static int gpio_setup_data_regs(struct s > static int gpio_pin_request(struct gpio_chip *gc, unsigned offset) > { > struct sh_pfc *pfc = gpio_to_pfc(gc); > - int idx = sh_pfc_get_pin_index(pfc, offset); > > - if (idx < 0 || pfc->info->pins[idx].enum_id == 0) > + if (pfc->info->pins[offset].enum_id == 0) > return -EINVAL; > > return pinctrl_request_gpio(offset); > Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c > =================================================================== > --- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pinctrl.c > +++ linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c > @@ -370,7 +370,7 @@ static int sh_pfc_gpio_request_enable(st > /* If GPIOs are handled externally the pin mux type need to be > * set to GPIO here. > */ > - const struct sh_pfc_pin *pin = &pfc->info->pins[idx]; > + const struct sh_pfc_pin *pin = &pfc->info->pins[offset]; > > ret = sh_pfc_config_mux(pfc, pin->enum_id, PINMUX_TYPE_GPIO); > if (ret < 0) > @@ -410,7 +410,7 @@ static int sh_pfc_gpio_set_direction(str > struct sh_pfc *pfc = pmx->pfc; > int new_type = input ? PINMUX_TYPE_INPUT : PINMUX_TYPE_OUTPUT; > int idx = sh_pfc_get_pin_index(pfc, offset); > - const struct sh_pfc_pin *pin = &pfc->info->pins[idx]; > + const struct sh_pfc_pin *pin = &pfc->info->pins[offset]; > struct sh_pfc_pin_config *cfg = &pmx->configs[idx]; > unsigned long flags; > unsigned int dir; > @@ -452,8 +452,7 @@ static const struct pinmux_ops sh_pfc_pi > static bool sh_pfc_pinconf_validate(struct sh_pfc *pfc, unsigned int _pin, > enum pin_config_param param) > { > - int idx = sh_pfc_get_pin_index(pfc, _pin); > - const struct sh_pfc_pin *pin = &pfc->info->pins[idx]; > + const struct sh_pfc_pin *pin = &pfc->info->pins[_pin]; > > switch (param) { > case PIN_CONFIG_BIAS_DISABLE:
Hello. On 06/18/2015 10:05 PM, Laurent Pinchart wrote: >> Calling sh_pfc_get_pin_index() to calculate a pin index based on the >> collected pin range data is unnecessary when we're dealing with >> 'pfc->info->pins' and 'chip->pins' arrays as those always reperesent the >> pins starting from index 0 sequentially. Being a mere optimization at >> this time, this change will become crucial when we'll allow the "holes" in >> those arrays... > Pin information is stored in per-SoC pins arrays (a.k.a. pfc->info->pins). The > driver support two models to number pins: > - The sequential model, in which pins are numbered sequentially starting at 0. > Pin numbers are equal to the index in the array. And I didn't touch this case. > - The explicit numbering model, in which each pin entry has an explicit number > (stored in struct sh_pfc_pin.pin). Pins numbers are not necessarily equal to > the index of the pin entry in the array. Ah... I was just looking at _GP_GPIO() which still assigns sequential pin #'s equal to the indices. > The sh_pfc_get_pin_index() function converts a pin number to the pin index in > the pins array. > Let's consider the sh_pfc_pinconf_validate() from which your patch removes the > call to sh_pfc_get_pin_index() and uses the pin number directly. The function > is called from the .pin_config_get() and .pin_config_set() handlers. One > possible call path is > pinconf_pins_show() -> pinconf_dump_pin() -> pinconf_dump_pins() -> > pinconf_generic_dump_pins() -> pinconf_generic_dump_one() -> > pin_config_get_for_pin() -> .pin_config_get() > The pin value passed to the .pin_config_get() function is pctldev->desc-> > pins[i].number, which is the pin number, not its index. It thus looks like > this patch introduces a bug. > There might be something obvious I'm not getting though, so please feel free > to prove me wrong. The bug seems more like theoretical one at this point (unless you have the examples with non-sequential pin #'s)... WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in
Hello. On 06/23/2015 01:42 AM, Sergei Shtylyov wrote: >>> Calling sh_pfc_get_pin_index() to calculate a pin index based on the >>> collected pin range data is unnecessary when we're dealing with >>> 'pfc->info->pins' and 'chip->pins' arrays as those always reperesent the >>> pins starting from index 0 sequentially. Being a mere optimization at >>> this time, this change will become crucial when we'll allow the "holes" in >>> those arrays... >> Pin information is stored in per-SoC pins arrays (a.k.a. pfc->info->pins). The >> driver support two models to number pins: >> - The sequential model, in which pins are numbered sequentially starting at 0. >> Pin numbers are equal to the index in the array. > And I didn't touch this case. >> - The explicit numbering model, in which each pin entry has an explicit number >> (stored in struct sh_pfc_pin.pin). Pins numbers are not necessarily equal to >> the index of the pin entry in the array. > Ah... I was just looking at _GP_GPIO() which still assigns sequential pin > #'s equal to the indices. And forgot about SH_PFC_PIN_NAMED()... >> The sh_pfc_get_pin_index() function converts a pin number to the pin index in >> the pins array. >> Let's consider the sh_pfc_pinconf_validate() from which your patch removes the >> call to sh_pfc_get_pin_index() and uses the pin number directly. The function >> is called from the .pin_config_get() and .pin_config_set() handlers. One >> possible call path is >> pinconf_pins_show() -> pinconf_dump_pin() -> pinconf_dump_pins() -> >> pinconf_generic_dump_pins() -> pinconf_generic_dump_one() -> >> pin_config_get_for_pin() -> .pin_config_get() >> The pin value passed to the .pin_config_get() function is pctldev->desc-> >> pins[i].number, which is the pin number, not its index. It thus looks like >> this patch introduces a bug. >> There might be something obvious I'm not getting though, so please feel free >> to prove me wrong. > The bug seems more like theoretical one at this point (unless you have the > examples with non-sequential pin #'s)... No, with non-GPIO pins it seems to be an actual bug. What if we remove the explicit array index from _GP_GPIO() and so don't have the holes at all? WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: linux-pinctrl/drivers/pinctrl/sh-pfc/gpio.c =================================================================== --- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/gpio.c +++ linux-pinctrl/drivers/pinctrl/sh-pfc/gpio.c @@ -52,8 +52,7 @@ static void gpio_get_data_reg(struct sh_ struct sh_pfc_gpio_data_reg **reg, unsigned int *bit) { - int idx = sh_pfc_get_pin_index(chip->pfc, offset); - struct sh_pfc_gpio_pin *gpio_pin = &chip->pins[idx]; + struct sh_pfc_gpio_pin *gpio_pin = &chip->pins[offset]; *reg = &chip->regs[gpio_pin->dreg]; *bit = gpio_pin->dbit; @@ -138,9 +137,8 @@ static int gpio_setup_data_regs(struct s static int gpio_pin_request(struct gpio_chip *gc, unsigned offset) { struct sh_pfc *pfc = gpio_to_pfc(gc); - int idx = sh_pfc_get_pin_index(pfc, offset); - if (idx < 0 || pfc->info->pins[idx].enum_id == 0) + if (pfc->info->pins[offset].enum_id == 0) return -EINVAL; return pinctrl_request_gpio(offset); Index: linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c =================================================================== --- linux-pinctrl.orig/drivers/pinctrl/sh-pfc/pinctrl.c +++ linux-pinctrl/drivers/pinctrl/sh-pfc/pinctrl.c @@ -370,7 +370,7 @@ static int sh_pfc_gpio_request_enable(st /* If GPIOs are handled externally the pin mux type need to be * set to GPIO here. */ - const struct sh_pfc_pin *pin = &pfc->info->pins[idx]; + const struct sh_pfc_pin *pin = &pfc->info->pins[offset]; ret = sh_pfc_config_mux(pfc, pin->enum_id, PINMUX_TYPE_GPIO); if (ret < 0) @@ -410,7 +410,7 @@ static int sh_pfc_gpio_set_direction(str struct sh_pfc *pfc = pmx->pfc; int new_type = input ? PINMUX_TYPE_INPUT : PINMUX_TYPE_OUTPUT; int idx = sh_pfc_get_pin_index(pfc, offset); - const struct sh_pfc_pin *pin = &pfc->info->pins[idx]; + const struct sh_pfc_pin *pin = &pfc->info->pins[offset]; struct sh_pfc_pin_config *cfg = &pmx->configs[idx]; unsigned long flags; unsigned int dir; @@ -452,8 +452,7 @@ static const struct pinmux_ops sh_pfc_pi static bool sh_pfc_pinconf_validate(struct sh_pfc *pfc, unsigned int _pin, enum pin_config_param param) { - int idx = sh_pfc_get_pin_index(pfc, _pin); - const struct sh_pfc_pin *pin = &pfc->info->pins[idx]; + const struct sh_pfc_pin *pin = &pfc->info->pins[_pin]; switch (param) { case PIN_CONFIG_BIAS_DISABLE:
Calling sh_pfc_get_pin_index() to calculate a pin index based on the collected pin range data is unnecessary when we're dealing with 'pfc->info->pins' and 'chip->pins' arrays as those always reperesent the pins starting from index 0 sequentially. Being a mere optimization at this time, this change will become crucial when we'll allow the "holes" in those arrays... Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- Changes in version 2: - new patch. drivers/pinctrl/sh-pfc/gpio.c | 6 ++---- drivers/pinctrl/sh-pfc/pinctrl.c | 7 +++---- 2 files changed, 5 insertions(+), 8 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html