Re: [PATCH 1/4] hw/arm: Add PCA9554 to ARM target

[etanous via <qemu-devel@nongnu.org>]

On Wed, Jul 02, 2025 at 09:04:25AM +0200, Philippe Mathieu-Daudé wrote:
> 
> Hi,
> 
> On 2/7/25 08:47, Cédric Le Goater wrote:
> > Hello Ed,
> > 
> > On 7/1/25 22:33, Ed Tanous wrote:
> > > From: Ed Tanous <ed@tanous.net>
> > > 
> > > There are arm targets that are connected to this io expander,
> > > specifically some varieties of Aspeed 2600 BMCs.  Add it to Kconfig to
> > > allow use.
> > > 
> > > Signed-off-by: Ed Tanous <etanous@nvidia.com>
> > > ---
> > >   hw/arm/Kconfig | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> > > index f543d944c3..6ea86534d5 100644
> > > --- a/hw/arm/Kconfig
> > > +++ b/hw/arm/Kconfig
> > > @@ -532,6 +532,7 @@ config ASPEED_SOC
> > >       select I2C
> > >       select DPS310
> > >       select PCA9552
> > > +    select PCA9554
> 
> Note, these i2c devices 1/ aren't part of the SoC, but boards/machines,
> 2/ nor are they required to have a functional machine (i.e. a i2c link
> could get cut or an i2c device ending dead).
> 
> I'd prefer 1/ add a ASPEED_MACHINE layer selecting ASPEED_SOC and the
> external devices, and 2/ use "imply" statement instead of "select" for
> devices, as per docs/devel/kconfig.rst:
> 
>   Boards specify their constituent devices using ``imply`` and
>   ``select`` directives.  A device should be listed under ``select``
>   if the board cannot be started at all without it.  It should be
>   listed under ``imply`` if (depending on the QEMU command line)
>   the board may or may not be started without it.  Boards default to
>   true, but also have a ``depends on`` clause to limit them to the
>   appropriate targets.
>   For some targets, not all boards may be supported by hardware
>   virtualization, in which case they also depend on the ``TCG``
>   symbol, other symbols that are commonly used as dependencies for
>   boards include libraries (such as ``FDT``) or ``TARGET_BIG_ENDIAN``
>   (possibly negated).
> 

ACK, seems reasonable.  I tried to follow the pattern that was there,
but agreed, it was odd that "board" level things were added at the ARM
layer.

> My 2 cents.
> 
> Regards,
> 
> Phil.