diff mbox

[U-Boot,13/13] misc: pmic: drop old Freescale's pmic driver

Message ID 1318091769-30979-14-git-send-email-sbabic@denx.de
State Awaiting Upstream
Headers show

Commit Message

Stefano Babic Oct. 8, 2011, 4:36 p.m. UTC 
Signed-off-by: Stefano Babic <sbabic@denx.de>
---
 drivers/misc/Makefile   |    1 -
 drivers/misc/fsl_pmic.c |  235 -----------------------------------------------
 2 files changed, 0 insertions(+), 236 deletions(-)
 delete mode 100644 drivers/misc/fsl_pmic.c

Comments

Helmut Raiger Oct. 19, 2011, 1:23 p.m. UTC | #1 
On 10/08/2011 06:36 PM, Stefano Babic wrote:
> Signed-off-by: Stefano Babic<sbabic@denx.de>
> ---
>   drivers/misc/Makefile   |    1 -
>   drivers/misc/fsl_pmic.c |  235 -----------------------------------------------
>   2 files changed, 0 insertions(+), 236 deletions(-)
>   delete mode 100644 drivers/misc/fsl_pmic.c
>
I just checked PMIC action on our board (i.mx31 and mc13783) and the new 
code is not working here, it even:

TT01> pmic write 20 17
raise: Signal # 8 caught
<reg num> = 32 is invalid. Should be less than 0
TT01> pmic read 20
<reg num> = 32 is invalid. Should be less than 0
PMIC: Register read failed

0x20: 0x00000000

At first glance I found in pmic_fsl.c:

static u32 pmic_spi_prepare_tx(u32 reg, u32 *val, u32 write)
{
     if ((val == NULL) && (write))
         return *val & ~(1 << 31);
     else
         return (write << 31) | (reg << 25) | (*val & 0x00FFFFFF);
}

which must be wrong. NULL is de-referenced in both cases and this error
is even forced by pmic_spi.c:

     if (write) {
         pmic_tx = p->hw.spi.prepare_tx(0, NULL, write);
         pmic_tx &= ~(1 << 31);


Probably val == NULL was meant as escape not to touch the pmic_tx value,
in the original driver it's done that way.
One could fix this by using a static variable in pmic_spi_prepare_tx(), 
but I'm
not sure if this was the intention.

I wonder why it was missed during testing as it seems configuration 
independent.
Helmut



--
Scanned by MailScanner.
Stefano Babic Oct. 19, 2011, 3:39 p.m. UTC | #2 
On 10/19/2011 03:23 PM, Helmut Raiger wrote:
> On 10/08/2011 06:36 PM, Stefano Babic wrote:
>> Signed-off-by: Stefano Babic<sbabic@denx.de>
>> ---
>>   drivers/misc/Makefile   |    1 -
>>   drivers/misc/fsl_pmic.c |  235
>> -----------------------------------------------
>>   2 files changed, 0 insertions(+), 236 deletions(-)
>>   delete mode 100644 drivers/misc/fsl_pmic.c
>>

Hi Helmut,

> I just checked PMIC action on our board (i.mx31 and mc13783) and the new
> code is not working here, it even:
> 
> TT01> pmic write 20 17
> raise: Signal # 8 caught
> <reg num> = 32 is invalid. Should be less than 0
> TT01> pmic read 20
> <reg num> = 32 is invalid. Should be less than 0
> PMIC: Register read failed
> 
> 0x20: 0x00000000
> 
> At first glance I found in pmic_fsl.c:
> 
> static u32 pmic_spi_prepare_tx(u32 reg, u32 *val, u32 write)
> {
>     if ((val == NULL) && (write))
>         return *val & ~(1 << 31);
>     else
>         return (write << 31) | (reg << 25) | (*val & 0x00FFFFFF);
> }
> 
> which must be wrong. NULL is de-referenced in both cases and this error
> is even forced by pmic_spi.c:

This is wrong - it is checked for NULL and then de-referenced...

> 
>     if (write) {
>         pmic_tx = p->hw.spi.prepare_tx(0, NULL, write);
>         pmic_tx &= ~(1 << 31);
> 
> 
> Probably val == NULL was meant as escape not to touch the pmic_tx value,
> in the original driver it's done that way.

Then there is too much statements. If spi.prepare_tx() should clear the
MSB with ~(1 << 31), why we need the second one :

pmic_tx &= ~(1 << 31);

something went simply wrong...

Because (from the old driver) the second SPI transfer is done to read
back the programmed value, we can maybe use :

     if (write) {
         pmic_tx = p->hw.spi.prepare_tx(reg, &val, 0);

and of course, we should fix the wrong prepare_tx()..

> One could fix this by using a static variable in pmic_spi_prepare_tx(),
> but I'm
> not sure if this was the intention.
> 

Well, it makes no sense - the prepare_tx() should not know the history,
and only rely on the parameters.

> I wonder why it was missed during testing as it seems configuration
> independent.

I wonder, too. I have tested with the 'date' command, that means writing
into the internal RTC registers of the MC13783. I cannot explain what
goes wrong, but of course the code is buggy and must be fixed.

Best regards,
Stefano Babic
diff mbox

Patch

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index f732a95..a709707 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -28,7 +28,6 @@  LIB	:= $(obj)libmisc.o
 COBJS-$(CONFIG_ALI152X) += ali512x.o
 COBJS-$(CONFIG_DS4510)  += ds4510.o
 COBJS-$(CONFIG_FSL_LAW) += fsl_law.o
-COBJS-$(CONFIG_FSL_PMIC) += fsl_pmic.o
 COBJS-$(CONFIG_GPIO_LED) += gpio_led.o
 COBJS-$(CONFIG_FSL_MC9SDZ60) += mc9sdz60.o
 COBJS-$(CONFIG_NS87308) += ns87308.o
diff --git a/drivers/misc/fsl_pmic.c b/drivers/misc/fsl_pmic.c
deleted file mode 100644
index 23255a5..0000000
--- a/drivers/misc/fsl_pmic.c
+++ /dev/null
@@ -1,235 +0,0 @@ 
-/*
- * (C) Copyright 2008-2009 Freescale Semiconductor, Inc.
- *
- * See file CREDITS for list of people who contributed to this
- * project.
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation; either version 2 of
- * the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
- * MA 02111-1307 USA
- */
-
-#include <config.h>
-#include <common.h>
-#include <asm/errno.h>
-#include <linux/types.h>
-#include <fsl_pmic.h>
-
-static 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;
-}
-
-#ifdef CONFIG_FSL_PMIC_I2C
-#include <i2c.h>
-
-u32 pmic_reg(u32 reg, u32 val, u32 write)
-{
-	unsigned char buf[4] = { 0 };
-	u32 ret_val = 0;
-
-	if (check_param(reg, write))
-		return -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)
-{
-	return spi_setup_slave(CONFIG_FSL_PMIC_BUS,
-		CONFIG_FSL_PMIC_CS,
-		CONFIG_FSL_PMIC_CLK,
-		CONFIG_FSL_PMIC_MODE);
-}
-
-void pmic_spi_free(struct spi_slave *slave)
-{
-	if (slave)
-		spi_free_slave(slave);
-}
-
-u32 pmic_reg(u32 reg, u32 val, u32 write)
-{
-	u32 pmic_tx, pmic_rx;
-	u32 tmp;
-
-	if (!slave) {
-		slave = pmic_spi_probe();
-
-		if (!slave)
-			return -1;
-	}
-
-	if (check_param(reg, write))
-		return -1;
-
-	if (spi_claim_bus(slave))
-		return -1;
-
-	pmic_tx = (write << 31) | (reg << 25) | (val & 0x00FFFFFF);
-
-	tmp = cpu_to_be32(pmic_tx);
-
-	if (spi_xfer(slave, 4 << 3, &tmp, &pmic_rx,
-			SPI_XFER_BEGIN | SPI_XFER_END)) {
-		spi_release_bus(slave);
-		return -1;
-	}
-
-	if (write) {
-		pmic_tx &= ~(1 << 31);
-		tmp = cpu_to_be32(pmic_tx);
-		if (spi_xfer(slave, 4 << 3, &tmp, &pmic_rx,
-			SPI_XFER_BEGIN | SPI_XFER_END)) {
-			spi_release_bus(slave);
-			return -1;
-		}
-	}
-
-	spi_release_bus(slave);
-	return cpu_to_be32(pmic_rx);
-}
-#endif
-
-void pmic_reg_write(u32 reg, u32 value)
-{
-	pmic_reg(reg, value, 1);
-}
-
-u32 pmic_reg_read(u32 reg)
-{
-	return pmic_reg(reg, 0, 0);
-}
-
-void pmic_show_pmic_info(void)
-{
-	u32 rev_id;
-
-	rev_id = pmic_reg_read(REG_IDENTIFICATION);
-	printf("PMIC ID: 0x%08x [Rev: ", rev_id);
-	switch (rev_id & 0x1F) {
-	case 0x1:
-		puts("1.0");
-		break;
-	case 0x9:
-		puts("1.1");
-		break;
-	case 0xA:
-		puts("1.2");
-		break;
-	case 0x10:
-		puts("2.0");
-		break;
-	case 0x11:
-		puts("2.1");
-		break;
-	case 0x18:
-		puts("3.0");
-		break;
-	case 0x19:
-		puts("3.1");
-		break;
-	case 0x1A:
-		puts("3.2");
-		break;
-	case 0x2:
-		puts("3.2A");
-		break;
-	case 0x1B:
-		puts("3.3");
-		break;
-	case 0x1D:
-		puts("3.5");
-		break;
-	default:
-		puts("unknown");
-		break;
-	}
-	puts("]\n");
-}
-
-static void pmic_dump(int numregs)
-{
-	u32 val;
-	int i;
-
-	pmic_show_pmic_info();
-	for (i = 0; i < numregs; i++) {
-		val = pmic_reg_read(i);
-		if (!(i % 8))
-			printf ("\n0x%02x: ", i);
-		printf("%08x ", val);
-	}
-	puts("\n");
-}
-
-int do_pmic(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
-{
-	char *cmd;
-	int nregs;
-	u32 val;
-
-	/* at least two arguments please */
-	if (argc < 2)
-		return cmd_usage(cmdtp);
-
-	cmd = argv[1];
-	if (strcmp(cmd, "dump") == 0) {
-		if (argc < 3)
-			return cmd_usage(cmdtp);
-
-		nregs = simple_strtoul(argv[2], NULL, 16);
-		pmic_dump(nregs);
-		return 0;
-	}
-	if (strcmp(cmd, "write") == 0) {
-		if (argc < 4)
-			return cmd_usage(cmdtp);
-
-		nregs = simple_strtoul(argv[2], NULL, 16);
-		val = simple_strtoul(argv[3], NULL, 16);
-		pmic_reg_write(nregs, val);
-		return 0;
-	}
-	/* No subcommand found */
-	return 1;
-}
-
-U_BOOT_CMD(
-	pmic,	CONFIG_SYS_MAXARGS, 1, do_pmic,
-	"Freescale PMIC (Atlas)",
-	"dump [numregs] - dump registers\n"
-	"pmic write <reg> <value> - write register"
-);