From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefano Babic <sbabic@denx.de>
Date: Thu, 12 May 2011 11:19:20 +0200
Subject: [U-Boot] [PATCH V7 2/3] PMIC: Add dialog pmic support
In-Reply-To: <1305101022-22546-2-git-send-email-jason.hui@linaro.org>
References: <1305101022-22546-1-git-send-email-jason.hui@linaro.org>
	<1305101022-22546-2-git-send-email-jason.hui@linaro.org>
Message-ID: <4DCBA618.2060608@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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.hui@linaro.org>
> Cc: sbabic at denx.de <sbabic@denx.de>
> Cc: Detlev Zundel <dzu@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: office at denx.de
=====================================================================