Re: [PATCH 1/3] mfd: Add realtek USB card reader driver

[Roger]

On 01/14/2014 09:04 PM, Lee Jones wrote:
> > From: Roger Tseng <rogerable@xxxxxxxxxxx>
> >
> > Realtek USB card reader provides a channel to transfer command or data to flash
> > memory cards. This driver exports host instances for mmc and memstick subsystems
> > and handles basic works.
> >
> > Signed-off-by: Roger Tseng <rogerable@xxxxxxxxxxx>

[snip]

> > +static int rtsx_usb_bulk_transfer_sglist(struct rtsx_ucr *ucr,
> > +             unsigned int pipe, struct scatterlist *sg, int num_sg,
> > +             unsigned int length, unsigned int *act_len, int timeout)
> > +{
> > +     int ret;
> > +
> > +     dev_dbg(&ucr->pusb_intf->dev, "%s: xfer %u bytes, %d entries\n",
> > +                     __func__, length, num_sg);
> > +     ret = usb_sg_init(&ucr->current_sg, ucr->pusb_dev, pipe, 0,
> > +                     sg, num_sg, length, GFP_NOIO);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ucr->sg_timer.expires = jiffies + msecs_to_jiffies(timeout);
> > +     add_timer(&ucr->sg_timer);
> > +     usb_sg_wait(&ucr->current_sg);
> > +     del_timer(&ucr->sg_timer);
> > +
> > +     if (act_len)
> > +             *act_len = ucr->current_sg.bytes;
> > +
> > +     return ucr->current_sg.status;
> > +}
>
> Code looks fine, but shouldn't this live an a USB driver?
We have studied drivers in usb/storage, the place that most likely to 
put the driver. All of them are for STANDARD usb mass storage 
class(something like USB_CLASS_MASS_STORAGE). But this is not the same 
case. This driver is for our vendor-class device with completely 
different protocol. It is really an USB interfaced flash card command 
converter(or channel) device rather than a real storage. It also has two 
derived modules(rtsx_usb_sdmmc, rtsx_usb_memstick) for other two subsystems.

We also have another driver: rtsx_pcr.c resident in mfd/ for similar 
devices but of PCIe interface. It is nature for us to choose the same 
place for this variant.

[snip]

> > +     }
> > +
> > +     /* set USB interface data */
> > +     usb_set_intfdata(intf, ucr);
> > +
> > +     ucr->vendor_id = id->idVendor;
> > +     ucr->product_id = id->idProduct;
> > +     ucr->cmd_buf = ucr->rsp_buf = ucr->iobuf;
> > +
> > +     mutex_init(&ucr->dev_mutex);
> > +
> > +     ucr->pusb_intf = intf;
> > +
> > +     /* initialize */
> > +     ret = rtsx_usb_init_chip(ucr);
> > +     if (ret)
> > +             goto out_init_fail;
> > +
> > +     for (i = 0; i < ARRAY_SIZE(rtsx_usb_cells); i++) {
> > +             rtsx_usb_cells[i].platform_data = &ucr;
>
> You've already put this in your drvdata (or ntfdata, as it's called
> here). Why do you also need it in platform_data?

Derived modules rtsx_usb_sdmmc and rtsx_usb_memstick will retrieve this 
from platform_data. They will not be aware of usb interface struct.

[snip]

> > +#ifdef CONFIG_PM
> > +static int rtsx_usb_suspend(struct usb_interface *intf, pm_message_t message)
> > +{
> > +     struct rtsx_ucr *ucr =
> > +             (struct rtsx_ucr *)usb_get_intfdata(intf);
> > +
> > +     dev_dbg(&intf->dev, "%s called with pm message 0x%04u\n",
> > +                     __func__, message.event);
> > +
> > +     mutex_lock(&ucr->dev_mutex);
> > +     rtsx_usb_turn_off_led(ucr);
>
> That's it? That's all you do during suspend? :)
Yes. The rest of power things like turning-off card power or clock 
should be taken care by cores of derived modules(mmc, memstick). We put 
only one task here to make sure the LED is off, preventing any module 
from turn it on but doesn't turn off before suspend.
> > +     mutex_unlock(&ucr->dev_mutex);
> > +     return 0;
> > +}
> > +
> > +static int rtsx_usb_resume(struct usb_interface *intf)
> > +{
>
> Don't you want to turn the LED back on here?
>
> Or will that happen automatically when you write/read to/from it again?
Yes, it would blink again during use. The turn-off before suspend is not 
a permanent power off.
> > +     return 0;
> > +}
> > +
> > +static int rtsx_usb_reset_resume(struct usb_interface *intf)
> > +{
> > +     struct rtsx_ucr *ucr =
> > +             (struct rtsx_ucr *)usb_get_intfdata(intf);
> > +
> > +     rtsx_usb_reset_chip(ucr);
> > +     return 0;
> > +}
> > +
> > +#else /* CONFIG_PM */
> > +
> > +#define rtsx_usb_suspend NULL
> > +#define rtsx_usb_resume NULL
> > +#define rtsx_usb_reset_resume NULL
> > +
> > +#endif /* CONFIG_PM */
> > +
> > +
> > +static int rtsx_usb_pre_reset(struct usb_interface *intf)
> > +{
> > +     struct rtsx_ucr *ucr = (struct rtsx_ucr *)usb_get_intfdata(intf);
> > +
> > +     mutex_lock(&ucr->dev_mutex);
>
> Is this normal?

Yes. It is used to prevent commands during port reset. Same to the one 
in usb/storage/usb.c.

[snip]
> > +#include <linux/usb.h>
> > +
> > +/* related module names */
> > +#define RTSX_USB_SD_CARD             0
> > +#define RTSX_USB_MS_CARD             1
> > +
> > +#define DRV_NAME_RTSX_USB            "rtsx_usb"
> > +#define DRV_NAME_RTSX_USB_SDMMC              "rtsx_usb_sdmmc"
> > +#define DRV_NAME_RTSX_USB_MS         "rtsx_usb_ms"
>
> Can you just put the names in the correct places please?
Do you mean just remove these definitions and fill the string directly 
at the using place?

> > +/* endpoint numbers */
> > +#define EP_BULK_OUT          1
> > +#define EP_BULK_IN           2
> > +#define EP_INTR_IN           3
>
> I assume these aren't defined anywhere else?
It should not be defined anywhere else since it is really depends on the 
hardware design but not any general spec.

[snip]
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org â Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
>
> ------Please consider the environment before printing this e-mail.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/