Re: [Qemu-devel] [PATCH v2 1/2] i2c: Move contents of bitbang_i2c.h to include/hw/i2c/i2c.h

[Corey Minyard]

On Fri, Mar 15, 2019 at 03:25:55PM +0100, BALATON Zoltan wrote:
> The bitbang i2c implementation is also useful for other device models
> such as DDC in display controllers. Because of this, part of the file had
> to be moved to the main i2c.h to avoid a warning in commit 2b4c1125ac.
> Move the rest of the hw/i2c/bitbang_i2c.h to the main i2c.h now to allow
> it to be used from other device models.

This patch is ok in principle, however, I would prefer that the
bitbang interface be left in it's own bitbang_i2c.h file and
moved into hw/i2c.  I think it's cleaner and more clear that
way.

-corey

> 
> Signed-off-by: BALATON Zoltan <address@hidden>
> ---
>  hw/i2c/bitbang_i2c.c   |  2 +-
>  hw/i2c/bitbang_i2c.h   | 12 ------------
>  hw/i2c/ppc4xx_i2c.c    |  1 -
>  hw/i2c/versatile_i2c.c |  2 +-
>  include/hw/i2c/i2c.h   |  7 +++++++
>  5 files changed, 9 insertions(+), 15 deletions(-)
>  delete mode 100644 hw/i2c/bitbang_i2c.h
> 
> diff --git a/hw/i2c/bitbang_i2c.c b/hw/i2c/bitbang_i2c.c
> index 8be88ee265..a1fe3ac35c 100644
> --- a/hw/i2c/bitbang_i2c.c
> +++ b/hw/i2c/bitbang_i2c.c
> @@ -11,7 +11,7 @@
>   */
>  #include "qemu/osdep.h"
>  #include "hw/hw.h"
> -#include "bitbang_i2c.h"
> +#include "hw/i2c/i2c.h"
>  #include "hw/sysbus.h"
>  
>  //#define DEBUG_BITBANG_I2C
> diff --git a/hw/i2c/bitbang_i2c.h b/hw/i2c/bitbang_i2c.h
> deleted file mode 100644
> index 9443021710..0000000000
> --- a/hw/i2c/bitbang_i2c.h
> +++ /dev/null
> @@ -1,12 +0,0 @@
> -#ifndef BITBANG_I2C_H
> -#define BITBANG_I2C_H
> -
> -#include "hw/i2c/i2c.h"
> -
> -#define BITBANG_I2C_SDA 0
> -#define BITBANG_I2C_SCL 1
> -
> -bitbang_i2c_interface *bitbang_i2c_init(I2CBus *bus);
> -int bitbang_i2c_set(bitbang_i2c_interface *i2c, int line, int level);
> -
> -#endif
> diff --git a/hw/i2c/ppc4xx_i2c.c b/hw/i2c/ppc4xx_i2c.c
> index d6dfafab31..a907d0194e 100644
> --- a/hw/i2c/ppc4xx_i2c.c
> +++ b/hw/i2c/ppc4xx_i2c.c
> @@ -30,7 +30,6 @@
>  #include "cpu.h"
>  #include "hw/hw.h"
>  #include "hw/i2c/ppc4xx_i2c.h"
> -#include "bitbang_i2c.h"
>  
>  #define PPC4xx_I2C_MEM_SIZE 18
>  
> diff --git a/hw/i2c/versatile_i2c.c b/hw/i2c/versatile_i2c.c
> index da9f298ee5..88f0b89f8d 100644
> --- a/hw/i2c/versatile_i2c.c
> +++ b/hw/i2c/versatile_i2c.c
> @@ -23,7 +23,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/sysbus.h"
> -#include "bitbang_i2c.h"
> +#include "hw/i2c/i2c.h"
>  #include "qemu/log.h"
>  
>  #define TYPE_VERSATILE_I2C "versatile_i2c"
> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
> index 8e236f7bb4..fa102dde80 100644
> --- a/include/hw/i2c/i2c.h
> +++ b/include/hw/i2c/i2c.h
> @@ -81,8 +81,15 @@ uint8_t i2c_recv(I2CBus *bus);
>  
>  DeviceState *i2c_create_slave(I2CBus *bus, const char *name, uint8_t addr);
>  
> +/* generic bitbang i2c interface */
> +#define BITBANG_I2C_SDA 0
> +#define BITBANG_I2C_SCL 1
> +
>  typedef struct bitbang_i2c_interface bitbang_i2c_interface;
>  
> +bitbang_i2c_interface *bitbang_i2c_init(I2CBus *bus);
> +int bitbang_i2c_set(bitbang_i2c_interface *i2c, int line, int level);
> +
>  /* lm832x.c */
>  void lm832x_key_event(DeviceState *dev, int key, int state);
>  
> -- 
> 2.13.7
>