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

Przemyslaw Marczak
2014-10-20T15:44:52Z

Hello Simon,

I missed some of your comments.

On 10/11/2014 01:18 AM, Simon Glass wrote:
> 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.
>

Ok, now I see.

>>
>>
>>>> + 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);
>

This could be an another interesting possible feature of dm.

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

Thank you again.
I'm going to check the i2c-working tree and maybe rebase the dm-pmic
onto it.

Is it good idea?

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