Re: [PATCH v3 6/6] pinctrl: amd: Implement pinmux functionality

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Tue, May 31, 2022 at 02:13:22PM +0530, Basavaraj Natikar wrote:
> Provide pinmux functionality by implementing pinmux_ops.

...

> +static int amd_set_mux(struct pinctrl_dev *pctrldev, unsigned int function, unsigned int group)
> +{
> +	struct amd_gpio *gpio_dev = pinctrl_dev_get_drvdata(pctrldev);

Consider to use

	struct device *dev = ...;

and make code few lines shorter.

> +	struct pin_desc *pd;
> +	int ind, index;
> +
> +	if (!gpio_dev->iomux_base)
> +		return -EINVAL;
> +
> +	for (index = 0; index < NSELECTS; index++) {
> +		if (strcmp(gpio_dev->groups[group].name,  pmx_functions[function].groups[index]))
> +			continue;
> +
> +		if (readb(gpio_dev->iomux_base + pmx_functions[function].index) ==
> +				FUNCTION_INVALID) {
> +			dev_warn(&gpio_dev->pdev->dev,
> +				 "IOMUX_GPIO 0x%x not present or supported\n",
> +				 pmx_functions[function].index);
> +			return -EINVAL;
> +		}
> +
> +		writeb(index, gpio_dev->iomux_base + pmx_functions[function].index);
> +
> +		if (index != (readb(gpio_dev->iomux_base + pmx_functions[function].index) &
> +					FUNCTION_MASK)) {
> +			dev_warn(&gpio_dev->pdev->dev,
> +				 "IOMUX_GPIO 0x%x not present or supported\n",
> +				 pmx_functions[function].index);
> +			return -EINVAL;
> +		}
> +
> +		for (ind = 0; ind < gpio_dev->groups[group].npins; ind++) {
> +			if (strncmp(gpio_dev->groups[group].name, "IMX_F", strlen("IMX_F")))
> +				continue;
> +
> +			pd = pin_desc_get(gpio_dev->pctrl, gpio_dev->groups[group].pins[ind]);
> +			pd->mux_owner = gpio_dev->groups[group].name;
> +		}
> +		break;
> +	}
> +
> +	return 0;
> +}

...

> +#define AMD_PINCTRL_FUNC_GRP(_number, _func)						\
> +	[IMX_F##_func##_GPIO##_number] = PINCTRL_GRP("IMX_F"#_func "_GPIO"#_number,	\
> +						     AMD_PINS(_number), 1)

Slightly better:

#define AMD_PINCTRL_FUNC_GRP(_number, _func)					\
	[IMX_F##_func##_GPIO##_number] =					\
		PINCTRL_GRP("IMX_F"#_func "_GPIO"#_number, AMD_PINS(_number), 1)

...

> +#define AMD_PMUX_FUNC(_number) {						\
> +	.name = "iomux_gpio_"#_number,						\
> +	.groups = { "IMX_F0_GPIO"#_number, "IMX_F1_GPIO"#_number,		\
> +		    "IMX_F2_GPIO"#_number, "IMX_F3_GPIO"#_number },		\
> +	.index = _number,							\
> +	.ngroups = NSELECTS,							\
> +}

Slightly better (indentation, comma):

#define AMD_PMUX_FUNC(_number) {				\
	.name = "iomux_gpio_"#_number,				\
	.groups = {						\
		"IMX_F0_GPIO"#_number, "IMX_F1_GPIO"#_number,	\
		"IMX_F2_GPIO"#_number, "IMX_F3_GPIO"#_number,	\
	},							\
	.index = _number,					\
	.ngroups = NSELECTS,					\
}

-- 
With Best Regards,
Andy Shevchenko