On Tue, Jun 16, 2020 at 2:29 AM Lukasz Majewski lukma@denx.de wrote:
Hi Jassi,
... a polite ping, Lukasz.
The only excuse for so long lack of my response are my personal issues caused by the covid-19.
Sorry if I came across as pestering you. I hope all is well now.
.....
+#define MAX3420_REG_IOPINS 20 +#define MAX3420_REG_IOPINS2 21 +#define MAX3420_REG_GPINIRQ 22 +#define MAX3420_REG_GPINIEN 23 +#define MAX3420_REG_GPINPOL 24 +#define MAX3420_REG_HIRQ 25 +#define MAX3420_REG_HIEN 26 +#define MAX3420_REG_MODE 27 +#define MAX3420_REG_PERADDR 28 +#define MAX3420_REG_HCTL 29 +#define MAX3420_REG_HXFR 30 +#define MAX3420_REG_HRSL 31
When I do look into drivers/usb/gadget/f_dfu.* the defines are placed in the f_dfu.h file.
One school of thought is to contain all code in one file, especially when no other file should access it -- these defines are very max3420 specific and none else should need these. But I am fine if you want them in a separate file.
+#define field(val, bit) ((val) << (bit))
+#define msleep(a) udelay((a) * 1000)
Aren't those two above already defined in some *.h files?
I replaced msleep with mdelay as Marek suggested. I couldn't find the simple shift op define as field.
+static void spi_wr8_ack(struct max3420_udc *udc, u8 reg, u8 val, int actstat) +{
struct spi_slave *slave = udc->slave;u8 txdata[2];txdata[0] = field(reg, MAX3420_SPI_REG_SHIFT) |field(MAX3420_SPI_DIR_WR,MAX3420_SPI_DIR_SHIFT) |
(actstat ? ACKSTAT : 0);txdata[1] = val;spi_xfer(slave, 2 * 8, txdata, NULL, SPI_XFER_ONCE);+}
2 * 8 -> sizeof(txdata) ?
Sure.
+static void spi_wr8(struct max3420_udc *udc, u8 reg, u8 val) +{
spi_wr8_ack(udc, reg, val, 0);+}
+static void spi_rd_buf(struct max3420_udc *udc, u8 reg, void *buf, u8 len) +{
struct spi_slave *slave = udc->slave;u8 txdata[1];txdata[0] = (field(reg, MAX3420_SPI_REG_SHIFT) |field(MAX3420_SPI_DIR_RD,MAX3420_SPI_DIR_SHIFT)); +
spi_xfer(slave, 1 * 8, txdata, NULL, SPI_XFER_BEGIN);I think that 1 * 8 shall be replaced with just 8.
sizeof(txdata) is more consistent
+static int max3420_udc_start(struct usb_gadget *gadget,
struct usb_gadget_driver *driver)+{
struct max3420_udc *udc = to_udc(gadget);unsigned long flags;udc->driver = driver;udc->remote_wkp = 0;udc->softconnect = true;//if (udc->vbus_active)__max3420_start(udc);Please refactor the code and remove any lines started with //.
ok.
+static int max3420_wakeup(struct usb_gadget *gadget) +{
struct max3420_udc *udc = to_udc(gadget);u8 usbctl;/* Only if wakeup allowed by host */if (!udc->remote_wkp || !udc->suspended)return 0;/* Set Remote-WkUp Signal*/WkUp? -> Wakeup?
Ok.
+static int max3420_irq(struct max3420_udc *udc) +{
/* needed ? */Please make sure if the code is needed or not.
Yup.
+static void max3420_setup_spi(struct max3420_udc *udc) +{
u8 reg[8];spi_claim_bus(udc->slave);/* necessary? */The same as above. If there are any quirks or doubts - please state them in the verbose comment.
The spec doesn't say explicitly but my platform needed these. So I'll keep them until someone comes with a reason not to.
+static int max3420_udc_probe(struct udevice *dev) +{
struct max3420_udc *udc = dev_get_priv(dev);struct udevice *spid;spi_get_bus_and_cs(3, 0, 10000000, SPI_MODE_3,"spi_generic_drv",
NULL, &spid, &udc->slave);IMHO the bus number (3?), CS (0), speed (10000000) and mode (SPI_MODE_3) shall be read from device tree.
Some other setups, which would use this IC, could reuse the code when it is connected to other bus/CS/mode/speed.
Yes, of course.
A few more remarks:
- Is this code ported from the Linux kernel? If yes, please explicitly
state SHA1 and version from which it was ported.
This code has no roots in the kernel.
- Does this particular driver require any extra explanation or
description, which shall be put into ./doc/?
We don't implement any platform specific feature, so it is generic udc driver.
- Please also follow comments from Marek, which have been sent in the
other mail.
Sure.
Thank you!