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