[U-Boot] [PATCH 2/2] power: regulator: lp873x: Add regulator support

[Simon Glass]

Hi Keerthy,

On 26 September 2016 at 21:45, Keerthy <a0393675 at ti.com> wrote:
>
>
> On Tuesday 27 September 2016 06:04 AM, Simon Glass wrote:
>>
>> Hi,
>>
>> On 26 September 2016 at 00:00, Keerthy <j-keerthy at ti.com> wrote:
>>>
>>> The driver provides regulator set/get voltage
>>> enable/disable functions for lp873x family of PMICs.
>>>
>>> Signed-off-by: Keerthy <j-keerthy at ti.com>
>>> ---
>>>  drivers/power/regulator/Kconfig            |   8 +
>>>  drivers/power/regulator/Makefile           |   1 +
>>>  drivers/power/regulator/lp873x_regulator.c | 361
>>> +++++++++++++++++++++++++++++
>>>  3 files changed, 370 insertions(+)
>>>  create mode 100644 drivers/power/regulator/lp873x_regulator.c
>>>
>>> diff --git a/drivers/power/regulator/Kconfig
>>> b/drivers/power/regulator/Kconfig
>>> index adb710a..84cf914 100644
>>> --- a/drivers/power/regulator/Kconfig
>>> +++ b/drivers/power/regulator/Kconfig
>>> @@ -123,3 +123,11 @@ config DM_REGULATOR_PALMAS
>>>         This enables implementation of driver-model regulator uclass
>>>         features for REGULATOR PALMAS and the family of PALMAS PMICs.
>>>         The driver implements get/set api for: value and enable.
>>> +
>>> +config DM_REGULATOR_LP873X
>>> +       bool "Enable driver for LP873X PMIC regulators"
>>> +        depends on PMIC_LP873X
>>> +       ---help---
>>> +       This enables implementation of driver-model regulator uclass
>>> +       features for REGULATOR LP873X and the family of LP873X PMICs.
>>> +       The driver implements get/set api for: value and enable.
>>> diff --git a/drivers/power/regulator/Makefile
>>> b/drivers/power/regulator/Makefile
>>> index 75080d4..2093048 100644
>>> --- a/drivers/power/regulator/Makefile
>>> +++ b/drivers/power/regulator/Makefile
>>> @@ -15,3 +15,4 @@ obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
>>>  obj-$(CONFIG_DM_REGULATOR_SANDBOX) += sandbox.o
>>>  obj-$(CONFIG_REGULATOR_TPS65090) += tps65090_regulator.o
>>>  obj-$(CONFIG_$(SPL_)DM_REGULATOR_PALMAS) += palmas_regulator.o
>>> +obj-$(CONFIG_$(SPL_)DM_REGULATOR_LP873X) += lp873x_regulator.o
>>> diff --git a/drivers/power/regulator/lp873x_regulator.c
>>> b/drivers/power/regulator/lp873x_regulator.c
>>> new file mode 100644
>>> index 0000000..7675173
>>> --- /dev/null
>>> +++ b/drivers/power/regulator/lp873x_regulator.c
>>> @@ -0,0 +1,361 @@
>>> +/*
>>> + * (C) Copyright 2016
>>> + * Texas Instruments Incorporated, <www.ti.com>
>>> + *
>>> + * Keerthy <j-keerthy at ti.com>
>>> + *
>>> + * SPDX-License-Identifier:    GPL-2.0+
>>> + */
>>> +
>>> +#include <common.h>
>>> +#include <fdtdec.h>
>>> +#include <errno.h>
>>> +#include <dm.h>
>>> +#include <i2c.h>
>>> +#include <power/pmic.h>
>>> +#include <power/regulator.h>
>>> +#include <power/lp873x.h>
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +static const char lp873x_buck_ctrl[LP873X_BUCK_NUM] = {0x2, 0x4};
>>> +static const char lp873x_buck_volt[LP873X_BUCK_NUM] = {0x6, 0x7};
>>> +static const char lp873x_ldo_ctrl[LP873X_LDO_NUM] = {0x8, 0x9};
>>> +static const char lp873x_ldo_volt[LP873X_LDO_NUM] = {0xA, 0xB};
>>> +
>>> +static int lp873x_buck_enable(struct udevice *dev, int op, bool *enable)
>>> +{
>>> +       int ret;
>>> +       uint8_t val;
>>> +       unsigned int adr;
>>> +       struct dm_regulator_uclass_platdata *uc_pdata;
>>> +
>>> +       uc_pdata = dev_get_uclass_platdata(dev);
>>> +       adr = uc_pdata->ctrl_reg;
>>> +
>>> +       ret = dm_i2c_u8_read(dev->parent, adr, &val);
>>> +       if (ret < 0)
>>> +               return ret;
>>
>>
>> You can use
>>
>> val = dm_i2c_reg_reg(dev->parent, adr);
>
>
> I guess you meant dm_i2c_reg_read.
>
>> if (val < 0)
>>    return val;
>>
>> What is the benefit of using extra u8 variable?
>
>
> Just keeping the consistency with write and read. ret stores the return
> value and any case it is not added just for read so not a redundancy.
>
> val is u8 and that is used with write as well hence i used the above.
>
>>
>> But in this case, you should use pmic_reg_read(). We should avoid i2c
>> access in drivers. There is no need for it, and the PMIC may come in a
>> SPI version one day.
>>
>
> I checked the data manual entirely. No SPI support in the case of lp873x.
>
> So I2C is the way to access.
>
> Hence instead of multiple indirections:
>
> pmic_reg_read--> pmic_read --> ops->read --> foo_read --> i2c_read
>
> I have used the i2c read as it is the only means in case of this PMIC>
> I am afraid i am yet to find a public document for this but i confirm there
> is no SPI involved and I2C is the way.
>
> Hope this justifies why i used i2c directly here.

Not really, sorry. Regulators which are part of a pmic should use the
pmic functions for access. It doesn't make sense to directly access
i2c. Are you trying to make things more efficient? Compared to the CPU
time, the I2C bus will take approximately forever to process this
transaction. It is better to keep the code consistent with other PMIC
drivers, and respect the software boundaries.

PMIC
  -> Regulator
  -> Battery
  -> GPIO
  ...
[..]


Regards,
Simon