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 <f...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
