Messages in this thread | | | Date | Mon, 09 Dec 2013 10:55:26 +0100 | From | Alexandre Belloni <> | Subject | Re: [PATCH 2/3] pinctrl: at91: initialize config parameter to 0 |
| |
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.belloni@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
| |