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
