On Mon, 2013-12-23 at 17:52 +0800, rogerable@xxxxxxxxxxx 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.
Thank you for writing this driver.
A few remarks about the code and sorry for the delay in reviewing.
> Signed-off-by: Roger Tseng <rogerable@xxxxxxxxxxx>
> +static void rtsx_usb_sg_timed_out(unsigned long data)
> +{
> + struct rtsx_ucr *ucr = (struct rtsx_ucr *)data;
> +
> + dev_dbg(&ucr->pusb_intf->dev, "%s: sg transfer timed out", __func__);
> + usb_sg_cancel(&ucr->current_sg);
> +
> + /* we know the cancellation is caused by time-out */
How do you know? You know it won't complete here, but it may have
completed for another reason.
> + ucr->current_sg.status = -ETIMEDOUT;
> +}
> +static int rtsx_usb_seq_write_register(struct rtsx_ucr *ucr,
> + u16 addr, u16 len, u8 *data)
> +{
> + u16 cmd_len = len + 12;
> +
> + if (data == NULL)
> + return -EINVAL;
> +
> + cmd_len = (cmd_len <= CMD_BUF_LEN) ? cmd_len : CMD_BUF_LEN;
> +
> + if (cmd_len % 4)
> + cmd_len += (4 - cmd_len % 4);
> +
> +
> + ucr->cmd_buf[0] = 'R';
> + ucr->cmd_buf[1] = 'T';
> + ucr->cmd_buf[2] = 'C';
> + ucr->cmd_buf[3] = 'R';
> + ucr->cmd_buf[PACKET_TYPE] = SEQ_WRITE;
> + ucr->cmd_buf[5] = (u8)(len >> 8);
> + ucr->cmd_buf[6] = (u8)len;
Please use the macros the kernel provides.
> + ucr->cmd_buf[STAGE_FLAG] = 0;
> + ucr->cmd_buf[8] = (u8)(addr >> 8);
> + ucr->cmd_buf[9] = (u8)addr;
Likewise.
> +
> + memcpy(ucr->cmd_buf + 12, data, len);
> +
> + return rtsx_usb_transfer_data(ucr,
> + usb_sndbulkpipe(ucr->pusb_dev, EP_BULK_OUT),
> + ucr->cmd_buf, cmd_len, 0, NULL, 100);
> +}
> +int rtsx_usb_send_cmd(struct rtsx_ucr *ucr, u8 flag, int timeout)
> +{
> + int ret;
> +
> + ucr->cmd_buf[CNT_H] = (u8)(ucr->cmd_idx >> 8);
> + ucr->cmd_buf[CNT_L] = (u8)(ucr->cmd_idx);
> + ucr->cmd_buf[STAGE_FLAG] = flag;
> +
> + ret = rtsx_usb_transfer_data(ucr,
> + usb_sndbulkpipe(ucr->pusb_dev, EP_BULK_OUT),
> + ucr->cmd_buf, ucr->cmd_idx * 4 + CMD_OFFSET,
> + 0, NULL, timeout);
> + if (ret) {
Even for fatal errors?
> + /* clear HW error*/
> + rtsx_usb_ep0_write_register(ucr, SFSM_ED, 0xf8, 0xf8);
> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(rtsx_usb_send_cmd);
> +#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);
> + mutex_unlock(&ucr->dev_mutex);
> + return 0;
> +}
> +
> +static int rtsx_usb_resume(struct usb_interface *intf)
> +{
> + return 0;
Don't you want to turn the LED back on?
> +}
> +static struct usb_driver rtsx_usb_driver = {
> + .name = DRV_NAME_RTSX_USB,
> + .probe = rtsx_usb_probe,
> + .disconnect = rtsx_usb_disconnect,
> + .suspend = rtsx_usb_suspend,
> + .resume = rtsx_usb_resume,
> + .reset_resume = rtsx_usb_reset_resume,
> + .pre_reset = rtsx_usb_pre_reset,
> + .post_reset = rtsx_usb_post_reset,
> + .id_table = rtsx_usb_usb_ids,
> + .supports_autosuspend = 1,
This is good, but what do you need remote wakeup for?
> + .soft_unbind = 1,
> +};
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
