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

[Greg Kroah-Hartman]

On Mon, Jan 20, 2014 at 04:55:52PM +0800, Roger wrote:
> On 01/16/2014 05:35 PM, Lee Jones wrote:
> +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(>pusb_intf->dev, "%s: xfer %u bytes, %d entries\n",
> + __func__, length, num_sg);
> + ret = usb_sg_init(>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(>sg_timer);
> + usb_sg_wait(>current_sg);
> + del_timer(>sg_timer);
> +
> + if (act_len)
> + *act_len = ucr->current_sg.bytes;
> +
> + return ucr->current_sg.status;
> +}
> >>>
> >>>Code looks fine, but shouldn't this live in 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.
> >
> >Very well, as long as it truly does not fit in drivers/usb. It would
> >be good to have one of the USB folk give the nod though.
> >
> I found Greg K-H is exactly one of the maintainers of USB subsystem as I
> read the MAINTAINERS document.
> 
> Greg, would you please comment this subsystem concern or introduce other
> members? Thanks.

It's the middle of the merge window, nothing can happen until after
3.14-rc1 is out.  So how about resending all of this in two weeks after
that happens so we can all properly discuss it?

As for where the driver should be located, if it shares logic with the
usb storage driver, then it should be in drivers/usb/storage, otherwise
put it wherever makes most sense, there's no need to put all USB drivers
under drivers/usb/ at all.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Roger]

On 01/16/2014 05:35 PM, Lee Jones wrote:

+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(>pusb_intf->dev, "%s: xfer %u bytes, %d entries\n",
+ __func__, length, num_sg);
+ ret = usb_sg_init(>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(>sg_timer);
+ usb_sg_wait(>current_sg);
+ del_timer(>sg_timer);
+
+ if (act_len)
+ *act_len = ucr->current_sg.bytes;
+
+ return ucr->current_sg.status;
+}


Code looks fine, but shouldn't this live in 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.


Very well, as long as it truly does not fit in drivers/usb. It would
be good to have one of the USB folk give the nod though.

I found Greg K-H is exactly one of the maintainers of USB subsystem as I 
read the MAINTAINERS document.


Greg, would you please comment this subsystem concern or introduce 
other members? Thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Greg Kroah-Hartman]

On Mon, Jan 20, 2014 at 04:55:52PM +0800, Roger wrote:
 On 01/16/2014 05:35 PM, Lee Jones wrote:
 +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 in 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.
 
 Very well, as long as it truly does not fit in drivers/usb. It would
 be good to have one of the USB folk give the nod though.
 
 I found Greg K-H is exactly one of the maintainers of USB subsystem as I
 read the MAINTAINERS document.
 
 Greg, would you please comment this subsystem concern or introduce other
 members? Thanks.

It's the middle of the merge window, nothing can happen until after
3.14-rc1 is out.  So how about resending all of this in two weeks after
that happens so we can all properly discuss it?

As for where the driver should be located, if it shares logic with the
usb storage driver, then it should be in drivers/usb/storage, otherwise
put it wherever makes most sense, there's no need to put all USB drivers
under drivers/usb/ at all.

thanks,

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Roger]

On 01/16/2014 05:35 PM, Lee Jones wrote:

+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 in 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.


Very well, as long as it truly does not fit in drivers/usb. It would
be good to have one of the USB folk give the nod though.

I found Greg K-H is exactly one of the maintainers of USB subsystem as I 
read the MAINTAINERS document.


Greg, would you please comment this subsystem concern or introduce 
other members? Thanks.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Lee Jones]

> >>+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(>pusb_intf->dev, "%s: xfer %u bytes, %d entries\n",
> >>+ __func__, length, num_sg);
> >>+ ret = usb_sg_init(>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(>sg_timer);
> >>+ usb_sg_wait(>current_sg);
> >>+ del_timer(>sg_timer);
> >>+
> >>+ if (act_len)
> >>+ *act_len = ucr->current_sg.bytes;
> >>+
> >>+ return ucr->current_sg.status;
> >>+}
> >
> >Code looks fine, but shouldn't this live in 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.

Very well, as long as it truly does not fit in drivers/usb. It would
be good to have one of the USB folk give the nod though.

> >>+ }
> >>+
> >>+ /* 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(>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 = 
> >
> >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.

They don't need to. If the MMC or Memstick subsystems do not have
their own helpers you can just use something like:

  struct rtsx_ucr *ucr = dev_get_drvdata(pdev->dev.parent);

Naturally this is untested code and might not work off the bat, but it
does give you some idea of what you can do without iterating through
and populating each sub-device's platform data.

> >>+#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?

I do.

I find these types of defines unhelpful and somewhat obfuscating.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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 

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 


[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(>pusb_intf->dev, "%s: xfer %u bytes, %d entries\n",
+ __func__, length, num_sg);
+ ret = usb_sg_init(>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(>sg_timer);
+ usb_sg_wait(>current_sg);
+ del_timer(>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(>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 = 


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(>dev, "%s called with pm message 0x%04u\n",
+ __func__, message.event);
+
+ mutex_lock(>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(>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(>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 
+
+/* 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 

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 rogera...@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.

Signed-off-by: Roger Tseng rogera...@realtek.com


[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_USBrtsx_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 

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

[Lee Jones]

 +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 in 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.

Very well, as long as it truly does not fit in drivers/usb. It would
be good to have one of the USB folk give the nod though.

 + }
 +
 + /* 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.

They don't need to. If the MMC or Memstick subsystems do not have
their own helpers you can just use something like:

  struct rtsx_ucr *ucr = dev_get_drvdata(pdev-dev.parent);

Naturally this is untested code and might not work off the bat, but it
does give you some idea of what you can do without iterating through
and populating each sub-device's platform data.

 +#define DRV_NAME_RTSX_USBrtsx_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?

I do.

I find these types of defines unhelpful and somewhat obfuscating.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Greg Kroah-Hartman]

On Tue, Jan 14, 2014 at 03:47:34PM +0800, rogera...@realtek.com wrote:
> From: Roger Tseng 
> 
> 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 
> ---
>  drivers/mfd/Kconfig  |  10 +
>  drivers/mfd/Makefile |   1 +
>  drivers/mfd/rtsx_usb.c   | 788 
> +++
>  include/linux/mfd/rtsx_usb.h | 619 +
>  4 files changed, 1418 insertions(+)
>  create mode 100644 drivers/mfd/rtsx_usb.c
>  create mode 100644 include/linux/mfd/rtsx_usb.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b7c74a7..4c99ebd 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -507,6 +507,16 @@ config MFD_RTSX_PCI
> types of memory cards, such as Memory Stick, Memory Stick Pro,
> Secure Digital and MultiMediaCard.
>  
> +config MFD_RTSX_USB
> + tristate "Realtek USB card reader"
> + depends on USB
> + select MFD_CORE
> + help
> + Select this option to get support for Realtek USB 2.0 card readers
> + including RTS5129, RTS5139, RTS5179 and RTS5170.
> + Realtek card reader supports access to many types of memory cards,
> + such as Memory Stick Pro, Secure Digital and MultiMediaCard.
> +
>  config MFD_RC5T583
>   bool "Ricoh RC5T583 Power Management system device"
>   depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 8a28dc9..33b8de6 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_MFD_CROS_EC_SPI)   += cros_ec_spi.o
>  
>  rtsx_pci-objs:= rtsx_pcr.o rts5209.o rts5229.o 
> rtl8411.o rts5227.o rts5249.o
>  obj-$(CONFIG_MFD_RTSX_PCI)   += rtsx_pci.o
> +obj-$(CONFIG_MFD_RTSX_USB)   += rtsx_usb.o
>  
>  obj-$(CONFIG_HTC_EGPIO)  += htc-egpio.o
>  obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o
> diff --git a/drivers/mfd/rtsx_usb.c b/drivers/mfd/rtsx_usb.c
> new file mode 100644
> index 000..905ec8d
> --- /dev/null
> +++ b/drivers/mfd/rtsx_usb.c
> @@ -0,0 +1,788 @@
> +/* Driver for Realtek USB card reader
> + *
> + * Copyright(c) 2009-2013 Realtek Semiconductor Corp. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2, or (at your option) any
> + * later version.

Do you really mean "any later version"?  (sorry, I have to ask.)

Same goes for the other files you add with this specific license
wording.

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

Why is this file neded?

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Lee Jones]

> [ Sorry, I am coming down with the flu today so I'm doing dorky things
>   like reviewing review comments.  I'm not sure how coherent I am.  ]

Always welcome.

NB: I did this review in double-quick time, which may account for some
of the weird thought processes (or lack there of).

> > > +static void rtsx_usb_sg_timed_out(unsigned long data)
> > > +{
> > > + struct rtsx_ucr *ucr = (struct rtsx_ucr *)data;
> > 
> > What's going to happen when your device runs 64bit?
> > 
> 
> I'm not sure I understand what you mean here.  On linux sizeof(long) is
> always the same as sizeof(void *).

I had an odd moment where I thought we'd need long long for 64bit.

Nevermind.

> > > + if (cmd_len > IOBUF_SIZE)
> > > + return -EINVAL;
> > > +
> > > + if (cmd_len % 4)
> > > + cmd_len += (4 - cmd_len % 4);
> > 
> > Please document in a comment.
> 
> There is a kernel macro for this:
> 
>   cmd_len = ALIGN(cmd_len, 4);
> 
>   if (cmd_len > IOBUF_SIZE)
>   return -EINVAL;

I had a feeling this was the intention, hence why I was asking for a
comment. But yes, if all this is doing is alignment then the kernel
macro is preferred.

> > > +
> > > +
> > 
> > Extra '/n'
> > 
> 
> It weirds me out when you mix up '\n' and /n'.

Typos happen on occasion...

> > > +int rtsx_usb_ep0_write_register(struct rtsx_ucr *ucr, u16 addr,
> > > + u8 mask, u8 data)
> > > +{
> > > + u16 value = 0, index = 0;
> > > +
> > > + value |= 0x03 << 14;
> > > + value |= addr & 0x3FFF;
> > > + value = ((value << 8) & 0xFF00) | ((value >> 8) & 0x00FF);
> > > + index |= (u16)mask;
> > > + index |= (u16)data << 8;
> > 
> > Lots of random numbers here, please #define for clarity and ease of
> > reading.
> > 
> 
> The only really random number is the 0x03, but yeah, it would help if
> that we a define.

See ---^

As I say, I just glossed over the code, thus didn't spend the time to
work out the arithmetic. Now I do, most of it is pretty self
explanatory. I would also like to see the SHIFT #defined, although
this may become superfluous once the 0x03 is clarified.

>   addr |= 0x03 << 14;
> 
>   value = __swab16(addr);
>   index = mask | (data << 8);

This is 100% better/clearer.

> > > +
> > > + dev_dbg(>dev,
> > > + ": Realtek USB Card Reader found at bus %03d address %03d\n",
> > > +  usb_dev->bus->busnum, usb_dev->devnum);
> > > +
> > > + ucr = kzalloc(sizeof(struct rtsx_ucr), GFP_KERNEL);
> > 
> > s/struct rtsx_ucr/*ucr/
> > 
> > Any reason for not using managed resources?
> > 
> Roger, he means the devm_kzalloc().

That I do.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Dan Carpenter]

On Tue, Jan 14, 2014 at 04:46:34PM +0300, Dan Carpenter wrote:
> [ Sorry, I am coming down with the flu today so I'm doing dorky things
>   like reviewing review comments.  I'm not sure how coherent I am.  ]
> 
> On Tue, Jan 14, 2014 at 01:04:09PM +, Lee Jones wrote:
>  
> > > +static void rtsx_usb_sg_timed_out(unsigned long data)
> > > +{
> > > + struct rtsx_ucr *ucr = (struct rtsx_ucr *)data;
> > 
> > What's going to happen when your device runs 64bit?
> > 
> 
> I'm not sure I understand what you mean here.  On linux sizeof(long) is
> always the same as sizeof(void *).
> 
> 
> > > + if (cmd_len > IOBUF_SIZE)
> > > + return -EINVAL;
> > > +
> > > + if (cmd_len % 4)
> > > + cmd_len += (4 - cmd_len % 4);
> > 
> > Please document in a comment.
> 
> There is a kernel macro for this:
> 
>   cmd_len = ALIGN(cmd_len, 4);
> 

Btw, what's the difference between ALIGN(cmd_len, 4) and
round_up(cmd_len, 4)?  Maybe round_up() is better.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Dan Carpenter]

[ Sorry, I am coming down with the flu today so I'm doing dorky things
  like reviewing review comments.  I'm not sure how coherent I am.  ]

On Tue, Jan 14, 2014 at 01:04:09PM +, Lee Jones wrote:
 
> > +static void rtsx_usb_sg_timed_out(unsigned long data)
> > +{
> > +   struct rtsx_ucr *ucr = (struct rtsx_ucr *)data;
> 
> What's going to happen when your device runs 64bit?
> 

I'm not sure I understand what you mean here.  On linux sizeof(long) is
always the same as sizeof(void *).


> > +   if (cmd_len > IOBUF_SIZE)
> > +   return -EINVAL;
> > +
> > +   if (cmd_len % 4)
> > +   cmd_len += (4 - cmd_len % 4);
> 
> Please document in a comment.

There is a kernel macro for this:

cmd_len = ALIGN(cmd_len, 4);

if (cmd_len > IOBUF_SIZE)
return -EINVAL;

> 
> > +
> > +
> 
> Extra '/n'
> 

It weirds me out when you mix up '\n' and /n'.

> > +int rtsx_usb_ep0_write_register(struct rtsx_ucr *ucr, u16 addr,
> > +   u8 mask, u8 data)
> > +{
> > +   u16 value = 0, index = 0;
> > +
> > +   value |= 0x03 << 14;
> > +   value |= addr & 0x3FFF;
> > +   value = ((value << 8) & 0xFF00) | ((value >> 8) & 0x00FF);
> > +   index |= (u16)mask;
> > +   index |= (u16)data << 8;
> 
> Lots of random numbers here, please #define for clarity and ease of
> reading.
> 

The only really random number is the 0x03, but yeah, it would help if
that we a define.

addr |= 0x03 << 14;

value = __swab16(addr);
index = mask | (data << 8);

> > +
> > +   dev_dbg(>dev,
> > +   ": Realtek USB Card Reader found at bus %03d address %03d\n",
> > +usb_dev->bus->busnum, usb_dev->devnum);
> > +
> > +   ucr = kzalloc(sizeof(struct rtsx_ucr), GFP_KERNEL);
> 
> s/struct rtsx_ucr/*ucr/
> 
> Any reason for not using managed resources?
> 

Roger, he means the devm_kzalloc().

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Lee Jones]

> From: Roger Tseng 
> 
> 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 
> ---
>  drivers/mfd/Kconfig  |  10 +
>  drivers/mfd/Makefile |   1 +
>  drivers/mfd/rtsx_usb.c   | 788 
> +++
>  include/linux/mfd/rtsx_usb.h | 619 +
>  4 files changed, 1418 insertions(+)
>  create mode 100644 drivers/mfd/rtsx_usb.c
>  create mode 100644 include/linux/mfd/rtsx_usb.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b7c74a7..4c99ebd 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -507,6 +507,16 @@ config MFD_RTSX_PCI
> types of memory cards, such as Memory Stick, Memory Stick Pro,
> Secure Digital and MultiMediaCard.
>  
> +config MFD_RTSX_USB
> + tristate "Realtek USB card reader"
> + depends on USB
> + select MFD_CORE
> + help
> + Select this option to get support for Realtek USB 2.0 card readers
> + including RTS5129, RTS5139, RTS5179 and RTS5170.
> + Realtek card reader supports access to many types of memory cards,
> + such as Memory Stick Pro, Secure Digital and MultiMediaCard.
> +

The help section should be indented by 2 spaces.

>  config MFD_RC5T583
>   bool "Ricoh RC5T583 Power Management system device"
>   depends on I2C=y
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 8a28dc9..33b8de6 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_MFD_CROS_EC_SPI)   += cros_ec_spi.o
>  
>  rtsx_pci-objs:= rtsx_pcr.o rts5209.o rts5229.o 
> rtl8411.o rts5227.o rts5249.o
>  obj-$(CONFIG_MFD_RTSX_PCI)   += rtsx_pci.o
> +obj-$(CONFIG_MFD_RTSX_USB)   += rtsx_usb.o
>  
>  obj-$(CONFIG_HTC_EGPIO)  += htc-egpio.o
>  obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o
> diff --git a/drivers/mfd/rtsx_usb.c b/drivers/mfd/rtsx_usb.c
> new file mode 100644
> index 000..905ec8d
> --- /dev/null
> +++ b/drivers/mfd/rtsx_usb.c
> @@ -0,0 +1,788 @@
> +/* Driver for Realtek USB card reader
> + *
> + * Copyright(c) 2009-2013 Realtek Semiconductor Corp. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2, or (at your option) any
> + * later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see .
> + *
> + * Author:
> + *   Roger Tseng 
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static int polling_pipe = 1;
> +module_param(polling_pipe, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(polling_pipe, "polling pipe (0: ctl, 1: bulk)");

'/n' here.

> +static struct mfd_cell rtsx_usb_cells[] = {
> + [RTSX_USB_SD_CARD] = {
> + .name = DRV_NAME_RTSX_USB_SDMMC,
> + },
> + [RTSX_USB_MS_CARD] = {
> + .name = DRV_NAME_RTSX_USB_MS,
> + },
> +};

I'm not entirely convinced that this is an MFD device?

> +static void rtsx_usb_sg_timed_out(unsigned long data)
> +{
> + struct rtsx_ucr *ucr = (struct rtsx_ucr *)data;

What's going to happen when your device runs 64bit?

> + dev_dbg(>pusb_intf->dev, "%s: sg transfer timed out", __func__);
> + usb_sg_cancel(>current_sg);

Are you sure this needs to live here?

Why isn't this in drivers/usb?

> + /* we know the cancellation is caused by time-out */
> + ucr->current_sg.status = -ETIMEDOUT;
> +}
> +
> +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(>pusb_intf->dev, "%s: xfer %u bytes, %d entries\n",
> + __func__, length, num_sg);
> + ret = usb_sg_init(>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(>sg_timer);
> + usb_sg_wait(>current_sg);
> + del_timer(>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?

> +int 

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

[Lee Jones]

 From: Roger Tseng rogera...@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.
 
 Signed-off-by: Roger Tseng rogera...@realtek.com
 ---
  drivers/mfd/Kconfig  |  10 +
  drivers/mfd/Makefile |   1 +
  drivers/mfd/rtsx_usb.c   | 788 
 +++
  include/linux/mfd/rtsx_usb.h | 619 +
  4 files changed, 1418 insertions(+)
  create mode 100644 drivers/mfd/rtsx_usb.c
  create mode 100644 include/linux/mfd/rtsx_usb.h
 
 diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
 index b7c74a7..4c99ebd 100644
 --- a/drivers/mfd/Kconfig
 +++ b/drivers/mfd/Kconfig
 @@ -507,6 +507,16 @@ config MFD_RTSX_PCI
 types of memory cards, such as Memory Stick, Memory Stick Pro,
 Secure Digital and MultiMediaCard.
  
 +config MFD_RTSX_USB
 + tristate Realtek USB card reader
 + depends on USB
 + select MFD_CORE
 + help
 + Select this option to get support for Realtek USB 2.0 card readers
 + including RTS5129, RTS5139, RTS5179 and RTS5170.
 + Realtek card reader supports access to many types of memory cards,
 + such as Memory Stick Pro, Secure Digital and MultiMediaCard.
 +

The help section should be indented by 2 spaces.

  config MFD_RC5T583
   bool Ricoh RC5T583 Power Management system device
   depends on I2C=y
 diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
 index 8a28dc9..33b8de6 100644
 --- a/drivers/mfd/Makefile
 +++ b/drivers/mfd/Makefile
 @@ -14,6 +14,7 @@ obj-$(CONFIG_MFD_CROS_EC_SPI)   += cros_ec_spi.o
  
  rtsx_pci-objs:= rtsx_pcr.o rts5209.o rts5229.o 
 rtl8411.o rts5227.o rts5249.o
  obj-$(CONFIG_MFD_RTSX_PCI)   += rtsx_pci.o
 +obj-$(CONFIG_MFD_RTSX_USB)   += rtsx_usb.o
  
  obj-$(CONFIG_HTC_EGPIO)  += htc-egpio.o
  obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o
 diff --git a/drivers/mfd/rtsx_usb.c b/drivers/mfd/rtsx_usb.c
 new file mode 100644
 index 000..905ec8d
 --- /dev/null
 +++ b/drivers/mfd/rtsx_usb.c
 @@ -0,0 +1,788 @@
 +/* Driver for Realtek USB card reader
 + *
 + * Copyright(c) 2009-2013 Realtek Semiconductor Corp. All rights reserved.
 + *
 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms of the GNU General Public License as published by the
 + * Free Software Foundation; either version 2, or (at your option) any
 + * later version.
 + *
 + * This program is distributed in the hope that it will be useful, but
 + * WITHOUT ANY WARRANTY; without even the implied warranty of
 + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 + * General Public License for more details.
 + *
 + * You should have received a copy of the GNU General Public License along
 + * with this program; if not, see http://www.gnu.org/licenses/.
 + *
 + * Author:
 + *   Roger Tseng rogera...@realtek.com
 + */
 +#include linux/module.h
 +#include linux/slab.h
 +#include linux/mutex.h
 +#include linux/usb.h
 +#include linux/platform_device.h
 +#include linux/mfd/core.h
 +#include asm/unaligned.h
 +#include linux/mfd/rtsx_usb.h
 +
 +static int polling_pipe = 1;
 +module_param(polling_pipe, int, S_IRUGO | S_IWUSR);
 +MODULE_PARM_DESC(polling_pipe, polling pipe (0: ctl, 1: bulk));

'/n' here.

 +static struct mfd_cell rtsx_usb_cells[] = {
 + [RTSX_USB_SD_CARD] = {
 + .name = DRV_NAME_RTSX_USB_SDMMC,
 + },
 + [RTSX_USB_MS_CARD] = {
 + .name = DRV_NAME_RTSX_USB_MS,
 + },
 +};

I'm not entirely convinced that this is an MFD device?

 +static void rtsx_usb_sg_timed_out(unsigned long data)
 +{
 + struct rtsx_ucr *ucr = (struct rtsx_ucr *)data;

What's going to happen when your device runs 64bit?

 + dev_dbg(ucr-pusb_intf-dev, %s: sg transfer timed out, __func__);
 + usb_sg_cancel(ucr-current_sg);

Are you sure this needs to live here?

Why isn't this in drivers/usb?

 + /* we know the cancellation is caused by time-out */
 + ucr-current_sg.status = -ETIMEDOUT;
 +}
 +
 +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 

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

[Dan Carpenter]

[ Sorry, I am coming down with the flu today so I'm doing dorky things
  like reviewing review comments.  I'm not sure how coherent I am.  ]

On Tue, Jan 14, 2014 at 01:04:09PM +, Lee Jones wrote:
 
  +static void rtsx_usb_sg_timed_out(unsigned long data)
  +{
  +   struct rtsx_ucr *ucr = (struct rtsx_ucr *)data;
 
 What's going to happen when your device runs 64bit?
 

I'm not sure I understand what you mean here.  On linux sizeof(long) is
always the same as sizeof(void *).


  +   if (cmd_len  IOBUF_SIZE)
  +   return -EINVAL;
  +
  +   if (cmd_len % 4)
  +   cmd_len += (4 - cmd_len % 4);
 
 Please document in a comment.

There is a kernel macro for this:

cmd_len = ALIGN(cmd_len, 4);

if (cmd_len  IOBUF_SIZE)
return -EINVAL;

 
  +
  +
 
 Extra '/n'
 

It weirds me out when you mix up '\n' and /n'.

  +int rtsx_usb_ep0_write_register(struct rtsx_ucr *ucr, u16 addr,
  +   u8 mask, u8 data)
  +{
  +   u16 value = 0, index = 0;
  +
  +   value |= 0x03  14;
  +   value |= addr  0x3FFF;
  +   value = ((value  8)  0xFF00) | ((value  8)  0x00FF);
  +   index |= (u16)mask;
  +   index |= (u16)data  8;
 
 Lots of random numbers here, please #define for clarity and ease of
 reading.
 

The only really random number is the 0x03, but yeah, it would help if
that we a define.

addr |= 0x03  14;

value = __swab16(addr);
index = mask | (data  8);

  +
  +   dev_dbg(intf-dev,
  +   : Realtek USB Card Reader found at bus %03d address %03d\n,
  +usb_dev-bus-busnum, usb_dev-devnum);
  +
  +   ucr = kzalloc(sizeof(struct rtsx_ucr), GFP_KERNEL);
 
 s/struct rtsx_ucr/*ucr/
 
 Any reason for not using managed resources?
 

Roger, he means the devm_kzalloc().

regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Dan Carpenter]

On Tue, Jan 14, 2014 at 04:46:34PM +0300, Dan Carpenter wrote:
 [ Sorry, I am coming down with the flu today so I'm doing dorky things
   like reviewing review comments.  I'm not sure how coherent I am.  ]
 
 On Tue, Jan 14, 2014 at 01:04:09PM +, Lee Jones wrote:
  
   +static void rtsx_usb_sg_timed_out(unsigned long data)
   +{
   + struct rtsx_ucr *ucr = (struct rtsx_ucr *)data;
  
  What's going to happen when your device runs 64bit?
  
 
 I'm not sure I understand what you mean here.  On linux sizeof(long) is
 always the same as sizeof(void *).
 
 
   + if (cmd_len  IOBUF_SIZE)
   + return -EINVAL;
   +
   + if (cmd_len % 4)
   + cmd_len += (4 - cmd_len % 4);
  
  Please document in a comment.
 
 There is a kernel macro for this:
 
   cmd_len = ALIGN(cmd_len, 4);
 

Btw, what's the difference between ALIGN(cmd_len, 4) and
round_up(cmd_len, 4)?  Maybe round_up() is better.

regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Lee Jones]

 [ Sorry, I am coming down with the flu today so I'm doing dorky things
   like reviewing review comments.  I'm not sure how coherent I am.  ]

Always welcome.

NB: I did this review in double-quick time, which may account for some
of the weird thought processes (or lack there of).

   +static void rtsx_usb_sg_timed_out(unsigned long data)
   +{
   + struct rtsx_ucr *ucr = (struct rtsx_ucr *)data;
  
  What's going to happen when your device runs 64bit?
  
 
 I'm not sure I understand what you mean here.  On linux sizeof(long) is
 always the same as sizeof(void *).

I had an odd moment where I thought we'd need long long for 64bit.

Nevermind.

   + if (cmd_len  IOBUF_SIZE)
   + return -EINVAL;
   +
   + if (cmd_len % 4)
   + cmd_len += (4 - cmd_len % 4);
  
  Please document in a comment.
 
 There is a kernel macro for this:
 
   cmd_len = ALIGN(cmd_len, 4);
 
   if (cmd_len  IOBUF_SIZE)
   return -EINVAL;

I had a feeling this was the intention, hence why I was asking for a
comment. But yes, if all this is doing is alignment then the kernel
macro is preferred.

   +
   +
  
  Extra '/n'
  
 
 It weirds me out when you mix up '\n' and /n'.

Typos happen on occasion...

   +int rtsx_usb_ep0_write_register(struct rtsx_ucr *ucr, u16 addr,
   + u8 mask, u8 data)
   +{
   + u16 value = 0, index = 0;
   +
   + value |= 0x03  14;
   + value |= addr  0x3FFF;
   + value = ((value  8)  0xFF00) | ((value  8)  0x00FF);
   + index |= (u16)mask;
   + index |= (u16)data  8;
  
  Lots of random numbers here, please #define for clarity and ease of
  reading.
  
 
 The only really random number is the 0x03, but yeah, it would help if
 that we a define.

See ---^

As I say, I just glossed over the code, thus didn't spend the time to
work out the arithmetic. Now I do, most of it is pretty self
explanatory. I would also like to see the SHIFT #defined, although
this may become superfluous once the 0x03 is clarified.

   addr |= 0x03  14;
 
   value = __swab16(addr);
   index = mask | (data  8);

This is 100% better/clearer.

   +
   + dev_dbg(intf-dev,
   + : Realtek USB Card Reader found at bus %03d address %03d\n,
   +  usb_dev-bus-busnum, usb_dev-devnum);
   +
   + ucr = kzalloc(sizeof(struct rtsx_ucr), GFP_KERNEL);
  
  s/struct rtsx_ucr/*ucr/
  
  Any reason for not using managed resources?
  
 Roger, he means the devm_kzalloc().

That I do.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Greg Kroah-Hartman]

On Tue, Jan 14, 2014 at 03:47:34PM +0800, rogera...@realtek.com wrote:
 From: Roger Tseng rogera...@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.
 
 Signed-off-by: Roger Tseng rogera...@realtek.com
 ---
  drivers/mfd/Kconfig  |  10 +
  drivers/mfd/Makefile |   1 +
  drivers/mfd/rtsx_usb.c   | 788 
 +++
  include/linux/mfd/rtsx_usb.h | 619 +
  4 files changed, 1418 insertions(+)
  create mode 100644 drivers/mfd/rtsx_usb.c
  create mode 100644 include/linux/mfd/rtsx_usb.h
 
 diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
 index b7c74a7..4c99ebd 100644
 --- a/drivers/mfd/Kconfig
 +++ b/drivers/mfd/Kconfig
 @@ -507,6 +507,16 @@ config MFD_RTSX_PCI
 types of memory cards, such as Memory Stick, Memory Stick Pro,
 Secure Digital and MultiMediaCard.
  
 +config MFD_RTSX_USB
 + tristate Realtek USB card reader
 + depends on USB
 + select MFD_CORE
 + help
 + Select this option to get support for Realtek USB 2.0 card readers
 + including RTS5129, RTS5139, RTS5179 and RTS5170.
 + Realtek card reader supports access to many types of memory cards,
 + such as Memory Stick Pro, Secure Digital and MultiMediaCard.
 +
  config MFD_RC5T583
   bool Ricoh RC5T583 Power Management system device
   depends on I2C=y
 diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
 index 8a28dc9..33b8de6 100644
 --- a/drivers/mfd/Makefile
 +++ b/drivers/mfd/Makefile
 @@ -14,6 +14,7 @@ obj-$(CONFIG_MFD_CROS_EC_SPI)   += cros_ec_spi.o
  
  rtsx_pci-objs:= rtsx_pcr.o rts5209.o rts5229.o 
 rtl8411.o rts5227.o rts5249.o
  obj-$(CONFIG_MFD_RTSX_PCI)   += rtsx_pci.o
 +obj-$(CONFIG_MFD_RTSX_USB)   += rtsx_usb.o
  
  obj-$(CONFIG_HTC_EGPIO)  += htc-egpio.o
  obj-$(CONFIG_HTC_PASIC3) += htc-pasic3.o
 diff --git a/drivers/mfd/rtsx_usb.c b/drivers/mfd/rtsx_usb.c
 new file mode 100644
 index 000..905ec8d
 --- /dev/null
 +++ b/drivers/mfd/rtsx_usb.c
 @@ -0,0 +1,788 @@
 +/* Driver for Realtek USB card reader
 + *
 + * Copyright(c) 2009-2013 Realtek Semiconductor Corp. All rights reserved.
 + *
 + * This program is free software; you can redistribute it and/or modify it
 + * under the terms of the GNU General Public License as published by the
 + * Free Software Foundation; either version 2, or (at your option) any
 + * later version.

Do you really mean any later version?  (sorry, I have to ask.)

Same goes for the other files you add with this specific license
wording.

 +#include linux/module.h
 +#include linux/slab.h
 +#include linux/mutex.h
 +#include linux/usb.h
 +#include linux/platform_device.h
 +#include linux/mfd/core.h
 +#include asm/unaligned.h

Why is this file neded?

thanks,

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Oliver Neukum]

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

> +static void rtsx_usb_sg_timed_out(unsigned long data)
> +{
> + struct rtsx_ucr *ucr = (struct rtsx_ucr *)data;
> +
> + dev_dbg(>pusb_intf->dev, "%s: sg transfer timed out", __func__);
> + usb_sg_cancel(>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(>dev, "%s called with pm message 0x%04u\n",
> + __func__, message.event);
> +
> + mutex_lock(>dev_mutex);
> + rtsx_usb_turn_off_led(ucr);
> + mutex_unlock(>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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Oliver Neukum]

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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Dan Carpenter]

On Wed, Jan 08, 2014 at 03:56:05PM +0800, Roger Tseng wrote:
> Hi Dan,
> 
> >> +int rtsx_usb_ep0_write_register(struct rtsx_ucr *ucr, u16 addr,
> >> + u8 mask, u8 data)
> >> +{
> >> + u16 value = 0, index = 0;
> >> +
> >> + value |= (u16)(3 & 0x03) << 14;
> >> + value |= (u16)(addr & 0x3FFF);
> >
> >Don't do pointless things:
> >
> >value |= 0x03 << 14;
> >value |= addr & 0x3FFF;
> >
> >> + value = ((value << 8) & 0xFF00) | ((value >> 8) & 0x00FF);
> >
> >This is an endian conversion?  It is buggy.  Use the kernel endian
> >conversion functions cpu_to_le16().
> 
> This is not a conversion for endianess with respect to CPU but for
> command format  of the device. It should always be performed
> regardless of platform.
> 
> In other words, it could be equivalent to:
> value |= 0x03 << 6; // lower byte
> value |= (addr & 0x3F00) >> 8; // lower byte
> value |= (addr & 0xFF) << 8; //higher byte
> 
> We think the previous form is easier to read. Should we keep it or
> change to the later one?

To me it's really weird that the standard would specify that the address
is in byte swapped reversed CPU-endian order.  But if that's what it
says then I don't care about formatting details.  The original code is
fine.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Dan Carpenter]

On Wed, Jan 08, 2014 at 03:56:05PM +0800, Roger Tseng wrote:
 Hi Dan,
 
  +int rtsx_usb_ep0_write_register(struct rtsx_ucr *ucr, u16 addr,
  + u8 mask, u8 data)
  +{
  + u16 value = 0, index = 0;
  +
  + value |= (u16)(3  0x03)  14;
  + value |= (u16)(addr  0x3FFF);
 
 Don't do pointless things:
 
 value |= 0x03  14;
 value |= addr  0x3FFF;
 
  + value = ((value  8)  0xFF00) | ((value  8)  0x00FF);
 
 This is an endian conversion?  It is buggy.  Use the kernel endian
 conversion functions cpu_to_le16().
 
 This is not a conversion for endianess with respect to CPU but for
 command format  of the device. It should always be performed
 regardless of platform.
 
 In other words, it could be equivalent to:
 value |= 0x03  6; // lower byte
 value |= (addr  0x3F00)  8; // lower byte
 value |= (addr  0xFF)  8; //higher byte
 
 We think the previous form is easier to read. Should we keep it or
 change to the later one?

To me it's really weird that the standard would specify that the address
is in byte swapped reversed CPU-endian order.  But if that's what it
says then I don't care about formatting details.  The original code is
fine.

regards,
dan carpenter
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Roger Tseng]

Hi Dan,

>> +int rtsx_usb_ep0_write_register(struct rtsx_ucr *ucr, u16 addr,
>> + u8 mask, u8 data)
>> +{
>> + u16 value = 0, index = 0;
>> +
>> + value |= (u16)(3 & 0x03) << 14;
>> + value |= (u16)(addr & 0x3FFF);
>
>Don't do pointless things:
>
>value |= 0x03 << 14;
>value |= addr & 0x3FFF;
>
>> + value = ((value << 8) & 0xFF00) | ((value >> 8) & 0x00FF);
>
>This is an endian conversion?  It is buggy.  Use the kernel endian
>conversion functions cpu_to_le16().

This is not a conversion for endianess with respect to CPU but for command 
format  of the device. It should always be performed regardless of platform.

In other words, it could be equivalent to:
value |= 0x03 << 6; // lower byte
value |= (addr & 0x3F00) >> 8; // lower byte
value |= (addr & 0xFF) << 8; //higher byte

We think the previous form is easier to read. Should we keep it or change to 
the later one?--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Roger Tseng]

Hi Dan,

 +int rtsx_usb_ep0_write_register(struct rtsx_ucr *ucr, u16 addr,
 + u8 mask, u8 data)
 +{
 + u16 value = 0, index = 0;
 +
 + value |= (u16)(3  0x03)  14;
 + value |= (u16)(addr  0x3FFF);

Don't do pointless things:

value |= 0x03  14;
value |= addr  0x3FFF;

 + value = ((value  8)  0xFF00) | ((value  8)  0x00FF);

This is an endian conversion?  It is buggy.  Use the kernel endian
conversion functions cpu_to_le16().

This is not a conversion for endianess with respect to CPU but for command 
format  of the device. It should always be performed regardless of platform.

In other words, it could be equivalent to:
value |= 0x03  6; // lower byte
value |= (addr  0x3F00)  8; // lower byte
value |= (addr  0xFF)  8; //higher byte

We think the previous form is easier to read. Should we keep it or change to 
the later one?--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Dan Carpenter]

On Mon, Dec 23, 2013 at 05:52:05PM +0800, rogera...@realtek.com wrote:
> +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;
> +


I do not like how you have three names for the same buffer length.

> +#define IOBUF_SIZE   1024
> +#define CMD_BUF_LEN  1024
> +#define RSP_BUF_LEN  1024

The buffer is allocated as IOBUF_SIZE and then CMD_BUF_LEN is used as
a limiter here and one other place.  RSP_BUF_LEN is not used at all.

> + 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;
> + ucr->cmd_buf[STAGE_FLAG] = 0;
> + ucr->cmd_buf[8] = (u8)(addr >> 8);
> + ucr->cmd_buf[9] = (u8)addr;
> +
> + memcpy(ucr->cmd_buf + 12, data, len);

Buffer overflow here because ->cmd_buf is only IOBUF_SIZE long.
Probably the earlier two uses of "len" are buffer overflows as well.

> +int rtsx_usb_ep0_write_register(struct rtsx_ucr *ucr, u16 addr,
> + u8 mask, u8 data)
> +{
> + u16 value = 0, index = 0;
> +
> + value |= (u16)(3 & 0x03) << 14;
> + value |= (u16)(addr & 0x3FFF);

Don't do pointless things:

value |= 0x03 << 14;
value |= addr & 0x3FFF;

> + value = ((value << 8) & 0xFF00) | ((value >> 8) & 0x00FF);

This is an endian conversion?  It is buggy.  Use the kernel endian
conversion functions cpu_to_le16().

> + index |= (u16)mask;
> + index |= (u16)data << 8;
> +
> + return usb_control_msg(ucr->pusb_dev,
> + usb_sndctrlpipe(ucr->pusb_dev, 0), 0, 0x40,
> + value, index, NULL, 0, 100);
> +}
> +EXPORT_SYMBOL_GPL(rtsx_usb_ep0_write_register);
> +
> +int rtsx_usb_ep0_read_register(struct rtsx_ucr *ucr, u16 addr, u8 *data)
> +{
> + u16 value = 0;
> +
> + if (data == NULL)
> + return -EINVAL;
> + *data = 0;
> +
> + value |= (u16)(2 & 0x03) << 14;
> + value |= (u16)(addr & 0x3FFF);
> + value = ((value << 8) & 0xFF00) | ((value >> 8) & 0x00FF);

same.

The rest of my comments below are minor white space nits.

> +
> + return usb_control_msg(ucr->pusb_dev,
> + usb_rcvctrlpipe(ucr->pusb_dev, 0), 0, 0xC0,
> + value, 0, data, 1, 100);
> +}
> +EXPORT_SYMBOL_GPL(rtsx_usb_ep0_read_register);
> +
> +void rtsx_usb_add_cmd(struct rtsx_ucr *ucr,
> + u8 cmd_type,
> + u16 reg_addr,
> + u8 mask,
> + u8 data)
> +{
> + int i;
> +
> + if (ucr->cmd_idx < ((CMD_BUF_LEN - CMD_OFFSET) / 4)) {
> + i = CMD_OFFSET + ucr->cmd_idx * 4;
> +
> + ucr->cmd_buf[i++] = ((cmd_type & 0x03) << 6) |
> + (u8)((reg_addr >> 8) & 0x3F);
> + ucr->cmd_buf[i++] = (u8)reg_addr;
> + ucr->cmd_buf[i++] = mask;
> + ucr->cmd_buf[i++] = data;
> +
> + ucr->cmd_idx++;
> + }
> +}
> +EXPORT_SYMBOL_GPL(rtsx_usb_add_cmd);
> +
> +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) {
> + /* clear HW error*/
> + rtsx_usb_ep0_write_register(ucr, SFSM_ED, 0xf8, 0xf8);

Make this into a function and remove the comment.

rtsx_usb_ep0_clear_hw_error(ucr);

> + return ret;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(rtsx_usb_send_cmd);
> +
> +int rtsx_usb_get_rsp(struct rtsx_ucr *ucr, int rsp_len, int timeout)
> +{
> + if (rsp_len <= 0)
> + return -EINVAL;
> +
> + if (rsp_len % 4)
> + rsp_len += (4 - rsp_len % 4);
> +
> + return rtsx_usb_transfer_data(ucr,
> + usb_rcvbulkpipe(ucr->pusb_dev, EP_BULK_IN),
> + ucr->rsp_buf, rsp_len, 0, NULL, timeout);
> +}
> +EXPORT_SYMBOL_GPL(rtsx_usb_get_rsp);
> +
> +static int rtsx_usb_get_status_with_bulk(struct rtsx_ucr *ucr, u16 *status)
> +{
> + int ret;
> +
> + rtsx_usb_init_cmd(ucr);
> + rtsx_usb_add_cmd(ucr, READ_REG_CMD, CARD_EXIST, 0x00, 0x00);
> + rtsx_usb_add_cmd(ucr, READ_REG_CMD, OCPSTAT, 0x00, 0x00);
> + ret = rtsx_usb_send_cmd(ucr, MODE_CR, 100);
> + if (ret)
> + return ret;
> +
> +

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

[Dan Carpenter]

On Mon, Dec 23, 2013 at 05:52:05PM +0800, rogera...@realtek.com wrote:
 +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;
 +


I do not like how you have three names for the same buffer length.

 +#define IOBUF_SIZE   1024
 +#define CMD_BUF_LEN  1024
 +#define RSP_BUF_LEN  1024

The buffer is allocated as IOBUF_SIZE and then CMD_BUF_LEN is used as
a limiter here and one other place.  RSP_BUF_LEN is not used at all.

 + 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;
 + ucr-cmd_buf[STAGE_FLAG] = 0;
 + ucr-cmd_buf[8] = (u8)(addr  8);
 + ucr-cmd_buf[9] = (u8)addr;
 +
 + memcpy(ucr-cmd_buf + 12, data, len);

Buffer overflow here because -cmd_buf is only IOBUF_SIZE long.
Probably the earlier two uses of len are buffer overflows as well.

 +int rtsx_usb_ep0_write_register(struct rtsx_ucr *ucr, u16 addr,
 + u8 mask, u8 data)
 +{
 + u16 value = 0, index = 0;
 +
 + value |= (u16)(3  0x03)  14;
 + value |= (u16)(addr  0x3FFF);

Don't do pointless things:

value |= 0x03  14;
value |= addr  0x3FFF;

 + value = ((value  8)  0xFF00) | ((value  8)  0x00FF);

This is an endian conversion?  It is buggy.  Use the kernel endian
conversion functions cpu_to_le16().

 + index |= (u16)mask;
 + index |= (u16)data  8;
 +
 + return usb_control_msg(ucr-pusb_dev,
 + usb_sndctrlpipe(ucr-pusb_dev, 0), 0, 0x40,
 + value, index, NULL, 0, 100);
 +}
 +EXPORT_SYMBOL_GPL(rtsx_usb_ep0_write_register);
 +
 +int rtsx_usb_ep0_read_register(struct rtsx_ucr *ucr, u16 addr, u8 *data)
 +{
 + u16 value = 0;
 +
 + if (data == NULL)
 + return -EINVAL;
 + *data = 0;
 +
 + value |= (u16)(2  0x03)  14;
 + value |= (u16)(addr  0x3FFF);
 + value = ((value  8)  0xFF00) | ((value  8)  0x00FF);

same.

The rest of my comments below are minor white space nits.

 +
 + return usb_control_msg(ucr-pusb_dev,
 + usb_rcvctrlpipe(ucr-pusb_dev, 0), 0, 0xC0,
 + value, 0, data, 1, 100);
 +}
 +EXPORT_SYMBOL_GPL(rtsx_usb_ep0_read_register);
 +
 +void rtsx_usb_add_cmd(struct rtsx_ucr *ucr,
 + u8 cmd_type,
 + u16 reg_addr,
 + u8 mask,
 + u8 data)
 +{
 + int i;
 +
 + if (ucr-cmd_idx  ((CMD_BUF_LEN - CMD_OFFSET) / 4)) {
 + i = CMD_OFFSET + ucr-cmd_idx * 4;
 +
 + ucr-cmd_buf[i++] = ((cmd_type  0x03)  6) |
 + (u8)((reg_addr  8)  0x3F);
 + ucr-cmd_buf[i++] = (u8)reg_addr;
 + ucr-cmd_buf[i++] = mask;
 + ucr-cmd_buf[i++] = data;
 +
 + ucr-cmd_idx++;
 + }
 +}
 +EXPORT_SYMBOL_GPL(rtsx_usb_add_cmd);
 +
 +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) {
 + /* clear HW error*/
 + rtsx_usb_ep0_write_register(ucr, SFSM_ED, 0xf8, 0xf8);

Make this into a function and remove the comment.

rtsx_usb_ep0_clear_hw_error(ucr);

 + return ret;
 + }
 +
 + return 0;
 +}
 +EXPORT_SYMBOL_GPL(rtsx_usb_send_cmd);
 +
 +int rtsx_usb_get_rsp(struct rtsx_ucr *ucr, int rsp_len, int timeout)
 +{
 + if (rsp_len = 0)
 + return -EINVAL;
 +
 + if (rsp_len % 4)
 + rsp_len += (4 - rsp_len % 4);
 +
 + return rtsx_usb_transfer_data(ucr,
 + usb_rcvbulkpipe(ucr-pusb_dev, EP_BULK_IN),
 + ucr-rsp_buf, rsp_len, 0, NULL, timeout);
 +}
 +EXPORT_SYMBOL_GPL(rtsx_usb_get_rsp);
 +
 +static int rtsx_usb_get_status_with_bulk(struct rtsx_ucr *ucr, u16 *status)
 +{
 + int ret;
 +
 + rtsx_usb_init_cmd(ucr);
 + rtsx_usb_add_cmd(ucr, READ_REG_CMD, CARD_EXIST, 0x00, 0x00);
 + rtsx_usb_add_cmd(ucr, READ_REG_CMD, OCPSTAT, 0x00, 0x00);
 + ret = rtsx_usb_send_cmd(ucr, MODE_CR, 100);
 + if (ret)
 + return ret;
 +
 + ret = rtsx_usb_get_rsp(ucr, 2, 100);
 + if (ret)
 + return ret;
 +
 + *status = (u16)((ucr-rsp_buf[0]  2)  0x0f) |
 + ((ucr-rsp_buf[1]  0x03)  4);

The cast to u16 is not