Re: [PATCH v2 RESEND] ppc/pnv: Fix number of I2C engines and ports for power9/10

[Miles Glenn <milesg@linux.vnet.ibm.com>]

On Fri, 2023-10-27 at 07:05 +0200, Philippe Mathieu-Daudé wrote:
> On 25/10/23 08:56, Cédric Le Goater wrote:
> > On 10/24/23 23:29, Glenn Miles wrote:
> > > Power9 is supposed to have 4 PIB-connected I2C engines with the
> > > following number of ports on each engine:
> > > 
> > >      0: 2
> > >      1: 13
> > >      2: 2
> > >      3: 2
> > > 
> > > Power10 also has 4 engines but has the following number of ports
> > > on each engine:
> > > 
> > >      0: 14
> > >      1: 14
> > >      2: 2
> > >      3: 16
> > > 
> > > Current code assumes that they all have the same (maximum)
> > > number.
> > > This can be a problem if software expects to see a certain number
> > > of ports present (Power Hypervisor seems to care).
> > > 
> > > Fixed this by adding separate tables for power9 and power10 that
> > > map the I2C controller number to the number of I2C buses that
> > > should
> > > be attached for that engine.
> > > 
> > > Signed-off-by: Glenn Miles <milesg@linux.vnet.ibm.com>
> > 
> > you could have kept :
> > 
> > Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > 
> > one comment below,
> > 
> > > ---
> > > Based-on: <20231017221434.810363-1-milesg@linux.vnet.ibm.com>
> > > ([PATCH] ppc/pnv: Connect PNV I2C controller to powernv10)
> > > 
> > > Changes from v1:
> > >      - Added i2c_ports_per_engine to PnvChipClass
> > >      - replaced the word "ctlr" with "engine"
> > > 
> > >   hw/ppc/pnv.c              | 14 ++++++++++----
> > >   include/hw/ppc/pnv_chip.h |  6 ++----
> > >   2 files changed, 12 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > > index 2655b6e506..f6dc84b869 100644
> > > --- a/hw/ppc/pnv.c
> > > +++ b/hw/ppc/pnv.c
> > > @@ -1507,6 +1507,8 @@ static void
> > > pnv_chip_power9_pec_realize(PnvChip 
> > > *chip, Error **errp)
> > >       }
> > >   }
> > > +static int pnv_power9_i2c_ports_per_engine[PNV9_CHIP_MAX_I2C] =
> > > {2, 
> > > 13, 2, 2};
> > > +
> > 
> > Generally, these class constants are located close to the class
> > definitions
> > in the file.
> 
> Either keep them close by for comparison, or, since there
> is a single use, declare it in the function using it here 
> pnv_chip_power9_class_init().

Yeah, I agree.  Version 3 of this patch places the arrays inside the
functions that use them.

Thanks,

Glenn
> 
> > Thanks,
> > 
> > C.