Re: [PATCH v2 1/9] hw/i2c/pca954x: Add method to get channels

[Peter Delevoryas]

On Wed, Jul 06, 2022 at 08:06:34AM +0200, Cédric Le Goater wrote:
> On 7/5/22 23:44, Peter Delevoryas wrote:
> > On Tue, Jul 05, 2022 at 02:40:32PM -0700, Peter Delevoryas wrote:
> > > On Tue, Jul 05, 2022 at 03:06:24PM -0500, Corey Minyard wrote:
> > > > On Tue, Jul 05, 2022 at 12:13:52PM -0700, Peter Delevoryas wrote:
> > > > > I added this helper in the Aspeed machine file a while ago to help
> > > > > initialize fuji-bmc i2c devices. This moves it to the official pca954x
> > > > > file so that other files can use it.
> > > > > 
> > > > > This does something very similar to pca954x_i2c_get_bus, but I think
> > > > > this is useful when you have a very complicated dts with a lot of
> > > > > switches, like the fuji dts.
> > > > > 
> > > > > This convenience method lets you write code that produces a flat array
> > > > > of I2C buses that matches the naming in the dts. After that you can 
> > > > > just
> > > > > add individual sensors using the flat array of I2C buses.
> > > > 
> > > > This is an improvment, I think.  But it really needs to be two patches,
> > > > one with the I2C portion, and one with the aspeed portion.
> > > > 
> > > > Also, the name is a little misleading, you might want to name it
> > > > pca954x_i2c_create_get_channels
> > > 
> > > You're right: Cedric, you can just ignore the pca954x patch then if you'd 
> > > like,
> > > I can resubmit it with the future I2C series that uses it. I probably 
> > > shouldn't
> > > have submitted it quite yet.
> > > 
> > > I can also resubmit the series with that patch removed, not sure if that's
> > > necessary or not.
> > 
> > This was hastily written, what I meant to say was:
> > 
> > Cedric, feel free to remove this patch from the series. If you'd like, I can
> > also resubmit this series as v3 with the patch removed.
> 
> 
> I moved it at the end of the series to come just before the other patches
> needing it, the last three patches of :
> 
>   http://patchwork.ozlabs.org/project/qemu-devel/list/?series=307305
> 
> You can resend all of them when fixed.

That sounds good, thanks.

> 
> 
> I think we are done with the initial fby35.
> 
> Next time, please add a cover letter and keep the Reviewed-by tags
> of the previous version. It helps the reviewers. I re-added them
> manually for this spin.

Yeah that's my bad again, I need to get in a better habit of adding those.
Thanks for reminding me again.

> Thanks,
> 
> C.
> 
> > 
> > > 
> > > > 
> > > > -corey
> > > > 
> > > > > 
> > > > > See fuji_bmc_i2c_init to understand this point further.
> > > > > 
> > > > > The fuji dts is here for reference:
> > > > > 
> > > > > https://github.com/torvalds/linux/blob/40cb6373b46/arch/arm/boot/dts/aspeed-bmc-facebook-fuji.dts
> > > > > 
> > > > > Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> > > > > ---
> > > > >   hw/arm/aspeed.c                  | 29 +++++++++--------------------
> > > > >   hw/i2c/i2c_mux_pca954x.c         | 10 ++++++++++
> > > > >   include/hw/i2c/i2c_mux_pca954x.h | 13 +++++++++++++
> > > > >   3 files changed, 32 insertions(+), 20 deletions(-)
> > > > > 
> > > > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> > > > > index 6fe9b13548..bee8a748ec 100644
> > > > > --- a/hw/arm/aspeed.c
> > > > > +++ b/hw/arm/aspeed.c
> > > > > @@ -793,15 +793,6 @@ static void 
> > > > > rainier_bmc_i2c_init(AspeedMachineState *bmc)
> > > > >       create_pca9552(soc, 15, 0x60);
> > > > >   }
> > > > > -static void get_pca9548_channels(I2CBus *bus, uint8_t mux_addr,
> > > > > -                                 I2CBus **channels)
> > > > > -{
> > > > > -    I2CSlave *mux = i2c_slave_create_simple(bus, "pca9548", 
> > > > > mux_addr);
> > > > > -    for (int i = 0; i < 8; i++) {
> > > > > -        channels[i] = pca954x_i2c_get_bus(mux, i);
> > > > > -    }
> > > > > -}
> > > > > -
> > > > >   #define TYPE_LM75 TYPE_TMP105
> > > > >   #define TYPE_TMP75 TYPE_TMP105
> > > > >   #define TYPE_TMP422 "tmp422"
> > > > > @@ -814,20 +805,18 @@ static void 
> > > > > fuji_bmc_i2c_init(AspeedMachineState *bmc)
> > > > >       for (int i = 0; i < 16; i++) {
> > > > >           i2c[i] = aspeed_i2c_get_bus(&soc->i2c, i);
> > > > >       }
> > > > > -    I2CBus *i2c180 = i2c[2];
> > > > > -    I2CBus *i2c480 = i2c[8];
> > > > > -    I2CBus *i2c600 = i2c[11];
> > > > > -    get_pca9548_channels(i2c180, 0x70, &i2c[16]);
> > > > > -    get_pca9548_channels(i2c480, 0x70, &i2c[24]);
> > > > > +    pca954x_i2c_get_channels(i2c[2], 0x70, "pca9548", &i2c[16]);
> > > > > +    pca954x_i2c_get_channels(i2c[8], 0x70, "pca9548", &i2c[24]);
> > > > >       /* NOTE: The device tree skips [32, 40) in the alias numbering 
> > > > > */
> > > > > -    get_pca9548_channels(i2c600, 0x77, &i2c[40]);
> > > > > -    get_pca9548_channels(i2c[24], 0x71, &i2c[48]);
> > > > > -    get_pca9548_channels(i2c[25], 0x72, &i2c[56]);
> > > > > -    get_pca9548_channels(i2c[26], 0x76, &i2c[64]);
> > > > > -    get_pca9548_channels(i2c[27], 0x76, &i2c[72]);
> > > > > +    pca954x_i2c_get_channels(i2c[11], 0x77, "pca9548", &i2c[40]);
> > > > > +    pca954x_i2c_get_channels(i2c[24], 0x71, "pca9548", &i2c[48]);
> > > > > +    pca954x_i2c_get_channels(i2c[25], 0x72, "pca9548", &i2c[56]);
> > > > > +    pca954x_i2c_get_channels(i2c[26], 0x76, "pca9548", &i2c[64]);
> > > > > +    pca954x_i2c_get_channels(i2c[27], 0x76, "pca9548", &i2c[72]);
> > > > >       for (int i = 0; i < 8; i++) {
> > > > > -        get_pca9548_channels(i2c[40 + i], 0x76, &i2c[80 + i * 8]);
> > > > > +        pca954x_i2c_get_channels(i2c[40 + i], 0x76, "pca9548",
> > > > > +                                 &i2c[80 + i * 8]);
> > > > >       }
> > > > >       i2c_slave_create_simple(i2c[17], TYPE_LM75, 0x4c);
> > > > > diff --git a/hw/i2c/i2c_mux_pca954x.c b/hw/i2c/i2c_mux_pca954x.c
> > > > > index 3945de795c..6b07804546 100644
> > > > > --- a/hw/i2c/i2c_mux_pca954x.c
> > > > > +++ b/hw/i2c/i2c_mux_pca954x.c
> > > > > @@ -169,6 +169,16 @@ I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, 
> > > > > uint8_t channel)
> > > > >       return pca954x->bus[channel];
> > > > >   }
> > > > > +void pca954x_i2c_get_channels(I2CBus *bus, uint8_t address,
> > > > > +                              const char *type_name, I2CBus 
> > > > > **channels)
> > > > > +{
> > > > > +    I2CSlave *mux = i2c_slave_create_simple(bus, type_name, address);
> > > > > +    Pca954xClass *pc = PCA954X_GET_CLASS(mux);
> > > > > +    Pca954xState *pca954x = PCA954X(mux);
> > > > > +
> > > > > +    memcpy(channels, pca954x->bus, pc->nchans * sizeof(channels[0]));
> > > > > +}
> > > > > +
> > > > >   static void pca9546_class_init(ObjectClass *klass, void *data)
> > > > >   {
> > > > >       Pca954xClass *s = PCA954X_CLASS(klass);
> > > > > diff --git a/include/hw/i2c/i2c_mux_pca954x.h 
> > > > > b/include/hw/i2c/i2c_mux_pca954x.h
> > > > > index 3dd25ec983..3a676a30a9 100644
> > > > > --- a/include/hw/i2c/i2c_mux_pca954x.h
> > > > > +++ b/include/hw/i2c/i2c_mux_pca954x.h
> > > > > @@ -16,4 +16,17 @@
> > > > >    */
> > > > >   I2CBus *pca954x_i2c_get_bus(I2CSlave *mux, uint8_t channel);
> > > > > +/**
> > > > > + * Creates an i2c mux and retrieves all of the channels associated 
> > > > > with it.
> > > > > + *
> > > > > + * @bus: the i2c bus where the i2c mux resides.
> > > > > + * @address: the address of the i2c mux on the aforementioned i2c 
> > > > > bus.
> > > > > + * @type_name: name of the i2c mux type to create.
> > > > > + * @channels: an output parameter specifying where to return the 
> > > > > channels.
> > > > > + *
> > > > > + * Returns: None
> > > > > + */
> > > > > +void pca954x_i2c_get_channels(I2CBus *bus, uint8_t address,
> > > > > +                              const char *type_name, I2CBus 
> > > > > **channels);
> > > > > +
> > > > >   #endif
> > > > > -- 
> > > > > 2.37.0
> > > > > 
> > > > > 
> > > 
>