Re: [PATCH v1 2/2] of: property: Add device link support for pinctrl-0 through pinctrl-8
On Wed, Jul 22, 2020 at 7:08 PM Saravana Kannan wrote: > > On Wed, Jul 22, 2020 at 2:09 PM Rob Herring wrote: > > > > On Wed, Jul 22, 2020 at 2:13 PM 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. > > > > 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
Re: [PATCH v1 2/2] of: property: Add device link support for pinctrl-0 through pinctrl-8
On Wed, Jul 22, 2020 at 2:09 PM Rob Herring wrote: > > On Wed, Jul 22, 2020 at 2:13 PM 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. > > 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
Re: [PATCH v1 2/2] of: property: Add device link support for pinctrl-0 through pinctrl-8
On Wed, Jul 22, 2020 at 1:56 PM Frank Rowand 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
Re: [PATCH v1 2/2] of: property: Add device link support for pinctrl-0 through pinctrl-8
On Wed, Jul 22, 2020 at 2:13 PM 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. 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
Re: [PATCH v1 2/2] of: property: Add device link support for pinctrl-0 through pinctrl-8
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 > --- > 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, }, >