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

[Simon Glass]

Hi Keethy,

On 29 September 2016 at 00:11, Keerthy <a0393675 at 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 at 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 at ti.com>
>>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>>> Reviewed-by: Tom Rini <trini at 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 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/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@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.

[...]

Regards,
Simon