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

[Bartosz Golaszewski <brgl@xxxxxxxx>]

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!

> +
> +#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?

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>