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

[Bernhard Beschow]

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?

Best regards,
Bernhard

>
>> 
>> Best regards,
>> Bernhard
>
>>> With the requested changes:
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> 
>