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

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello Simon,

On 05/10/2015 03:56 PM, Simon Glass wrote:
> 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>
>

Thank you for review and testing!

> 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.
>
>

Ah, that's right, I will fix that.

>> 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
>>
>

Best regards,
-- 
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com