Re: [PATCH v4 02/10] pinctrl: axp209: add pinctrl features

[Quentin Schulz]

Hi Maxime,

On 01/12/2017 16:57, Maxime Ripard wrote:
> On Fri, Dec 01, 2017 at 02:44:43PM +0100, Quentin Schulz wrote:
>> +static void axp20x_gpio_set(struct gpio_chip *chip, unsigned offset,
>> +			    int value)
>> +{
> 
> checkpatch output:
> WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
> 
>> +static int axp20x_pmx_set_mux(struct pinctrl_dev *pctldev,
>> +			      unsigned int function, unsigned int group)
>> +{
>> +	struct axp20x_gpio *gpio = pinctrl_dev_get_drvdata(pctldev);
>> +	unsigned int mask;
>> +
>> +	/* Every pin supports GPIO_OUT and GPIO_IN functions */
>> +	if (function <= AXP20X_FUNC_GPIO_IN)
>> +		return axp20x_pmx_set(pctldev, group,
>> +				      gpio->funcs[function].muxval);
>> +
>> +	if (function == AXP20X_FUNC_LDO)
>> +		mask = gpio->desc->ldo_mask;
>> +	else
>> +		mask = gpio->desc->adc_mask;
> 
> What is the point of this test...
> 
>> +	if (!(BIT(group) & mask))
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * We let the regulator framework handle the LDO muxing as muxing bits
>> +	 * are basically also regulators on/off bits. It's better not to enforce
>> +	 * any state of the regulator when selecting LDO mux so that we don't
>> +	 * interfere with the regulator driver.
>> +	 */
>> +	if (function == AXP20X_FUNC_LDO)
>> +		return 0;
> 
> ... if you know that you're not going to do anything with one of the
> outcomes. It would be better to just move that part above, instead of
> doing the same test twice.
> 

Return value is different. In one case, it is an error to request "ldo"
for a pin that does not support it. In the other case, the ldo request
is valid but nothing's done on driver side.

Both cases are handled differently by the core:
http://elixir.free-electrons.com/linux/latest/source/drivers/pinctrl/pinmux.c#L439

I think that's the behavior we're expecting from this driver.

Or maybe you're asking to do:

+	if (function == AXP20X_FUNC_LDO) {
+		if (!(BIT(group) & gpio->desc->ldo_mask))
+			return -EINVAL;
+		return 0;
+	} else if (!(BIT(group) & gpio->desc->adc_mask)) {
+		return -EINVAL;
+	}

?

Thanks,
Quentin
-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachment: signature.asc
Description: OpenPGP digital signature