On 05/11/2011 10:03 AM, Jason Liu wrote:
> This patch add dialog pmic(DA9053) driver with I2C interface support
> In order to not duplicate code and according to the discussion on the
> mail-list, change fsl_pmic.c to spi_i2c_pmic.c.Actaully,spi_i2c_pmic.c
> is just a wrapper for PMIC communication when SPI or I2C is used.
>
> Signed-off-by: Jason Liu <jason....@linaro.org>
> Cc: sba...@denx.de <sba...@denx.de>
> Cc: Detlev Zundel <d...@denx.de>
Hi Jason,
> --- a/drivers/misc/fsl_pmic.c
> +++ b/drivers/misc/spi_i2c_pmic.c
> @@ -22,13 +22,16 @@
>
> #include <config.h>
> #include <common.h>
> +#include <i2c.h>
> #include <asm/errno.h>
> #include <linux/types.h>
> +#if defined(CONFIG_FSL_PMIC)
> #include <fsl_pmic.h>
> +#endif
>
> -static int check_param(u32 reg, u32 write)
> +static int check_param(u32 reg, u32 write, u32 max_reg)
> {
> - if (reg > 63 || write > 1) {
> + if (reg > max_reg || write > 1) {
> printf("<reg num> = %d is invalid. Should be less then 63\n",
> reg);
> return -1;
> @@ -37,15 +40,13 @@ static int check_param(u32 reg, u32 write)
> return 0;
> }
>
> -#ifdef CONFIG_FSL_PMIC_I2C
> -#include <i2c.h>
> -
> -u32 pmic_reg(u32 reg, u32 val, u32 write)
> +#if defined(CONFIG_FSL_PMIC_I2C)
> +u32 fsl_pmic_reg(u32 reg, u32 val, u32 write)
> {
> - unsigned char buf[4] = { 0 };
> u32 ret_val = 0;
> + unsigned char buf[4] = { 0 };
>
> - if (check_param(reg, write))
> + if (check_param(reg, write, 63))
> return -1;
>
> if (write) {
> @@ -62,7 +63,44 @@ u32 pmic_reg(u32 reg, u32 val, u32 write)
>
> return ret_val;
> }
> -#else /* SPI interface */
> +#endif
> +
> +#if defined(CONFIG_DIALOG_PMIC_I2C)
> +u32 dlg_pmic_reg(u32 reg, u32 val, u32 write)
> +{
> + u32 ret_val = 0;
> + unsigned char buf[1] = { 0 };
> +
> + if (check_param(reg, write, 142))
> + return -1;
> +
> + if (write) {
> + buf[0] = (val) & 0xff;
> + if (i2c_write(CONFIG_SYS_DIALOG_PMIC_I2C_ADDR, reg, 1, buf, 1))
> + return -1;
> + } else {
> + if (i2c_read(CONFIG_SYS_DIALOG_PMIC_I2C_ADDR, reg, 1, buf, 1))
> + return -1;
> + ret_val = buf[0];
> + }
> +
> + return ret_val;
This is not what I meant. You have duplicated the code, instead of merge
the two functions together. And I think the switch is wrong.
This file provides a general access to PMIc using SPI/I2C. There should
not be #ifdef related to a single PMIC. Instead of that, You need
additional CONFIG_ to select how to access the PMIC (8 bit or 32 bit).
IMHO we can rid of the check_param() function, as this is a constraint
to have an implementation independent from a single PMIC.
I would prefer something like this:
u32 pmic_reg(u32 reg, u32 val, u32 write) {
........
#ifdef CONFIG_SYS_PMIC_8BIT
<read / write 8 bit register>
#else
<actual implementation for fsl PMICs>
#endif
> +#if defined(CONFIG_FSL_PMIC_I2C) || defined(CONFIG_DIALOG_PMIC_I2C)
Same comments apply here. We should select only between I2C and SPI, not
the chip.
> +#if defined(CONFIG_FSL_PMIC)
> +#define PMIC_NAME "Freescale PMIC (Atlas)"
> +#elif defined(CONFIG_DIALOG_PMIC)
> +#define PMIC_NAME "Dialog PMIC (DA905x)"
> +#else
> +#error "Unkown PMIC name"
> +#endif
Instead of that, we can set a general name or put the PMIC_NAME inside
the specific PMIC header file.
Best regards,
Stefano
--
=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de
=====================================================================
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
