Re: [PATCH v12 3/8] platform: cznic: turris-omnia-mcu: Add support for MCU connected GPIOs

[Marek Behún <kabel@xxxxxxxxxx>]

On Mon, 1 Jul 2024 11:37:44 +0200
Bartosz Golaszewski <brgl@xxxxxxxx> wrote:

> On Mon, Jun 17, 2024 at 5:26 PM Marek Behún <kabel@xxxxxxxxxx> wrote:
> >
> > Add support for GPIOs connected to the MCU on the Turris Omnia board.
> >
> > This includes:
> > - front button pin
> > - enable pins for USB regulators
> > - MiniPCIe / mSATA card presence pins in MiniPCIe port 0
> > - LED output pins from WAN ethernet PHY, LAN switch and MiniPCIe ports
> > - on board revisions 32+ also various peripheral resets and another
> >   voltage regulator enable pin
> >
> > Signed-off-by: Marek Behún <kabel@xxxxxxxxxx>  
> 
> This is one heck of a complex GPIO driver!

The complexity comes from how the MCU command API was engineered and
then when I took over the MCU firmware, I had to extend the command in
a way that would ensure backwards compatibility.

> > +
> > +#define _DEF_GPIO(_cmd, _ctl_cmd, _bit, _ctl_bit, _int_bit, _feat, _feat_mask) \
> > +       {                                                               \
> > +               .cmd = _cmd,                                            \
> > +               .ctl_cmd = _ctl_cmd,                                    \
> > +               .bit = _bit,                                            \
> > +               .ctl_bit = _ctl_bit,                                    \
> > +               .int_bit = (_int_bit) < 0 ? OMNIA_GPIO_INVALID_INT_BIT  \
> > +                                         : (_int_bit),                 \
> > +               .feat = _feat,                                          \
> > +               .feat_mask = _feat_mask,                                \
> > +       }
> > +#define _DEF_GPIO_STS(_name) \
> > +       _DEF_GPIO(OMNIA_CMD_GET_STATUS_WORD, 0, __bf_shf(OMNIA_STS_ ## _name), \
> > +                 0, __bf_shf(OMNIA_INT_ ## _name), 0, 0)
> > +#define _DEF_GPIO_CTL(_name) \
> > +       _DEF_GPIO(OMNIA_CMD_GET_STATUS_WORD, OMNIA_CMD_GENERAL_CONTROL, \
> > +                 __bf_shf(OMNIA_STS_ ## _name), __bf_shf(OMNIA_CTL_ ## _name), \
> > +                 -1, 0, 0)
> > +#define _DEF_GPIO_EXT_STS(_name, _feat) \
> > +       _DEF_GPIO(OMNIA_CMD_GET_EXT_STATUS_DWORD, 0, \
> > +                 __bf_shf(OMNIA_EXT_STS_ ## _name), 0, \
> > +                 __bf_shf(OMNIA_INT_ ## _name), \
> > +                 OMNIA_FEAT_ ## _feat | OMNIA_FEAT_EXT_CMDS, \
> > +                 OMNIA_FEAT_ ## _feat | OMNIA_FEAT_EXT_CMDS)
> > +#define _DEF_GPIO_EXT_STS_LED(_name, _ledext) \
> > +       _DEF_GPIO(OMNIA_CMD_GET_EXT_STATUS_DWORD, 0, \
> > +                 __bf_shf(OMNIA_EXT_STS_ ## _name), 0, \
> > +                 __bf_shf(OMNIA_INT_ ## _name), \
> > +                 OMNIA_FEAT_LED_STATE_ ## _ledext, \
> > +                 OMNIA_FEAT_LED_STATE_EXT_MASK)
> > +#define _DEF_GPIO_EXT_STS_LEDALL(_name) \
> > +       _DEF_GPIO(OMNIA_CMD_GET_EXT_STATUS_DWORD, 0, \
> > +                 __bf_shf(OMNIA_EXT_STS_ ## _name), 0, \
> > +                 __bf_shf(OMNIA_INT_ ## _name), \
> > +                 OMNIA_FEAT_LED_STATE_EXT_MASK, 0)
> > +#define _DEF_GPIO_EXT_CTL(_name, _feat) \
> > +       _DEF_GPIO(OMNIA_CMD_GET_EXT_CONTROL_STATUS, OMNIA_CMD_EXT_CONTROL, \
> > +                 __bf_shf(OMNIA_EXT_CTL_ ## _name), \
> > +                 __bf_shf(OMNIA_EXT_CTL_ ## _name), -1, \
> > +                 OMNIA_FEAT_ ## _feat | OMNIA_FEAT_EXT_CMDS, \
> > +                 OMNIA_FEAT_ ## _feat | OMNIA_FEAT_EXT_CMDS)
> > +#define _DEF_INT(_name) \
> > +       _DEF_GPIO(0, 0, 0, 0, __bf_shf(OMNIA_INT_ ## _name), 0, 0)
> > +  
> 
> One coding-style nit: can you add newlines between these macros?

Sent v13 with this change.

> I'm having a hard-time understanding the entire architecture of this
> MCU but the code overall looks good to me so I trust you know what
> you're doing.
> 
> Acked-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>

Thanks.

Marek