Re: [PATCH v4 2/3] pinctrl: tps6594: Add driver for TPS6594 pinctrl and GPIOs

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, May 17, 2023 at 5:43 PM Esteban Blanc <eblanc@xxxxxxxxxxxx> wrote:
> On Wed May 17, 2023 at 3:51 PM CEST, Andy Shevchenko wrote:
> > On Wed, May 17, 2023 at 12:58 PM Esteban Blanc <eblanc@xxxxxxxxxxxx> wrote:
> > > On Tue May 16, 2023 at 6:48 PM CEST, Andy Shevchenko wrote:
> > > > On Tue, May 16, 2023 at 4:05 PM Esteban Blanc <eblanc@xxxxxxxxxxxx> 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                             0x31
> > > > > > > +#define TPS6594_REG_GPIOX_CONF(gpio_inst)  (TPS6594_REG_GPIO1_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 computation
> > > > > in TPS6594_REG_GPIOX_CONFIG more understandable. What do you think?
> > > >
> > > > The question is why that register is so special that you need to have
> > > > 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 weird
> >
> > 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).

-- 
With Best Regards,
Andy Shevchenko