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

[Philippe Mathieu-Daudé]

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!