On Wednesday 28 September 2016 08:57 AM, Keerthy wrote:
>
>
> 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.
Reworked and fixed the comments and posted v2 of this series. Hope that
is good to go.
>
> Regards,
> Keerthy
>>
>>
>> Regards,
>> Simon
>>