[U-Boot] [PATCH v5 2/6] power: pmic: Palmas: Add the base pmic support

[Keerthy <a0393675@ti.com>]

On Friday 30 September 2016 05:12 AM, Simon Glass wrote:
> Hi Keethy,
>
> On 29 September 2016 at 00:11, Keerthy <a0393675@ti.com> wrote:
>>
>>
>> On Thursday 29 September 2016 11:32 AM, Keerthy wrote:
>>>
>>>
>>>
>>> On Wednesday 28 September 2016 11:21 PM, Simon Glass wrote:
>>>>
>>>> Hi Keerthy,
>>>>
>>>> On 27 September 2016 at 22:27, Keerthy <j-keerthy@ti.com> wrote:
>>>>>
>>>>> Add support to bind the regulators/child nodes with the pmic.
>>>>> Also adds the pmic i2c based read/write funtions to access pmic
>>>>> registers.
>>>>>
>>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>> Reviewed-by: Tom Rini <trini@konsulko.com>
>>>>> ---
>>>>>
>>>>> Changes in v5:
>>>>>
>>>>>   * Added pmic read/write functions.
>>>>>
>>>>>  drivers/power/pmic/Kconfig  |   7 +++
>>>>>  drivers/power/pmic/Makefile |   1 +
>>>>>  drivers/power/pmic/palmas.c | 108
>>>>> ++++++++++++++++++++++++++++++++++++++++++++
>>>>>  include/power/palmas.h      |  25 ++++++++++
>>>>>  4 files changed, 141 insertions(+)
>>>>>  create mode 100644 drivers/power/pmic/palmas.c
>>>>>  create mode 100644 include/power/palmas.h
>>>>>
>>>>> diff --git a/drivers/power/pmic/Kconfig b/drivers/power/pmic/Kconfig
>>>>> index 69f8d51..92931c5 100644
>>>>> --- a/drivers/power/pmic/Kconfig
>>>>> +++ b/drivers/power/pmic/Kconfig
>>>>> @@ -135,3 +135,10 @@ config PMIC_TPS65090
>>>>>         FETs and a battery charger. This driver provides register access
>>>>>         only, and you can enable the regulator/charger drivers
>>>>> separately if
>>>>>         required.
>>>>> +
>>>>> +config PMIC_PALMAS
>>>>> +       bool "Enable driver for Texas Instruments PALMAS PMIC"
>>>>> +       depends on DM_PMIC
>>>>> +       ---help---
>>>>> +       The PALMAS is a PMIC containing several LDOs, SMPS.
>>>>> +       This driver binds the pmic children.
>>>>> diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile
>>>>> index 52b4f71..828c0cf 100644
>>>>> --- a/drivers/power/pmic/Makefile
>>>>> +++ b/drivers/power/pmic/Makefile
>>>>> @@ -15,6 +15,7 @@ obj-$(CONFIG_PMIC_PM8916) += pm8916.o
>>>>>  obj-$(CONFIG_PMIC_RK808) += rk808.o
>>>>>  obj-$(CONFIG_PMIC_TPS65090) += tps65090.o
>>>>>  obj-$(CONFIG_PMIC_S5M8767) += s5m8767.o
>>>>> +obj-$(CONFIG_$(SPL_)PMIC_PALMAS) += palmas.o
>>>>>
>>>>>  obj-$(CONFIG_POWER_LTC3676) += pmic_ltc3676.o
>>>>>  obj-$(CONFIG_POWER_MAX77696) += pmic_max77696.o
>>>>> diff --git a/drivers/power/pmic/palmas.c b/drivers/power/pmic/palmas.c
>>>>> new file mode 100644
>>>>> index 0000000..1d2bd67
>>>>> --- /dev/null
>>>>> +++ b/drivers/power/pmic/palmas.c
>>>>> @@ -0,0 +1,108 @@
>>>>> +/*
>>>>> + * (C) Copyright 2016 Texas Instruments Incorporated, <www.ti.com>
>>>>> + * Keerthy <j-keerthy@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/palmas.h>
>>>>> +#include <dm/device.h>
>>>>> +
>>>>> +DECLARE_GLOBAL_DATA_PTR;
>>>>> +
>>>>> +static const struct pmic_child_info pmic_children_info[] = {
>>>>> +       { .prefix = "ldo", .driver = PALMAS_LDO_DRIVER },
>>>>> +       { .prefix = "smps", .driver = PALMAS_SMPS_DRIVER },
>>>>> +       { },
>>>>> +};
>>>>> +
>>>>> +static int palmas_write(struct udevice *dev, uint reg, const uint8_t
>>>>> *buff,
>>>>> +                         int len)
>>>>> +{
>>>>> +       if (dm_i2c_reg_write(dev, reg, *buff)) {
>>>>
>>>>
>>>> I think this should be dm_i2c_write(). You are only writing a single
>>>> byte.
>>>
>>>
>>> Now the v2 of this series had comments suggesting the opposite:
>>>
>>> https://www.mail-archive.com/u-boot at lists.denx.de/msg225123.html
>>
>>
>> And i thought dm_i2c_reg_write/read was specifically introduced for writing
>> and reading one byte value?
>>
>> int dm_i2c_reg_write(struct udevice *dev, uint offset, uint value)
>> {
>>         uint8_t val = value;
>>
>>         return dm_i2c_write(dev, offset, &val, 1);
>> }
>
> Yes, but your palmas_write() function should be able to write multiple
> bytes. If you look at the function signature and the comments in
> pmic.h you should be able to see that.

Okay.

>
> [...]
>
> Regards,
> Simon
>