Re: [PATCH v3 1/2] dt-bindings: pinctrl: mediatek: add support for mt8196

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

Il 20/01/25 10:17, Cathy Xu (许华婷) ha scritto:
> On Thu, 2025-01-16 at 11:20 +0100, Krzysztof Kozlowski wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> On 16/01/2025 09:18, Cathy Xu (许华婷) wrote:
>>> On Thu, 2025-01-16 at 08:28 +0100, Krzysztof Kozlowski wrote:
>>>> External email : Please do not click links or open attachments
>>>> until
>>>> you have verified the sender or the content.
>>>>
>>>>
>>>> On 16/01/2025 03:20, Cathy Xu (许华婷) wrote:
>>>>>>> +          bias-pull-down:
>>>>>>> +            oneOf:
>>>>>>> +              - type: boolean
>>>>>>> +              - enum: [100, 101, 102, 103]
>>>>>>> +                description: mt8196 pull down PUPD/R0/R1
>>>>>>> type
>>>>>>> define value.
>>>>>>> +              - enum: [200, 201, 202, 203, 204, 205, 206,
>>>>>>> 207]
>>>>>>> +                description: mt8196 pull down RSEL type
>>>>>>> define
>>>>>>> value.
>>>>>>
>>>>>> Not much improved.
>>>>>
>>>>>    I have removed the content related to 'resistance value', we
>>>>> use
>>>>> 'RSEL' instead of 'resistance value'.

This is wrong.

>>>>
>>>> So the value in Ohms was removed? I assume above do not have
>>>> known
>>>> value
>>>> in Ohms?
>>>
>>>    Yes, value in Ohns was removed, no code have knowm value.
>>
>> Does the hardware have known value in Ohms?

It does.

> 
>    What do you mean by 'hardware'? When writing to the rsel register,
> the value written is 0-7.
> 

Hardware means "the pin controller of the mt8196 SoC" :-)

Anyway.

The RSEL registers' function is to select a specific resistance value to
pullup/down a pin, or a group of pins.

Devicetree bindings require to specify values in known units, so in device tree
you *need* to specify the RSEL resistance in Ohms.

You cannot specify RSEL register value in device-tree. That's unacceptable.

Regards,
Angelo

>>
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>
>>>>>>> +            description: |
>>>>>>> +              For pull down type is normal, it doesn't
>>>>>>> need
>>>>>>> add
>>>>>>> RSEL & R1R0.
>>>>>>> +              For pull down type is PUPD/R0/R1 type, it
>>>>>>> can
>>>>>>> add
>>>>>>> R1R0 define to
>>>>>>> +              set different resistance. It can support
>>>>>>> "MTK_PUPD_SET_R1R0_00" &
>>>>>>> +              "MTK_PUPD_SET_R1R0_01" &
>>>>>>> "MTK_PUPD_SET_R1R0_10"
>>>>>>> &
>>>>>>> +              "MTK_PUPD_SET_R1R0_11" define in mt8196.
>>>>>>> +              For pull down type is PD/RSEL, it can add
>>>>>>> RSEL
>>>>>>> define to set
>>>>>>> +              different resistance. It can support
>>>>>>> +              "MTK_PULL_SET_RSEL_000" &
>>>>>>> "MTK_PULL_SET_RSEL_001" &
>>>>>>> +              "MTK_PULL_SET_RSEL_010" &
>>>>>>> "MTK_PULL_SET_RSEL_011" &
>>>>>>> +              "MTK_PULL_SET_RSEL_100" &
>>>>>>> "MTK_PULL_SET_RSEL_101" &
>>>>>>> +              "MTK_PULL_SET_RSEL_110" &
>>>>>>> "MTK_PULL_SET_RSEL_111"
>>>>>>> define in
>>>>>>> +              mt8196.
>>>>>>> diff --git a/include/dt-bindings/pinctrl/mt8196-pinfunc.h
>>>>>>> b/include/dt-bindings/pinctrl/mt8196-pinfunc.h
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..bf0c8374407c
>>>>>>> --- /dev/null
>>>>>>> +++ b/include/dt-bindings/pinctrl/mt8196-pinfunc.h
>>>>>>> @@ -0,0 +1,1572 @@
>>>>>>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>>>>> */
>>>>>>> +/*
>>>>>>> + * Copyright (C) 2025 Mediatek Inc.
>>>>>>> + * Author: Guodong Liu <Guodong.Liu@mediatek.com>
>>>>>>> + */
>>>>>>> +
>>>>>>> +#ifndef __MT8196_PINFUNC_H
>>>>>>> +#define __MT8196_PINFUNC_H
>>>>>>> +
>>>>>>> +#include <dt-bindings/pinctrl/mt65xx.h>
>>>>>>> +
>>>>>>> +#define PINMUX_GPIO0__FUNC_GPIO0 (MTK_PIN_NO(0) | 0)
>>>>>>> +#define PINMUX_GPIO0__FUNC_DMIC1_CLK (MTK_PIN_NO(0) | 1)
>>>>>>> +#define PINMUX_GPIO0__FUNC_SPI3_A_MO (MTK_PIN_NO(0) | 3)
>>>>>>> +#define PINMUX_GPIO0__FUNC_FMI2S_B_LRCK (MTK_PIN_NO(0) |
>>>>>>> 4)
>>>>>>> +#define PINMUX_GPIO0__FUNC_SCP_DMIC1_CLK (MTK_PIN_NO(0) |
>>>>>>> 5)
>>>>>>> +#define PINMUX_GPIO0__FUNC_TP_GPIO14_AO (MTK_PIN_NO(0) |
>>>>>>> 6)
>>>>>>
>>>>>> I do not see how you resolved my comment from v1. In v2 I
>>>>>> reminded
>>>>>> about
>>>>>> it, so you responded that yopu will change something, but I
>>>>>> do
>>>>>> not
>>>>>> see
>>>>>> any changes.
>>>>>>
>>>>>> So explain: how did you resolve my comment?
>>>>>>
>>>>>> These two examples where you claim you will change something,
>>>>>> but
>>>>>> send
>>>>>> the same. I skipped the rest of the patch.
>>>>>
>>>>>    Thank you for your patient response, here is my explanation
>>>>> for
>>>>> you
>>>>> question:
>>>>>
>>>>>    In v1, I undertand that you meant I didn't sent a real
>>>>> binding,
>>>>> and
>>>>
>>>>
>>>> The comment is under specific lines, so I said these defines are
>>>> not
>>>> a
>>>> real binding. You sent them again, but they are still not
>>>> bindings,
>>>> because they are not used in the driver. Maybe the usage is
>>>> convoluted,
>>>> so which part of implementation are these connecting with DTS?
>>>> IOW,
>>>> which part of driver relies on the binding?
>>>
>>>    I got you. This binding define many macros, which will be used
>>> for
>>> 'pinmux' setting in the DTS. The usage like this:
>>>
>>>    adsp_uart_pins: adsp-uart-pins {
>>>                  pins-tx-rx {
>>>                          pinmux =
>>> <PINMUX_GPIO35__FUNC_O_ADSP_UTXD0>,
>>>                                   <PINMUX_GPIO36__FUNC_I1_ADSP_URXD0
>>>> ;
>>>                  };
>>>          };
>>
>>
>> That's DTS, not driver, so not a binding. Drop the header from
>> bindings.
> 
>    Sorry, I don't quite understand the relationship between binding and
> driver. Driver will parse this macro to get gpio number and function.
>