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

[Krzysztof Kozlowski <krzk@xxxxxxxxxx>]

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'.
>>
>> 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?


> 
>>
>>>
>>>>
>>>>
>>>>> +            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@xxxxxxxxxxxx>
>>>>> + */
>>>>> +
>>>>> +#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.


Best regards,
Krzysztof