Re: [PATCH v2 2/2] gpio: Add Realtek Otto GPIO support

[Sander Vanheule <sander@xxxxxxxxxxxxx>]

Hi Andy,

Thanks for the review. I'll address the style comments in a v3. Some
further comments and discussion below.


On Wed, 2021-03-17 at 15:08 +0200, Andy Shevchenko wrote:
> On Mon, Mar 15, 2021 at 11:11 PM Sander Vanheule < 
> sander@xxxxxxxxxxxxx> wrote:
> > +       depends on OF_GPIO
> 
> Don't see how it's used.

It isn't, so I'll remove it.


> > +#include <linux/of_irq.h>
> 
> Why?
> Perhaps what you need is property.h and mod_devicetable.h. See below.

With you suggestions, I was able to drop most explicit OF references.
Only of_device_id remains, for which I'll include mod_devicetable.h.


> > +#include <linux/swab.h>
> 
> Not sure why you need this? See below.

[snip]

> 
> > +
> > +static inline u32 realtek_gpio_isr_read(struct realtek_gpio_ctrl
> > *ctrl)
> > +{
> > +       return swab32(readl(ctrl->base + REALTEK_GPIO_REG_ISR));
> 
> Why swab?! How is this supposed to work on BE CPUs?
> Ditto for all swabXX() usage.

My use of swab32/swahw32 has little to do with the CPU being BE or LE,
but more with the register packing in the GPIO peripheral.

The supported SoCs have port layout A-B-C-D in the registers, where
firmware built with Realtek's SDK always denotes A0 as the first GPIO
line. So bit 24 in a register has the value for A0 (with the exception
of the IMR register).

I wrote these wrapper functions to be able to use the BIT() macro with
the GPIO line number, similar to how gpio-mmio uses ioread32be() when
the BGPIOF_BIG_ENDIAN_BYTE_ORDER flag is used.

For the IMR register, port A again comes first, but is now 16 bits wide
instead of 8, with A0 at bits 16:17. That's why swahw32 is used for
this register.

On the currently unsupported RTL9300-series, the port layout is
reversed: D-C-B-A. GPIO line A0 is then at bit 0, so the swapping
functions won't be required. When support for this alternate port
layout is added, some code will need to be added to differentiate
between the two cases.


> > +}
> > +
> > +static inline void realtek_gpio_isr_clear(struct realtek_gpio_ctrl
> > *ctrl,
> > +       unsigned int pin_mask)
> > +{
> > +       writel(swab32(pin_mask), ctrl->base +
> > REALTEK_GPIO_REG_ISR);
> > +}
> > +
> > +static inline void realtek_gpio_update_imr(struct
> > realtek_gpio_ctrl *ctrl,
> > +       unsigned int imr_offset, u32 type, u32 mask)
> > +{
> > +       unsigned int reg;
> > +
> > +       if (imr_offset == 0)
> > +               reg = REALTEK_GPIO_REG_IMR_AB;
> > +       else
> > +               reg = REALTEK_GPIO_REG_IMR_CD;
> > +       writel(swahw32(type & mask), ctrl->base + reg);
> > +}

[snip]

> > +       switch (flow_type & IRQ_TYPE_SENSE_MASK) {
> 
> > +       case IRQ_TYPE_NONE:
> > +               type = 0;
> > +               handler = handle_bad_irq;
> > +               break;
> 
> Why is it here? Make it default like many other GPIO drivers do.
> 
> > +       case IRQ_TYPE_EDGE_FALLING:
> > +               type = REALTEK_GPIO_IRQ_EDGE_FALLING;
> > +               handler = handle_edge_irq;
> > +               break;
> > +       case IRQ_TYPE_EDGE_RISING:
> > +               type = REALTEK_GPIO_IRQ_EDGE_RISING;
> > +               handler = handle_edge_irq;
> > +               break;
> > +       case IRQ_TYPE_EDGE_BOTH:
> > +               type = REALTEK_GPIO_IRQ_EDGE_BOTH;
> > +               handler = handle_edge_irq;
> > +               break;
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       irq_set_handler_locked(data, handler);
> 
> handler is always the same. Use it directly here.

I'll drop the IRQ_TYPE_NONE case. Do I understand it correctly, that
IRQ_TYPE_NONE should never be used as the new value, but only as the
default initial value?


Best,
Sander