qemu-arm
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Cédric Le Goater
Subject: Re: [PATCH 05/12] hw/ssi: Introduce a ssi_get_cs() helper
Date: Mon, 5 Jun 2023 18:21:27 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.10.0

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.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]