* [PATCH v3 13/22] pinctrl: core: Embed struct pingroup into struct group_desc
From: Andy Shevchenko @ 2023-11-28 19:57 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng
In-Reply-To: <20231128200155.438722-1-andriy.shevchenko@linux.intel.com>
struct group_desc is a particular version of the struct pingroup
with associated opaque data. Start switching pin control core and
drivers to use it explicitly.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/core.c | 15 ++++++++++++---
drivers/pinctrl/core.h | 5 +++++
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 3f1fd50fbb10..e08d4b3b0a56 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -559,7 +559,10 @@ const char *pinctrl_generic_get_group_name(struct pinctrl_dev *pctldev,
if (!group)
return NULL;
- return group->name;
+ if (group->name)
+ return group->name;
+
+ return group->grp.name;
}
EXPORT_SYMBOL_GPL(pinctrl_generic_get_group_name);
@@ -585,8 +588,14 @@ int pinctrl_generic_get_group_pins(struct pinctrl_dev *pctldev,
return -EINVAL;
}
- *pins = group->pins;
- *num_pins = group->num_pins;
+ if (group->pins) {
+ *pins = group->pins;
+ *num_pins = group->num_pins;
+ return 0;
+ }
+
+ *pins = group->grp.pins;
+ *num_pins = group->grp.npins;
return 0;
}
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 276a631fd49c..863b4956a41e 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -194,14 +194,18 @@ struct pinctrl_maps {
#ifdef CONFIG_GENERIC_PINCTRL_GROUPS
+#include <linux/pinctrl/pinctrl.h>
+
/**
* struct group_desc - generic pin group descriptor
+ * @grp: generic data of the pin group (name and pins)
* @name: name of the pin group
* @pins: array of pins that belong to the group
* @num_pins: number of pins in the group
* @data: pin controller driver specific data
*/
struct group_desc {
+ struct pingroup grp;
const char *name;
const int *pins;
int num_pins;
@@ -211,6 +215,7 @@ struct group_desc {
/* Convenience macro to define a generic pin group descriptor */
#define PINCTRL_GROUP_DESC(_name, _pins, _num_pins, _data) \
(struct group_desc) { \
+ .grp = PINCTRL_PINGROUP(_name, _pins, _num_pins), \
.name = _name, \
.pins = _pins, \
.num_pins = _num_pins, \
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related
* [PATCH v3 11/22] pinctrl: ingenic: Make use of PINCTRL_GROUP_DESC()
From: Andy Shevchenko @ 2023-11-28 19:57 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng
In-Reply-To: <20231128200155.438722-1-andriy.shevchenko@linux.intel.com>
Make use of PINCTRL_GROUP_DESC() instead of open coding it.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/pinctrl-ingenic.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c
index ee718f6e2556..393873de910a 100644
--- a/drivers/pinctrl/pinctrl-ingenic.c
+++ b/drivers/pinctrl/pinctrl-ingenic.c
@@ -83,15 +83,10 @@
#define JZ4730_PINS_PER_PAIRED_REG 16
#define INGENIC_PIN_GROUP_FUNCS(name, id, funcs) \
- { \
- name, \
- id##_pins, \
- ARRAY_SIZE(id##_pins), \
- funcs, \
- }
+ PINCTRL_GROUP_DESC(name, id##_pins, ARRAY_SIZE(id##_pins), funcs)
#define INGENIC_PIN_GROUP(name, id, func) \
- INGENIC_PIN_GROUP_FUNCS(name, id, (void *)(func))
+ PINCTRL_GROUP_DESC(name, id##_pins, ARRAY_SIZE(id##_pins), (void *)(func))
enum jz_version {
ID_JZ4730,
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related
* [PATCH v3 10/22] pinctrl: core: Add a convenient define PINCTRL_GROUP_DESC()
From: Andy Shevchenko @ 2023-11-28 19:56 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng
In-Reply-To: <20231128200155.438722-1-andriy.shevchenko@linux.intel.com>
Add PINCTRL_GROUP_DESC() macro for inline use.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/core.c | 5 +----
drivers/pinctrl/core.h | 9 +++++++++
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index d20e3aad923e..3f1fd50fbb10 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -660,10 +660,7 @@ int pinctrl_generic_add_group(struct pinctrl_dev *pctldev, const char *name,
if (!group)
return -ENOMEM;
- group->name = name;
- group->pins = pins;
- group->num_pins = num_pins;
- group->data = data;
+ *group = PINCTRL_GROUP_DESC(name, pins, num_pins, data);
error = radix_tree_insert(&pctldev->pin_group_tree, selector, group);
if (error)
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 01ea1ce99fe8..276a631fd49c 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -208,6 +208,15 @@ struct group_desc {
void *data;
};
+/* Convenience macro to define a generic pin group descriptor */
+#define PINCTRL_GROUP_DESC(_name, _pins, _num_pins, _data) \
+(struct group_desc) { \
+ .name = _name, \
+ .pins = _pins, \
+ .num_pins = _num_pins, \
+ .data = _data, \
+}
+
int pinctrl_generic_get_group_count(struct pinctrl_dev *pctldev);
const char *pinctrl_generic_get_group_name(struct pinctrl_dev *pctldev,
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related
* [PATCH v3 22/22] pinctrl: core: Remove unused members from struct group_desc
From: Andy Shevchenko @ 2023-11-28 19:57 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng
In-Reply-To: <20231128200155.438722-1-andriy.shevchenko@linux.intel.com>
All drivers are converted to use embedded struct pingroup.
Remove unused members from struct group_desc.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/core.c | 9 ---------
drivers/pinctrl/core.h | 9 ---------
2 files changed, 18 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index e08d4b3b0a56..88de80187445 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -559,9 +559,6 @@ const char *pinctrl_generic_get_group_name(struct pinctrl_dev *pctldev,
if (!group)
return NULL;
- if (group->name)
- return group->name;
-
return group->grp.name;
}
EXPORT_SYMBOL_GPL(pinctrl_generic_get_group_name);
@@ -588,12 +585,6 @@ int pinctrl_generic_get_group_pins(struct pinctrl_dev *pctldev,
return -EINVAL;
}
- if (group->pins) {
- *pins = group->pins;
- *num_pins = group->num_pins;
- return 0;
- }
-
*pins = group->grp.pins;
*num_pins = group->grp.npins;
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 863b4956a41e..c1ace4c2eccc 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -199,16 +199,10 @@ struct pinctrl_maps {
/**
* struct group_desc - generic pin group descriptor
* @grp: generic data of the pin group (name and pins)
- * @name: name of the pin group
- * @pins: array of pins that belong to the group
- * @num_pins: number of pins in the group
* @data: pin controller driver specific data
*/
struct group_desc {
struct pingroup grp;
- const char *name;
- const int *pins;
- int num_pins;
void *data;
};
@@ -216,9 +210,6 @@ struct group_desc {
#define PINCTRL_GROUP_DESC(_name, _pins, _num_pins, _data) \
(struct group_desc) { \
.grp = PINCTRL_PINGROUP(_name, _pins, _num_pins), \
- .name = _name, \
- .pins = _pins, \
- .num_pins = _num_pins, \
.data = _data, \
}
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related
* [PATCH v3 08/22] pinctrl: keembay: Convert to use struct pingroup
From: Andy Shevchenko @ 2023-11-28 19:56 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng
In-Reply-To: <20231128200155.438722-1-andriy.shevchenko@linux.intel.com>
The pin control header provides struct pingroup.
Utilize it instead of open coded variants in the driver.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/pinctrl-keembay.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-keembay.c b/drivers/pinctrl/pinctrl-keembay.c
index 152c35bce8ec..87d328853ae4 100644
--- a/drivers/pinctrl/pinctrl-keembay.c
+++ b/drivers/pinctrl/pinctrl-keembay.c
@@ -1517,7 +1517,7 @@ static int keembay_gpiochip_probe(struct keembay_pinctrl *kpc,
static int keembay_build_groups(struct keembay_pinctrl *kpc)
{
- struct group_desc *grp;
+ struct pingroup *grp;
unsigned int i;
kpc->ngroups = kpc->npins;
@@ -1528,7 +1528,7 @@ static int keembay_build_groups(struct keembay_pinctrl *kpc)
/* Each pin is categorised as one group */
for (i = 0; i < kpc->ngroups; i++) {
const struct pinctrl_pin_desc *pdesc = keembay_pins + i;
- struct group_desc *kmb_grp = grp + i;
+ struct pingroup *kmb_grp = grp + i;
kmb_grp->name = pdesc->name;
kmb_grp->pins = (int *)&pdesc->number;
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related
* [PATCH v3 06/22] pinctrl: core: Make pins const in struct group_desc
From: Andy Shevchenko @ 2023-11-28 19:56 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng
In-Reply-To: <20231128200155.438722-1-andriy.shevchenko@linux.intel.com>
It's unclear why it's not a const from day 1. Make the pins member
const in struct group_desc. Update necessary APIs.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/core.c | 2 +-
drivers/pinctrl/core.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index f2977eb65522..d20e3aad923e 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -642,7 +642,7 @@ static int pinctrl_generic_group_name_to_selector(struct pinctrl_dev *pctldev,
* Note that the caller must take care of locking.
*/
int pinctrl_generic_add_group(struct pinctrl_dev *pctldev, const char *name,
- int *pins, int num_pins, void *data)
+ const int *pins, int num_pins, void *data)
{
struct group_desc *group;
int selector, error;
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 530370443c19..01ea1ce99fe8 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -203,7 +203,7 @@ struct pinctrl_maps {
*/
struct group_desc {
const char *name;
- int *pins;
+ const int *pins;
int num_pins;
void *data;
};
@@ -222,7 +222,7 @@ struct group_desc *pinctrl_generic_get_group(struct pinctrl_dev *pctldev,
unsigned int group_selector);
int pinctrl_generic_add_group(struct pinctrl_dev *pctldev, const char *name,
- int *gpins, int ngpins, void *data);
+ const int *pins, int num_pins, void *data);
int pinctrl_generic_remove_group(struct pinctrl_dev *pctldev,
unsigned int group_selector);
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related
* [PATCH v3 17/22] pinctrl: ingenic: Convert to use grp member
From: Andy Shevchenko @ 2023-11-28 19:57 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng
In-Reply-To: <20231128200155.438722-1-andriy.shevchenko@linux.intel.com>
Convert drivers to use grp member embedded in struct group_desc.
Acked-by: Paul Cercueil <paul@crapouillou.net>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/pinctrl-ingenic.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-ingenic.c b/drivers/pinctrl/pinctrl-ingenic.c
index 393873de910a..6806fede5df4 100644
--- a/drivers/pinctrl/pinctrl-ingenic.c
+++ b/drivers/pinctrl/pinctrl-ingenic.c
@@ -3756,17 +3756,17 @@ static int ingenic_pinmux_set_mux(struct pinctrl_dev *pctldev,
return -EINVAL;
dev_dbg(pctldev->dev, "enable function %s group %s\n",
- func->name, grp->name);
+ func->name, grp->grp.name);
mode = (uintptr_t)grp->data;
if (mode <= 3) {
- for (i = 0; i < grp->num_pins; i++)
- ingenic_pinmux_set_pin_fn(jzpc, grp->pins[i], mode);
+ for (i = 0; i < grp->grp.npins; i++)
+ ingenic_pinmux_set_pin_fn(jzpc, grp->grp.pins[i], mode);
} else {
pin_modes = grp->data;
- for (i = 0; i < grp->num_pins; i++)
- ingenic_pinmux_set_pin_fn(jzpc, grp->pins[i], pin_modes[i]);
+ for (i = 0; i < grp->grp.npins; i++)
+ ingenic_pinmux_set_pin_fn(jzpc, grp->grp.pins[i], pin_modes[i]);
}
return 0;
@@ -4293,12 +4293,12 @@ static int __init ingenic_pinctrl_probe(struct platform_device *pdev)
for (i = 0; i < chip_info->num_groups; i++) {
const struct group_desc *group = &chip_info->groups[i];
+ const struct pingroup *grp = &group->grp;
- err = pinctrl_generic_add_group(jzpc->pctl, group->name,
- group->pins, group->num_pins, group->data);
+ err = pinctrl_generic_add_group(jzpc->pctl, grp->name, grp->pins, grp->npins,
+ group->data);
if (err < 0) {
- dev_err(dev, "Failed to register group %s\n",
- group->name);
+ dev_err(dev, "Failed to register group %s\n", grp->name);
return err;
}
}
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related
* [PATCH v3 20/22] pinctrl: renesas: Convert to use grp member
From: Andy Shevchenko @ 2023-11-28 19:57 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng
In-Reply-To: <20231128200155.438722-1-andriy.shevchenko@linux.intel.com>
Convert drivers to use grp member embedded in struct group_desc.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/renesas/pinctrl-rza1.c | 2 +-
drivers/pinctrl/renesas/pinctrl-rza2.c | 10 +++++-----
drivers/pinctrl/renesas/pinctrl-rzg2l.c | 6 +++---
drivers/pinctrl/renesas/pinctrl-rzv2m.c | 6 +++---
4 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/pinctrl/renesas/pinctrl-rza1.c b/drivers/pinctrl/renesas/pinctrl-rza1.c
index ab334de89b69..b03f22c54ca8 100644
--- a/drivers/pinctrl/renesas/pinctrl-rza1.c
+++ b/drivers/pinctrl/renesas/pinctrl-rza1.c
@@ -1131,7 +1131,7 @@ static int rza1_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
return -EINVAL;
mux_confs = (struct rza1_mux_conf *)func->data;
- for (i = 0; i < grp->num_pins; ++i) {
+ for (i = 0; i < grp->grp.npins; ++i) {
int ret;
ret = rza1_pin_mux_single(rza1_pctl, &mux_confs[i]);
diff --git a/drivers/pinctrl/renesas/pinctrl-rza2.c b/drivers/pinctrl/renesas/pinctrl-rza2.c
index 990b96d45967..af689d7c117f 100644
--- a/drivers/pinctrl/renesas/pinctrl-rza2.c
+++ b/drivers/pinctrl/renesas/pinctrl-rza2.c
@@ -447,15 +447,15 @@ static int rza2_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
psel_val = func->data;
- for (i = 0; i < grp->num_pins; ++i) {
+ for (i = 0; i < grp->grp.npins; ++i) {
dev_dbg(priv->dev, "Setting P%c_%d to PSEL=%d\n",
- port_names[RZA2_PIN_ID_TO_PORT(grp->pins[i])],
- RZA2_PIN_ID_TO_PIN(grp->pins[i]),
+ port_names[RZA2_PIN_ID_TO_PORT(grp->grp.pins[i])],
+ RZA2_PIN_ID_TO_PIN(grp->grp.pins[i]),
psel_val[i]);
rza2_set_pin_function(
priv->base,
- RZA2_PIN_ID_TO_PORT(grp->pins[i]),
- RZA2_PIN_ID_TO_PIN(grp->pins[i]),
+ RZA2_PIN_ID_TO_PORT(grp->grp.pins[i]),
+ RZA2_PIN_ID_TO_PIN(grp->grp.pins[i]),
psel_val[i]);
}
diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index aed59c53207c..3cfe4558eb92 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -273,7 +273,7 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev,
struct function_desc *func;
unsigned int i, *psel_val;
struct group_desc *group;
- int *pins;
+ const int *pins;
func = pinmux_generic_get_function(pctldev, func_selector);
if (!func)
@@ -283,9 +283,9 @@ static int rzg2l_pinctrl_set_mux(struct pinctrl_dev *pctldev,
return -EINVAL;
psel_val = func->data;
- pins = group->pins;
+ pins = group->grp.pins;
- for (i = 0; i < group->num_pins; i++) {
+ for (i = 0; i < group->grp.npins; i++) {
unsigned int *pin_data = pctrl->desc.pins[pins[i]].drv_data;
u32 off = RZG2L_PIN_CFG_TO_PORT_OFFSET(*pin_data);
u32 pin = RZG2L_PIN_ID_TO_PIN(pins[i]);
diff --git a/drivers/pinctrl/renesas/pinctrl-rzv2m.c b/drivers/pinctrl/renesas/pinctrl-rzv2m.c
index 21d7d5ac8c4a..eb66e306b8c8 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzv2m.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzv2m.c
@@ -165,7 +165,7 @@ static int rzv2m_pinctrl_set_mux(struct pinctrl_dev *pctldev,
struct function_desc *func;
unsigned int i, *psel_val;
struct group_desc *group;
- int *pins;
+ const int *pins;
func = pinmux_generic_get_function(pctldev, func_selector);
if (!func)
@@ -175,9 +175,9 @@ static int rzv2m_pinctrl_set_mux(struct pinctrl_dev *pctldev,
return -EINVAL;
psel_val = func->data;
- pins = group->pins;
+ pins = group->grp.pins;
- for (i = 0; i < group->num_pins; i++) {
+ for (i = 0; i < group->grp.npins; i++) {
dev_dbg(pctrl->dev, "port:%u pin: %u PSEL:%u\n",
RZV2M_PIN_ID_TO_PORT(pins[i]), RZV2M_PIN_ID_TO_PIN(pins[i]),
psel_val[i]);
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related
* [PATCH v3 16/22] pinctrl: imx: Convert to use grp member
From: Andy Shevchenko @ 2023-11-28 19:57 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng
In-Reply-To: <20231128200155.438722-1-andriy.shevchenko@linux.intel.com>
Convert drivers to use grp member embedded in struct group_desc.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/freescale/pinctrl-imx.c | 31 +++++++++++--------------
1 file changed, 14 insertions(+), 17 deletions(-)
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 9099a7c81d4a..4245189b59a5 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -42,7 +42,7 @@ static inline const struct group_desc *imx_pinctrl_find_group_by_name(
for (i = 0; i < pctldev->num_groups; i++) {
grp = pinctrl_generic_get_group(pctldev, i);
- if (grp && !strcmp(grp->name, name))
+ if (grp && !strcmp(grp->grp.name, name))
break;
}
@@ -79,9 +79,9 @@ static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
}
if (info->flags & IMX_USE_SCU) {
- map_num += grp->num_pins;
+ map_num += grp->grp.npins;
} else {
- for (i = 0; i < grp->num_pins; i++) {
+ for (i = 0; i < grp->grp.npins; i++) {
pin = &((struct imx_pin *)(grp->data))[i];
if (!(pin->conf.mmio.config & IMX_NO_PAD_CTL))
map_num++;
@@ -109,7 +109,7 @@ static int imx_dt_node_to_map(struct pinctrl_dev *pctldev,
/* create config map */
new_map++;
- for (i = j = 0; i < grp->num_pins; i++) {
+ for (i = j = 0; i < grp->grp.npins; i++) {
pin = &((struct imx_pin *)(grp->data))[i];
/*
@@ -263,10 +263,10 @@ static int imx_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
if (!func)
return -EINVAL;
- npins = grp->num_pins;
+ npins = grp->grp.npins;
dev_dbg(ipctl->dev, "enable function %s group %s\n",
- func->name, grp->name);
+ func->name, grp->grp.name);
for (i = 0; i < npins; i++) {
/*
@@ -423,7 +423,7 @@ static void imx_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
if (!grp)
return;
- for (i = 0; i < grp->num_pins; i++) {
+ for (i = 0; i < grp->grp.npins; i++) {
struct imx_pin *pin = &((struct imx_pin *)(grp->data))[i];
name = pin_get_name(pctldev, pin->pin);
@@ -526,7 +526,7 @@ static int imx_pinctrl_parse_groups(struct device_node *np,
pin_size = FSL_PIN_SIZE;
/* Initialise group */
- grp->name = np->name;
+ grp->grp.name = np->name;
/*
* the binding format is fsl,pins = <PIN_FUNC_ID CONFIG ...>,
@@ -554,19 +554,17 @@ static int imx_pinctrl_parse_groups(struct device_node *np,
return -EINVAL;
}
- grp->num_pins = size / pin_size;
- grp->data = devm_kcalloc(ipctl->dev,
- grp->num_pins, sizeof(struct imx_pin),
- GFP_KERNEL);
+ grp->grp.npins = size / pin_size;
+ grp->data = devm_kcalloc(ipctl->dev, grp->grp.npins, sizeof(*pin), GFP_KERNEL);
if (!grp->data)
return -ENOMEM;
- pins = devm_kcalloc(ipctl->dev, grp->num_pins, sizeof(*pins), GFP_KERNEL);
+ pins = devm_kcalloc(ipctl->dev, grp->grp.npins, sizeof(*pins), GFP_KERNEL);
if (!pins)
return -ENOMEM;
- grp->pins = pins;
+ grp->grp.pins = pins;
- for (i = 0; i < grp->num_pins; i++) {
+ for (i = 0; i < grp->grp.npins; i++) {
pin = &((struct imx_pin *)(grp->data))[i];
if (info->flags & IMX_USE_SCU)
info->imx_pinctrl_parse_pin(ipctl, &pins[i], pin, &list);
@@ -613,8 +611,7 @@ static int imx_pinctrl_parse_functions(struct device_node *np,
i = 0;
for_each_child_of_node(np, child) {
- grp = devm_kzalloc(ipctl->dev, sizeof(struct group_desc),
- GFP_KERNEL);
+ grp = devm_kzalloc(ipctl->dev, sizeof(*grp), GFP_KERNEL);
if (!grp) {
of_node_put(child);
return -ENOMEM;
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related
* [PATCH v3 14/22] pinctrl: bcm: Convert to use grp member
From: Andy Shevchenko @ 2023-11-28 19:57 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng, Florian Fainelli
In-Reply-To: <20231128200155.438722-1-andriy.shevchenko@linux.intel.com>
Convert drivers to use grp member embedded in struct group_desc.
Tested-by: Florian Fainelli <florian.fainelli@broadcom.com>
Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/bcm/pinctrl-ns.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/bcm/pinctrl-ns.c b/drivers/pinctrl/bcm/pinctrl-ns.c
index d099a7f25f64..6bb2b461950b 100644
--- a/drivers/pinctrl/bcm/pinctrl-ns.c
+++ b/drivers/pinctrl/bcm/pinctrl-ns.c
@@ -171,8 +171,8 @@ static int ns_pinctrl_set_mux(struct pinctrl_dev *pctrl_dev,
if (!group)
return -EINVAL;
- for (i = 0; i < group->num_pins; i++)
- unset |= BIT(group->pins[i]);
+ for (i = 0; i < group->grp.npins; i++)
+ unset |= BIT(group->grp.pins[i]);
tmp = readl(ns_pinctrl->base);
tmp &= ~unset;
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related
* [PATCH v3 00/22] pinctrl: Convert struct group_desc to use struct pingroup
From: Andy Shevchenko @ 2023-11-28 19:56 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng
The struct group_desc has a lot of duplication with struct pingroup.
Deduplicate that by embeddind the latter in the former and convert
users.
Linus, assuming everything is fine, I can push this to my tree.
Or you can apply it (assumming all CIs and people are happy with
the series).
NB. It seems to me that GCC 7.x has an issue when compound literal
is being assigned to a constant object. I believe it's a false positive
(at least I can't reproduce this with recent GCC and LLVM and hence
I haven't touched the code in order to address this.
NB. This series contains previously sent patches for Qualcomm and
Nuovoton. Here the updated version for Qualcomm that splits previous
patch to two and fixes compilation warnings.
NB. The function_desc is in plan to follow the similar deduplication.
In v3:
- fixed reported bug in equilibrium code (LKP)
- collected tags (Emil, Florian, Paul)
v2: https://lore.kernel.org/r/20231123193355.3400852-1-andriy.shevchenko@linux.intel.com
In v2:
- added a few patches to fix multiple compile-time errors (LKP)
- added tag (Jonathan)
v1: https://lore.kernel.org/r/20231122164040.2262742-1-andriy.shevchenko@linux.intel.com
Andy Shevchenko (22):
pinctrl: qcom: lpass-lpi: Replace kernel.h with what is being used
pinctrl: qcom: lpass-lpi: Remove unused member in struct lpi_pingroup
pinctrl: equilibrium: Unshadow error code of
of_property_count_u32_elems()
pinctrl: equilibrium: Use temporary variable to hold pins
pinctrl: imx: Use temporary variable to hold pins
pinctrl: core: Make pins const in struct group_desc
pinctrl: equilibrium: Convert to use struct pingroup
pinctrl: keembay: Convert to use struct pingroup
pinctrl: nuvoton: Convert to use struct pingroup and
PINCTRL_PINGROUP()
pinctrl: core: Add a convenient define PINCTRL_GROUP_DESC()
pinctrl: ingenic: Make use of PINCTRL_GROUP_DESC()
pinctrl: mediatek: Make use of PINCTRL_GROUP_DESC()
pinctrl: core: Embed struct pingroup into struct group_desc
pinctrl: bcm: Convert to use grp member
pinctrl: equilibrium: Convert to use grp member
pinctrl: imx: Convert to use grp member
pinctrl: ingenic: Convert to use grp member
pinctrl: keembay: Convert to use grp member
pinctrl: mediatek: Convert to use grp member
pinctrl: renesas: Convert to use grp member
pinctrl: starfive: Convert to use grp member
pinctrl: core: Remove unused members from struct group_desc
drivers/pinctrl/bcm/pinctrl-ns.c | 4 +-
drivers/pinctrl/core.c | 13 +++---
drivers/pinctrl/core.h | 19 +++++---
drivers/pinctrl/freescale/pinctrl-imx.c | 44 +++++++++----------
drivers/pinctrl/mediatek/pinctrl-moore.c | 13 +++---
drivers/pinctrl/mediatek/pinctrl-moore.h | 7 +--
drivers/pinctrl/mediatek/pinctrl-paris.h | 7 +--
drivers/pinctrl/nuvoton/pinctrl-wpcm450.c | 9 ++--
drivers/pinctrl/pinctrl-equilibrium.c | 42 +++++++++---------
drivers/pinctrl/pinctrl-ingenic.c | 27 +++++-------
drivers/pinctrl/pinctrl-keembay.c | 6 +--
drivers/pinctrl/qcom/pinctrl-lpass-lpi.h | 6 +--
.../pinctrl/qcom/pinctrl-sc7280-lpass-lpi.c | 16 -------
.../pinctrl/qcom/pinctrl-sc8280xp-lpass-lpi.c | 20 ---------
.../pinctrl/qcom/pinctrl-sm6115-lpass-lpi.c | 20 ---------
.../pinctrl/qcom/pinctrl-sm8250-lpass-lpi.c | 15 -------
.../pinctrl/qcom/pinctrl-sm8350-lpass-lpi.c | 16 -------
.../pinctrl/qcom/pinctrl-sm8450-lpass-lpi.c | 24 ----------
.../pinctrl/qcom/pinctrl-sm8550-lpass-lpi.c | 24 ----------
.../pinctrl/qcom/pinctrl-sm8650-lpass-lpi.c | 24 ----------
drivers/pinctrl/renesas/pinctrl-rza1.c | 2 +-
drivers/pinctrl/renesas/pinctrl-rza2.c | 10 ++---
drivers/pinctrl/renesas/pinctrl-rzg2l.c | 6 +--
drivers/pinctrl/renesas/pinctrl-rzv2m.c | 6 +--
.../starfive/pinctrl-starfive-jh7100.c | 8 ++--
.../starfive/pinctrl-starfive-jh7110.c | 8 ++--
26 files changed, 108 insertions(+), 288 deletions(-)
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply
* [PATCH v3 03/22] pinctrl: equilibrium: Unshadow error code of of_property_count_u32_elems()
From: Andy Shevchenko @ 2023-11-28 19:56 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng
In-Reply-To: <20231128200155.438722-1-andriy.shevchenko@linux.intel.com>
of_property_count_u32_elems() might return an error code in some cases.
It's naturally better to assign what it's returned to the err variable
and supply the real code to the upper layer(s). Besides that, it's a
common practice to avoid assignments for the data in cases when we know
that the error condition happened. Refactor the code accordingly.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/pinctrl-equilibrium.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/pinctrl/pinctrl-equilibrium.c b/drivers/pinctrl/pinctrl-equilibrium.c
index 5b5ddf7e5d0e..54755b583d3f 100644
--- a/drivers/pinctrl/pinctrl-equilibrium.c
+++ b/drivers/pinctrl/pinctrl-equilibrium.c
@@ -715,12 +715,13 @@ static int eqbr_build_groups(struct eqbr_pinctrl_drv_data *drvdata)
if (!prop)
continue;
- group.num_pins = of_property_count_u32_elems(np, "pins");
- if (group.num_pins < 0) {
+ err = of_property_count_u32_elems(np, "pins");
+ if (err < 0) {
dev_err(dev, "No pins in the group: %s\n", prop->name);
of_node_put(np);
- return -EINVAL;
+ return err;
}
+ group.num_pins = err;
group.name = prop->value;
group.pins = devm_kcalloc(dev, group.num_pins,
sizeof(*(group.pins)), GFP_KERNEL);
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related
* [PATCH v3 12/22] pinctrl: mediatek: Make use of PINCTRL_GROUP_DESC()
From: Andy Shevchenko @ 2023-11-28 19:57 UTC (permalink / raw)
To: Linus Walleij, Bartosz Golaszewski, Andy Shevchenko,
Rasmus Villemoes, Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc
Cc: Ray Jui, Scott Branden, Broadcom internal kernel review list,
Dong Aisheng, Fabio Estevam, Shawn Guo, Jacky Bai,
Pengutronix Kernel Team, Sascha Hauer, NXP Linux Team, Sean Wang,
Paul Cercueil, Lakshmi Sowjanya D, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Emil Renner Berthing, Hal Feng
In-Reply-To: <20231128200155.438722-1-andriy.shevchenko@linux.intel.com>
Make use of PINCTRL_GROUP_DESC() instead of open coding it.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/mediatek/pinctrl-moore.h | 7 +------
drivers/pinctrl/mediatek/pinctrl-paris.h | 7 +------
2 files changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/pinctrl/mediatek/pinctrl-moore.h b/drivers/pinctrl/mediatek/pinctrl-moore.h
index e1b4b82b9d3d..22ef1ffbcdcb 100644
--- a/drivers/pinctrl/mediatek/pinctrl-moore.h
+++ b/drivers/pinctrl/mediatek/pinctrl-moore.h
@@ -38,12 +38,7 @@
}
#define PINCTRL_PIN_GROUP(name, id) \
- { \
- name, \
- id##_pins, \
- ARRAY_SIZE(id##_pins), \
- id##_funcs, \
- }
+ PINCTRL_GROUP_DESC(name, id##_pins, ARRAY_SIZE(id##_pins), id##_funcs)
int mtk_moore_pinctrl_probe(struct platform_device *pdev,
const struct mtk_pin_soc *soc);
diff --git a/drivers/pinctrl/mediatek/pinctrl-paris.h b/drivers/pinctrl/mediatek/pinctrl-paris.h
index 8762ac599329..f208a904c4a8 100644
--- a/drivers/pinctrl/mediatek/pinctrl-paris.h
+++ b/drivers/pinctrl/mediatek/pinctrl-paris.h
@@ -50,12 +50,7 @@
}
#define PINCTRL_PIN_GROUP(name, id) \
- { \
- name, \
- id##_pins, \
- ARRAY_SIZE(id##_pins), \
- id##_funcs, \
- }
+ PINCTRL_GROUP_DESC(name, id##_pins, ARRAY_SIZE(id##_pins), id##_funcs)
int mtk_paris_pinctrl_probe(struct platform_device *pdev);
--
2.43.0.rc1.1.gbec44491f096
^ permalink raw reply related
* Re: [PATCH v3] docs: dt-bindings: add DTS Coding Style document
From: Dragan Simic @ 2023-11-28 20:00 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Matthias Brugger,
AngeloGioacchino Del Regno, devicetree, linux-kernel,
linux-arm-kernel, linux-mediatek, Andrew Davis, Andrew Lunn,
Arnd Bergmann, Bjorn Andersson, Chen-Yu Tsai, Dmitry Baryshkov,
Geert Uytterhoeven, Heiko Stuebner, Jonathan Corbet,
Konrad Dybcio, Michal Simek, Neil Armstrong, Nishanth Menon,
Olof Johansson, Rafał Miłecki, linux-rockchip,
linux-samsung-soc, linux-amlogic, linux-arm-msm, workflows,
linux-doc
In-Reply-To: <20231125184422.12315-1-krzysztof.kozlowski@linaro.org>
Hello Krzysztof,
On 2023-11-25 19:44, Krzysztof Kozlowski wrote:
> Document preferred coding style for Devicetree sources (DTS and DTSI),
> to bring consistency among all (sub)architectures and ease in reviews.
>
> Cc: Andrew Davis <afd@ti.com>
> cc: Andrew Lunn <andrew@lunn.ch>
> Cc: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Chen-Yu Tsai <wens@kernel.org>
> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Cc: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Konrad Dybcio <konrad.dybcio@linaro.org>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: Michal Simek <michal.simek@amd.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Olof Johansson <olof@lixom.net>
> Cc: Rafał Miłecki <zajec5@gmail.com>
> Acked-by: Neil Armstrong <neil.armstrong@linaro.org>
> Acked-by: Heiko Stuebner <heiko@sntech.de>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Merging idea: Rob/DT bindings
>
> Changes in v3
> =============
> 1. should->shall (Angelo)
> 2. Comments // -> /* (Angelo, Michal)
> 3. Use imaginary example in "Order of Properties in Device Node"
> (Angelo)
> 4. Added paragraphs for three sections with justifications of chosen
> style.
> 5. Allow two style of ordering overrides in board DTS: alphabetically
> or
> by order of DTSI (Rob).
> 6. I did not incorporate feedback about, due to lack of consensus and
> my
> disagreement:
> a. SoM being DTS without DTSI in "Organizing DTSI and DTS"
I went through the language of the entire patch, after the notice that
the v4 would no longer accept language improvements. My wording- and
grammar-related suggestions are available inline below.
> Changes in v2
> =============
> 1. Hopefully incorporate entire feedback from comments:
> a. Fix \ { => / { (Rob)
> b. Name: dts-coding-style (Rob)
> c. Exceptions for ordering nodes by name for Renesas and pinctrl
> (Geert,
> Konrad)
> d. Ordering properties by common/vendor (Rob)
> e. Array entries in <> (Rob)
>
> 2. New chapter: Organizing DTSI and DTS
>
> 3. Several grammar fixes (missing articles)
>
> Cc: linux-rockchip@lists.infradead.org
> Cc: linux-mediatek@lists.infradead.org
> Cc: linux-samsung-soc@vger.kernel.org
> Cc: linux-amlogic@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-arm-msm@vger.kernel.org
> Cc: workflows@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> ---
> .../devicetree/bindings/dts-coding-style.rst | 194 ++++++++++++++++++
> Documentation/devicetree/bindings/index.rst | 1 +
> 2 files changed, 195 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/dts-coding-style.rst
>
> diff --git a/Documentation/devicetree/bindings/dts-coding-style.rst
> b/Documentation/devicetree/bindings/dts-coding-style.rst
> new file mode 100644
> index 000000000000..e374bec0f555
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/dts-coding-style.rst
> @@ -0,0 +1,194 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +.. _dtscodingstyle:
> +
> +=====================================
> +Devicetree Sources (DTS) Coding Style
> +=====================================
> +
> +When writing Devicetree Sources (DTS) please observe below guidelines.
> They
The sentence above should be replaced with: "The following guidelines
are to be followed when writing Devicetree Source (DTS) files."
> +should be considered complementary to any rules expressed already in
> Devicetree
> +Specification and dtc compiler (including W=1 and W=2 builds).
A definite article ("the") should be added before "Devicetree
Specification" and "dtc". Also, "Specification" in "Devicetree
Specification" should be capitalized.
> +
> +Individual architectures and sub-architectures can add additional
> rules, making
> +the style stricter.
"Sub-architectures" should be replaced with "subarchitectures". "Can
add" should be replaced with "can define". "Style" should be replaced
with "coding style".
> +
> +Naming and Valid Characters
> +---------------------------
> +
> +Devicetree specification allows broader range of characters in node
> and
> +property names, but for code readability the choice shall be narrowed.
The definite article ("the") should be added before "Devicetree
Specification", and "specification" should be capitalised. As already
suggested, "broader range" should be replaced with "a broad range".
"But for code readability the choice shall be narrowed" should be
replaced with "but this coding style narrows the range down to achieve
better code readability".
> +
> +1. Node and property names are allowed to use only:
"Are allowed to" should be replaced with "can". After "only", "the
following characters" should be added.
> +
> + * lowercase characters: [a-z]
> + * digits: [0-9]
> + * dash: -
List items should be capitalized, i.e. "Lowercase characters" should be
used instead of "lowercase characters", etc.
> +
> +2. Labels are allowed to use only:
"Are allowed to" should be replaced with "can". After "only", "the
following characters" should be added.
> +
> + * lowercase characters: [a-z]
> + * digits: [0-9]
> + * underscore: _
List items should be capitalized, i.e. "Lowercase characters" should be
used instead of "lowercase characters", etc.
> +
> +3. Unit addresses shall use lowercase hex, without leading zeros
> (padding).
"Lowercase hex" should be replaced with "lowercase hexadecimal digits".
> +
> +4. Hex values in properties, e.g. "reg", shall use lowercase hex. The
> address
> + part can be padded with leading zeros.
"Hex values" should be replaced with "Hexadecimal values". "Lowercase
hex" should be replaced with "lowercase hexadecimal digits".
> +
> +Example::
> +
> + gpi_dma2: dma-controller@800000 {
> + compatible = "qcom,sm8550-gpi-dma", "qcom,sm6350-gpi-dma";
> + reg = <0x0 0x00800000 0x0 0x60000>;
> + }
> +
> +Order of Nodes
> +--------------
> +
> +1. Nodes within any bus, thus using unit addresses for children, shall
> be
"Within" should be replaced "on".
> + ordered incrementally by unit address.
Should be replaced with "ordered by unit address in ascending order".
> + Alternatively for some sub-architectures, nodes of the same type
> can be
> + grouped together (e.g. all I2C controllers one after another even
> if this
> + breaks unit address ordering).
"Sub-architectures" should be replaced with "subarchitectures". The
"(e.g. ...)" form should be replaced with ", e.g. ...".
> +
> +2. Nodes without unit addresses shall be ordered alpha-numerically by
> the node
> + name. For a few types of nodes, they can be ordered by the main
> property
> + (e.g. pin configuration states ordered by value of "pins"
> property).
"Alpha-numerically" should be replaced with "alphabetically". "Types of
nodes" should be replaced with "node types". The "(e.g. ...)" form
should be replaced with ", e.g. ...".
> +
> +3. When extending nodes in the board DTS via &label, the entries shall
> be
> + ordered either alpha-numerically or by keeping the order from DTSI
> (choice
> + depending on sub-architecture).
"Alpha-numerically" should be replaced with "alphabetically".
"Sub-architecture" should be replaced with "subarchitecture". "(Choice
depending on sub-architecture)" should be replaced with ", where the
choice depends on the subarchitecture".
> +
> +Above ordering rules are easy to enforce during review, reduce chances
> of
> +conflicts for simultaneous additions (new nodes) to a file and help in
> +navigating through the DTS source.
"Above" should be replaced with "The above-described". "(New nodes)"
should be replaced with "of new nodes".
> +
> +Example::
> +
> + /* SoC DTSI */
> +
> + / {
> + cpus {
> + /* ... */
> + };
> +
> + psci {
> + /* ... */
> + };
> +
> + soc@ {
> + dma: dma-controller@10000 {
> + /* ... */
> + };
> +
> + clk: clock-controller@80000 {
> + /* ... */
> + };
> + };
> + };
> +
> + /* Board DTS - alphabetical order */
> +
> + &clk {
> + /* ... */
> + };
> +
> + &dma {
> + /* ... */
> + };
> +
> + /* Board DTS - alternative order, keep as DTSI */
> +
> + &dma {
> + /* ... */
> + };
> +
> + &clk {
> + /* ... */
> + };
> +
> +Order of Properties in Device Node
> +----------------------------------
> +
> +Following order of properties in device nodes is preferred:
"Following" should be replaced with "The following".
> +
> +1. compatible
> +2. reg
> +3. ranges
> +4. Standard/common properties (defined by common bindings, e.g.
> without
> + vendor-prefixes)
> +5. Vendor-specific properties
> +6. status (if applicable)
> +7. Child nodes, where each node is preceded with a blank line
> +
> +The "status" property is by default "okay", thus it can be omitted.
> +
> +Above order follows approach:
The last sentence should be replaced with "The above-described ordering
follows this approach:".
> +
> +1. Most important properties start the node: compatible then bus
> addressing to
> + match unit address.
> +2. Each node will have common properties in similar place.
> +3. Status is the last information to annotate that device node is or
> is not
> + finished (board resources are needed).
> +
> +Example::
> +
> + /* SoC DTSI */
> +
> + device_node: device-class@6789abc {
> + compatible = "vendor,device";
> + reg = <0x0 0x06789abc 0x0 0xa123>;
> + ranges = <0x0 0x0 0x06789abc 0x1000>;
> + #dma-cells = <1>;
> + clocks = <&clock_controller 0>, <&clock_controller 1>;
> + clock-names = "bus", "host";
> + vendor,custom-property = <2>;
> + status = "disabled";
> +
> + child_node: child-class@100 {
> + reg = <0x100 0x200>;
> + /* ... */
> + };
> + };
> +
> + /* Board DTS */
> +
> + &device_node {
> + vdd-supply = <&board_vreg1>;
> + status = "okay";
> + }
> +
> +Indentation
> +-----------
> +
> +1. Use indentation according to :ref:`codingstyle`.
> +2. For arrays spanning across lines, it is preferred to align the
> continued
> + entries with opening < from the first line.
> +3. Each entry in arrays with multiple cells (e.g. "reg" with two IO
> addresses)
> + shall be enclosed in <>.
The "(e.g. ...)" form should be replaced with ", e.g. ...,".
> +
> +Example::
> +
> + thermal-sensor@c271000 {
> + compatible = "qcom,sm8550-tsens", "qcom,tsens-v2";
> + reg = <0x0 0x0c271000 0x0 0x1000>,
> + <0x0 0x0c222000 0x0 0x1000>;
> + };
> +
> +Organizing DTSI and DTS
> +-----------------------
> +
> +The DTSI and DTS files shall be organized in a way representing the
> common
> +(and re-usable) parts of the hardware. Typically this means
> organizing DTSI
The "(and re-usable)" form should be replaced with ", reusable". "The
hardware" should be replaced with "hardware". A comma should be added
after "Typically".
> +and DTS files into several files:
> +
> +1. DTSI with contents of the entire SoC (without nodes for hardware
> not present
> + on the SoC).
> +2. If applicable: DTSI with common or re-usable parts of the hardware
> (e.g.
> + entire System-on-Module).
> +3. DTS representing the board.
The "(...)" forms should be replaced with ", ...". Periods at the ends
of list items should be deleted, because those aren't real, complete
sentences.
> +
> +Hardware components which are present on the board shall be placed in
> the
"Which" should be replaced with "that".
> +board DTS, not in the SoC or SoM DTSI. A partial exception is a
> common
> +external reference SoC-input clock, which could be coded as a
> fixed-clock in
"SoC-input" should be replaced with "SoC input".
> +the SoC DTSI with its frequency provided by each board DTS.
> diff --git a/Documentation/devicetree/bindings/index.rst
> b/Documentation/devicetree/bindings/index.rst
> index d9002a3a0abb..cc1fbdc05657 100644
> --- a/Documentation/devicetree/bindings/index.rst
> +++ b/Documentation/devicetree/bindings/index.rst
> @@ -4,6 +4,7 @@
> :maxdepth: 1
>
> ABI
> + dts-coding-style
> writing-bindings
> writing-schema
> submitting-patches
^ permalink raw reply
* Re: [PATCH] dt-bindings: net: wireless: mt76: add interrupts description for MT7986
From: Rob Herring @ 2023-11-28 17:37 UTC (permalink / raw)
To: Peter Chiu
Cc: Felix Fietkau, Lorenzo Bianconi, Ryder Lee, Evelyn Tsai,
Shayne Chen, Sam Shih, linux-wireless, linux-mediatek, devicetree
In-Reply-To: <20231128035723.5217-1-chui-hao.chiu@mediatek.com>
On Tue, Nov 28, 2023 at 11:57:23AM +0800, Peter Chiu wrote:
> The mt7986 can support four interrupts to distribute the interrupts
> to different CPUs.
>
> Signed-off-by: Peter Chiu <chui-hao.chiu@mediatek.com>
> ---
> .../bindings/net/wireless/mediatek,mt76.yaml | 24 +++++++++++++++----
> 1 file changed, 20 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.yaml b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.yaml
> index 252207adbc54..20f5f2ead265 100644
> --- a/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.yaml
> +++ b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.yaml
> @@ -19,9 +19,6 @@ description: |
> Alternatively, it can specify the wireless part of the MT7628/MT7688
> or MT7622/MT7986 SoC.
>
> -allOf:
> - - $ref: ieee80211.yaml#
> -
> properties:
> compatible:
> enum:
> @@ -38,7 +35,8 @@ properties:
> MT7986 should contain 3 regions consys, dcm, and sku, in this order.
>
> interrupts:
> - maxItems: 1
> + minItems: 1
> + maxItems: 4
>
> power-domains:
> maxItems: 1
> @@ -219,6 +217,24 @@ required:
>
> unevaluatedProperties: false
>
> +allOf:
> + - $ref: ieee80211.yaml#
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - mediatek,mt7986-wmac
> + then:
> + properties:
> + interrupts:
> + minItems: 1
> + items:
> + - description: major interrupt for rings
> + - description: addditional interrupt for ring 19
> + - description: addditional interrupt for ring 4
> + - description: addditional interrupt for ring 5
This list belongs in the top level. The if/then schema needs to set
'maxItems: 1' for the cases with only 1 interrupt and 'minItems: 4' for
the cases with 4 interrupts.
Rob
^ permalink raw reply
* Re: [PATCH] dt-bindings: pwm: remove Xinlei's mail
From: Thierry Reding @ 2023-11-28 17:49 UTC (permalink / raw)
To: Uwe Kleine-König, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Matthias Brugger, AngeloGioacchino Del Regno,
Jitao Shi, Michael Walle
Cc: linux-pwm, devicetree, linux-kernel, linux-arm-kernel,
linux-mediatek
In-Reply-To: <20231123134716.2033769-1-mwalle@kernel.org>
On Thu, 23 Nov 2023 14:47:16 +0100, Michael Walle wrote:
> Xinlei Lee's mail is bouncing:
>
> <xinlei.lee@mediatek.com>: host mailgw02.mediatek.com[216.200.240.185] said:
> 550 Relaying mail to xinlei.lee@mediatek.com is not allowed (in reply to
> RCPT TO command)
>
> Remove it.
>
> [...]
Applied, thanks!
[1/1] dt-bindings: pwm: remove Xinlei's mail
commit: c7861b3ffaba39021968505bd6807bf4fa7148c9
Best regards,
--
Thierry Reding <thierry.reding@gmail.com>
^ permalink raw reply
* Re: [RFC PATCH v3 3/5] platform/chrome: Introduce device tree hardware prober
From: Andy Shevchenko @ 2023-11-28 16:25 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Wolfram Sang,
Benson Leung, Tzung-Bi Shih, chrome-platform, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, Douglas Anderson,
Johan Hovold, Hsin-Yi Wang, Dmitry Torokhov, Jiri Kosina,
linus.walleij, broonie, gregkh, hdegoede, james.clark, james,
keescook, rafael, tglx, Jeff LaBundy, linux-input, linux-i2c
In-Reply-To: <20231128084236.157152-4-wenst@chromium.org>
On Tue, Nov 28, 2023 at 04:42:32PM +0800, Chen-Yu Tsai wrote:
> Some devices are designed and manufactured with some components having
> multiple drop-in replacement options. These components are often
> connected to the mainboard via ribbon cables, having the same signals
> and pin assignments across all options. These may include the display
> panel and touchscreen on laptops and tablets, and the trackpad on
> laptops. Sometimes which component option is used in a particular device
> can be detected by some firmware provided identifier, other times that
> information is not available, and the kernel has to try to probe each
> device.
>
> This change attempts to make the "probe each device" case cleaner. The
> current approach is to have all options added and enabled in the device
> tree. The kernel would then bind each device and run each driver's probe
> function. This works, but has been broken before due to the introduction
> of asynchronous probing, causing multiple instances requesting "shared"
> resources, such as pinmuxes, GPIO pins, interrupt lines, at the same
> time, with only one instance succeeding. Work arounds for these include
> moving the pinmux to the parent I2C controller, using GPIO hogs or
> pinmux settings to keep the GPIO pins in some fixed configuration, and
> requesting the interrupt line very late. Such configurations can be seen
> on the MT8183 Krane Chromebook tablets, and the Qualcomm sc8280xp-based
> Lenovo Thinkpad 13S.
>
> Instead of this delicate dance between drivers and device tree quirks,
> this change introduces a simple I2C component prober. For any given
> class of devices on the same I2C bus, it will go through all of them,
> doing a simple I2C read transfer and see which one of them responds.
> It will then enable the device that responds.
>
> This requires some minor modifications in the existing device tree.
> The status for all the device nodes for the component options must be
> set to "failed-needs-probe". This makes it clear that some mechanism is
> needed to enable one of them, and also prevents the prober and device
> drivers running at the same time.
...
> +#include <linux/array_size.h>
> +#include <linux/i2c.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
init.h for init calls.
> +static int chromeos_of_hw_prober_probe(struct platform_device *pdev)
> +{
> + for (size_t i = 0; i < ARRAY_SIZE(hw_prober_platforms); i++)
> + if (of_machine_is_compatible(hw_prober_platforms[i].compatible)) {
> + int ret;
Perhaps
if (!of_machine_is_compatible(hw_prober_platforms[i].compatible))
continue;
?
> + ret = hw_prober_platforms[i].prober(&pdev->dev,
> + hw_prober_platforms[i].data);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC PATCH v3 2/5] i2c: of: Introduce component probe function
From: Andy Shevchenko @ 2023-11-28 16:22 UTC (permalink / raw)
To: Chen-Yu Tsai
Cc: Rob Herring, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Wolfram Sang,
Benson Leung, Tzung-Bi Shih, chrome-platform, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, Douglas Anderson,
Johan Hovold, Hsin-Yi Wang, Dmitry Torokhov, Jiri Kosina,
linus.walleij, broonie, gregkh, hdegoede, james.clark, james,
keescook, rafael, tglx, Jeff LaBundy, linux-input, linux-i2c
In-Reply-To: <20231128084236.157152-3-wenst@chromium.org>
On Tue, Nov 28, 2023 at 04:42:31PM +0800, Chen-Yu Tsai wrote:
> Some devices are designed and manufactured with some components having
> multiple drop-in replacement options. These components are often
> connected to the mainboard via ribbon cables, having the same signals
> and pin assignments across all options. These may include the display
> panel and touchscreen on laptops and tablets, and the trackpad on
> laptops. Sometimes which component option is used in a particular device
> can be detected by some firmware provided identifier, other times that
> information is not available, and the kernel has to try to probe each
> device.
>
> This change attempts to make the "probe each device" case cleaner. The
> current approach is to have all options added and enabled in the device
> tree. The kernel would then bind each device and run each driver's probe
> function. This works, but has been broken before due to the introduction
> of asynchronous probing, causing multiple instances requesting "shared"
> resources, such as pinmuxes, GPIO pins, interrupt lines, at the same
> time, with only one instance succeeding. Work arounds for these include
> moving the pinmux to the parent I2C controller, using GPIO hogs or
> pinmux settings to keep the GPIO pins in some fixed configuration, and
> requesting the interrupt line very late. Such configurations can be seen
> on the MT8183 Krane Chromebook tablets, and the Qualcomm sc8280xp-based
> Lenovo Thinkpad 13S.
>
> Instead of this delicate dance between drivers and device tree quirks,
> this change introduces a simple I2C component probe. function For a
> given class of devices on the same I2C bus, it will go through all of
> them, doing a simple I2C read transfer and see which one of them responds.
> It will then enable the device that responds.
>
> This requires some minor modifications in the existing device tree. The
> status for all the device nodes for the component options must be set
> to "failed-needs-probe". This makes it clear that some mechanism is
> needed to enable one of them, and also prevents the prober and device
> drivers running at the same time.
...
> +/**
> + * i2c_of_probe_component() - probe for devices of "type" on the same i2c bus
> + * @dev: &struct device of the caller, only used for dev_* printk messages
> + * @type: a string to match the device node name prefix to probe for
> + *
> + * Probe for possible I2C components of the same "type" on the same I2C bus
> + * that have their status marked as "fail".
Definitely you haven't run kernel-doc validation.
> + */
...
> + return dev_err_probe(dev, -ENODEV, "Could not find %s device node\n", type);
I haven't noticed clear statement in the description that this API is only for
the ->probe() stages.
...
> + if (i2c_smbus_xfer(i2c, addr, 0, I2C_SMBUS_READ, 0, I2C_SMBUS_BYTE, &data) < 0)
> + continue;
This will require the device to be powered on. Are you sure it will be always
the case?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [Patch v6 10/12] pstore/ram: Add dynamic ramoops region support through commandline
From: Pavan Kondeti @ 2023-11-28 16:03 UTC (permalink / raw)
To: Mukesh Ojha
Cc: Pavan Kondeti, corbet, agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, keescook, tony.luck, gpiccoli,
mathieu.poirier, vigneshr, nm, matthias.bgg, kgene, alim.akhtar,
bmasney, linux-doc, linux-kernel, linux-arm-msm, linux-hardening,
linux-remoteproc, linux-arm-kernel, linux-mediatek,
linux-samsung-soc, kernel
In-Reply-To: <d5fa95e7-ad2a-0dc7-5c79-6a9a789dad5f@quicinc.com>
On Tue, Nov 28, 2023 at 04:02:33PM +0530, Mukesh Ojha wrote:
> > > static int ramoops_pstore_open(struct pstore_info *psi)
> > > {
> > > @@ -915,14 +965,18 @@ static void __init ramoops_register_dummy(void)
> > > /*
> > > * Prepare a dummy platform data structure to carry the module
> > > - * parameters. If mem_size isn't set, then there are no module
> > > - * parameters, and we can skip this.
> > > + * parameters. If mem_size isn't set, check for dynamic ramoops
> > > + * size and use if it is set.
> > > */
> > > - if (!mem_size)
> > > + if (!mem_size && !dyn_ramoops_size)
> > > return;
> >
> > If mem_size and dyn_ramoops_size are set, you are taking
> > dyn_ramoops_size precedence here. The comment is a bit confusing, pls
> > review it once.
>
> Ideally, both should not be set and there will always be
> confusion.
>
> Do you think, if we use mem_size a single variable both for earlier
> and dynamic ramoops where based on dyn_ramoops_size=true/on a boolean
> it will take dynamic ramoops path and if not mentioned it will take older
> path.
>
Sounds like a good idea to me. You would need a callback for mem_size
module param handling. Because dyn_ramoops can be passed before mem_size
parameter. Also, we can't make pstore ram as module i.e decoupling
dynamic ramoops from pstore ram module.
Thanks,
Pavan
^ permalink raw reply
* Re: [PATCH v2 12/21] pinctrl: core: Embed struct pingroup into struct group_desc
From: Andy Shevchenko @ 2023-11-28 15:58 UTC (permalink / raw)
To: kernel test robot
Cc: Linus Walleij, Bartosz Golaszewski, Rasmus Villemoes,
Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc, oe-kbuild-all, Ray Jui, Scott Branden,
Broadcom internal kernel review list, Dong Aisheng, Fabio Estevam,
Shawn Guo, Jacky Bai, Pengutronix Kernel Team, Sascha Hauer,
NXP Linux Team, Sean Wang
In-Reply-To: <ZWYM8Pjl-S-8CMPu@smile.fi.intel.com>
On Tue, Nov 28, 2023 at 05:53:21PM +0200, Andy Shevchenko wrote:
> On Sat, Nov 25, 2023 at 07:39:02AM +0800, kernel test robot wrote:
> > Hi Andy,
> >
> > kernel test robot noticed the following build errors:
>
> > [also build test ERROR on linusw-pinctrl/for-next next-20231124]
>
> Hmm... I have compiled tested on Linux Next it several times, I can't reproduce
> this neither with GCC nor with LLVM.
Actually it rings a bell that some versions of GCC have a bug (?) that they may
not identify initializations like this to be converted to constants as we are
using compound literal it should make no difference.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2 12/21] pinctrl: core: Embed struct pingroup into struct group_desc
From: Andy Shevchenko @ 2023-11-28 15:53 UTC (permalink / raw)
To: kernel test robot
Cc: Linus Walleij, Bartosz Golaszewski, Rasmus Villemoes,
Jonathan Neuschäfer, Krzysztof Kozlowski,
Uwe Kleine-König, Geert Uytterhoeven, Biju Das,
Claudiu Beznea, Jianlong Huang, linux-arm-kernel, linux-gpio,
linux-kernel, linux-mediatek, openbmc, linux-mips, linux-arm-msm,
linux-renesas-soc, oe-kbuild-all, Ray Jui, Scott Branden,
Broadcom internal kernel review list, Dong Aisheng, Fabio Estevam,
Shawn Guo, Jacky Bai, Pengutronix Kernel Team, Sascha Hauer,
NXP Linux Team, Sean Wang
In-Reply-To: <202311250448.uz5Yom3N-lkp@intel.com>
On Sat, Nov 25, 2023 at 07:39:02AM +0800, kernel test robot wrote:
> Hi Andy,
>
> kernel test robot noticed the following build errors:
> [also build test ERROR on linusw-pinctrl/for-next next-20231124]
Hmm... I have compiled tested on Linux Next it several times, I can't reproduce
this neither with GCC nor with LLVM.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [Patch v6 03/12] docs: qcom: Add qualcomm minidump guide
From: Bryan O'Donoghue @ 2023-11-28 13:15 UTC (permalink / raw)
To: Mukesh Ojha, corbet, agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, keescook, tony.luck, gpiccoli,
mathieu.poirier, vigneshr, nm, matthias.bgg, kgene, alim.akhtar,
bmasney
Cc: linux-doc, linux-kernel, linux-arm-msm, linux-hardening,
linux-remoteproc, linux-arm-kernel, linux-mediatek,
linux-samsung-soc, kernel
In-Reply-To: <1700864395-1479-4-git-send-email-quic_mojha@quicinc.com>
On 24/11/2023 22:19, Mukesh Ojha wrote:
> Add the qualcomm minidump guide for the users which tries to cover
> the dependency, API use and the way to test and collect minidump
> on Qualcomm supported SoCs.
>
> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
> ---
> Documentation/admin-guide/index.rst | 1 +
> Documentation/admin-guide/qcom_minidump.rst | 272 ++++++++++++++++++++++++++++
> 2 files changed, 273 insertions(+)
> create mode 100644 Documentation/admin-guide/qcom_minidump.rst
General nit-pick on sequencing of your patches. Its good practice (tm)
to put your documentation first in your series.
> diff --git a/Documentation/admin-guide/index.rst b/Documentation/admin-guide/index.rst
> index 43ea35613dfc..251d070486c2 100644
> --- a/Documentation/admin-guide/index.rst
> +++ b/Documentation/admin-guide/index.rst
> @@ -120,6 +120,7 @@ configure specific aspects of kernel behavior to your liking.
> perf-security
> pm/index
> pnp
> + qcom_minidump
> rapidio
> ras
> rtc
> diff --git a/Documentation/admin-guide/qcom_minidump.rst b/Documentation/admin-guide/qcom_minidump.rst
> new file mode 100644
> index 000000000000..b492f2b79639
> --- /dev/null
> +++ b/Documentation/admin-guide/qcom_minidump.rst
> @@ -0,0 +1,272 @@
> +Qualcomm minidump feature
> +=========================
> +
> +Introduction
> +------------
> +
> +Minidump is a best effort mechanism to collect useful and predefined
> +data for first level of debugging on end user devices running on
> +Qualcomm SoCs.
What does "first-level debugging" mean here ? You use the term
"post-mortem" later, stick with one term for consistency throughout your
document.
Suggest:
"Minidump is a best-effort mechanism to collect useful predefined data
for post-mortem debugging on a Qualcomm System on Chips (SoCs)."
Generally better to stick with "Qualcomm SoC" and "Minidump" once you
establish the terms upfront and early in your text - instead of
reverting to "device" and "it".
It is built on the premise that System on Chip (SoC)
> +or subsystem part of SoC crashes, due to a range of hardware and
> +software bugs.
Instead of saying "It" say "Minidump"
Hence, the ability to collect accurate data is only
> +a best-effort. The data collected could be invalid or corrupted, data
> +collection itself could fail, and so on.
"and so on" is a redundancy, drop.
Suggest:
Minidump is built on the premise that a hardware or software component
on the SoC has encountered an unexpected fault. This means that the data
collected by minidump cannot be assumed to be correct or even present.
> +
> +Qualcomm devices in engineering mode provides a mechanism for generating
> +full system RAM dumps for post-mortem debugging.
Stick with the established naming convention
"Qualcomm SoCs in engineering mode provide a mechanism for generating
full system RAM dumps for this post-mortem debugging"
But in some cases it's
> +however not feasible to capture the entire content of RAM. The minidump
> +mechanism provides the means for selected region should be included in
> +the ramdump.
Dont' start a sentence with "But"
You're also not being clear what you mean by "the entire content of RAM"
since you obviously can't capture a full snap-shot of DRAM and store in
DRAM.
Better to say IMO
"Minidump captures specific pre-defined regions of RAM and stores those
regions in a reserved Minidump specific buffer."
"The region of RAM used to store Minidump data shall be referred to as
SMEM throughout this document." [1]
> +::
> +
> + +-----------------------------------------------+
> + | DDR +-------------+ |
Instead of saying "DDR" just mark this as RAM or Memory.
Its a terrible nit-pick from me but DDR = "Double Data Rate Synchronous
Dynamic Random-Access Memory" but that's irrelevant to this
specification. We could be living in a gigantic SRAM for argument sake.
> + | | SS0-ToC| |
> + | +----------------+ +----------------+ | |
> + | |Shared memory | | SS1-ToC| | |
> + | |(SMEM) | | | | |
> + | | | +-->|--------+ | | |
> + | |G-ToC | | | SS-ToC \ | | |
> + | |+-------------+ | | | +-----------+ | | |
> + | ||-------------| | | | |-----------| | | |
> + | || SS0-ToC | | | +-|<|SS1 region1| | | |
> + | ||-------------| | | | | |-----------| | | |
> + | || SS1-ToC |-|>+ | | |SS1 region2| | | |
> + | ||-------------| | | | |-----------| | | |
> + | || SS2-ToC | | | | | ... | | | |
> + | ||-------------| | | | |-----------| | | |
> + | || ... | | |-|<|SS1 regionN| | | |
> + | ||-------------| | | | |-----------| | | |
> + | || SSn-ToC | | | | +-----------+ | | |
> + | |+-------------+ | | | | | |
> + | | | | |----------------| | |
> + | | | +>| regionN | | |
> + | | | | |----------------| | |
> + | +----------------+ | | | | |
> + | | |----------------| | |
> + | +>| region1 | | |
> + | |----------------| | |
> + | | | | |
> + | |----------------|-+ |
> + | | region5 | |
> + | |----------------| |
> + | | | |
> + | Region information +----------------+ |
> + | +---------------+ |
> + | |region name | |
> + | |---------------| |
> + | |region address | |
> + | |---------------| |
> + | |region size | |
> + | +---------------+ |
> + +-----------------------------------------------+
> + G-ToC: Global table of contents
> + SS-ToC: Subsystem table of contents
> + SS0-SSn: Subsystem numbered from 0 to n
You need to be more consistent here
SSX-ToC -> SSX-SSn
Where X is an integer from 0 upwards and similarly n is a integer from 0
upwards.
This name SSX-SSn is not especially descriptive, I'm not sure if this is
a name you are choosing here ? If so then let me suggest a new name like
"Subsystem Memory Segment" SSX-MSn
->
G-ToC: Global table of contents
SSX-ToC: Subsystem X table of contents.
X is an integer in the range of 0 to ?
Is there an upper limit ?
Presumably this is an 8, 16, 32 or 64 bit integer
Please define either the size of the integer or the
valid range of values 0..128, 0..256
SSX-MSn: Subsystem numbered from 0 to n
Same comment for the 'n' here.
> +
> +It depends on SoC where the underlying firmware is keeping the
> +minidump global table taking care of subsystem ToC part for
> +minidump like for above diagram, it is for shared memory sitting
> +in DDR and it is shared among various master however it is possible
> +that this could be implemented via memory mapped regions but the
> +general idea should remain same. Here, various subsystem could be
> +DSP's like ADSP/CDSP/MODEM etc, along with Application processor
> +(APSS) where Linux runs.
DSP minidump gets collected when DSP's goes
> +for recovery followed by a crash. The minidump part of code for
> +that resides in ``qcom_rproc_minidump.c``.
This paragraph is difficult to parse.
What you are describing here is a linked list, I think you should have a
paragraph describing how the memory structure works
->
"Minidump determines which areas of DRAM to capture via a Minidump
defined linked-list structure.
At the top level a Global Table of Contents (GTOC) enumerates a variable
number of SubSystem Table Of Contents (SSTOC) structures.
Each SSTOC contains a list of SubSystem Memory Segements which are named
according to the containing SSTOC hence (SSX-MSn) where "X" denotes the
SystemSystem index of the containing SSX-ToC and "n" denotes an
individual Memory Segment within the SystemSystem. Hence SS0-MS0 belongs
to SS0-ToC whereas SS1-MS0 belongs to SS1-ToC."
Then I think you can describe how the crash dump colleciton works and
which agents of the system - DSP ? is responsible for collecting the
crashdump
->
"The Application Processor SubSystem (APSS) runs the Linux kernel and is
therefore not responsible for assembling Minidump data. One of the other
system agents in the SoC will be responsible for capturing the Minidump
data during system reset.
Typically one of the SoC Digital Signal Processors (DSP) will be used
for this purpose.
During reset the DSP will walk the GTOC, SSX-ToCs and SSX-MSns
populating the Minidump RAM area with the indicated memory"
> +
> +
> +SMEM as backend
> +----------------
> +
> +In this document, SMEM will be used as the backend implementation
> +of minidump.
[1] As per the above link, you need to introduce the term SMEM earlier.
It's fine to expand on its meaning later but, do please define it once
upfront before you use it in your awesome ASCII art.
> +The core of minidump feature is part of Qualcomm's boot firmware code.
> +It initializes shared memory (SMEM), which is a part of DDR and
> +allocates a small section of it to minidump table, i.e. also called
> +global table of contents (G-ToC). Each subsystem (APSS, ADSP, ...) has
> +its own table of segments to be included in the minidump, all
> +references from a descriptor in SMEM (G-ToC). Each segment/region has
> +some details like name, physical address and its size etc. and it
> +could be anywhere scattered in the DDR.
->
"The SoC's bootloader must reserve an area of RAM as SMEM prior to
handing over control to the run-time operating system. The bootloader is
responsible to place the GTOC at the starting address of SMEM."
If you want to give more technical details of size, physical address -
then explicitly define those in the section above which talks about the
linked-list structure.
Please try to avoid use of "etc" or "and so on" since it assumes the
reader already knows how the system works and can fill in the blanks
but, what you are doing here is educating a Minidump novice in how
things work.
> +
> +Qualcomm APSS Minidump kernel driver concept
> +--------------------------------------------
> +
> +Qualcomm APSS minidump kernel driver adds the capability to add Linux
So why "Minidump" and then "minidump" choose one.
> +region to be dumped as part of RAM dump collection.
OK so this really is the "meat" of the system. The bootloader/firmware
populates the GTOC.
The Q this document should probably answer is how the kernel driver
knows how/where to place its data.
Assumed to be parsing the DTB.
At the moment,
> +shared memory driver creates platform device for minidump driver and
> +give a means to APSS minidump to initialize itself on probe.
"At the moment" is another drop.
Just make a clear statement
"The minidump platform driver populates the APSS porition of the GTOC"
more interesting to me is - are there defined numbers, identifiers for
the APSS ? or do we just add new entries to the GTOC ?
ie. is there a reserved index or "type" in the GTOC that identifies
where the APSS needs to insert itself ?
> +This driver provides ``qcom_minidump_region_register`` and
> +``qcom_minidump_region_unregister`` API's to register and unregister
> +APSS minidump region.
Why does it do that ? Is it not the case that the driver knows where the
APSS data goes ?
It also supports registration for the clients
> +who came before minidump driver was initialized. It maintains pending
> +list of clients who came before minidump and once minidump is initialized
> +it registers them in one go.
Don't start sentences with "It" -> "The driver" or "Minidump"
As I read this though, the Minidump driver in Linux isn't just
registering / managing the APSS side of things but also "doing stuff"
for other clients ?
How does the Linux driver know what to register ?
> +
> +To simplify post-mortem debugging, driver creates and maintain an ELF
the driver creates and maintains
> +header as first region that gets updated each time a new region gets
> +registered.
as the first region
So - who is registering these regions ? Linux kernel drivers ? aDSP / cDSP ?
If I write a new driver for Venus or GPU can I define my own region(s)
to be captured ?
Presumably. Please give more detail on this.
> +
> +The solution supports extracting the RAM dump/minidump produced either
> +over USB or stored to an attached storage device.
What provides that functionality ? The bootloader ?
How do you trigger / capture that dump from the bootloader ?
No need to go into super-detail but give some idea.
> +
> +Dependency of minidump kernel driver
> +------------------------------------
> +
> +It is to note that whole of minidump depends on Qualcomm boot firmware
> +whether it supports minidump or not.
You can drop this - you've already stated the bootloader/firmware must
setup the initial table so, you're not providing additional information
with this statement.
> So, if the minidump SMEM ID is
Try not to start sentences with "So"
SMEM ID ? This is your first time using this term - please relate it
back to your ASCII diagram and the description you give with that text.
> +present in shared memory, it indicates that minidump is supported from
> +boot firmware and it is possible to dump Linux (APSS) region as part
> +of minidump collection.
If _which_ SMEM ID ?
It seems to me as if we are missing some important information here -
what are the list of SMEM IDs ?
Are these IDs serial and incrementing across SoC versions or SoC specific ?
You need to define that data.
> +How a kernel client driver can register region with minidump
> +------------------------------------------------------------
Answering yes to my earlier question. A driver I write can make use of
the API you are providing here.
Great. Please give some indication of that earlier, even if its a
reference to this description you give here "See X.Y later in this document"
> +
> +Client driver can use ``qcom_minidump_region_register`` API's to register
> +and ``qcom_minidump_region_unregister`` to unregister their region from
> +minidump driver.
> +
> +Client needs to fill their region by filling ``qcom_minidump_region``
> +structure object which consists of the region name, region's virtual
> +and physical address and its size.
Nit pick. You need a definite article here "A client driver" etc.
> +
> +Below, is one sample client driver snippet which tries to allocate a
> +region from kernel heap of certain size and it writes a certain known
> +pattern.
Good
(that can help in verification after collection that we got
> +the exact pattern, what we wrote) and registers it with minidump.
Not necessary to define this. We are all smart here and by now the
intent of the mechanism is defined..
> +
> + .. code-block:: c
> +
> + #include <soc/qcom/qcom_minidump.h>
> + [...]
> +
> +
> + [... inside a function ...]
> + struct qcom_minidump_region region;
> +
> + [...]
> +
> + client_mem_region = kzalloc(region_size, GFP_KERNEL);
> + if (!client_mem_region)
> + return -ENOMEM;
> +
> + [... Just write a pattern ...]
> + memset(client_mem_region, 0xAB, region_size);
> +
> + [... Fill up the region object ...]
> + strlcpy(region.name, "REGION_A", sizeof(region.name));
> + region.virt_addr = client_mem_region;
> + region.phys_addr = virt_to_phys(client_mem_region);
> + region.size = region_size;
> +
> + ret = qcom_minidump_region_register(®ion);
> + if (ret < 0) {
> + pr_err("failed to add region in minidump: err: %d\n", ret);
> + return ret;
> + }
> +
> + [...]
> +
> +
> +Test
> +----
Testing
> +
> +Existing Qualcomm devices already supports entire RAM dump (also called
> +full dump) by writing appropriate value to Qualcomm's top control and
> +status register (tcsr) in ``driver/firmware/qcom_scm.c`` .
"Existing Qualcomm SoCs already support dumping the entire RAM to the
SMEM area/segment/whatever"
This is 100% counter-intuitive since SMEM lives in RAM, correct ?
Full dump means what, a full dump of the APSS RAM ? What happens if SMEM
cannot accommodate the full APSS RAM dump ?
> +
> +SCM device Tree bindings required to support download mode
> +For example (sm8450) ::
> +
> + / {
> +
> + [...]
> +
> + firmware {
> + scm: scm {
> + compatible = "qcom,scm-sm8450", "qcom,scm";
> + [... tcsr register ... ]
> + qcom,dload-mode = <&tcsr 0x13000>;
> +
> + [...]
> + };
> + };
> +
> + [...]
> +
> + soc: soc@0 {
> +
> + [...]
> +
> + tcsr: syscon@1fc0000 {
> + compatible = "qcom,sm8450-tcsr", "syscon";
> + reg = <0x0 0x1fc0000 0x0 0x30000>;
> + };
> +
> + [...]
> + };
> + [...]
> +
> + };
> +
> +User of minidump can pass ``qcom_scm.download_mode="mini"`` to kernel
> +commandline to set the current download mode to minidump.
"A kernel command line parameter is provided
``qcom_scm.download_mode="mini"`` to facilitate ... but you aren't
telling us what "minidump" captures "the current download" ? do you mean
the current state ?
Does the system continue to boot up if you pass
qcom_scm.download_mode="mini ? will additional registrations to
SMEM/Minidump work ?
What happens to the minidump data if there is a _subsequent_ real
crashdump ?
Overwritten ?
Also what happens if SMEM runs out of space ? Say I boot with
``qcom_scm.download_mode="mini"`` and then the system crashes - SMEM has
a limit right ?
So the minidump gets overwritten ?
> +Similarly, ``"full"`` is passed to set the download mode to full dump
> +where entire RAM dump will be collected while setting it ``"full,mini"``
> +will collect minidump along with fulldump.
Still not super-clear what the difference between mini and full is here.
> +
> +Writing to sysfs node can also be used to set the mode to minidump::
> +
> + echo "mini" > /sys/module/qcom_scm/parameter/download_mode
> +
> +Once the download mode is set, any kind of crash will make the device collect
> +respective dump as per set download mode.
Nice.
> +
> +Dump collection
> +---------------
> +::
> +
> + +-----------+
> + | |
> + | | +------+
> + | | | |
> + | | +--+---+ Product(Qualcomm SoC)
> + +-----------+ |
> + |+++++++++++|<------------+
> + |+++++++++++| usb cable
> + +-----------+
> + x86_64 PC
> +
> +The solution supports a product running with Qualcomm SoC (where minidump)
> +is supported from the firmware) connected to x86_64 host PC running PCAT
> +tool.
It supports downloading the minidump produced from product to the
> +host PC over USB or to save the minidump to the product attached storage
> +device(UFS/eMMC/SD Card) into minidump dedicated partition.
It would be a good idea to reference this section earlier.
> +
> +By default, dumps are downloaded via USB to the attached x86_64 PC running
> +PCAT (Qualcomm tool) software. Upon download, we will see a set of binary
> +blobs starting with name ``md_*`` in PCAT configured directory in x86_64
> +machine, so for above example from the client it will be ``md_REGION_A.BIN``.
> +This binary blob depends on region content to determine whether it needs
> +external parser support to get the content of the region, so for simple
> +plain ASCII text we don't need any parsing and the content can be seen
> +just opening the binary file.
> +
> +To collect the dump to attached storage type, one needs to write appropriate
> +value to IMEM register, in that case dumps are collected in rawdump
> +partition on the product device itself.
> +
> +One needs to read the entire rawdump partition and pull out content to
> +save it onto the attached x86_64 machine over USB. Later, this rawdump
> +can be passed to another tool (``dexter.exe`` [Qualcomm tool]) which
> +converts this into the similar binary blobs which we have got it when
> +download type was set to USB, i.e. a set of registered regions as blobs
> +and their name starts with ``md_*``.
> +
> +Replacing the ``dexter.exe`` with some open source tool can be added as future
> +scope of this document.
---
bod
^ permalink raw reply
* Re: [net-next PATCH RFC v3 2/8] net: phy: add initial support for PHY package in DT
From: Christian Marangi @ 2023-11-28 12:09 UTC (permalink / raw)
To: Andrew Lunn
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Andy Gross,
Bjorn Andersson, Konrad Dybcio, Heiner Kallweit, Russell King,
Matthias Brugger, AngeloGioacchino Del Regno, Robert Marko,
netdev, devicetree, linux-kernel, linux-arm-msm, linux-arm-kernel,
linux-mediatek
In-Reply-To: <b28b5d10-08cd-4e30-9909-f37834d80c81@lunn.ch>
On Tue, Nov 28, 2023 at 01:39:02AM +0100, Andrew Lunn wrote:
> > +static int of_phy_package(struct phy_device *phydev)
> > +{
> > + struct device_node *node = phydev->mdio.dev.of_node;
> > + struct device_node *package_node;
> > + u32 base_addr;
> > + int ret;
> > +
> > + if (!node)
> > + return 0;
> > +
> > + package_node = of_get_parent(node);
> > + if (!package_node)
> > + return 0;
> > +
> > + if (!of_device_is_compatible(package_node, "ethernet-phy-package"))
> > + return 0;
> > +
> > + if (of_property_read_u32(package_node, "reg", &base_addr))
> > + return -EINVAL;
> > +
> > + ret = devm_phy_package_join(&phydev->mdio.dev, phydev,
> > + base_addr, 0);
>
> No don't do this. It is just going to lead to errors. The PHY driver
> knows how many PHYs are in the package. So it can figure out what the
> base address is and create the package. It can add each PHY as they
> probe. That cannot go wrong.
>
No it can't and we experiment this with a funny implementation on the
QSDK. Maybe I'm confused?
Example on QSDK they were all based on a value first_phy_addr. This was
filled with the first phy addr found (for the package).
All OEM followed a template with declaring all sort of bloat and they
probably have no idea what they are actually putting in DT. We reverse
all the properties and we gave a meaning to all the values...
We quikly notice that the parsing was very fragile and on some strange
device (an example a Xiaomi 36000) the first PHY for the package was
actually not attached to anything. Resulting in all this logic of
"first_phy_addr" failing as by removing the first PHY, the value was set
to a wrong addr resulting in all sort of problems.
This changed a lot (the original series used a more robust way with
phandle, but it was asked to use base_addr and use offset in the PHY
addr, less robust still OK)
If we revert to PHY driver making the PHY package then we lose all kind
of ""automation"" of having a correct base addr. In PHY driver we would
have to manually parse with parent (as we do here) and check the value?
Why not do the thing directly on PHY core?
By making the PHY driver creating the package, we are back on all kind
of bloat in the PHY driver doing the same thing (parsing package nodes,
probe_once check, config_once check) instead of handling them directly
in PHY core.
Also just to point out, the way the current PHY driver derive the base
addr is problematic. All of them use special regs to find the base one
but I can totally see a PHY driver in the future assuming the first PHY
being the first in the package to be probed, set the base addr on the
first PHY and also assuming that it's always define in DT.
If we really don't want the OF parsing in PHY core and autojoin them,
is at least OK to introduce an helper to get the base addr from a PHY
package node structure? (to at least try to minimaze the bloat that PHY
driver will have?)
Also with the OF support dropped, I guess also the added API in this
series has to be dropped. (as they would be called after the first PHY
probe and that is problematic if for some reason only one PHY of the
package is declared in DT) (an alternative might be moving the
probe_once after the PHY is probed as we expect the phy_package_join
call done in the PHY probe call)
> If you create the package based on DT you have to validate that the DT
> is correct. You need the same information, the base address, how many
> packages are in the PHY, etc. So DT gains your nothing except more
> potential to get it wrong.
>
For the sake of package join, only the reg has to be validated and the
validation is just if addrs makes sense. Everything else can be done by
PHY package probe once.
> Please use DT just for properties for the package, nothing else.
>
--
Ansuel
^ permalink raw reply
* Re: [PATCH 1/3] scripts/gdb/tasks: Fix lx-ps command error
From: Kuan-Ying Lee (李冠穎) @ 2023-11-28 11:45 UTC (permalink / raw)
To: oleg@redhat.com
Cc: linux-kernel@vger.kernel.org, linux-mediatek@lists.infradead.org,
Qun-wei Lin (林群崴), linux-mm@kvack.org,
Chinwen Chang (張錦文), stable@vger.kernel.org,
Casper Li (李中榮), akpm@linux-foundation.org,
kbingham@kernel.org, linux-arm-kernel@lists.infradead.org,
matthias.bgg@gmail.com, angelogioacchino.delregno@collabora.com,
jan.kiszka@siemens.com
In-Reply-To: <20231127120314.GA19669@redhat.com>
On Mon, 2023-11-27 at 13:03 +0100, Oleg Nesterov wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> On 11/27, Kuan-Ying Lee wrote:
> >
> > @@ -25,13 +25,9 @@ def task_lists():
> > t = g = init_task
> >
> > while True:
> > - while True:
> > - yield t
> > -
> > - t = utils.container_of(t['thread_group']['next'],
> > - task_ptr_type, "thread_group")
> > - if t == g:
> > - break
> > + thread_head = t['signal']['thread_head']
> > + for thread in lists.list_for_each_entry(thread_head,
> task_ptr_type, 'thread_node'):
> > + yield thread
> >
> > t = g = utils.container_of(g['tasks']['next'],
> > task_ptr_type, "tasks")
>
> Thanks!
>
> I do not know python, but it seems that with this patch we can kill g
> or t?
> Can't
>
> def task_lists():
> task_ptr_type = task_type.get_type().pointer()
> init_task = gdb.parse_and_eval("init_task").address
> t = init_task
>
> while True:
> thread_head = t['signal']['thread_head']
> for thread in lists.list_for_each_entry(thread_head, task_ptr_type,
> 'thread_node'):
> yield thread
>
> t = utils.container_of(t['tasks']['next'],
> task_ptr_type, "tasks")
> if t == init_task:
> return
>
> work?
Yes, you are right.
I will fix it in v2.
Thanks,
Kuan-Ying Lee
>
> Oleg.
>
^ permalink raw reply
* Re: [Patch v6 11/12] pstore/ram: Add ramoops ready notifier support
From: Mukesh Ojha @ 2023-11-28 11:10 UTC (permalink / raw)
To: Pavan Kondeti
Cc: corbet, agross, andersson, konrad.dybcio, robh+dt,
krzysztof.kozlowski+dt, conor+dt, keescook, tony.luck, gpiccoli,
mathieu.poirier, vigneshr, nm, matthias.bgg, kgene, alim.akhtar,
bmasney, linux-doc, linux-kernel, linux-arm-msm, linux-hardening,
linux-remoteproc, linux-arm-kernel, linux-mediatek,
linux-samsung-soc, kernel
In-Reply-To: <3636dc3a-b62b-4ff9-bdc3-fec496a804b7@quicinc.com>
On 11/27/2023 3:40 PM, Pavan Kondeti wrote:
> On Sat, Nov 25, 2023 at 03:49:54AM +0530, Mukesh Ojha wrote:
>> Client like minidump, is only interested in ramoops
>> region addresses/size so that it could register them
>> with its table and also it is only deals with ram
>> backend and does not use pstorefs to read the records.
>> Let's introduce a client notifier in ramoops which
>> gets called when ramoops driver probes successfully
>> and it passes the ramoops region information to the
>> passed callback by the client and If the call for
>> ramoops ready register comes after ramoops probe
>> than call the callback directly.
>>
>> Signed-off-by: Mukesh Ojha <quic_mojha@quicinc.com>
>> ---
>> fs/pstore/ram.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/pstore_ram.h | 6 ++++
>> 2 files changed, 83 insertions(+)
>>
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index a6c0da8cfdd4..72341fd21aec 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -22,6 +22,7 @@
>> #include <linux/of_address.h>
>> #include <linux/memblock.h>
>> #include <linux/mm.h>
>> +#include <linux/mutex.h>
>>
>> #include "internal.h"
>> #include "ram_internal.h"
>> @@ -101,6 +102,14 @@ struct ramoops_context {
>> unsigned int ftrace_read_cnt;
>> unsigned int pmsg_read_cnt;
>> struct pstore_info pstore;
>> + /*
>> + * Lock to serialize calls to register_ramoops_ready_notifier,
>> + * ramoops_ready_notifier and read/modification of 'ramoops_ready'.
>> + */
>> + struct mutex lock;
>> + bool ramoops_ready;
>> + int (*callback)(const char *name, int id, void *vaddr,
>> + phys_addr_t paddr, size_t size);
>> };
>>
>> static struct platform_device *dummy;
>> @@ -488,6 +497,7 @@ static int ramoops_pstore_erase(struct pstore_record *record)
>> }
>>
>> static struct ramoops_context oops_cxt = {
>> + .lock = __MUTEX_INITIALIZER(oops_cxt.lock),
>> .pstore = {
>> .owner = THIS_MODULE,
>> .name = "ramoops",
>> @@ -662,6 +672,68 @@ static int ramoops_init_prz(const char *name,
>> return 0;
>> }
>>
>> +void ramoops_ready_notifier(struct ramoops_context *cxt)
>> +{
>> + struct persistent_ram_zone *prz;
>> + int i;
>> +
>> + if (!cxt->callback)
>> + return;
>> +
>> + for (i = 0; i < cxt->max_dump_cnt; i++) {
>> + prz = cxt->dprzs[i];
>> + cxt->callback("dmesg", i, prz->vaddr, prz->paddr, prz->size);
>> + }
>> +
>> + if (cxt->console_size) {
>> + prz = cxt->cprz;
>> + cxt->callback("console", 0, prz->vaddr, prz->paddr, prz->size);
>> + }
>> +
>> + for (i = 0; i < cxt->max_ftrace_cnt; i++) {
>> + prz = cxt->fprzs[i];
>> + cxt->callback("ftrace", i, prz->vaddr, prz->paddr, prz->size);
>> + }
>> +
>> + if (cxt->pmsg_size) {
>> + prz = cxt->mprz;
>> + cxt->callback("pmsg", 0, prz->vaddr, prz->paddr, prz->size);
>> + }
>> +}
>> +
>> +int register_ramoops_ready_notifier(int (*fn)(const char *, int,
>> + void *, phys_addr_t, size_t))
>> +{
>> + struct ramoops_context *cxt = &oops_cxt;
>> +
>> + mutex_lock(&cxt->lock);
>> + if (cxt->callback) {
>> + mutex_unlock(&cxt->lock);
>> + return -EEXIST;
>> + }
>> +
>> + cxt->callback = fn;
>> + if (cxt->ramoops_ready)
>> + ramoops_ready_notifier(cxt);
>> +
>> + mutex_unlock(&cxt->lock);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(register_ramoops_ready_notifier);
>> +
>
> Can you please elaborate on why do we need this custom notifier logic?
>
> why would not a standard notifier (include/linux/notifier.h) work here?
> The notifier_call callback can recieve custom data from the
> notifier chain implementer. All we need is to define a custom struct like
> struct pstore_ramoops_zone_data {
> const char *name;
> int id;
> void *vaddr;
> phys_addr_t paddr;
> size_t size;
> };
>
> and pass the pointer to array of this struct.
>
>
> btw, the current logic only supports just one client and this limitation
> is not highlighted any where.
I could work on it, was not sure if that will be helpful
for other users .
-Mukesh
>
> Thanks,
> Pavan
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox