Re: [PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 17/1/25 00:20, Bernhard Beschow wrote:
> 
> 
> Am 12. Januar 2025 18:06:04 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> On 9/1/25 17:20, Bernhard Beschow wrote:
>>>
>>>
>>> Am 9. Januar 2025 11:40:10 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>>> Hi Bernhard,
>>>>
>>>> On 8/1/25 10:25, Bernhard Beschow wrote:
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>>     hw/sd/sd.c | 12 ++++++++----
>>>>>     1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>>
>>>>> @@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
>>>>>         sd->cmd_line = true;
>>>>>         sd->multi_blk_cnt = 0;
>>>>>     -    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
>>>>> -    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
>>>>> +    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);
>>>>
>>>> Please embed in sd_get_readonly(),
>>>>
>>>>> +    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);
>>>>
>>>> and sd_get_inserted().
>>>
>>> Are you sure? I deliberately implemented it as is because embedding would change the internal logic of the device as well as SDCardClass::{get_inserted, get_readonly}.
>>
>> Yes, this is why I requested that change. Why don't you think it is correct?
> 
> I'm asking because I think that moving the xor inside the methods would break the device model.
> 
> The goal of the active_low attributes is to invert the polarity of the GPIOs only which allows to model boards where these are inverted. IOW they are intended to influence the wiring. That is in contrast to the two methods which determine the internal logic of the device model. They are also used as virtual method implementations of SDCardClass::{get_inserted, get_readonly} which determine the logic of the sd bus. Moving the xor inside inverts their return values if s->*_active_low are true, effectively flipping every if statement, which seems wrong to me. What do I miss?

Right... Then maybe we should model polarity in qemu_irq.

Patches 7 & 8 queued, thanks!