[v3,2/2] pinctrl: qcom: sc8180x: add ACPI probe support

It adds ACPI probe support for pinctrl-sc8180x driver.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/pinctrl/qcom/Kconfig           |  2 +-
 drivers/pinctrl/qcom/pinctrl-sc8180x.c | 39 ++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 3 deletions(-)

On Thu, Mar 04, 2021 at 02:05:20PM +0800, Shawn Guo wrote:
> It adds ACPI probe support for pinctrl-sc8180x driver.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/pinctrl/qcom/Kconfig           |  2 +-
>  drivers/pinctrl/qcom/pinctrl-sc8180x.c | 39 ++++++++++++++++++++++++--
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
> index 6853a896c476..9f0218c4f9b3 100644
> --- a/drivers/pinctrl/qcom/Kconfig
> +++ b/drivers/pinctrl/qcom/Kconfig
> @@ -222,7 +222,7 @@ config PINCTRL_SC7280
>  
>  config PINCTRL_SC8180X
>  	tristate "Qualcomm Technologies Inc SC8180x pin controller driver"
> -	depends on GPIOLIB && OF
> +	depends on GPIOLIB && (OF || ACPI)
>  	select PINCTRL_MSM
>  	help
>  	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the
> diff --git a/drivers/pinctrl/qcom/pinctrl-sc8180x.c b/drivers/pinctrl/qcom/pinctrl-sc8180x.c
> index 66f76ed22200..45ecb4a022ca 100644
> --- a/drivers/pinctrl/qcom/pinctrl-sc8180x.c
> +++ b/drivers/pinctrl/qcom/pinctrl-sc8180x.c
> @@ -1546,6 +1546,13 @@ static const struct msm_pingroup sc8180x_groups[] = {
>  	[193] = SDC_QDSD_PINGROUP(sdc2_data, 0x4b2000, 9, 0),
>  };
>  
> +static const int sc8180x_acpi_reserved_gpios[] = {
> +	0, 1, 2, 3,
> +	47, 48, 49, 50,
> +	126, 127, 128, 129,
> +	-1 /* terminator */
> +};

Wondering if this is converted to valid_mask at some point?

>  static const struct msm_gpio_wakeirq_map sc8180x_pdc_map[] = {
>  	{ 3, 31 }, { 5, 32 }, { 8, 33 }, { 9, 34 }, { 10, 100 }, { 12, 104 },
>  	{ 24, 37 }, { 26, 38 }, { 27, 41 }, { 28, 42 }, { 30, 39 }, { 36, 43 },
> @@ -1575,13 +1582,40 @@ static struct msm_pinctrl_soc_data sc8180x_pinctrl = {
>  	.nwakeirq_map = ARRAY_SIZE(sc8180x_pdc_map),
>  };
>  
> +static const struct msm_pinctrl_soc_data sc8180x_acpi_pinctrl = {
> +	.pins = sc8180x_pins,
> +	.npins = ARRAY_SIZE(sc8180x_pins),
> +	.groups = sc8180x_groups,
> +	.ngroups = ARRAY_SIZE(sc8180x_groups),
> +	.reserved_gpios = sc8180x_acpi_reserved_gpios,
> +	.ngpios = 191,
> +};
> +
>  static int sc8180x_pinctrl_probe(struct platform_device *pdev)
>  {
> -	return msm_pinctrl_probe(pdev, &sc8180x_pinctrl);
> +	const struct msm_pinctrl_soc_data *soc_data;
> +
> +	soc_data = device_get_match_data(&pdev->dev);

#include <linux/property.h>

> +	if (!soc_data)
> +		return -EINVAL;
> +
> +	return msm_pinctrl_probe(pdev, soc_data);
>  }
>  
> +static const struct acpi_device_id sc8180x_pinctrl_acpi_match[] = {

#include <linux/mod_devicetable.h>

> +	{
> +		.id = "QCOM040D",
> +		.driver_data = (kernel_ulong_t) &sc8180x_acpi_pinctrl,
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, sc8180x_pinctrl_acpi_match);
> +
>  static const struct of_device_id sc8180x_pinctrl_of_match[] = {
> -	{ .compatible = "qcom,sc8180x-tlmm", },
> +	{
> +		.compatible = "qcom,sc8180x-tlmm",
> +		.data = &sc8180x_pinctrl,
> +	},
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, sc8180x_pinctrl_of_match);
> @@ -1590,6 +1624,7 @@ static struct platform_driver sc8180x_pinctrl_driver = {
>  	.driver = {
>  		.name = "sc8180x-pinctrl",
>  		.of_match_table = sc8180x_pinctrl_of_match,
> +		.acpi_match_table = sc8180x_pinctrl_acpi_match,
>  	},
>  	.probe = sc8180x_pinctrl_probe,
>  	.remove = msm_pinctrl_remove,
> -- 
> 2.17.1
>

Hi, 

>  
> +static const int sc8180x_acpi_reserved_gpios[] = {
> +	0, 1, 2, 3,
> +	47, 48, 49, 50,
> +	126, 127, 128, 129,
> +	-1 /* terminator */
> +};
> +
These can vary per device (unless Qualcomm is enforcing something on the SC platform), so I don't think hardcoding is a good option.. Isn't there any data being passed on which ones should not be touched?

Konrad

On Sun 07 Mar 05:54 CST 2021, Konrad Dybcio wrote:

> Hi, 
> 
> >  
> > +static const int sc8180x_acpi_reserved_gpios[] = {
> > +	0, 1, 2, 3,
> > +	47, 48, 49, 50,
> > +	126, 127, 128, 129,
> > +	-1 /* terminator */
> > +};
> > +
> These can vary per device (unless Qualcomm is enforcing something on
> the SC platform), so I don't think hardcoding is a good option.. Isn't
> there any data being passed on which ones should not be touched?
> 

You're right, this is both ugly and error prone.

But we looked at the same when we added support for the 845 laptops but
where not able to find anything useful. On qdx2xxx the solution was to
pass a list of GPIOs that should be exposed by the driver, and only
those are made available, but nothing like this exist on the WoS devices.

Regards,
Bjorn