On Fri, Mar 19, 2021 at 5:51 PM Sander Vanheule <sander@xxxxxxxxxxxxx> wrote: > 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: ... > > > +#include <linux/swab.h> > > > > Not sure why you need this? See below. > > > + 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. Yes, you have different endianess on the hardware level, why not to use the proper accessors (with or without utilization of the above mentioned BGPIOF_BIG_ENDIAN_BYTE_ORDER)? ... > > > + case IRQ_TYPE_NONE: > > > + type = 0; > > > + handler = handle_bad_irq; > > > + break; > > > > Why is it here? Make it default like many other GPIO drivers do. > > > + 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? Initially you initialize the default handler to be "bad" (in order to easily catch up issues with IRQ configurations). When ->irq_set_type() is called, if everything is okay it will lock the handler to the proper one. -- With Best Regards, Andy Shevchenko
