[U-Boot] [PATCH V2] drivers:power:max77686: add function to set voltage and mode

[Piotr Wilczek <p.wilczek@samsung.com>]

Dear Minkyu Kang,

 

Thank you for comments. I agree with all of them.

 

Best regards,

Piotr Wilczek

 

From: Minkyu Kang [mailto:promsoft at gmail.com] 
Sent: Friday, June 21, 2013 6:24 PM
To: Piotr Wilczek
Cc: u-boot at lists.denx.de; Kyungmin Park; Rajeshwari Shinde
Subject: Re: [U-Boot] [PATCH V2] drivers:power:max77686: add function to set
voltage and mode

 

Dear Piotr Wilczek,

 

On 21 May 2013 21:54, Piotr Wilczek <p.wilczek@samsung.com> wrote:

This patch add new functions to pmic max77686 to set voltage and mode.

Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
CC: Minkyu Kang <mk7.kang@samsung.com>
CC: Rajeshwari Shinde <rajeshwari.s@samsung.com>

Acked-by: Rajeshwari Shinde <rajeshwari.s@samsung.com>
---
Changes in v2:
- changed printf to debug

 drivers/power/pmic/pmic_max77686.c |  186
++++++++++++++++++++++++++++++++++++
 include/power/max77686_pmic.h      |   11 +++
 2 files changed, 197 insertions(+)

diff --git a/drivers/power/pmic/pmic_max77686.c
b/drivers/power/pmic/pmic_max77686.c
index 7fcb4c0..dabd6b6 100644
--- a/drivers/power/pmic/pmic_max77686.c
+++ b/drivers/power/pmic/pmic_max77686.c
@@ -30,6 +30,192 @@

 DECLARE_GLOBAL_DATA_PTR;

+static const char max77686_buck_addr[] = {
+       0xff, 0x10, 0x12, 0x1c, 0x26, 0x30, 0x32, 0x34, 0x36, 0x38
+};
+
+static unsigned int max77686_ldo_volt2hex(int ldo, ulong uV)
+{
+       unsigned int hex = 0;
+       const unsigned int max_hex = 0x3f;

 

Is it constant? then please define it.

 

+
+       switch (ldo) {
+       case 1:
+       case 2:
+       case 6:
+       case 7:
+       case 8:
+       case 15:
+               hex = (uV - 800000) / 25000;
+               break;
+       default:
+               hex = (uV - 800000) / 50000;
+       }
+
+       if (0 <= hex && hex <= max_hex)

 

I think, hex >= 0 looks more comfortable.

 

+               return hex;
+
+       debug("%s: %ld is wrong voltage value for LDO%d\n", __func__, uV,
ldo);
+       return 0;
+}
+
+int max77686_set_ldo_voltage(struct pmic *p, int ldo, ulong uV)
+{
+       unsigned int val, ret, hex, adr, mask;
+
+       if (ldo < 1 && 26 < ldo) {

 

ditto.

 

+               printf("%s: %d is wrong ldo number\n", __func__, ldo);
+               return -1;
+       }
+
+       adr = MAX77686_REG_PMIC_LDO1CTRL1 + ldo - 1;
+       mask = 0x3f;
+       hex = max77686_ldo_volt2hex(ldo, uV);
+
+       if (!hex)
+               return -1;
+
+       ret = pmic_reg_read(p, adr, &val);

 

I think.. if you got error while read the register then please return error
without writing.

 

+       val &= ~mask;
+       val |= hex;
+       ret |= pmic_reg_write(p, adr, val);

+
+       return ret;
+}
+
+int max77686_set_ldo_mode(struct pmic *p, int ldo, char opmode)
+{
+       unsigned int val, ret, mask, adr, mode;
+
+       if (ldo < 1 && 26 < ldo) {
+               printf("%s: %d is wrong ldo number\n", __func__, ldo);
+               return -1;
+       }
+
+       adr = MAX77686_REG_PMIC_LDO1CTRL1 + ldo - 1;
+
+       /* mask */
+       mask = 0xc0;

 

Is it constant? then please define it.

 

+
+       /* mode */
+       if (opmode == OPMODE_OFF) {
+               mode = 0x00;
+       } else if (opmode == OPMODE_STANDBY) {
+               switch (ldo) {
+               case 2:
+               case 6:
+               case 7:
+               case 8:
+               case 10:
+               case 11:
+               case 12:
+               case 14:
+               case 15:
+               case 16:
+                       mode = 0x40; 

+                       break;
+               default:
+                       mode = 0xff;
+               }
+       } else if (opmode == OPMODE_LPM) {
+               mode = 0x80;
+       } else if (opmode == OPMODE_ON) {
+               mode = 0xc0;
+       } else {
+               mode = 0xff;
+       }

 

switch case?

What means 0x40 and 0x80, 0xc0?

 

There are so many magic values in this patch.

I'll not mentioned about it anymore.

Please fix it globally.

 

+
+       if (mode == 0xff) {
+               printf("%s: %d is not supported on LDO%d\n",
+                      __func__, opmode, ldo);
+               return -1;
+       }
+
+       ret = pmic_reg_read(p, adr, &val);

 

ditto.

 

+       val &= ~mask;
+       val |= mode;
+       ret |= pmic_reg_write(p, adr, val);
+
+       return ret;
+}
+
+int max77686_set_buck_mode(struct pmic *p, int buck, char opmode)
+{
+       unsigned int val, ret, mask, adr, size, mode;
+
+       size = sizeof(max77686_buck_addr) / sizeof(*max77686_buck_addr);

 

ARRAY_SIZE()?

 

+       if (buck >= size) {
+               printf("%s: %d is wrong buck number\n", __func__, buck);
+               return -1;
+       }
+
+       adr = max77686_buck_addr[buck];
+
+       /* mask */
+       switch (buck) {
+       case 2:
+       case 3:
+       case 4:
+               mask = 0x30;
+               break;
+       default:
+               mask = 0x03;
+       }
+
+       /* mode */
+       if (opmode == OPMODE_OFF) {
+               mode = 0x00;
+       } else if (opmode == OPMODE_STANDBY) {
+               switch (buck) {
+               case 1:
+                       mode = 0x01;
+                       break;
+               case 2:
+               case 3:
+               case 4:
+                       mode = 0x10;
+                       break;
+               default:
+                       mode = 0xff;
+               }
+       } else if (opmode == OPMODE_LPM) {
+               switch (buck) {
+               case 2:
+               case 3:
+               case 4:
+                       mode = 0x20;
+                       break;
+               default:
+                       mode = 0xff;
+               }
+       } else if (opmode == OPMODE_ON) {
+               switch (buck) {
+               case 2:
+               case 3:
+               case 4:
+                       mode = 0x30;
+                       break;
+               default:
+                       mode = 0x03;
+               }
+       } else {
+               mode = 0xff;
+       }
+
+       if (mode == 0xff) {
+               printf("%s: %d is not supported on BUCK%d\n",
+                      __func__, opmode, buck);
+               return -1;
+       }
+
+       ret = pmic_reg_read(p, adr, &val);
+       val &= ~mask;
+       val |= mode;
+       ret |= pmic_reg_write(p, adr, val);
+
+       return ret;
+}
+
 int pmic_init(unsigned char bus)
 {
        static const char name[] = "MAX77686_PMIC";
diff --git a/include/power/max77686_pmic.h b/include/power/max77686_pmic.h
index fdc7ca9..ce58c02 100644
--- a/include/power/max77686_pmic.h
+++ b/include/power/max77686_pmic.h
@@ -155,6 +155,17 @@ enum {
        EN_LDO = (0x3 << 6),
 };

+enum {
+       OPMODE_OFF = 0,
+       OPMODE_STANDBY,
+       OPMODE_LPM,
+       OPMODE_ON,
+};
+
+int max77686_set_ldo_voltage(struct pmic *p, int ldo, ulong uV);
+int max77686_set_ldo_mode(struct pmic *p, int ldo, char opmode);
+int max77686_set_buck_mode(struct pmic *p, int buck, char opmode);
+
 /* Buck1 1 volt value */
 #define MAX77686_BUCK1OUT_1V   0x5
 #define MAX77686_BUCK1CTRL_EN  (3 << 0)
--
1.7.9.5

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Thanks,

Minkyu Kang.
-- 
from. prom.
www.promsoft.net