[U-Boot] [PATCH 01/12] dm: pmic: code cleanup of PMIC uclass driver
Simon Glass
sjg at chromium.org
Sun May 10 15:56:32 CEST 2015
Hi Przemyslaw,
On 8 May 2015 at 10:20, Przemyslaw Marczak <p.marczak at samsung.com> wrote:
> The cleanup includes:
> - pmic.h - fix mistakes in a few comments
> - pmic operations: value 'reg_count' - redefine as function call
> - fix function name: pmic_bind_childs() -> pmic_bind_children()
> - pmic_bind_children: increment child_info pointer if operation in loop fail
>
> Signed-off-by: Przemyslaw Marczak <p.marczak at samsung.com>
> ---
> drivers/power/pmic/pmic-uclass.c | 25 +++++++++----------------
> include/power/pmic.h | 39 ++++++++++++++++++++-------------------
> 2 files changed, 29 insertions(+), 35 deletions(-)
>
Acked-by: Simon Glass <sjg at chromium.org>
Tested on sandbox:
Tested-by: Simon Glass <sjg at chromium.org>
BTW in pmic_bind_children() you have something like this:
info = child_info;
while (info->prefix) {
...
if (ret) {
debug(" - child binding error: %d\n", ret);
info++;
continue;
}
I think this would be better:
for (info = child_info; info->prefix, info++)
and remove the three info++ expressions inside the loop.
> diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c
> index d82d3da..6766bd5 100644
> --- a/drivers/power/pmic/pmic-uclass.c
> +++ b/drivers/power/pmic/pmic-uclass.c
> @@ -27,8 +27,8 @@ static ulong str_get_num(const char *ptr, const char *maxptr)
> return simple_strtoul(ptr, NULL, 0);
> }
>
> -int pmic_bind_childs(struct udevice *pmic, int offset,
> - const struct pmic_child_info *child_info)
> +int pmic_bind_children(struct udevice *pmic, int offset,
> + const struct pmic_child_info *child_info)
> {
> const struct pmic_child_info *info;
> const void *blob = gd->fdt_blob;
> @@ -68,6 +68,7 @@ int pmic_bind_childs(struct udevice *pmic, int offset,
> if (!drv) {
> debug(" - driver: '%s' not found!\n",
> info->driver);
> + info++;
> continue;
> }
>
> @@ -77,6 +78,7 @@ int pmic_bind_childs(struct udevice *pmic, int offset,
> node, &child);
> if (ret) {
> debug(" - child binding error: %d\n", ret);
> + info++;
> continue;
> }
>
> @@ -110,16 +112,16 @@ int pmic_get(const char *name, struct udevice **devp)
> int pmic_reg_count(struct udevice *dev)
> {
> const struct dm_pmic_ops *ops = dev_get_driver_ops(dev);
> - if (!ops)
> +
> + if (!ops || !ops->reg_count)
> return -ENOSYS;
>
> - return ops->reg_count;
> + return ops->reg_count(dev);
> }
>
> 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;
> @@ -127,17 +129,12 @@ int pmic_read(struct udevice *dev, uint reg, uint8_t *buffer, int len)
> if (!ops || !ops->read)
> return -ENOSYS;
>
> - ret = ops->read(dev, reg, buffer, len);
> - if (ret)
> - return ret;
> -
> - return 0;
> + return ops->read(dev, reg, buffer, len);
> }
>
> 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;
> @@ -145,11 +142,7 @@ int pmic_write(struct udevice *dev, uint reg, const uint8_t *buffer, int len)
> if (!ops || !ops->write)
> return -ENOSYS;
>
> - ret = ops->write(dev, reg, buffer, len);
> - if (ret)
> - return ret;
> -
> - return 0;
> + return ops->write(dev, reg, buffer, len);
> }
>
> UCLASS_DRIVER(pmic) = {
> diff --git a/include/power/pmic.h b/include/power/pmic.h
> index f7ae781..eb152ef 100644
> --- a/include/power/pmic.h
> +++ b/include/power/pmic.h
> @@ -11,9 +11,9 @@
> #ifndef __CORE_PMIC_H_
> #define __CORE_PMIC_H_
>
> -#include <linux/list.h>
> -#include <spi.h>
> #include <i2c.h>
> +#include <spi.h>
> +#include <linux/list.h>
> #include <power/power_chrg.h>
>
> enum { PMIC_I2C, PMIC_SPI, PMIC_NONE};
> @@ -90,10 +90,10 @@ struct pmic {
> * U-Boot PMIC Framework
> * =====================
> *
> - * UCLASS_PMIC - The is designed to provide an I/O interface for PMIC devices.
> + * UCLASS_PMIC - This is 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 each IC's interface. Then, each child uses its parent for read/write.
> *
> * The driver model tree could look like this:
> *
> @@ -112,8 +112,8 @@ struct pmic {
> * 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,
> - * of proper child devices. Each child usually implements a different function,
> + * We bind a single PMIC device for each interface, to provide an I/O for
> + * its child devices. And each child usually implements a different function,
> * controlled by the same interface.
> *
> * The binding should be done automatically. If device tree nodes/subnodes are
> @@ -131,7 +131,7 @@ struct 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 access the PMIC-specific registers,
> * it need know only the register address and the access can be done through
> * the parent pmic driver. Like in the example:
> *
> @@ -141,10 +141,10 @@ struct 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
> - * 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.
> + * all its child devices, like in the example below. It can be done by calling
> + * the 'pmic_bind_children()' - please refer to the function description, which
> + * can be found in this header file. This function, should be called inside the
> + * driver's bind() method.
> *
> * For the example driver, please refer the MAX77686 driver:
> * - 'drivers/power/pmic/max77686.c'
> @@ -156,18 +156,19 @@ struct pmic {
> * 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
> + * @reg_count: device's 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 (*reg_count)(struct udevice *dev);
> 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 - 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
> @@ -181,7 +182,7 @@ enum pmic_op_type {
> /**
> * 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().
> + * for function: pmic_bind_children().
> *
> * @prefix - child node name prefix (or its name if is unique or single)
> * @driver - driver name for the sub-node with prefix
> @@ -194,7 +195,7 @@ struct pmic_child_info {
> /* drivers/power/pmic-uclass.c */
>
> /**
> - * pmic_bind_childs() - bind drivers for given parent pmic, using child info
> + * pmic_bind_children() - bind drivers for given parent pmic, using child info
> * found in 'child_info' array.
> *
> * @pmic - pmic device - the parent of found child's
> @@ -216,7 +217,7 @@ struct pmic_child_info {
> * (pmic - bind automatically by compatible)
> * compatible = "my_pmic";
> * ...
> - * (pmic's childs - bind by pmic_bind_childs())
> + * (pmic's childs - bind by pmic_bind_children())
> * (nodes prefix: "ldo", driver: "my_regulator_ldo")
> * ldo1 { ... };
> * ldo2 { ... };
> @@ -226,8 +227,8 @@ struct pmic_child_info {
> * buck2 { ... };
> * };
> */
> -int pmic_bind_childs(struct udevice *pmic, int offset,
> - const struct pmic_child_info *child_info);
> +int pmic_bind_children(struct udevice *pmic, int offset,
> + const struct pmic_child_info *child_info);
>
> /**
> * pmic_get: get the pmic device using its name
> --
> 1.9.1
>
More information about the U-Boot
mailing list