Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230512141755.1712358-1-eblanc@baylibre.com> <20230512141755.1712358-3-eblanc@baylibre.com>
 <ZF5yb4DbVDbfxVU4@surfacebook> <CSNQ2RRG7XDC.164H6P357UHSR@burritosblues>
 <CAHp75VdNgBv5yVTXzDpY3rrF31p=p99cfXdEs0q7m8VmLLJwbg@mail.gmail.com>
 <CSOGQIRNP61G.1Q2A4ZXB43YYQ@burritosblues> <CAHp75Vc-c=VU5Bfy5097z4wm43=bZ4LG83QBYV19YOrC7zSGag@mail.gmail.com>
 <CSOMS0W9MIUG.2MN43QZO9EX7Q@burritosblues>
In-Reply-To: <CSOMS0W9MIUG.2MN43QZO9EX7Q@burritosblues>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 17 May 2023 18:04:41 +0300
Message-ID: <CAHp75VeNvfDqkQZq_ghiv8vb2NaogKqkiFi9i0N3yLgA=ZTDbA@mail.gmail.com>
Subject: Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl
 and GPIOs
To:     Esteban Blanc <eblanc@baylibre.com>
Cc:     linus.walleij@linaro.org, lgirdwood@gmail.com, broonie@kernel.org,
        a.zummo@towertech.it, alexandre.belloni@bootlin.com,
        linux-kernel@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-rtc@vger.kernel.org, jpanis@baylibre.com,
        jneanne@baylibre.com, aseketeli@baylibre.com, sterzik@ti.com,
        u-kumar1@ti.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, May 17, 2023 at 5:43=E2=80=AFPM Esteban Blanc <eblanc@baylibre.com>=
 wrote:
> On Wed May 17, 2023 at 3:51 PM CEST, Andy Shevchenko wrote:
> > On Wed, May 17, 2023 at 12:58=E2=80=AFPM Esteban Blanc <eblanc@baylibre=
.com> wrote:
> > > On Tue May 16, 2023 at 6:48 PM CEST, Andy Shevchenko wrote:
> > > > On Tue, May 16, 2023 at 4:05=E2=80=AFPM Esteban Blanc <eblanc@bayli=
bre.com> wrote:
> > > > > On Fri May 12, 2023 at 7:07 PM CEST,  wrote:
> > > > > > Fri, May 12, 2023 at 04:17:54PM +0200, Esteban Blanc kirjoitti:

...

> > > > > > > -#define TPS6594_REG_GPIOX_CONF(gpio_inst)          (0x31 + (=
gpio_inst))
> > > > > > > +#define TPS6594_REG_GPIO1_CONF                             0=
x31
> > > > > > > +#define TPS6594_REG_GPIOX_CONF(gpio_inst)  (TPS6594_REG_GPIO=
1_CONF + (gpio_inst))
> > > > > >
> > > > > > Why? The original code with parameter 0 will issue the same.
> > > > >
> > > > > I felt that replacing 0x31 with a constant would make the computa=
tion
> > > > > in TPS6594_REG_GPIOX_CONFIG more understandable. What do you thin=
k?
> > > >
> > > > The question is why that register is so special that you need to ha=
ve
> > > > it as a constant explicitly?
> > >
> > > It is not special, it's just the first one of the serie of config
> > > registers. I felt like just having 0x31 without context was a bit wei=
rd
> >
> > I'm not sure I understand what 'context' you are talking about.
> I was trying to convey the fact that 0x31 was representing
> TPS6594_REG_GPIO1_CONF address. This way when looking at
> TPS6594_REG_GPIOX_CONF(...), one will better understand that this macro
> is just about offsetting from the first GPIO_CONF register.

You can add a comment on top of the macro, so anybody can read and see
what this macro is doing.

> > This is pretty normal to have two kind of definitions (depending on the=
 case):
> > 1/
> >
> >   #define FOO_1 ...
> >   #define FOO_2 ...
> >
> > and so on
> >
> > 2/
> >
> >   #define FOO(x)  (... (x) ...)
> >
> > Having a mix of them seems quite unusual.
> I did not know that. I will revert this change for next version then.

Don't get me wrong, it's possible to have, but since it's unusual it
needs to be well justified. In the change you proposed you have
changed that, but I haven't seen where the new definition is used  (in
*.c files).

--=20
With Best Regards,
Andy Shevchenko