From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lee Jones Subject: Re: [PATCH 1/3] mfd: Add realtek USB card reader driver Date: Thu, 16 Jan 2014 09:35:06 +0000 Message-ID: <20140116093506.GI11820@lee--X1> References: <1389685656-880-1-git-send-email-rogerable@realtek.com> <1389685656-880-2-git-send-email-rogerable@realtek.com> <20140114130409.GB11820@lee--X1> <52D79E31.6000300@realtek.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <52D79E31.6000300@realtek.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: driverdev-devel-bounces@linuxdriverproject.org To: Roger Cc: Samuel Ortiz , Alex Dubov , Greg Kroah-Hartman , "driverdev-devel@linuxdriverproject.org" , "linux-mmc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Wei_wang , Chris Ball , Dan Carpenter , Maxim Levitsky List-Id: linux-mmc@vger.kernel.org PiA+PitzdGF0aWMgaW50IHJ0c3hfdXNiX2J1bGtfdHJhbnNmZXJfc2dsaXN0KHN0cnVjdCBydHN4 X3VjciAqdWNyLAo+ID4+KyAgICAgICAgICAgICB1bnNpZ25lZCBpbnQgcGlwZSwgc3RydWN0IHNj YXR0ZXJsaXN0ICpzZywgaW50IG51bV9zZywKPiA+PisgICAgICAgICAgICAgdW5zaWduZWQgaW50 IGxlbmd0aCwgdW5zaWduZWQgaW50ICphY3RfbGVuLCBpbnQgdGltZW91dCkKPiA+Pit7Cj4gPj4r ICAgICBpbnQgcmV0Owo+ID4+Kwo+ID4+KyAgICAgZGV2X2RiZygmdWNyLT5wdXNiX2ludGYtPmRl diwgIiVzOiB4ZmVyICV1IGJ5dGVzLCAlZCBlbnRyaWVzXG4iLAo+ID4+KyAgICAgICAgICAgICAg ICAgICAgIF9fZnVuY19fLCBsZW5ndGgsIG51bV9zZyk7Cj4gPj4rICAgICByZXQgPSB1c2Jfc2df aW5pdCgmdWNyLT5jdXJyZW50X3NnLCB1Y3ItPnB1c2JfZGV2LCBwaXBlLCAwLAo+ID4+KyAgICAg ICAgICAgICAgICAgICAgIHNnLCBudW1fc2csIGxlbmd0aCwgR0ZQX05PSU8pOwo+ID4+KyAgICAg aWYgKHJldCkKPiA+PisgICAgICAgICAgICAgcmV0dXJuIHJldDsKPiA+PisKPiA+PisgICAgIHVj ci0+c2dfdGltZXIuZXhwaXJlcyA9IGppZmZpZXMgKyBtc2Vjc190b19qaWZmaWVzKHRpbWVvdXQp Owo+ID4+KyAgICAgYWRkX3RpbWVyKCZ1Y3ItPnNnX3RpbWVyKTsKPiA+PisgICAgIHVzYl9zZ193 YWl0KCZ1Y3ItPmN1cnJlbnRfc2cpOwo+ID4+KyAgICAgZGVsX3RpbWVyKCZ1Y3ItPnNnX3RpbWVy KTsKPiA+PisKPiA+PisgICAgIGlmIChhY3RfbGVuKQo+ID4+KyAgICAgICAgICAgICAqYWN0X2xl biA9IHVjci0+Y3VycmVudF9zZy5ieXRlczsKPiA+PisKPiA+PisgICAgIHJldHVybiB1Y3ItPmN1 cnJlbnRfc2cuc3RhdHVzOwo+ID4+K30KPiA+Cj4gPkNvZGUgbG9va3MgZmluZSwgYnV0IHNob3Vs ZG4ndCB0aGlzIGxpdmUgaW4gYSBVU0IgZHJpdmVyPwo+ID4KPiBXZSBoYXZlIHN0dWRpZWQgZHJp dmVycyBpbiB1c2Ivc3RvcmFnZSwgdGhlIHBsYWNlIHRoYXQgbW9zdCBsaWtlbHkKPiB0byBwdXQg dGhlIGRyaXZlci4gQWxsIG9mIHRoZW0gYXJlIGZvciBTVEFOREFSRCB1c2IgbWFzcyBzdG9yYWdl Cj4gY2xhc3Moc29tZXRoaW5nIGxpa2UgVVNCX0NMQVNTX01BU1NfU1RPUkFHRSkuIEJ1dCB0aGlz IGlzIG5vdCB0aGUKPiBzYW1lIGNhc2UuIFRoaXMgZHJpdmVyIGlzIGZvciBvdXIgdmVuZG9yLWNs YXNzIGRldmljZSB3aXRoCj4gY29tcGxldGVseSBkaWZmZXJlbnQgcHJvdG9jb2wuIEl0IGlzIHJl YWxseSBhbiBVU0IgaW50ZXJmYWNlZCBmbGFzaAo+IGNhcmQgY29tbWFuZCBjb252ZXJ0ZXIob3Ig Y2hhbm5lbCkgZGV2aWNlIHJhdGhlciB0aGFuIGEgcmVhbAo+IHN0b3JhZ2UuIEl0IGFsc28gaGFz IHR3byBkZXJpdmVkIG1vZHVsZXMocnRzeF91c2Jfc2RtbWMsCj4gcnRzeF91c2JfbWVtc3RpY2sp IGZvciBvdGhlciB0d28gc3Vic3lzdGVtcy4KPiAKPiBXZSBhbHNvIGhhdmUgYW5vdGhlciBkcml2 ZXI6IHJ0c3hfcGNyLmMgcmVzaWRlbnQgaW4gbWZkLyBmb3Igc2ltaWxhcgo+IGRldmljZXMgYnV0 IG9mIFBDSWUgaW50ZXJmYWNlLiBJdCBpcyBuYXR1cmUgZm9yIHVzIHRvIGNob29zZSB0aGUKPiBz YW1lIHBsYWNlIGZvciB0aGlzIHZhcmlhbnQuCgpWZXJ5IHdlbGwsIGFzIGxvbmcgYXMgaXQgdHJ1 bHkgZG9lcyBub3QgZml0IGluIGRyaXZlcnMvdXNiLiBJdCB3b3VsZApiZSBnb29kIHRvIGhhdmUg b25lIG9mIHRoZSBVU0IgZm9sayBnaXZlIHRoZSBub2QgdGhvdWdoLgoKPiA+PisgICAgIH0KPiA+ PisKPiA+PisgICAgIC8qIHNldCBVU0IgaW50ZXJmYWNlIGRhdGEgKi8KPiA+PisgICAgIHVzYl9z ZXRfaW50ZmRhdGEoaW50ZiwgdWNyKTsKPiA+PisKPiA+PisgICAgIHVjci0+dmVuZG9yX2lkID0g aWQtPmlkVmVuZG9yOwo+ID4+KyAgICAgdWNyLT5wcm9kdWN0X2lkID0gaWQtPmlkUHJvZHVjdDsK PiA+PisgICAgIHVjci0+Y21kX2J1ZiA9IHVjci0+cnNwX2J1ZiA9IHVjci0+aW9idWY7Cj4gPj4r Cj4gPj4rICAgICBtdXRleF9pbml0KCZ1Y3ItPmRldl9tdXRleCk7Cj4gPj4rCj4gPj4rICAgICB1 Y3ItPnB1c2JfaW50ZiA9IGludGY7Cj4gPj4rCj4gPj4rICAgICAvKiBpbml0aWFsaXplICovCj4g Pj4rICAgICByZXQgPSBydHN4X3VzYl9pbml0X2NoaXAodWNyKTsKPiA+PisgICAgIGlmIChyZXQp Cj4gPj4rICAgICAgICAgICAgIGdvdG8gb3V0X2luaXRfZmFpbDsKPiA+PisKPiA+PisgICAgIGZv ciAoaSA9IDA7IGkgPCBBUlJBWV9TSVpFKHJ0c3hfdXNiX2NlbGxzKTsgaSsrKSB7Cj4gPj4rICAg ICAgICAgICAgIHJ0c3hfdXNiX2NlbGxzW2ldLnBsYXRmb3JtX2RhdGEgPSAmdWNyOwo+ID4KPiA+ WW91J3ZlIGFscmVhZHkgcHV0IHRoaXMgaW4geW91ciBkcnZkYXRhIChvciBudGZkYXRhLCBhcyBp dCdzIGNhbGxlZAo+ID5oZXJlKS4gV2h5IGRvIHlvdSBhbHNvIG5lZWQgaXQgaW4gcGxhdGZvcm1f ZGF0YT8KPiAKPiBEZXJpdmVkIG1vZHVsZXMgcnRzeF91c2Jfc2RtbWMgYW5kIHJ0c3hfdXNiX21l bXN0aWNrIHdpbGwgcmV0cmlldmUKPiB0aGlzIGZyb20gcGxhdGZvcm1fZGF0YS4gVGhleSB3aWxs IG5vdCBiZSBhd2FyZSBvZiB1c2IgaW50ZXJmYWNlCj4gc3RydWN0LgoKVGhleSBkb24ndCBuZWVk IHRvLiBJZiB0aGUgTU1DIG9yIE1lbXN0aWNrIHN1YnN5c3RlbXMgZG8gbm90IGhhdmUKdGhlaXIg b3duIGhlbHBlcnMgeW91IGNhbiBqdXN0IHVzZSBzb21ldGhpbmcgbGlrZToKCiAgc3RydWN0IHJ0 c3hfdWNyICp1Y3IgPSBkZXZfZ2V0X2RydmRhdGEocGRldi0+ZGV2LnBhcmVudCk7CgpOYXR1cmFs bHkgdGhpcyBpcyB1bnRlc3RlZCBjb2RlIGFuZCBtaWdodCBub3Qgd29yayBvZmYgdGhlIGJhdCwg YnV0IGl0CmRvZXMgZ2l2ZSB5b3Ugc29tZSBpZGVhIG9mIHdoYXQgeW91IGNhbiBkbyB3aXRob3V0 IGl0ZXJhdGluZyB0aHJvdWdoCmFuZCBwb3B1bGF0aW5nIGVhY2ggc3ViLWRldmljZSdzIHBsYXRm b3JtIGRhdGEuCgo+ID4+KyNkZWZpbmUgRFJWX05BTUVfUlRTWF9VU0IgICAgICAgICAgICAicnRz eF91c2IiCj4gPj4rI2RlZmluZSBEUlZfTkFNRV9SVFNYX1VTQl9TRE1NQyAgICAgICAgICAgICAg InJ0c3hfdXNiX3NkbW1jIgo+ID4+KyNkZWZpbmUgRFJWX05BTUVfUlRTWF9VU0JfTVMgICAgICAg ICAicnRzeF91c2JfbXMiCj4gPgo+ID5DYW4geW91IGp1c3QgcHV0IHRoZSBuYW1lcyBpbiB0aGUg Y29ycmVjdCBwbGFjZXMgcGxlYXNlPwo+ID4KPiBEbyB5b3UgbWVhbiBqdXN0IHJlbW92ZSB0aGVz ZSBkZWZpbml0aW9ucyBhbmQgZmlsbCB0aGUgc3RyaW5nCj4gZGlyZWN0bHkgYXQgdGhlIHVzaW5n IHBsYWNlPwoKSSBkby4KCkkgZmluZCB0aGVzZSB0eXBlcyBvZiBkZWZpbmVzIHVuaGVscGZ1bCBh bmQgc29tZXdoYXQgb2JmdXNjYXRpbmcuCgotLSAKTGVlIEpvbmVzCkxpbmFybyBTVE1pY3JvZWxl Y3Ryb25pY3MgTGFuZGluZyBUZWFtIExlYWQKTGluYXJvLm9yZyDilIIgT3BlbiBzb3VyY2Ugc29m dHdhcmUgZm9yIEFSTSBTb0NzCkZvbGxvdyBMaW5hcm86IEZhY2Vib29rIHwgVHdpdHRlciB8IEJs b2cKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZGV2ZWwg bWFpbGluZyBsaXN0CmRldmVsQGxpbnV4ZHJpdmVycHJvamVjdC5vcmcKaHR0cDovL2RyaXZlcmRl di5saW51eGRyaXZlcnByb2plY3Qub3JnL21haWxtYW4vbGlzdGluZm8vZHJpdmVyZGV2LWRldmVs Cg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752301AbaAPJfV (ORCPT ); Thu, 16 Jan 2014 04:35:21 -0500 Received: from mail-wg0-f43.google.com ([74.125.82.43]:62189 "EHLO mail-wg0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751520AbaAPJfQ (ORCPT ); Thu, 16 Jan 2014 04:35:16 -0500 Date: Thu, 16 Jan 2014 09:35:06 +0000 From: Lee Jones To: Roger Cc: Samuel Ortiz , Chris Ball , Greg Kroah-Hartman , Maxim Levitsky , Alex Dubov , Dan Carpenter , "linux-kernel@vger.kernel.org" , "linux-mmc@vger.kernel.org" , "driverdev-devel@linuxdriverproject.org" , Wei_wang , "micky_ching@realsil.com.cn" Subject: Re: [PATCH 1/3] mfd: Add realtek USB card reader driver Message-ID: <20140116093506.GI11820@lee--X1> References: <1389685656-880-1-git-send-email-rogerable@realtek.com> <1389685656-880-2-git-send-email-rogerable@realtek.com> <20140114130409.GB11820@lee--X1> <52D79E31.6000300@realtek.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <52D79E31.6000300@realtek.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > >>+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_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