On Thu, Jun 20, 2013 at 10:18 AM, Fabio Baltieri
<fabio.balti...@linaro.org> wrote:
> On Thu, Jun 20, 2013 at 09:23:22AM +0200, patrice.chotard...@gmail.com wrote:
>> From: Patrice Chotard <patrice.chot...@st.com>
>>
>> _ Update abx500_pin_config_set() in order to take in
>> account PIN_CONFIG_BIAS_DISABLE state to disable
>> pull up or pull down.
>>
>> _ Rework error path.
>>
>> Signed-off-by: Patrice Chotard <patrice.chot...@st.com>
>> ---
>> drivers/pinctrl/pinctrl-abx500.c | 31 +++++++++++++++++++++++++------
>> 1 file changed, 25 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pinctrl/pinctrl-abx500.c
>> b/drivers/pinctrl/pinctrl-abx500.c
>> index b5b5460..14dc078 100644
>> --- a/drivers/pinctrl/pinctrl-abx500.c
>> +++ b/drivers/pinctrl/pinctrl-abx500.c
>> @@ -33,6 +33,7 @@
>> #include <linux/pinctrl/machine.h>
>>
>> #include "pinctrl-abx500.h"
>> +#include "core.h"
>> #include "pinconf.h"
>>
>> /*
>> @@ -963,7 +964,7 @@ static int abx500_pin_config_set(struct pinctrl_dev
>> *pctldev,
>> struct pullud *pullud = pct->soc->pullud;
>> struct gpio_chip *chip = &pct->chip;
>> unsigned offset;
>> - int ret = 0;
>> + int ret = -EINVAL;
>> enum pin_config_param param = pinconf_to_config_param(config);
>> enum pin_config_param argument = pinconf_to_config_argument(config);
>>
>> @@ -976,13 +977,32 @@ static int abx500_pin_config_set(struct pinctrl_dev
>> *pctldev,
>> offset = pin - 1;
>>
>> switch (param) {
>> - case PIN_CONFIG_BIAS_PULL_DOWN:
>> + case PIN_CONFIG_BIAS_DISABLE:
>> + ret = abx500_gpio_direction_input(chip, offset);
>> /*
>> - * if argument = 1 set the pull down
>> - * else clear the pull down
>> + * Some chips only support pull down, while some actually
>> + * support both pull up and pull down. Such chips have
>> + * a "pullud" range specified for the pins that support
>> + * both features. If the pin is not within that range, we
>> + * fall back to the old bit set that only support pull down.
>> */
>> + if (pullud &&
>> + pin >= pullud->first_pin &&
>> + pin <= pullud->last_pin)
>
> This multi-line check is replicated in all conditions, would it make
> sense to move it on a dedicated function to improve readability?
Yes i will add a dedicated function.
>
>> + ret = abx500_set_pull_updown(pct,
>> + pin,
>> + ABX500_GPIO_PULL_NONE);
>> + else
>> + /* Chip only supports pull down */
>> + ret = abx500_gpio_set_bits(chip, AB8500_GPIO_PUD1_REG,
>> + offset, ABX500_GPIO_PULL_NONE);
>> + break;
>> +
>> + case PIN_CONFIG_BIAS_PULL_DOWN:
>> ret = abx500_gpio_direction_input(chip, offset);
>> /*
>> + * if argument = 1 set the pull down
>> + * else clear the pull down
>> * Some chips only support pull down, while some actually
>> * support both pull up and pull down. Such chips have
>> * a "pullud" range specified for the pins that support
>> @@ -1002,6 +1022,7 @@ static int abx500_pin_config_set(struct pinctrl_dev
>> *pctldev,
>> break;
>>
>> case PIN_CONFIG_BIAS_PULL_UP:
>> + ret = abx500_gpio_direction_input(chip, offset);
>
> Here the return value of abx500_gpio_direction_input is set but never
> checked, and will be always overwritten by the next abx500_gpio_ call...
> Would it make sense to add a pr_err for it? On the other side, if it
> never fails, you can just drop the return field altogether.
I will rework this part and make a global review as i notice that same issue
appears on several other place in this file.
>
> That's also done in other conditions in the same 'switch', it may make
> sense to have a patch just for that.
>
> Thanks,
> Fabio
>
>> /*
>> * if argument = 1 set the pull up
>> * else clear the pull up
>> @@ -1030,8 +1051,6 @@ static int abx500_pin_config_set(struct pinctrl_dev
>> *pctldev,
>>
>> default:
>> dev_err(chip->dev, "illegal configuration requested\n");
>> -
>> - return -EINVAL;
>> }
>>
>> return ret;
>> --
>> 1.7.10
>>
>
> --
> Fabio Baltieri
--
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/
