| Message ID | 20200722201313.754671-2-saravanak@google.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v1,1/2] of: property: Add device link support for multiple DT bindings | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On 2020-07-22 15:13, Saravana Kannan wrote: > Add support for pinctrl-0 through pinctrl-8 explicitly instead of trying > to add support for pinctrl-%d properties. > > Of all the pinctrl-* properties in dts files (20322), only 47% (9531) > are pinctrl-%d properties. Of all the pinctrl-%d properties, 99.5% > (9486) are made up of pinctrl-[0-2]. > > Trying to parse all pinctrl-* properties and checking for pinctrl-%d is > unnecessarily complicated. So, just add support for pinctrl-[0-8] for > now. In the unlikely event we ever exceed pinctrl-8, we can come back > and improve this. If you were to implement the more general pinctrl-* case, roughly what would it look like (pseudo-code or english description is fine). -Frank > > Signed-off-by: Saravana Kannan <saravanak@google.com> > --- > drivers/of/property.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index b06edeb1f88b..d40d923ffeaf 100644 > --- a/drivers/of/property.c > +++ b/drivers/of/property.c > @@ -1274,6 +1274,15 @@ DEFINE_SIMPLE_PROP(interrupts_extended, "interrupts-extended", > DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", NULL) > DEFINE_SIMPLE_PROP(phys, "phys", "#phy-cells") > DEFINE_SIMPLE_PROP(wakeup_parent, "wakeup-parent", NULL) > +DEFINE_SIMPLE_PROP(pinctrl0, "pinctrl-0", NULL) > +DEFINE_SIMPLE_PROP(pinctrl1, "pinctrl-1", NULL) > +DEFINE_SIMPLE_PROP(pinctrl2, "pinctrl-2", NULL) > +DEFINE_SIMPLE_PROP(pinctrl3, "pinctrl-3", NULL) > +DEFINE_SIMPLE_PROP(pinctrl4, "pinctrl-4", NULL) > +DEFINE_SIMPLE_PROP(pinctrl5, "pinctrl-5", NULL) > +DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL) > +DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL) > +DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL) > DEFINE_SUFFIX_PROP(regulators, "-supply", NULL) > DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells") > DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells") > @@ -1303,6 +1312,15 @@ static const struct supplier_bindings of_supplier_bindings[] = { > { .parse_prop = parse_nvmem_cells, }, > { .parse_prop = parse_phys, }, > { .parse_prop = parse_wakeup_parent, }, > + { .parse_prop = parse_pinctrl0, }, > + { .parse_prop = parse_pinctrl1, }, > + { .parse_prop = parse_pinctrl2, }, > + { .parse_prop = parse_pinctrl3, }, > + { .parse_prop = parse_pinctrl4, }, > + { .parse_prop = parse_pinctrl5, }, > + { .parse_prop = parse_pinctrl6, }, > + { .parse_prop = parse_pinctrl7, }, > + { .parse_prop = parse_pinctrl8, }, > { .parse_prop = parse_regulators, }, > { .parse_prop = parse_gpio, }, > { .parse_prop = parse_gpios, }, >
On Wed, Jul 22, 2020 at 2:13 PM Saravana Kannan <saravanak@google.com> wrote: > > Add support for pinctrl-0 through pinctrl-8 explicitly instead of trying > to add support for pinctrl-%d properties. > > Of all the pinctrl-* properties in dts files (20322), only 47% (9531) > are pinctrl-%d properties. Of all the pinctrl-%d properties, 99.5% > (9486) are made up of pinctrl-[0-2]. > > Trying to parse all pinctrl-* properties and checking for pinctrl-%d is > unnecessarily complicated. So, just add support for pinctrl-[0-8] for > now. In the unlikely event we ever exceed pinctrl-8, we can come back > and improve this. It wasn't immediately clear from this that pinctrl-8 is the current max you found vs. a should be enough for a while. Pinctrl is also a bit special in that we have 100s of child nodes and only 1 to a few actual dependencies (the pinctrl node). I assume in the end here, it's just the pin controller node that's the dependency rather than creating lot's of dependencies? Rob
On Wed, Jul 22, 2020 at 1:56 PM Frank Rowand <frowand.list@gmail.com> wrote: > > On 2020-07-22 15:13, Saravana Kannan wrote: > > Add support for pinctrl-0 through pinctrl-8 explicitly instead of trying > > to add support for pinctrl-%d properties. > > > > Of all the pinctrl-* properties in dts files (20322), only 47% (9531) > > are pinctrl-%d properties. Of all the pinctrl-%d properties, 99.5% > > (9486) are made up of pinctrl-[0-2]. > > > > Trying to parse all pinctrl-* properties and checking for pinctrl-%d is > > unnecessarily complicated. So, just add support for pinctrl-[0-8] for > > now. In the unlikely event we ever exceed pinctrl-8, we can come back > > and improve this. > > If you were to implement the more general pinctrl-* case, roughly what would > it look like (pseudo-code or english description is fine). So when I say "unnecessarily complicated", it's in terms of readability. I can't use these macros -- which are succinct 1 line entries that are super easy to understand. Pseudo code: parse_pinctrl(np, prop_name, index) { if (doens't start with "pinctrl-") return NULL; ret = kstrtouint(propname + strlen("pinctrl"), 10, &val); check that it's not something line "pinctrl-2nd-val" that'll still set val to 2. parse phandle with args and return phandle node. } All this when effectively 99.5% of the DT just use pinctrl-0, pinctrl-1 and pinctrl-2. There are a few that use pinctrl-3. And literally 6 DT files in the entire kernel source tree use pinctrl-4 or greater. And for those 6 files, pinctrl-[0-8] really point to the same pinctrl node. So even if I didn't parse pinctrl-[4-8], all the device dependencies would be tracked properly. -Saravana
On Wed, Jul 22, 2020 at 2:09 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Wed, Jul 22, 2020 at 2:13 PM Saravana Kannan <saravanak@google.com> wrote: > > > > Add support for pinctrl-0 through pinctrl-8 explicitly instead of trying > > to add support for pinctrl-%d properties. > > > > Of all the pinctrl-* properties in dts files (20322), only 47% (9531) > > are pinctrl-%d properties. Of all the pinctrl-%d properties, 99.5% > > (9486) are made up of pinctrl-[0-2]. > > > > Trying to parse all pinctrl-* properties and checking for pinctrl-%d is > > unnecessarily complicated. So, just add support for pinctrl-[0-8] for > > now. In the unlikely event we ever exceed pinctrl-8, we can come back > > and improve this. > > It wasn't immediately clear from this that pinctrl-8 is the current > max you found vs. a should be enough for a while. Hmmm... I tried. Looks like I failed. Open to copy-pasting any commit text that you think will make it clearer. > Pinctrl is also a bit special in that we have 100s of child nodes and > only 1 to a few actual dependencies (the pinctrl node). I assume in > the end here, it's just the pin controller node that's the dependency > rather than creating lot's of dependencies? Correct. In the end, it just links to the one (or few) pin controller devices. Is there a requirement that all pinctrl-N properties point to the child state nodes of the same pin-controller node? Or can pinctrl-0 point to one and pinctrl-1 point to another pin controller node? If the former, all I'd need to do is parse pinctrl-0. -Saravana
On Wed, Jul 22, 2020 at 7:08 PM Saravana Kannan <saravanak@google.com> wrote: > > On Wed, Jul 22, 2020 at 2:09 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Wed, Jul 22, 2020 at 2:13 PM Saravana Kannan <saravanak@google.com> wrote: > > > > > > Add support for pinctrl-0 through pinctrl-8 explicitly instead of trying > > > to add support for pinctrl-%d properties. > > > > > > Of all the pinctrl-* properties in dts files (20322), only 47% (9531) > > > are pinctrl-%d properties. Of all the pinctrl-%d properties, 99.5% > > > (9486) are made up of pinctrl-[0-2]. > > > > > > Trying to parse all pinctrl-* properties and checking for pinctrl-%d is > > > unnecessarily complicated. So, just add support for pinctrl-[0-8] for > > > now. In the unlikely event we ever exceed pinctrl-8, we can come back > > > and improve this. > > > > It wasn't immediately clear from this that pinctrl-8 is the current > > max you found vs. a should be enough for a while. > > Hmmm... I tried. Looks like I failed. Open to copy-pasting any commit > text that you think will make it clearer. Append to the 2nd paragraph: 'pinctrl-8' is the current maximum found in dts files. > > Pinctrl is also a bit special in that we have 100s of child nodes and > > only 1 to a few actual dependencies (the pinctrl node). I assume in > > the end here, it's just the pin controller node that's the dependency > > rather than creating lot's of dependencies? > > Correct. In the end, it just links to the one (or few) pin controller > devices. Is there a requirement that all pinctrl-N properties point to > the child state nodes of the same pin-controller node? Or can > pinctrl-0 point to one and pinctrl-1 point to another pin controller > node? If the former, all I'd need to do is parse pinctrl-0. My initial thought was I'd expect the dependencies to be uniform across pinctrl-%d properties as each one is supposed to be a different mode of the same set of pins. However, the dra7 pathologic cases don't follow that exactly with the higher speed MMC modes having an additional i/o delay controller setting. Rob
diff --git a/drivers/of/property.c b/drivers/of/property.c index b06edeb1f88b..d40d923ffeaf 100644 --- a/drivers/of/property.c +++ b/drivers/of/property.c @@ -1274,6 +1274,15 @@ DEFINE_SIMPLE_PROP(interrupts_extended, "interrupts-extended", DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", NULL) DEFINE_SIMPLE_PROP(phys, "phys", "#phy-cells") DEFINE_SIMPLE_PROP(wakeup_parent, "wakeup-parent", NULL) +DEFINE_SIMPLE_PROP(pinctrl0, "pinctrl-0", NULL) +DEFINE_SIMPLE_PROP(pinctrl1, "pinctrl-1", NULL) +DEFINE_SIMPLE_PROP(pinctrl2, "pinctrl-2", NULL) +DEFINE_SIMPLE_PROP(pinctrl3, "pinctrl-3", NULL) +DEFINE_SIMPLE_PROP(pinctrl4, "pinctrl-4", NULL) +DEFINE_SIMPLE_PROP(pinctrl5, "pinctrl-5", NULL) +DEFINE_SIMPLE_PROP(pinctrl6, "pinctrl-6", NULL) +DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL) +DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL) DEFINE_SUFFIX_PROP(regulators, "-supply", NULL) DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells") DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells") @@ -1303,6 +1312,15 @@ static const struct supplier_bindings of_supplier_bindings[] = { { .parse_prop = parse_nvmem_cells, }, { .parse_prop = parse_phys, }, { .parse_prop = parse_wakeup_parent, }, + { .parse_prop = parse_pinctrl0, }, + { .parse_prop = parse_pinctrl1, }, + { .parse_prop = parse_pinctrl2, }, + { .parse_prop = parse_pinctrl3, }, + { .parse_prop = parse_pinctrl4, }, + { .parse_prop = parse_pinctrl5, }, + { .parse_prop = parse_pinctrl6, }, + { .parse_prop = parse_pinctrl7, }, + { .parse_prop = parse_pinctrl8, }, { .parse_prop = parse_regulators, }, { .parse_prop = parse_gpio, }, { .parse_prop = parse_gpios, },
Add support for pinctrl-0 through pinctrl-8 explicitly instead of trying to add support for pinctrl-%d properties. Of all the pinctrl-* properties in dts files (20322), only 47% (9531) are pinctrl-%d properties. Of all the pinctrl-%d properties, 99.5% (9486) are made up of pinctrl-[0-2]. Trying to parse all pinctrl-* properties and checking for pinctrl-%d is unnecessarily complicated. So, just add support for pinctrl-[0-8] for now. In the unlikely event we ever exceed pinctrl-8, we can come back and improve this. Signed-off-by: Saravana Kannan <saravanak@google.com> --- drivers/of/property.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)