Hi, On Mon, Dec 04, 2017 at 09:07:52AM +0100, Quentin Schulz wrote: > >> +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. Ah, right. > 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; > + } > > ? No, it's definitely better the way you did it. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Attachment:
signature.asc
Description: PGP signature