Re: [PATCH 05/12] hw/ssi: Introduce a ssi_get_cs() helper

[Cédric Le Goater]

On 6/5/23 07:57, Bernhard Beschow wrote:
> Am 31. Mai 2023 07:39:32 UTC schrieb "Philippe Mathieu-Daudé" 
> <philmd@linaro.org>:
> > On 31/5/23 08:36, Cédric Le Goater wrote:
> > > On 5/31/23 08:17, Philippe Mathieu-Daudé wrote:
> > > > +QOM tinkerers
> > > >
> > > > On 31/5/23 07:59, Cédric Le Goater wrote:
> > > > > On 5/30/23 23:15, Philippe Mathieu-Daudé wrote:
> > > > > > On 30/5/23 22:34, Philippe Mathieu-Daudé wrote:
> > > > > > > On 8/5/23 09:58, Cédric Le Goater wrote:
> > > > > > > > Simple routine to retrieve a DeviceState object on a SPI bus using its
> > > > > > > > address/cs. It will be useful for the board to wire the CS lines.
> > > > > > > >
> > > > > > > > Cc: Alistair Francis <alistair@alistair23.me>
> > > > > > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > > > > > ---
> > > > > > > >    include/hw/ssi/ssi.h |  2 ++
> > > > > > > >    hw/ssi/ssi.c         | 15 +++++++++++++++
> > > > > > > >    2 files changed, 17 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/include/hw/ssi/ssi.h b/include/hw/ssi/ssi.h
> > > > > > > > index ffd3a34ba4..c7beabdb09 100644
> > > > > > > > --- a/include/hw/ssi/ssi.h
> > > > > > > > +++ b/include/hw/ssi/ssi.h
> > > > > > > > @@ -112,4 +112,6 @@ SSIBus *ssi_create_bus(DeviceState *parent, const char 
> > > > > > > > *name);
> > > > > > > >    uint32_t ssi_transfer(SSIBus *bus, uint32_t val);
> > > > > > > > +DeviceState *ssi_get_cs(SSIBus *bus, int addr);
> > > > > >
> > > > > > Also, this helper should (preferably) return a SSIPeripheral type.
> > > > >
> > > > > Well, having a DeviceState is handy for the callers (today) and
> > > > > ssi_create_peripheral() returns a DeviceState. Let's keep it that
> > > > > way.
> > > >
> > > > Yes I know it is handy :) I'm not against your patch; besides other
> > > > APIs do that. I'm wondering about QOM design here. Having Foo device,
> > > > should FOO API return the common qdev abstract type (DeviceState) or
> > > > a Foo type? Either ways we keep QOM-casting around, but I still tend
> > > > to consider FOO API returning Foo pointer provides some type check
> > > > safety, and also provides the API user hints about what is used.
> > > > Need more coffee.
> > >
> > > It is used in two code paths today:
> > >
> > >       ...
> > >           DeviceState *dev = ssi_get_cs(bmc->soc.fmc.spi, 0);
> > >           BlockBackend *fmc0 = dev ? m25p80_get_blk(dev) : NULL;
>
> Looks like m25p80_get_blk() should take a more specific argument as well, like 
> Phil suggested already. Wouldn't that avoid the QOM cast here?

Exposing the m25p80 internals could be painful. I'd rather keep the
definitions where they are under m25p80.c.

Thanks,

C.


> Best regards,
> Bernhard
>
> > >       ...
> > > and
> > >       ...
> > >           DeviceState *dev = ssi_get_cs(s->spi, i);
> > >           if (dev) {
> > >               qemu_irq cs_line = qdev_get_gpio_in_named(dev, SSI_GPIO_CS, 0);
> > >       ...
> > >
> > >
> > > Changing the interface is not a radical change, it will add two QOM-casts.
> > > I can give it a try in v2.
> >
> > Hold on, lets see what others think first.