Patchwork [U-Boot,2/3] fsl_pmic: add I2C interface support

login
register
mail settings
Submitter Liu Hui-R64343
Date Dec. 22, 2010, 1:23 p.m.
Message ID <1293024231-4897-2-git-send-email-r64343@freescale.com>
Download mbox | patch
Permalink /patch/76406/
State Changes Requested
Delegated to: Stefano Babic
Headers show

Comments

Liu Hui-R64343 - Dec. 22, 2010, 1:23 p.m.
This patch add I2C interface for fsl_pmic driver support

Signed-off-by: Jason Liu <r64343@freescale.com>
---
 drivers/misc/fsl_pmic.c |   39 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 38 insertions(+), 1 deletions(-)
Stefano Babic - Dec. 27, 2010, 10:43 a.m.
On 12/22/2010 02:23 PM, Jason Liu wrote:
> This patch add I2C interface for fsl_pmic driver support
> 
> +#ifdef CONFIG_FSL_PMIC_I2C
> +#include <i2c.h>
> +static int init_done;
> +
> +u32 pmic_reg(u32 reg, u32 val, u32 write)
> +{
> +	unsigned char buf[4] = { 0 };
> +	u32 ret_val = 0;
> +
> +	if (reg > 63 || write > 1) {
> +		printf("<reg num> = %d is invalid. Should be less then 63\n",
> +			reg);
> +		return -1;
> +	}

Probably we can factorize better this function and do not duplicate the
pmic_reg function for SPI and I2C.

> +
> +	if (init_done == 0) {
> +		i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
> +		init_done = 1;
> +	}
> +
> +	if (write) {
> +		buf[0] = (val >> 16) & 0xff;
> +		buf[1] = (val >> 8) & 0xff;
> +		buf[2] = (val) & 0xff;

It seems you are converting the endianess. No chance to use an already
provided accessor ?

> +		if (i2c_write(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3))
> +			return -1;
> +	} else {
> +		if (i2c_read(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3)) {
> +			return -1;
> +		ret_val = buf[0] << 16 | buf[1] << 8 | buf[2];

Ditto.

Best regards,
Stefano Babic
Jason Liu - Dec. 28, 2010, 8 a.m.
Hi, Stefano,

2010/12/27 Stefano Babic <sbabic@denx.de>:
> On 12/22/2010 02:23 PM, Jason Liu wrote:
>> This patch add I2C interface for fsl_pmic driver support
>>
>> +#ifdef CONFIG_FSL_PMIC_I2C
>> +#include <i2c.h>
>> +static int init_done;
>> +
>> +u32 pmic_reg(u32 reg, u32 val, u32 write)
>> +{
>> +     unsigned char buf[4] = { 0 };
>> +     u32 ret_val = 0;
>> +
>> +     if (reg > 63 || write > 1) {
>> +             printf("<reg num> = %d is invalid. Should be less then 63\n",
>> +                     reg);
>> +             return -1;
>> +     }
>
> Probably we can factorize better this function and do not duplicate the
> pmic_reg function for SPI and I2C.

OK, I will write one function to check it.

>
>> +
>> +     if (init_done == 0) {
>> +             i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
>> +             init_done = 1;
>> +     }
>> +
>> +     if (write) {
>> +             buf[0] = (val >> 16) & 0xff;
>> +             buf[1] = (val >> 8) & 0xff;
>> +             buf[2] = (val) & 0xff;
>
> It seems you are converting the endianess. No chance to use an already
> provided accessor ?

Good point. I will do it.

>
>> +             if (i2c_write(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3))
>> +                     return -1;
>> +     } else {
>> +             if (i2c_read(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3)) {
>> +                     return -1;
>> +             ret_val = buf[0] << 16 | buf[1] << 8 | buf[2];
>
> Ditto.

OK,

>
> Best regards,
> Stefano Babic
>
> --
> =====================================================================
> 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@denx.de
> =====================================================================
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>
Jason Liu - Dec. 28, 2010, 8:53 a.m.
Hi, Stefano,

2010/12/27 Stefano Babic <sbabic@denx.de>:
> On 12/22/2010 02:23 PM, Jason Liu wrote:
>> This patch add I2C interface for fsl_pmic driver support
>>
>> +#ifdef CONFIG_FSL_PMIC_I2C
>> +#include <i2c.h>
>> +static int init_done;
>> +
>> +u32 pmic_reg(u32 reg, u32 val, u32 write)
>> +{
>> +     unsigned char buf[4] = { 0 };
>> +     u32 ret_val = 0;
>> +
>> +     if (reg > 63 || write > 1) {
>> +             printf("<reg num> = %d is invalid. Should be less then 63\n",
>> +                     reg);
>> +             return -1;
>> +     }
>
> Probably we can factorize better this function and do not duplicate the
> pmic_reg function for SPI and I2C.

I want to make sure what I did is what you want. Do you mean that I need
write one function like:

int check_param(u32 reg, u32 write)
{
      if (reg > 63 || write > 1) {
            printf("<reg num> = %d is invalid. Should be less then 63\n",
                    reg);
             return -1;
      }

     return 0;
}

and put the function call check_param in pmic_reg function to avoid
the code duplication.

If something different with what I though, please correct me. Thanks,

>
>> +
>> +     if (init_done == 0) {
>> +             i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
>> +             init_done = 1;
>> +     }
>> +
>> +     if (write) {
>> +             buf[0] = (val >> 16) & 0xff;
>> +             buf[1] = (val >> 8) & 0xff;
>> +             buf[2] = (val) & 0xff;
>
> It seems you are converting the endianess. No chance to use an already
> provided accessor ?
>
>> +             if (i2c_write(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3))
>> +                     return -1;
>> +     } else {
>> +             if (i2c_read(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3)) {
>> +                     return -1;
>> +             ret_val = buf[0] << 16 | buf[1] << 8 | buf[2];
>
> Ditto.
>
> Best regards,
> Stefano Babic
>
> --
> =====================================================================
> 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@denx.de
> =====================================================================
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

Patch

diff --git a/drivers/misc/fsl_pmic.c b/drivers/misc/fsl_pmic.c
index 5ee1de1..a54f220 100644
--- a/drivers/misc/fsl_pmic.c
+++ b/drivers/misc/fsl_pmic.c
@@ -22,11 +22,47 @@ 
 
 #include <config.h>
 #include <common.h>
-#include <spi.h>
 #include <asm/errno.h>
 #include <linux/types.h>
 #include <fsl_pmic.h>
 
+#ifdef CONFIG_FSL_PMIC_I2C
+#include <i2c.h>
+static int init_done;
+
+u32 pmic_reg(u32 reg, u32 val, u32 write)
+{
+	unsigned char buf[4] = { 0 };
+	u32 ret_val = 0;
+
+	if (reg > 63 || write > 1) {
+		printf("<reg num> = %d is invalid. Should be less then 63\n",
+			reg);
+		return -1;
+	}
+
+	if (init_done == 0) {
+		i2c_init(CONFIG_SYS_I2C_SPEED, CONFIG_SYS_I2C_SLAVE);
+		init_done = 1;
+	}
+
+	if (write) {
+		buf[0] = (val >> 16) & 0xff;
+		buf[1] = (val >> 8) & 0xff;
+		buf[2] = (val) & 0xff;
+		if (i2c_write(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3))
+			return -1;
+	} else {
+		if (i2c_read(CONFIG_SYS_FSL_PMIC_I2C_ADDR, reg, 1, buf, 3)) {
+			return -1;
+		ret_val = buf[0] << 16 | buf[1] << 8 | buf[2];
+		}
+	}
+
+	return ret_val;
+}
+#else /* SPI interface */
+#include <spi.h>
 static struct spi_slave *slave;
 
 struct spi_slave *pmic_spi_probe(void)
@@ -87,6 +123,7 @@  u32 pmic_reg(u32 reg, u32 val, u32 write)
 	spi_release_bus(slave);
 	return cpu_to_be32(pmic_rx);
 }
+#endif
 
 void pmic_reg_write(u32 reg, u32 value)
 {