[RESEND PATCH] usb: max3420: add the gadget driver

[Jassi Brar]

On Tue, Jun 16, 2020 at 2:29 AM Lukasz Majewski <lukma at 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:
>
> 1. 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.

> 2. 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.

> 3. Please also follow comments from Marek, which have been sent in the
> other mail.
>
Sure.

Thank you!