[U-Boot] [PATCH v4 04/16] dm: pmic: add implementation of driver model pmic uclass
Przemyslaw Marczak
p.marczak at samsung.com
Thu Apr 23 13:33:14 CEST 2015
Hello Simon,
On 04/22/2015 06:30 PM, Simon Glass wrote:
> Hi Przemyslaw,
>
> On 20 April 2015 at 12:07, Przemyslaw Marczak <p.marczak at 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 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
>>
>> 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 at 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.
>
That's good information. I will fix it all and resend ASAP.
>>
>> 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 at 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 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>
>
> 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?
>
Adding this to uclass platform data will add another problem - choosing
the right place for setting this value, because it's usually not given
in dts, however the device node "reg" property could also provide the
address length as it is for the memory.
But probably you will agree, that this is a job for I2C/SPI subsystem,
and moreover has no sense, since usually the dts files don't provide the
"reg" property as "<reg length>" for i2c/spi devices.
So maybe putting here an 'ops' function is better idea.
int (*get_reg_count)(struct udevice *dev);
This is used only for the PMIC command, so if return -ENOSYS, then just
"dump" command will no available for the device.
>> + 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
>
Thanks for the review.
Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com
More information about the U-Boot
mailing list