Hi Przemyslaw, On 23 April 2015 at 05:33, Przemyslaw Marczak <p.marc...@samsung.com> wrote: > Hello Simon, > > > On 04/22/2015 06:30 PM, Simon Glass wrote: >> >> Hi Przemyslaw, >> >> On 20 April 2015 at 12:07, Przemyslaw Marczak <p.marc...@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.marc...@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 <s...@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.marc...@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.marc...@samsung.com> >>> + * >>> * Copyright (C) 2011-2012 Samsung Electronics >>> * Lukasz Majewski <l.majew...@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.
Yes I think turning this value into a function is fine. I just don't think we should have values in the operations struct. > > >>> + 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@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot