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

[Simon Glass]

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.

[..]

>>> +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.

[...]

Regards,
Simon