Re: [Qemu-devel] [PATCH v2 3/8] ppc4xx_i2c: Implement directcntl register

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]

On Wed, Jun 13, 2018 at 10:54:22AM +0200, BALATON Zoltan wrote:
> On Wed, 13 Jun 2018, David Gibson wrote:
> > On Wed, Jun 06, 2018 at 03:31:48PM +0200, BALATON Zoltan wrote:
> > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> > > ---
> > >  default-configs/ppc-softmmu.mak    |  1 +
> > >  default-configs/ppcemb-softmmu.mak |  1 +
> > >  hw/i2c/ppc4xx_i2c.c                | 14 +++++++++++++-
> > >  3 files changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/default-configs/ppc-softmmu.mak b/default-configs/ppc-softmmu.mak
> > > index 4d7be45..7d0dc2f 100644
> > > --- a/default-configs/ppc-softmmu.mak
> > > +++ b/default-configs/ppc-softmmu.mak
> > > @@ -26,6 +26,7 @@ CONFIG_USB_EHCI_SYSBUS=y
> > >  CONFIG_SM501=y
> > >  CONFIG_IDE_SII3112=y
> > >  CONFIG_I2C=y
> > > +CONFIG_BITBANG_I2C=y
> > > 
> > >  # For Macs
> > >  CONFIG_MAC=y
> > > diff --git a/default-configs/ppcemb-softmmu.mak b/default-configs/ppcemb-softmmu.mak
> > > index 67d18b2..37af193 100644
> > > --- a/default-configs/ppcemb-softmmu.mak
> > > +++ b/default-configs/ppcemb-softmmu.mak
> > > @@ -19,3 +19,4 @@ CONFIG_USB_EHCI_SYSBUS=y
> > >  CONFIG_SM501=y
> > >  CONFIG_IDE_SII3112=y
> > >  CONFIG_I2C=y
> > > +CONFIG_BITBANG_I2C=y
> > > diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> > > index a68b5f7..5806209 100644
> > > --- a/hw/i2c/ppc4xx_i2c.c
> > > +++ b/hw/i2c/ppc4xx_i2c.c
> > > @@ -30,6 +30,7 @@
> > >  #include "cpu.h"
> > >  #include "hw/hw.h"
> > >  #include "hw/i2c/ppc4xx_i2c.h"
> > > +#include "bitbang_i2c.h"
> > > 
> > >  #define PPC4xx_I2C_MEM_SIZE 18
> > > 
> > > @@ -46,7 +47,13 @@
> > > 
> > >  #define IIC_XTCNTLSS_SRST   (1 << 0)
> > > 
> > > +#define IIC_DIRECTCNTL_SDAC (1 << 3)
> > > +#define IIC_DIRECTCNTL_SCLC (1 << 2)
> > > +#define IIC_DIRECTCNTL_MSDA (1 << 1)
> > > +#define IIC_DIRECTCNTL_MSCL (1 << 0)
> > > +
> > >  typedef struct {
> > > +    bitbang_i2c_interface *bitbang;
> > >      uint8_t mdata;
> > >      uint8_t lmadr;
> > >      uint8_t hmadr;
> > > @@ -308,7 +315,11 @@ static void ppc4xx_i2c_writeb(void *opaque, hwaddr addr, uint64_t value,
> > >          i2c->xtcntlss = value;
> > >          break;
> > >      case 16:
> > > -        i2c->directcntl = value & 0x7;
> > > +        i2c->directcntl = value & (IIC_DIRECTCNTL_SDAC & IIC_DIRECTCNTL_SCLC);
> > > +        i2c->directcntl |= (value & IIC_DIRECTCNTL_SCLC ? 1 : 0);
> > > +        bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SCL, i2c->directcntl & 1);
> > 
> > Shouldn't that use i2c->directcntl & IIC_DIRECTCNTL_MSCL ?
> > 
> > > +        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> > > +                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
> > 
> > Last expression might be clearer as:
> > 	value & IIC_DIRECTCNTL_SDAC ? IIC_DIRECTCNTL_MSDA : 0
> 
> I guess this is a matter of taste but to me IIC_DIRECTCNTL_MSDA is a bit
> position in the register so I use that when accessing that bit but when I
> check for the values of a bit being 0 or 1 I don't use the define which is
> for something else, just happens to have value 1 as well.

Hmm.. but the bit is being store in i2c->directcntl, which means it
can be read back from the register in that position, no?

> If this does not explain your question and you think it's better to use
> defines here I can change that in next version, please let me know.
> 
> Regards,
> BALATON Zoltan
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]