[U-Boot] [PATCH] power: pmic/regulator: Add basic support for TPS65910

[Felix Brack]

Hello Simon

On 26.11.2017 12:39, Simon Glass wrote:
> Hi Felix,
> 
> On 22 November 2017 at 03:39, Felix Brack <fb at ltec.ch> wrote:
>> Hello Simon,
>>
>> Many thanks for taking the time to review my patch.
>>
>> On 20.11.2017 16:38, Simon Glass wrote:
>>> Hi Felix,
>>>
>>> On 8 November 2017 at 04:04, Felix Brack <fb at ltec.ch> wrote:
>>>> Texas Instrument's TPS65910 PMIC contains 3 buck DC-DC converts, one
>>>> boost DC-DC converter and 8 LDOs. This patch implements driver model
>>>> support for the TPS65910 PMIC and its regulators making the get/set
>>>> API for regulator value/enable available.
>>>> This patch depends on the patch "am33xx: Add a function to query MPU
>>>> voltage in uV" to build correctly. For boards relying on the DT
>>>> include file tps65910.dtsi the v2 patch "power: extend prefix match
>>>> to regulator-name property" and an appropriate regulator naming is
>>>> also required.
>>>>
>>>> Signed-off-by: Felix Brack <fb at ltec.ch>
>>>> ---
>>>>
>>>>  drivers/power/pmic/Kconfig                   |   8 +
>>>>  drivers/power/pmic/Makefile                  |   1 +
>>>>  drivers/power/pmic/pmic_tps65910_dm.c        | 138 ++++++++
>>>>  drivers/power/regulator/Kconfig              |   7 +
>>>>  drivers/power/regulator/Makefile             |   1 +
>>>>  drivers/power/regulator/tps65910_regulator.c | 493 +++++++++++++++++++++++++++
>>>>  include/power/tps65910_pmic.h                | 130 +++++++
>>>>  7 files changed, 778 insertions(+)
>>>>  create mode 100644 drivers/power/pmic/pmic_tps65910_dm.c
>>>>  create mode 100644 drivers/power/regulator/tps65910_regulator.c
>>>>  create mode 100644 include/power/tps65910_pmic.h
>>>>
> 
> [..]
> 
>>>> +static int pmic_tps65910_write(struct udevice *dev, uint reg, const u8 *buffer,
>>>> +                              int len)
>>>> +{
>>>> +       if (dm_i2c_write(dev, reg, buffer, len)) {
>>>> +               error("%s write error on register %02x\n", dev->name, reg);
>>>> +               return -EIO;
>>>
>>> Can you return ret here instead (and in cases below)? I does not seem
>>> necessary to obscure the original error.
>>>
>>> [...]
>>>
>> Yes for the calls to dm_i2c_write() and dm_i2c_read(). How about the
>> call to ofnode_valid()? Is it okay to return -ENXIO instead of NULL in
>> case of failure?
> 
> Yes, all I was referring to was that we should not change the error
> number when passing it through, unless there is a valid reason.
> 
> For ofnode_valid() you have an invalid device tree node I think, so
> -EINVAL is better.
> 
> [..]
>
queued for V2

>>>> +static int get_ctrl_reg_from_unit_addr(const int unit_addr)
>>>> +{
>>>> +       switch (unit_addr) {
>>>> +       case TPS65910_UNIT_VRTC:
>>>> +               return TPS65910_REG_VRTC;
>>>> +       case TPS65910_UNIT_VIO:
>>>> +               return TPS65910_REG_VIO;
>>>> +       case TPS65910_UNIT_VDD1:
>>>> +               return TPS65910_REG_VDD1;
>>>> +       case TPS65910_UNIT_VDD2:
>>>> +               return TPS65910_REG_VDD2;
>>>> +       case TPS65910_UNIT_VDD3:
>>>> +               return TPS65910_REG_VDD3;
>>>> +       case TPS65910_UNIT_VDIG1:
>>>> +               return TPS65910_REG_VDIG1;
>>>> +       case TPS65910_UNIT_VDIG2:
>>>> +               return TPS65910_REG_VDIG2;
>>>> +       case TPS65910_UNIT_VPLL:
>>>> +               return TPS65910_REG_VPLL;
>>>> +       case TPS65910_UNIT_VDAC:
>>>> +               return TPS65910_REG_VDAC;
>>>> +       case TPS65910_UNIT_VAUX1:
>>>> +               return TPS65910_REG_VAUX1;
>>>> +       case TPS65910_UNIT_VAUX2:
>>>> +               return TPS65910_REG_VAUX2;
>>>> +       case TPS65910_UNIT_VAUX33:
>>>> +               return TPS65910_REG_VAUX33;
>>>> +       case TPS65910_UNIT_VMMC:
>>>> +               return TPS65910_REG_VMMC;
>>>
>>> I'm guess this cannot be done with an array lookup?
>>>
>> It could be done, as the unit numbers are consecutive (for now) and
>> starting at 0. I don't like that piece of code either. Should I replace
>> it with a static array of constants? I'm just not sure ...
> 
> I think it is better to use const array in this case.
> 
> [...]
> 
queued for V2

> Regards,
> Simon
> 

regards, Felix