Re: [PATCH v2 2/4] dt-bindings: reset: Add MT6735 reset bindings

[Yassine Oudjana]

On Fri, May 20 2022 at 10:55:24 +0200, AngeloGioacchino Del Regno 
<angelogioacchino.delregno@xxxxxxxxxxxxx> wrote:
> Il 19/05/22 16:22, Yassine Oudjana ha scritto:
> > From: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>
> >
> > Add reset definitions for Mediatek MT6735 resets provided by
> > infracfg and pericfg.
> >
> > Signed-off-by: Yassine Oudjana <y.oudjana@xxxxxxxxxxxxxx>
> > Acked-by: Rob Herring <robh@xxxxxxxxxx>
> > ---
> >   MAINTAINERS                                   |  2 ++
> >   .../reset/mediatek,mt6735-infracfg.h          | 31 
> > +++++++++++++++++++
> >   .../reset/mediatek,mt6735-pericfg.h           | 31 
> > +++++++++++++++++++
> >   3 files changed, 64 insertions(+)
> >   create mode 100644 
> > include/dt-bindings/reset/mediatek,mt6735-infracfg.h
> >   create mode 100644 
> > include/dt-bindings/reset/mediatek,mt6735-pericfg.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index a59069263cfb..1c0af554a7b6 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12503,6 +12503,8 @@ 
> > F:	include/dt-bindings/clock/mediatek,mt6735-apmixedsys.h
> >   F:	include/dt-bindings/clock/mediatek,mt6735-infracfg.h
> >   F:	include/dt-bindings/clock/mediatek,mt6735-pericfg.h
> >   F:	include/dt-bindings/clock/mediatek,mt6735-topckgen.h
> > +F:	include/dt-bindings/reset/mediatek,mt6735-infracfg.h
> > +F:	include/dt-bindings/reset/mediatek,mt6735-pericfg.h
> >     MEDIATEK MT76 WIRELESS LAN DRIVER
> >   M:	Felix Fietkau <nbd@xxxxxxxx>
>
> ..snip..
>
> > diff --git a/include/dt-bindings/reset/mediatek,mt6735-pericfg.h 
> > b/include/dt-bindings/reset/mediatek,mt6735-pericfg.h
> > new file mode 100644
> > index 000000000000..6cdfaa7ddadf
> > --- /dev/null
> > +++ b/include/dt-bindings/reset/mediatek,mt6735-pericfg.h
> > @@ -0,0 +1,31 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +
> > +#ifndef _DT_BINDINGS_RST_MT6735_PERICFG_H
> > +#define _DT_BINDINGS_RST_MT6735_PERICFG_H
> > +
> > +#define UART0_SW_RST			0
> > +#define UART1_SW_RST			1
> > +#define UART2_SW_RST			2
> > +#define UART3_SW_RST			3
> > +#define UART4_SW_RST			4
>
> where's number 5?
>
> > +#define BTIF_SW_RST			6
> > +#define DISP_PWM_SW_RST			7
> > +#define PWM_SW_RST			8
>
> ...and where's 9?
>
> > +#define AUXADC_SW_RST			10
> > +#define DMA_SW_RST			11
> > +#define IRDA_SW_RST			12
> > +#define IRTX_SW_RST			13
>
> and 14, 15?
>
> > +#define THERM_SW_RST			16
> > +#define MSDC2_SW_RST			17
> > +#define MSDC3_SW_RST			17
>
> MSDC 2 and 3 are both 17?! :-)
>
> > +#define MSDC0_SW_RST			19
> > +#define MSDC1_SW_RST			20
>
> 21?
>
> > +#define I2C0_SW_RST			22
> > +#define I2C1_SW_RST			23
> > +#define I2C2_SW_RST			24
> > +#define I2C3_SW_RST			25
> > +#define USB_SW_RST			28
> > +
>
> and 29-32?
>
> > +#define SPI0_SW_RST			33
> > +
> > +#endif
>
> I have a hunch that you've misunderstood the changes in the resets...
>
> What Rex-BC has done in his reset cleanup is exactly to stop directly
> mapping these to the actual bits that we're using... so the 
> definitions
> in there will simply be sequential, and the actual mapping is done in
> your clk-mt6735-pericfg.c driver.

I did notice that, but reading the documentation in reset.h:

* @rst_idx_map:Pointer to an array containing ids if input argument is 
index.
*		This array is not necessary if our input argument does not mean 
index.

I thought that it wasn't necessary to use it. Thinking
about it now however, I guess that was to maintain compatibility
with old device trees. I'll change it next time.
Maybe a note should be put there to avoid confusion
in the future.

Thanks,
Yassine