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

[Keerthy]

On Tuesday 27 September 2016 11:25 PM, Simon Glass wrote:
> 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
>   ...
> [..]

Simon,

I understand now. Though i would have loved to have this feedback on 
palmas regulator driver as well. Based on which i did this.

Regards,
Keerthy
>
>
> Regards,
> Simon
>