Hello Simon On 26.11.2017 12:39, Simon Glass wrote: > Hi Felix, > > On 22 November 2017 at 03:39, Felix Brack <f...@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 <f...@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 <f...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot