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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Wed, Jun 5, 2024 at 7:19 PM Marek Behún <kabel@kernel.org> 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

...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/bug.h>
> +#include <linux/cleanup.h>
> +#include <linux/device.h>
> +#include <linux/devm-helpers.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mutex.h>
> +#include <linux/sysfs.h>

> +#include <linux/turris-omnia-mcu-interface.h>

As per previous patch.

> +#include <linux/types.h>
> +#include <linux/workqueue.h>
> +#include <asm/unaligned.h>
> +
> +#include "turris-omnia-mcu.h"

...

> +struct omnia_gpio {
> +       u8 cmd, ctl_cmd, bit, ctl_bit, int_bit;
> +       bool has_int;
> +       u16 feat, feat_mask;
> +};

This is hard to follow, please make one field per line.

...

> +#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,                                    \

> +               .has_int = (_int_bit) != -1,                            \

Useless field, you can always perform this check in the code Since you
have int_bit field defined. Filling it with kinda garbage is not an
excuse, you can just define the INVALID_IRQ like many drivers do that
want to use unsigned types.

#define OMNIA_GPIO_INVALID_INT_BIT  0xff

Possibly to have
static inline is_int_bit_valid(struct omnia_gpio *gpio)
{
  return gpio->int_bit != OMNIA_GPIO_INVALID_INT_BIT;
}

...

> +       /*
> +        * If firmware does support the new interrupt API, we may have cached
> +        * the value of a GPIO in the interrupt service routine. If not, read
> +        * the relevant bit now.
> +        */

> +       if (gpio->has_int && test_bit(gpio->int_bit, &mcu->is_cached))

See above.

> +               return test_bit(gpio->int_bit, &mcu->cached);
> +
> +       return omnia_cmd_read_bit(mcu->client, gpio->cmd, BIT(gpio->bit));
> +}


> +       /* assign relevant bits in result */
> +       for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
> +               field = _relevant_field_for_sts_cmd(omnia_gpios[i].cmd,
> +                                                   &sts, &ext_sts, &ext_ctl);

> +               if (field)

Perhaps

  if (!field)
    continue;

?

> +                       __assign_bit(i, bits,
> +                                    test_bit(omnia_gpios[i].bit, field));

This will become one line after the above change.

> +       }
> +
> +       return 0;
> +}

...

> +       int i;

Why signed?

> +       for_each_set_bit(i, mask, ARRAY_SIZE(omnia_gpios)) {
> +               unsigned long *field, *field_mask;
> +               u8 bit = omnia_gpios[i].ctl_bit;
> +
> +               switch (omnia_gpios[i].ctl_cmd) {
> +               case OMNIA_CMD_GENERAL_CONTROL:
> +                       field = &ctl;
> +                       field_mask = &ctl_mask;
> +                       break;
> +               case OMNIA_CMD_EXT_CONTROL:
> +                       field = &ext_ctl;
> +                       field_mask = &ext_ctl_mask;
> +                       break;
> +               default:
> +                       field = field_mask = NULL;
> +                       break;
> +               }

> +               if (field) {

Like the above

  if (!field)
    continue;

> +                       __set_bit(bit, field_mask);
> +                       __assign_bit(bit, field, test_bit(i, bits));
> +               }
> +       }

...

> +       if (!(type & IRQ_TYPE_EDGE_BOTH)) {
> +               dev_err(dev, "irq %u: unsupported type %u\n", d->irq, type);

You have hwirq, no need to dereference again.

> +               return -EINVAL;
> +       }

...

> +/**
> + * omnia_mask_interleave - Interleaves the bytes from @rising and @falling
> + *     @dst: the destination u8 array of interleaved bytes
> + *     @rising: rising mask
> + *     @falling: falling mask

Why so many spaces before @? One is enough.

> + *
> + * Interleaves the little-endian bytes from @rising and @falling words.
> + *
> + * If @rising = (r0, r1, r2, r3) and @falling = (f0, f1, f2, f3), the result is
> + * @dst = (r0, f0, r1, f1, r2, f2, r3, f3).
> + *
> + * The MCU receives an interrupt mask and reports a pending interrupt bitmap in
> + * this interleaved format. The rationale behind this is that the low-indexed
> + * bits are more important - in many cases, the user will be interested only in
> + * interrupts with indexes 0 to 7, and so the system can stop reading after
> + * first 2 bytes (r0, f0), to save time on the slow I2C bus.
> + *
> + * Feel free to remove this function and its inverse, omnia_mask_deinterleave,
> + * and use an appropriate bitmap_*() function once such a function exists.
> + */
> +static void
> +omnia_mask_interleave(u8 *dst, unsigned long rising, unsigned long falling)

But rising and failing should be either u64 or unsigned long *.

> +{
> +       for (int i = 0; i < sizeof(u32); ++i) {

In other cases you use:
1) unsigned
2) post-increment

What makes this one special?

> +               dst[2 * i] = rising >> (8 * i);
> +               dst[2 * i + 1] = falling >> (8 * i);
> +       }
> +}

...

> +/**
> + * omnia_mask_deinterleave - Deinterleaves the bytes into @rising and @falling
> + *     @src: the source u8 array containing the interleaved bytes
> + *     @rising: pointer where to store the rising mask gathered from @src
> + *     @falling: pointer where to store the falling mask gathered from @src
> + *
> + * This is the inverse function to omnia_mask_interleave.
> + */
> +static void omnia_mask_deinterleave(const u8 *src, unsigned long *rising,
> +                                   unsigned long *falling)
> +{
> +       *rising = *falling = 0;
> +
> +       for (int i = 0; i < sizeof(u32); ++i) {
> +               *rising |= src[2 * i] << (8 * i);
> +               *falling |= src[2 * i + 1] << (8 * i);
> +       }
> +}

Same comments as per above function.

-- 
With Best Regards,
Andy Shevchenko