[U-Boot] [PATCH 04/19] dm: pmic: add implementation of driver model pmic uclass

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello Simon,

On 10/10/2014 05:17 AM, Simon Glass wrote:
> Hi,
>
> On 8 October 2014 14:48, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>> This is an introduction to driver-model multi class PMIC support.
>> It starts with UCLASS_PMIC - a common PMIC class type for I/O, which
>> doesn't need to implement any specific operations and features beside
>> the platform data, which is the 'struct pmic_platdata' defined in file:
>> - 'include/power/pmic.h'
>>
>> New files:
>> - pmic-uclass.c - provides a common code for UCLASS_PMIC drivers
>> - pmic_i2c.c    - provides dm interface for i2c pmic drivers
>> - pmic_spi.c    - provides dm interface for spi pmic drivers
>>
>> Those files are based on a current PMIC framework files and code.
>> The new files are introduced to keep code readability and allow to
>> make an easy drivers migration. The old pmic framework is still kept
>> and full independent.
>>
>> Changes:
>> - new uclass-id: UCLASS_PMIC,
>> - new configs: CONFIG_DM_PMIC; CONFIG_DM_PMIC_SPI; CONFIG_DM_PMIC_I2C,
>>
>> New pmic api is documented in: doc/README.power-framework-dm
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> ---
>>   drivers/power/Makefile      |   3 +
>>   drivers/power/pmic-uclass.c | 255 ++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/power/pmic_i2c.c    | 136 +++++++++++++++++++++++
>>   drivers/power/pmic_spi.c    | 137 ++++++++++++++++++++++++
>>   include/dm/uclass-id.h      |   3 +
>>   include/power/pmic.h        |  64 +++++++++++
>>   6 files changed, 598 insertions(+)
>>   create mode 100644 drivers/power/pmic-uclass.c
>>   create mode 100644 drivers/power/pmic_i2c.c
>>   create mode 100644 drivers/power/pmic_spi.c
>>
>> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
>> index dc64e4d..8def501 100644
>> --- a/drivers/power/Makefile
>> +++ b/drivers/power/Makefile
>> @@ -19,3 +19,6 @@ obj-$(CONFIG_DIALOG_POWER) += power_dialog.o
>>   obj-$(CONFIG_POWER_FSL) += power_fsl.o
>>   obj-$(CONFIG_POWER_I2C) += power_i2c.o
>>   obj-$(CONFIG_POWER_SPI) += power_spi.o
>> +obj-$(CONFIG_DM_PMIC_SPI) += pmic_spi.o
>> +obj-$(CONFIG_DM_PMIC_I2C) += pmic_i2c.o
>> +obj-$(CONFIG_DM_PMIC) += pmic-uclass.o
>> diff --git a/drivers/power/pmic-uclass.c b/drivers/power/pmic-uclass.c
>> new file mode 100644
>> index 0000000..5e8494b
>> --- /dev/null
>> +++ b/drivers/power/pmic-uclass.c
>> @@ -0,0 +1,255 @@
>> +/*
>> + * Copyright (C) 2014 Samsung Electronics
>> + * Przemyslaw Marczak <p.marczak@samsung.com>
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +#include <common.h>
>> +#include <linux/types.h>
>> +#include <fdtdec.h>
>> +#include <power/pmic.h>
>> +#include <dm/device-internal.h>
>> +#include <dm/uclass-internal.h>
>> +#include <dm/root.h>
>> +#include <dm/lists.h>
>> +#include <i2c.h>
>> +#include <compiler.h>
>> +#include <errno.h>
>> +
>> +DECLARE_GLOBAL_DATA_PTR;
>> +
>> +int pmic_check_reg(struct pmic_platdata *p, unsigned reg)
>> +{
>> +       if (!p)
>> +               return -ENODEV;
>> +
>> +       if (reg >= p->regs_num) {
>> +               error("<reg num> = %d is invalid. Should be less than %d",
>> +                      reg, p->regs_num);
>> +               return -EINVAL;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +int pmic_reg_write(struct udevice *dev, unsigned reg, unsigned val)
>> +{
>> +       struct pmic_platdata *p;
>> +
>> +       p = dev_get_platdata(dev);
>> +       if (!p)
>> +               return -EPERM;
>> +
>> +       switch (p->interface) {
>> +       case PMIC_I2C:
>> +#ifdef CONFIG_DM_PMIC_I2C
>> +               return pmic_i2c_reg_write(dev, reg, val);
>> +#else
>> +               return -ENOSYS;
>> +#endif
>> +       case PMIC_SPI:
>> +#ifdef CONFIG_DM_PMIC_SPI
>> +               return pmic_spi_reg_write(dev, reg, val);
>> +#else
>> +               return -ENOSYS;
>> +#endif
>
> Perhaps one day we should add another uclass which is some sort of
> register cache. It could be implemented by I2C and SPI drivers (or
> better perhaps the I2C and SPI uclasses could provide a register cache
> uclass device for their main when requested).
>
> For now this seems fine though. I think the 'return -ENOSYS' can just
> go at the end of the function and appear once.
>

Why do we need the registers cache?
This maybe is good for a many read operations at a time in the system,
but actually I think, that it is not required for the bootloader purposes.

If we write some data - sequential, then we probably would like to check 
it in the same time, e.g. update some range of I2C registers.

I don't know the Chromebook design, but how it could be useful for you?

>> +       default:
>> +               return -ENODEV;
>> +       }
>> +}
>> +
>> +int pmic_reg_read(struct udevice *dev, unsigned reg, unsigned *val)
>> +{
>> +       struct pmic_platdata *p;
>> +
>> +       p = dev_get_platdata(dev);
>> +       if (!p)
>> +               return -EPERM;
>> +
>> +       switch (p->interface) {
>> +       case PMIC_I2C:
>> +#ifdef CONFIG_DM_PMIC_I2C
>> +               return pmic_i2c_reg_read(dev, reg, val);
>> +#else
>> +               return -ENOSYS;
>> +#endif
>> +       case PMIC_SPI:
>> +#ifdef CONFIG_DM_PMIC_SPI
>> +               return pmic_spi_reg_read(dev, reg, val);
>> +#else
>> +               return -ENOSYS;
>> +#endif
>> +       default:
>> +               return -ENODEV;
>> +       }
>> +}
>> +
>> +int pmic_probe(struct udevice *dev, struct spi_slave *spi_slave)
>> +{
>> +       struct pmic_platdata *p;
>> +
>> +       p = dev_get_platdata(dev);
>> +       if (!p)
>> +               return -EPERM;
>> +
>> +       switch (p->interface) {
>> +       case PMIC_I2C:
>> +#ifdef CONFIG_DM_PMIC_I2C
>> +               return pmic_i2c_probe(dev);
>> +#else
>> +               return -ENOSYS;
>> +#endif
>> +       case PMIC_SPI:
>> +               if (!spi_slave)
>> +                       return -EINVAL;
>> +#ifdef CONFIG_DM_PMIC_SPI
>> +               spi_slave = pmic_spi_probe(dev);
>> +               if (!spi_slave)
>> +                       return -EIO;
>> +
>> +               return 0;
>> +#else
>> +               return -ENOSYS;
>> +#endif
>> +       default:
>> +               return -ENODEV;
>> +       }
>> +}
>> +
>> +struct udevice *pmic_get_by_interface(int uclass_id, int bus, int addr_cs)
>
> This is an interesting function! Again we can probably improve things
> when we have an i2c uclass.
>
Thanks! But even if we have i2c uclass, then we also should know how to 
recognize each device, e.g. in the board code. So the interface and 
address will probably no change.

>> +{
>> +       struct pmic_platdata *pl;
>> +       struct udevice *dev;
>> +       struct uclass *uc;
>> +       int ret;
>> +
>> +       if (uclass_id < 0 || uclass_id >= UCLASS_COUNT) {
>> +               error("Bad uclass id.\n");
>> +               return NULL;
>> +       }
>> +
>> +       ret = uclass_get(uclass_id, &uc);
>> +       if (ret) {
>> +               error("PMIC uclass: %d not initialized!\n", uclass_id);
>> +               return NULL;
>> +       }
>> +
>> +       uclass_foreach_dev(dev, uc) {
>> +               if (!dev || !dev->platdata)
>> +                       continue;
>> +
>> +               pl = dev_get_platdata(dev);
>> +
>> +               if (pl->bus != bus)
>> +                       continue;
>> +
>> +               switch (pl->interface) {
>> +               case PMIC_I2C:
>> +                       if (addr_cs != pl->hw.i2c.addr)
>> +                               continue;
>> +                       break;
>> +               case PMIC_SPI:
>> +                       if (addr_cs != pl->hw.spi.cs)
>> +                               continue;
>> +                       break;
>> +               default:
>> +                       error("Unsupported interface of: %s", dev->name);
>> +                       return NULL;
>> +               }
>> +
>> +               ret = device_probe(dev);
>> +               if (ret) {
>> +                       error("Dev: %s probe failed", dev->name);
>> +                       return NULL;
>> +               }
>> +               return dev;
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>> +struct udevice *pmic_get_by_name(int uclass_id, char *name)
>> +{
>> +       struct udevice *dev;
>> +       struct uclass *uc;
>> +       int ret;
>> +
>> +       if (uclass_id < 0 || uclass_id >= UCLASS_COUNT) {
>> +               error("Bad uclass id.\n");
>> +               return NULL;
>> +       }
>> +
>> +       ret = uclass_get(uclass_id, &uc);
>> +       if (ret) {
>> +               error("PMIC uclass: %d not initialized!", uclass_id);
>> +               return NULL;
>> +       }
>> +
>> +       uclass_foreach_dev(dev, uc) {
>> +               if (!dev)
>> +                       continue;
>> +
>> +               if (!strncmp(name, dev->name, strlen(name))) {
>> +                       ret = device_probe(dev);
>> +                       if (ret)
>> +                               error("Dev: %s probe failed", dev->name);
>> +                       return dev;
>> +               }
>> +       }
>> +
>> +       return NULL;
>> +}
>> +
>> +int pmic_init_dm(void)
>
> I don't understand why you need this function at all. Driver model
> should find the pmics automatically. Is it because we don't have an
> i2c uclass? In that case I think it would be better to limit this hack
> to I2C. For SPI it should work correctly without this function.
>
Currently, it shouldn't and yes - it isn't because of the I2C uclass 
missing. So no one check the I2C nodes - and its child subnodes.
Actually, this can be easy fixed - just don't add the "pmic" alias to 
the dts. But I will also change fix the code.

And by the way - is there any check in the code, which protects for the 
device bind more than once?

>> +{
>> +       const void *blob = gd->fdt_blob;
>> +       const struct fdt_property *prop;
>> +       struct udevice *dev = NULL;
>> +       const char *path;
>> +       const char *alias;
>> +       int alias_node, node, offset, ret = 0;
>> +       int alias_len;
>> +       int len;
>> +
>> +       alias = "pmic";
>> +       alias_len = strlen(alias);
>> +
>> +       alias_node = fdt_path_offset(blob, "/aliases");
>> +       offset = fdt_first_property_offset(blob, alias_node);
>> +
>> +       if (offset < 0) {
>> +               error("Alias node not found.");
>> +               return -ENODEV;
>> +       }
>> +
>> +       offset = fdt_first_property_offset(blob, alias_node);
>> +       for (; offset > 0; offset = fdt_next_property_offset(blob, offset)) {
>> +               prop = fdt_get_property_by_offset(blob, offset, &len);
>> +               if (!len)
>> +                       continue;
>> +
>> +               path = fdt_string(blob, fdt32_to_cpu(prop->nameoff));
>> +
>> +               if (!strncmp(alias, path, alias_len))
>> +                       node = fdt_path_offset(blob, prop->data);
>> +               else
>> +                       node = 0;
>> +
>> +               if (node <= 0)
>> +                       continue;
>> +
>> +               ret = lists_bind_fdt(gd->dm_root, blob, node, &dev);
>> +               if (ret < 0)
>> +                       continue;
>> +
>> +               if (device_probe(dev))
>> +                       error("Device: %s, probe error", dev->name);
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +UCLASS_DRIVER(pmic) = {
>> +       .id             = UCLASS_PMIC,
>> +       .name           = "pmic",
>> +};
>> diff --git a/drivers/power/pmic_i2c.c b/drivers/power/pmic_i2c.c
>> new file mode 100644
>> index 0000000..350d375
>> --- /dev/null
>> +++ b/drivers/power/pmic_i2c.c
>> @@ -0,0 +1,136 @@
>> +/*
>> + * Copyright (C) 2014 Samsung Electronics
>> + * Przemyslaw Marczak <p.marczak@samsung.com>
>> + *
>> + * Copyright (C) 2011 Samsung Electronics
>> + * Lukasz Majewski <l.majewski@samsung.com>
>> + *
>> + * (C) Copyright 2010
>> + * Stefano Babic, DENX Software Engineering, sbabic at denx.de
>> + *
>> + * (C) Copyright 2008-2009 Freescale Semiconductor, Inc.
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <linux/types.h>
>> +#include <power/pmic.h>
>> +#include <errno.h>
>> +#include <dm.h>
>> +#include <i2c.h>
>> +
>> +int pmic_i2c_reg_write(struct udevice *dev, unsigned reg, unsigned val)
>> +{
>> +       struct pmic_platdata *p;
>> +       unsigned char buf[4] = { 0 };
>> +
>> +       if (!dev)
>> +               return -ENODEV;
>> +
>> +       p = dev->platdata;
>> +
>> +       if (pmic_check_reg(p, reg))
>> +               return -EINVAL;
>> +
>> +       I2C_SET_BUS(p->bus);
>> +
>> +       switch (pmic_i2c_tx_num) {
>> +       case 3:
>> +               if (p->byte_order == PMIC_SENSOR_BYTE_ORDER_BIG) {
>> +                       buf[2] = (cpu_to_le32(val) >> 16) & 0xff;
>> +                       buf[1] = (cpu_to_le32(val) >> 8) & 0xff;
>> +                       buf[0] = cpu_to_le32(val) & 0xff;
>> +               } else {
>> +                       buf[0] = (cpu_to_le32(val) >> 16) & 0xff;
>> +                       buf[1] = (cpu_to_le32(val) >> 8) & 0xff;
>> +                       buf[2] = cpu_to_le32(val) & 0xff;
>> +               }
>> +               break;
>> +       case 2:
>> +               if (p->byte_order == PMIC_SENSOR_BYTE_ORDER_BIG) {
>> +                       buf[1] = (cpu_to_le32(val) >> 8) & 0xff;
>> +                       buf[0] = cpu_to_le32(val) & 0xff;
>> +               } else {
>> +                       buf[0] = (cpu_to_le32(val) >> 8) & 0xff;
>> +                       buf[1] = cpu_to_le32(val) & 0xff;
>> +               }
>> +               break;
>> +       case 1:
>> +               buf[0] = cpu_to_le32(val) & 0xff;
>> +               break;
>> +       default:
>> +               printf("%s: invalid tx_num: %d", __func__, pmic_i2c_tx_num);
>> +               return -1;
>> +       }
>> +
>> +       if (i2c_write(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>> +               return -1;
>> +
>> +       return 0;
>> +}
>> +
>> +int pmic_i2c_reg_read(struct udevice *dev, unsigned reg, unsigned *val)
>> +{
>> +       struct pmic_platdata *p;
>> +       unsigned char buf[4] = { 0 };
>> +       u32 ret_val = 0;
>> +
>> +       if (!dev)
>> +               return -ENODEV;
>> +
>> +       p = dev->platdata;
>> +
>> +       if (pmic_check_reg(p, reg))
>> +               return -EINVAL;
>> +
>> +       I2C_SET_BUS(p->bus);
>> +
>> +       if (i2c_read(pmic_i2c_addr, reg, 1, buf, pmic_i2c_tx_num))
>> +               return -1;
>> +
>> +       switch (pmic_i2c_tx_num) {
>> +       case 3:
>> +               if (p->byte_order == PMIC_SENSOR_BYTE_ORDER_BIG)
>> +                       ret_val = le32_to_cpu(buf[2] << 16
>> +                                             | buf[1] << 8 | buf[0]);
>> +               else
>> +                       ret_val = le32_to_cpu(buf[0] << 16 |
>> +                                             buf[1] << 8 | buf[2]);
>> +               break;
>> +       case 2:
>> +               if (p->byte_order == PMIC_SENSOR_BYTE_ORDER_BIG)
>> +                       ret_val = le32_to_cpu(buf[1] << 8 | buf[0]);
>> +               else
>> +                       ret_val = le32_to_cpu(buf[0] << 8 | buf[1]);
>> +               break;
>> +       case 1:
>> +               ret_val = le32_to_cpu(buf[0]);
>> +               break;
>> +       default:
>> +               printf("%s: invalid tx_num: %d", __func__, pmic_i2c_tx_num);
>> +               return -1;
>> +       }
>> +       memcpy(val, &ret_val, sizeof(ret_val));
>> +
>> +       return 0;
>> +}
>> +
>> +int pmic_i2c_probe(struct udevice *dev)
>> +{
>> +       struct pmic_platdata *p;
>> +
>> +       if (!dev)
>> +               return -ENODEV;
>> +
>> +       p = dev->platdata;
>> +
>> +       i2c_set_bus_num(p->bus);
>> +       debug("Bus: %d PMIC:%s probed!\n", p->bus, dev->name);
>> +       if (i2c_probe(pmic_i2c_addr)) {
>> +               printf("Can't find PMIC:%s\n", dev->name);
>> +               return -1;
>> +       }
>> +
>> +       return 0;
>> +}
>> diff --git a/drivers/power/pmic_spi.c b/drivers/power/pmic_spi.c
>> new file mode 100644
>> index 0000000..7851adf
>> --- /dev/null
>> +++ b/drivers/power/pmic_spi.c
>> @@ -0,0 +1,137 @@
>> +/*
>> + * Copyright (C) 2014 Samsung Electronics
>> + * Przemyslaw Marczak <p.marczak@samsung.com>
>> + *
>> + * Copyright (C) 2011 Samsung Electronics
>> + * Lukasz Majewski <l.majewski@samsung.com>
>> + *
>> + * (C) Copyright 2010
>> + * Stefano Babic, DENX Software Engineering, sbabic at denx.de
>> + *
>> + * (C) Copyright 2008-2009 Freescale Semiconductor, Inc.
>> + *
>> + * SPDX-License-Identifier:    GPL-2.0+
>> + */
>> +
>> +#include <common.h>
>> +#include <linux/types.h>
>> +#include <power/pmic.h>
>> +#include <errno.h>
>> +#include <dm.h>
>> +#include <spi.h>
>> +
>> +static struct spi_slave *slave;
>> +
>> +void pmic_spi_free(struct spi_slave *slave)
>> +{
>> +       if (slave)
>> +               spi_free_slave(slave);
>> +}
>> +
>> +struct spi_slave *pmic_spi_probe(struct udevice *dev)
>> +{
>> +       struct pmic_platdata *p;
>> +
>> +       if (!dev)
>> +               return NULL;
>> +
>> +       p = dev->platdata;
>> +
>> +       if (pmic_check_reg(p, reg))
>> +               return NULL;
>> +
>> +       return spi_setup_slave(p->bus,
>> +               p->hw.spi.cs,
>> +               p->hw.spi.clk,
>> +               p->hw.spi.mode);
>> +}
>> +
>> +static int pmic_spi_reg(struct udevice *dev, unsigned reg, unsigned *val,
>> +                       int write)
>> +{
>> +       struct pmic_platdata *p;
>> +       u32 pmic_tx, pmic_rx;
>> +       u32 tmp;
>> +
>> +       if (!dev)
>> +               return -EINVAL;
>> +
>> +       p = dev->platdata;
>> +
>> +       if (pmic_check_reg(p, reg))
>> +               return -EFAULT;
>> +
>> +       if (!slave) {
>> +               slave = pmic_spi_probe(p);
>> +
>> +               if (!slave)
>> +                       return -ENODEV;
>> +       }
>> +
>> +       if (pmic_check_reg(p, reg))
>> +               return -EFAULT;
>> +
>> +       if (spi_claim_bus(slave))
>> +               return -EIO;
>> +
>> +       pmic_tx = p->hw.spi.prepare_tx(reg, val, write);
>> +
>> +       tmp = cpu_to_be32(pmic_tx);
>> +
>> +       if (spi_xfer(slave, pmic_spi_bitlen, &tmp, &pmic_rx,
>> +                    pmic_spi_flags)) {
>> +               spi_release_bus(slave);
>> +               return -EIO;
>> +       }
>> +
>> +       if (write) {
>> +               pmic_tx = p->hw.spi.prepare_tx(reg, val, 0);
>> +               tmp = cpu_to_be32(pmic_tx);
>> +               if (spi_xfer(slave, pmic_spi_bitlen, &tmp, &pmic_rx,
>> +                            pmic_spi_flags)) {
>> +                       spi_release_bus(slave);
>> +                       return -EIO;
>> +               }
>> +       }
>> +
>> +       spi_release_bus(slave);
>> +       *val = cpu_to_be32(pmic_rx);
>> +
>> +       return 0;
>> +}
>> +
>> +int pmic_spi_reg_write(struct udevice *dev, unsigned reg, unsigned val)
>> +{
>> +       struct pmic_platdata *p;
>> +
>> +       if (!dev)
>> +               return -EINVAL;
>> +
>> +       p = dev->platdata;
>> +
>> +       if (pmic_check_reg(p, reg))
>> +               return -EFAULT;
>> +
>> +       if (pmic_spi_reg(p, reg, &val, 1))
>> +               return -1;
>> +
>> +       return 0;
>> +}
>> +
>> +int pmic_spi_reg_read(struct udevice *dev, unsigned reg, unsigned *val)
>> +{
>> +       struct pmic_platdata *p;
>> +
>> +       if (!dev)
>> +               return -EINVAL;
>> +
>> +       p = dev->platdata;
>> +
>> +       if (pmic_check_reg(p, reg))
>> +               return -EFAULT;
>> +
>> +       if (pmic_spi_reg(p, reg, val, 0))
>> +               return -1;
>> +
>> +       return 0;
>> +}
>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>> index e3e9296..e6d9d39 100644
>> --- a/include/dm/uclass-id.h
>> +++ b/include/dm/uclass-id.h
>> @@ -29,6 +29,9 @@ enum uclass_id {
>>          UCLASS_SPI_FLASH,       /* SPI flash */
>>          UCLASS_CROS_EC,         /* Chrome OS EC */
>>
>> +       /* PMIC uclass and PMIC related uclasses */
>> +       UCLASS_PMIC,
>> +
>>          UCLASS_COUNT,
>>          UCLASS_INVALID = -1,
>>   };
>> diff --git a/include/power/pmic.h b/include/power/pmic.h
>> index afbc5aa..7114650 100644
>> --- a/include/power/pmic.h
>> +++ b/include/power/pmic.h
>> @@ -1,4 +1,7 @@
>>   /*
>> + *  Copyright (C) 2014 Samsung Electronics
>> + *  Przemyslaw Marczak <p.marczak@samsung.com>
>> + *
>>    *  Copyright (C) 2011-2012 Samsung Electronics
>>    *  Lukasz Majewski <l.majewski@samsung.com>
>>    *
>> @@ -8,9 +11,12 @@
>>   #ifndef __CORE_PMIC_H_
>>   #define __CORE_PMIC_H_
>>
>> +#include <dm.h>
>>   #include <linux/list.h>
>> +#include <spi.h>
>>   #include <i2c.h>
>>   #include <power/power_chrg.h>
>> +#include <power/regulator.h>
>>
>>   enum { PMIC_I2C, PMIC_SPI, PMIC_NONE};
>>   enum { I2C_PMIC, I2C_NUM, };
>> @@ -78,6 +84,63 @@ struct pmic {
>>          struct list_head list;
>>   };
>>
>> +#ifdef CONFIG_DM_PMIC
>> +/* struct pmic_platdata - a standard descriptor for pmic device, which holds
>> + * an informations about interface. It is common for all pmic devices.
>> + *
>> + * Note:
>> + * Interface fields are the same as in: struct pmic.
>> + * Note: struct pmic will be removed in the future after drivers migration
>> + *
>> + * @bus        - a physical bus on which device is connected
>> + * @interface  - an interface type, one of enum: PMIC_I2C, PMIC_SPI, PMIC_NONE
>> + * @byte_order - one of enum: PMIC_SENSOR_BYTE_ORDER*_LITTLE/_BIG
>> + * @regs_num   - number of device registers
>> + * @hw         - one of union structure: p_i2c or p_spi
>> + *               based on @interface field
>> +*/
>> +struct pmic_platdata {
>> +       int bus;
>> +       int interface;
>> +       int byte_order;
>> +       int regs_num;
>> +       union hw hw;
>
> If we have a 'register cache' uclass (later, once i2c is done) then
> this could just be a device.
>
>> +};
>> +
>> +/* enum pmic_op_type - used for various pmic devices operation calls,
>> + * for decrease a number of functions with the same code for read/write
>> + * or get/set.
>> + *
>> + * @PMIC_OP_GET - get operation
>> + * @PMIC_OP_SET - set operation
>> +*/
>> +enum pmic_op_type {
>> +       PMIC_OP_GET,
>> +       PMIC_OP_SET,
>> +};
>> +
>> +/* drivers/power/pmic-uclass.c */
>> +int power_init_dm(void);
>> +struct udevice *pmic_get_by_name(int uclass_id, char *name);
>> +struct udevice *pmic_get_by_interface(int uclass_id, int bus, int addr_cs);
>> +const char *pmic_if_str(int interface);
>> +int pmic_check_reg(struct pmic_platdata *p, unsigned reg);
>> +int pmic_reg_write(struct udevice *dev, unsigned reg, unsigned val);
>> +int pmic_reg_read(struct udevice *dev, unsigned reg, unsigned *val);
>> +int pmic_probe(struct udevice *dev, struct spi_slave *spi_slave);
>> +
>> +/* drivers/power/pmic_i2c.c */
>> +int pmic_i2c_reg_write(struct udevice *dev, unsigned reg, unsigned val);
>> +int pmic_i2c_reg_read(struct udevice *dev, unsigned reg, unsigned *val);
>> +int pmic_i2c_probe(struct udevice *dev);
>> +
>> +/* drivers/power/pmic_spi.c */
>> +int pmic_spi_reg_write(struct udevice *dev, unsigned reg, unsigned val);
>> +int pmic_spi_reg_read(struct udevice *dev, unsigned reg, unsigned *val);
>> +struct spi_slave *pmic_spi_probe(struct udevice *dev);
>> +#endif /* CONFIG_DM_PMIC */
>> +
>> +#ifdef CONFIG_POWER
>>   int pmic_init(unsigned char bus);
>>   int power_init_board(void);
>>   int pmic_dialog_init(unsigned char bus);
>> @@ -88,6 +151,7 @@ int pmic_probe(struct pmic *p);
>>   int pmic_reg_read(struct pmic *p, u32 reg, u32 *val);
>>   int pmic_reg_write(struct pmic *p, u32 reg, u32 val);
>>   int pmic_set_output(struct pmic *p, u32 reg, int ldo, int on);
>
> These should have comments.
>
Ok, I will add it.

>> +#endif
>>
>>   #define pmic_i2c_addr (p->hw.i2c.addr)
>>   #define pmic_i2c_tx_num (p->hw.i2c.tx_num)
>> --
>> 1.9.1
>>
>
> Regards,
> Simon
>
Thank you for the fast review :)

Best Regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com