[U-Boot] [PATCH v3 05/17] dm: pmic: add implementation of driver model pmic uclass
Simon Glass
sjg at chromium.org
Sun Mar 29 15:07:36 CEST 2015
Hi Prazemyslaw,
On 24 March 2015 at 14:30, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
> This is an introduction to driver-model multi uclass PMIC support.
> It starts with UCLASS_PMIC - a common PMIC devices uclass type
> to provide device read/write operations only.
Your Kconfig docs are a model to others! It describes the function
very nicely without a lot of words.
Please excuse the nits, they are intended to help it read better.
>
> Beside two basic operations the pmic platform data is introduced,
> which provides basic informations about the pmic device I/O interface
> and is shared with all childs (and should also for childs new uclass
> types in the future).
>
> Usually PMIC devices provides various functionalities with single
> or multiple I/O interfaces.
> Using this new framework and new uclass types introduced in the future,
> it can be handle like this:
>
> _ root device
> |
> |_ BUS 0 device (e.g. I2C0) - UCLASS_I2C/SPI/...
> | |_ PMIC device 1 (read/write ops) - UCLASS_PMIC
> | |_ REGULATOR device (ldo/buck/... ops) - UCLASS_REGULATOR
> | |_ CHARGER device (charger ops) - UCLASS_CHARGER (in the future)
> | |_ MUIC device (microUSB con ops) - UCLASS_MUIC (in the future)
> | |_ ...
> |
> |_ BUS 1 device (e.g. I2C1) - UCLASS_I2C/SPI/...
> |_ PMIC device 2 (read/write ops) - UCLASS_PMIC
> |_ RTC device (rtc ops) - UCLASS_MUIC (in the future)
>
> For each PMIC device interface, new UCLASS_PMIC device is bind with proper
> pmic driver, and it's child devices provides some specified operations.
>
> All new definitions can be found in file:
> - 'include/power/pmic.h'
>
> Uclass file:
> - pmic-uclass.c - provides a common code for UCLASS_PMIC device drivers
>
> The old pmic framework is still kept and is independent.
>
> Changes:
> - new uclass-id: UCLASS_PMIC
> - new config: CONFIG_DM_PMIC
>
> New pmic api is documented in: doc/driver-model/pmic-framework.txt
>
> Signed-off-by: Przemyslaw Marczak <p.marczak at 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
> ---
> drivers/power/Kconfig | 68 ++++++++++++++
> drivers/power/Makefile | 1 +
> drivers/power/pmic-uclass.c | 130 +++++++++++++++++++++++++++
> include/dm/uclass-id.h | 3 +
> include/power/pmic.h | 210 ++++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 412 insertions(+)
> create mode 100644 drivers/power/pmic-uclass.c
>
> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
> index f8f0239..3513b46 100644
> --- a/drivers/power/Kconfig
> +++ b/drivers/power/Kconfig
> @@ -1,3 +1,71 @@
> +config DM_PMIC
> + bool "Enable Driver Model for PMIC drivers (UCLASS_PMIC)"
> + depends on DM
> + ---help---
> + This config enables the driver-model multi uclass PMIC support.
> + Its basic uclass type is: UCLASS_PMIC, which is designed to provide
> + a common I/O interface for pmic child devices of various uclass types.
Should be consistent - and use PMIC instead pmic.
> +
> + Usually PMIC IC's provides more than one functionality, which means
s/functionality/function/
> + that we should implement new uclass operations for each one. Usually
> + PMIC's provide those various functionalities by one or more interfaces.
functions
> + And this could looks like this:
> +
> + root device
> + |_ BUS 0 device (e.g. I2C0) - UCLASS_I2C/SPI/...
> + | |_ PMIC device (READ/WRITE ops) - UCLASS_PMIC
> + | | (pmic sub-devices)
> + | |_ REGULATOR device (ldo/buck/... ops) - UCLASS_REGULATOR
> + | |_ CHARGER device (charger ops) - UCLASS_CHARGER (future)
> + | |_ MUIC device (microUSB connector ops) - UCLASS_MUIC (future)
> + | |_ ...
> + |
> + |_ BUS 1 device (e.g. I2C1) - UCLASS_I2C/SPI/...
> + |_ PMIC device (READ/WRITE ops) - UCLASS_PMIC
> + | (pmic sub-devices)
> + |_ RTC device (rtc ops) - UCLASS_MUIC (future)
Would this be UCLASS_RTC?
> +
> + From the I/O interface point of view, there can be found two PMIC types:
> + - single I/O interface - then UCLASS_PMIC device should be a parent of
> + all pmic sub-devices, where each is usually different uclass type, but
> + need to access the same interface
> +
> + - multiple I/O interfaces - for each interface the UCLASS_PMIC device
> + should be a parent of only those devices (different uclass types),
> + which needs to access the specified interface.
> +
> + For each case, binding should be done automatically. If only device tree
Remove the word 'only'?
> + nodes/subnodes are proper defined, then:
how about a blank line here?
> + |_ 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:
> + | (pmic sub-devices)
> + |_ regulator (child)
> + |_ charger (child)
> + |_ other (child)
> +
> + The same for other I/O bus nodes, if pmic uses more then one interface.
> +
> + Note:
> + Each PMIC interface driver should use different compatible string.
use a different
> +
> + There are few basic functions in the UCLASS_PMIC driver API, declared
There are a few
> + in the file 'include/power/pmic.h':
> + - int pmic_get(...);
> + - int pmic_read(struct udevice *pmic, ...);
> + - int pmic_write(struct udevice *pmic, ...);
> + For the simple implementation, in some cases the pmic uclass device,
> + can be self-sufficient to drive the PMIC functionality. In other case,
s/self-sufficient/enough/
> + if each pmic sub-device(child) driver need access to the pmic specified
PMIC-specified
> + registers, it need to know only the register address and then the access
s/to//
> + is done through the parent pmic driver. Like in the example:
s/is/can be/
For example: (blank line here?)
> + _ root driver
> + |_ dev: bus I2C0 - UCLASS_I2C
> + | |_ dev: my_pmic (read/write) (parent) - UCLASS_PMIC
> + | |_ dev: my_regulator (set value/etc.) (child) - UCLASS_REGULATOR
> + So the call will looks like below:
> + 'pmic_write(regulator->parent, addr, value, len);'
> +
> config AXP221_POWER
> boolean "axp221 / axp223 pmic support"
> depends on MACH_SUN6I || MACH_SUN8I
> diff --git a/drivers/power/Makefile b/drivers/power/Makefile
> index 2145652..5c9a189 100644
> --- a/drivers/power/Makefile
> +++ b/drivers/power/Makefile
> @@ -21,3 +21,4 @@ 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) += pmic-uclass.o
> diff --git a/drivers/power/pmic-uclass.c b/drivers/power/pmic-uclass.c
> new file mode 100644
> index 0000000..df49a4b
> --- /dev/null
> +++ b/drivers/power/pmic-uclass.c
> @@ -0,0 +1,130 @@
> +/*
> + * Copyright (C) 2014-2015 Samsung Electronics
> + * Przemyslaw Marczak <p.marczak at samsung.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0+
> + */
> +#include <common.h>
> +#include <linux/types.h>
> +#include <fdtdec.h>
> +#include <dm.h>
> +#include <power/pmic.h>
> +#include <dm/device-internal.h>
> +#include <dm/uclass-internal.h>
> +#include <dm/root.h>
> +#include <dm/lists.h>
> +#include <compiler.h>
> +#include <errno.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +int pmic_get(char *name, struct udevice **devp)
> +{
> + struct udevice *dev;
> + int ret;
> +
> + *devp = NULL;
> +
> + for (ret = uclass_first_device(UCLASS_PMIC, &dev);
> + dev;
> + ret = uclass_next_device(&dev)) {
> + if (!strcmp(name, dev->name)) {
> + *devp = dev;
> + return ret;
> + }
> + }
> +
> + return -ENODEV;
This function will probe every PMIC, looking for the correct one.
I think you should change pmic_get() to call a core function in
uclass.c, like uclass_get_device_by_name(). It should use
uclass_foreach_dev() so that it doesn't probe anything except the one
it finds.You can use uclass_get_device_tail() at the end of that
function - see uclass_get_device_by_of_offset() as an example.
> +}
> +
> +int pmic_reg_count(struct udevice *dev)
> +{
> + const struct dm_pmic_ops *ops;
> +
> + ops = pmic_get_uclass_ops(dev, UCLASS_PMIC);
> + if (!ops)
> + return -ENODEV;
> +
> + return ops->reg_count;
> +}
> +
> +int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len)
> +{
> + const struct dm_pmic_ops *ops;
> +
> + if (!buffer)
> + return -EFAULT;
> +
> + ops = pmic_get_uclass_ops(dev, UCLASS_PMIC);
> + if (!ops)
> + return -ENODEV;
-ENOSYS, although IMO assert() would be OK.
> +
> + if (!ops->read)
> + return -EPERM;
-ENOSYS
> +
> + if (ops->read(dev, reg, buffer, len))
> + return -EIO;
We must not swallow errors.
ret = ops->read(...)
if (ret)
return ret;
Please adjust below also.
> +
> + return 0;
> +}
> +
> +int pmic_write(struct udevice *dev, uint reg, uint8_t *buffer, int len)
> +{
> + const struct dm_pmic_ops *ops;
> +
> + if (!buffer)
> + return -EFAULT;
> +
> + ops = pmic_get_uclass_ops(dev, UCLASS_PMIC);
> + if (!ops)
> + return -ENODEV;
> +
> + if (!ops->write)
> + return -EPERM;
> +
> + if (ops->write(dev, reg, buffer, len))
> + return -EIO;
> +
> + return 0;
> +}
> +
> +int pmic_child_node_scan(struct udevice *parent,
> + const char *optional_subnode,
> + const char *compatible_property_name)
> +{
> + const void *blob = gd->fdt_blob;
> + struct udevice *childp;
> + int offset = parent->of_offset;
> + int childs = 0;
> + int ret;
> +
> + debug("%s: bind child for %s\n", __func__, parent->name);
> +
> + if (optional_subnode) {
> + debug("Looking for %s optional subnode: %s \n", parent->name,
> + optional_subnode);
> + offset = fdt_subnode_offset(blob, offset, optional_subnode);
> + if (offset <= 0) {
> + debug("Pmic: %s subnode: %s not found!",
> + parent->name, optional_subnode);
> + return -ENXIO;
> + }
> + }
> +
> + for (offset = fdt_first_subnode(blob, offset); offset > 0;
> + offset = fdt_next_subnode(blob, offset)) {
> + ret = lists_bind_fdt_by_prop(parent, blob, offset,
> + compatible_property_name, &childp);
> + if (ret)
> + continue;
> +
> + childs++;
> + }
> +
> + return childs;
> +}
> +
> +UCLASS_DRIVER(pmic) = {
> + .id = UCLASS_PMIC,
> + .name = "pmic",
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index 91bb90d..3ecfa23 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -35,6 +35,9 @@ enum uclass_id {
> UCLASS_I2C_EEPROM, /* I2C EEPROM device */
> UCLASS_MOD_EXP, /* RSA Mod Exp device */
>
> + /* PMIC uclass and PMIC-related uclass types */
This comment seemed confusing - is it for PMICs or other things also?
> + UCLASS_PMIC,
> +
> UCLASS_COUNT,
> UCLASS_INVALID = -1,
> };
> diff --git a/include/power/pmic.h b/include/power/pmic.h
> index afbc5aa..b55304f 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 at samsung.com>
> + *
> * Copyright (C) 2011-2012 Samsung Electronics
> * Lukasz Majewski <l.majewski at 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>
>
> 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,210 @@ struct pmic {
> struct pmic *parent;
> struct list_head list;
> };
> +#endif /* CONFIG_POWER */
> +
> +#ifdef CONFIG_DM_PMIC
> +/**
> + * Driver model pmic framework.
> + * The PMIC_UCLASS uclass is designed to provide a common I/O
> + * interface for pmic child devices of various uclass types.
I worry about having the docs in multiple places. Should you adjust
this to point to the Kconfig? Or change the Kconfig to point here? Or
maybe it would be better to drop both and put these in your README? I
am concerned that if we later change something, we end up with
inconsistent docs.
> + *
> + * Usually PMIC devices provides more than one functionality,
> + * which means that we should implement uclass operations for
> + * each functionality - one driver per uclass.
> + *
> + * And usually PMIC devices provide those various functionalities
> + * by one or more interfaces. And this could looks 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_MUIC (in the future)
> + *
> + * Two PMIC types are possible:
> + * - single I/O interface
> + * Then UCLASS_PMIC device should be a parent of all pmic devices, where each
> + * is usually different uclass type, but need to access the same interface
> + *
> + * - multiple I/O interfaces
> + * For each interface the UCLASS_PMIC device should be a parent of only those
> + * devices (different uclass types), which needs to access the specified
> + * interface.
> + *
> + * For each case binding should be done automatically. If only 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 bus nodes, if pmic uses more then one interface.
> + *
> + * Note:
> + * Each PMIC interface driver should use different compatible string.
> + *
> + * If each pmic child device driver need access the pmic specified registers,
> + * it need to know only the register address and the access is 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 by the call
> + * to 'pmic_child_node_scan()' - it allows bind in optional subnode and with
> + * optional compatible property, e.g. "regulator-compatible" or "regulator".
> + *
> + * my_pmic.c - pmic driver:
> + *
> + * int my_pmic_bind(struct udevice *my_pmic)
> + * {
> + * ...
> + * ret = pmic_child_node_scan(my_pmic, NULL, "compatible");
> + * ...
> + * ...
> + * }
> + *
> + * my_regulator.c - regulator driver (child of pmic I/O driver):
> + *
> + * int my_regulator_set_value(struct udevice *dev, int type, int num, int *val)
> + * {
> + * ...
> + * some_val = ...;
> + *
> + * // dev->parent == my_pmic
> + * pmic_write(dev->parent, some_reg, some_val);
> + * ...
> + * }
> + *
> + * In this example pmic driver is always a parent for other pmic devices.
> + */
> +
> +/**
> + * 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;
> + int (*read)(struct udevice *dev, uint reg, uint8_t *buffer, int len);
> + int (*write)(struct udevice *dev, uint reg, uint8_t *buffer, int len);
nit: const uint8_t *buffer?
> +};
> +
> +/* enum pmic_op_type - used for various pmic devices operation calls,
> + * 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,
> +};
> +
> +/**
> + * pmic_get_uclass_ops - inline function for getting device uclass operations
> + *
> + * @dev - device to check
> + * @uclass_id - device uclass id
> + *
> + * Returns:
> + * void pointer to device operations or NULL pointer on error
> + */
> +static __inline__ const void *pmic_get_uclass_ops(struct udevice *dev,
> + int uclass_id)
> +{
> + const void *ops;
> +
> + if (!dev)
> + return NULL;
> +
> + if (dev->driver->id != uclass_id)
> + return NULL;
> +
> + ops = dev->driver->ops;
> + if (!ops)
> + return NULL;
> +
> + return ops;
> +}
> +
> +/* drivers/power/pmic-uclass.c */
> +
> +/**
> + * pmic_child_node_scan - scan the pmic node or its 'optional_subnode' for its
> + * childs, by the compatible property name given by arg 'compatible_prop_name'.
> + *
> + * Few dts files with a different pmic regulators, uses different property
> + * name for its driver 'compatible' string, like:
> + * - 'compatible'
> + * 'regulator-compatible'
> + * The pmic driver should should bind its devices by proper compatible property
> + * name.
> + *
> + * @parent - pmic parent device (usually UCLASS_PMIC)
> + * @optional_subnode - optional pmic subnode with the childs within it
> + * @compatible_property_name - e.g.: 'compatible'; 'regulator-compatible', ...
@return
> + */
> +int pmic_child_node_scan(struct udevice *parent,
> + const char *optional_subnode,
> + const char *compatible_property_name);
> +
> +/**
> + * pmic_get: get the pmic device using its name
> + *
> + * @name - device name
> + * @devp - returned pointer to the pmic device
> + * Returns: 0 on success or negative value of errno.
* @return 0 on success or negative value of errno.
> + *
> + * The returned devp device can be used with pmic_read/write calls
> + */
> +int pmic_get(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
> + *
> + * Returns: 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
> + * @val - value for write or its pointer to read
This doesn't match the functions.
> + *
> + * Returns: 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, uint8_t *buffer, int len);
const uint8_t buffer for pmic_write()?
> +#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 +297,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
More information about the U-Boot
mailing list