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

Simon Glass
2015-04-22T16:30:01Z

Hi Przemyslaw,

On 20 April 2015 at 12:07, Przemyslaw Marczak <p.marczak@samsung.com> wrote:
> This commit introduces the PMIC uclass implementation.
> It allows providing the basic I/O interface for PMIC devices.
> For the multi-function PMIC devices, this can be used as I/O
> parent device, for each IC's interface. Then, each PMIC particular
> function can be provided by the child device's operations, and the
> child devices will use its parent for read/write by the common API.
>
> Core files:
> - 'include/power/pmic.h'
> - 'drivers/power/pmic/pmic-uclass.c'
>
> The old pmic framework is still kept and is independent.
>
> For more detailed informations, please look into the header file.
>
> Changes:
> - new uclass-id: UCLASS_PMIC
> - new config: CONFIG_DM_PMIC
>
> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
> ---
> Changes V2:
> - pmic uclass: adjust uclass code to the mainline changes
> - pmic uclass: remove pmic_i2c and pmic_spi
> - pmic uclass: modify pmic_platdata
> - pmic uclass: add pmic_if_* functions
> - pmic uclass: remove pmic_init_dm()
> - pmic uclass: cleanup
> - pmic.h: define pmic ops structure (read/write operations)
> - pmic.h: add comments to functions
>
> Changes V3:
> - pmic-uclass.c and pmic.h:
> -- remove pmic_if_* functions
> -- add new function pmic_child_node_scan()
> - add Kconfig entry
>
> Changes V4:
> - move drivers/power/pmic-uclass.c to drivers/power/pmic/pmic-uclass.c
> - move DM_PMIC Kconfig entry: drivers/power/Kconfig to drivers/power/pmic/Kconfig
> - drivers/power/Kconfig: Add menu "Power" and include pmic Kconfig
> - Kconfig: provide only the general information about the PMIC
> - pmic-uclass.c: add pmic_bind_childs()
> - pmic-uclass.c: add debug
> - pmic-uclass.c: cleanup includes
> - pmic-uclass.c: remove pmic_get_uclass_ops() and use of dev_get_driver_ops()
> - pmic-uclass.c: use of uclass_get_device_by_name()
> - pmic-uclass.c: add str_get_num() - for get number from string
> - include/power/pmic.h - start comments rewording
> - power/pmic.h: comments update
> ---
> drivers/power/Kconfig | 6 ++
> drivers/power/pmic/Kconfig | 11 +++
> drivers/power/pmic/Makefile | 1 +
> drivers/power/pmic/pmic-uclass.c | 158 ++++++++++++++++++++++++++++++++
> include/dm/uclass-id.h | 3 +
> include/power/pmic.h | 189 +++++++++++++++++++++++++++++++++++++++
> 6 files changed, 368 insertions(+)
> create mode 100644 drivers/power/pmic/Kconfig
> create mode 100644 drivers/power/pmic/pmic-uclass.c

Acked-by: Simon Glass <sjg@chromium.org>

I have a few nits below - perhaps they can be targeted in a follow-up
patch or two? I'd like to merge this soon and it is not worth holding
up the series for nits.

>
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index f8f0239..d03626e 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -1,3 +1,7 @@
> +menu "Power"
> +
> +source "drivers/power/pmic/Kconfig"
> +
> config AXP221_POWER
> boolean "axp221 / axp223 pmic support"
> depends on MACH_SUN6I || MACH_SUN8I
> @@ -73,3 +77,5 @@ config AXP221_ELDO3_VOLT
> disable eldo3. On some A31(s) tablets it might be used to supply
> 1.2V for the SSD2828 chip (converter of parallel LCD interface
> into MIPI DSI).
> +
> +endmenu
> diff --git a/drivers/power/pmic/Kconfig b/drivers/power/pmic/Kconfig
> new file mode 100644
> index 0000000..d06d632
> --- /dev/null
> +++ b/drivers/power/pmic/Kconfig
> @@ -0,0 +1,11 @@
> +config DM_PMIC
> + bool "Enable Driver Model for PMIC drivers (UCLASS_PMIC)"
> + depends on DM
> + ---help---
> + This config enables the driver-model PMIC support.
> + UCLASS_PMIC - designed to provide an I/O interface for PMIC devices.
> + For the multi-function PMIC devices, this can be used as parent I/O
> + device for each IC's interface. Then, each children uses its parent
> + for read/write. For detailed description, please refer to the files:
> + - 'drivers/power/pmic/pmic-uclass.c'
> + - 'include/power/pmic.h'
> diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile
> index 985cfdb..594f620 100644
> --- a/drivers/power/pmic/Makefile
> +++ b/drivers/power/pmic/Makefile
> @@ -5,6 +5,7 @@
> # SPDX-License-Identifier: GPL-2.0+
> #
>
> +obj-$(CONFIG_DM_PMIC) += pmic-uclass.o
> obj-$(CONFIG_POWER_LTC3676) += pmic_ltc3676.o
> obj-$(CONFIG_POWER_MAX8998) += pmic_max8998.o
> obj-$(CONFIG_POWER_MAX8997) += pmic_max8997.o
> diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c
> new file mode 100644
> index 0000000..d82d3da
> --- /dev/null
> +++ b/drivers/power/pmic/pmic-uclass.c
> @@ -0,0 +1,158 @@
> +/*
> + * Copyright (C) 2014-2015 Samsung Electronics
> + * Przemyslaw Marczak <p.marczak@samsung.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <fdtdec.h>
> +#include <errno.h>
> +#include <dm.h>
> +#include <dm/lists.h>
> +#include <dm/device-internal.h>
> +#include <dm/uclass-internal.h>
> +#include <power/pmic.h>
> +#include <linux/ctype.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +static ulong str_get_num(const char *ptr, const char *maxptr)
> +{
> + if (!ptr || !maxptr)
> + return 0;
> +
> + while (!isdigit(*ptr) && ptr++ < maxptr);
> +
> + return simple_strtoul(ptr, NULL, 0);
> +}
> +
> +int pmic_bind_childs(struct udevice *pmic, int offset,
> + const struct pmic_child_info *child_info)
> +{
> + const struct pmic_child_info *info;
> + const void *blob = gd->fdt_blob;
> + struct driver *drv;
> + struct udevice *child;
> + const char *node_name;
> + int node_name_len;
> + int bind_count = 0;
> + int node;
> + int prefix_len;
> + int ret;
> +
> + debug("%s for '%s' at node offset: %d\n", __func__, pmic->name,
> + pmic->of_offset);
> +
> + for (node = fdt_first_subnode(blob, offset);
> + node > 0;
> + node = fdt_next_subnode(blob, node)) {
> + node_name = fdt_get_name(blob, node, &node_name_len);
> +
> + debug("* Found child node: '%s' at offset:%d\n", node_name,
> + node);
> +
> + child = NULL;
> + info = child_info;
> + while (info->prefix) {
> + prefix_len = strlen(info->prefix);
> + if (strncasecmp(info->prefix, node_name, prefix_len) ||
> + !info->driver) {
> + info++;
> + continue;
> + }
> +
> + debug(" - compatible prefix: '%s'\n", info->prefix);
> +
> + drv = lists_driver_lookup_name(info->driver);
> + if (!drv) {
> + debug(" - driver: '%s' not found!\n",
> + info->driver);
> + continue;
> + }
> +
> + debug(" - found child driver: '%s'\n", drv->name);
> +
> + ret = device_bind(pmic, drv, node_name, NULL,
> + node, &child);
> + if (ret) {
> + debug(" - child binding error: %d\n", ret);
> + continue;
> + }
> +
> + debug(" - bound child device: '%s'\n", child->name);
> +
> + child->driver_data = str_get_num(node_name +
> + prefix_len,
> + node_name +
> + node_name_len);
> +
> + debug(" - set 'child->driver_data': %lu\n",
> + child->driver_data);
> + break;
> + }
> +
> + if (child)
> + bind_count++;
> + else
> + debug(" - compatible prefix not found\n");
> + }
> +
> + debug("Bound: %d childs for PMIC: '%s'\n", bind_count, pmic->name);
> + return bind_count;
> +}
> +
> +int pmic_get(const char *name, struct udevice **devp)
> +{
> + return uclass_get_device_by_name(UCLASS_PMIC, name, devp);
> +}
> +
> +int pmic_reg_count(struct udevice *dev)
> +{
> + const struct dm_pmic_ops *ops = dev_get_driver_ops(dev);

blank line here

> + if (!ops)
> + return -ENOSYS;
> +
> + return ops->reg_count;
> +}
> +
> +int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len)
> +{
> + const struct dm_pmic_ops *ops = dev_get_driver_ops(dev);
> + int ret;
> +
> + if (!buffer)
> + return -EFAULT;
> +
> + if (!ops || !ops->read)
> + return -ENOSYS;
> +
> + ret = ops->read(dev, reg, buffer, len);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +int pmic_write(struct udevice *dev, uint reg, const uint8_t *buffer, int len)
> +{
> + const struct dm_pmic_ops *ops = dev_get_driver_ops(dev);
> + int ret;
> +
> + if (!buffer)
> + return -EFAULT;
> +
> + if (!ops || !ops->write)
> + return -ENOSYS;
> +
> + ret = ops->write(dev, reg, buffer, len);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +UCLASS_DRIVER(pmic) = {
> + .id = UCLASS_PMIC,
> + .name = "pmic",
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index fddfd35..23b3eb9 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -46,6 +46,9 @@ enum uclass_id {
> UCLASS_USB_DEV_GENERIC, /* USB generic device */
> UCLASS_MASS_STORAGE, /* Mass storage device */
>
> + /* Power Management */
> + UCLASS_PMIC, /* PMIC I/O device */
> +
> UCLASS_COUNT,
> UCLASS_INVALID = -1,
> };
> diff --git a/include/power/pmic.h b/include/power/pmic.h
> index afbc5aa..f7ae781 100644
> --- a/include/power/pmic.h
> +++ b/include/power/pmic.h
> @@ -1,4 +1,7 @@
> /*
> + * Copyright (C) 2014-2015 Samsung Electronics
> + * Przemyslaw Marczak <p.marczak@samsung.com>
> + *
> * Copyright (C) 2011-2012 Samsung Electronics
> * Lukasz Majewski <l.majewski@samsung.com>
> *
> @@ -9,10 +12,13 @@
> #define __CORE_PMIC_H_
>
> #include <linux/list.h>
> +#include <spi.h>
> #include <i2c.h>
> #include <power/power_chrg.h>

At some point can you update the ordering of this lot? I think it should be:

i2c.g
spi.h
linux/
power/

>
> enum { PMIC_I2C, PMIC_SPI, PMIC_NONE};
> +
> +#ifdef CONFIG_POWER
> enum { I2C_PMIC, I2C_NUM, };
> enum { PMIC_READ, PMIC_WRITE, };
> enum { PMIC_SENSOR_BYTE_ORDER_LITTLE, PMIC_SENSOR_BYTE_ORDER_BIG, };
> @@ -77,7 +83,189 @@ struct pmic {
> struct pmic *parent;
> struct list_head list;
> };
> +#endif /* CONFIG_POWER */
> +
> +#ifdef CONFIG_DM_PMIC
> +/**
> + * U-Boot PMIC Framework
> + * =====================
> + *
> + * UCLASS_PMIC - The is designed to provide an I/O interface for PMIC devices.

This is designed

> + *
> + * For the multi-function PMIC devices, this can be used as parent I/O device
> + * for each IC's interface. Then, each children uses its parent for read/write.

each child

> + *
> + * The driver model tree could look like this:
> + *
> + *_ root device
> + * |_ BUS 0 device (e.g. I2C0) - UCLASS_I2C/SPI/...
> + * | |_ PMIC device (READ/WRITE ops) - UCLASS_PMIC
> + * | |_ REGULATOR device (ldo/buck/... ops) - UCLASS_REGULATOR
> + * | |_ CHARGER device (charger ops) - UCLASS_CHARGER (in the future)
> + * | |_ MUIC device (microUSB connector ops) - UCLASS_MUIC (in the future)
> + * | |_ ...
> + * |
> + * |_ BUS 1 device (e.g. I2C1) - UCLASS_I2C/SPI/...
> + * |_ PMIC device (READ/WRITE ops) - UCLASS_PMIC
> + * |_ RTC device (rtc ops) - UCLASS_RTC (in the future)
> + *
> + * We can find two PMIC cases in boards design:
> + * - single I/O interface
> + * - multiple I/O interfaces
> + * We bind single PMIC device for each interface, to provide an I/O as a parent,

a single

> + * of proper child devices. Each child usually implements a different function,
> + * controlled by the same interface.
> + *
> + * The binding should be done automatically. If device tree nodes/subnodes are
> + * proper defined, then:
> + *
> + * |_ the ROOT driver will bind the device for I2C/SPI node:
> + * |_ the I2C/SPI driver should bind a device for pmic node:
> + * |_ the PMIC driver should bind devices for its childs:
> + * |_ regulator (child)
> + * |_ charger (child)
> + * |_ other (child)
> + *
> + * The same for other device nodes, for multi-interface PMIC.
> + *
> + * Note:
> + * Each PMIC interface driver should use a different compatible string.
> + *
> + * If each pmic child device driver need access the PMIC-specific registers,

If a pmic child device driver needs

> + * it need know only the register address and the access can be done through
> + * the parent pmic driver. Like in the example:
> + *
> + *_ root driver
> + * |_ dev: bus I2C0 - UCLASS_I2C
> + * | |_ dev: my_pmic (read/write) (is parent) - UCLASS_PMIC
> + * | |_ dev: my_regulator (set value/etc..) (is child) - UCLASS_REGULATOR
> + *
> + * To ensure such device relationship, the pmic device driver should also bind
> + * all its child devices, like in the example below. It should be done by call

by calling pmic_bind_childs()

(which you should rename to pmic_bind_children() I think)

> + * the 'pmic_bind_childs()' - please refer to the description of this function
> + * in this header file. This function, should be called in the driver's '.bind'
> + * method.
> + *
> + * For the example driver, please refer the MAX77686 driver:
> + * - 'drivers/power/pmic/max77686.c'
> + */
> +
> +/**
> + * struct dm_pmic_ops - PMIC device I/O interface
> + *
> + * Should be implemented by UCLASS_PMIC device drivers. The standard
> + * device operations provides the I/O interface for it's childs.
> + *
> + * @reg_count: devices register count
> + * @read: read 'len' bytes at "reg" and store it into the 'buffer'
> + * @write: write 'len' bytes from the 'buffer' to the register at 'reg' address
> + */
> +struct dm_pmic_ops {
> + int reg_count;

This should not be in ops as it is not an operation. Perhaps add a
function for it, or put it in the uclass platform data?

> + int (*read)(struct udevice *dev, uint reg, uint8_t *buffer, int len);
> + int (*write)(struct udevice *dev, uint reg, const uint8_t *buffer,
> + int len);
> +};
> +
> +/* enum pmic_op_type - used for various pmic devices operation calls,

/**
* enum pmic_op_type -

> + * for reduce a number of lines 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,
> +};
> +
> +/**
> + * struct pmic_child_info - basic device's child info for bind child nodes with
> + * the driver by the node name prefix and driver name. This is a helper struct
> + * for function: pmic_bind_childs().
> + *
> + * @prefix - child node name prefix (or its name if is unique or single)
> + * @driver - driver name for the sub-node with prefix
> + */
> +struct pmic_child_info {
> + const char *prefix;
> + const char *driver;
> +};
> +
> +/* drivers/power/pmic-uclass.c */
> +
> +/**
> + * pmic_bind_childs() - bind drivers for given parent pmic, using child info
> + * found in 'child_info' array.
> + *
> + * @pmic - pmic device - the parent of found child's
> + * @child_info - N-childs info array
> + * @return a positive number of childs, or 0 if no child found (error)
> + *
> + * Note: For N-childs the child_info array should have N+1 entries and the last
> + * entry prefix should be NULL - the same as for drivers compatible.
> + *
> + * For example, a single prefix info (N=1):
> + * static const struct pmic_child_info bind_info[] = {
> + * { .prefix = "ldo", .driver = "ldo_driver" },
> + * { },
> + * };
> + *
> + * This function is useful for regulator sub-nodes:
> + * my_regulator at 0xa {
> + * reg = <0xa>;
> + * (pmic - bind automatically by compatible)
> + * compatible = "my_pmic";
> + * ...
> + * (pmic's childs - bind by pmic_bind_childs())
> + * (nodes prefix: "ldo", driver: "my_regulator_ldo")
> + * ldo1 { ... };
> + * ldo2 { ... };
> + *
> + * (nodes prefix: "buck", driver: "my_regulator_buck")
> + * buck1 { ... };
> + * buck2 { ... };
> + * };
> + */
> +int pmic_bind_childs(struct udevice *pmic, int offset,
> + const struct pmic_child_info *child_info);

pmic_bind_children

> +
> +/**
> + * pmic_get: get the pmic device using its name
> + *
> + * @name - device name
> + * @devp - returned pointer to the pmic device
> + * @return 0 on success or negative value of errno.
> + *
> + * The returned devp device can be used with pmic_read/write calls
> + */
> +int pmic_get(const char *name, struct udevice **devp);
> +
> +/**
> + * pmic_reg_count: get the pmic register count
> + *
> + * The required pmic device can be obtained by 'pmic_get()'
> + *
> + * @dev - pointer to the UCLASS_PMIC device
> + * @return register count value on success or negative value of errno.
> + */
> +int pmic_reg_count(struct udevice *dev);
> +
> +/**
> + * pmic_read/write: read/write to the UCLASS_PMIC device
> + *
> + * The required pmic device can be obtained by 'pmic_get()'
> + *
> + * @pmic - pointer to the UCLASS_PMIC device
> + * @reg - device register offset
> + * @buffer - pointer to read/write buffer
> + * @len - byte count for read/write
> + * @return 0 on success or negative value of errno.
> + */
> +int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len);
> +int pmic_write(struct udevice *dev, uint reg, const uint8_t *buffer, int len);
> +#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 +276,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);
> +#endif
>
> #define pmic_i2c_addr (p->hw.i2c.addr)
> #define pmic_i2c_tx_num (p->hw.i2c.tx_num)
> --
> 1.9.1
>

Regards,
Simon