Re: [PATCH 2/2] pinctrl: qcom: Add QDU1000/QRU1000 pinctrl driver

[Bjorn Andersson]

On Fri, Sep 30, 2022 at 08:05:46PM -0700, Melody Olvera wrote:
[..]
> diff --git a/drivers/pinctrl/qcom/pinctrl-qdru1000.c b/drivers/pinctrl/qcom/pinctrl-qdru1000.c
> new file mode 100644
> index 000000000000..8b931ff80bb4
> --- /dev/null
> +++ b/drivers/pinctrl/qcom/pinctrl-qdru1000.c
> @@ -0,0 +1,59 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pinctrl/pinctrl.h>
> +
> +#include "pinctrl-msm.h"
> +#include "pinctrl-qdru1000.h"
> +
> +static const struct msm_pinctrl_soc_data qdru1000_tlmm = {
> +	.pins = qdru1000_pins,
> +	.npins = ARRAY_SIZE(qdru1000_pins),
> +	.functions = qdru1000_functions,
> +	.nfunctions = ARRAY_SIZE(qdru1000_functions),
> +	.groups = qdru1000_groups,
> +	.ngroups = ARRAY_SIZE(qdru1000_groups),
> +	.ngpios = 151,
> +};
> +
> +static int qdru1000_tlmm_probe(struct platform_device *pdev)
> +{
> +	return msm_pinctrl_probe(pdev, &qdru1000_tlmm);
> +}
> +
> +static const struct of_device_id qdru1000_tlmm_of_match[] = {
> +	{ .compatible = "qcom,qdu1000-tlmm", },
> +	{ .compatible = "qcom,qru1000-tlmm", },
> +	{ },
> +};
> +
> +static struct platform_driver qdru1000_tlmm_driver = {
> +	.driver = {
> +		.name = "qdru1000-tlmm",
> +		.of_match_table = qdru1000_tlmm_of_match,
> +	},
> +	.probe = qdru1000_tlmm_probe,
> +	.remove = msm_pinctrl_remove,
> +};
> +
> +static int __init qdru1000_tlmm_init(void)
> +{
> +	return platform_driver_register(&qdru1000_tlmm_driver);
> +}
> +arch_initcall(qdru1000_tlmm_init);
> +
> +static void __exit qdru1000_tlmm_exit(void)
> +{
> +	platform_driver_unregister(&qdru1000_tlmm_driver);
> +}
> +module_exit(qdru1000_tlmm_exit);
> +
> +MODULE_DESCRIPTION("QTI QDRU1000 TLMM driver");
> +MODULE_LICENSE("GPL v2");

"GPL" only please.

> +MODULE_DEVICE_TABLE(of, qdru1000_tlmm_of_match);

Please add this next to qdru1000_tlmm_of_match.

> diff --git a/drivers/pinctrl/qcom/pinctrl-qdru1000.h b/drivers/pinctrl/qcom/pinctrl-qdru1000.h

I'm not able to see why this is in a header file and the commit message
doesn't give a clue. Please align with the customary form, or motivate
your choice.

[..]
> +
> +enum qdru1000_functions {
> +	msm_mux_gpio,
> +	msm_mux_CMO_PRI,
> +	msm_mux_SI5518_INT,
> +	msm_mux_atest_char_start,
> +	msm_mux_atest_char_status0,
> +	msm_mux_atest_char_status1,
> +	msm_mux_atest_char_status2,
> +	msm_mux_atest_char_status3,

For anything that denotes different pins in one function, please drop
the suffix and make this a list of functions.

[..]
> +	msm_mux_qspi_data_0,
> +	msm_mux_qspi_data_1,
> +	msm_mux_qspi_data_2,
> +	msm_mux_qspi_data_3,
> +	msm_mux_qup0_se0_l0,

E.g. msm_mux_qup0_se0 is enough, giving each pin its own function means
that we need to define each pin separate in DT.

Regards,
Bjorn