* [RFC PATCH 1/4] pinctrl: Rename pinctrl_utils_dt_free_map to pinctrl_utils_free_map
2016-03-31 11:44 [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Irina Tirdea
@ 2016-03-31 11:44 ` Irina Tirdea
2016-04-01 13:08 ` Linus Walleij
2016-03-31 11:44 ` [RFC PATCH 2/4] pinctrl: pinconf-generic: Add ACPI support Irina Tirdea
` (4 subsequent siblings)
5 siblings, 1 reply; 41+ messages in thread
From: Irina Tirdea @ 2016-03-31 11:44 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
linux-gpio, linux-acpi
Cc: Rob Herring, Heikki Krogerus, Andy Shevchenko, Octavian Purdila,
Cristina Ciocan, devicetree, linux-kernel, Irina Tirdea
Rename pinctrl_utils_dt_free_map to pinctrl_utils_free_map, since
it does not depend on device tree despite the current name. This
will enforce a consistent naming in pinctr-utils.c and will make
it clear it can be called from outside device tree (e.g. from
ACPI handling code).
Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
drivers/pinctrl/bcm/pinctrl-bcm281xx.c | 2 +-
drivers/pinctrl/bcm/pinctrl-cygnus-mux.c | 2 +-
drivers/pinctrl/bcm/pinctrl-iproc-gpio.c | 2 +-
drivers/pinctrl/bcm/pinctrl-nsp-gpio.c | 2 +-
drivers/pinctrl/berlin/berlin.c | 2 +-
drivers/pinctrl/mediatek/pinctrl-mtk-common.c | 4 ++--
drivers/pinctrl/meson/pinctrl-meson.c | 2 +-
drivers/pinctrl/nomadik/pinctrl-abx500.c | 4 ++--
drivers/pinctrl/nomadik/pinctrl-nomadik.c | 4 ++--
drivers/pinctrl/pinconf-generic.c | 2 +-
drivers/pinctrl/pinctrl-amd.c | 2 +-
drivers/pinctrl/pinctrl-as3722.c | 2 +-
drivers/pinctrl/pinctrl-at91-pio4.c | 4 ++--
drivers/pinctrl/pinctrl-digicolor.c | 2 +-
drivers/pinctrl/pinctrl-lpc18xx.c | 2 +-
drivers/pinctrl/pinctrl-palmas.c | 2 +-
drivers/pinctrl/pinctrl-pic32.c | 2 +-
drivers/pinctrl/pinctrl-pistachio.c | 2 +-
drivers/pinctrl/pinctrl-tb10x.c | 2 +-
drivers/pinctrl/pinctrl-utils.c | 4 ++--
drivers/pinctrl/pinctrl-utils.h | 2 +-
drivers/pinctrl/pinctrl-zynq.c | 2 +-
drivers/pinctrl/pxa/pinctrl-pxa2xx.c | 2 +-
drivers/pinctrl/qcom/pinctrl-msm.c | 2 +-
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c | 2 +-
drivers/pinctrl/qcom/pinctrl-spmi-mpp.c | 2 +-
drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c | 2 +-
drivers/pinctrl/qcom/pinctrl-ssbi-mpp.c | 2 +-
drivers/pinctrl/stm32/pinctrl-stm32.c | 4 ++--
drivers/pinctrl/tegra/pinctrl-tegra-xusb.c | 2 +-
drivers/pinctrl/tegra/pinctrl-tegra.c | 4 ++--
drivers/pinctrl/uniphier/pinctrl-uniphier-core.c | 2 +-
32 files changed, 39 insertions(+), 39 deletions(-)
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm281xx.c b/drivers/pinctrl/bcm/pinctrl-bcm281xx.c
index c3c692e..f043b83 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm281xx.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm281xx.c
@@ -1024,7 +1024,7 @@ static struct pinctrl_ops bcm281xx_pinctrl_ops = {
.get_group_pins = bcm281xx_pinctrl_get_group_pins,
.pin_dbg_show = bcm281xx_pinctrl_pin_dbg_show,
.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int bcm281xx_pinctrl_get_fcns_count(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/bcm/pinctrl-cygnus-mux.c b/drivers/pinctrl/bcm/pinctrl-cygnus-mux.c
index 9728f3d..f0184dc 100644
--- a/drivers/pinctrl/bcm/pinctrl-cygnus-mux.c
+++ b/drivers/pinctrl/bcm/pinctrl-cygnus-mux.c
@@ -737,7 +737,7 @@ static const struct pinctrl_ops cygnus_pinctrl_ops = {
.get_group_pins = cygnus_get_group_pins,
.pin_dbg_show = cygnus_pin_dbg_show,
.dt_node_to_map = pinconf_generic_dt_node_to_map_group,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int cygnus_get_functions_count(struct pinctrl_dev *pctrl_dev)
diff --git a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
index d530ab4..9ed9881 100644
--- a/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
+++ b/drivers/pinctrl/bcm/pinctrl-iproc-gpio.c
@@ -379,7 +379,7 @@ static const struct pinctrl_ops iproc_pctrl_ops = {
.get_groups_count = iproc_get_groups_count,
.get_group_name = iproc_get_group_name,
.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int iproc_gpio_set_pull(struct iproc_gpio *chip, unsigned gpio,
diff --git a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
index ac90043..6c7d5f5 100644
--- a/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
+++ b/drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
@@ -363,7 +363,7 @@ static const struct pinctrl_ops nsp_pctrl_ops = {
.get_groups_count = nsp_get_groups_count,
.get_group_name = nsp_get_group_name,
.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int nsp_gpio_set_slew(struct nsp_gpio *chip, unsigned gpio, u16 slew)
diff --git a/drivers/pinctrl/berlin/berlin.c b/drivers/pinctrl/berlin/berlin.c
index 46f2b48..87b17b7 100644
--- a/drivers/pinctrl/berlin/berlin.c
+++ b/drivers/pinctrl/berlin/berlin.c
@@ -104,7 +104,7 @@ static const struct pinctrl_ops berlin_pinctrl_ops = {
.get_groups_count = &berlin_pinctrl_get_group_count,
.get_group_name = &berlin_pinctrl_get_group_name,
.dt_node_to_map = &berlin_pinctrl_dt_node_to_map,
- .dt_free_map = &pinctrl_utils_dt_free_map,
+ .dt_free_map = &pinctrl_utils_free_map,
};
static int berlin_pinmux_get_functions_count(struct pinctrl_dev *pctrl_dev)
diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
index 2bbe6f7..8ca82c1 100644
--- a/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
+++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common.c
@@ -605,7 +605,7 @@ static int mtk_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
ret = mtk_pctrl_dt_subnode_to_map(pctldev, np, map,
&reserved_maps, num_maps);
if (ret < 0) {
- pinctrl_utils_dt_free_map(pctldev, *map, *num_maps);
+ pinctrl_utils_free_map(pctldev, *map, *num_maps);
of_node_put(np);
return ret;
}
@@ -644,7 +644,7 @@ static int mtk_pctrl_get_group_pins(struct pinctrl_dev *pctldev,
static const struct pinctrl_ops mtk_pctrl_ops = {
.dt_node_to_map = mtk_pctrl_dt_node_to_map,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
.get_groups_count = mtk_pctrl_get_groups_count,
.get_group_name = mtk_pctrl_get_group_name,
.get_group_pins = mtk_pctrl_get_group_pins,
diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
index 0bdb8fd..5bcbd3e 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.c
+++ b/drivers/pinctrl/meson/pinctrl-meson.c
@@ -171,7 +171,7 @@ static const struct pinctrl_ops meson_pctrl_ops = {
.get_group_name = meson_get_group_name,
.get_group_pins = meson_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
.pin_dbg_show = meson_pin_dbg_show,
};
diff --git a/drivers/pinctrl/nomadik/pinctrl-abx500.c b/drivers/pinctrl/nomadik/pinctrl-abx500.c
index 1f7469c..5323568 100644
--- a/drivers/pinctrl/nomadik/pinctrl-abx500.c
+++ b/drivers/pinctrl/nomadik/pinctrl-abx500.c
@@ -937,7 +937,7 @@ static int abx500_dt_node_to_map(struct pinctrl_dev *pctldev,
ret = abx500_dt_subnode_to_map(pctldev, np, map,
&reserved_maps, num_maps);
if (ret < 0) {
- pinctrl_utils_dt_free_map(pctldev, *map, *num_maps);
+ pinctrl_utils_free_map(pctldev, *map, *num_maps);
return ret;
}
}
@@ -951,7 +951,7 @@ static const struct pinctrl_ops abx500_pinctrl_ops = {
.get_group_pins = abx500_get_group_pins,
.pin_dbg_show = abx500_pin_dbg_show,
.dt_node_to_map = abx500_dt_node_to_map,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int abx500_pin_config_get(struct pinctrl_dev *pctldev,
diff --git a/drivers/pinctrl/nomadik/pinctrl-nomadik.c b/drivers/pinctrl/nomadik/pinctrl-nomadik.c
index 3524061..c9c8c17 100644
--- a/drivers/pinctrl/nomadik/pinctrl-nomadik.c
+++ b/drivers/pinctrl/nomadik/pinctrl-nomadik.c
@@ -1612,7 +1612,7 @@ static int nmk_pinctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
ret = nmk_pinctrl_dt_subnode_to_map(pctldev, np, map,
&reserved_maps, num_maps);
if (ret < 0) {
- pinctrl_utils_dt_free_map(pctldev, *map, *num_maps);
+ pinctrl_utils_free_map(pctldev, *map, *num_maps);
return ret;
}
}
@@ -1626,7 +1626,7 @@ static const struct pinctrl_ops nmk_pinctrl_ops = {
.get_group_pins = nmk_get_group_pins,
.pin_dbg_show = nmk_pin_dbg_show,
.dt_node_to_map = nmk_pinctrl_dt_node_to_map,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int nmk_pmx_get_funcs_cnt(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index 79e6159..d5bf9fa 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -386,7 +386,7 @@ int pinconf_generic_dt_node_to_map(struct pinctrl_dev *pctldev,
return 0;
exit:
- pinctrl_utils_dt_free_map(pctldev, *map, *num_maps);
+ pinctrl_utils_free_map(pctldev, *map, *num_maps);
return ret;
}
EXPORT_SYMBOL_GPL(pinconf_generic_dt_node_to_map);
diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index 5c025f5..f71a04e 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -580,7 +580,7 @@ static const struct pinctrl_ops amd_pinctrl_ops = {
.get_group_pins = amd_get_group_pins,
#ifdef CONFIG_OF
.dt_node_to_map = pinconf_generic_dt_node_to_map_group,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
#endif
};
diff --git a/drivers/pinctrl/pinctrl-as3722.c b/drivers/pinctrl/pinctrl-as3722.c
index e844fdc..1070bb9 100644
--- a/drivers/pinctrl/pinctrl-as3722.c
+++ b/drivers/pinctrl/pinctrl-as3722.c
@@ -201,7 +201,7 @@ static const struct pinctrl_ops as3722_pinctrl_ops = {
.get_group_name = as3722_pinctrl_get_group_name,
.get_group_pins = as3722_pinctrl_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int as3722_pinctrl_get_funcs_count(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
index 4429312..629b6fe 100644
--- a/drivers/pinctrl/pinctrl-at91-pio4.c
+++ b/drivers/pinctrl/pinctrl-at91-pio4.c
@@ -579,7 +579,7 @@ static int atmel_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
}
if (ret < 0) {
- pinctrl_utils_dt_free_map(pctldev, *map, *num_maps);
+ pinctrl_utils_free_map(pctldev, *map, *num_maps);
dev_err(pctldev->dev, "can't create maps for node %s\n",
np_config->full_name);
}
@@ -592,7 +592,7 @@ static const struct pinctrl_ops atmel_pctlops = {
.get_group_name = atmel_pctl_get_group_name,
.get_group_pins = atmel_pctl_get_group_pins,
.dt_node_to_map = atmel_pctl_dt_node_to_map,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int atmel_pmx_get_functions_count(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/pinctrl-digicolor.c b/drivers/pinctrl/pinctrl-digicolor.c
index f1343d6..b347806 100644
--- a/drivers/pinctrl/pinctrl-digicolor.c
+++ b/drivers/pinctrl/pinctrl-digicolor.c
@@ -84,7 +84,7 @@ static struct pinctrl_ops dc_pinctrl_ops = {
.get_group_name = dc_get_group_name,
.get_group_pins = dc_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static const char *const dc_functions[] = {
diff --git a/drivers/pinctrl/pinctrl-lpc18xx.c b/drivers/pinctrl/pinctrl-lpc18xx.c
index b1767f7..e897b63 100644
--- a/drivers/pinctrl/pinctrl-lpc18xx.c
+++ b/drivers/pinctrl/pinctrl-lpc18xx.c
@@ -1252,7 +1252,7 @@ static const struct pinctrl_ops lpc18xx_pctl_ops = {
.get_group_name = lpc18xx_pctl_get_group_name,
.get_group_pins = lpc18xx_pctl_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static struct pinctrl_desc lpc18xx_scu_desc = {
diff --git a/drivers/pinctrl/pinctrl-palmas.c b/drivers/pinctrl/pinctrl-palmas.c
index f7e1680..8f0163f 100644
--- a/drivers/pinctrl/pinctrl-palmas.c
+++ b/drivers/pinctrl/pinctrl-palmas.c
@@ -656,7 +656,7 @@ static const struct pinctrl_ops palmas_pinctrl_ops = {
.get_group_name = palmas_pinctrl_get_group_name,
.get_group_pins = palmas_pinctrl_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int palmas_pinctrl_get_funcs_count(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/pinctrl-pic32.c b/drivers/pinctrl/pinctrl-pic32.c
index 0b07d4b..87ee1c4 100644
--- a/drivers/pinctrl/pinctrl-pic32.c
+++ b/drivers/pinctrl/pinctrl-pic32.c
@@ -1743,7 +1743,7 @@ static const struct pinctrl_ops pic32_pinctrl_ops = {
.get_group_name = pic32_pinctrl_get_group_name,
.get_group_pins = pic32_pinctrl_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int pic32_pinmux_get_functions_count(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/pinctrl-pistachio.c b/drivers/pinctrl/pinctrl-pistachio.c
index 2673cd9..65874b5 100644
--- a/drivers/pinctrl/pinctrl-pistachio.c
+++ b/drivers/pinctrl/pinctrl-pistachio.c
@@ -913,7 +913,7 @@ static const struct pinctrl_ops pistachio_pinctrl_ops = {
.get_group_name = pistachio_pinctrl_get_group_name,
.get_group_pins = pistachio_pinctrl_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int pistachio_pinmux_get_functions_count(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/pinctrl-tb10x.c b/drivers/pinctrl/pinctrl-tb10x.c
index 6546b9b..620af57 100644
--- a/drivers/pinctrl/pinctrl-tb10x.c
+++ b/drivers/pinctrl/pinctrl-tb10x.c
@@ -582,7 +582,7 @@ static struct pinctrl_ops tb10x_pinctrl_ops = {
.get_group_name = tb10x_get_group_name,
.get_group_pins = tb10x_get_group_pins,
.dt_node_to_map = tb10x_dt_node_to_map,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int tb10x_get_functions_count(struct pinctrl_dev *pctl)
diff --git a/drivers/pinctrl/pinctrl-utils.c b/drivers/pinctrl/pinctrl-utils.c
index d77693f..9189fba 100644
--- a/drivers/pinctrl/pinctrl-utils.c
+++ b/drivers/pinctrl/pinctrl-utils.c
@@ -122,7 +122,7 @@ int pinctrl_utils_add_config(struct pinctrl_dev *pctldev,
}
EXPORT_SYMBOL_GPL(pinctrl_utils_add_config);
-void pinctrl_utils_dt_free_map(struct pinctrl_dev *pctldev,
+void pinctrl_utils_free_map(struct pinctrl_dev *pctldev,
struct pinctrl_map *map, unsigned num_maps)
{
int i;
@@ -139,4 +139,4 @@ void pinctrl_utils_dt_free_map(struct pinctrl_dev *pctldev,
}
kfree(map);
}
-EXPORT_SYMBOL_GPL(pinctrl_utils_dt_free_map);
+EXPORT_SYMBOL_GPL(pinctrl_utils_free_map);
diff --git a/drivers/pinctrl/pinctrl-utils.h b/drivers/pinctrl/pinctrl-utils.h
index d0ffe1c..8f9f2d2 100644
--- a/drivers/pinctrl/pinctrl-utils.h
+++ b/drivers/pinctrl/pinctrl-utils.h
@@ -37,7 +37,7 @@ int pinctrl_utils_add_map_configs(struct pinctrl_dev *pctldev,
int pinctrl_utils_add_config(struct pinctrl_dev *pctldev,
unsigned long **configs, unsigned *num_configs,
unsigned long config);
-void pinctrl_utils_dt_free_map(struct pinctrl_dev *pctldev,
+void pinctrl_utils_free_map(struct pinctrl_dev *pctldev,
struct pinctrl_map *map, unsigned num_maps);
#endif /* __PINCTRL_UTILS_H__ */
diff --git a/drivers/pinctrl/pinctrl-zynq.c b/drivers/pinctrl/pinctrl-zynq.c
index 76f1abd..3f1b55f 100644
--- a/drivers/pinctrl/pinctrl-zynq.c
+++ b/drivers/pinctrl/pinctrl-zynq.c
@@ -862,7 +862,7 @@ static const struct pinctrl_ops zynq_pctrl_ops = {
.get_group_name = zynq_pctrl_get_group_name,
.get_group_pins = zynq_pctrl_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
/* pinmux */
diff --git a/drivers/pinctrl/pxa/pinctrl-pxa2xx.c b/drivers/pinctrl/pxa/pinctrl-pxa2xx.c
index f553313..6098685 100644
--- a/drivers/pinctrl/pxa/pinctrl-pxa2xx.c
+++ b/drivers/pinctrl/pxa/pinctrl-pxa2xx.c
@@ -57,7 +57,7 @@ static int pxa2xx_pctrl_get_group_pins(struct pinctrl_dev *pctldev,
static const struct pinctrl_ops pxa2xx_pctl_ops = {
#ifdef CONFIG_OF
.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
#endif
.get_groups_count = pxa2xx_pctrl_get_groups_count,
.get_group_name = pxa2xx_pctrl_get_group_name,
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index 8777cf0..f9322d4 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -101,7 +101,7 @@ static const struct pinctrl_ops msm_pinctrl_ops = {
.get_group_name = msm_get_group_name,
.get_group_pins = msm_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_group,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int msm_get_functions_count(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
index 4e12ded..857ccfa 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c
@@ -212,7 +212,7 @@ static const struct pinctrl_ops pmic_gpio_pinctrl_ops = {
.get_group_name = pmic_gpio_get_group_name,
.get_group_pins = pmic_gpio_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_group,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int pmic_gpio_get_functions_count(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/qcom/pinctrl-spmi-mpp.c b/drivers/pinctrl/qcom/pinctrl-spmi-mpp.c
index 2a3e549..4696600 100644
--- a/drivers/pinctrl/qcom/pinctrl-spmi-mpp.c
+++ b/drivers/pinctrl/qcom/pinctrl-spmi-mpp.c
@@ -235,7 +235,7 @@ static const struct pinctrl_ops pmic_mpp_pinctrl_ops = {
.get_group_name = pmic_mpp_get_group_name,
.get_group_pins = pmic_mpp_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_group,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int pmic_mpp_get_functions_count(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c b/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
index cd8580d..ecd784b 100644
--- a/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
+++ b/drivers/pinctrl/qcom/pinctrl-ssbi-gpio.c
@@ -200,7 +200,7 @@ static const struct pinctrl_ops pm8xxx_pinctrl_ops = {
.get_group_name = pm8xxx_get_group_name,
.get_group_pins = pm8xxx_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_group,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int pm8xxx_get_functions_count(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/qcom/pinctrl-ssbi-mpp.c b/drivers/pinctrl/qcom/pinctrl-ssbi-mpp.c
index 54a5402..ac79021 100644
--- a/drivers/pinctrl/qcom/pinctrl-ssbi-mpp.c
+++ b/drivers/pinctrl/qcom/pinctrl-ssbi-mpp.c
@@ -277,7 +277,7 @@ static const struct pinctrl_ops pm8xxx_pinctrl_ops = {
.get_group_name = pm8xxx_get_group_name,
.get_group_pins = pm8xxx_get_group_pins,
.dt_node_to_map = pinconf_generic_dt_node_to_map_group,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int pm8xxx_get_functions_count(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 8deb566..b97b7c0 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -358,7 +358,7 @@ static int stm32_pctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
ret = stm32_pctrl_dt_subnode_to_map(pctldev, np, map,
&reserved_maps, num_maps);
if (ret < 0) {
- pinctrl_utils_dt_free_map(pctldev, *map, *num_maps);
+ pinctrl_utils_free_map(pctldev, *map, *num_maps);
return ret;
}
}
@@ -396,7 +396,7 @@ static int stm32_pctrl_get_group_pins(struct pinctrl_dev *pctldev,
static const struct pinctrl_ops stm32_pctrl_ops = {
.dt_node_to_map = stm32_pctrl_dt_node_to_map,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
.get_groups_count = stm32_pctrl_get_groups_count,
.get_group_name = stm32_pctrl_get_group_name,
.get_group_pins = stm32_pctrl_get_group_pins,
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra-xusb.c b/drivers/pinctrl/tegra/pinctrl-tegra-xusb.c
index 2f06029..e58b5f3 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra-xusb.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra-xusb.c
@@ -267,7 +267,7 @@ static const struct pinctrl_ops tegra_xusb_padctl_pinctrl_ops = {
.get_group_name = tegra_xusb_padctl_get_group_name,
.get_group_pins = tegra_xusb_padctl_get_group_pins,
.dt_node_to_map = tegra_xusb_padctl_dt_node_to_map,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int tegra_xusb_padctl_get_functions_count(struct pinctrl_dev *pinctrl)
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
index 4938882..3f7fce9 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
@@ -215,7 +215,7 @@ static int tegra_pinctrl_dt_node_to_map(struct pinctrl_dev *pctldev,
ret = tegra_pinctrl_dt_subnode_to_map(pctldev, np, map,
&reserved_maps, num_maps);
if (ret < 0) {
- pinctrl_utils_dt_free_map(pctldev, *map,
+ pinctrl_utils_free_map(pctldev, *map,
*num_maps);
of_node_put(np);
return ret;
@@ -233,7 +233,7 @@ static const struct pinctrl_ops tegra_pinctrl_ops = {
.pin_dbg_show = tegra_pinctrl_pin_dbg_show,
#endif
.dt_node_to_map = tegra_pinctrl_dt_node_to_map,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int tegra_pinctrl_get_funcs_count(struct pinctrl_dev *pctldev)
diff --git a/drivers/pinctrl/uniphier/pinctrl-uniphier-core.c b/drivers/pinctrl/uniphier/pinctrl-uniphier-core.c
index 589872c..c9e4a85 100644
--- a/drivers/pinctrl/uniphier/pinctrl-uniphier-core.c
+++ b/drivers/pinctrl/uniphier/pinctrl-uniphier-core.c
@@ -115,7 +115,7 @@ static const struct pinctrl_ops uniphier_pctlops = {
.pin_dbg_show = uniphier_pctl_pin_dbg_show,
#endif
.dt_node_to_map = pinconf_generic_dt_node_to_map_all,
- .dt_free_map = pinctrl_utils_dt_free_map,
+ .dt_free_map = pinctrl_utils_free_map,
};
static int uniphier_conf_pin_bias_get(struct pinctrl_dev *pctldev,
--
1.9.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [RFC PATCH 1/4] pinctrl: Rename pinctrl_utils_dt_free_map to pinctrl_utils_free_map
2016-03-31 11:44 ` [RFC PATCH 1/4] pinctrl: Rename pinctrl_utils_dt_free_map to pinctrl_utils_free_map Irina Tirdea
@ 2016-04-01 13:08 ` Linus Walleij
0 siblings, 0 replies; 41+ messages in thread
From: Linus Walleij @ 2016-04-01 13:08 UTC (permalink / raw)
To: Irina Tirdea
Cc: Rafael J. Wysocki, Len Brown, Mika Westerberg, linux-gpio,
ACPI Devel Maling List, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Octavian Purdila, Cristina Ciocan, devicetree,
linux-kernel
On Thu, Mar 31, 2016 at 1:44 PM, Irina Tirdea <irina.tirdea@intel.com> wrote:
> Rename pinctrl_utils_dt_free_map to pinctrl_utils_free_map, since
> it does not depend on device tree despite the current name. This
> will enforce a consistent naming in pinctr-utils.c and will make
> it clear it can be called from outside device tree (e.g. from
> ACPI handling code).
>
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
OK that's a good change making nice ground work for the rest.
Patch applied.
For the rest I want to hear from Rafael and/or Mika first.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 41+ messages in thread
* [RFC PATCH 2/4] pinctrl: pinconf-generic: Add ACPI support
2016-03-31 11:44 [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Irina Tirdea
2016-03-31 11:44 ` [RFC PATCH 1/4] pinctrl: Rename pinctrl_utils_dt_free_map to pinctrl_utils_free_map Irina Tirdea
@ 2016-03-31 11:44 ` Irina Tirdea
2016-04-01 14:05 ` Andy Shevchenko
2016-03-31 11:44 ` [RFC PATCH 3/4] pinctrl: " Irina Tirdea
` (3 subsequent siblings)
5 siblings, 1 reply; 41+ messages in thread
From: Irina Tirdea @ 2016-03-31 11:44 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
linux-gpio, linux-acpi
Cc: Rob Herring, Heikki Krogerus, Andy Shevchenko, Octavian Purdila,
Cristina Ciocan, devicetree, linux-kernel, Irina Tirdea
Add ACPI support for the generic device tree properties.
Convert the pinconf generic code to handle both ACPI and
device tree by using the fwnode_property API. Also include
renaming device tree references in names of functions and
structures from 'dt' to 'fwnode'.
Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
drivers/acpi/property.c | 8 +--
drivers/base/property.c | 36 ++++++++--
drivers/pinctrl/pinconf-generic.c | 121 +++++++++++++++++++-------------
include/linux/acpi.h | 4 +-
include/linux/pinctrl/pinconf-generic.h | 27 ++++---
include/linux/property.h | 9 +++
6 files changed, 130 insertions(+), 75 deletions(-)
diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
index f2fd3fe..b10f935 100644
--- a/drivers/acpi/property.c
+++ b/drivers/acpi/property.c
@@ -794,13 +794,13 @@ int acpi_node_prop_read(struct fwnode_handle *fwnode, const char *propname,
/**
* acpi_get_next_subnode - Return the next child node handle for a device.
- * @dev: Device to find the next child node for.
+ * @fwnode: Handle to the device to find the next child node for.
* @child: Handle to one of the device's child nodes or a null handle.
*/
-struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
+struct fwnode_handle *acpi_get_next_subnode(struct fwnode_handle *fwnode,
struct fwnode_handle *child)
{
- struct acpi_device *adev = ACPI_COMPANION(dev);
+ struct acpi_device *adev = to_acpi_device_node(fwnode);
struct list_head *head, *next;
if (!adev)
@@ -816,7 +816,7 @@ struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
next = adev->node.next;
if (next == head) {
child = NULL;
- adev = ACPI_COMPANION(dev);
+ adev = to_acpi_device_node(fwnode);
goto nondev;
}
adev = list_entry(next, struct acpi_device, node);
diff --git a/drivers/base/property.c b/drivers/base/property.c
index 9b1a65d..153a316 100644
--- a/drivers/base/property.c
+++ b/drivers/base/property.c
@@ -859,24 +859,37 @@ int device_add_property_set(struct device *dev, const struct property_set *pset)
EXPORT_SYMBOL_GPL(device_add_property_set);
/**
- * device_get_next_child_node - Return the next child node handle for a device
- * @dev: Device to find the next child node for.
+ * fwnode_get_next_child_node - Return the next child node handle for a device
+ * @fwnode: Handle to the device to find the next child node for.
* @child: Handle to one of the device's child nodes or a null handle.
*/
-struct fwnode_handle *device_get_next_child_node(struct device *dev,
+struct fwnode_handle *fwnode_get_next_child_node(struct fwnode_handle *fwnode,
struct fwnode_handle *child)
{
- if (IS_ENABLED(CONFIG_OF) && dev->of_node) {
+ if (IS_ENABLED(CONFIG_OF) && to_of_node(fwnode)) {
struct device_node *node;
- node = of_get_next_available_child(dev->of_node, to_of_node(child));
+ node = of_get_next_available_child(to_of_node(fwnode),
+ to_of_node(child));
if (node)
return &node->fwnode;
} else if (IS_ENABLED(CONFIG_ACPI)) {
- return acpi_get_next_subnode(dev, child);
+ return acpi_get_next_subnode(fwnode, child);
}
return NULL;
}
+EXPORT_SYMBOL_GPL(fwnode_get_next_child_node);
+
+/**
+ * device_get_next_child_node - Return the next child node handle for a device
+ * @dev: Device to find the next child node for.
+ * @child: Handle to one of the device's child nodes or a null handle.
+ */
+struct fwnode_handle *device_get_next_child_node(struct device *dev,
+ struct fwnode_handle *child)
+{
+ return fwnode_get_next_child_node(dev_fwnode(dev), child);
+}
EXPORT_SYMBOL_GPL(device_get_next_child_node);
/**
@@ -1016,3 +1029,14 @@ void *device_get_mac_address(struct device *dev, char *addr, int alen)
return device_get_mac_addr(dev, "address", addr, alen);
}
EXPORT_SYMBOL(device_get_mac_address);
+
+const char *fwnode_get_name(struct fwnode_handle *fwnode)
+{
+ if (is_of_node(fwnode))
+ return of_node_full_name(to_of_node(fwnode));
+ else if (is_acpi_node(fwnode))
+ return acpi_dev_name(to_acpi_device_node(fwnode));
+
+ return NULL;
+}
+EXPORT_SYMBOL(fwnode_get_name);
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index d5bf9fa..9d1571e 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -21,6 +21,7 @@
#include <linux/pinctrl/pinctrl.h>
#include <linux/pinctrl/pinconf.h>
#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/acpi.h>
#include <linux/of.h>
#include "core.h"
#include "pinconf.h"
@@ -148,8 +149,8 @@ void pinconf_generic_dump_config(struct pinctrl_dev *pctldev,
EXPORT_SYMBOL_GPL(pinconf_generic_dump_config);
#endif
-#ifdef CONFIG_OF
-static const struct pinconf_generic_params dt_params[] = {
+#if defined(CONFIG_OF) || defined(CONFIG_ACPI)
+static const struct pinconf_generic_params fw_params[] = {
{ "bias-bus-hold", PIN_CONFIG_BIAS_BUS_HOLD, 0 },
{ "bias-disable", PIN_CONFIG_BIAS_DISABLE, 0 },
{ "bias-high-impedance", PIN_CONFIG_BIAS_HIGH_IMPEDANCE, 0 },
@@ -175,22 +176,22 @@ static const struct pinconf_generic_params dt_params[] = {
};
/**
- * parse_dt_cfg() - Parse DT pinconf parameters
- * @np: DT node
+ * parse_fwnode_cfg() - Parse FW pinconf parameters
+ * @fw: FW node
* @params: Array of describing generic parameters
* @count: Number of entries in @params
* @cfg: Array of parsed config options
* @ncfg: Number of entries in @cfg
*
- * Parse the config options described in @params from @np and puts the result
+ * Parse the config options described in @params from @fw and puts the result
* in @cfg. @cfg does not need to be empty, entries are added beggining at
* @ncfg. @ncfg is updated to reflect the number of entries after parsing. @cfg
* needs to have enough memory allocated to hold all possible entries.
*/
-static void parse_dt_cfg(struct device_node *np,
- const struct pinconf_generic_params *params,
- unsigned int count, unsigned long *cfg,
- unsigned int *ncfg)
+static void parse_fwnode_cfg(struct fwnode_handle *fwnode,
+ const struct pinconf_generic_params *params,
+ unsigned int count, unsigned long *cfg,
+ unsigned int *ncfg)
{
int i;
@@ -199,7 +200,7 @@ static void parse_dt_cfg(struct device_node *np,
int ret;
const struct pinconf_generic_params *par = ¶ms[i];
- ret = of_property_read_u32(np, par->property, &val);
+ ret = fwnode_property_read_u32(fwnode, par->property, &val);
/* property not found */
if (ret == -EINVAL)
@@ -216,38 +217,38 @@ static void parse_dt_cfg(struct device_node *np,
}
/**
- * pinconf_generic_parse_dt_config()
+ * pinconf_generic_parse_fwnode_config()
* parse the config properties into generic pinconfig values.
- * @np: node containing the pinconfig properties
+ * @fw: node containing the pinconfig properties
* @configs: array with nconfigs entries containing the generic pinconf values
* must be freed when no longer necessary.
* @nconfigs: umber of configurations
*/
-int pinconf_generic_parse_dt_config(struct device_node *np,
- struct pinctrl_dev *pctldev,
- unsigned long **configs,
- unsigned int *nconfigs)
+static int pinconf_generic_parse_fwnode_config(struct fwnode_handle *fwnode,
+ struct pinctrl_dev *pctldev,
+ unsigned long **configs,
+ unsigned int *nconfigs)
{
unsigned long *cfg;
unsigned int max_cfg, ncfg = 0;
int ret;
- if (!np)
+ if (!fwnode)
return -EINVAL;
/* allocate a temporary array big enough to hold one of each option */
- max_cfg = ARRAY_SIZE(dt_params);
+ max_cfg = ARRAY_SIZE(fw_params);
if (pctldev)
max_cfg += pctldev->desc->num_custom_params;
cfg = kcalloc(max_cfg, sizeof(*cfg), GFP_KERNEL);
if (!cfg)
return -ENOMEM;
- parse_dt_cfg(np, dt_params, ARRAY_SIZE(dt_params), cfg, &ncfg);
+ parse_fwnode_cfg(fwnode, fw_params, ARRAY_SIZE(fw_params), cfg, &ncfg);
if (pctldev && pctldev->desc->num_custom_params &&
pctldev->desc->custom_params)
- parse_dt_cfg(np, pctldev->desc->custom_params,
- pctldev->desc->num_custom_params, cfg, &ncfg);
+ parse_fwnode_cfg(fwnode, pctldev->desc->custom_params,
+ pctldev->desc->num_custom_params, cfg, &ncfg);
ret = 0;
@@ -275,24 +276,23 @@ out:
return ret;
}
-int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
- struct device_node *np, struct pinctrl_map **map,
+static int pinconf_generic_fwnode_subnode_to_map(struct pinctrl_dev *pctldev,
+ struct fwnode_handle *fwnode, struct pinctrl_map **map,
unsigned *reserved_maps, unsigned *num_maps,
enum pinctrl_map_type type)
{
- int ret;
+ int ret, i;
const char *function;
struct device *dev = pctldev->dev;
unsigned long *configs = NULL;
unsigned num_configs = 0;
unsigned reserve, strings_count;
- struct property *prop;
- const char *group;
+ const char **groups;
const char *subnode_target_type = "pins";
- ret = of_property_count_strings(np, "pins");
+ ret = fwnode_property_read_string_array(fwnode, "pins", NULL, 0);
if (ret < 0) {
- ret = of_property_count_strings(np, "groups");
+ ret = fwnode_property_read_string_array(fwnode, "groups", NULL, 0);
if (ret < 0)
/* skip this node; may contain config child nodes */
return 0;
@@ -305,20 +305,20 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
}
strings_count = ret;
- ret = of_property_read_string(np, "function", &function);
+ ret = fwnode_property_read_string(fwnode, "function", &function);
if (ret < 0) {
/* EINVAL=missing, which is fine since it's optional */
if (ret != -EINVAL)
dev_err(dev, "%s: could not parse property function\n",
- of_node_full_name(np));
+ fwnode_get_name(fwnode));
function = NULL;
}
- ret = pinconf_generic_parse_dt_config(np, pctldev, &configs,
- &num_configs);
+ ret = pinconf_generic_parse_fwnode_config(fwnode, pctldev, &configs,
+ &num_configs);
if (ret < 0) {
dev_err(dev, "%s: could not parse node property\n",
- of_node_full_name(np));
+ fwnode_get_name(fwnode));
return ret;
}
@@ -333,52 +333,64 @@ int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
ret = pinctrl_utils_reserve_map(pctldev, map, reserved_maps,
num_maps, reserve);
if (ret < 0)
- goto exit;
+ goto exit_free_configs;
+
+ groups = kcalloc(strings_count, sizeof(*groups), GFP_KERNEL);
+ if (!groups) {
+ ret = -ENOMEM;
+ goto exit_free_configs;
+ }
+
+ ret = fwnode_property_read_string_array(fwnode, subnode_target_type,
+ groups, strings_count);
+ if (ret < 0)
+ goto exit_free_groups;
- of_property_for_each_string(np, subnode_target_type, prop, group) {
+ for (i = 0; i < strings_count; i++) {
if (function) {
ret = pinctrl_utils_add_map_mux(pctldev, map,
- reserved_maps, num_maps, group,
+ reserved_maps, num_maps, groups[i],
function);
if (ret < 0)
- goto exit;
+ goto exit_free_groups;
}
if (num_configs) {
ret = pinctrl_utils_add_map_configs(pctldev, map,
- reserved_maps, num_maps, group, configs,
- num_configs, type);
+ reserved_maps, num_maps, groups[i],
+ configs, num_configs, type);
if (ret < 0)
- goto exit;
+ goto exit_free_groups;
}
}
ret = 0;
-exit:
+exit_free_groups:
+ kfree(groups);
+exit_free_configs:
kfree(configs);
return ret;
}
-EXPORT_SYMBOL_GPL(pinconf_generic_dt_subnode_to_map);
-int pinconf_generic_dt_node_to_map(struct pinctrl_dev *pctldev,
- struct device_node *np_config, struct pinctrl_map **map,
+int pinconf_generic_fwnode_to_map(struct pinctrl_dev *pctldev,
+ struct fwnode_handle *fwnode, struct pinctrl_map **map,
unsigned *num_maps, enum pinctrl_map_type type)
{
unsigned reserved_maps;
- struct device_node *np;
+ struct fwnode_handle *fw;
int ret;
reserved_maps = 0;
*map = NULL;
*num_maps = 0;
- ret = pinconf_generic_dt_subnode_to_map(pctldev, np_config, map,
+ ret = pinconf_generic_fwnode_subnode_to_map(pctldev, fwnode, map,
&reserved_maps, num_maps, type);
if (ret < 0)
goto exit;
- for_each_child_of_node(np_config, np) {
- ret = pinconf_generic_dt_subnode_to_map(pctldev, np, map,
+ fwnode_for_each_child_node(fwnode, fw) {
+ ret = pinconf_generic_fwnode_subnode_to_map(pctldev, fw, map,
&reserved_maps, num_maps, type);
if (ret < 0)
goto exit;
@@ -389,6 +401,17 @@ exit:
pinctrl_utils_free_map(pctldev, *map, *num_maps);
return ret;
}
-EXPORT_SYMBOL_GPL(pinconf_generic_dt_node_to_map);
+EXPORT_SYMBOL_GPL(pinconf_generic_fwnode_to_map);
+
+#endif
+#ifdef CONFIG_OF
+inline int pinconf_generic_parse_dt_config(struct device_node *np,
+ struct pinctrl_dev *pctldev,
+ unsigned long **configs,
+ unsigned int *nconfigs)
+{
+ return pinconf_generic_parse_fwnode_config(&np->fwnode, pctldev,
+ configs, nconfigs);
+}
#endif
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 06ed7e5..78a1050 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -876,7 +876,7 @@ int acpi_node_prop_read(struct fwnode_handle *fwnode, const char *propname,
int acpi_dev_prop_read(struct acpi_device *adev, const char *propname,
enum dev_prop_type proptype, void *val, size_t nval);
-struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
+struct fwnode_handle *acpi_get_next_subnode(struct fwnode_handle *fwnode,
struct fwnode_handle *subnode);
struct acpi_probe_entry;
@@ -986,7 +986,7 @@ static inline int acpi_dev_prop_read(struct acpi_device *adev,
return -ENXIO;
}
-static inline struct fwnode_handle *acpi_get_next_subnode(struct device *dev,
+static inline struct fwnode_handle *acpi_get_next_subnode(struct fwnode_handle *fwnode,
struct fwnode_handle *subnode)
{
return NULL;
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index d921afd..587daa1 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -155,8 +155,7 @@ static inline unsigned long pinconf_to_config_packed(enum pin_config_param param
return PIN_CONF_PACKED(param, argument);
}
-#ifdef CONFIG_OF
-
+#if defined(CONFIG_OF) || defined(CONFIG_ACPI)
#include <linux/device.h>
#include <linux/pinctrl/machine.h>
struct pinctrl_dev;
@@ -168,28 +167,28 @@ struct pinconf_generic_params {
u32 default_value;
};
-int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
- struct device_node *np, struct pinctrl_map **map,
- unsigned *reserved_maps, unsigned *num_maps,
- enum pinctrl_map_type type);
-int pinconf_generic_dt_node_to_map(struct pinctrl_dev *pctldev,
- struct device_node *np_config, struct pinctrl_map **map,
+int pinconf_generic_fwnode_to_map(struct pinctrl_dev *pctldev,
+ struct fwnode_handle *fwnode, struct pinctrl_map **map,
unsigned *num_maps, enum pinctrl_map_type type);
+#endif
+
+#ifdef CONFIG_OF
+#include <linux/of.h>
static inline int pinconf_generic_dt_node_to_map_group(
struct pinctrl_dev *pctldev, struct device_node *np_config,
struct pinctrl_map **map, unsigned *num_maps)
{
- return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps,
- PIN_MAP_TYPE_CONFIGS_GROUP);
+ return pinconf_generic_fwnode_to_map(pctldev, &np_config->fwnode, map,
+ num_maps, PIN_MAP_TYPE_CONFIGS_GROUP);
}
static inline int pinconf_generic_dt_node_to_map_pin(
struct pinctrl_dev *pctldev, struct device_node *np_config,
struct pinctrl_map **map, unsigned *num_maps)
{
- return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps,
- PIN_MAP_TYPE_CONFIGS_PIN);
+ return pinconf_generic_fwnode_to_map(pctldev, &np_config->fwnode, map,
+ num_maps, PIN_MAP_TYPE_CONFIGS_PIN);
}
static inline int pinconf_generic_dt_node_to_map_all(
@@ -200,8 +199,8 @@ static inline int pinconf_generic_dt_node_to_map_all(
* passing the type as PIN_MAP_TYPE_INVALID causes the underlying parser
* to infer the map type from the DT properties used.
*/
- return pinconf_generic_dt_node_to_map(pctldev, np_config, map, num_maps,
- PIN_MAP_TYPE_INVALID);
+ return pinconf_generic_fwnode_to_map(pctldev, &np_config->fwnode, map,
+ num_maps, PIN_MAP_TYPE_INVALID);
}
#endif
diff --git a/include/linux/property.h b/include/linux/property.h
index b51fcd3..b0868d0 100644
--- a/include/linux/property.h
+++ b/include/linux/property.h
@@ -70,9 +70,16 @@ int fwnode_property_read_string(struct fwnode_handle *fwnode,
int fwnode_property_match_string(struct fwnode_handle *fwnode,
const char *propname, const char *string);
+struct fwnode_handle *fwnode_get_next_child_node(struct fwnode_handle *fwnode,
+ struct fwnode_handle *child);
+
struct fwnode_handle *device_get_next_child_node(struct device *dev,
struct fwnode_handle *child);
+#define fwnode_for_each_child_node(fwnode, child) \
+ for (child = fwnode_get_next_child_node(fwnode, NULL); child; \
+ child = fwnode_get_next_child_node(fwnode, child))
+
#define device_for_each_child_node(dev, child) \
for (child = device_get_next_child_node(dev, NULL); child; \
child = device_get_next_child_node(dev, child))
@@ -259,4 +266,6 @@ int device_get_phy_mode(struct device *dev);
void *device_get_mac_address(struct device *dev, char *addr, int alen);
+const char *fwnode_get_name(struct fwnode_handle *fwnode);
+
#endif /* _LINUX_PROPERTY_H_ */
--
1.9.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [RFC PATCH 2/4] pinctrl: pinconf-generic: Add ACPI support
2016-03-31 11:44 ` [RFC PATCH 2/4] pinctrl: pinconf-generic: Add ACPI support Irina Tirdea
@ 2016-04-01 14:05 ` Andy Shevchenko
2016-04-04 13:03 ` Tirdea, Irina
0 siblings, 1 reply; 41+ messages in thread
From: Andy Shevchenko @ 2016-04-01 14:05 UTC (permalink / raw)
To: Irina Tirdea, Rafael J. Wysocki, Len Brown, Mika Westerberg,
Linus Walleij, linux-gpio, linux-acpi
Cc: Rob Herring, Heikki Krogerus, Octavian Purdila, Cristina Ciocan,
devicetree, linux-kernel
On Thu, 2016-03-31 at 14:44 +0300, Irina Tirdea wrote:
> Add ACPI support for the generic device tree properties.
> Convert the pinconf generic code to handle both ACPI and
> device tree by using the fwnode_property API. Also include
> renaming device tree references in names of functions and
> structures from 'dt' to 'fwnode'.
This is looks good to me, though few style / minor comments below.
> --- a/drivers/pinctrl/pinconf-generic.c
> +++ b/drivers/pinctrl/pinconf-generic.c
>
> @@ -175,22 +176,22 @@ static const struct pinconf_generic_params
> dt_params[] = {
> };
>
> /**
> - * parse_dt_cfg() - Parse DT pinconf parameters
> - * @np: DT node
> + * parse_fwnode_cfg() - Parse FW pinconf parameters
> + * @fw: FW node
Here and below it should be @fwnode.
> -int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
> - struct device_node *np, struct pinctrl_map **map,
> +static int pinconf_generic_fwnode_subnode_to_map(struct pinctrl_dev
> *pctldev,
> + struct fwnode_handle *fwnode, struct pinctrl_map
> **map,
> unsigned *reserved_maps, unsigned *num_maps,
> enum pinctrl_map_type type)
> {
> - int ret;
> + int ret, i;
Since you change this line anyway, perhaps move it to be last in the
definition block?
> const char *function;
> struct device *dev = pctldev->dev;
> unsigned long *configs = NULL;
> unsigned num_configs = 0;
> unsigned reserve, strings_count;
> - struct property *prop;
> - const char *group;
> + const char **groups;
> const char *subnode_target_type = "pins";
...to here.
> +#ifdef CONFIG_OF
> +inline int pinconf_generic_parse_dt_config(struct device_node *np,
> + struct pinctrl_dev
> *pctldev,
> + unsigned long **configs,
> + unsigned int *nconfigs)
> +{
> + return pinconf_generic_parse_fwnode_config(&np->fwnode,
> pctldev,
> + configs,
> nconfigs);
> +}
I didn't see any user of this.
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [RFC PATCH 2/4] pinctrl: pinconf-generic: Add ACPI support
2016-04-01 14:05 ` Andy Shevchenko
@ 2016-04-04 13:03 ` Tirdea, Irina
0 siblings, 0 replies; 41+ messages in thread
From: Tirdea, Irina @ 2016-04-04 13:03 UTC (permalink / raw)
To: Andy Shevchenko, Rafael J. Wysocki, Len Brown, Mika Westerberg,
Linus Walleij, linux-gpio, linux-acpi
Cc: Rob Herring, Heikki Krogerus, Purdila, Octavian, Ciocan,
Cristina, devicetree, linux-kernel
> -----Original Message-----
> From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> Sent: 01 April, 2016 17:05
> To: Tirdea, Irina; Rafael J. Wysocki; Len Brown; Mika Westerberg; Linus Walleij; linux-gpio@vger.kernel.org; linux-
> acpi@vger.kernel.org
> Cc: Rob Herring; Heikki Krogerus; Purdila, Octavian; Ciocan, Cristina; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH 2/4] pinctrl: pinconf-generic: Add ACPI support
>
> On Thu, 2016-03-31 at 14:44 +0300, Irina Tirdea wrote:
> > Add ACPI support for the generic device tree properties.
> > Convert the pinconf generic code to handle both ACPI and
> > device tree by using the fwnode_property API. Also include
> > renaming device tree references in names of functions and
> > structures from 'dt' to 'fwnode'.
>
> This is looks good to me, though few style / minor comments below.
Thanks for the review, Andy!
>
> > --- a/drivers/pinctrl/pinconf-generic.c
> > +++ b/drivers/pinctrl/pinconf-generic.c
> >
>
> > @@ -175,22 +176,22 @@ static const struct pinconf_generic_params
> > dt_params[] = {
> > };
> >
> > /**
> > - * parse_dt_cfg() - Parse DT pinconf parameters
> > - * @np: DT node
> > + * parse_fwnode_cfg() - Parse FW pinconf parameters
> > + * @fw: FW node
>
> Here and below it should be @fwnode.
OK.
>
> > -int pinconf_generic_dt_subnode_to_map(struct pinctrl_dev *pctldev,
> > - struct device_node *np, struct pinctrl_map **map,
> > +static int pinconf_generic_fwnode_subnode_to_map(struct pinctrl_dev
> > *pctldev,
> > + struct fwnode_handle *fwnode, struct pinctrl_map
> > **map,
> > unsigned *reserved_maps, unsigned *num_maps,
> > enum pinctrl_map_type type)
> > {
> > - int ret;
> > + int ret, i;
>
> Since you change this line anyway, perhaps move it to be last in the
> definition block?
>
> > const char *function;
> > struct device *dev = pctldev->dev;
> > unsigned long *configs = NULL;
> > unsigned num_configs = 0;
> > unsigned reserve, strings_count;
> > - struct property *prop;
> > - const char *group;
> > + const char **groups;
> > const char *subnode_target_type = "pins";
>
> ...to here.
>
Sure.
> > +#ifdef CONFIG_OF
> > +inline int pinconf_generic_parse_dt_config(struct device_node *np,
> > + struct pinctrl_dev
> > *pctldev,
> > + unsigned long **configs,
> > + unsigned int *nconfigs)
> > +{
> > + return pinconf_generic_parse_fwnode_config(&np->fwnode,
> > pctldev,
> > + configs,
> > nconfigs);
> > +}
>
> I didn't see any user of this.
pinconf_generic_parse_dt_config is defined in drivers/pinctrl/pinconf.h
and is used by various pinctrl drivers(e.g.: drivers/pinctrl/nomadik/pinctrl-abx500.c,
drivers/pinctrl/pinctrl-tz1090-pdc.c, drivers/pinctrl/pinctrl-tz1090.c). Taking a closer
look at its prototype, I should match it here exactly (by removing inline).
^ permalink raw reply [flat|nested] 41+ messages in thread
* [RFC PATCH 3/4] pinctrl: Add ACPI support
2016-03-31 11:44 [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Irina Tirdea
2016-03-31 11:44 ` [RFC PATCH 1/4] pinctrl: Rename pinctrl_utils_dt_free_map to pinctrl_utils_free_map Irina Tirdea
2016-03-31 11:44 ` [RFC PATCH 2/4] pinctrl: pinconf-generic: Add ACPI support Irina Tirdea
@ 2016-03-31 11:44 ` Irina Tirdea
2016-04-01 14:14 ` Andy Shevchenko
2016-04-04 13:37 ` Mika Westerberg
2016-03-31 11:44 ` [RFC PATCH 4/4] pinctrl: Parse GpioInt/GpioIo resources Irina Tirdea
` (2 subsequent siblings)
5 siblings, 2 replies; 41+ messages in thread
From: Irina Tirdea @ 2016-03-31 11:44 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
linux-gpio, linux-acpi
Cc: Rob Herring, Heikki Krogerus, Andy Shevchenko, Octavian Purdila,
Cristina Ciocan, devicetree, linux-kernel, Irina Tirdea
Add ACPI support for pin controller properties. These are
based on ACPI _DSD properties and follow the device tree
model based on states and node configurations. The states
are defined as _DSD properties and configuration nodes
are defined using the _DSD Hierarchical Properties Extension.
A configuration node supports the generic device tree properties.
The implementation is based on device tree code from devicetree.c.
Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
Documentation/acpi/pinctrl-properties.txt | 274 +++++++++++++++++++++++++
drivers/pinctrl/Makefile | 1 +
drivers/pinctrl/acpi.c | 322 ++++++++++++++++++++++++++++++
drivers/pinctrl/acpi.h | 32 +++
drivers/pinctrl/core.c | 26 +++
drivers/pinctrl/core.h | 2 +
6 files changed, 657 insertions(+)
create mode 100644 Documentation/acpi/pinctrl-properties.txt
create mode 100644 drivers/pinctrl/acpi.c
create mode 100644 drivers/pinctrl/acpi.h
diff --git a/Documentation/acpi/pinctrl-properties.txt b/Documentation/acpi/pinctrl-properties.txt
new file mode 100644
index 0000000..e93aaaf
--- /dev/null
+++ b/Documentation/acpi/pinctrl-properties.txt
@@ -0,0 +1,274 @@
+= _DSD Device Properties related to pin controllers =
+
+== Introduction ==
+
+This document is an extension of the pin control subsystem in Linux [1]
+and provides a way to describe pin controller properties in ACPI. It is
+based on the Device Specific Data (_DSD) configuration object [2] that
+was introduced in ACPI 5.1.
+
+Pin controllers are hardware modules that control pins by allowing pin
+multiplexing and configuration. Pin multiplexing allows using the same
+physical pins for multiple functions; for example, one pin or group of pins
+may be used for the I2C bus, SPI bus or as general-purpose GPIO pin. Pin
+configuration allows setting various properties such as pull-up/down,
+tri-state, drive-strength, etc.
+
+Hardware modules whose signals are affected by pin configuration are
+designated client devices. For a client device to operate correctly,
+certain pin controllers must set up certain specific pin configurations.
+Some client devices need a single static pin configuration, e.g. set up
+during initialization. Others need to reconfigure pins at run-time,
+for example to tri-state pins when the device is inactive. Hence, each
+client device can define a set of named states. Each named state is
+mapped to a pin controller configuration that describes the pin multiplexing
+or configuration for that state.
+
+In ACPI, each pin controller and each client device is represented as an
+ACPI device, just like any other hardware module. The pin controller
+properties are defined using _DSD properties [2] under these devices.
+The named states are defined using Device Properties UUID [3] under the
+ACPI client device. The configuration nodes are defined using Hierarchical
+Properties Extension UUID [4] and are split between the ACPI client device
+and the pin controller device. The configuration nodes contain properties
+that describe pin multiplexing or configuration that very similar to the
+ones used for device tree [5].
+
+== Example ==
+
+For example, let's consider an accelerometer connected to the I2C bus on
+a platform with a Baytrail pin controller. The accelerometer uses 2 GPIO
+pins for I2C (SDA, SCL) and one GPIO pin for interrupt.
+
+The name for the pins, groups and functions used are the ones defined in the
+pin controller driver, in the same way as it is done for device tree [5].
+
+For the I2C pins, the pin controller driver defines one group called
+"i2c5_grp" that can be multiplexed with functions "i2c" or "gpio".
+In our case, we need to select function "i2c" for group "i2c5_grp" in
+the ACPI description.
+
+For the GPIO pin, the pin controller driver defines the name "GPIO_S5[00]"
+for the pin with index 0 that we use. We need to configure this pin to
+pull-down with pull strength of 10000 Ohms. We might also want to disable
+the bias for the GPIO interrupt pin when entering sleep.
+
+Here is an ASL example for this device:
+
+ // Pin controller device
+ Scope (_SB.GPO0)
+ {
+ Name (MUX0, Package()
+ {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package()
+ {
+ Package (2) {"function", "i2c"},
+ Package (2) {"groups", Package () {"i2c5_grp"}},
+ }
+ })
+
+ Name (CFG0, Package()
+ {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package()
+ {
+ Package (2) {"pins", Package () {"GPIO_S5[00]"}},
+ Package (2) {"bias-pull-down", 10000},
+ }
+ })
+
+ Name (CFG1, Package()
+ {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package()
+ {
+ Package (2) {"pins", Package () {"GPIO_S5[00]"}},
+ Package (2) {"bias-disable", 0},
+ }
+ })
+ }
+
+ // Accelerometer device with default pinmux and pinconfig for i2c and
+ // GPIO pins
+ Scope (_SB.I2C0)
+ {
+ Device (ACL0)
+ {
+ Name (_HID, ...)
+
+ Method (_CRS, 0, Serialized)
+ {
+ Name (RBUF, ResourceTemplate ()
+ {
+ I2cSerialBus (...)
+ GpioInt (Edge, ActiveHigh, Exclusive, PullDown, 0x0000,
+ "\\_SB.GPO0", 0x00, ResourceConsumer, , ) { 0 }
+ })
+ Return (RBUF)
+ }
+
+ Name (_DSD, Package ()
+ {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package ()
+ {
+ Package () {"pinctrl-names", Package() {"default", "sleep"}},
+ Package ()
+ {
+ "pinctrl-0",
+ Package()
+ {
+ "accel-default-mux-i2c",
+ "accel-default-cfg-int",
+ }
+ },
+ Package ()
+ {
+ "pinctrl-1",
+ Package()
+ {
+ "accel-sleep-cfg-int",
+ }
+ },
+
+ },
+ ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+ Package ()
+ {
+ Package (2) {"accel-default-mux-i2c", "\\_SB.GPO0.MUX0"},
+ Package (2) {"accel-default-cfg-int", "\\_SB.GPO0.CFG0"},
+ Package (2) {"accel-sleep-cfg-int", "\\_SB.GPO0.CFG1"},
+ },
+ })
+ }
+ }
+
+In the ASL excerpt, the accelerometer device has 2 states:
+ - a default state with 2 pin configurations:
+ - a pin multiplexing node for the i2c pins that sets function "i2c"
+ for the "i2c5_grp" pin group
+ - a pin configuration node for the GPIO interrupt pin that pull down
+ the "GPIO_S5[00]" pin and sets a pull strength of 10000 Ohms
+ - a sleep state with 1 pin configuration:
+ - a pin configuration node for pin "GPIO_S5[00]" that disables pin
+ bias
+
+== _DSD pinctrl properties format ==
+
+=== Pin controller client device states ===
+
+The pinctrl states are defined under the device node they apply to.
+The format of the pinctrl states is:
+
+ Name (_DSD, Package ()
+ {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package ()
+ {
+ Package () {"pinctrl-names", Package() {statename0, statename1, ...}},
+ Package () {"pinctrl-0", Package() {cfgname0, cfgname1, ...}},
+ Package () {"pinctrl-1", Package() {cfgname2, cfgname3, ...}},
+ }
+ }
+
+ statename - name of the pinctrl device state (e.g.: default, sleep, etc.).
+ These names are associated with the lists of configurations
+ defined below: statename0 defines the name for configuration
+ property "pinctrl-0", statename1 defines the name for
+ configuration property "pinctrl-1", etc.
+ cfgname - name for the configuration data-only subnode.
+
+=== Pin controller configuration nodes ===
+
+The configuration data-only subnodes are defined using the Hierarchical
+Properties Extension UUID [4]. Their definition is split between the device
+node and the pin controller node. The format for these subnodes is:
+
+ Scope (DEV0)
+ {
+ Name (_DSD, Package ()
+ {
+ ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
+ Package ()
+ {
+ Package (2) {cfgname0, "\\GPO0.DNP0"},
+ Package (2) {cfgname1, "\\GPO0.DNP1"},
+ },
+ })
+ }
+
+ Scope (GPO0)
+ {
+ Name (DPN0, Package()
+ {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package() {...}
+ })
+ Name (DPN1, Package()
+ {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package() {...}
+ })
+ }
+
+Each DPN data subnode is a regular _DSD node that uses Device Properties
+UUID [3]. There are 2 types of subnodes, depending on the properties it
+contains: pin multiplexing nodes and pin configuration nodes.
+
+==== Pin multiplexing nodes ====
+
+The pin multiplexing nodes must contain a property named "function" and
+define a mux function to be applied to a list of pin groups. The properties
+supported by this node are the same as for device tree [5]. The name for the
+pins, groups and functions used are the ones defined in the pin controller
+driver, in the same way as it is done for device tree [5]. The format for
+this data subnode is:
+
+ Name (DPN0, Package()
+ {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package()
+ {
+ Package (2) {"function", functioname},
+ Package (2) {"groups", Package () {groupname1, groupname2, ...}},
+ }
+ })
+
+ functioname - the pinmux function to select.
+ groups - the list of groups to select with this function
+
+==== Pin configuration nodes ====
+
+The pin configuration nodes do not contain a property named "function".
+They must contain a property named "group" or "pins". They will also
+contain one or more configuration properties like bias-pull-up,
+drive-open-drain, etc. The properties supported by this node are the
+same as for device tree. Standard pinctrl properties are defined in the
+device tree documentation [5] and in <include/linux/pinctrl/pinconf-generic.h>.
+Pinctrl drivers may also define their own custom properties. The name for the
+pins/groups used are the ones defined in the pin controller driver, in the
+same way as it is done for device tree [5]. The format for the data subnode is:
+
+ Name (DPN0, Package()
+ {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package()
+ {
+ Package (2) {"pins", Package () {pin1, pin2, ...}},
+ Package (2) {configname1, configval1},
+ Package (2) {configname2, configval2},
+ }
+ })
+
+ pins - list of pins that properties in the node apply to
+ configname - name of the pin configuration property
+ configval - value of the pin configuration property
+
+== References ==
+
+[1] Documentation/pinctrl.txt
+[2] http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel-1_1.htm
+[3] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
+[4] http://www.uefi.org/sites/default/files/resources/_DSD-hierarchical-data-extension-UUID-v1.pdf
+[5] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index e4bc115..12d3af6 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -6,6 +6,7 @@ obj-y += core.o pinctrl-utils.o
obj-$(CONFIG_PINMUX) += pinmux.o
obj-$(CONFIG_PINCONF) += pinconf.o
obj-$(CONFIG_OF) += devicetree.o
+obj-$(CONFIG_ACPI) += acpi.o
obj-$(CONFIG_GENERIC_PINCONF) += pinconf-generic.o
obj-$(CONFIG_PINCTRL_ADI2) += pinctrl-adi2.o
obj-$(CONFIG_PINCTRL_AS3722) += pinctrl-as3722.o
diff --git a/drivers/pinctrl/acpi.c b/drivers/pinctrl/acpi.c
new file mode 100644
index 0000000..bed1d88
--- /dev/null
+++ b/drivers/pinctrl/acpi.c
@@ -0,0 +1,322 @@
+/*
+ * ACPI integration for the pin control subsystem
+ *
+ * Copyright (c) 2016, Intel Corporation.
+ *
+ * Derived from:
+ * devicetree.c - Copyright (C) 2012 NVIDIA CORPORATION
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/acpi.h>
+#include <linux/device.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf-generic.h>
+
+#include "acpi.h"
+#include "core.h"
+#include "pinconf.h"
+#include "pinctrl-utils.h"
+
+/**
+ * struct pinctrl_acpi_map - mapping table chunk parsed from ACPI
+ * @node: list node for struct pinctrl's @fw_maps field
+ * @pctldev: the pin controller that allocated this struct, and will free it
+ * @maps: the mapping table entries
+ */
+struct pinctrl_acpi_map {
+ struct list_head node;
+ struct pinctrl_dev *pctldev;
+ struct pinctrl_map *map;
+ unsigned num_maps;
+};
+
+static void acpi_maps_list_dh(acpi_handle handle, void *data)
+{
+ /* The address of this function is used as a key. */
+}
+
+static struct list_head *acpi_get_maps(struct device *dev)
+{
+ acpi_handle handle = ACPI_HANDLE(dev);
+ struct list_head *maps;
+ acpi_status status;
+
+ status = acpi_get_data(handle, acpi_maps_list_dh, (void **)&maps);
+ if (ACPI_FAILURE(status))
+ return NULL;
+
+ return maps;
+}
+
+static void acpi_free_maps(struct device *dev, struct list_head *maps)
+{
+ acpi_handle handle = ACPI_HANDLE(dev);
+
+ acpi_detach_data(handle, acpi_maps_list_dh);
+ kfree(maps);
+}
+
+static int acpi_init_maps(struct device *dev)
+{
+ acpi_handle handle = ACPI_HANDLE(dev);
+ struct list_head *maps;
+ acpi_status status;
+
+ maps = kzalloc(sizeof(*maps), GFP_KERNEL);
+ if (!maps)
+ return -ENOMEM;
+
+ INIT_LIST_HEAD(maps);
+
+ status = acpi_attach_data(handle, acpi_maps_list_dh, maps);
+ if (ACPI_FAILURE(status))
+ return -EINVAL;
+
+ return 0;
+}
+
+void pinctrl_acpi_free_maps(struct pinctrl *p)
+{
+ struct pinctrl_acpi_map *map, *_map;
+ struct list_head *maps;
+
+ maps = acpi_get_maps(p->dev);
+ if (!maps)
+ goto out;
+
+ list_for_each_entry_safe(map, _map, maps, node) {
+ pinctrl_unregister_map(map->map);
+ list_del(&map->node);
+ pinctrl_utils_free_map(map->pctldev, map->map, map->num_maps);
+ kfree(map);
+ }
+
+ acpi_free_maps(p->dev, maps);
+out:
+ acpi_bus_put_acpi_device(ACPI_COMPANION(p->dev));
+}
+
+static int acpi_remember_or_free_map(struct pinctrl *p, const char *statename,
+ struct pinctrl_dev *pctldev,
+ struct pinctrl_map *map, unsigned num_maps)
+{
+ struct pinctrl_acpi_map *acpi_map;
+ struct list_head *acpi_maps;
+ unsigned int i;
+
+ acpi_maps = acpi_get_maps(p->dev);
+ if (!acpi_maps)
+ return -EINVAL;
+
+ /* Initialize common mapping table entry fields */
+ for (i = 0; i < num_maps; i++) {
+ map[i].dev_name = dev_name(p->dev);
+ map[i].name = statename;
+ if (pctldev)
+ map[i].ctrl_dev_name = dev_name(pctldev->dev);
+ }
+
+ /* Remember the converted mapping table entries */
+ acpi_map = kzalloc(sizeof(*acpi_map), GFP_KERNEL);
+ if (!acpi_map) {
+ pinctrl_utils_free_map(pctldev, map, num_maps);
+ return -ENOMEM;
+ }
+
+ acpi_map->pctldev = pctldev;
+ acpi_map->map = map;
+ acpi_map->num_maps = num_maps;
+ list_add_tail(&acpi_map->node, acpi_maps);
+
+ return pinctrl_register_map(map, num_maps, false);
+}
+
+static int acpi_remember_dummy_state(struct pinctrl *p, const char *statename)
+{
+ struct pinctrl_map *map;
+
+ map = kzalloc(sizeof(*map), GFP_KERNEL);
+ if (!map)
+ return -ENOMEM;
+
+ /* There is no pctldev for PIN_MAP_TYPE_DUMMY_STATE */
+ map->type = PIN_MAP_TYPE_DUMMY_STATE;
+
+ return acpi_remember_or_free_map(p, statename, NULL, map, 1);
+}
+
+static struct pinctrl_dev *acpi_find_pctldev(struct fwnode_handle *fw_config)
+{
+ struct acpi_buffer path = {ACPI_ALLOCATE_BUFFER, NULL};
+ acpi_handle pctrl_handle, cfg_handle;
+ struct acpi_data_node *dn;
+ acpi_status status;
+ int ret;
+
+ /*
+ * In ACPI, the pinctrl device is the parent of the configuration
+ * node. In the kernel internal representation, the device node is
+ * the parent of the configuration node. We need to extract the
+ * original path for the configuration node and search for its parent
+ * in the ACPI hierarchy.
+ */
+ dn = to_acpi_data_node(fw_config);
+ if (!dn)
+ return ERR_PTR(-EINVAL);
+
+ ret = acpi_get_name(dn->handle, ACPI_FULL_PATHNAME, &path);
+ if (ret)
+ return ERR_PTR(ret);
+
+ status = acpi_get_handle(NULL, (char *)path.pointer, &cfg_handle);
+ kfree(path.pointer);
+ if (ACPI_FAILURE(status))
+ return ERR_PTR(-EINVAL);
+
+ status = acpi_get_parent(cfg_handle, &pctrl_handle);
+ if (ACPI_FAILURE(status))
+ return ERR_PTR(-EINVAL);
+
+ return get_pinctrl_dev_from_acpi(pctrl_handle);
+}
+
+static int acpi_to_map_one_config(struct pinctrl *p, const char *statename,
+ struct fwnode_handle *fw_config)
+{
+ struct pinctrl_map *map;
+ struct pinctrl_dev *pctldev;
+ unsigned num_maps;
+ int ret;
+
+ /* Find the pin controller containing fw_config */
+ pctldev = acpi_find_pctldev(fw_config);
+ if (!pctldev)
+ return -ENODEV;
+ if (IS_ERR(pctldev))
+ return PTR_ERR(pctldev);
+
+ /* Parse ACPI node and generate mapping table entries */
+ ret = pinconf_generic_fwnode_to_map(pctldev, fw_config, &map, &num_maps,
+ PIN_MAP_TYPE_INVALID);
+ if (ret < 0)
+ return ret;
+
+ /* Stash the mapping table chunk away for later use */
+ return acpi_remember_or_free_map(p, statename, pctldev, map, num_maps);
+}
+
+static struct fwnode_handle *acpi_find_config_prop(struct device *dev,
+ char *propname)
+{
+ struct fwnode_handle *child;
+ struct acpi_data_node *dn;
+
+ /*
+ * Pinctrl configuration properties are described with ACPI data
+ * nodes using _DSD Hierarchical Properties Extension.
+ */
+ device_for_each_child_node(dev, child) {
+ dn = to_acpi_data_node(child);
+ if (!dn)
+ continue;
+ if (!strcmp(dn->name, propname))
+ break;
+ }
+
+ return child;
+}
+
+int pinctrl_acpi_to_map(struct pinctrl *p)
+{
+ const union acpi_object *prop, *statenames, *configs;
+ unsigned int state, nstates, nconfigs, config;
+ char *statename, *propname, *configname;
+ struct fwnode_handle *fw_prop;
+ struct acpi_device *adev;
+ int ret;
+
+ /* We may store pointers to property names within the node */
+ adev = acpi_bus_get_acpi_device(ACPI_HANDLE(p->dev));
+ if (!adev)
+ return -ENODEV;
+
+ /* Only allow named states (device must have prop 'pinctrl-names') */
+ ret = acpi_dev_get_property(adev, "pinctrl-names", ACPI_TYPE_PACKAGE,
+ &prop);
+ if (ret) {
+ acpi_bus_put_acpi_device(adev);
+ /* No pinctrl properties */
+ return 0;
+ }
+ statenames = prop->package.elements;
+ nstates = prop->package.count;
+
+ ret = acpi_init_maps(p->dev);
+ if (ret)
+ return ret;
+
+ /* For each defined state ID */
+ for (state = 0; state < nstates; state++) {
+ /* Get state name */
+ if (statenames[state].type != ACPI_TYPE_STRING) {
+ ret = -EINVAL;
+ goto err_free_map;
+ }
+ statename = statenames[state].string.pointer;
+
+ /* Retrieve the pinctrl-* property */
+ propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
+ ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_PACKAGE,
+ &prop);
+ kfree(propname);
+ if (ret)
+ break;
+ configs = prop->package.elements;
+ nconfigs = prop->package.count;
+
+ /* For every referenced pin configuration node in it */
+ for (config = 0; config < nconfigs; config++) {
+ if (configs[config].type != ACPI_TYPE_STRING) {
+ ret = -EINVAL;
+ goto err_free_map;
+ }
+ configname = configs[config].string.pointer;
+
+ /*
+ * Look up the pin configuration node as
+ * an ACPI data node in the device node.
+ */
+ fw_prop = acpi_find_config_prop(p->dev, configname);
+ if (!fw_prop) {
+ ret = -EINVAL;
+ goto err_free_map;
+ }
+
+ /* Parse the configuration node */
+ ret = acpi_to_map_one_config(p, statename, fw_prop);
+ if (ret < 0)
+ goto err_free_map;
+ }
+ /* No entries in ACPI? Generate a dummy state table entry */
+ if (!nconfigs) {
+ ret = acpi_remember_dummy_state(p, statename);
+ if (ret < 0)
+ goto err_free_map;
+ }
+ }
+
+ return 0;
+
+err_free_map:
+ pinctrl_acpi_free_maps(p);
+ return ret;
+}
diff --git a/drivers/pinctrl/acpi.h b/drivers/pinctrl/acpi.h
new file mode 100644
index 0000000..fb9f6ed
--- /dev/null
+++ b/drivers/pinctrl/acpi.h
@@ -0,0 +1,32 @@
+/*
+ * Internal interface to pinctrl ACPI integration
+ *
+ * Copyright (c) 2016, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#ifdef CONFIG_ACPI
+
+void pinctrl_acpi_free_maps(struct pinctrl *p);
+int pinctrl_acpi_to_map(struct pinctrl *p);
+
+#else
+
+static inline int pinctrl_acpi_to_map(struct pinctrl *p)
+{
+ return 0;
+}
+
+static inline void pinctrl_acpi_free_maps(struct pinctrl *p)
+{
+}
+
+#endif
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index f67a8b7..1bf3774 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -36,6 +36,7 @@
#include "devicetree.h"
#include "pinmux.h"
#include "pinconf.h"
+#include "acpi.h"
static bool pinctrl_dummy_state;
@@ -137,6 +138,23 @@ struct pinctrl_dev *get_pinctrl_dev_from_of_node(struct device_node *np)
return NULL;
}
+struct pinctrl_dev *get_pinctrl_dev_from_acpi(acpi_handle handle)
+{
+ struct pinctrl_dev *pctldev;
+
+ mutex_lock(&pinctrldev_list_mutex);
+
+ list_for_each_entry(pctldev, &pinctrldev_list, node)
+ if (ACPI_HANDLE(pctldev->dev) == handle) {
+ mutex_unlock(&pinctrldev_list_mutex);
+ return pctldev;
+ }
+
+ mutex_unlock(&pinctrldev_list_mutex);
+
+ return NULL;
+}
+
/**
* pin_get_from_name() - look up a pin number from a name
* @pctldev: the pin control device to lookup the pin on
@@ -827,6 +845,12 @@ static struct pinctrl *create_pinctrl(struct device *dev)
return ERR_PTR(ret);
}
+ ret = pinctrl_acpi_to_map(p);
+ if (ret < 0) {
+ kfree(p);
+ return ERR_PTR(ret);
+ }
+
devname = dev_name(dev);
mutex_lock(&pinctrl_maps_mutex);
@@ -937,6 +961,8 @@ static void pinctrl_free(struct pinctrl *p, bool inlist)
pinctrl_dt_free_maps(p);
+ pinctrl_acpi_free_maps(p);
+
if (inlist)
list_del(&p->node);
kfree(p);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index ca08723..797ab8b 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -14,6 +14,7 @@
#include <linux/radix-tree.h>
#include <linux/pinctrl/pinconf.h>
#include <linux/pinctrl/machine.h>
+#include <linux/acpi.h>
struct pinctrl_gpio_range;
@@ -171,6 +172,7 @@ struct pinctrl_maps {
struct pinctrl_dev *get_pinctrl_dev_from_devname(const char *dev_name);
struct pinctrl_dev *get_pinctrl_dev_from_of_node(struct device_node *np);
+struct pinctrl_dev *get_pinctrl_dev_from_acpi(acpi_handle handle);
int pin_get_from_name(struct pinctrl_dev *pctldev, const char *name);
const char *pin_get_name(struct pinctrl_dev *pctldev, const unsigned pin);
int pinctrl_get_group_selector(struct pinctrl_dev *pctldev,
--
1.9.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [RFC PATCH 3/4] pinctrl: Add ACPI support
2016-03-31 11:44 ` [RFC PATCH 3/4] pinctrl: " Irina Tirdea
@ 2016-04-01 14:14 ` Andy Shevchenko
2016-04-04 13:13 ` Tirdea, Irina
2016-04-04 13:37 ` Mika Westerberg
1 sibling, 1 reply; 41+ messages in thread
From: Andy Shevchenko @ 2016-04-01 14:14 UTC (permalink / raw)
To: Irina Tirdea, Rafael J. Wysocki, Len Brown, Mika Westerberg,
Linus Walleij, linux-gpio, linux-acpi
Cc: Rob Herring, Heikki Krogerus, Octavian Purdila, Cristina Ciocan,
devicetree, linux-kernel
On Thu, 2016-03-31 at 14:44 +0300, Irina Tirdea wrote:
> Add ACPI support for pin controller properties. These are
> based on ACPI _DSD properties and follow the device tree
> model based on states and node configurations. The states
> are defined as _DSD properties and configuration nodes
> are defined using the _DSD Hierarchical Properties Extension.
>
> A configuration node supports the generic device tree properties.
>
> The implementation is based on device tree code from devicetree.c.
>
Patch is good to me, though few minor comments below.
> +/**
> + * struct pinctrl_acpi_map - mapping table chunk parsed from ACPI
> + * @node: list node for struct pinctrl's @fw_maps field
> + * @pctldev: the pin controller that allocated this struct, and will
> free it
> + * @maps: the mapping table entries
We have @map and @num_maps.
> + */
> +struct pinctrl_acpi_map {
> + struct list_head node;
> + struct pinctrl_dev *pctldev;
> + struct pinctrl_map *map;
> + unsigned num_maps;
> +};
> +
>
> +static int acpi_remember_or_free_map(struct pinctrl *p, const char
> *statename,
> + struct pinctrl_dev *pctldev,
> + struct pinctrl_map *map,
> unsigned num_maps)
> +{
> + struct pinctrl_acpi_map *acpi_map;
> + struct list_head *acpi_maps;
> + unsigned int i;
Just unsigned i to be in align with unsigned num_maps.
> +
> + /* Initialize common mapping table entry fields */
> + for (i = 0; i < num_maps; i++) {
> + map[i].dev_name = dev_name(p->dev);
> + map[i].name = statename;
> + if (pctldev)
> + map[i].ctrl_dev_name = dev_name(pctldev-
> >dev);
> + }
> +int pinctrl_acpi_to_map(struct pinctrl *p)
> +{
> + const union acpi_object *prop, *statenames, *configs;
> + unsigned int state, nstates, nconfigs, config;
> + char *statename, *propname, *configname;
> + struct fwnode_handle *fw_prop;
> + struct acpi_device *adev;
> + int ret;
> +
> + /* We may store pointers to property names within the node
> */
> + adev = acpi_bus_get_acpi_device(ACPI_HANDLE(p->dev));
> + if (!adev)
> + return -ENODEV;
> +
> + /* Only allow named states (device must have prop 'pinctrl-
> names') */
Does it fit one line?
> +err_free_map:
Perhaps err_free_maps?
> + pinctrl_acpi_free_maps(p);
> + return ret;
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [RFC PATCH 3/4] pinctrl: Add ACPI support
2016-04-01 14:14 ` Andy Shevchenko
@ 2016-04-04 13:13 ` Tirdea, Irina
0 siblings, 0 replies; 41+ messages in thread
From: Tirdea, Irina @ 2016-04-04 13:13 UTC (permalink / raw)
To: Andy Shevchenko, Rafael J. Wysocki, Len Brown, Mika Westerberg,
Linus Walleij, linux-gpio, linux-acpi
Cc: Rob Herring, Heikki Krogerus, Purdila, Octavian, Ciocan,
Cristina, devicetree, linux-kernel
> -----Original Message-----
> From: Andy Shevchenko [mailto:andriy.shevchenko@linux.intel.com]
> Sent: 01 April, 2016 17:15
> To: Tirdea, Irina; Rafael J. Wysocki; Len Brown; Mika Westerberg; Linus Walleij; linux-gpio@vger.kernel.org; linux-
> acpi@vger.kernel.org
> Cc: Rob Herring; Heikki Krogerus; Purdila, Octavian; Ciocan, Cristina; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH 3/4] pinctrl: Add ACPI support
>
> On Thu, 2016-03-31 at 14:44 +0300, Irina Tirdea wrote:
> > Add ACPI support for pin controller properties. These are
> > based on ACPI _DSD properties and follow the device tree
> > model based on states and node configurations. The states
> > are defined as _DSD properties and configuration nodes
> > are defined using the _DSD Hierarchical Properties Extension.
> >
> > A configuration node supports the generic device tree properties.
> >
> > The implementation is based on device tree code from devicetree.c.
> >
>
> Patch is good to me, though few minor comments below.
>
> > +/**
> > + * struct pinctrl_acpi_map - mapping table chunk parsed from ACPI
> > + * @node: list node for struct pinctrl's @fw_maps field
> > + * @pctldev: the pin controller that allocated this struct, and will
> > free it
>
> > + * @maps: the mapping table entries
>
> We have @map and @num_maps.
Right, I'll add num_maps description.
>
> > + */
> > +struct pinctrl_acpi_map {
> > + struct list_head node;
> > + struct pinctrl_dev *pctldev;
> > + struct pinctrl_map *map;
> > + unsigned num_maps;
> > +};
> > +
> >
>
> > +static int acpi_remember_or_free_map(struct pinctrl *p, const char
> > *statename,
> > + struct pinctrl_dev *pctldev,
> > + struct pinctrl_map *map,
> > unsigned num_maps)
> > +{
> > + struct pinctrl_acpi_map *acpi_map;
> > + struct list_head *acpi_maps;
>
>
> > + unsigned int i;
>
> Just unsigned i to be in align with unsigned num_maps.
>
OK.
> > +
> > + /* Initialize common mapping table entry fields */
> > + for (i = 0; i < num_maps; i++) {
> > + map[i].dev_name = dev_name(p->dev);
> > + map[i].name = statename;
> > + if (pctldev)
> > + map[i].ctrl_dev_name = dev_name(pctldev-
> > >dev);
> > + }
>
>
> > +int pinctrl_acpi_to_map(struct pinctrl *p)
> > +{
> > + const union acpi_object *prop, *statenames, *configs;
> > + unsigned int state, nstates, nconfigs, config;
> > + char *statename, *propname, *configname;
> > + struct fwnode_handle *fw_prop;
> > + struct acpi_device *adev;
> > + int ret;
> > +
> > + /* We may store pointers to property names within the node
> > */
> > + adev = acpi_bus_get_acpi_device(ACPI_HANDLE(p->dev));
> > + if (!adev)
> > + return -ENODEV;
> > +
>
> > + /* Only allow named states (device must have prop 'pinctrl-
> > names') */
>
> Does it fit one line?
>
It has 77 characters, so it should fit one line.
> > +err_free_map:
>
> Perhaps err_free_maps?
>
OK.
> > + pinctrl_acpi_free_maps(p);
> > + return ret;
>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 3/4] pinctrl: Add ACPI support
2016-03-31 11:44 ` [RFC PATCH 3/4] pinctrl: " Irina Tirdea
2016-04-01 14:14 ` Andy Shevchenko
@ 2016-04-04 13:37 ` Mika Westerberg
2016-04-04 14:01 ` Tirdea, Irina
1 sibling, 1 reply; 41+ messages in thread
From: Mika Westerberg @ 2016-04-04 13:37 UTC (permalink / raw)
To: Irina Tirdea
Cc: Rafael J. Wysocki, Len Brown, Linus Walleij, linux-gpio,
linux-acpi, Rob Herring, Heikki Krogerus, Andy Shevchenko,
Octavian Purdila, Cristina Ciocan, devicetree, linux-kernel
On Thu, Mar 31, 2016 at 02:44:44PM +0300, Irina Tirdea wrote:
> Add ACPI support for pin controller properties. These are
> based on ACPI _DSD properties and follow the device tree
> model based on states and node configurations. The states
> are defined as _DSD properties and configuration nodes
> are defined using the _DSD Hierarchical Properties Extension.
>
> A configuration node supports the generic device tree properties.
>
> The implementation is based on device tree code from devicetree.c.
>
> Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
Looks good to me. Few minor comments below, though.
> ---
> Documentation/acpi/pinctrl-properties.txt | 274 +++++++++++++++++++++++++
> drivers/pinctrl/Makefile | 1 +
> drivers/pinctrl/acpi.c | 322 ++++++++++++++++++++++++++++++
> drivers/pinctrl/acpi.h | 32 +++
> drivers/pinctrl/core.c | 26 +++
> drivers/pinctrl/core.h | 2 +
> 6 files changed, 657 insertions(+)
> create mode 100644 Documentation/acpi/pinctrl-properties.txt
> create mode 100644 drivers/pinctrl/acpi.c
> create mode 100644 drivers/pinctrl/acpi.h
>
> diff --git a/Documentation/acpi/pinctrl-properties.txt b/Documentation/acpi/pinctrl-properties.txt
> new file mode 100644
> index 0000000..e93aaaf
> --- /dev/null
> +++ b/Documentation/acpi/pinctrl-properties.txt
> @@ -0,0 +1,274 @@
> += _DSD Device Properties related to pin controllers =
> +
> +== Introduction ==
> +
> +This document is an extension of the pin control subsystem in Linux [1]
> +and provides a way to describe pin controller properties in ACPI. It is
> +based on the Device Specific Data (_DSD) configuration object [2] that
> +was introduced in ACPI 5.1.
> +
> +Pin controllers are hardware modules that control pins by allowing pin
> +multiplexing and configuration. Pin multiplexing allows using the same
> +physical pins for multiple functions; for example, one pin or group of pins
> +may be used for the I2C bus, SPI bus or as general-purpose GPIO pin. Pin
> +configuration allows setting various properties such as pull-up/down,
> +tri-state, drive-strength, etc.
> +
> +Hardware modules whose signals are affected by pin configuration are
> +designated client devices. For a client device to operate correctly,
> +certain pin controllers must set up certain specific pin configurations.
> +Some client devices need a single static pin configuration, e.g. set up
> +during initialization. Others need to reconfigure pins at run-time,
> +for example to tri-state pins when the device is inactive. Hence, each
> +client device can define a set of named states. Each named state is
> +mapped to a pin controller configuration that describes the pin multiplexing
> +or configuration for that state.
> +
> +In ACPI, each pin controller and each client device is represented as an
> +ACPI device, just like any other hardware module. The pin controller
> +properties are defined using _DSD properties [2] under these devices.
> +The named states are defined using Device Properties UUID [3] under the
> +ACPI client device. The configuration nodes are defined using Hierarchical
> +Properties Extension UUID [4] and are split between the ACPI client device
> +and the pin controller device. The configuration nodes contain properties
> +that describe pin multiplexing or configuration that very similar to the
> +ones used for device tree [5].
> +
> +== Example ==
> +
> +For example, let's consider an accelerometer connected to the I2C bus on
> +a platform with a Baytrail pin controller. The accelerometer uses 2 GPIO
> +pins for I2C (SDA, SCL) and one GPIO pin for interrupt.
> +
> +The name for the pins, groups and functions used are the ones defined in the
> +pin controller driver, in the same way as it is done for device tree [5].
> +
> +For the I2C pins, the pin controller driver defines one group called
> +"i2c5_grp" that can be multiplexed with functions "i2c" or "gpio".
> +In our case, we need to select function "i2c" for group "i2c5_grp" in
> +the ACPI description.
> +
> +For the GPIO pin, the pin controller driver defines the name "GPIO_S5[00]"
BTW, those pin names were changed for Baytrail pinctrl driver so you
should probably do that here also.
> +for the pin with index 0 that we use. We need to configure this pin to
> +pull-down with pull strength of 10000 Ohms. We might also want to disable
> +the bias for the GPIO interrupt pin when entering sleep.
> +
> +Here is an ASL example for this device:
> +
> + // Pin controller device
> + Scope (_SB.GPO0)
> + {
> + Name (MUX0, Package()
> + {
> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + Package()
> + {
> + Package (2) {"function", "i2c"},
> + Package (2) {"groups", Package () {"i2c5_grp"}},
> + }
> + })
> +
> + Name (CFG0, Package()
> + {
> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + Package()
> + {
> + Package (2) {"pins", Package () {"GPIO_S5[00]"}},
> + Package (2) {"bias-pull-down", 10000},
> + }
> + })
> +
> + Name (CFG1, Package()
> + {
> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + Package()
> + {
> + Package (2) {"pins", Package () {"GPIO_S5[00]"}},
> + Package (2) {"bias-disable", 0},
> + }
> + })
> + }
> +
> + // Accelerometer device with default pinmux and pinconfig for i2c and
> + // GPIO pins
> + Scope (_SB.I2C0)
> + {
> + Device (ACL0)
> + {
> + Name (_HID, ...)
> +
> + Method (_CRS, 0, Serialized)
> + {
> + Name (RBUF, ResourceTemplate ()
> + {
> + I2cSerialBus (...)
> + GpioInt (Edge, ActiveHigh, Exclusive, PullDown, 0x0000,
> + "\\_SB.GPO0", 0x00, ResourceConsumer, , ) { 0 }
> + })
> + Return (RBUF)
> + }
> +
> + Name (_DSD, Package ()
> + {
> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + Package ()
> + {
> + Package () {"pinctrl-names", Package() {"default", "sleep"}},
> + Package ()
> + {
> + "pinctrl-0",
> + Package()
> + {
> + "accel-default-mux-i2c",
> + "accel-default-cfg-int",
> + }
> + },
> + Package ()
> + {
> + "pinctrl-1",
> + Package()
> + {
> + "accel-sleep-cfg-int",
> + }
> + },
> +
> + },
> + ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> + Package ()
> + {
> + Package (2) {"accel-default-mux-i2c", "\\_SB.GPO0.MUX0"},
> + Package (2) {"accel-default-cfg-int", "\\_SB.GPO0.CFG0"},
> + Package (2) {"accel-sleep-cfg-int", "\\_SB.GPO0.CFG1"},
> + },
> + })
> + }
> + }
> +
> +In the ASL excerpt, the accelerometer device has 2 states:
> + - a default state with 2 pin configurations:
> + - a pin multiplexing node for the i2c pins that sets function "i2c"
> + for the "i2c5_grp" pin group
> + - a pin configuration node for the GPIO interrupt pin that pull down
> + the "GPIO_S5[00]" pin and sets a pull strength of 10000 Ohms
> + - a sleep state with 1 pin configuration:
> + - a pin configuration node for pin "GPIO_S5[00]" that disables pin
> + bias
> +
> +== _DSD pinctrl properties format ==
> +
> +=== Pin controller client device states ===
> +
> +The pinctrl states are defined under the device node they apply to.
> +The format of the pinctrl states is:
> +
> + Name (_DSD, Package ()
> + {
> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + Package ()
> + {
> + Package () {"pinctrl-names", Package() {statename0, statename1, ...}},
> + Package () {"pinctrl-0", Package() {cfgname0, cfgname1, ...}},
> + Package () {"pinctrl-1", Package() {cfgname2, cfgname3, ...}},
> + }
> + }
> +
> + statename - name of the pinctrl device state (e.g.: default, sleep, etc.).
> + These names are associated with the lists of configurations
> + defined below: statename0 defines the name for configuration
> + property "pinctrl-0", statename1 defines the name for
> + configuration property "pinctrl-1", etc.
> + cfgname - name for the configuration data-only subnode.
> +
> +=== Pin controller configuration nodes ===
> +
> +The configuration data-only subnodes are defined using the Hierarchical
> +Properties Extension UUID [4]. Their definition is split between the device
> +node and the pin controller node. The format for these subnodes is:
> +
> + Scope (DEV0)
> + {
> + Name (_DSD, Package ()
> + {
> + ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"),
> + Package ()
> + {
> + Package (2) {cfgname0, "\\GPO0.DNP0"},
> + Package (2) {cfgname1, "\\GPO0.DNP1"},
> + },
> + })
> + }
> +
> + Scope (GPO0)
> + {
> + Name (DPN0, Package()
Maybe we should use node names that relate to the pinctrl subsystem and
not the ones used in the hierarchical properties extension example? I mean
real examples.
> + {
> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + Package() {...}
> + })
> + Name (DPN1, Package()
> + {
> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + Package() {...}
> + })
> + }
> +
> +Each DPN data subnode is a regular _DSD node that uses Device Properties
> +UUID [3]. There are 2 types of subnodes, depending on the properties it
> +contains: pin multiplexing nodes and pin configuration nodes.
> +
> +==== Pin multiplexing nodes ====
> +
> +The pin multiplexing nodes must contain a property named "function" and
> +define a mux function to be applied to a list of pin groups. The properties
> +supported by this node are the same as for device tree [5]. The name for the
> +pins, groups and functions used are the ones defined in the pin controller
> +driver, in the same way as it is done for device tree [5]. The format for
> +this data subnode is:
> +
> + Name (DPN0, Package()
Same here.
> + {
> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + Package()
> + {
> + Package (2) {"function", functioname},
> + Package (2) {"groups", Package () {groupname1, groupname2, ...}},
> + }
> + })
> +
> + functioname - the pinmux function to select.
> + groups - the list of groups to select with this function
> +
> +==== Pin configuration nodes ====
> +
> +The pin configuration nodes do not contain a property named "function".
> +They must contain a property named "group" or "pins". They will also
> +contain one or more configuration properties like bias-pull-up,
> +drive-open-drain, etc. The properties supported by this node are the
> +same as for device tree. Standard pinctrl properties are defined in the
> +device tree documentation [5] and in <include/linux/pinctrl/pinconf-generic.h>.
> +Pinctrl drivers may also define their own custom properties. The name for the
> +pins/groups used are the ones defined in the pin controller driver, in the
> +same way as it is done for device tree [5]. The format for the data subnode is:
> +
> + Name (DPN0, Package()
And here.
> + {
> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> + Package()
> + {
> + Package (2) {"pins", Package () {pin1, pin2, ...}},
> + Package (2) {configname1, configval1},
These should be enclosed in quotes, like "configname1" and so on.
> + Package (2) {configname2, configval2},
> + }
> + })
> +
> + pins - list of pins that properties in the node apply to
> + configname - name of the pin configuration property
> + configval - value of the pin configuration property
> +
> +== References ==
> +
> +[1] Documentation/pinctrl.txt
> +[2] http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel-1_1.htm
> +[3] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
> +[4] http://www.uefi.org/sites/default/files/resources/_DSD-hierarchical-data-extension-UUID-v1.pdf
> +[5] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> index e4bc115..12d3af6 100644
> --- a/drivers/pinctrl/Makefile
> +++ b/drivers/pinctrl/Makefile
> @@ -6,6 +6,7 @@ obj-y += core.o pinctrl-utils.o
> obj-$(CONFIG_PINMUX) += pinmux.o
> obj-$(CONFIG_PINCONF) += pinconf.o
> obj-$(CONFIG_OF) += devicetree.o
> +obj-$(CONFIG_ACPI) += acpi.o
> obj-$(CONFIG_GENERIC_PINCONF) += pinconf-generic.o
> obj-$(CONFIG_PINCTRL_ADI2) += pinctrl-adi2.o
> obj-$(CONFIG_PINCTRL_AS3722) += pinctrl-as3722.o
> diff --git a/drivers/pinctrl/acpi.c b/drivers/pinctrl/acpi.c
> new file mode 100644
> index 0000000..bed1d88
> --- /dev/null
> +++ b/drivers/pinctrl/acpi.c
> @@ -0,0 +1,322 @@
> +/*
> + * ACPI integration for the pin control subsystem
> + *
> + * Copyright (c) 2016, Intel Corporation.
> + *
> + * Derived from:
> + * devicetree.c - Copyright (C) 2012 NVIDIA CORPORATION
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +
> +#include "acpi.h"
> +#include "core.h"
> +#include "pinconf.h"
> +#include "pinctrl-utils.h"
> +
> +/**
> + * struct pinctrl_acpi_map - mapping table chunk parsed from ACPI
> + * @node: list node for struct pinctrl's @fw_maps field
> + * @pctldev: the pin controller that allocated this struct, and will free it
> + * @maps: the mapping table entries
> + */
> +struct pinctrl_acpi_map {
> + struct list_head node;
> + struct pinctrl_dev *pctldev;
> + struct pinctrl_map *map;
> + unsigned num_maps;
> +};
> +
> +static void acpi_maps_list_dh(acpi_handle handle, void *data)
> +{
> + /* The address of this function is used as a key. */
> +}
> +
> +static struct list_head *acpi_get_maps(struct device *dev)
> +{
> + acpi_handle handle = ACPI_HANDLE(dev);
> + struct list_head *maps;
> + acpi_status status;
> +
> + status = acpi_get_data(handle, acpi_maps_list_dh, (void **)&maps);
> + if (ACPI_FAILURE(status))
> + return NULL;
> +
> + return maps;
> +}
> +
> +static void acpi_free_maps(struct device *dev, struct list_head *maps)
> +{
> + acpi_handle handle = ACPI_HANDLE(dev);
> +
> + acpi_detach_data(handle, acpi_maps_list_dh);
> + kfree(maps);
> +}
> +
> +static int acpi_init_maps(struct device *dev)
> +{
> + acpi_handle handle = ACPI_HANDLE(dev);
> + struct list_head *maps;
> + acpi_status status;
> +
> + maps = kzalloc(sizeof(*maps), GFP_KERNEL);
> + if (!maps)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(maps);
> +
> + status = acpi_attach_data(handle, acpi_maps_list_dh, maps);
> + if (ACPI_FAILURE(status))
This leaks maps.
> + return -EINVAL;
> +
> + return 0;
> +}
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [RFC PATCH 3/4] pinctrl: Add ACPI support
2016-04-04 13:37 ` Mika Westerberg
@ 2016-04-04 14:01 ` Tirdea, Irina
2016-04-05 7:49 ` Mika Westerberg
0 siblings, 1 reply; 41+ messages in thread
From: Tirdea, Irina @ 2016-04-04 14:01 UTC (permalink / raw)
To: Mika Westerberg
Cc: Rafael J. Wysocki, Len Brown, Linus Walleij, linux-gpio,
linux-acpi, Rob Herring, Heikki Krogerus, Andy Shevchenko,
Purdila, Octavian, Ciocan, Cristina, devicetree, linux-kernel
> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: 04 April, 2016 16:37
> To: Tirdea, Irina
> Cc: Rafael J. Wysocki; Len Brown; Linus Walleij; linux-gpio@vger.kernel.org; linux-acpi@vger.kernel.org; Rob Herring; Heikki Krogerus;
> Andy Shevchenko; Purdila, Octavian; Ciocan, Cristina; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH 3/4] pinctrl: Add ACPI support
>
> On Thu, Mar 31, 2016 at 02:44:44PM +0300, Irina Tirdea wrote:
> > Add ACPI support for pin controller properties. These are
> > based on ACPI _DSD properties and follow the device tree
> > model based on states and node configurations. The states
> > are defined as _DSD properties and configuration nodes
> > are defined using the _DSD Hierarchical Properties Extension.
> >
> > A configuration node supports the generic device tree properties.
> >
> > The implementation is based on device tree code from devicetree.c.
> >
> > Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
>
> Looks good to me. Few minor comments below, though.
Thanks for the review, Mika!
>
> > ---
> > Documentation/acpi/pinctrl-properties.txt | 274 +++++++++++++++++++++++++
> > drivers/pinctrl/Makefile | 1 +
> > drivers/pinctrl/acpi.c | 322 ++++++++++++++++++++++++++++++
> > drivers/pinctrl/acpi.h | 32 +++
> > drivers/pinctrl/core.c | 26 +++
> > drivers/pinctrl/core.h | 2 +
> > 6 files changed, 657 insertions(+)
> > create mode 100644 Documentation/acpi/pinctrl-properties.txt
> > create mode 100644 drivers/pinctrl/acpi.c
> > create mode 100644 drivers/pinctrl/acpi.h
> >
> > diff --git a/Documentation/acpi/pinctrl-properties.txt b/Documentation/acpi/pinctrl-properties.txt
> > new file mode 100644
> > index 0000000..e93aaaf
> > --- /dev/null
> > +++ b/Documentation/acpi/pinctrl-properties.txt
> > @@ -0,0 +1,274 @@
> > += _DSD Device Properties related to pin controllers =
> > +
> > +== Introduction ==
> > +
> > +This document is an extension of the pin control subsystem in Linux [1]
> > +and provides a way to describe pin controller properties in ACPI. It is
> > +based on the Device Specific Data (_DSD) configuration object [2] that
> > +was introduced in ACPI 5.1.
> > +
> > +Pin controllers are hardware modules that control pins by allowing pin
> > +multiplexing and configuration. Pin multiplexing allows using the same
> > +physical pins for multiple functions; for example, one pin or group of pins
> > +may be used for the I2C bus, SPI bus or as general-purpose GPIO pin. Pin
> > +configuration allows setting various properties such as pull-up/down,
> > +tri-state, drive-strength, etc.
> > +
> > +Hardware modules whose signals are affected by pin configuration are
> > +designated client devices. For a client device to operate correctly,
> > +certain pin controllers must set up certain specific pin configurations.
> > +Some client devices need a single static pin configuration, e.g. set up
> > +during initialization. Others need to reconfigure pins at run-time,
> > +for example to tri-state pins when the device is inactive. Hence, each
> > +client device can define a set of named states. Each named state is
> > +mapped to a pin controller configuration that describes the pin multiplexing
> > +or configuration for that state.
> > +
> > +In ACPI, each pin controller and each client device is represented as an
> > +ACPI device, just like any other hardware module. The pin controller
> > +properties are defined using _DSD properties [2] under these devices.
> > +The named states are defined using Device Properties UUID [3] under the
> > +ACPI client device. The configuration nodes are defined using Hierarchical
> > +Properties Extension UUID [4] and are split between the ACPI client device
> > +and the pin controller device. The configuration nodes contain properties
> > +that describe pin multiplexing or configuration that very similar to the
> > +ones used for device tree [5].
> > +
> > +== Example ==
> > +
> > +For example, let's consider an accelerometer connected to the I2C bus on
> > +a platform with a Baytrail pin controller. The accelerometer uses 2 GPIO
> > +pins for I2C (SDA, SCL) and one GPIO pin for interrupt.
> > +
> > +The name for the pins, groups and functions used are the ones defined in the
> > +pin controller driver, in the same way as it is done for device tree [5].
> > +
> > +For the I2C pins, the pin controller driver defines one group called
> > +"i2c5_grp" that can be multiplexed with functions "i2c" or "gpio".
> > +In our case, we need to select function "i2c" for group "i2c5_grp" in
> > +the ACPI description.
> > +
> > +For the GPIO pin, the pin controller driver defines the name "GPIO_S5[00]"
>
> BTW, those pin names were changed for Baytrail pinctrl driver so you
> should probably do that here also.
>
Right, will update it according to the current naming.
> > +for the pin with index 0 that we use. We need to configure this pin to
> > +pull-down with pull strength of 10000 Ohms. We might also want to disable
> > +the bias for the GPIO interrupt pin when entering sleep.
> > +
> > +Here is an ASL example for this device:
> > +
> > + // Pin controller device
> > + Scope (_SB.GPO0)
> > + {
> > + Name (MUX0, Package()
> > + {
> > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + Package()
> > + {
> > + Package (2) {"function", "i2c"},
> > + Package (2) {"groups", Package () {"i2c5_grp"}},
> > + }
> > + })
> > +
> > + Name (CFG0, Package()
> > + {
> > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + Package()
> > + {
> > + Package (2) {"pins", Package () {"GPIO_S5[00]"}},
> > + Package (2) {"bias-pull-down", 10000},
> > + }
> > + })
> > +
> > + Name (CFG1, Package()
> > + {
> > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + Package()
> > + {
> > + Package (2) {"pins", Package () {"GPIO_S5[00]"}},
> > + Package (2) {"bias-disable", 0},
> > + }
> > + })
> > + }
> > +
> > +
<snip>
> > + Scope (GPO0)
> > + {
> > + Name (DPN0, Package()
>
> Maybe we should use node names that relate to the pinctrl subsystem and
> not the ones used in the hierarchical properties extension example? I mean
> real examples.
>
OK, I will use MUX0 and CFG0 instead.
> > + {
> > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + Package() {...}
> > + })
> > + Name (DPN1, Package()
> > + {
> > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + Package() {...}
> > + })
> > + }
> > +
> > +Each DPN data subnode is a regular _DSD node that uses Device Properties
> > +UUID [3]. There are 2 types of subnodes, depending on the properties it
> > +contains: pin multiplexing nodes and pin configuration nodes.
> > +
> > +==== Pin multiplexing nodes ====
> > +
> > +The pin multiplexing nodes must contain a property named "function" and
> > +define a mux function to be applied to a list of pin groups. The properties
> > +supported by this node are the same as for device tree [5]. The name for the
> > +pins, groups and functions used are the ones defined in the pin controller
> > +driver, in the same way as it is done for device tree [5]. The format for
> > +this data subnode is:
> > +
> > + Name (DPN0, Package()
>
> Same here.
>
> > + {
> > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + Package()
> > + {
> > + Package (2) {"function", functioname},
> > + Package (2) {"groups", Package () {groupname1, groupname2, ...}},
> > + }
> > + })
> > +
> > + functioname - the pinmux function to select.
> > + groups - the list of groups to select with this function
> > +
> > +==== Pin configuration nodes ====
> > +
> > +The pin configuration nodes do not contain a property named "function".
> > +They must contain a property named "group" or "pins". They will also
> > +contain one or more configuration properties like bias-pull-up,
> > +drive-open-drain, etc. The properties supported by this node are the
> > +same as for device tree. Standard pinctrl properties are defined in the
> > +device tree documentation [5] and in <include/linux/pinctrl/pinconf-generic.h>.
> > +Pinctrl drivers may also define their own custom properties. The name for the
> > +pins/groups used are the ones defined in the pin controller driver, in the
> > +same way as it is done for device tree [5]. The format for the data subnode is:
> > +
> > + Name (DPN0, Package()
>
> And here.
>
> > + {
> > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > + Package()
> > + {
> > + Package (2) {"pins", Package () {pin1, pin2, ...}},
> > + Package (2) {configname1, configval1},
>
> These should be enclosed in quotes, like "configname1" and so on.
>
OK. Should all string properties be enclosed in quotes or only the property names
(e.g. should I also change values like "configval1", "pin1", "statename1", etc.)?
> > + Package (2) {configname2, configval2},
> > + }
> > + })
> > +
> > + pins - list of pins that properties in the node apply to
> > + configname - name of the pin configuration property
> > + configval - value of the pin configuration property
> > +
> > +== References ==
> > +
> > +[1] Documentation/pinctrl.txt
> > +[2] http://www.uefi.org/sites/default/files/resources/_DSD-implementation-guide-toplevel-1_1.htm
> > +[3] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
> > +[4] http://www.uefi.org/sites/default/files/resources/_DSD-hierarchical-data-extension-UUID-v1.pdf
> > +[5] Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> > diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
> > index e4bc115..12d3af6 100644
> > --- a/drivers/pinctrl/Makefile
> > +++ b/drivers/pinctrl/Makefile
> > @@ -6,6 +6,7 @@ obj-y += core.o pinctrl-utils.o
> > obj-$(CONFIG_PINMUX) += pinmux.o
> > obj-$(CONFIG_PINCONF) += pinconf.o
> > obj-$(CONFIG_OF) += devicetree.o
> > +obj-$(CONFIG_ACPI) += acpi.o
> > obj-$(CONFIG_GENERIC_PINCONF) += pinconf-generic.o
> > obj-$(CONFIG_PINCTRL_ADI2) += pinctrl-adi2.o
> > obj-$(CONFIG_PINCTRL_AS3722) += pinctrl-as3722.o
> > diff --git a/drivers/pinctrl/acpi.c b/drivers/pinctrl/acpi.c
> > new file mode 100644
> > index 0000000..bed1d88
> > --- /dev/null
> > +++ b/drivers/pinctrl/acpi.c
> > @@ -0,0 +1,322 @@
> > +/*
> > + * ACPI integration for the pin control subsystem
> > + *
> > + * Copyright (c) 2016, Intel Corporation.
> > + *
> > + * Derived from:
> > + * devicetree.c - Copyright (C) 2012 NVIDIA CORPORATION
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms and conditions of the GNU General Public License,
> > + * version 2, as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but WITHOUT
> > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> > + * more details.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/device.h>
> > +#include <linux/pinctrl/pinctrl.h>
> > +#include <linux/pinctrl/pinconf-generic.h>
> > +
> > +#include "acpi.h"
> > +#include "core.h"
> > +#include "pinconf.h"
> > +#include "pinctrl-utils.h"
> > +
> > +/**
> > + * struct pinctrl_acpi_map - mapping table chunk parsed from ACPI
> > + * @node: list node for struct pinctrl's @fw_maps field
> > + * @pctldev: the pin controller that allocated this struct, and will free it
> > + * @maps: the mapping table entries
> > + */
> > +struct pinctrl_acpi_map {
> > + struct list_head node;
> > + struct pinctrl_dev *pctldev;
> > + struct pinctrl_map *map;
> > + unsigned num_maps;
> > +};
> > +
> > +static void acpi_maps_list_dh(acpi_handle handle, void *data)
> > +{
> > + /* The address of this function is used as a key. */
> > +}
> > +
> > +static struct list_head *acpi_get_maps(struct device *dev)
> > +{
> > + acpi_handle handle = ACPI_HANDLE(dev);
> > + struct list_head *maps;
> > + acpi_status status;
> > +
> > + status = acpi_get_data(handle, acpi_maps_list_dh, (void **)&maps);
> > + if (ACPI_FAILURE(status))
> > + return NULL;
> > +
> > + return maps;
> > +}
> > +
> > +static void acpi_free_maps(struct device *dev, struct list_head *maps)
> > +{
> > + acpi_handle handle = ACPI_HANDLE(dev);
> > +
> > + acpi_detach_data(handle, acpi_maps_list_dh);
> > + kfree(maps);
> > +}
> > +
> > +static int acpi_init_maps(struct device *dev)
> > +{
> > + acpi_handle handle = ACPI_HANDLE(dev);
> > + struct list_head *maps;
> > + acpi_status status;
> > +
> > + maps = kzalloc(sizeof(*maps), GFP_KERNEL);
> > + if (!maps)
> > + return -ENOMEM;
> > +
> > + INIT_LIST_HEAD(maps);
> > +
> > + status = acpi_attach_data(handle, acpi_maps_list_dh, maps);
> > + if (ACPI_FAILURE(status))
>
> This leaks maps.
Oops. I'll fix this.
>
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [RFC PATCH 3/4] pinctrl: Add ACPI support
2016-04-04 14:01 ` Tirdea, Irina
@ 2016-04-05 7:49 ` Mika Westerberg
0 siblings, 0 replies; 41+ messages in thread
From: Mika Westerberg @ 2016-04-05 7:49 UTC (permalink / raw)
To: Tirdea, Irina
Cc: Rafael J. Wysocki, Len Brown, Linus Walleij, linux-gpio,
linux-acpi, Rob Herring, Heikki Krogerus, Andy Shevchenko,
Purdila, Octavian, Ciocan, Cristina, devicetree, linux-kernel
On Mon, Apr 04, 2016 at 02:01:13PM +0000, Tirdea, Irina wrote:
> > > + {
> > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
> > > + Package()
> > > + {
> > > + Package (2) {"pins", Package () {pin1, pin2, ...}},
> > > + Package (2) {configname1, configval1},
> >
> > These should be enclosed in quotes, like "configname1" and so on.
> >
> OK. Should all string properties be enclosed in quotes or only the property names
> (e.g. should I also change values like "configval1", "pin1", "statename1", etc.)?
I think all types should be documented using correct format. Strings
should have "" and so on.
^ permalink raw reply [flat|nested] 41+ messages in thread
* [RFC PATCH 4/4] pinctrl: Parse GpioInt/GpioIo resources
2016-03-31 11:44 [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Irina Tirdea
` (2 preceding siblings ...)
2016-03-31 11:44 ` [RFC PATCH 3/4] pinctrl: " Irina Tirdea
@ 2016-03-31 11:44 ` Irina Tirdea
2016-04-04 13:47 ` Mika Westerberg
2016-04-04 21:40 ` [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Mark Brown
2016-04-04 22:52 ` Mark Rutland
5 siblings, 1 reply; 41+ messages in thread
From: Irina Tirdea @ 2016-03-31 11:44 UTC (permalink / raw)
To: Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
linux-gpio, linux-acpi
Cc: Rob Herring, Heikki Krogerus, Andy Shevchenko, Octavian Purdila,
Cristina Ciocan, devicetree, linux-kernel, Irina Tirdea
Parse GpioInt/GpioIo ACPI resources and use the pin configuration
information to generate pin controller maps. These maps are associated
with the "default" state, only if this state is defined in the _DSD
of the acpi device.
Signed-off-by: Irina Tirdea <irina.tirdea@intel.com>
---
Documentation/acpi/pinctrl-properties.txt | 8 ++
drivers/pinctrl/acpi.c | 229 ++++++++++++++++++++++++++++++
2 files changed, 237 insertions(+)
diff --git a/Documentation/acpi/pinctrl-properties.txt b/Documentation/acpi/pinctrl-properties.txt
index e93aaaf..8243190 100644
--- a/Documentation/acpi/pinctrl-properties.txt
+++ b/Documentation/acpi/pinctrl-properties.txt
@@ -265,6 +265,14 @@ same way as it is done for device tree [5]. The format for the data subnode is:
configname - name of the pin configuration property
configval - value of the pin configuration property
+== GpioInt()/GpioIo() _CRS resources ==
+
+If the device has any GpioInt/GpioIo _CRS ACPI resources, they are parsed so
+that the pin configuration information is used (e.g. PullUp/PullDown/PullNone).
+The pin configuration from GpioInt/GpioIo will be associated with the "default"
+state, only if such a state is defined in the _DSD of the device. If no
+"default" state is defined, the GPIO configuration from _CRS will be ignored.
+
== References ==
[1] Documentation/pinctrl.txt
diff --git a/drivers/pinctrl/acpi.c b/drivers/pinctrl/acpi.c
index bed1d88..7199c3d 100644
--- a/drivers/pinctrl/acpi.c
+++ b/drivers/pinctrl/acpi.c
@@ -140,6 +140,224 @@ static int acpi_remember_or_free_map(struct pinctrl *p, const char *statename,
return pinctrl_register_map(map, num_maps, false);
}
+#ifdef CONFIG_GENERIC_PINCONF
+struct acpi_gpio_lookup {
+ unsigned int index;
+ bool found;
+ unsigned int n;
+ struct pinctrl_map *map;
+ unsigned num_maps;
+ unsigned reserved_maps;
+ struct pinctrl_dev **pctldevs;
+};
+
+/* For now we only handle acpi pin config values */
+#define ACPI_MAX_CFGS 1
+
+static int acpi_parse_gpio_config(const struct acpi_resource_gpio *agpio,
+ unsigned long **configs,
+ unsigned int *nconfigs)
+{
+ enum pin_config_param param;
+ int ret;
+
+ /* Parse configs from GpioInt/GpioIo ACPI resource */
+ *nconfigs = 0;
+ *configs = kcalloc(ACPI_MAX_CFGS, sizeof(*configs), GFP_KERNEL);
+ if (!*configs)
+ return -ENOMEM;
+
+ /* For now, only parse pin_config */
+ switch (agpio->pin_config) {
+ case ACPI_PIN_CONFIG_DEFAULT:
+ param = PIN_CONFIG_BIAS_PULL_PIN_DEFAULT;
+ break;
+ case ACPI_PIN_CONFIG_PULLUP:
+ param = PIN_CONFIG_BIAS_PULL_UP;
+ break;
+ case ACPI_PIN_CONFIG_PULLDOWN:
+ param = PIN_CONFIG_BIAS_PULL_DOWN;
+ break;
+ case ACPI_PIN_CONFIG_NOPULL:
+ param = PIN_CONFIG_BIAS_DISABLE;
+ break;
+ default:
+ ret = -EINVAL;
+ goto exit_free;
+ }
+ *configs[*nconfigs] = pinconf_to_config_packed(param,
+ param == PIN_CONFIG_BIAS_DISABLE ? 0 : 1);
+ (*nconfigs)++;
+
+ return 0;
+
+exit_free:
+ kfree(*configs);
+ return ret;
+}
+
+static int acpi_gpio_to_map(struct acpi_resource *ares, void *data)
+{
+ struct pinctrl_dev *pctldev, **new_pctldevs;
+ struct acpi_gpio_lookup *lookup = data;
+ const struct acpi_resource_gpio *agpio;
+ acpi_handle pctrl_handle = NULL;
+ unsigned int nconfigs, i;
+ unsigned long *configs;
+ acpi_status status;
+ const char *pin;
+ int ret;
+
+ if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
+ return 1;
+ if (lookup->n++ != lookup->index || lookup->found)
+ return 1;
+
+ agpio = &ares->data.gpio;
+
+ /* Get configs from ACPI GPIO resource */
+ ret = acpi_parse_gpio_config(agpio, &configs, &nconfigs);
+ if (ret)
+ return ret;
+
+ /* Get pinctrl reference from GPIO resource */
+ status = acpi_get_handle(NULL, agpio->resource_source.string_ptr,
+ &pctrl_handle);
+ if (ACPI_FAILURE(status) || !pctrl_handle) {
+ ret = -EINVAL;
+ goto exit_free_configs;
+ }
+
+ /* Find the pin controller */
+ pctldev = get_pinctrl_dev_from_acpi(pctrl_handle);
+ if (!pctldev) {
+ ret = -EINVAL;
+ goto exit_free_configs;
+ }
+
+ /* Allocate space for maps and pinctrl_dev references */
+ ret = pinctrl_utils_reserve_map(pctldev, &lookup->map,
+ &lookup->reserved_maps,
+ &lookup->num_maps,
+ agpio->pin_table_length);
+ if (ret < 0)
+ goto exit_free_configs;
+
+ new_pctldevs = krealloc(lookup->pctldevs,
+ sizeof(*new_pctldevs) * lookup->reserved_maps,
+ GFP_KERNEL);
+ if (!new_pctldevs) {
+ ret = -ENOMEM;
+ goto exit_free_configs;
+ }
+ lookup->pctldevs = new_pctldevs;
+
+ /* For each GPIO pin */
+ for (i = 0; i < agpio->pin_table_length; i++) {
+ pin = pin_get_name(pctldev, agpio->pin_table[i]);
+ if (!pin) {
+ ret = -EINVAL;
+ goto exit_free_configs;
+ }
+ lookup->pctldevs[lookup->num_maps] = pctldev;
+ ret = pinctrl_utils_add_map_configs(pctldev, &lookup->map,
+ &lookup->reserved_maps,
+ &lookup->num_maps, pin,
+ configs, nconfigs,
+ PIN_MAP_TYPE_CONFIGS_PIN);
+ if (ret < 0)
+ goto exit_free_configs;
+ }
+
+ lookup->found = true;
+ kfree(configs);
+ return 1;
+
+exit_free_configs:
+ kfree(configs);
+ return ret;
+}
+
+static int acpi_parse_gpio_res(struct pinctrl *p,
+ struct pinctrl_map **map,
+ unsigned *num_maps,
+ struct pinctrl_dev ***pctldevs)
+{
+ struct acpi_gpio_lookup lookup;
+ struct list_head res_list;
+ struct acpi_device *adev;
+ unsigned int index;
+ int ret;
+
+ adev = ACPI_COMPANION(p->dev);
+
+ *map = NULL;
+ *num_maps = 0;
+ memset(&lookup, 0, sizeof(lookup));
+
+ /* Parse all GpioInt/GpioIo resources in _CRS and extract pin conf */
+ for (index = 0; ; index++) {
+ lookup.index = index;
+ lookup.n = 0;
+ lookup.found = false;
+
+ INIT_LIST_HEAD(&res_list);
+ ret = acpi_dev_get_resources(adev, &res_list, acpi_gpio_to_map,
+ &lookup);
+ if (ret < 0)
+ goto exit_free;
+ acpi_dev_free_resource_list(&res_list);
+ if (!lookup.found)
+ break;
+ }
+
+ *map = lookup.map;
+ *num_maps = lookup.num_maps;
+ *pctldevs = lookup.pctldevs;
+
+ return 0;
+
+exit_free:
+ pinctrl_utils_free_map(NULL, lookup.map, lookup.num_maps);
+ kfree(lookup.pctldevs);
+ return ret;
+}
+
+static int acpi_parse_gpio_resources(struct pinctrl *p, char *statename)
+{
+ struct pinctrl_dev **pctldevs;
+ struct pinctrl_map *map;
+ unsigned num_maps;
+ unsigned int i;
+ int ret;
+
+ ret = acpi_parse_gpio_res(p, &map, &num_maps, &pctldevs);
+ if (ret)
+ return ret;
+
+ /* Add maps one by one since pinctrl devices might be different */
+ for (i = 0; i < num_maps; i++) {
+ ret = acpi_remember_or_free_map(p, statename, pctldevs[i],
+ &map[i], 1);
+ if (ret < 0)
+ goto exit_free;
+ }
+
+ kfree(pctldevs);
+ return 0;
+
+exit_free:
+ pinctrl_utils_free_map(NULL, map, num_maps);
+ kfree(pctldevs);
+ return ret;
+}
+#else
+static inline int acpi_parse_gpio_resources(struct pinctrl *p, char *statename)
+{
+ return 0;
+}
+#endif
+
static int acpi_remember_dummy_state(struct pinctrl *p, const char *statename)
{
struct pinctrl_map *map;
@@ -273,6 +491,17 @@ int pinctrl_acpi_to_map(struct pinctrl *p)
}
statename = statenames[state].string.pointer;
+ /*
+ * Parse any GpioInt/GpioIo resources and
+ * associate them with the 'default' state.
+ */
+ if (!strcmp(statename, PINCTRL_STATE_DEFAULT)) {
+ ret = acpi_parse_gpio_resources(p, statename);
+ if (ret)
+ dev_err(p->dev,
+ "Could not parse GPIO resources\n");
+ }
+
/* Retrieve the pinctrl-* property */
propname = kasprintf(GFP_KERNEL, "pinctrl-%d", state);
ret = acpi_dev_get_property(adev, propname, ACPI_TYPE_PACKAGE,
--
1.9.1
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [RFC PATCH 4/4] pinctrl: Parse GpioInt/GpioIo resources
2016-03-31 11:44 ` [RFC PATCH 4/4] pinctrl: Parse GpioInt/GpioIo resources Irina Tirdea
@ 2016-04-04 13:47 ` Mika Westerberg
2016-04-04 14:05 ` Tirdea, Irina
0 siblings, 1 reply; 41+ messages in thread
From: Mika Westerberg @ 2016-04-04 13:47 UTC (permalink / raw)
To: Irina Tirdea
Cc: Rafael J. Wysocki, Len Brown, Linus Walleij, linux-gpio,
linux-acpi, Rob Herring, Heikki Krogerus, Andy Shevchenko,
Octavian Purdila, Cristina Ciocan, devicetree, linux-kernel
On Thu, Mar 31, 2016 at 02:44:45PM +0300, Irina Tirdea wrote:
> +static int acpi_parse_gpio_res(struct pinctrl *p,
> + struct pinctrl_map **map,
> + unsigned *num_maps,
> + struct pinctrl_dev ***pctldevs)
> +{
> + struct acpi_gpio_lookup lookup;
> + struct list_head res_list;
> + struct acpi_device *adev;
> + unsigned int index;
> + int ret;
> +
> + adev = ACPI_COMPANION(p->dev);
> +
> + *map = NULL;
> + *num_maps = 0;
> + memset(&lookup, 0, sizeof(lookup));
> +
> + /* Parse all GpioInt/GpioIo resources in _CRS and extract pin conf */
> + for (index = 0; ; index++) {
> + lookup.index = index;
> + lookup.n = 0;
> + lookup.found = false;
> +
> + INIT_LIST_HEAD(&res_list);
> + ret = acpi_dev_get_resources(adev, &res_list, acpi_gpio_to_map,
> + &lookup);
> + if (ret < 0)
> + goto exit_free;
> + acpi_dev_free_resource_list(&res_list);
> + if (!lookup.found)
> + break;
> + }
> +
> + *map = lookup.map;
> + *num_maps = lookup.num_maps;
> + *pctldevs = lookup.pctldevs;
This function has quite many stars in arguments and in the function
body. In particular pctldevs has three stars!
I wonder if this could be written in such way that avoids that. Like
create a structure holding the map information and pass that to the
function instead.
Other parts of the patch look good to me.
^ permalink raw reply [flat|nested] 41+ messages in thread* RE: [RFC PATCH 4/4] pinctrl: Parse GpioInt/GpioIo resources
2016-04-04 13:47 ` Mika Westerberg
@ 2016-04-04 14:05 ` Tirdea, Irina
0 siblings, 0 replies; 41+ messages in thread
From: Tirdea, Irina @ 2016-04-04 14:05 UTC (permalink / raw)
To: Mika Westerberg
Cc: Rafael J. Wysocki, Len Brown, Linus Walleij, linux-gpio,
linux-acpi, Rob Herring, Heikki Krogerus, Andy Shevchenko,
Purdila, Octavian, Ciocan, Cristina, devicetree, linux-kernel
> -----Original Message-----
> From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com]
> Sent: 04 April, 2016 16:48
> To: Tirdea, Irina
> Cc: Rafael J. Wysocki; Len Brown; Linus Walleij; linux-gpio@vger.kernel.org; linux-acpi@vger.kernel.org; Rob Herring; Heikki Krogerus;
> Andy Shevchenko; Purdila, Octavian; Ciocan, Cristina; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH 4/4] pinctrl: Parse GpioInt/GpioIo resources
>
> On Thu, Mar 31, 2016 at 02:44:45PM +0300, Irina Tirdea wrote:
> > +static int acpi_parse_gpio_res(struct pinctrl *p,
> > + struct pinctrl_map **map,
> > + unsigned *num_maps,
> > + struct pinctrl_dev ***pctldevs)
> > +{
> > + struct acpi_gpio_lookup lookup;
> > + struct list_head res_list;
> > + struct acpi_device *adev;
> > + unsigned int index;
> > + int ret;
> > +
> > + adev = ACPI_COMPANION(p->dev);
> > +
> > + *map = NULL;
> > + *num_maps = 0;
> > + memset(&lookup, 0, sizeof(lookup));
> > +
> > + /* Parse all GpioInt/GpioIo resources in _CRS and extract pin conf */
> > + for (index = 0; ; index++) {
> > + lookup.index = index;
> > + lookup.n = 0;
> > + lookup.found = false;
> > +
> > + INIT_LIST_HEAD(&res_list);
> > + ret = acpi_dev_get_resources(adev, &res_list, acpi_gpio_to_map,
> > + &lookup);
> > + if (ret < 0)
> > + goto exit_free;
> > + acpi_dev_free_resource_list(&res_list);
> > + if (!lookup.found)
> > + break;
> > + }
> > +
> > + *map = lookup.map;
> > + *num_maps = lookup.num_maps;
> > + *pctldevs = lookup.pctldevs;
>
> This function has quite many stars in arguments and in the function
> body. In particular pctldevs has three stars!
>
> I wonder if this could be written in such way that avoids that. Like
> create a structure holding the map information and pass that to the
> function instead.
>
Yes, that would make the code more clear. I will rewrite it using a structure.
> Other parts of the patch look good to me.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-03-31 11:44 [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Irina Tirdea
` (3 preceding siblings ...)
2016-03-31 11:44 ` [RFC PATCH 4/4] pinctrl: Parse GpioInt/GpioIo resources Irina Tirdea
@ 2016-04-04 21:40 ` Mark Brown
2016-04-05 9:00 ` Linus Walleij
2016-04-05 12:51 ` Octavian Purdila
2016-04-04 22:52 ` Mark Rutland
5 siblings, 2 replies; 41+ messages in thread
From: Mark Brown @ 2016-04-04 21:40 UTC (permalink / raw)
To: Irina Tirdea
Cc: Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
linux-gpio, linux-acpi, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Octavian Purdila, Cristina Ciocan, devicetree,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1649 bytes --]
On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
> This is a proposal for adding ACPI support for pin controller
> configuration.
> It has been developed to enable the MinnowBoard and IoT community
> by providing an easy way to specify pin multiplexing and
> pin configuration.
So this is mainly targeted at modules being added to base boards?
Without getting into the binding at all here it seems like this is not
solving the problem at the right abstraction level. It's exposing the
pins on the SoC directly without any tie in with the functionality that
goes over those pins. This means that any binding of a board to an ACPI
using system that just uses this is going to be entirely specific to the
particular combination of base and expansion board even if the
electrical connections are standard.
This is something that people are currently looking at for DT, there the
discussion has been about defining the connectors as entities and hiding
the details of the muxing on the SoC behind that along with higher level
concepts like instantiation of buses like I2C and SPI. It seems like if
we do want to try to share between DT and ACPI we should be doing it at
that level rather than dealing with pinmuxing at the extremely low level
that pinctrl does.
Obviously for the more general ACPI use case the idiomatic way of
handling this is that the OS should never see anything about the
pin muxing. With DT we need to really know what's going on with the
pinbox because the model is that even for things built into a single
board the OS is responsible for managing the pins but that's really not
how ACPI is expected to work.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-04 21:40 ` [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Mark Brown
@ 2016-04-05 9:00 ` Linus Walleij
2016-04-05 16:12 ` Mark Brown
2016-04-05 12:51 ` Octavian Purdila
1 sibling, 1 reply; 41+ messages in thread
From: Linus Walleij @ 2016-04-05 9:00 UTC (permalink / raw)
To: Mark Brown
Cc: Irina Tirdea, Rafael J. Wysocki, Len Brown, Mika Westerberg,
linux-gpio, ACPI Devel Maling List, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Octavian Purdila, Cristina Ciocan, devicetree,
linux-kernel
On Mon, Apr 4, 2016 at 11:40 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
>
>> This is a proposal for adding ACPI support for pin controller
>> configuration.
>
>> It has been developed to enable the MinnowBoard and IoT community
>> by providing an easy way to specify pin multiplexing and
>> pin configuration.
>
> So this is mainly targeted at modules being added to base boards?
> Without getting into the binding at all here it seems like this is not
> solving the problem at the right abstraction level. It's exposing the
> pins on the SoC directly without any tie in with the functionality that
> goes over those pins. This means that any binding of a board to an ACPI
> using system that just uses this is going to be entirely specific to the
> particular combination of base and expansion board even if the
> electrical connections are standard.
I have seen the same need beyond strictly embedded (MinnowBoard)
from the Intel camp.
These chips with funny Atom-specific codenames (baytrail, cherryview,
broxton, sunrisepoint etc) are not just used for these IoT use cases
but also for e.g. laptops of the ChromeBook form factor, and the same
pin control needs arise there, just at a different cadence related to
product cycle.
I bet they also have funny product-specific kernel trees :(
> This is something that people are currently looking at for DT, there the
> discussion has been about defining the connectors as entities and hiding
> the details of the muxing on the SoC behind that along with higher level
> concepts like instantiation of buses like I2C and SPI. It seems like if
> we do want to try to share between DT and ACPI we should be doing it at
> that level rather than dealing with pinmuxing at the extremely low level
> that pinctrl does.
Agree: work is needed here. It is a big confusion, the whole model is
based around the configuration being pretty static as I recently
realized when just wanting to add a runtime-detected LCD panel
to a certain driver. No runtime patching of the DT or overlays or any
of the sort deliver what is really needed.
The only thing I heard which was actually doing something sensible
was when Matthew Garret once told that apple mice provide a
device tree fragment to the OS on how to handle it during bus
discovery.
I think that for random complex hotpluggable devices like what
greybus is trying to achieve this is needed too, despite the standard
USB-like device classes in all their glory.
> Obviously for the more general ACPI use case the idiomatic way of
> handling this is that the OS should never see anything about the
> pin muxing. With DT we need to really know what's going on with the
> pinbox because the model is that even for things built into a single
> board the OS is responsible for managing the pins but that's really not
> how ACPI is expected to work.
See my previous mail for some kind of answer to this.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-05 9:00 ` Linus Walleij
@ 2016-04-05 16:12 ` Mark Brown
0 siblings, 0 replies; 41+ messages in thread
From: Mark Brown @ 2016-04-05 16:12 UTC (permalink / raw)
To: Linus Walleij
Cc: Irina Tirdea, Rafael J. Wysocki, Len Brown, Mika Westerberg,
linux-gpio, ACPI Devel Maling List, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Octavian Purdila, Cristina Ciocan, devicetree,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2501 bytes --]
On Tue, Apr 05, 2016 at 11:00:50AM +0200, Linus Walleij wrote:
> On Mon, Apr 4, 2016 at 11:40 PM, Mark Brown <broonie@kernel.org> wrote:
> > So this is mainly targeted at modules being added to base boards?
> > Without getting into the binding at all here it seems like this is not
> > solving the problem at the right abstraction level. It's exposing the
> I have seen the same need beyond strictly embedded (MinnowBoard)
> from the Intel camp.
> These chips with funny Atom-specific codenames (baytrail, cherryview,
> broxton, sunrisepoint etc) are not just used for these IoT use cases
> but also for e.g. laptops of the ChromeBook form factor, and the same
> pin control needs arise there, just at a different cadence related to
> product cycle.
Right, the chips are used in a broader space but in cases where they're
being used in fixed systems where we don't have to deal with repaceable
modules then the idiomatic thing for ACPI is to hide all the pinmuxing
from the operating system.
> I bet they also have funny product-specific kernel trees :(
Yup, they do.
> Agree: work is needed here. It is a big confusion, the whole model is
> based around the configuration being pretty static as I recently
> realized when just wanting to add a runtime-detected LCD panel
> to a certain driver. No runtime patching of the DT or overlays or any
> of the sort deliver what is really needed.
Yes, funnily enough the CHIP people were just talking about that
specific case at ELC yesterday (they patched u-boot to parse overlays
so the kernel never sees the hotplug).
> The only thing I heard which was actually doing something sensible
> was when Matthew Garret once told that apple mice provide a
> device tree fragment to the OS on how to handle it during bus
> discovery.
That's what people are doing with the BeagleBone/RPi/CHIP/96boards case
- it's one of the primary usecases for DT overlays but currently
everyone's final integration layer is out of tree.
> > Obviously for the more general ACPI use case the idiomatic way of
> > handling this is that the OS should never see anything about the
> > pin muxing. With DT we need to really know what's going on with the
> > pinbox because the model is that even for things built into a single
> > board the OS is responsible for managing the pins but that's really not
> > how ACPI is expected to work.
> See my previous mail for some kind of answer to this.
Yup, will reply there.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-04 21:40 ` [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Mark Brown
2016-04-05 9:00 ` Linus Walleij
@ 2016-04-05 12:51 ` Octavian Purdila
2016-04-05 17:31 ` Mark Brown
1 sibling, 1 reply; 41+ messages in thread
From: Octavian Purdila @ 2016-04-05 12:51 UTC (permalink / raw)
To: Mark Brown
Cc: Irina Tirdea, Rafael J. Wysocki, Len Brown, Mika Westerberg,
Linus Walleij, linux-gpio, linux-acpi, Rob Herring,
Heikki Krogerus, Andy Shevchenko, Cristina Ciocan, devicetree,
lkml
On Tue, Apr 5, 2016 at 12:40 AM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
>
>> This is a proposal for adding ACPI support for pin controller
>> configuration.
>
>> It has been developed to enable the MinnowBoard and IoT community
>> by providing an easy way to specify pin multiplexing and
>> pin configuration.
>
> So this is mainly targeted at modules being added to base boards?
That is the main use case, yes. Velocity of platform
debugging/enabling is another one.
> Without getting into the binding at all here it seems like this is not
> solving the problem at the right abstraction level. It's exposing the
> pins on the SoC directly without any tie in with the functionality that
> goes over those pins.
This is not completely true. The pinctrl drivers are exposing
functionality in terms of function groups (e.g., i2c1_grp, spi1_grp,
pwm1_grp, etc.) It is not enough, but it is a step in the right
direction for standard bindings and for tools that can build on top of
this.
> This means that any binding of a board to an ACPI
> using system that just uses this is going to be entirely specific to the
> particular combination of base and expansion board even if the
> electrical connections are standard.
>
This can be solved by tools that work with high level abstractions and
generate this specific information.
> This is something that people are currently looking at for DT, there the
> discussion has been about defining the connectors as entities and hiding
> the details of the muxing on the SoC behind that along with higher level
> concepts like instantiation of buses like I2C and SPI. It seems like if
> we do want to try to share between DT and ACPI we should be doing it at
> that level rather than dealing with pinmuxing at the extremely low level
> that pinctrl does.
>
At some point we still need to poke registers. We already have a
drivers that do this (pinctrl) and a standard configuration interface
(the pinctrl device tree bindings). It seems natural to build on top
of this for those higher level concepts / tools.
> Obviously for the more general ACPI use case the idiomatic way of
> handling this is that the OS should never see anything about the
> pin muxing. With DT we need to really know what's going on with the
> pinbox because the model is that even for things built into a single
> board the OS is responsible for managing the pins but that's really not
> how ACPI is expected to work.
Maybe. But we already expose pin control / muxing in
drivers/pinctrl/intel, yes, read-only at this point. Very useful for
debugging. Having write access would make debugging even more useful,
not to mention fast prototyping.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-05 12:51 ` Octavian Purdila
@ 2016-04-05 17:31 ` Mark Brown
0 siblings, 0 replies; 41+ messages in thread
From: Mark Brown @ 2016-04-05 17:31 UTC (permalink / raw)
To: Octavian Purdila
Cc: Irina Tirdea, Rafael J. Wysocki, Len Brown, Mika Westerberg,
Linus Walleij, linux-gpio, linux-acpi, Rob Herring,
Heikki Krogerus, Andy Shevchenko, Cristina Ciocan, devicetree,
lkml, mark.rutland
[-- Attachment #1: Type: text/plain, Size: 2182 bytes --]
On Tue, Apr 05, 2016 at 03:51:18PM +0300, Octavian Purdila wrote:
> On Tue, Apr 5, 2016 at 12:40 AM, Mark Brown <broonie@kernel.org> wrote:
> > So this is mainly targeted at modules being added to base boards?
> That is the main use case, yes. Velocity of platform
> debugging/enabling is another one.
The speed thing sounds like someone needs to work on the tooling for
working on firmware TBH.
> > This means that any binding of a board to an ACPI
> > using system that just uses this is going to be entirely specific to the
> > particular combination of base and expansion board even if the
> > electrical connections are standard.
> This can be solved by tools that work with high level abstractions and
> generate this specific information.
Simply stating that there are in future going to be some higher level
things which make use of this firmware interface isn't altogether
reassuring when we're still in the process of solving the issues for the
DT version of this...
> > This is something that people are currently looking at for DT, there the
> > discussion has been about defining the connectors as entities and hiding
> > the details of the muxing on the SoC behind that along with higher level
> > concepts like instantiation of buses like I2C and SPI. It seems like if
> > we do want to try to share between DT and ACPI we should be doing it at
> > that level rather than dealing with pinmuxing at the extremely low level
> > that pinctrl does.
> At some point we still need to poke registers. We already have a
> drivers that do this (pinctrl) and a standard configuration interface
> (the pinctrl device tree bindings). It seems natural to build on top
> of this for those higher level concepts / tools.
Both DT and ACPI have a system model behind them and they're not the
stame system model. Importing one into the other directly without
carefully thinking through how they play nicely with each other seems
likely to lead to problems further down the line, you might know exactly
how you intend this to work but how do we make sure that a firmware
author in some system integrator knows about this and is able to safely
write firmware in a few years?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-03-31 11:44 [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Irina Tirdea
` (4 preceding siblings ...)
2016-04-04 21:40 ` [RFC PATCH 0/4] Add ACPI support for pinctrl configuration Mark Brown
@ 2016-04-04 22:52 ` Mark Rutland
2016-04-05 8:43 ` Linus Walleij
` (2 more replies)
5 siblings, 3 replies; 41+ messages in thread
From: Mark Rutland @ 2016-04-04 22:52 UTC (permalink / raw)
To: Irina Tirdea
Cc: Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
linux-gpio, linux-acpi, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Octavian Purdila, Cristina Ciocan, devicetree,
linux-kernel, charles.garcia-tobin
Hi,
On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
> This is a proposal for adding ACPI support for pin controller
> configuration.
>
> It has been developed to enable the MinnowBoard and IoT community
> by providing an easy way to specify pin multiplexing and
> pin configuration.
>
> This proposal is based on using _DSD properties to specify device
> states and configuration nodes and it follows closely the device
> tree model. Device states are defined using the Device Properties
> format and the configuration nodes are defined using the
> Hierarchical Properties Extension format. The generic properties
> for the configuration nodes are the same as the ones for device
> tree, while pincontroller drivers can also define custom ones.
>From a look of the Documentation addition, and of the current uses of
pinctrl-names in device tree bindings, one reason for requiring multiple
pinctrl states is power management. Given that, I'm somewhat concerned by this,
as it goes against the usual ACPI model of abstracting this sort of thing
behind power control methods.
To the best of my knowledge, that goes against the ASWG's expectations on how
_DSD will be used (per [1]). Charles, please correct me if that document is no
longer representative.
Additionally, pinctrl has some cross-cutting concerns (e.g. mutually exclusive
device usage due to a shared pin), and I can imagine that may interact poorly
with any AML or firmware assumptions about the state of the world, as there's
no mechanism present to notify those of changes to pins.
I think that this is a class of problem which needs to be raised with the ASWG,
and solved in an ACPI-native fashion, rather than simply copying properties
from DT using _DSD. If nothing else, the restrictions on FW and AML would need
to be specified.
Thanks,
Mark.
[1] https://lists.acpica.org/pipermail/dsd/2015-September/000019.html
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-04 22:52 ` Mark Rutland
@ 2016-04-05 8:43 ` Linus Walleij
2016-04-05 16:59 ` Mark Brown
2016-04-05 8:56 ` Charles Garcia-Tobin
2016-04-05 15:33 ` Tirdea, Irina
2 siblings, 1 reply; 41+ messages in thread
From: Linus Walleij @ 2016-04-05 8:43 UTC (permalink / raw)
To: Mark Rutland
Cc: Irina Tirdea, Rafael J. Wysocki, Len Brown, Mika Westerberg,
linux-gpio, ACPI Devel Maling List, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Octavian Purdila, Cristina Ciocan, devicetree,
linux-kernel, Charles Garcia-Tobin
On Tue, Apr 5, 2016 at 12:52 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
>> This proposal is based on using _DSD properties to specify device
>> states and configuration nodes and it follows closely the device
>> tree model. Device states are defined using the Device Properties
>> format and the configuration nodes are defined using the
>> Hierarchical Properties Extension format. The generic properties
>> for the configuration nodes are the same as the ones for device
>> tree, while pincontroller drivers can also define custom ones.
>
> From a look of the Documentation addition, and of the current uses of
> pinctrl-names in device tree bindings, one reason for requiring multiple
> pinctrl states is power management. Given that, I'm somewhat concerned by this,
> as it goes against the usual ACPI model of abstracting this sort of thing
> behind power control methods.
There are other use cases but in essence there are two common use
cases:
- Initial set-up of muxing and electrical properties
- Runtime reconfiguration for power saving (especially
biasing and pull options)
I did see that the ACPI people's ambition has been for all things
power save to be embedded into AML and the like.
However AFAICT
the development model for deployment of ACPI seems to imply that
products are shipped with ACPI tables resident in ROM-like storage,
and at product release several devices are disabled from muxing
while others at the same time are totally lacking power save
enablement.
And these products are only update-able
with hairy BIOS patches that need to be applied
using $SPECIAL_TOOL that "normal users" do not want to
concern themselves with, as this is not an "apt-get upgrade"
kind of thing.
And as this is server silicon the systems have a bunch of these
"normal users" that are not embedded development experts
and they are very hesitant about doing such things.
Under that scenario it is of course tempting to simply set up the
pins in a known good state and then remove
the responsibility of pin controlling from ACPI firmware altogether
and into the kernel, where "apt-get upgrade" will take care of
post-product launch upgrading of the functionality.
And I think that is what is happening, it's just that so much
prestige is involved that no-one wants to officially admit it.
I was once poking fun at this development model accusing
firmware engineers of having a God complex when they claim
to be able to engineer in these complex use cases during
product firmware development. Now I just feel sad about this
situation and want things to "just work" for them. I think these
patches are good.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-05 8:43 ` Linus Walleij
@ 2016-04-05 16:59 ` Mark Brown
2016-04-05 19:37 ` Octavian Purdila
0 siblings, 1 reply; 41+ messages in thread
From: Mark Brown @ 2016-04-05 16:59 UTC (permalink / raw)
To: Linus Walleij
Cc: Mark Rutland, Irina Tirdea, Rafael J. Wysocki, Len Brown,
Mika Westerberg, linux-gpio, ACPI Devel Maling List, Rob Herring,
Heikki Krogerus, Andy Shevchenko, Octavian Purdila,
Cristina Ciocan, devicetree, linux-kernel, Charles Garcia-Tobin
[-- Attachment #1: Type: text/plain, Size: 2395 bytes --]
On Tue, Apr 05, 2016 at 10:43:11AM +0200, Linus Walleij wrote:
> And these products are only update-able
> with hairy BIOS patches that need to be applied
> using $SPECIAL_TOOL that "normal users" do not want to
> concern themselves with, as this is not an "apt-get upgrade"
> kind of thing.
This bit has been addressed in UEFI - there's now a mechanism for the OS
to supply firmware updates to the BIOS via runtime sevices calls without
needing to go to the hairy tools. There's also been facilities for some
time which allow ACPI fragments to be loaded at runtime without patching
the firmware (though they're not used so often at present).
> And I think that is what is happening, it's just that so much
> prestige is involved that no-one wants to officially admit it.
> I was once poking fun at this development model accusing
> firmware engineers of having a God complex when they claim
> to be able to engineer in these complex use cases during
That's not really a fair characterization of the situation. The goal
that ACPI has been trying to meet is allowing new hardware to work
without needing software updates so people's installation experience
isn't miserable. The Windows monoculture that firmware developers have
been targetting has hurt the achievement of that but it's the idea.
It's a good model for some classes of device but not for all.
> product firmware development. Now I just feel sad about this
> situation and want things to "just work" for them. I think these
> patches are good.
See Mark Rutland's reply - there's a whole model behind how ACPI
abstracts the system that needs to be taken into account, just stuffing
the existing DT in through a mechanism intended for simple vendor
specific key/value properties isn't guaranteed to give something that's
coherent and sensible. DT design decisions will obviously not have
considered ACPI and exposing the full combination of the two system
models to system integrators seems likely to lead to unfortunate
corners, especially with things that happen to work right now but cause
problems later on.
Given the rush to shoehorn existing DT into ACPI at some point it's
looking like it would be much more sensible for x86 to just do what
arm64 has done and support both DT and ACPI in parallel and let people
(either system integrators or end users) choose the most appropriate
interface for their application.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-05 16:59 ` Mark Brown
@ 2016-04-05 19:37 ` Octavian Purdila
2016-04-05 22:44 ` Mark Brown
` (2 more replies)
0 siblings, 3 replies; 41+ messages in thread
From: Octavian Purdila @ 2016-04-05 19:37 UTC (permalink / raw)
To: Mark Brown
Cc: Linus Walleij, Mark Rutland, Irina Tirdea, Rafael J. Wysocki,
Len Brown, Mika Westerberg, linux-gpio, ACPI Devel Maling List,
Rob Herring, Heikki Krogerus, Andy Shevchenko, Cristina Ciocan,
devicetree, linux-kernel, Charles Garcia-Tobin
On Tue, Apr 5, 2016 at 7:59 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Apr 05, 2016 at 10:43:11AM +0200, Linus Walleij wrote:
>
>> And these products are only update-able
>> with hairy BIOS patches that need to be applied
>> using $SPECIAL_TOOL that "normal users" do not want to
>> concern themselves with, as this is not an "apt-get upgrade"
>> kind of thing.
>
> This bit has been addressed in UEFI - there's now a mechanism for the OS
> to supply firmware updates to the BIOS via runtime sevices calls without
> needing to go to the hairy tools.
AFAIK there is still no standard way of updating just the ACPI tables.
You need various tools to "stitch" a full firmware image that can be
consumed by the update mechanism.
> There's also been facilities for some
> time which allow ACPI fragments to be loaded at runtime without patching
> the firmware (though they're not used so often at present).
And I just sent a separate patch series for "ACPI overlays", similar
with the dynamic DT, with the primary goal of supporting open-ended
hardware configurations.
But to be able to effectively use this we need a way to change pinmux settings.
>> And I think that is what is happening, it's just that so much
>> prestige is involved that no-one wants to officially admit it.
>
>> I was once poking fun at this development model accusing
>> firmware engineers of having a God complex when they claim
>> to be able to engineer in these complex use cases during
>
> That's not really a fair characterization of the situation. The goal
> that ACPI has been trying to meet is allowing new hardware to work
> without needing software updates so people's installation experience
> isn't miserable. The Windows monoculture that firmware developers have
> been targetting has hurt the achievement of that but it's the idea.
> It's a good model for some classes of device but not for all.
>
>> product firmware development. Now I just feel sad about this
>> situation and want things to "just work" for them. I think these
>> patches are good.
>
> See Mark Rutland's reply - there's a whole model behind how ACPI
> abstracts the system that needs to be taken into account, just stuffing
> the existing DT in through a mechanism intended for simple vendor
> specific key/value properties isn't guaranteed to give something that's
> coherent and sensible. DT design decisions will obviously not have
> considered ACPI and exposing the full combination of the two system
> models to system integrators seems likely to lead to unfortunate
> corners, especially with things that happen to work right now but cause
> problems later on.
>
> Given the rush to shoehorn existing DT into ACPI at some point it's
> looking like it would be much more sensible for x86 to just do what
> arm64 has done and support both DT and ACPI in parallel and let people
> (either system integrators or end users) choose the most appropriate
> interface for their application.
Lets look at this from a different perspective. The proposal is not
about importing the DT model into ACPI but importing the Linux pinctrl
model into ACPI. That will allow us to use the Linux pinctrl drivers
to their full potential.
That doesn't stop the development of other, more OS independent, ACPI
models for pinmuxing. Which we can also support.
I know that there are some discussions for pinmux configuration in the
ASWG, but it does not match the Linux pinctrl model. So we will end up
with a pinctrl driver that offers groups, functions and pin names and
a totally different ACPI description that we can't map to the pinctrl
driver.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-05 19:37 ` Octavian Purdila
@ 2016-04-05 22:44 ` Mark Brown
2016-04-05 23:48 ` Al Stone
2016-04-06 8:52 ` Lorenzo Pieralisi
2 siblings, 0 replies; 41+ messages in thread
From: Mark Brown @ 2016-04-05 22:44 UTC (permalink / raw)
To: Octavian Purdila
Cc: Linus Walleij, Mark Rutland, Irina Tirdea, Rafael J. Wysocki,
Len Brown, Mika Westerberg, linux-gpio, ACPI Devel Maling List,
Rob Herring, Heikki Krogerus, Andy Shevchenko, Cristina Ciocan,
devicetree, linux-kernel, Charles Garcia-Tobin
[-- Attachment #1: Type: text/plain, Size: 3686 bytes --]
On Tue, Apr 05, 2016 at 10:37:14PM +0300, Octavian Purdila wrote:
> On Tue, Apr 5, 2016 at 7:59 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Tue, Apr 05, 2016 at 10:43:11AM +0200, Linus Walleij wrote:
> >> And these products are only update-able
> >> with hairy BIOS patches that need to be applied
> >> using $SPECIAL_TOOL that "normal users" do not want to
> >> concern themselves with, as this is not an "apt-get upgrade"
> >> kind of thing.
> > This bit has been addressed in UEFI - there's now a mechanism for the OS
> > to supply firmware updates to the BIOS via runtime sevices calls without
> > needing to go to the hairy tools.
> AFAIK there is still no standard way of updating just the ACPI tables.
> You need various tools to "stitch" a full firmware image that can be
> consumed by the update mechanism.
Right, but in terms of end users that doesn't matter - the users that
Linus is concerned with here aren't going to know what ACPI is.
> > There's also been facilities for some
> > time which allow ACPI fragments to be loaded at runtime without patching
> > the firmware (though they're not used so often at present).
> And I just sent a separate patch series for "ACPI overlays", similar
> with the dynamic DT, with the primary goal of supporting open-ended
> hardware configurations.
> But to be able to effectively use this we need a way to change pinmux settings.
I'll need to find that series I guess...
Note that we currently don't have any actual mechanism to load a DT
overlay in mainline. One of the issues there is making sure that the
external interfaces we're offering are actually stable, another is
making sure (for safety) that the overlays are only updating bits of the
device tree that corrspond to the bit of the hardware that the overlay
is for.
> > Given the rush to shoehorn existing DT into ACPI at some point it's
> > looking like it would be much more sensible for x86 to just do what
> > arm64 has done and support both DT and ACPI in parallel and let people
> > (either system integrators or end users) choose the most appropriate
> > interface for their application.
> Lets look at this from a different perspective. The proposal is not
> about importing the DT model into ACPI but importing the Linux pinctrl
It's not like this is the only change that has been sent to reuse some
DT element rather directly in ACPI recently...
> model into ACPI. That will allow us to use the Linux pinctrl drivers
> to their full potential.
> That doesn't stop the development of other, more OS independent, ACPI
> models for pinmuxing. Which we can also support.
That's not really a great answer. Regardless of how good a solution
this is for handling pinmuxing ACPI it needs to be joined up with the
rest of ACPI, we need some clear instructions for both firmware and OS
authors about how this works in a wider ACPI context and interacts with
other features, preferably in the ACPI specs where people can find them.
It could be something really simple like just saying that if we've got
these pinctrl definitions then the firmware is not allowed to do any pin
management, or it could be something more involved like saying that the
OS has to notify the OS before using them.
> I know that there are some discussions for pinmux configuration in the
> ASWG, but it does not match the Linux pinctrl model. So we will end up
> with a pinctrl driver that offers groups, functions and pin names and
> a totally different ACPI description that we can't map to the pinctrl
> driver.
If there's an ASWG spec in progress that explicitly overlaps it seems
even more important that this is joined up - I'm not aware of what's
going on there.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-05 19:37 ` Octavian Purdila
2016-04-05 22:44 ` Mark Brown
@ 2016-04-05 23:48 ` Al Stone
2016-04-06 8:52 ` Lorenzo Pieralisi
2 siblings, 0 replies; 41+ messages in thread
From: Al Stone @ 2016-04-05 23:48 UTC (permalink / raw)
To: Octavian Purdila, Mark Brown
Cc: Linus Walleij, Mark Rutland, Irina Tirdea, Rafael J. Wysocki,
Len Brown, Mika Westerberg, linux-gpio, ACPI Devel Maling List,
Rob Herring, Heikki Krogerus, Andy Shevchenko, Cristina Ciocan,
devicetree, linux-kernel, Charles Garcia-Tobin
On 04/05/2016 01:37 PM, Octavian Purdila wrote:
> On Tue, Apr 5, 2016 at 7:59 PM, Mark Brown <broonie@kernel.org> wrote:
>> On Tue, Apr 05, 2016 at 10:43:11AM +0200, Linus Walleij wrote:
>>[snip...]
>
> I know that there are some discussions for pinmux configuration in the
> ASWG, but it does not match the Linux pinctrl model. So we will end up
> with a pinctrl driver that offers groups, functions and pin names and
> a totally different ACPI description that we can't map to the pinctrl
> driver.
Um, as a member of the ASWG, this is the first I've heard of this discussion.
And I do attend the meetings very regularly. Please send me an issue number
off-line because I'm not finding it in the current issue list.
Secondly, if this is being discussed within the ASWG, please refer to the UEFI
member agreements about disclosing information prior to it being approved by
the forum; there are limits to such disclosures, and a public mailing list may
be well past those limits.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-05 19:37 ` Octavian Purdila
2016-04-05 22:44 ` Mark Brown
2016-04-05 23:48 ` Al Stone
@ 2016-04-06 8:52 ` Lorenzo Pieralisi
2 siblings, 0 replies; 41+ messages in thread
From: Lorenzo Pieralisi @ 2016-04-06 8:52 UTC (permalink / raw)
To: Octavian Purdila
Cc: Mark Brown, Linus Walleij, Mark Rutland, Irina Tirdea,
Rafael J. Wysocki, Len Brown, Mika Westerberg, linux-gpio,
ACPI Devel Maling List, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Cristina Ciocan, devicetree, linux-kernel,
Charles Garcia-Tobin
On Tue, Apr 05, 2016 at 10:37:14PM +0300, Octavian Purdila wrote:
> On Tue, Apr 5, 2016 at 7:59 PM, Mark Brown <broonie@kernel.org> wrote:
[...]
> Lets look at this from a different perspective. The proposal is not
> about importing the DT model into ACPI but importing the Linux pinctrl
> model into ACPI. That will allow us to use the Linux pinctrl drivers
> to their full potential.
Yes we understood that, and in the process you are bypassing the eg ACPI
power management model completely, but since all you are after is using
the Linux pinctrl kernel driver with ACPI _today_ without going through
the ASWG (and without booting with a device tree instead of ACPI) and
define a specification that has a chance to co-exist with the ACPI power
management model this proposal is the end result, it is a shortcut fraught
with problems.
> That doesn't stop the development of other, more OS independent, ACPI
> models for pinmuxing. Which we can also support.
>
> I know that there are some discussions for pinmux configuration in the
> ASWG, but it does not match the Linux pinctrl model. So we will end up
> with a pinctrl driver that offers groups, functions and pin names and
> a totally different ACPI description that we can't map to the pinctrl
> driver.
If you know that there are some discussions please take place in those
discussions and work towards a solution that takes into account
other parts of ACPI specifications that can be affected, it may
take longer to get you there but that's true for everyone who
wants to contribute to ACPI specifications I am afraid.
Thank you,
Lorenzo
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-04 22:52 ` Mark Rutland
2016-04-05 8:43 ` Linus Walleij
@ 2016-04-05 8:56 ` Charles Garcia-Tobin
2016-04-06 0:00 ` Al Stone
2016-04-05 15:33 ` Tirdea, Irina
2 siblings, 1 reply; 41+ messages in thread
From: Charles Garcia-Tobin @ 2016-04-05 8:56 UTC (permalink / raw)
To: Mark Rutland, Irina Tirdea
Cc: Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
linux-gpio, linux-acpi, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Octavian Purdila, Cristina Ciocan, devicetree,
linux-kernel
On 04/04/2016 23:52, "Mark Rutland" <mark.rutland@arm.com> wrote:
>Hi,
>
>On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
>> This is a proposal for adding ACPI support for pin controller
>> configuration.
>>
>> It has been developed to enable the MinnowBoard and IoT community
>> by providing an easy way to specify pin multiplexing and
>> pin configuration.
>>
>> This proposal is based on using _DSD properties to specify device
>> states and configuration nodes and it follows closely the device
>> tree model. Device states are defined using the Device Properties
>> format and the configuration nodes are defined using the
>> Hierarchical Properties Extension format. The generic properties
>> for the configuration nodes are the same as the ones for device
>> tree, while pincontroller drivers can also define custom ones.
>
>From a look of the Documentation addition, and of the current uses of
>pinctrl-names in device tree bindings, one reason for requiring multiple
>pinctrl states is power management. Given that, I'm somewhat concerned by
>this,
>as it goes against the usual ACPI model of abstracting this sort of thing
>behind power control methods.
>
>To the best of my knowledge, that goes against the ASWG's expectations on
>how
>_DSD will be used (per [1]). Charles, please correct me if that document
>is no
>longer representative.
It is though latest version was posted by Rafael a bit later:
https://lists.acpica.org/pipermail/dsd/2015-December/000027.html
In addition the core rules requiring that existing ACPI paradigms are not
subverted through DSD (basically the concern you express) are also
documented in the main DSD documentation itself:
http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UU
ID.pdf (section 2.3)
Cheers
Charles
>
>Additionally, pinctrl has some cross-cutting concerns (e.g. mutually
>exclusive
>device usage due to a shared pin), and I can imagine that may interact
>poorly
>with any AML or firmware assumptions about the state of the world, as
>there's
>no mechanism present to notify those of changes to pins.
>
>I think that this is a class of problem which needs to be raised with the
>ASWG,
>and solved in an ACPI-native fashion, rather than simply copying
>properties
>from DT using _DSD. If nothing else, the restrictions on FW and AML would
>need
>to be specified.
>
>Thanks,
>Mark.
>
>[1] https://lists.acpica.org/pipermail/dsd/2015-September/000019.html
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-05 8:56 ` Charles Garcia-Tobin
@ 2016-04-06 0:00 ` Al Stone
2016-04-06 10:49 ` Graeme Gregory
0 siblings, 1 reply; 41+ messages in thread
From: Al Stone @ 2016-04-06 0:00 UTC (permalink / raw)
To: Charles Garcia-Tobin, Mark Rutland, Irina Tirdea
Cc: Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
linux-gpio, linux-acpi, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Octavian Purdila, Cristina Ciocan, devicetree,
linux-kernel
On 04/05/2016 02:56 AM, Charles Garcia-Tobin wrote:
>
>
> On 04/04/2016 23:52, "Mark Rutland" <mark.rutland@arm.com> wrote:
>
>> Hi,
>>
>> On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
>>> This is a proposal for adding ACPI support for pin controller
>>> configuration.
>>>
>>> It has been developed to enable the MinnowBoard and IoT community
>>> by providing an easy way to specify pin multiplexing and
>>> pin configuration.
>>>
>>> This proposal is based on using _DSD properties to specify device
>>> states and configuration nodes and it follows closely the device
>>> tree model. Device states are defined using the Device Properties
>>> format and the configuration nodes are defined using the
>>> Hierarchical Properties Extension format. The generic properties
>>> for the configuration nodes are the same as the ones for device
>>> tree, while pincontroller drivers can also define custom ones.
>>
>>From a look of the Documentation addition, and of the current uses of
>> pinctrl-names in device tree bindings, one reason for requiring multiple
>> pinctrl states is power management. Given that, I'm somewhat concerned by
>> this,
>> as it goes against the usual ACPI model of abstracting this sort of thing
>> behind power control methods.
>>
>> To the best of my knowledge, that goes against the ASWG's expectations on
>> how
>> _DSD will be used (per [1]). Charles, please correct me if that document
>> is no
>> longer representative.
>
> It is though latest version was posted by Rafael a bit later:
> https://lists.acpica.org/pipermail/dsd/2015-December/000027.html
>
>
> In addition the core rules requiring that existing ACPI paradigms are not
> subverted through DSD (basically the concern you express) are also
> documented in the main DSD documentation itself:
> http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UU
> ID.pdf (section 2.3)
I have to echo Mark's concern: wholesale importation of portions of current
DT bindings simply because it's expedient is one of the things I've been hoping
to avoid. These patches seem to be just that.
And while the latest version mentioned above describes a bit of a review
process to handle this case, I don't recall the kernel community at large
agreeing to it, nor to it having been implemented. If I missed that part,
my apologies; please let me know where it was decided. If I haven't, then
perhaps this needs to be the first test case of that process.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-06 0:00 ` Al Stone
@ 2016-04-06 10:49 ` Graeme Gregory
2016-04-07 14:17 ` Octavian Purdila
0 siblings, 1 reply; 41+ messages in thread
From: Graeme Gregory @ 2016-04-06 10:49 UTC (permalink / raw)
To: ahs3
Cc: Charles Garcia-Tobin, Mark Rutland, Irina Tirdea,
Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
linux-gpio, linux-acpi, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Octavian Purdila, Cristina Ciocan, devicetree,
linux-kernel
On Tue, Apr 05, 2016 at 06:00:31PM -0600, Al Stone wrote:
> On 04/05/2016 02:56 AM, Charles Garcia-Tobin wrote:
> >
> >
> > On 04/04/2016 23:52, "Mark Rutland" <mark.rutland@arm.com> wrote:
> >
> >> Hi,
> >>
> >> On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
> >>> This is a proposal for adding ACPI support for pin controller
> >>> configuration.
> >>>
> >>> It has been developed to enable the MinnowBoard and IoT community
> >>> by providing an easy way to specify pin multiplexing and
> >>> pin configuration.
> >>>
> >>> This proposal is based on using _DSD properties to specify device
> >>> states and configuration nodes and it follows closely the device
> >>> tree model. Device states are defined using the Device Properties
> >>> format and the configuration nodes are defined using the
> >>> Hierarchical Properties Extension format. The generic properties
> >>> for the configuration nodes are the same as the ones for device
> >>> tree, while pincontroller drivers can also define custom ones.
> >>
> >>From a look of the Documentation addition, and of the current uses of
> >> pinctrl-names in device tree bindings, one reason for requiring multiple
> >> pinctrl states is power management. Given that, I'm somewhat concerned by
> >> this,
> >> as it goes against the usual ACPI model of abstracting this sort of thing
> >> behind power control methods.
> >>
> >> To the best of my knowledge, that goes against the ASWG's expectations on
> >> how
> >> _DSD will be used (per [1]). Charles, please correct me if that document
> >> is no
> >> longer representative.
> >
> > It is though latest version was posted by Rafael a bit later:
> > https://lists.acpica.org/pipermail/dsd/2015-December/000027.html
> >
> >
> > In addition the core rules requiring that existing ACPI paradigms are not
> > subverted through DSD (basically the concern you express) are also
> > documented in the main DSD documentation itself:
> > http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UU
> > ID.pdf (section 2.3)
>
> I have to echo Mark's concern: wholesale importation of portions of current
> DT bindings simply because it's expedient is one of the things I've been hoping
> to avoid. These patches seem to be just that.
>
> And while the latest version mentioned above describes a bit of a review
> process to handle this case, I don't recall the kernel community at large
> agreeing to it, nor to it having been implemented. If I missed that part,
> my apologies; please let me know where it was decided. If I haven't, then
> perhaps this needs to be the first test case of that process.
>
My concern over this is similar to Mark/Als this looks like a quick hack
to "solve" an issue that has been worked on in Linux for at least 10
years now. And thats how to describe a non descoverable card that is
plugged into random busses on a SoC.
The issue I see with a quick hack of DT pinctrl into _DSD is that this
does not take into account the power model of ACPI. As far as I can see
the core code has no manner to tell a pin/function allocated through
_DSD actually has effects on the power of other devices. So the core
could end up powering down devices when it should not. This is very
relevent to your intended use of IoT devices where power control is key.
Why has there been no attempt in ASWG to make these sort of features a
1st class citizen of ACPI so they can interact correctly with the other
features?
Thanks
Graeme
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-06 10:49 ` Graeme Gregory
@ 2016-04-07 14:17 ` Octavian Purdila
2016-04-07 18:01 ` Linus Walleij
0 siblings, 1 reply; 41+ messages in thread
From: Octavian Purdila @ 2016-04-07 14:17 UTC (permalink / raw)
To: Graeme Gregory
Cc: ahs3, Charles Garcia-Tobin, Mark Rutland, Irina Tirdea,
Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
linux-gpio, linux-acpi, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Cristina Ciocan, devicetree, linux-kernel
On Wed, Apr 6, 2016 at 1:49 PM, Graeme Gregory <gg@slimlogic.co.uk> wrote:
>>
>> I have to echo Mark's concern: wholesale importation of portions of current
>> DT bindings simply because it's expedient is one of the things I've been hoping
>> to avoid. These patches seem to be just that.
>>
>> And while the latest version mentioned above describes a bit of a review
>> process to handle this case, I don't recall the kernel community at large
>> agreeing to it, nor to it having been implemented. If I missed that part,
>> my apologies; please let me know where it was decided. If I haven't, then
>> perhaps this needs to be the first test case of that process.
>>
>
> My concern over this is similar to Mark/Als this looks like a quick hack
> to "solve" an issue that has been worked on in Linux for at least 10
> years now. And thats how to describe a non descoverable card that is
> plugged into random busses on a SoC.
>
> The issue I see with a quick hack of DT pinctrl into _DSD is that this
> does not take into account the power model of ACPI. As far as I can see
> the core code has no manner to tell a pin/function allocated through
> _DSD actually has effects on the power of other devices. So the core
> could end up powering down devices when it should not. This is very
> relevent to your intended use of IoT devices where power control is key.
>
It is not clear to me how this could happen - core powering down
devices when it should not, unless pins are shared. And if I am not
missing something, pinctrl is designed to handle this case by only by
switching to idle/suspend/default states from the controller drivers
in the suspend/resume routine. ACPI _Sx calls also happens at the same
time so there does not seem to be a contention point.
Perhaps I am missing something and an example of such issue would make
it more clear.
And, as our primary use case is to set the initial muxing, we can
easily remove the suspend/resume functionality if this turns out to be
the issue.
> Why has there been no attempt in ASWG to make these sort of features a
> 1st class citizen of ACPI so they can interact correctly with the other
> features?
>
IMO having an ASWG pinctrl specification and using _DSD for Linux are
orthogonal approaches. The latter is very specific to Linux and we
want it so that we can support the Linux pinctrl model as best as
possible and I doubt it is a common denominator for all OSes that ACPI
supports.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-07 14:17 ` Octavian Purdila
@ 2016-04-07 18:01 ` Linus Walleij
0 siblings, 0 replies; 41+ messages in thread
From: Linus Walleij @ 2016-04-07 18:01 UTC (permalink / raw)
To: Octavian Purdila
Cc: Graeme Gregory, ahs3, Charles Garcia-Tobin, Mark Rutland,
Irina Tirdea, Rafael J. Wysocki, Len Brown, Mika Westerberg,
linux-gpio, linux-acpi, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Cristina Ciocan, devicetree, linux-kernel
On Thu, Apr 7, 2016 at 4:17 PM, Octavian Purdila
<octavian.purdila@intel.com> wrote:
> On Wed, Apr 6, 2016 at 1:49 PM, Graeme Gregory <gg@slimlogic.co.uk> wrote:
>> Why has there been no attempt in ASWG to make these sort of features a
>> 1st class citizen of ACPI so they can interact correctly with the other
>> features?
>
> IMO having an ASWG pinctrl specification and using _DSD for Linux are
> orthogonal approaches. The latter is very specific to Linux and we
> want it so that we can support the Linux pinctrl model as best as
> possible and I doubt it is a common denominator for all OSes that ACPI
> supports.
Sob, I tried so hard to implement pin control referring to generic
concepts that everyone should be able to reuse. E.g. stressing SI
units to be used all over the place, neutral scientific terms for all
config options and what not.
The multiplexing with groups and functions is a simple (aehm,
OK, super-complicated) matter of mapping disjunct sets onto each
other, so that comes from logic.
Not even the GPIO ranges that map GPIOs to pin ranges are very
Linux-specific: it is a property of the hardware that sometimes
GPIO is "behind" the pin, in a distinct hardware unit. (See pinctrl.txt
for the GPIO pitfalls.)
If I was to rewrite pin control and GPIO from scratch I would make it
one subsystem instead of two. This is more of an architectural
choice. GPIO would still be a stand-alone part of it.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 41+ messages in thread
* RE: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-04 22:52 ` Mark Rutland
2016-04-05 8:43 ` Linus Walleij
2016-04-05 8:56 ` Charles Garcia-Tobin
@ 2016-04-05 15:33 ` Tirdea, Irina
2016-04-05 18:16 ` Mark Rutland
2 siblings, 1 reply; 41+ messages in thread
From: Tirdea, Irina @ 2016-04-05 15:33 UTC (permalink / raw)
To: Mark Rutland
Cc: Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
linux-gpio, linux-acpi, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Purdila, Octavian, Ciocan, Cristina, devicetree,
linux-kernel, charles.garcia-tobin
> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@arm.com]
> Sent: 05 April, 2016 1:52
> To: Tirdea, Irina
> Cc: Rafael J. Wysocki; Len Brown; Mika Westerberg; Linus Walleij; linux-gpio@vger.kernel.org; linux-acpi@vger.kernel.org; Rob
> Herring; Heikki Krogerus; Andy Shevchenko; Purdila, Octavian; Ciocan, Cristina; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; charles.garcia-tobin@arm.com
> Subject: Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
>
> Hi,
Hi Mark,
>
> On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
> > This is a proposal for adding ACPI support for pin controller
> > configuration.
> >
> > It has been developed to enable the MinnowBoard and IoT community
> > by providing an easy way to specify pin multiplexing and
> > pin configuration.
> >
> > This proposal is based on using _DSD properties to specify device
> > states and configuration nodes and it follows closely the device
> > tree model. Device states are defined using the Device Properties
> > format and the configuration nodes are defined using the
> > Hierarchical Properties Extension format. The generic properties
> > for the configuration nodes are the same as the ones for device
> > tree, while pincontroller drivers can also define custom ones.
>
> From a look of the Documentation addition, and of the current uses of
> pinctrl-names in device tree bindings, one reason for requiring multiple
> pinctrl states is power management. Given that, I'm somewhat concerned by this,
> as it goes against the usual ACPI model of abstracting this sort of thing
> behind power control methods.
>
Right, there is an overlap of the pinctrl "sleep" state with the ACPI power
management model.
However, the main reason for implementing this is setting initial pin multiplexing
and configuration. This is normally done by BIOS, but there is currently no way of
changing the default configuration (except for a BIOS update). This is a problem
when using boards like MinnowBoard, where it is expected to get the board and
to be able to add various devices to its exported GPIO pins. We need a way to
change default pin multiplexing to enable the functionality required by a specific
device. In some cases we also need to set the bias to get things like interrupts working.
Another use case for pinctrl states is using custom reset pin configurations that need
to be controlled from the driver.
Since we have the pinctrl infrastructure and the all Intel ACPI pinctrl drivers already
support it, this seems the most straightforward way to implement it.
> To the best of my knowledge, that goes against the ASWG's expectations on how
> _DSD will be used (per [1]). Charles, please correct me if that document is no
> longer representative.
>
> Additionally, pinctrl has some cross-cutting concerns (e.g. mutually exclusive
> device usage due to a shared pin), and I can imagine that may interact poorly
> with any AML or firmware assumptions about the state of the world, as there's
> no mechanism present to notify those of changes to pins.
>
This is a problem with any modification we would make to the ACPI tables.
It is true that pinctrl configuration could create conflicts when changing
pin multiplexing, but that could be avoided by using only the pinctrl states that
make sense for the entire ACPI configuration.
> I think that this is a class of problem which needs to be raised with the ASWG,
> and solved in an ACPI-native fashion, rather than simply copying properties
> from DT using _DSD. If nothing else, the restrictions on FW and AML would need
> to be specified.
I agree this should be at least mentioned in the Documentation. I'll update it accordingly.
Thanks,
Irina
>
> Thanks,
> Mark.
>
> [1] https://lists.acpica.org/pipermail/dsd/2015-September/000019.html
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-05 15:33 ` Tirdea, Irina
@ 2016-04-05 18:16 ` Mark Rutland
2016-04-05 20:09 ` Octavian Purdila
0 siblings, 1 reply; 41+ messages in thread
From: Mark Rutland @ 2016-04-05 18:16 UTC (permalink / raw)
To: Tirdea, Irina
Cc: Rafael J. Wysocki, Len Brown, Mika Westerberg, Linus Walleij,
linux-gpio, linux-acpi, Rob Herring, Heikki Krogerus,
Andy Shevchenko, Purdila, Octavian, Ciocan, Cristina, devicetree,
linux-kernel, charles.garcia-tobin
On Tue, Apr 05, 2016 at 03:33:43PM +0000, Tirdea, Irina wrote:
> > -----Original Message-----
> > From: Mark Rutland [mailto:mark.rutland@arm.com]
> > On Thu, Mar 31, 2016 at 02:44:41PM +0300, Irina Tirdea wrote:
> > > This is a proposal for adding ACPI support for pin controller
> > > configuration.
> > >
> > > It has been developed to enable the MinnowBoard and IoT community
> > > by providing an easy way to specify pin multiplexing and
> > > pin configuration.
> > >
> > > This proposal is based on using _DSD properties to specify device
> > > states and configuration nodes and it follows closely the device
> > > tree model. Device states are defined using the Device Properties
> > > format and the configuration nodes are defined using the
> > > Hierarchical Properties Extension format. The generic properties
> > > for the configuration nodes are the same as the ones for device
> > > tree, while pincontroller drivers can also define custom ones.
> >
> > From a look of the Documentation addition, and of the current uses of
> > pinctrl-names in device tree bindings, one reason for requiring multiple
> > pinctrl states is power management. Given that, I'm somewhat concerned by this,
> > as it goes against the usual ACPI model of abstracting this sort of thing
> > behind power control methods.
> >
>
> Right, there is an overlap of the pinctrl "sleep" state with the ACPI power
> management model.
>
> However, the main reason for implementing this is setting initial pin multiplexing
> and configuration. This is normally done by BIOS, but there is currently no way of
> changing the default configuration (except for a BIOS update). This is a problem
> when using boards like MinnowBoard, where it is expected to get the board and
> to be able to add various devices to its exported GPIO pins.
In the absence of a BIOS update, how is it expected that the relevant pinctrl
settings for a given device will be known? Does the device provide ACPI
fragments like an SSDT? Does it simply identify itself in some manner, and
leave the rest to the kernel? Is this entirely user-driven?
> We need a way to change default pin multiplexing to enable the functionality
> required by a specific device. In some cases we also need to set the bias to
> get things like interrupts working.
>
> Another use case for pinctrl states is using custom reset pin configurations that need
> to be controlled from the driver.
To be clear, I'm not stating that having pinctrl under the OS is necessarily
wrong, and I can see why the firmware may not have all the relevant knowledge
in advance. I can certainly see why having the OS in control can be preferable.
My concern is that there is a conflict with the ACPI model, and potential
fragility, given that:
* The firmware does not have the relevant information in advance for a given
device that may be connected (i.e. how devices may change the pinctrl setup
is unknown).
* The firmware is to some extent expected to manage pinctrl today (for power
management of devices it does know about), and hence a pinctrl device is
potentially under shared management of ACPI and the OS.
* The ACPI specification says nothing about this style of pinctrl management,
so it is unclear what the expectations are:
- Is a given pinctrl device under shared ownership by the firmware and
kernel, or is a given device entirely under the control of just one?
- How shared access to the pinctrl device is mediated, e.g. is any locking or
signalling mechanism required to ensure that firmware and kernel do not
access the device simultaneously in a manner that causes problems.
- Is the firmware permitted to perform power management of devices for which
the kernel handles pinctrl? What states can either expect, and when is such
management permitted? e.g. must the OS ensure that a device is in its
default state? Can it only call power management calls from particular
states? What is the restored state?
- What the expectations are w.r.t. ownership of pins, e.g. must the firmware
never change the state of certain pins? Must it save/restore their state in
system-wide power-management scenarios like suspend or hibernate?
I think this needs to be raised with the ASWG, and some level of model needs to
be defined for this, to cater for the issues raised above.
That might be very simple, e.g. pins are never shared, the pinctrl management
device must permit concurrent accesses by FW and kernel, the kernel is
responsible for all management of those pins after system reset.
In the absence of that, this is liable to become fragmented and fragile, and is
practically impossible to rectify post-hoc.
> Since we have the pinctrl infrastructure and the all Intel ACPI pinctrl drivers already
> support it, this seems the most straightforward way to implement it.
I do not disagree that this is expedient. I am concerned that it permits a very
large set of avoidable problems, and I believe that we need to be somewhat
stricter from the outset.
> > Additionally, pinctrl has some cross-cutting concerns (e.g. mutually exclusive
> > device usage due to a shared pin), and I can imagine that may interact poorly
> > with any AML or firmware assumptions about the state of the world, as there's
> > no mechanism present to notify those of changes to pins.
>
> This is a problem with any modification we would make to the ACPI tables.
> It is true that pinctrl configuration could create conflicts when changing
> pin multiplexing, but that could be avoided by using only the pinctrl states that
> make sense for the entire ACPI configuration.
I think that "makes sense for the entire ACPI configuration" needs to be
defined, and then mandated, which implies an ACPI spec addition.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-05 18:16 ` Mark Rutland
@ 2016-04-05 20:09 ` Octavian Purdila
2016-04-06 0:01 ` Mark Rutland
0 siblings, 1 reply; 41+ messages in thread
From: Octavian Purdila @ 2016-04-05 20:09 UTC (permalink / raw)
To: Mark Rutland
Cc: Tirdea, Irina, Rafael J. Wysocki, Len Brown, Mika Westerberg,
Linus Walleij, linux-gpio, linux-acpi, Rob Herring,
Heikki Krogerus, Andy Shevchenko, Ciocan, Cristina, devicetree,
linux-kernel, charles.garcia-tobin
On Tue, Apr 5, 2016 at 9:16 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>>
>> Right, there is an overlap of the pinctrl "sleep" state with the ACPI power
>> management model.
>>
>> However, the main reason for implementing this is setting initial pin multiplexing
>> and configuration. This is normally done by BIOS, but there is currently no way of
>> changing the default configuration (except for a BIOS update). This is a problem
>> when using boards like MinnowBoard, where it is expected to get the board and
>> to be able to add various devices to its exported GPIO pins.
>
> In the absence of a BIOS update, how is it expected that the relevant pinctrl
> settings for a given device will be known? Does the device provide ACPI
> fragments like an SSDT? Does it simply identify itself in some manner, and
> leave the rest to the kernel? Is this entirely user-driven?
>
See this patch set I just proposed: https://lkml.org/lkml/2016/3/31/334
>> We need a way to change default pin multiplexing to enable the functionality
>> required by a specific device. In some cases we also need to set the bias to
>> get things like interrupts working.
>>
>> Another use case for pinctrl states is using custom reset pin configurations that need
>> to be controlled from the driver.
>
> To be clear, I'm not stating that having pinctrl under the OS is necessarily
> wrong, and I can see why the firmware may not have all the relevant knowledge
> in advance. I can certainly see why having the OS in control can be preferable.
>
> My concern is that there is a conflict with the ACPI model, and potential
> fragility, given that:
>
> * The firmware does not have the relevant information in advance for a given
> device that may be connected (i.e. how devices may change the pinctrl setup
> is unknown).
>
> * The firmware is to some extent expected to manage pinctrl today (for power
> management of devices it does know about), and hence a pinctrl device is
> potentially under shared management of ACPI and the OS.
>
> * The ACPI specification says nothing about this style of pinctrl management,
> so it is unclear what the expectations are:
>
Does it say anything at all about pinctrl management?
> - Is a given pinctrl device under shared ownership by the firmware and
> kernel, or is a given device entirely under the control of just one?
>
> - How shared access to the pinctrl device is mediated, e.g. is any locking or
> signalling mechanism required to ensure that firmware and kernel do not
> access the device simultaneously in a manner that causes problems.
>
> - Is the firmware permitted to perform power management of devices for which
> the kernel handles pinctrl? What states can either expect, and when is such
> management permitted? e.g. must the OS ensure that a device is in its
> default state? Can it only call power management calls from particular
> states? What is the restored state?
>
> - What the expectations are w.r.t. ownership of pins, e.g. must the firmware
> never change the state of certain pins? Must it save/restore their state in
> system-wide power-management scenarios like suspend or hibernate?
>
> I think this needs to be raised with the ASWG, and some level of model needs to
> be defined for this, to cater for the issues raised above.
>
> That might be very simple, e.g. pins are never shared, the pinctrl management
> device must permit concurrent accesses by FW and kernel, the kernel is
> responsible for all management of those pins after system reset.
>
> In the absence of that, this is liable to become fragmented and fragile, and is
> practically impossible to rectify post-hoc.
>
All very good points.
Since our focus is for open-ended configurations and for hardware that
it is not know to firmware we only considered the case where the pins
are not touched after the system boots.
Now I wonder what are the cases were the firmware changes the pin
configuration after boot.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-05 20:09 ` Octavian Purdila
@ 2016-04-06 0:01 ` Mark Rutland
2016-04-07 12:11 ` Octavian Purdila
0 siblings, 1 reply; 41+ messages in thread
From: Mark Rutland @ 2016-04-06 0:01 UTC (permalink / raw)
To: Octavian Purdila
Cc: Tirdea, Irina, Rafael J. Wysocki, Len Brown, Mika Westerberg,
Linus Walleij, linux-gpio, linux-acpi, Rob Herring,
Heikki Krogerus, Andy Shevchenko, Ciocan, Cristina, devicetree,
linux-kernel, charles.garcia-tobin
On Tue, Apr 05, 2016 at 11:09:34PM +0300, Octavian Purdila wrote:
> On Tue, Apr 5, 2016 at 9:16 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > * The firmware is to some extent expected to manage pinctrl today (for power
> > management of devices it does know about), and hence a pinctrl device is
> > potentially under shared management of ACPI and the OS.
> >
> > * The ACPI specification says nothing about this style of pinctrl management,
> > so it is unclear what the expectations are:
>
> Does it say anything at all about pinctrl management?
To the best of my knowledge the ACPI spec does not explicitly mention pinctrl.
In another reply it was mentioned that there is work in progress for pinmuxing,
so evidently it is on the ASWG radar and within the scope of ACPI.
I was trying to point out that it is _implicitly_ firmware's responsibility to
do any pinctrl today, due to the ACPI model for power management, and the lack
of an existing ACPI mechanisms to provide pinctrl data.
In practice, firmware configures pinctrl at boot, and may modify pinctrl as
part of the runtime power management firmware is put in charge of.
The lack of any statement about pinctrl would mean that there is effectively no
reasonable limitation on what firmware might do with pinctrl, and we cannot
assume specific behaviour from the firmware.
[...]
> Since our focus is for open-ended configurations and for hardware that
> it is not know to firmware we only considered the case where the pins
> are not touched after the system boots.
>
> Now I wonder what are the cases were the firmware changes the pin
> configuration after boot.
Device power management and suspend/resume seem like the obvious cases.
Thanks,
Mark
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-06 0:01 ` Mark Rutland
@ 2016-04-07 12:11 ` Octavian Purdila
2016-04-06 10:39 ` Mark Rutland
0 siblings, 1 reply; 41+ messages in thread
From: Octavian Purdila @ 2016-04-07 12:11 UTC (permalink / raw)
To: Mark Rutland
Cc: Tirdea, Irina, Rafael J. Wysocki, Len Brown, Mika Westerberg,
Linus Walleij, linux-gpio, linux-acpi, Rob Herring,
Heikki Krogerus, Andy Shevchenko, Ciocan, Cristina, devicetree,
linux-kernel, charles.garcia-tobin
On Wed, Apr 6, 2016 at 3:01 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Tue, Apr 05, 2016 at 11:09:34PM +0300, Octavian Purdila wrote:
>> On Tue, Apr 5, 2016 at 9:16 PM, Mark Rutland <mark.rutland@arm.com> wrote:
>> > * The firmware is to some extent expected to manage pinctrl today (for power
>> > management of devices it does know about), and hence a pinctrl device is
>> > potentially under shared management of ACPI and the OS.
>> >
>> > * The ACPI specification says nothing about this style of pinctrl management,
>> > so it is unclear what the expectations are:
>>
>> Does it say anything at all about pinctrl management?
>
> To the best of my knowledge the ACPI spec does not explicitly mention pinctrl.
> In another reply it was mentioned that there is work in progress for pinmuxing,
> so evidently it is on the ASWG radar and within the scope of ACPI.
>
> I was trying to point out that it is _implicitly_ firmware's responsibility to
> do any pinctrl today, due to the ACPI model for power management, and the lack
> of an existing ACPI mechanisms to provide pinctrl data.
>
> In practice, firmware configures pinctrl at boot, and may modify pinctrl as
> part of the runtime power management firmware is put in charge of.
>
AFAIK the firmware only uses the pinctrl at boot to set the initial
values and the OS owns it after boot. The only interaction I know of
after boot are GPIO signaled events, but those are executed under the
control of the OS.
I don't understand the part about firmware being put in charge of
runtime power management. Do you mean that the firmware directly
accesses the pinctrl registers? Doesn't this contradict the ACPI goal
of having the OS control power management? Or do you mean accessing
pinctrl registers via _PSx and PowerResource._On/_Off?
> The lack of any statement about pinctrl would mean that there is effectively no
> reasonable limitation on what firmware might do with pinctrl, and we cannot
> assume specific behaviour from the firmware.
>
There is noting specified in the spec about other controllers, why is
pinctrl special in this regard?
> [...]
>
>> Since our focus is for open-ended configurations and for hardware that
>> it is not know to firmware we only considered the case where the pins
>> are not touched after the system boots.
>>
>> Now I wonder what are the cases were the firmware changes the pin
>> configuration after boot.
>
> Device power management and suspend/resume seem like the obvious cases.
I assume you are suggesting something like the following: we have a
device that is powered via a GPIO and associated ACPI _PS3/_PS0
methods are poking the pinctrl register to drive 0 or 1 to power on or
off the device.
If that is the case, we can easily break that today, by changing that
particular GPIO value via, e.g., sysfs.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-07 12:11 ` Octavian Purdila
@ 2016-04-06 10:39 ` Mark Rutland
2016-04-07 21:24 ` Rafael J. Wysocki
0 siblings, 1 reply; 41+ messages in thread
From: Mark Rutland @ 2016-04-06 10:39 UTC (permalink / raw)
To: Octavian Purdila
Cc: Tirdea, Irina, Rafael J. Wysocki, Len Brown, Mika Westerberg,
Linus Walleij, linux-gpio, linux-acpi, Rob Herring,
Heikki Krogerus, Andy Shevchenko, Ciocan, Cristina, devicetree,
linux-kernel, charles.garcia-tobin
On Thu, Apr 07, 2016 at 03:11:53PM +0300, Octavian Purdila wrote:
> On Wed, Apr 6, 2016 at 3:01 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > On Tue, Apr 05, 2016 at 11:09:34PM +0300, Octavian Purdila wrote:
> >> On Tue, Apr 5, 2016 at 9:16 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> >> > * The firmware is to some extent expected to manage pinctrl today (for power
> >> > management of devices it does know about), and hence a pinctrl device is
> >> > potentially under shared management of ACPI and the OS.
> >> >
> >> > * The ACPI specification says nothing about this style of pinctrl management,
> >> > so it is unclear what the expectations are:
> >>
> >> Does it say anything at all about pinctrl management?
> >
> > To the best of my knowledge the ACPI spec does not explicitly mention pinctrl.
> > In another reply it was mentioned that there is work in progress for pinmuxing,
> > so evidently it is on the ASWG radar and within the scope of ACPI.
> >
> > I was trying to point out that it is _implicitly_ firmware's responsibility to
> > do any pinctrl today, due to the ACPI model for power management, and the lack
> > of an existing ACPI mechanisms to provide pinctrl data.
> >
> > In practice, firmware configures pinctrl at boot, and may modify pinctrl as
> > part of the runtime power management firmware is put in charge of.
> >
>
> AFAIK the firmware only uses the pinctrl at boot to set the initial
> values and the OS owns it after boot. The only interaction I know of
> after boot are GPIO signaled events, but those are executed under the
> control of the OS.
>
> I don't understand the part about firmware being put in charge of
> runtime power management. Do you mean that the firmware directly
> accesses the pinctrl registers? Doesn't this contradict the ACPI goal
> of having the OS control power management? Or do you mean accessing
> pinctrl registers via _PSx and PowerResource._On/_Off?
I mostly mean the latter. Even if the OS is in charge of _when_ those happen,
that only solves mutual exclusion over the pinctrl registers. That does not
handle expectations regarding the current _state_ of the pinctrl configuation,
or the configurations the OS/FW can permit and/or require (e.g. there's no
refcounting between OS and FW for the state of shared pins).
It may also be that pinctrl gets altered in the background (e.g. in SMM),
outside of the OS's control also, but that's probably a rare/extreme case.
> > The lack of any statement about pinctrl would mean that there is effectively no
> > reasonable limitation on what firmware might do with pinctrl, and we cannot
> > assume specific behaviour from the firmware.
>
> There is noting specified in the spec about other controllers, why is
> pinctrl special in this regard?
Because there are clear demonstrable cases why FW would want to touch pinctrl
today, that may clash with the pinctrl model you are importing from DT (where
the OS is assumed to have complete ownership and control over pinctrl).
The ACPI model implies FW-driven pinctrl management, so if we're going to put
the OS in direct control of pinctrl, we have to make clear what expectation FW
and OS can have.
> > [...]
> >
> >> Since our focus is for open-ended configurations and for hardware that
> >> it is not know to firmware we only considered the case where the pins
> >> are not touched after the system boots.
> >>
> >> Now I wonder what are the cases were the firmware changes the pin
> >> configuration after boot.
> >
> > Device power management and suspend/resume seem like the obvious cases.
>
> I assume you are suggesting something like the following: we have a
> device that is powered via a GPIO and associated ACPI _PS3/_PS0
> methods are poking the pinctrl register to drive 0 or 1 to power on or
> off the device.
Potentially. In that case it's not clear what the FW is expected to do, and
what the OS is expected to do.
For things like system suspend/resume or hibernate, it's not clear what state
the FW is expected to save/restore, and what state might arbitrarily change.
> If that is the case, we can easily break that today, by changing that
> particular GPIO value via, e.g., sysfs.
Sure, but that is not part of the usual, automatic management of the system.
There are plenty of ways a user with sufficient privilege can bring easily down
a system. They are irrelevant.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-06 10:39 ` Mark Rutland
@ 2016-04-07 21:24 ` Rafael J. Wysocki
2016-04-12 12:15 ` Mark Brown
0 siblings, 1 reply; 41+ messages in thread
From: Rafael J. Wysocki @ 2016-04-07 21:24 UTC (permalink / raw)
To: Mark Rutland
Cc: Octavian Purdila, Tirdea, Irina, Len Brown, Mika Westerberg,
Linus Walleij, linux-gpio, linux-acpi, Rob Herring,
Heikki Krogerus, Andy Shevchenko, Ciocan, Cristina, devicetree,
linux-kernel, charles.garcia-tobin
On Wednesday, April 06, 2016 11:39:27 AM Mark Rutland wrote:
> On Thu, Apr 07, 2016 at 03:11:53PM +0300, Octavian Purdila wrote:
> > On Wed, Apr 6, 2016 at 3:01 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> > > On Tue, Apr 05, 2016 at 11:09:34PM +0300, Octavian Purdila wrote:
> > >> On Tue, Apr 5, 2016 at 9:16 PM, Mark Rutland <mark.rutland@arm.com> wrote:
> > >> > * The firmware is to some extent expected to manage pinctrl today (for power
> > >> > management of devices it does know about), and hence a pinctrl device is
> > >> > potentially under shared management of ACPI and the OS.
> > >> >
> > >> > * The ACPI specification says nothing about this style of pinctrl management,
> > >> > so it is unclear what the expectations are:
> > >>
> > >> Does it say anything at all about pinctrl management?
> > >
> > > To the best of my knowledge the ACPI spec does not explicitly mention pinctrl.
> > > In another reply it was mentioned that there is work in progress for pinmuxing,
> > > so evidently it is on the ASWG radar and within the scope of ACPI.
> > >
> > > I was trying to point out that it is _implicitly_ firmware's responsibility to
> > > do any pinctrl today, due to the ACPI model for power management, and the lack
> > > of an existing ACPI mechanisms to provide pinctrl data.
> > >
> > > In practice, firmware configures pinctrl at boot, and may modify pinctrl as
> > > part of the runtime power management firmware is put in charge of.
> > >
> >
> > AFAIK the firmware only uses the pinctrl at boot to set the initial
> > values and the OS owns it after boot. The only interaction I know of
> > after boot are GPIO signaled events, but those are executed under the
> > control of the OS.
> >
> > I don't understand the part about firmware being put in charge of
> > runtime power management. Do you mean that the firmware directly
> > accesses the pinctrl registers? Doesn't this contradict the ACPI goal
> > of having the OS control power management? Or do you mean accessing
> > pinctrl registers via _PSx and PowerResource._On/_Off?
>
> I mostly mean the latter. Even if the OS is in charge of _when_ those happen,
> that only solves mutual exclusion over the pinctrl registers. That does not
> handle expectations regarding the current _state_ of the pinctrl configuation,
> or the configurations the OS/FW can permit and/or require (e.g. there's no
> refcounting between OS and FW for the state of shared pins).
>
> It may also be that pinctrl gets altered in the background (e.g. in SMM),
> outside of the OS's control also, but that's probably a rare/extreme case.
>
> > > The lack of any statement about pinctrl would mean that there is effectively no
> > > reasonable limitation on what firmware might do with pinctrl, and we cannot
> > > assume specific behaviour from the firmware.
> >
> > There is noting specified in the spec about other controllers, why is
> > pinctrl special in this regard?
>
> Because there are clear demonstrable cases why FW would want to touch pinctrl
> today, that may clash with the pinctrl model you are importing from DT (where
> the OS is assumed to have complete ownership and control over pinctrl).
>
> The ACPI model implies FW-driven pinctrl management, so if we're going to put
> the OS in direct control of pinctrl, we have to make clear what expectation FW
> and OS can have.
Well, orthodox ACPI people from the Windows camp would definitely agree with you,
but I'm not one of them, so let me play a devil's advocate for a while. :-)
IMO, exposing anything in the ACPI tables without explicitly saying "but you
should not touch that", eg by defining an operation region covering it or similar,
is equivalent to giving the OS a license to play with that thing as it wishes.
Therefore I would regard exposing pin control information in cases when the
OS is not allowed to use it to its fit as a firmware bug.
Now, the question in this patch series is how to expose things and not when to
do that. It adds support for a specific method of exposing information to the
OS, but should it be concerned about the possible consequences of inappropriate
use of that method by firmware?
Thanks,
Rafael
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-07 21:24 ` Rafael J. Wysocki
@ 2016-04-12 12:15 ` Mark Brown
2016-04-13 5:08 ` Rafael J. Wysocki
0 siblings, 1 reply; 41+ messages in thread
From: Mark Brown @ 2016-04-12 12:15 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Mark Rutland, Octavian Purdila, Tirdea, Irina, Len Brown,
Mika Westerberg, Linus Walleij, linux-gpio, linux-acpi,
Rob Herring, Heikki Krogerus, Andy Shevchenko, Ciocan, Cristina,
devicetree, linux-kernel, charles.garcia-tobin
[-- Attachment #1: Type: text/plain, Size: 2090 bytes --]
On Thu, Apr 07, 2016 at 11:24:00PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, April 06, 2016 11:39:27 AM Mark Rutland wrote:
> > The ACPI model implies FW-driven pinctrl management, so if we're going to put
> > the OS in direct control of pinctrl, we have to make clear what expectation FW
> > and OS can have.
> Well, orthodox ACPI people from the Windows camp would definitely agree with you,
> but I'm not one of them, so let me play a devil's advocate for a while. :-)
> IMO, exposing anything in the ACPI tables without explicitly saying "but you
> should not touch that", eg by defining an operation region covering it or similar,
> is equivalent to giving the OS a license to play with that thing as it wishes.
> Therefore I would regard exposing pin control information in cases when the
> OS is not allowed to use it to its fit as a firmware bug.
This is something where it seems like it would be very easy for people
to end up relying on current OS behaviour even if that only happened
through accident rather than design. You might assume that such
behaviour is a bug but that may not be obvious to the firmware author if
the OS behaviour makes sense to them.
There may also be some expectation on the part of firmware that the OS
will do some management of the pins even for devices that it's not
actively working with (put them in an idle mode for example).
> Now, the question in this patch series is how to expose things and not when to
> do that. It adds support for a specific method of exposing information to the
> OS, but should it be concerned about the possible consequences of inappropriate
> use of that method by firmware?
The other issue here is that if (as Octavian mentioned) there are
ongoing discussions within ASWG on producing an actual spec for this it
doesn't seem great that we'd just go and do something completely
different rather than trying to make sure that the spec works well. Or
if there isn't any spec work then writing one there for other OSs to
pick up if they like. This seems like it'd make us much better
community players.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [RFC PATCH 0/4] Add ACPI support for pinctrl configuration
2016-04-12 12:15 ` Mark Brown
@ 2016-04-13 5:08 ` Rafael J. Wysocki
0 siblings, 0 replies; 41+ messages in thread
From: Rafael J. Wysocki @ 2016-04-13 5:08 UTC (permalink / raw)
To: Mark Brown
Cc: Rafael J. Wysocki, Mark Rutland, Octavian Purdila, Tirdea, Irina,
Len Brown, Mika Westerberg, Linus Walleij, linux-gpio,
linux-acpi, Rob Herring, Heikki Krogerus, Andy Shevchenko,
Ciocan, Cristina, devicetree, linux-kernel, charles.garcia-tobin
On Tue, Apr 12, 2016 at 2:15 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Apr 07, 2016 at 11:24:00PM +0200, Rafael J. Wysocki wrote:
>> On Wednesday, April 06, 2016 11:39:27 AM Mark Rutland wrote:
>
>> > The ACPI model implies FW-driven pinctrl management, so if we're going to put
>> > the OS in direct control of pinctrl, we have to make clear what expectation FW
>> > and OS can have.
>
>> Well, orthodox ACPI people from the Windows camp would definitely agree with you,
>> but I'm not one of them, so let me play a devil's advocate for a while. :-)
>
>> IMO, exposing anything in the ACPI tables without explicitly saying "but you
>> should not touch that", eg by defining an operation region covering it or similar,
>> is equivalent to giving the OS a license to play with that thing as it wishes.
>
>> Therefore I would regard exposing pin control information in cases when the
>> OS is not allowed to use it to its fit as a firmware bug.
>
> This is something where it seems like it would be very easy for people
> to end up relying on current OS behaviour even if that only happened
> through accident rather than design. You might assume that such
> behaviour is a bug but that may not be obvious to the firmware author if
> the OS behaviour makes sense to them.
If the OS started to support that interface, it would need to
guarantee consistent behavior with respect to it going forward.
It would be an interface for interacting with firmware, so it is not
much different from an interface to user space as far as the stability
rules go. Which may be a reason for refusing to support it in the
first place if the OS is not willing to make such a guarantee.
> There may also be some expectation on the part of firmware that the OS
> will do some management of the pins even for devices that it's not
> actively working with (put them in an idle mode for example).
>
>> Now, the question in this patch series is how to expose things and not when to
>> do that. It adds support for a specific method of exposing information to the
>> OS, but should it be concerned about the possible consequences of inappropriate
>> use of that method by firmware?
>
> The other issue here is that if (as Octavian mentioned) there are
> ongoing discussions within ASWG on producing an actual spec for this it
> doesn't seem great that we'd just go and do something completely
> different rather than trying to make sure that the spec works well. Or
> if there isn't any spec work then writing one there for other OSs to
> pick up if they like. This seems like it'd make us much better
> community players.
Agreed. I actually asked Octavian to look at the other proposal and
see if/how that could be used to address the use case in question
and/or possibly extended to cover it.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 41+ messages in thread