Re: [PATCH V2 2/2] drivers: qcom: pinctrl: Add pinctrl driver for sm6125

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Sun 23 May 16:18 CDT 2021, Martin Botka wrote:
> diff --git a/drivers/pinctrl/qcom/pinctrl-sm6125.c b/drivers/pinctrl/qcom/pinctrl-sm6125.c
[..]
> +static const struct msm_pingroup sm6125_groups[] = {
> +	[0] = PINGROUP(0, WEST, qup00, _, qdss_gpio6, _, _, _, _, _, _),

There's no individual pin where we need to distinguish between two
different qdss_gpio* functions, so please lump all of qdss_gpio*
together as "qdss".

> +	[1] = PINGROUP(1, WEST, qup00, _, qdss_gpio7, _, _, _, _, _, _),
> +	[2] = PINGROUP(2, WEST, qup00, _, qdss_gpio8, _, _, _, _, _, _),
> +	[3] = PINGROUP(3, WEST, qup00, _, qdss_gpio9, _, _, _, _, _, _),
> +	[4] = PINGROUP(4, WEST, qup01, _, _, _, _, _, _, _, _),
> +	[5] = PINGROUP(5, WEST, qup01, _, _, _, _, _, _, _, _),
> +	[6] = PINGROUP(6, WEST, qup02, ddr_pxi0, _, _, _, _, _, _, _),
> +	[7] = PINGROUP(7, WEST, qup02, ddr_bist, atest_tsens2, vsense_trigger,
> +		       atest_usb1, ddr_pxi0, _, _, _),

Please ignore the line length limit and leave all these unwrapped.

> +	[8] = PINGROUP(8, WEST, qup02, gp_pdm1, ddr_bist, _, phase_flag23, _,

As with qdss_gpioX, please join all phase_flagNN as "phase_flag".

> +		       _, _, _),
[..]
> +	[131] = PINGROUP(131, SOUTH, phase_flag20, _, _, _, _, _, _, _,
> +			 _),
> +	[132] = PINGROUP(132, SOUTH, _, _, _, _, _, _, _, _, _),
> +	[133] = SDC_QDSD_PINGROUP(sdc1_rclk, WEST, 0x18d000, 15, 0),
> +	[134] = SDC_QDSD_PINGROUP(sdc1_clk, WEST, 0x18d000, 13, 6),
> +	[135] = SDC_QDSD_PINGROUP(sdc1_cmd, WEST, 0x18d000, 11, 3),
> +	[136] = SDC_QDSD_PINGROUP(sdc1_data, WEST, 0x18d000, 9, 0),
> +	[137] = SDC_QDSD_PINGROUP(sdc2_clk, SOUTH, 0x58b000, 14, 6),
> +	[138] = SDC_QDSD_PINGROUP(sdc2_cmd, SOUTH, 0x58b000, 11, 3),
> +	[139] = SDC_QDSD_PINGROUP(sdc2_data, SOUTH, 0x58b000, 9, 0),
> +	[140] = UFS_RESET(ufs_reset, 0x190000),

Move this above the SDC nodes, to make it the item with index 133.
Together with ngpios = 134 this can then be accessed using the gpio
framework in the UFS driver.

> +};
> +
> +static const struct msm_pinctrl_soc_data sm6125_pinctrl = {
> +	.pins = sm6125_pins,
> +	.npins = ARRAY_SIZE(sm6125_pins),
> +	.functions = sm6125_functions,
> +	.nfunctions = ARRAY_SIZE(sm6125_functions),
> +	.groups = sm6125_groups,
> +	.ngroups = ARRAY_SIZE(sm6125_groups),
> +	.ngpios = 134,
> +	.tiles = sm6125_tiles,
> +	.ntiles = ARRAY_SIZE(sm6125_tiles),
> +};
> +
> +static int sm6125_pinctrl_probe(struct platform_device *pdev)
> +{
> +	return msm_pinctrl_probe(pdev, &sm6125_pinctrl);
> +}
> +
> +static const struct of_device_id sm6125_pinctrl_of_match[] = {
> +	{ .compatible = "qcom,sm6125-pinctrl", },

Please change "pinctrl" to "tlmm".

Regards,
Bjorn