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

[Bernhard Beschow <shentey@gmail.com>]

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?

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.