| Message ID | 20210301014329.30104-1-shawn.guo@linaro.org |
|---|---|
| Headers | show |
| Series | Add ACPI support for SC8180X pinctrl driver | expand |
On Mon, Mar 01, 2021 at 09:43:27AM +0800, Shawn Guo wrote: > This is a couple of patches that enable ACPI probe for SC8180X pinctrl > driver. The msm core driver needs a bit change to handle tiles mapping > differently between DT and ACPI. Please, Cc me for this series. Meanwhile I think we have to understand the numbering scheme used by ACPI firmware on the machine in question.
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. -- With Best Regards, Andy Shevchenko
On Mon, Mar 01, 2021 at 04:34:30PM +0200, Andy Shevchenko wrote: > On Mon, Mar 01, 2021 at 09:43:28AM +0800, Shawn Guo wrote: > > It's not always the case that DT and ACPI describe hardware resource in > > the same schema, even for a single platform. For example, on SC8180X, > > DT uses the tiles schema while ACPI describe memory resource as a single > > region. It patches msm_pinctrl_probe() function to map tiles regions > > only for DT. While for ACPI, it maps the single memory resource and > > calculate tile bases with offsets passed from SoC data. > > ... > > > +#include <linux/acpi.h> > > No use of this header. See below. > (Perhaps you meant mod_devicetable.h) has_acpi_companion() call needs the header. > > ... > > > - if (soc_data->tiles) { > > + if (soc_data->tiles && !has_acpi_companion(&pdev->dev)) { > > Any documentation to understand this change? Well, !has_acpi_companion() is just to rule out ACPI boot and ensure this is a DT boot with tiles. > > ... > > > + if (soc_data->tiles) { > > + for (i = 0; i < soc_data->ntiles; i++) > > + pctrl->regs[i] = base + > > + soc_data->tile_offsets[i]; > > + } else { > > + pctrl->regs[0] = base; > > + } > > And so this? For ACPI boot or DT without tiles, there is only one single memory resource to map. But for SoC driver like pinctrl-sc8180x that defines pins with tiles, even with ACPI boot, we need to have multiple regs[] to hold bases for tiles. I will add comment to make it easier for understanding. Shawn
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