On 07/19/2013 02:00 PM, Tom Rini wrote:
> From: Greg Guyotte <gguyo...@ti.com>
>
> Add a driver for the TPS65217 PMIC that is found in the Beaglebone
> family of boards.
>
> Signed-off-by: Greg Guyotte <gguyo...@ti.com>
> [trini: Split and rework Greg's changes into new drivers/power
> framework]
> Signed-off-by: Tom Rini <tr...@ti.com>
> ---
>  drivers/power/pmic/Makefile        |    1 +
>  drivers/power/pmic/pmic_tps65217.c |  108 
> ++++++++++++++++++++++++++++++++++++
>  include/power/tps65217.h           |   92 ++++++++++++++++++++++++++++++
>  3 files changed, 201 insertions(+)
>  create mode 100644 drivers/power/pmic/pmic_tps65217.c
>  create mode 100644 include/power/tps65217.h
>
> diff --git a/drivers/power/pmic/Makefile b/drivers/power/pmic/Makefile
> index 14d426f..473cb80 100644
> --- a/drivers/power/pmic/Makefile
> +++ b/drivers/power/pmic/Makefile
> @@ -29,6 +29,7 @@ COBJS-$(CONFIG_POWER_MAX8998) += pmic_max8998.o
>  COBJS-$(CONFIG_POWER_MAX8997) += pmic_max8997.o
>  COBJS-$(CONFIG_POWER_MUIC_MAX8997) += muic_max8997.o
>  COBJS-$(CONFIG_POWER_MAX77686) += pmic_max77686.o
> +COBJS-$(CONFIG_POWER_TPS65217) += pmic_tps65217.o
>  
>  COBJS        := $(COBJS-y)
>  SRCS := $(COBJS:.o=.c)
> diff --git a/drivers/power/pmic/pmic_tps65217.c 
> b/drivers/power/pmic/pmic_tps65217.c
> new file mode 100644
> index 0000000..c84bbcd
> --- /dev/null
> +++ b/drivers/power/pmic/pmic_tps65217.c
> @@ -0,0 +1,108 @@
> +/*
> + * (C) Copyright 2011-2013
Curious if this is the first time in why does it have a 2011 copyright?
> + * Texas Instruments, <www.ti.com>
> + *
> + * 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 <common.h>
> +#include <i2c.h>
> +#include <power/tps65217.h>
> +
> +/**
> + * tps65217_reg_read() - Generic function that can read a TPS65217 register
> + * @src_reg:   Source register address
> + * @src_val:   Address of destination variable
No return defined here in the brief
> + */
> +uchar tps65217_reg_read(uchar src_reg, uchar *src_val)
> +{
> +     if (i2c_read(TPS65217_CHIP_PM, src_reg, 1, src_val, 1))
> +             return 1;
This may be nit picky but generally in error cases we return negative.
Also why not return an error from errno?

Also why an uchar when you are returning an int?

> +     return 0;
> +}
> +
> +/**
> + *  tps65217_reg_write() - Generic function that can write a TPS65217 PMIC
> + *                    register or bit field regardless of protection
> + *                    level.
> + *
> + *  @prot_level:     Register password protection.
> + *                 use PROT_LEVEL_NONE, PROT_LEVEL_1, or PROT_LEVEL_2
> + *  @dest_reg:         Register address to write.
> + *  @dest_val:         Value to write.
> + *  @mask:         Bit mask (8 bits) to be applied.  Function will only
> + *                 change bits that are set in the bit mask.
> + *
> + *  @return:     0 for success, 1 for failure.
> + */
> +int tps65217_reg_write(uchar prot_level, uchar dest_reg, uchar dest_val,
is prot_level a uchar or int?
Also would it not be better to have an interface that will check for mask and 
do the read and
just have a dedicated write function?
> +                    uchar mask)
> +{
> +     uchar read_val;
> +     uchar xor_reg;
> +
> +     /*
> +      * If we are affecting only a bit field, read dest_reg and apply the
> +      * mask
> +      */
> +     if (mask != MASK_ALL_BITS) {
> +             if (i2c_read(TPS65217_CHIP_PM, dest_reg, 1, &read_val, 1))
> +                     return 1;
> +             read_val &= (~mask);
> +             read_val |= (dest_val & mask);
> +             dest_val = read_val;
> +     }
> +
> +     if (prot_level > 0) {
> +             xor_reg = dest_reg ^ PASSWORD_UNLOCK;
> +             if (i2c_write(TPS65217_CHIP_PM, PASSWORD, 1, &xor_reg, 1))
> +                     return 1;
Same comment as above
> +     }
> +
> +     if (i2c_write(TPS65217_CHIP_PM, dest_reg, 1, &dest_val, 1))
> +             return 1;
> +
> +     if (prot_level == PROT_LEVEL_2) {
> +             if (i2c_write(TPS65217_CHIP_PM, PASSWORD, 1, &xor_reg, 1))
> +                     return 1;
> +
> +             if (i2c_write(TPS65217_CHIP_PM, dest_reg, 1, &dest_val, 1))
> +                     return 1;
> +     }
> +
> +     return 0;
> +}
> +
> +int tps65217_voltage_update(uchar dc_cntrl_reg, uchar volt_sel)
No header for the interface
> +{
> +     if ((dc_cntrl_reg != DEFDCDC1) && (dc_cntrl_reg != DEFDCDC2) &&
> +         (dc_cntrl_reg != DEFDCDC3))
What do these magic numbers mean?  Are these HEX numbers or a string?
> +             return 1;
> +
> +     /* set voltage level */
> +     if (tps65217_reg_write(PROT_LEVEL_2, dc_cntrl_reg, volt_sel,
> +                            MASK_ALL_BITS))
> +             return 1;
> +
> +     /* set GO bit to initiate voltage transition */
> +     if (tps65217_reg_write(PROT_LEVEL_2, DEFSLEW, DCDC_GO, DCDC_GO))
> +             return 1;
> +
> +     return 0;
> +}
> diff --git a/include/power/tps65217.h b/include/power/tps65217.h
> new file mode 100644
> index 0000000..c12a709
> --- /dev/null
> +++ b/include/power/tps65217.h
> @@ -0,0 +1,92 @@
> +/*
> + * (C) Copyright 2011-2013
Same copyright comment as above
> + * Texas Instruments, <www.ti.com>
> + *
> + * 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
> + */
> +
> +#ifndef __TPS65217_H__
> +#define __TPS65217_H__
> +
> +/* I2C chip address */
> +#define TPS65217_CHIP_PM             0x24
> +
> +/* Registers */
> +#define CHIPID                               0x00
> +#define POWER_PATH                   0x01
> +#define INTERRUPT                    0x02
> +#define CHGCONFIG0                   0x03
> +#define CHGCONFIG1                   0x04
> +#define CHGCONFIG2                   0x05
> +#define CHGCONFIG3                   0x06
> +#define WLEDCTRL1                    0x07
> +#define WLEDCTRL2                    0x08
> +#define MUXCTRL                              0x09
> +#define STATUS                               0x0A
> +#define PASSWORD                     0x0B
> +#define PGOOD                                0x0C
> +#define DEFPG                                0x0D
> +#define DEFDCDC1                     0x0E
> +#define DEFDCDC2                     0x0F
> +#define DEFDCDC3                     0x10
> +#define DEFSLEW                              0x11
> +#define DEFLDO1                              0x12
> +#define DEFLDO2                              0x13
> +#define DEFLS1                               0x14
> +#define DEFLS2                               0x15
> +#define ENABLE                               0x16
> +#define DEFUVLO                              0x18
> +#define SEQ1                         0x19
> +#define SEQ2                         0x1A
> +#define SEQ3                         0x1B
> +#define SEQ4                         0x1C
> +#define SEQ5                         0x1D
> +#define SEQ6                         0x1E
> +
> +#define PROT_LEVEL_NONE                      0x00
Are these registers or a mask now?
> +#define PROT_LEVEL_1                 0x01
> +#define PROT_LEVEL_2                 0x02
> +
> +#define PASSWORD_LOCK_FOR_WRITE              0x00
> +#define PASSWORD_UNLOCK                      0x7D
> +
> +#define DCDC_GO                              0x80
> +
> +#define MASK_ALL_BITS                        0xFF
> +
> +#define USB_INPUT_CUR_LIMIT_MASK     0x03
> +#define USB_INPUT_CUR_LIMIT_100MA    0x00
> +#define USB_INPUT_CUR_LIMIT_500MA    0x01
> +#define USB_INPUT_CUR_LIMIT_1300MA   0x02
> +#define USB_INPUT_CUR_LIMIT_1800MA   0x03
> +
> +#define DCDC_VOLT_SEL_1275MV         0x0F
> +#define DCDC_VOLT_SEL_1325MV         0x11
> +
> +#define LDO_MASK                     0x1F
> +#define LDO_VOLTAGE_OUT_3_3          0x1F
> +
> +#define PWR_SRC_USB_BITMASK          0x4
> +#define PWR_SRC_AC_BITMASK           0x8
> +
> +uchar tps65217_reg_read(uchar src_reg, uchar *src_val);
> +int tps65217_reg_write(uchar prot_level, uchar dest_reg, uchar dest_val,
> +                    uchar mask);
> +int tps65217_voltage_update(uchar dc_cntrl_reg, uchar volt_sel);
Are these interfaces supposed to be accessed by an outside object?

Typically there should be no direct register access from other objects.
> +#endif


-- 
------------------
Dan Murphy

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

Reply via email to