Re: [U-Boot] [PATCH 01/12] dm: pmic: code cleanup of PMIC uclass driver

Hi Przemyslaw,

On 8 May 2015 at 10:20, Przemyslaw Marczak p.marczak@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@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@chromium.org Tested on sandbox: Tested-by: Simon Glass sjg@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