From mboxrd@z Thu Jan 1 00:00:00 1970 From: Przemyslaw Marczak Date: Mon, 11 May 2015 09:34:02 +0200 Subject: [U-Boot] [PATCH 01/12] dm: pmic: code cleanup of PMIC uclass driver In-Reply-To: References: <1431102040-745-1-git-send-email-p.marczak@samsung.com> <1431102040-745-2-git-send-email-p.marczak@samsung.com> Message-ID: <55505B6A.8080207@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hello Simon, On 05/10/2015 03:56 PM, Simon Glass wrote: > Hi Przemyslaw, > > On 8 May 2015 at 10:20, Przemyslaw Marczak 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 >> --- >> drivers/power/pmic/pmic-uclass.c | 25 +++++++++---------------- >> include/power/pmic.h | 39 ++++++++++++++++++++------------------- >> 2 files changed, 29 insertions(+), 35 deletions(-) >> > > Acked-by: Simon Glass > Tested on sandbox: > Tested-by: Simon Glass > 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 >> -#include >> #include >> +#include >> +#include >> #include >> >> 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