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

[Lukasz Majewski <lukma@denx.de>]

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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180514/60a86dc2/attachment.sig>