Re: [PATCH v2 06/13] hw/sd/sd: Remove legacy sd_set_cb() in favor of GPIOs

[Bernhard Beschow]

Am 28. Januar 2025 10:34:23 UTC schrieb Peter Maydell 
<peter.maydell@linaro.org>:
>On Mon, 27 Jan 2025 at 23:11, Bernhard Beschow <shentey@gmail.com> wrote:
>>
>>
>>
>> Am 27. Januar 2025 13:24:46 UTC schrieb Peter Maydell 
>> <peter.maydell@linaro.org>:
>> >On Sat, 11 Jan 2025 at 18:37, Bernhard Beschow <shentey@gmail.com> wrote:
>> >>
>> >> Commit ce5dd27534b0 "hw/sd: Remove omap2_mmc device" removed the last 
>> >> user of
>> >> sd_set_cb(). Rework this functionality into GPIOs.
>> >>
>> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> >> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> >
>> >What is this for? We have a non-legacy API for "the SD controller
>> >needs to know when the SD card is inserted or the readonly
>> >status changes", which is that the controller implements the
>> >SDBasClass set_inserted and set_readonly methods. (See the pl011
>> >for an example.)
>> >
>> >I would prefer it if we used that consistently, rather than having
>> >two mechanisms, one using GPIO lines and one using class methods.
>> >I think we should delete the sd_set_cb() API and handling code
>> >entirely.
>>
>> According to the Linux MMC controller DT schema, there are actually two ways 
>> to implement cd and wp lines [1]. When implementing the imx8mp-evk board, I 
>> thought I would need to model the GPIO style [2], hence I implemented it 
>> plus the active low part on the SD card. Later I noticed that the card gets 
>> detected anyway without the GPIO wiring, so I'm fine if the code gets 
>> removed instead.
>
>Even if you did need to implement that GPIO wiring, I think the
>right way to do it is for the SD controller to implement a
>subclass of SDBusClass so it can have its own implementations
>of the set_inserted and set_readonly methods, and then it
>is the SD controller object that has the GPIO lines, not
>the SD card itself.

I agree.

>
>(I have a series almost ready to send out which QOMifies
>the omap_mmc device and then deletes the "legacy" SD API,
>including sd_set_cb().)

Okay, let's just remove this legacy API.

Best regards,
Bernhard

>
>thanks
>-- PMM