Hi Linus,

On Wed, Oct 7, 2020 at 11:29 AM Linus Walleij [off-list ref] wrote:

Hi Martin,

thanks for your patch!

thank you for reviewing the whole series!

As noted on the earlier patches I think this should be folded into the
existing XHCI USB driver in drivers/usb/host/xhci-pci.c or, if that
gets messy, as a separate bolt-on, something like
xhci-pci-gpio.[c|h] in the drivers/usb/host/* directory.
You can use a Kconfig symbol for the GPIO portions or not.

OK, I will do that if there are no objections from other developers
I am intending to place the relevant code in xhci-pci-etron.c, similar
to what we already have with xhci-pci-renesas.c

[...]

This should not be necessary. Tie the GPIO state into the PCI device
driver state, possibly using some #ifdefs.

quoted

+static u8 ej1x8_gpio_shift(unsigned int gpio, u8 mask)
+{
+       return (gpio * fls(mask));
+}
+
+static u8 ej1x8_gpio_mask(unsigned int gpio, u8 mask)
+{
+       return mask << ej1x8_gpio_shift(gpio, mask);
+}

This looks a bit like regmap but trying to use regmap for this
would probably be overengineering.

the problem is also the "INIT" register which needs to be set before
writing the registers

Looking at the code I get annoyed that it uses the config space to
manipulate the GPIOs, else you could have used GPIO_GENERIC
but now you can't, how typical.

I think this won't work in practice because of the EJ1X8_GPIO_CTRL for
which we have to read from bits [7:0] but write to bits [23:16]
due to this (and the INIT register as mentioned above) I did not
consider GPIO_GENERIC any further

Other than that the code looks nice, but fold it into the USB
host driver somehow unless there is a compelling argument
as to why not.

will do so, thanks!


Best regards,
Martin