Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch

From: Patrick Venture <venture@google.com>
To: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	Thomas Huth <thuth@redhat.com>,
	 Corey Minyard <cminyard@mvista.com>,
	Hao Wu <wuhaotsh@google.com>,
	 Havard Skinnemoen <hskinnemoen@google.com>,
	qemu-arm@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [PATCH v2 4/4] hw/i2c: add pca954x i2c-mux switch
Date: Fri, 9 Apr 2021 14:32:50 -0700	[thread overview]
Message-ID: <CAO=notwo5U4xRHDYKyH5e83qA0jaqEWKCPBsZUUeVbW19YjKdQ@mail.gmail.com> (raw)
In-Reply-To: <4c44381d-99ac-f1c9-60a9-0f3132422473@amsat.org>

On Fri, Apr 9, 2021 at 2:20 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> +Paolo/Thomas
>
> On 4/9/21 7:21 PM, Patrick Venture wrote:
> > On Fri, Apr 9, 2021 at 9:51 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> Hi Patrick,
> >>
> >> On 4/9/21 6:25 PM, Patrick Venture wrote:
> >>> The pca954x is an i2c mux, and this adds support for two variants of
> >>> this device: the pca9546 and pca9548.
> >>>
> >>> This device is very common on BMCs to route a different channel to each
> >>> PCIe i2c bus downstream from the BMC.
> >>>
> >>> Signed-off-by: Patrick Venture <venture@google.com>
> >>> Reviewed-by: Hao Wu <wuhaotsh@google.com>
> >>> Reviewed-by: Havard Skinnemoen <hskinnemoen@google.com>
> >>> ---
> >>>  MAINTAINERS                      |   6 +
> >>>  hw/i2c/Kconfig                   |   4 +
> >>>  hw/i2c/i2c_mux_pca954x.c         | 290 +++++++++++++++++++++++++++++++
> >>>  hw/i2c/meson.build               |   1 +
> >>>  hw/i2c/trace-events              |   5 +
> >>>  include/hw/i2c/i2c_mux_pca954x.h |  19 ++
> >>>  6 files changed, 325 insertions(+)
> >>>  create mode 100644 hw/i2c/i2c_mux_pca954x.c
> >>>  create mode 100644 include/hw/i2c/i2c_mux_pca954x.h
> >>
> >>> diff --git a/hw/i2c/Kconfig b/hw/i2c/Kconfig
> >>> index 09642a6dcb..8d120a25d5 100644
> >>> --- a/hw/i2c/Kconfig
> >>> +++ b/hw/i2c/Kconfig
> >>> @@ -28,3 +28,7 @@ config IMX_I2C
> >>>  config MPC_I2C
> >>>      bool
> >>>      select I2C
> >>> +
> >>> +config PCA954X
> >>> +    bool
> >>> +    select I2C
> >>
> >> Do you have a circular dependency when also using:
> >>
> >>        depends on I2C
> >>
> >> ?
> >
> > I'm somewhat new to qemu -- I don't know what you mean, since I2C
> > doesn't depend on pca954x, I don't imagine there could be a circular
> > dependency.
>
> See
> https://qemu-project.gitlab.io/qemu/devel/kconfig.html#guidelines-for-writing-kconfig-files
>
> PCA954X is plugged on an I2C bus
> -> depends on I2C
>
> PCA954X provides I2C buses
> -> select I2C

So from the guide it looks like my KConfig should have _depends_ on
I2C.  My board that I'm testing with selects PCA954X and doesn't
explicitly select I2C.  My device _does_ provide I2C buses, as you
say.

>
> Your device is a particular case consuming and providing the Kconfig
> 'I2C' symbol. I expect a circular dependency problem. Easy to test with
> your series but I haven't.
>
> I suppose in this case, the "select" takes over on "depends on" so this
> is OK.

I have to imagine there is a similar situation for PCIe bridges, as
they depend on PCI but also provide it.

>
> Now (unrelated to your series) thinking at the graphical Kconfig tree
> representation (like this one generated 2 years ago:
> https://drive.google.com/open?id=1kvwl7guuAmCh2Y2UqeXynlA2HmjWcRs9),
> I'd rather see a circular dep.
>
> Regards,
>
> Phil.