Re: [Qemu-ppc] [PATCH v3 2/9] ppc4xx_i2c: Implement directcntl register

[BALATON Zoltan]

On Thu, 14 Jun 2018, David Gibson wrote:
> On Thu, Jun 14, 2018 at 02:17:00AM +0200, BALATON Zoltan wrote:
> > Signed-off-by: BALATON Zoltan <address@hidden>
>
> Patch looks good, but it needs a commit message.  What is the
> directcntl register?  Now does the bitbang interface play into that?
> (Note that I know the answer to those questions right now, but it
> needs to be in the commit message for the benefit of people looking
> back in the future).

If you know the answer could you please suggest an appropriate commit 
message? I'm a bit lost what should be written here that would explain the 
patch to someone who knows nothing about what this is. And for those who 
know, the patch itself should be fairly simple and not need more 
explanation.

Regards,
BALATON Zoltan

> > ---
> >  default-configs/ppc-softmmu.mak    |  1 +
> >  default-configs/ppcemb-softmmu.mak |  1 +
> >  hw/i2c/ppc4xx_i2c.c                | 13 ++++++++++++-
> >  include/hw/i2c/ppc4xx_i2c.h        |  4 ++++
> >  4 files changed, 18 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 4e0aaae..c0a1930 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,6 +47,11 @@
> >
> >  #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)
> > +
> >  static void ppc4xx_i2c_reset(DeviceState *s)
> >  {
> >      PPC4xxI2CState *i2c = PPC4xx_I2C(s);
> > @@ -289,7 +295,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);
> > +        i2c->directcntl |= bitbang_i2c_set(i2c->bitbang, BITBANG_I2C_SDA,
> > +                               (value & IIC_DIRECTCNTL_SDAC) != 0) << 1;
> >          break;
> >      default:
> >          if (addr < PPC4xx_I2C_MEM_SIZE) {
> > @@ -322,6 +332,7 @@ static void ppc4xx_i2c_init(Object *o)
> >      sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
> >      sysbus_init_irq(SYS_BUS_DEVICE(s), &s->irq);
> >      s->bus = i2c_init_bus(DEVICE(s), "i2c");
> > +    s->bitbang = bitbang_i2c_init(s->bus);
> >  }
> >
> >  static void ppc4xx_i2c_class_init(ObjectClass *klass, void *data)
> > diff --git a/include/hw/i2c/ppc4xx_i2c.h b/include/hw/i2c/ppc4xx_i2c.h
> > index e4b6ded..ea6c8e1 100644
> > --- a/include/hw/i2c/ppc4xx_i2c.h
> > +++ b/include/hw/i2c/ppc4xx_i2c.h
> > @@ -31,6 +31,9 @@
> >  #include "hw/sysbus.h"
> >  #include "hw/i2c/i2c.h"
> >
> > +/* from hw/i2c/bitbang_i2c.h */
> > +typedef struct bitbang_i2c_interface bitbang_i2c_interface;
> > +
> >  #define TYPE_PPC4xx_I2C "ppc4xx-i2c"
> >  #define PPC4xx_I2C(obj) OBJECT_CHECK(PPC4xxI2CState, (obj), TYPE_PPC4xx_I2C)
> >
> > @@ -42,6 +45,7 @@ typedef struct PPC4xxI2CState {
> >      I2CBus *bus;
> >      qemu_irq irq;
> >      MemoryRegion iomem;
> > +    bitbang_i2c_interface *bitbang;
> >      uint8_t mdata;
> >      uint8_t lmadr;
> >      uint8_t hmadr;