| Message ID | 20210301014329.30104-3-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add ACPI support for SC8180X pinctrl driver | expand |
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? > +#include <linux/acpi.h> No use of this header, see below. (Perhaps you meant mod_devicetable.h) ... > +static const u32 sc8180x_tile_offsets[] = { > + 0x00d00000, > + 0x00500000, > + 0x00100000 Leave comma here. > +}; ... > +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? > +}; ... > + 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. ... > +#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.
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 --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,
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(-)