[U-Boot] [PATCH 1/2] pmic:max77686: add function to set voltage and mode
Piotr Wilczek
p.wilczek at samsung.com
Mon Mar 25 13:53:47 CET 2013
Hi Rajeshwari Shinde,
Thanks for your comments.
Please see my answers below.
> -----Original Message-----
> From: Rajeshwari Birje [mailto:rajeshwari.birje at gmail.com]
> Sent: Monday, March 25, 2013 11:24 AM
> To: Piotr Wilczek
> Cc: u-boot at lists.denx.de; Kyungmin Park; Rajeshwari Shinde
> Subject: Re: [U-Boot] [PATCH 1/2] pmic:max77686: add function to set
> voltage and mode
>
> Hi Piotr Wilczek,
>
> Nice idea to have a generic function.
> Just have some minor comments.
>
> On Mon, Mar 25, 2013 at 3:28 PM, Piotr Wilczek <p.wilczek at samsung.com>
> wrote:
> > This patch add new functions to pmic max77686 to set voltage and
> mode.
> >
> > Signed-off-by: Piotr Wilczek <p.wilczek at samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> > CC: Minkyu Kang <mk7.kang at samsung.com>
> > CC: Rajeshwari Shinde <rajeshwari.s at samsung.com>
> > ---
> > 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..3895bc5 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 };
> 0xff is for?
> I guess this is a array of Buck control 1 registers so we can be
> specific in naming the same instead of just buck address.
> Any way we have address for them defined as a enum in
> include/power/max77686_pmic.h cant we use the same?
This is an array of address of every Buck control 1 register.
When the user specifies which Buck number he wants to change, the driver
figures out the address from the array.
If the address is given directly it is still necessary to get the correct
mode hex value depending on the address.
> > +
> > +static unsigned int max77686_ldo_volt2hex(int ldo, ulong uV) {
> > + unsigned int hex = 0;
> > + const unsigned int MAX_HEX = 0x3f;
> Any specific reason for this variable to be in capital..
Only to stress it is a const, I will change 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)
> > + return hex;
> > +
> > + printf("%s: %ld is wrong voltage value for LDO%d\n",
> __func__,
> > + uV, ldo);
> Can we use debug instead of printf through out.
When code execution reach here, wrong voltage value is given.
With printf it will be immediately known that there is something wrong, but
I can change it to debug.
> > + 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) {
> > + 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);
> > + 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;
> Cant we get the ldo addresses directly as they are available in the
> include file.
> Then top check would also go off.
Well, I decided that is better that user specifies the ldo (or buck) number
and the driver figures out the address to write and also the hex value for
the mode or voltage. What do you think?
Please note that in every case it is necessary to get ldo number from the
address (or directly check the address) to get the correct hex value to
write to register.
The top check is not really necessary, but for a very little cost of
performance it makes the code safer.
> > +
> > + /* mask */
> > + mask = 0xc0;
> > +
> > + /* 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;
> Cant we remove this hard coding.
Ok.
> > + } else {
> > + mode = 0xff;
> > + }
> > +
> > + 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);
> > + 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);
> > + 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
> > d949ace..e1874ec 100644
> > --- a/include/power/max77686_pmic.h
> > +++ b/include/power/max77686_pmic.h
> > @@ -155,4 +155,15 @@ 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);
> > +
> > #endif /* __MAX77686_PMIC_H_ */
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > http://lists.denx.de/mailman/listinfo/u-boot
>
> --
> Regards,
> Rajeshwari Shinde
Best regards,
Piotr Wilczek
--
Samsung R&D Poland (SRPOL) | Linux Platform Group
More information about the U-Boot
mailing list