From mboxrd@z Thu Jan  1 00:00:00 1970
From: Simon Glass <sjg@chromium.org>
Date: Fri, 10 Oct 2014 17:18:42 -0600
Subject: [U-Boot] [PATCH 04/19] dm: pmic: add implementation of driver
	model pmic uclass
In-Reply-To: <5437DFDA.2040407@samsung.com>
References: <1412801335-1591-1-git-send-email-p.marczak@samsung.com>
	<1412801335-1591-5-git-send-email-p.marczak@samsung.com>
	<CAPnjgZ18T9=Hw85GpEuEwpzVBBgM_k1eZC4X+5g7NU2eQzNoBw@mail.gmail.com>
	<5437DFDA.2040407@samsung.com>
Message-ID: <CAPnjgZ25vy7DV8254r1t_azj9jgJesJJwZWnpRZbES5OcBQhcw@mail.gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi,

On 10 October 2014 07:32, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> 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?

We don't need a register cache - I was just thinking of that from the
kernel. But it feels like an interface like:

int reg_read(int reg, uint32_t *value)
int reg_write(int reg, uint32_t value)
int reg_write_range(int reg_start, int reg_count, uint32_t value[])

might be useful - it could be implemented for both I2C and SPI.

>
>
>>> +       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.

I'm thinking that one day you could do something like:

// Find the regmap associated with the device
reg_dev = device_get_child(UCLASS_REGMAP, dev);

// Call the regmap uclass to access registers whether it is I2C or SPI
regmap_write(reg_dev, value);

>
>
>>> +{
>>> +       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?

No there is no such check at present.

>
>
>>> +{
>>> +       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 :)

Very interested to see this development.

Regards,
Simon