diff mbox series

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

Message ID 20210301014329.30104-3-shawn.guo@linaro.org
State New
Headers show
Series Add ACPI support for SC8180X pinctrl driver | expand

Commit Message

Shawn Guo March 1, 2021, 1:43 a.m. UTC
It adds ACPI probe support with tile offsets passed over to msm core
driver via sc8180x_tile_offsets, as TLMM is described a single memory
region in ACPI DSDT.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>

---
 drivers/pinctrl/qcom/Kconfig           |  2 +-
 drivers/pinctrl/qcom/pinctrl-sc8180x.c | 48 +++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 2 deletions(-)

-- 
2.17.1

Comments

Shawn Guo March 2, 2021, 3 a.m. UTC | #1
On Mon, Mar 01, 2021 at 04:37:50PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 01, 2021 at 09:43:29AM +0800, Shawn Guo wrote:
> > It adds ACPI probe support with tile offsets passed over to msm core
> > driver via sc8180x_tile_offsets, as TLMM is described a single memory
> > region in ACPI DSDT.
> 
> ...
> 
> >  config PINCTRL_SC8180X
> >  	tristate "Qualcomm Technologies Inc SC8180x pin controller driver"
> > -	depends on GPIOLIB && OF
> > +	depends on GPIOLIB && (OF || ACPI)
> 
> Can you consider dropping OF dependency completely?

Not sure.  Looking at those driver options in drivers/pinctrl/qcom/Kconfig,
I think it's a global thing, and should be addressed separately anyway.

> 
> > +#include <linux/acpi.h>
> 
> No use of this header, see below.

has_acpi_companion() and ACPI_PTR use it.

> 
> (Perhaps you meant mod_devicetable.h)
> 
> ...
> 
> > +static const u32 sc8180x_tile_offsets[] = {
> > +	0x00d00000,
> > +	0x00500000,
> > +	0x00100000
> 
> Leave comma here.

Well, this is to respect the taste of original author of the driver, if
you take a look at sc8180x_tiles[] above and enum after.

> 
> > +};
> 
> ...
> 
> > +static const int sc8180x_acpi_reserved_gpios[] = {
> > +	0, 1, 2, 3,
> > +	47, 48, 49, 50,
> > +	126, 127, 128, 129,
> 
> > +	-1
> 
> -1?
> Is it kinda terminator?

Yes, it is.  I will add a comment there.

> 
> > +};
> 
> ...
> 
> > +	if (pdev->dev.of_node) {
> > +		ret = msm_pinctrl_probe(pdev, &sc8180x_pinctrl);
> > +	} else if (has_acpi_companion(&pdev->dev)) {
> > +		ret = msm_pinctrl_probe(pdev, &sc8180x_acpi_pinctrl);
> > +	} else {
> > +		dev_err(&pdev->dev, "DT and ACPI disabled\n");
> > +		ret = -EINVAL;
> > +	}
> 
> Use driver_data field for this and device_get_match_data() instead of above.

Good suggestion, thanks!

> 
> ...
> 
> > +#ifdef CONFIG_ACPI
> 
> Drop this ugly ifdeffery.
> 
> > +static const struct acpi_device_id sc8180x_pinctrl_acpi_match[] = {
> > +	{ "QCOM040D"},
> 
> > +	{ },
> 
> No comma for terminator line.
> 
> > +};
> > +MODULE_DEVICE_TABLE(acpi, sc8180x_pinctrl_acpi_match);
> > +#endif
> 
> ...
> 
> > +		.acpi_match_table = ACPI_PTR(sc8180x_pinctrl_acpi_match),
> 
> No ACPI_PTR(), please.

Sounds good.

Shawn
diff mbox series

Patch

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 b765bf667574..38117ceb4d8f 100644
--- a/drivers/pinctrl/qcom/pinctrl-sc8180x.c
+++ b/drivers/pinctrl/qcom/pinctrl-sc8180x.c
@@ -4,6 +4,7 @@ 
  * Copyright (c) 2020-2021, Linaro Ltd.
  */
 
+#include <linux/acpi.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -17,6 +18,12 @@  static const char * const sc8180x_tiles[] = {
 	"west"
 };
 
+static const u32 sc8180x_tile_offsets[] = {
+	0x00d00000,
+	0x00500000,
+	0x00100000
+};
+
 enum {
 	SOUTH,
 	EAST,
@@ -1557,6 +1564,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
+};
+
 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 },
@@ -1588,11 +1602,42 @@  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 = {
+	.tiles = sc8180x_tiles,
+	.ntiles = ARRAY_SIZE(sc8180x_tiles),
+	.tile_offsets = sc8180x_tile_offsets,
+	.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);
+	int ret;
+
+	if (pdev->dev.of_node) {
+		ret = msm_pinctrl_probe(pdev, &sc8180x_pinctrl);
+	} else if (has_acpi_companion(&pdev->dev)) {
+		ret = msm_pinctrl_probe(pdev, &sc8180x_acpi_pinctrl);
+	} else {
+		dev_err(&pdev->dev, "DT and ACPI disabled\n");
+		ret = -EINVAL;
+	}
+
+	return ret;
 }
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id sc8180x_pinctrl_acpi_match[] = {
+	{ "QCOM040D"},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, sc8180x_pinctrl_acpi_match);
+#endif
+
 static const struct of_device_id sc8180x_pinctrl_of_match[] = {
 	{ .compatible = "qcom,sc8180x-tlmm", },
 	{ },
@@ -1603,6 +1648,7 @@  static struct platform_driver sc8180x_pinctrl_driver = {
 	.driver = {
 		.name = "sc8180x-pinctrl",
 		.of_match_table = sc8180x_pinctrl_of_match,
+		.acpi_match_table = ACPI_PTR(sc8180x_pinctrl_acpi_match),
 	},
 	.probe = sc8180x_pinctrl_probe,
 	.remove = msm_pinctrl_remove,