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

["Cathy Xu (许华婷)" <ot_cathy.xu@mediatek.com>]

On Thu, 2025-01-16 at 16:33 +0800, Chen-Yu Tsai wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Thu, Jan 16, 2025 at 4:19 PM Cathy Xu (许华婷) <
> ot_cathy.xu@mediatek.com> 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.
> 
> That's sad. We went through a lot during the MT8195 cycle to get the
> paris driver library to support proper SI unit values [1] to replace
> the RSEL values (200 ~ 207). Why can't this be supported anymore?
> 
> Also we never got around to getting rid the PUPD/R0/R1 values (100 ~
> 103).

  Sorry, I didn't see any parsing of 'mediatek,rsel-resistence-in-si-
unit' in driver, and I noticed that in driver, the final value written
to the rsel register is based on the value of RSEL(200 ~ 207).

  Additionally, I don't understand why we abandon the RSEL/R0/R1 values
(200 ~ 207/100 ~ 103). Is it to prevent confusion? Perhaps adding some
comments would help?
  
> 
> > > 
> > > > 
> > > > > 
> > > > > 
> > > > > > +            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
> > >;
> >                 };
> >         };
> 
> The only binding between the DT and the driver is the structure of
> the value, given as "(MTK_PIN_NO(<N>) | <function mux value>)".
> 
> The whole list of "PINMUX_GPIOxxx__FUNC_xyz" macros is just a
> convenience
> table for developers, and not used by the driver. The driver simply
> takes
> the values from the two bit fields and uses them directly.
> 
> That's why Krzysztof is saying the macros are not used in the driver
> and therefore not a binding.
> 
> Please move the header file to under "arch/arm64/boot/dts/mediatek",
> and split it out as a separate commit with a subject like:
> 
>     arm64: dts: mediatek: mt8196: Add pinmux macro header file

  ok, got it, I will fix in next version. However, the previous 'xxx-
pinfunc.h' file were placed under binding.

> 
> 
> ChenYu
> 
> > > 
> > > 
> > > 
> > > > the bindings should be separated from driver. In addition, I
> > > > should
> > > > run scripts/checkpatch.pl and scripts/get_maintainers.pl. So in
> > > > v2,
> > > > I
> > > > sent a real binding(mediatek,mt8196-pinctrl.yaml), and sent two
> > > > separate patches, one for driver and one for bindings, also ran
> > > > scripts/get_maintainers.pl get necessary people and sent to
> > > > them.
> > > > 
> > > >   In v2, I understand that I need refer to git history to
> > > > modify
> > > > the
> > > > commit msgs, so I made the changes in v3. Then you asked me
> > > > about
> > > > the
> > > > difference between 'RSEL' and 'resistance value'. I replied
> > > > that
> > > > the
> > > > 'resistance value' method is no longer use, so in v3, I removed
> > > > all
> > > > content about it(include entire 'rsel-resistance-in-si-unit'
> > > > property
> > > > and the parts mentioned in bias-pull-up/down).
> > > 
> > > Yes, thank you this I saw, the comments appear under specific
> > > places,
> > > so
> > > only these places are discussed.
> > 
> >   ok, thank you, we can discuss if there are any issues.
> > 
> > > 
> > > 
> > > 
> > > Best regards,
> > > Krzysztof