diff mbox series

[U-Boot,v2,04/11] pmic: dm: Rewrite pmic_reg_{read|write|clrsetbits} to support 3 bytes transmissions

Message ID 20180506202608.5899-5-lukma@denx.de
State Superseded
Delegated to: Lukasz Majewski
Headers show
Series pmic: sandbox: Add support for MC34709 PMIC | expand

Commit Message

Lukasz Majewski May 6, 2018, 8:26 p.m. UTC 
This commit provides support for transmissions larger than 1 byte for
PMIC devices used with DM (e.g. MC34708 from NXP).

Signed-off-by: Lukasz Majewski <lukma@denx.de>

---

Changes in v2:
- pmic_reg_* fixes to use uclass private structure

 drivers/power/pmic/pmic-uclass.c | 44 +++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 12 deletions(-)

Comments

Simon Glass May 13, 2018, 10:01 p.m. UTC | #1 
Hi Lukasz,

On 7 May 2018 at 06:26, Lukasz Majewski <lukma@denx.de> wrote:
> This commit provides support for transmissions larger than 1 byte for
> PMIC devices used with DM (e.g. MC34708 from NXP).
>
> Signed-off-by: Lukasz Majewski <lukma@denx.de>
>
> ---
>
> Changes in v2:
> - pmic_reg_* fixes to use uclass private structure
>
>  drivers/power/pmic/pmic-uclass.c | 44 +++++++++++++++++++++++++++++-----------
>  1 file changed, 32 insertions(+), 12 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

with comments below

> diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c
> index 88669533bd..c149b84a68 100644
> --- a/drivers/power/pmic/pmic-uclass.c
> +++ b/drivers/power/pmic/pmic-uclass.c
> @@ -131,23 +131,36 @@ int pmic_write(struct udevice *dev, uint reg, const uint8_t *buffer, int len)
>
>  int pmic_reg_read(struct udevice *dev, uint reg)
>  {
> -       u8 byte;
> +       struct dm_pmic_info *pmic_info = dev_get_uclass_priv(dev);
> +       int tx_num = pmic_info->trans_len;

Can you use trans_len instead of tx_num, which is confusing?

> +       u32 val = 0;
>         int ret;
>
> -       debug("%s: reg=%x", __func__, reg);
> -       ret = pmic_read(dev, reg, &byte, 1);
> -       debug(", value=%x, ret=%d\n", byte, ret);
> +       if (tx_num < 1 || tx_num > sizeof(val)) {
> +               printf("Wrong transmission size [%d]\n", tx_num);

debug()

> +               return -EINVAL;
> +       }
> +
> +       debug("%s: reg=%x tx_num:%d", __func__, reg, tx_num);
> +       ret = pmic_read(dev, reg, (uint8_t *)&val, tx_num);

This is assuming little-endian, isn't it? I think you need to build up
the number in a loop 8 bits at a time, or use some sort of unaligned
function.

> +       debug(", value=%x, ret=%d\n", val, ret);
>
> -       return ret ? ret : byte;
> +       return ret ? ret : val;
>  }
>
>  int pmic_reg_write(struct udevice *dev, uint reg, uint value)
>  {
> -       u8 byte = value;
> +       struct dm_pmic_info *pmic_info = dev_get_uclass_priv(dev);
> +       int tx_num = pmic_info->trans_len;
>         int ret;
>
> -       debug("%s: reg=%x, value=%x", __func__, reg, value);
> -       ret = pmic_write(dev, reg, &byte, 1);
> +       if (tx_num < 1 || tx_num > sizeof(value)) {
> +               printf("Wrong transmission size [%d]\n", tx_num);
> +               return -EINVAL;
> +       }
> +
> +       debug("%s: reg=%x, value=%x tx_num:%d", __func__, reg, value, tx_num);
> +       ret = pmic_write(dev, reg, (uint8_t *)&value, tx_num);
>         debug(", ret=%d\n", ret);
>
>         return ret;
> @@ -155,15 +168,22 @@ int pmic_reg_write(struct udevice *dev, uint reg, uint value)
>
>  int pmic_clrsetbits(struct udevice *dev, uint reg, uint clr, uint set)
>  {
> -       u8 byte;
> +       struct dm_pmic_info *pmic_info = dev_get_uclass_priv(dev);
> +       int tx_num = pmic_info->trans_len;
> +       u32 val = 0;
>         int ret;
>
> -       ret = pmic_reg_read(dev, reg);
> +       if (tx_num < 1 || tx_num > sizeof(val)) {
> +               printf("Wrong transmission size [%d]\n", tx_num);
> +               return -EINVAL;
> +       }
> +
> +       ret = pmic_read(dev, reg, (uint8_t *)&val, tx_num);
>         if (ret < 0)
>                 return ret;
> -       byte = (ret & ~clr) | set;
>
> -       return pmic_reg_write(dev, reg, byte);
> +       val = (val & ~clr) | set;
> +       return pmic_write(dev, reg, (uint8_t *)&val, tx_num);
>  }
>
>  static int pmic_pre_probe(struct udevice *dev)
> --
> 2.11.0
>

Regards,
Simon
Lukasz Majewski May 14, 2018, 9:47 a.m. UTC | #2 
Hi Simon,

> Hi Lukasz,
> 
> On 7 May 2018 at 06:26, Lukasz Majewski <lukma@denx.de> wrote:
> > This commit provides support for transmissions larger than 1 byte
> > for PMIC devices used with DM (e.g. MC34708 from NXP).
> >
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> >
> > ---
> >
> > Changes in v2:
> > - pmic_reg_* fixes to use uclass private structure
> >
> >  drivers/power/pmic/pmic-uclass.c | 44
> > +++++++++++++++++++++++++++++----------- 1 file changed, 32
> > insertions(+), 12 deletions(-) 
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> with comments below
> 
> > diff --git a/drivers/power/pmic/pmic-uclass.c
> > b/drivers/power/pmic/pmic-uclass.c index 88669533bd..c149b84a68
> > 100644 --- a/drivers/power/pmic/pmic-uclass.c
> > +++ b/drivers/power/pmic/pmic-uclass.c
> > @@ -131,23 +131,36 @@ int pmic_write(struct udevice *dev, uint reg,
> > const uint8_t *buffer, int len)
> >
> >  int pmic_reg_read(struct udevice *dev, uint reg)
> >  {
> > -       u8 byte;
> > +       struct dm_pmic_info *pmic_info = dev_get_uclass_priv(dev);
> > +       int tx_num = pmic_info->trans_len;  
> 
> Can you use trans_len instead of tx_num, which is confusing?

Ok.

> 
> > +       u32 val = 0;
> >         int ret;
> >
> > -       debug("%s: reg=%x", __func__, reg);
> > -       ret = pmic_read(dev, reg, &byte, 1);
> > -       debug(", value=%x, ret=%d\n", byte, ret);
> > +       if (tx_num < 1 || tx_num > sizeof(val)) {
> > +               printf("Wrong transmission size [%d]\n", tx_num);  
> 
> debug()
> 
> > +               return -EINVAL;
> > +       }
> > +
> > +       debug("%s: reg=%x tx_num:%d", __func__, reg, tx_num);
> > +       ret = pmic_read(dev, reg, (uint8_t *)&val, tx_num);  
> 
> This is assuming little-endian, isn't it? I think you need to build up
> the number in a loop 8 bits at a time, or use some sort of unaligned
> function.

This is fixed in the latter patch (mc34708 PMIC support).
[PATCH v2 05/11] pmic: dm: Add support for MC34708 for PMIC DM

The part:

       buf[0] = buff[2];
       buf[1] = buff[1];
       buf[2] = buff[0];  

Is responsible for byte swap in this particular device.

This code performs similar operation when compared with old PMIC driver:
pmic_reg_write() in the drivers/power/power_i2c.c

However, the new DM model looks a bit better, as we do need to
implement only the conversion relevant to the specific PMIC IC.

> 
> > +       debug(", value=%x, ret=%d\n", val, ret);
> >
> > -       return ret ? ret : byte;
> > +       return ret ? ret : val;
> >  }
> >
> >  int pmic_reg_write(struct udevice *dev, uint reg, uint value)
> >  {
> > -       u8 byte = value;
> > +       struct dm_pmic_info *pmic_info = dev_get_uclass_priv(dev);
> > +       int tx_num = pmic_info->trans_len;
> >         int ret;
> >
> > -       debug("%s: reg=%x, value=%x", __func__, reg, value);
> > -       ret = pmic_write(dev, reg, &byte, 1);
> > +       if (tx_num < 1 || tx_num > sizeof(value)) {
> > +               printf("Wrong transmission size [%d]\n", tx_num);
> > +               return -EINVAL;
> > +       }
> > +
> > +       debug("%s: reg=%x, value=%x tx_num:%d", __func__, reg,
> > value, tx_num);
> > +       ret = pmic_write(dev, reg, (uint8_t *)&value, tx_num);
> >         debug(", ret=%d\n", ret);
> >
> >         return ret;
> > @@ -155,15 +168,22 @@ int pmic_reg_write(struct udevice *dev, uint
> > reg, uint value)
> >
> >  int pmic_clrsetbits(struct udevice *dev, uint reg, uint clr, uint
> > set) {
> > -       u8 byte;
> > +       struct dm_pmic_info *pmic_info = dev_get_uclass_priv(dev);
> > +       int tx_num = pmic_info->trans_len;
> > +       u32 val = 0;
> >         int ret;
> >
> > -       ret = pmic_reg_read(dev, reg);
> > +       if (tx_num < 1 || tx_num > sizeof(val)) {
> > +               printf("Wrong transmission size [%d]\n", tx_num);
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = pmic_read(dev, reg, (uint8_t *)&val, tx_num);
> >         if (ret < 0)
> >                 return ret;
> > -       byte = (ret & ~clr) | set;
> >
> > -       return pmic_reg_write(dev, reg, byte);
> > +       val = (val & ~clr) | set;
> > +       return pmic_write(dev, reg, (uint8_t *)&val, tx_num);
> >  }
> >
> >  static int pmic_pre_probe(struct udevice *dev)
> > --
> > 2.11.0
> >  
> 
> Regards,
> Simon




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
diff mbox series

Patch

diff --git a/drivers/power/pmic/pmic-uclass.c b/drivers/power/pmic/pmic-uclass.c
index 88669533bd..c149b84a68 100644
--- a/drivers/power/pmic/pmic-uclass.c
+++ b/drivers/power/pmic/pmic-uclass.c
@@ -131,23 +131,36 @@  int pmic_write(struct udevice *dev, uint reg, const uint8_t *buffer, int len)
 
 int pmic_reg_read(struct udevice *dev, uint reg)
 {
-	u8 byte;
+	struct dm_pmic_info *pmic_info = dev_get_uclass_priv(dev);
+	int tx_num = pmic_info->trans_len;
+	u32 val = 0;
 	int ret;
 
-	debug("%s: reg=%x", __func__, reg);
-	ret = pmic_read(dev, reg, &byte, 1);
-	debug(", value=%x, ret=%d\n", byte, ret);
+	if (tx_num < 1 || tx_num > sizeof(val)) {
+		printf("Wrong transmission size [%d]\n", tx_num);
+		return -EINVAL;
+	}
+
+	debug("%s: reg=%x tx_num:%d", __func__, reg, tx_num);
+	ret = pmic_read(dev, reg, (uint8_t *)&val, tx_num);
+	debug(", value=%x, ret=%d\n", val, ret);
 
-	return ret ? ret : byte;
+	return ret ? ret : val;
 }
 
 int pmic_reg_write(struct udevice *dev, uint reg, uint value)
 {
-	u8 byte = value;
+	struct dm_pmic_info *pmic_info = dev_get_uclass_priv(dev);
+	int tx_num = pmic_info->trans_len;
 	int ret;
 
-	debug("%s: reg=%x, value=%x", __func__, reg, value);
-	ret = pmic_write(dev, reg, &byte, 1);
+	if (tx_num < 1 || tx_num > sizeof(value)) {
+		printf("Wrong transmission size [%d]\n", tx_num);
+		return -EINVAL;
+	}
+
+	debug("%s: reg=%x, value=%x tx_num:%d", __func__, reg, value, tx_num);
+	ret = pmic_write(dev, reg, (uint8_t *)&value, tx_num);
 	debug(", ret=%d\n", ret);
 
 	return ret;
@@ -155,15 +168,22 @@  int pmic_reg_write(struct udevice *dev, uint reg, uint value)
 
 int pmic_clrsetbits(struct udevice *dev, uint reg, uint clr, uint set)
 {
-	u8 byte;
+	struct dm_pmic_info *pmic_info = dev_get_uclass_priv(dev);
+	int tx_num = pmic_info->trans_len;
+	u32 val = 0;
 	int ret;
 
-	ret = pmic_reg_read(dev, reg);
+	if (tx_num < 1 || tx_num > sizeof(val)) {
+		printf("Wrong transmission size [%d]\n", tx_num);
+		return -EINVAL;
+	}
+
+	ret = pmic_read(dev, reg, (uint8_t *)&val, tx_num);
 	if (ret < 0)
 		return ret;
-	byte = (ret & ~clr) | set;
 
-	return pmic_reg_write(dev, reg, byte);
+	val = (val & ~clr) | set;
+	return pmic_write(dev, reg, (uint8_t *)&val, tx_num);
 }
 
 static int pmic_pre_probe(struct udevice *dev)