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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.

It looks good otherwise, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux