Re: [PATCH v2 6/6] arm64: dts: qcom: msm8956-loire: Add SD Card Detect to SDC2 pin states

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 22/01/24 16:15, Marijn Suijten ha scritto:
> On 2024-01-22 15:59:37, AngeloGioacchino Del Regno wrote:
>> Il 22/01/24 14:49, Marijn Suijten ha scritto:
>>> On 2024-01-22 12:48:27, AngeloGioacchino Del Regno wrote:
>>>> Il 21/01/24 23:33, Marijn Suijten ha scritto:
>>>>> In addition to the SDC2 pins, set the SD Card Detect pin in a sane state
>>>>> to be used as an interrupt when an SD Card is slotted in or removed.
>>>>>
>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>>> ---
>>>>>     arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire.dtsi | 17 +++++++++++++++++
>>>>>     1 file changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire.dtsi b/arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire.dtsi
>>>>> index b0b83edd3627..75412e37334c 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/msm8956-sony-xperia-loire.dtsi
>>>>> @@ -264,10 +264,27 @@ &sdhc_1 {
>>>>>     	status = "okay";
>>>>>     };
>>>>>     
>>>>> +&sdc2_off_state {
>>>>> +	sd-cd-pins {
>>>>> +		pins = "gpio100";
>>>>> +		function = "gpio";
>>>>> +		drive-strength = <2>;
>>>>> +		bias-disable;
>>>>> +	};
>>>>
>>>> Are you sure that you really don't want card detect during system suspend?
>>>
>>> Does it make a difference if the rest of pinctrl and the SDHCI controller are
>>> also turned off?
>>>
>>>> You could simply add a sdc2-cd-pins out of sdc2_{on,off}_state and add use it for
>>>> both default and sleep.
>>>
>>> This sounds close to what Konrad suggested by using a new block wit its own
>>> label rather than extending the existing state.
>>>
>>>> pinctrl-0 = <&sdc2_on_state>, <&sdc2_card_det_n>;
>>>> pinctrl-1 = <&sdc2_off_state>;
>>>
>>> You said both, but it's not in pinctrl-1 here?  (And might unselect bias-pull-up
>>> implicitly instead of explicitly selecting bias-disable via an off node?)
>>>
>>
>> I meant to add it to both, sorry.
>>
>> In any case, take the typo'ed example as a simplification of your first version :-)
> 
> Okay, I'll resend a version that creates a new pinctrl node and applies it to both cases.
> 
> Unfortunately I can no longer test and confirm that it makes a difference
> to have the card-detect IRQ always biased, even while the SDHCI controller
> is "asleep" or off, so I'll trust your word for it.  If I remember correctly
> downstream turns it off as well?

Marijn, I don't remember anything about a downstream kernel from 10 years ago..!

But yes, it's okay to do so - the worst case is that it won't wake up, but that
won't happen, as it's going to generate an interrupt and wake up the entire system
which includes the SDHCI controller.

Go on!

Cheers,
Angelo