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

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

On Tue, 2025-01-21 at 18:03 +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 Tue, Jan 21, 2025 at 5:58 PM Cathy Xu (许华婷) <
> ot_cathy.xu@mediatek.com> wrote:
> > 
> > On Mon, 2025-01-20 at 13:42 +0100, AngeloGioacchino Del Regno
> > wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > 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.
> > 
> >   Sorry, I think I may not have expressed myself clearly. What I
> > meant
> > is that the attribute 'mediatek,rsel-resistance-in-si-unit' is not
> > supported. In the dts, can write the resistance value, for example:
> > bias-pull-up=<1000>, but can't use 'mediatek,rsel-resistance-in-si-
> > unit
> > = <xxx>'.
> 
> `mediatek,rsel-resistance-in-si-unit` is supported in
> drivers/pinctrl/mediatek/pinctrl-paris.c
> 
> Angelo is requesting that you continue that support and make it
> exclusive, i.e. not support the RSEL macro magic numbers, and
> _only_ support ohm values, in the device tree.
> 
> ChenYu

  Thank you for your response. Can I understand that in next version, I
should remove the RSEL macro magic numbers and add `mediatek,rsel-
resistance-in-si-unit`?
> 
> > > > > > > 
> > > > > > > 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
> > 
> >  Yes, I understand what you mean. However, I was referring to
> > writing
> > the rsel register in the driver, not in dts.
> > 
> > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > +            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_A
> > > > > > DSP_
> > > > > > 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.
> > > >