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

[Oliver Neukum <oneukum@suse.de>]

On Mon, 2013-12-23 at 17:52 +0800, rogerable@realtek.com wrote:
> From: Roger Tseng <rogerable@realtek.com>
> 
> 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@realtek.com>

> +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,
> +};
> +