Hi, On 09/12/2013 09:24, Nicolas Ferre wrote: > On 07/12/2013 14:08, Alexandre Belloni : >> When passing a not initialized config parameter, at91_pinconf_get() >> would return >> a bogus value. Fix that by initializing it to zero before using it. >> >> Signed-off-by: Alexandre Belloni <alexandre.bell...@free-electrons.com> >> --- >> drivers/pinctrl/pinctrl-at91.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pinctrl/pinctrl-at91.c >> b/drivers/pinctrl/pinctrl-at91.c >> index 6446dc804aa7..b0b78f3468ae 100644 >> --- a/drivers/pinctrl/pinctrl-at91.c >> +++ b/drivers/pinctrl/pinctrl-at91.c >> @@ -722,7 +722,8 @@ static int at91_pinconf_get(struct pinctrl_dev >> *pctldev, >> unsigned pin; >> int div; >> >> - dev_dbg(info->dev, "%s:%d, pin_id=%d, config=0x%lx", __func__, >> __LINE__, pin_id, *config); >> + *config = 0; >> + dev_dbg(info->dev, "%s:%d, pin_id=%d", __func__, __LINE__, pin_id); >> pio = pin_to_controller(info, pin_to_bank(pin_id)); >> pin = pin_id % MAX_NB_GPIO_PER_BANK; > > Beyond this patch, I must say that I am puzzled by this function. > > What I read from the prototype documentation and what I see in > different implementations is different... > > Linus, can we have a review of this function because it seems not in > line with what is used for u300 (but on the other hand looks like the > what is returned by pinctrl-exynos5440.c driver for example). > > What would be the consequences if we change this function's behavior: > I mean use of -EINVAL for pin configuration "available but disabled" > as said in include/linux/pinctrl/pinconf.h? >
>From what I understand, it doesn't really matter because that function is never used. I guess the best implementation is the tegra one ;) It is only called in one specific case: - you have ops->is_generic == true (that is only true for a few implmentations) - and you are dumping the pinconf state using debugfs I'm actually wondering if the checks for the ops->pin_config_get are not a bit overkill. We check for that function in: - pinconf_check_ops() - before calling it in pin_config_get_for_pin() which is only used once, in the same path : dump using debugfs and having ops->is_generic == true - in pinconf_pins_show() which is the function calling also in the same path What I would do is: - remove all the checks in pinconf_check_ops() and pinconf_pins_show() so that people are not pressured to implement a function that is simply never used. - modify pin_config_get_for_pin() by removing the dev_err() call and returning -ENOTSUPP instead of -EINVAL (it doesn't change the behaviour but I feel -ENOTSUPP is more appropriate) I have a patch ready but I can't test it as I don't own any of the is_generic platforms. -- Alexandre Belloni, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/