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 <mil...@linux.vnet.ibm.com> > > > > you could have kept : > > > > Reviewed-by: Cédric Le Goater <c...@kaod.org> > > > > one comment below, > > > > > --- > > > Based-on: <20231017221434.810363-1-mil...@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.