| Message ID | 1508319595-20536-1-git-send-email-fb@ltec.ch |
|---|---|
| State | Changes Requested |
| Delegated to: | Lukasz Majewski |
| Headers | show |
| Series | [U-Boot,v2] power: extend prefix match to regulator-name property | expand |
On 10/18/2017 06:39 PM, Felix Brack wrote: > This patch extends pmic_bind_children prefix matching. In addition to > the node name the property regulator-name is used while trying to match > prefixes. This allows assigning different drivers to regulator nodes > named regulator@1 and regulator@10 for example. > I have discarded the idea of using other properties then regulator-name > as I do not see any benefit in using property compatible or even > regulator-compatible. Of course I am open to change this if there are > good reasons to do so. > > Signed-off-by: Felix Brack <fb@ltec.ch> > --- > > Changes in v2: > - add documentation > - add a regulator to the sandbox for testing > - extend the test for the new sandbox regulator > > arch/sandbox/dts/sandbox_pmic.dtsi | 6 ++++++ > doc/device-tree-bindings/regulator/regulator.txt | 16 ++++++++++++++-- > drivers/power/pmic/pmic-uclass.c | 15 +++++++++++++-- > include/power/sandbox_pmic.h | 5 ++++- > test/dm/regulator.c | 2 ++ > 5 files changed, 39 insertions(+), 5 deletions(-) > > diff --git a/arch/sandbox/dts/sandbox_pmic.dtsi b/arch/sandbox/dts/sandbox_pmic.dtsi > index ce261b9..acb4799 100644 > --- a/arch/sandbox/dts/sandbox_pmic.dtsi > +++ b/arch/sandbox/dts/sandbox_pmic.dtsi > @@ -75,4 +75,10 @@ > regulator-min-microvolt = <3300000>; > regulator-max-microvolt = <3300000>; > }; > + > + no_match_by_nodename { > + regulator-name = "buck_SUPPLY_1.5V"; > + regulator-min-microvolt = <1500000>; > + regulator-max-microvolt = <1500000>; > + }; > }; > diff --git a/doc/device-tree-bindings/regulator/regulator.txt b/doc/device-tree-bindings/regulator/regulator.txt > index 918711e..65b69c4 100644 > --- a/doc/device-tree-bindings/regulator/regulator.txt > +++ b/doc/device-tree-bindings/regulator/regulator.txt > @@ -2,7 +2,8 @@ Voltage/Current regulator > > Binding: > The regulator devices don't use the "compatible" property. The binding is done > -by the prefix of regulator node's name. Usually the pmic I/O driver will provide > +by the prefix of regulator node's name, or, if this fails, by the prefix of the > +regulator's "regulator-name" property. Usually the pmic I/O driver will provide > the array of 'struct pmic_child_info' with the prefixes and compatible drivers. > The bind is done by calling function: pmic_bind_childs(). > Example drivers: > @@ -15,8 +16,19 @@ For the node name e.g.: "prefix[:alpha:]num { ... }": > > Example the prefix "ldo" will pass for: "ldo1", "ldo@1", "ldoreg@1, ... > > +Binding by means of the node's name is preferred. However if the node names > +would produce ambiguous prefixes (like "regulator@1" and "regualtor@11") and you > +can't or do not want to change them then binding against the "regulator-name" > +property is possible. The syntax for the prefix of the "regulator-name" property > +is the same as the one for the regulator's node name. > +Use case: a regulator named "regulator@1" to be bound to a driver named > +"LDO_DRV" and a regulator named "regualator@11" to be bound to an other driver > +named "BOOST_DRV". Using prefix "regualtor@1" for driver matching would load > +the same driver for both regulators, hence the prefix is ambiguous. > + > Optional properties: > -- regulator-name: a string, required by the regulator uclass > +- regulator-name: a string, required by the regulator uclass, used for driver > + binding if binding by node's name prefix fails > - regulator-min-microvolt: a minimum allowed Voltage value > - regulator-max-microvolt: a maximum allowed Voltage value > - regulator-min-microamp: a minimum allowed Current value > diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c > index 64964e4..5a034f0 100644 > --- a/drivers/power/pmic/pmic-uclass.c > +++ b/drivers/power/pmic/pmic-uclass.c > @@ -26,6 +26,7 @@ int pmic_bind_children(struct udevice *pmic, ofnode parent, > struct driver *drv; > struct udevice *child; > const char *node_name; > + const char *reg_name; > int bind_count = 0; > ofnode node; > int prefix_len; > @@ -44,8 +45,18 @@ int pmic_bind_children(struct udevice *pmic, ofnode parent, > debug(" - compatible prefix: '%s'\n", info->prefix); > > prefix_len = strlen(info->prefix); > - if (strncmp(info->prefix, node_name, prefix_len)) > - continue; > + if (strncmp(info->prefix, node_name, prefix_len)) { > + reg_name = ofnode_read_string(node, > + "regulator-name"); > + if (reg_name) { > + if (strncmp(info->prefix, reg_name, > + prefix_len)) { > + continue; > + } > + } else { > + continue; > + }The below code is more readable? if (!reg_name) continue; if (strncmp()) continue; ... how about? > + } > > drv = lists_driver_lookup_name(info->driver); > if (!drv) { > diff --git a/include/power/sandbox_pmic.h b/include/power/sandbox_pmic.h > index 7fdbfb9..da2c296 100644 > --- a/include/power/sandbox_pmic.h > +++ b/include/power/sandbox_pmic.h > @@ -13,7 +13,7 @@ > #define SANDBOX_BUCK_DRIVER "sandbox_buck" > #define SANDBOX_OF_BUCK_PREFIX "buck" > > -#define SANDBOX_BUCK_COUNT 2 > +#define SANDBOX_BUCK_COUNT 3 > #define SANDBOX_LDO_COUNT 2 > /* > * Sandbox PMIC registers: > @@ -114,6 +114,9 @@ enum { > #define SANDBOX_LDO1_PLATNAME "VDD_EMMC_1.8V" > #define SANDBOX_LDO2_DEVNAME "ldo2" > #define SANDBOX_LDO2_PLATNAME "VDD_LCD_3.3V" > +/* BUCK3: for testing fallback regulator prefix matching during bind */ > +#define SANDBOX_BUCK3_DEVNAME "no_match_by_nodename" > +#define SANDBOX_BUCK3_PLATNAME "buck_SUPPLY_1.5V" These defined might be located on above point. (SANDBOX_BUCK2_xxxx) Best Regards, Jaehoon Chung > > /* > * Expected regulators setup after call of: > diff --git a/test/dm/regulator.c b/test/dm/regulator.c > index 3d0056f..395381d 100644 > --- a/test/dm/regulator.c > +++ b/test/dm/regulator.c > @@ -27,6 +27,7 @@ DECLARE_GLOBAL_DATA_PTR; > enum { > BUCK1, > BUCK2, > + BUCK3, > LDO1, > LDO2, > OUTPUT_COUNT, > @@ -42,6 +43,7 @@ static const char *regulator_names[OUTPUT_COUNT][OUTPUT_NAME_COUNT] = { > /* devname, platname */ > { SANDBOX_BUCK1_DEVNAME, SANDBOX_BUCK1_PLATNAME }, > { SANDBOX_BUCK2_DEVNAME, SANDBOX_BUCK2_PLATNAME }, > + { SANDBOX_BUCK3_DEVNAME, SANDBOX_BUCK3_PLATNAME }, > { SANDBOX_LDO1_DEVNAME, SANDBOX_LDO1_PLATNAME}, > { SANDBOX_LDO2_DEVNAME, SANDBOX_LDO2_PLATNAME}, > }; >
On 18 October 2017 at 11:39, Felix Brack <fb@ltec.ch> wrote: > This patch extends pmic_bind_children prefix matching. In addition to > the node name the property regulator-name is used while trying to match > prefixes. This allows assigning different drivers to regulator nodes > named regulator@1 and regulator@10 for example. > I have discarded the idea of using other properties then regulator-name > as I do not see any benefit in using property compatible or even > regulator-compatible. Of course I am open to change this if there are > good reasons to do so. > > Signed-off-by: Felix Brack <fb@ltec.ch> > --- > > Changes in v2: > - add documentation > - add a regulator to the sandbox for testing > - extend the test for the new sandbox regulator > > arch/sandbox/dts/sandbox_pmic.dtsi | 6 ++++++ > doc/device-tree-bindings/regulator/regulator.txt | 16 ++++++++++++++-- > drivers/power/pmic/pmic-uclass.c | 15 +++++++++++++-- > include/power/sandbox_pmic.h | 5 ++++- > test/dm/regulator.c | 2 ++ > 5 files changed, 39 insertions(+), 5 deletions(-) Reviewed-by: Simon Glass <sjg@chromium.org>
On 20.10.2017 15:34, Jaehoon Chung wrote: > On 10/18/2017 06:39 PM, Felix Brack wrote: >> This patch extends pmic_bind_children prefix matching. In addition to >> the node name the property regulator-name is used while trying to match >> prefixes. This allows assigning different drivers to regulator nodes >> named regulator@1 and regulator@10 for example. >> I have discarded the idea of using other properties then regulator-name >> as I do not see any benefit in using property compatible or even >> regulator-compatible. Of course I am open to change this if there are >> good reasons to do so. >> >> Signed-off-by: Felix Brack <fb@ltec.ch> >> --- >> >> Changes in v2: >> - add documentation >> - add a regulator to the sandbox for testing >> - extend the test for the new sandbox regulator >> >> arch/sandbox/dts/sandbox_pmic.dtsi | 6 ++++++ >> doc/device-tree-bindings/regulator/regulator.txt | 16 ++++++++++++++-- >> drivers/power/pmic/pmic-uclass.c | 15 +++++++++++++-- >> include/power/sandbox_pmic.h | 5 ++++- >> test/dm/regulator.c | 2 ++ >> 5 files changed, 39 insertions(+), 5 deletions(-) >> >> diff --git a/arch/sandbox/dts/sandbox_pmic.dtsi b/arch/sandbox/dts/sandbox_pmic.dtsi >> index ce261b9..acb4799 100644 >> --- a/arch/sandbox/dts/sandbox_pmic.dtsi >> +++ b/arch/sandbox/dts/sandbox_pmic.dtsi >> @@ -75,4 +75,10 @@ >> regulator-min-microvolt = <3300000>; >> regulator-max-microvolt = <3300000>; >> }; >> + >> + no_match_by_nodename { >> + regulator-name = "buck_SUPPLY_1.5V"; >> + regulator-min-microvolt = <1500000>; >> + regulator-max-microvolt = <1500000>; >> + }; >> }; >> diff --git a/doc/device-tree-bindings/regulator/regulator.txt b/doc/device-tree-bindings/regulator/regulator.txt >> index 918711e..65b69c4 100644 >> --- a/doc/device-tree-bindings/regulator/regulator.txt >> +++ b/doc/device-tree-bindings/regulator/regulator.txt >> @@ -2,7 +2,8 @@ Voltage/Current regulator >> >> Binding: >> The regulator devices don't use the "compatible" property. The binding is done >> -by the prefix of regulator node's name. Usually the pmic I/O driver will provide >> +by the prefix of regulator node's name, or, if this fails, by the prefix of the >> +regulator's "regulator-name" property. Usually the pmic I/O driver will provide >> the array of 'struct pmic_child_info' with the prefixes and compatible drivers. >> The bind is done by calling function: pmic_bind_childs(). >> Example drivers: >> @@ -15,8 +16,19 @@ For the node name e.g.: "prefix[:alpha:]num { ... }": >> >> Example the prefix "ldo" will pass for: "ldo1", "ldo@1", "ldoreg@1, ... >> >> +Binding by means of the node's name is preferred. However if the node names >> +would produce ambiguous prefixes (like "regulator@1" and "regualtor@11") and you >> +can't or do not want to change them then binding against the "regulator-name" >> +property is possible. The syntax for the prefix of the "regulator-name" property >> +is the same as the one for the regulator's node name. >> +Use case: a regulator named "regulator@1" to be bound to a driver named >> +"LDO_DRV" and a regulator named "regualator@11" to be bound to an other driver >> +named "BOOST_DRV". Using prefix "regualtor@1" for driver matching would load >> +the same driver for both regulators, hence the prefix is ambiguous. >> + >> Optional properties: >> -- regulator-name: a string, required by the regulator uclass >> +- regulator-name: a string, required by the regulator uclass, used for driver >> + binding if binding by node's name prefix fails >> - regulator-min-microvolt: a minimum allowed Voltage value >> - regulator-max-microvolt: a maximum allowed Voltage value >> - regulator-min-microamp: a minimum allowed Current value >> diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c >> index 64964e4..5a034f0 100644 >> --- a/drivers/power/pmic/pmic-uclass.c >> +++ b/drivers/power/pmic/pmic-uclass.c >> @@ -26,6 +26,7 @@ int pmic_bind_children(struct udevice *pmic, ofnode parent, >> struct driver *drv; >> struct udevice *child; >> const char *node_name; >> + const char *reg_name; >> int bind_count = 0; >> ofnode node; >> int prefix_len; >> @@ -44,8 +45,18 @@ int pmic_bind_children(struct udevice *pmic, ofnode parent, >> debug(" - compatible prefix: '%s'\n", info->prefix); >> >> prefix_len = strlen(info->prefix); >> - if (strncmp(info->prefix, node_name, prefix_len)) >> - continue; >> + if (strncmp(info->prefix, node_name, prefix_len)) { >> + reg_name = ofnode_read_string(node, >> + "regulator-name"); >> + if (reg_name) { >> + if (strncmp(info->prefix, reg_name, >> + prefix_len)) { >> + continue; >> + } >> + } else { >> + continue; >> + }The below code is more readable? > > if (!reg_name) > continue; > > if (strncmp()) > continue; > > ... > > how about? > Agreed. Looks much better and is simpler to read. If somebody insists on this modification that's fine with me and I will present a v3 patch. > >> + } >> >> drv = lists_driver_lookup_name(info->driver); >> if (!drv) { >> diff --git a/include/power/sandbox_pmic.h b/include/power/sandbox_pmic.h >> index 7fdbfb9..da2c296 100644 >> --- a/include/power/sandbox_pmic.h >> +++ b/include/power/sandbox_pmic.h >> @@ -13,7 +13,7 @@ >> #define SANDBOX_BUCK_DRIVER "sandbox_buck" >> #define SANDBOX_OF_BUCK_PREFIX "buck" >> >> -#define SANDBOX_BUCK_COUNT 2 >> +#define SANDBOX_BUCK_COUNT 3 >> #define SANDBOX_LDO_COUNT 2 >> /* >> * Sandbox PMIC registers: >> @@ -114,6 +114,9 @@ enum { >> #define SANDBOX_LDO1_PLATNAME "VDD_EMMC_1.8V" >> #define SANDBOX_LDO2_DEVNAME "ldo2" >> #define SANDBOX_LDO2_PLATNAME "VDD_LCD_3.3V" >> +/* BUCK3: for testing fallback regulator prefix matching during bind */ >> +#define SANDBOX_BUCK3_DEVNAME "no_match_by_nodename" >> +#define SANDBOX_BUCK3_PLATNAME "buck_SUPPLY_1.5V" > > These defined might be located on above point. (SANDBOX_BUCK2_xxxx) > Yes of course. But the idea of putting them here is to emphasize the special name due to binding against regulator-name property, kind of separating this BUCK regulator from 'normal' BUCK regulators. > Best Regards, > Jaehoon Chung > >> >> /* >> * Expected regulators setup after call of: >> diff --git a/test/dm/regulator.c b/test/dm/regulator.c >> index 3d0056f..395381d 100644 >> --- a/test/dm/regulator.c >> +++ b/test/dm/regulator.c >> @@ -27,6 +27,7 @@ DECLARE_GLOBAL_DATA_PTR; >> enum { >> BUCK1, >> BUCK2, >> + BUCK3, >> LDO1, >> LDO2, >> OUTPUT_COUNT, >> @@ -42,6 +43,7 @@ static const char *regulator_names[OUTPUT_COUNT][OUTPUT_NAME_COUNT] = { >> /* devname, platname */ >> { SANDBOX_BUCK1_DEVNAME, SANDBOX_BUCK1_PLATNAME }, >> { SANDBOX_BUCK2_DEVNAME, SANDBOX_BUCK2_PLATNAME }, >> + { SANDBOX_BUCK3_DEVNAME, SANDBOX_BUCK3_PLATNAME }, >> { SANDBOX_LDO1_DEVNAME, SANDBOX_LDO1_PLATNAME}, >> { SANDBOX_LDO2_DEVNAME, SANDBOX_LDO2_PLATNAME}, >> }; >> > regards, Felix
diff --git a/arch/sandbox/dts/sandbox_pmic.dtsi b/arch/sandbox/dts/sandbox_pmic.dtsi index ce261b9..acb4799 100644 --- a/arch/sandbox/dts/sandbox_pmic.dtsi +++ b/arch/sandbox/dts/sandbox_pmic.dtsi @@ -75,4 +75,10 @@ regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; }; + + no_match_by_nodename { + regulator-name = "buck_SUPPLY_1.5V"; + regulator-min-microvolt = <1500000>; + regulator-max-microvolt = <1500000>; + }; }; diff --git a/doc/device-tree-bindings/regulator/regulator.txt b/doc/device-tree-bindings/regulator/regulator.txt index 918711e..65b69c4 100644 --- a/doc/device-tree-bindings/regulator/regulator.txt +++ b/doc/device-tree-bindings/regulator/regulator.txt @@ -2,7 +2,8 @@ Voltage/Current regulator Binding: The regulator devices don't use the "compatible" property. The binding is done -by the prefix of regulator node's name. Usually the pmic I/O driver will provide +by the prefix of regulator node's name, or, if this fails, by the prefix of the +regulator's "regulator-name" property. Usually the pmic I/O driver will provide the array of 'struct pmic_child_info' with the prefixes and compatible drivers. The bind is done by calling function: pmic_bind_childs(). Example drivers: @@ -15,8 +16,19 @@ For the node name e.g.: "prefix[:alpha:]num { ... }": Example the prefix "ldo" will pass for: "ldo1", "ldo@1", "ldoreg@1, ... +Binding by means of the node's name is preferred. However if the node names +would produce ambiguous prefixes (like "regulator@1" and "regualtor@11") and you +can't or do not want to change them then binding against the "regulator-name" +property is possible. The syntax for the prefix of the "regulator-name" property +is the same as the one for the regulator's node name. +Use case: a regulator named "regulator@1" to be bound to a driver named +"LDO_DRV" and a regulator named "regualator@11" to be bound to an other driver +named "BOOST_DRV". Using prefix "regualtor@1" for driver matching would load +the same driver for both regulators, hence the prefix is ambiguous. + Optional properties: -- regulator-name: a string, required by the regulator uclass +- regulator-name: a string, required by the regulator uclass, used for driver + binding if binding by node's name prefix fails - regulator-min-microvolt: a minimum allowed Voltage value - regulator-max-microvolt: a maximum allowed Voltage value - regulator-min-microamp: a minimum allowed Current value diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c index 64964e4..5a034f0 100644 --- a/drivers/power/pmic/pmic-uclass.c +++ b/drivers/power/pmic/pmic-uclass.c @@ -26,6 +26,7 @@ int pmic_bind_children(struct udevice *pmic, ofnode parent, struct driver *drv; struct udevice *child; const char *node_name; + const char *reg_name; int bind_count = 0; ofnode node; int prefix_len; @@ -44,8 +45,18 @@ int pmic_bind_children(struct udevice *pmic, ofnode parent, debug(" - compatible prefix: '%s'\n", info->prefix); prefix_len = strlen(info->prefix); - if (strncmp(info->prefix, node_name, prefix_len)) - continue; + if (strncmp(info->prefix, node_name, prefix_len)) { + reg_name = ofnode_read_string(node, + "regulator-name"); + if (reg_name) { + if (strncmp(info->prefix, reg_name, + prefix_len)) { + continue; + } + } else { + continue; + } + } drv = lists_driver_lookup_name(info->driver); if (!drv) { diff --git a/include/power/sandbox_pmic.h b/include/power/sandbox_pmic.h index 7fdbfb9..da2c296 100644 --- a/include/power/sandbox_pmic.h +++ b/include/power/sandbox_pmic.h @@ -13,7 +13,7 @@ #define SANDBOX_BUCK_DRIVER "sandbox_buck" #define SANDBOX_OF_BUCK_PREFIX "buck" -#define SANDBOX_BUCK_COUNT 2 +#define SANDBOX_BUCK_COUNT 3 #define SANDBOX_LDO_COUNT 2 /* * Sandbox PMIC registers: @@ -114,6 +114,9 @@ enum { #define SANDBOX_LDO1_PLATNAME "VDD_EMMC_1.8V" #define SANDBOX_LDO2_DEVNAME "ldo2" #define SANDBOX_LDO2_PLATNAME "VDD_LCD_3.3V" +/* BUCK3: for testing fallback regulator prefix matching during bind */ +#define SANDBOX_BUCK3_DEVNAME "no_match_by_nodename" +#define SANDBOX_BUCK3_PLATNAME "buck_SUPPLY_1.5V" /* * Expected regulators setup after call of: diff --git a/test/dm/regulator.c b/test/dm/regulator.c index 3d0056f..395381d 100644 --- a/test/dm/regulator.c +++ b/test/dm/regulator.c @@ -27,6 +27,7 @@ DECLARE_GLOBAL_DATA_PTR; enum { BUCK1, BUCK2, + BUCK3, LDO1, LDO2, OUTPUT_COUNT, @@ -42,6 +43,7 @@ static const char *regulator_names[OUTPUT_COUNT][OUTPUT_NAME_COUNT] = { /* devname, platname */ { SANDBOX_BUCK1_DEVNAME, SANDBOX_BUCK1_PLATNAME }, { SANDBOX_BUCK2_DEVNAME, SANDBOX_BUCK2_PLATNAME }, + { SANDBOX_BUCK3_DEVNAME, SANDBOX_BUCK3_PLATNAME }, { SANDBOX_LDO1_DEVNAME, SANDBOX_LDO1_PLATNAME}, { SANDBOX_LDO2_DEVNAME, SANDBOX_LDO2_PLATNAME}, };
This patch extends pmic_bind_children prefix matching. In addition to the node name the property regulator-name is used while trying to match prefixes. This allows assigning different drivers to regulator nodes named regulator@1 and regulator@10 for example. I have discarded the idea of using other properties then regulator-name as I do not see any benefit in using property compatible or even regulator-compatible. Of course I am open to change this if there are good reasons to do so. Signed-off-by: Felix Brack <fb@ltec.ch> --- Changes in v2: - add documentation - add a regulator to the sandbox for testing - extend the test for the new sandbox regulator arch/sandbox/dts/sandbox_pmic.dtsi | 6 ++++++ doc/device-tree-bindings/regulator/regulator.txt | 16 ++++++++++++++-- drivers/power/pmic/pmic-uclass.c | 15 +++++++++++++-- include/power/sandbox_pmic.h | 5 ++++- test/dm/regulator.c | 2 ++ 5 files changed, 39 insertions(+), 5 deletions(-)