| Message ID | 20210303132622.4115-2-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add ACPI support for SC8180X pinctrl driver | expand |
On Wed, Mar 03, 2021 at 09:26:21PM +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> There are at least two possibilities to avoid this: - use is_of_node(dev_fwnode(dev)), or in case you need board files support, !(fwnode && is_of_fwnode(fwnode)) - provide the tiles support directly from the driver thru internal data structures And to me the second approach seems better, because there is no guarantee that tiles support is only defined by the fwnode type. -- With Best Regards, Andy Shevchenko
On Wed, Mar 03, 2021 at 04:08:09PM +0200, Andy Shevchenko wrote: > On Wed, Mar 03, 2021 at 09:26:21PM +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> > > There are at least two possibilities to avoid this: So could you explain why we should avoid including this header? Shawn > - use is_of_node(dev_fwnode(dev)), or in case you need board files support, > !(fwnode && is_of_fwnode(fwnode)) > - provide the tiles support directly from the driver thru internal data structures > > And to me the second approach seems better, because there is no guarantee that > tiles support is only defined by the fwnode type.
On Wed, Mar 03, 2021 at 10:45:27PM +0800, Shawn Guo wrote: > On Wed, Mar 03, 2021 at 04:08:09PM +0200, Andy Shevchenko wrote: > > On Wed, Mar 03, 2021 at 09:26:21PM +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> > > > > There are at least two possibilities to avoid this: > > So could you explain why we should avoid including this header? Here you can include it, but it's quite huge in order to have just one little function out of it. But main point is it seems that relying on firmware type for the tiles support is fragile. > > - use is_of_node(dev_fwnode(dev)), or in case you need board files support, > > !(fwnode && is_of_fwnode(fwnode)) > > - provide the tiles support directly from the driver thru internal data structures > > > > And to me the second approach seems better, because there is no guarantee that > > tiles support is only defined by the fwnode type. -- With Best Regards, Andy Shevchenko
On Wed, Mar 03, 2021 at 06:14:09PM +0200, Andy Shevchenko wrote: > On Wed, Mar 03, 2021 at 10:45:27PM +0800, Shawn Guo wrote: > > On Wed, Mar 03, 2021 at 04:08:09PM +0200, Andy Shevchenko wrote: > > > On Wed, Mar 03, 2021 at 09:26:21PM +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> > > > > > > There are at least two possibilities to avoid this: > > > > So could you explain why we should avoid including this header? > > Here you can include it, but it's quite huge in order to have just one little > function out of it. But main point is it seems that relying on firmware type > for the tiles support is fragile. Okay, I'm not entirely happy about the tiles checking for so many conditions, so looked for other way around. It turns out that pinctrl-sdm845 driver sets an example for not using tiles at all. I will send v3 to drop this tiles thing completely. Shawn
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c index d70caecd21d2..af6ed7f43058 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.c +++ b/drivers/pinctrl/qcom/pinctrl-msm.c @@ -4,6 +4,7 @@ * Copyright (c) 2013, The Linux Foundation. All rights reserved. */ +#include <linux/acpi.h> #include <linux/delay.h> #include <linux/err.h> #include <linux/io.h> @@ -1396,6 +1397,7 @@ int msm_pinctrl_probe(struct platform_device *pdev, { struct msm_pinctrl *pctrl; struct resource *res; + void __iomem *base; int ret; int i; @@ -1412,7 +1414,8 @@ int msm_pinctrl_probe(struct platform_device *pdev, raw_spin_lock_init(&pctrl->lock); - if (soc_data->tiles) { + if (soc_data->tiles && !has_acpi_companion(&pdev->dev)) { + /* DT boot with multiple tile resources */ for (i = 0; i < soc_data->ntiles; i++) { res = platform_get_resource_byname(pdev, IORESOURCE_MEM, soc_data->tiles[i]); @@ -1421,10 +1424,26 @@ int msm_pinctrl_probe(struct platform_device *pdev, return PTR_ERR(pctrl->regs[i]); } } else { + /* + * DT boot with one single resource or ACPI boot (always + * one memory resource in DSDT) + */ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - pctrl->regs[0] = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(pctrl->regs[0])) - return PTR_ERR(pctrl->regs[0]); + base = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(base)) + return PTR_ERR(base); + + if (soc_data->tiles) { + /* + * ACPI boot with SoC driver using titles like + * pinctrl-sc8180x. + */ + for (i = 0; i < soc_data->ntiles; i++) + pctrl->regs[i] = base + + soc_data->tile_offsets[i]; + } else { + pctrl->regs[0] = base; + } pctrl->phys_base[0] = res->start; } diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h index e31a5167c91e..91333942d53c 100644 --- a/drivers/pinctrl/qcom/pinctrl-msm.h +++ b/drivers/pinctrl/qcom/pinctrl-msm.h @@ -131,6 +131,7 @@ struct msm_pinctrl_soc_data { bool pull_no_keeper; const char *const *tiles; unsigned int ntiles; + const u32 *tile_offsets; const int *reserved_gpios; const struct msm_gpio_wakeirq_map *wakeirq_map; unsigned int nwakeirq_map;
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. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- drivers/pinctrl/qcom/pinctrl-msm.c | 27 +++++++++++++++++++++++---- drivers/pinctrl/qcom/pinctrl-msm.h | 1 + 2 files changed, 24 insertions(+), 4 deletions(-)