From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Ribeiro Subject: Re: [patch 05/14] mfd: PCAP2 driver Date: Sun, 23 Nov 2008 01:38:57 -0200 Message-ID: <1227411537.3148.22.camel@brutus> References: <20081121160403.073751031@dodger.lab.datenfreihafen.org> <200811221558.03915.david-b@pacbell.net> <20081123003306.GE24437@datenfreihafen.org> <200811221819.53186.david-b@pacbell.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Cc: Stefan Schmidt , eric.y.miao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, sameo-RWuK6r/cQWRpLGFMi4vTTA@public.gmane.org, linux-arm-kernel-xIg/pKzrS19vn6HldHNs0ANdhmdF6hFW@public.gmane.org, Stefan Schmidt To: David Brownell Return-path: In-Reply-To: <200811221819.53186.david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: spi-devel-general-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: linux-spi.vger.kernel.org RW0gU8OhYiwgMjAwOC0xMS0yMiDDoHMgMTg6MTkgLTA4MDAsIERhdmlkIEJyb3duZWxsIGVzY3Jl dmV1Ogo+IFNQSS1zcGVjaWZpYyBiaXRzOgo+IAo+ICAtIEknZCBoYXZlIHRvIHNlZSB0aGUgcGF0 Y2guICBUaGUgbGFzdCBvbmUgSSBzYXcgc3RpbGwgZGlkbid0Cj4gICAgbGlzdCBhIFNQSV9NQVNU RVIgZGVwZW5kZW5jeSBmb3IgdGhlIGNvcmUgTUZEIG1vZHVsZS4KClRoZSBuZXh0IG9uZSB3aWxs LgoKPiAgLSBZb3Ugc2hvdWxkIG1ha2UgZXp4X3BjYXBfd3JpdGUoKSBhbmQgZXp4X3BjYXBfcmVh ZCgpIHN3aXRjaAo+ICAgIG92ZXIgdG8gc3BpX3dyaXRlX3RoZW5fcmVhZCgpLCB0byBlbnN1cmUg aXQncyBuZXZlciBkb2luZwo+ICAgIERNQSB0by9mcm9tIHRoZSBzdGFjay4gIEkgc2VlIGEgYnl0 ZS1vcmRlciBkZXBlbmRlbmN5IHRvby4uLgoKSSB0cmllZCBzcGlfd3JpdGVfdGhlbl9yZWFkIGJl Zm9yZSwgYnV0IGl0IGRpZG4ndCB3b3JrLiBJIHN1cHBvc2VkIGl0CndhcyBiZWNhdXNlIGl0IHdh cyBkb2luZyAyIHRyYW5zZmVycyBhcyB0aGUgc2Vjb25kIHRyYW5zZmVyIHJ4X2J1ZgphbHdheXMg Y2FtZSB6ZXJvZWQuIEkgc2VlIHRoYXQgY29tbWl0CmY5YjkwZTM5Y2JjNWM0ZDZlZjYwMDIyZmQx ZjI1ZDU0MWRmMGFhZDEgY2hhbmdlZCBpdCB0byBkbyBhIHNpbmdsZQp0cmFuc2Zlciwgc28gaSB3 aWxsIHRyeSBpdCBhZ2Fpbi4KCj4gIC0gRm9yIGdlbmVyYWwgcGFyYW5vaWEsIHRoZSBwcm9iZSgp IHNob3VsZCBhYm9ydCBpZiBwY2FwLnNwaSBpcwo+ICAgIGFscmVhZHkgc2V0IC4uLiBhbmQgaXRz IGNsZWFudXAgcGF0aCwgcGx1cyByZW1vdmUoKSwgc2hvdWxkCj4gICAgbnVsbCB0aGF0IHBvaW50 ZXIuCgpPay4KCj4gIC0gSWYgeW91J3JlIGdvaW5nIHRvIG1hcmsgdGhlIHByb2JlKCkgYXMgX19k ZXZpbml0LCB0aGVuIG1hcmsKPiAgICB0aGUgcmVtb3ZlKCkgYXMgX19kZXZleGl0IGFuZCB1c2Ug X19kZXZleGl0X3AoKSBpbiB0aGUgZHJpdmVyCj4gICAgc3RydWN0LgoKT2suIFNob3VsZG4ndCBp IHVzZSBfX2luaXQgaW5zdGVhZD8KCj4gT3RoZXIgY29tbWVudHMgYWJvdXQgdGhlIHBjYXAyIGNv cmU6Cgo+IC0gVGhvc2Ugc2hvd19yZWdzL3N0b3JlX3JlZ3MgY2FsbHMgd291bGQgSU1PIG1ha2Ug bW9yZSBzZW5zZQo+ICAgIGluIGRlYnVnZnMgdGhhbiBpbiBzeXNmcy4KCkkgd2lsbCBqdXN0IHJl bW92ZSB0aG9zZSBmb3Igbm93LgoKPiAgLSBUaGUgc2V0X3ZyZWcoKSBzdHVmZiB3b3VsZCBzZWVt IHRvIG1ha2UgbW9yZSBzZW5zZSBpbiBhCj4gICAgcmVndWxhdG9yIHN1YmRldmljZSBhbmQgZHJp dmVyIC4uLiBidXQgdGhhdCdzIG1vcmUgb2YgYQo+ICAgIGdlbmVyYWwgZHJpdmVyIHN0cnVjdHVy ZSB0aGluZywgYW5kIG1pZ2h0IGJlIGZpeGVkIGxhdGVyLgo+IAo+ICAtIEFuZHJldyBzZWVtcyB0 byBhbHdheXMgd2FudCBhIGNvbW1lbnQgZXhwbGFpbmluZyB3aHkgdGhlCj4gICAgSVJRIGhhbmRs ZXIgZm9yIEkyQyBhbmQgU1BJIGRldmljZXMgbmVlZHMgdG8gcXVldWVfd29yaygpLgo+ICAgIE1h eWJlIHRoZSB0aHJlYWRlZCBJUlEgc3R1ZmYgd2lsbCBoZWxwIHRoZXJlLi4uCj4gCj4gIC0gVGhl IG1hc2tfZXZlbnQoKS91bm1hc2tfZXZlbnQoKSBzdHVmZiBsb29rcyBsaWtlIHlvdSdyZQo+ICAg IG1vcmUgb3IgbGVzcyByZWludmVudGluZyBhIGJhYnkgInN0cnVjdCBpcnFfY2hpcCIsIHdpdGgK PiAgICByZWdpc3Rlcl9ldmVudCgpIGluc3RlYWQgb2YgcmVxdWVzdF9pcnEoKS4KCj4gUmUgdGhl IElSUSBzdHVmZiwgdGhpcyBsb29rcyBtb3JlIGxpa2Ugd2hhdCBpMmMvY2hpcHMvbWVuZWxhdXMu Ywo+IGRpZCB0aGFuIG1mZC90d2w0MDMwLWlycS5jIC4uLiBpbiBnZW5lcmFsIEkgdGhpbmsgaXQn cyBiZXR0ZXIgdG8KPiBwdXJzdWUgdGhlIGxhdHRlciBhcHByb2FjaCwgbWFraW5nIGdlbmlycSBo YW5kbGUgc3VjaCBzdHVmZi4KPiAoRXZlbiB0aG91Z2ggaXQncyBraW5kIG9mIGF3a3dhcmQgdG8g dXNlIGl0IGZvciBJMkMgb3IgU1BJIGJhc2VkCj4gaW50ZXJydXB0IGNvbnRyb2xsZXJzIGp1c3Qg bm93LikKCkkgZGlkbid0IGtub3cgaSBjb3VsZCB1c2UgZ2VuaXJxIHdpdGggc3BpLiBJbiBmYWN0 IGkgd2FzIHVzaW5nIGdlbmlycQpiZWZvcmUsIHdoZW4gdGhlIGRyaXZlciB3YXMgdXNpbmcgc3Nw LmMuIE5vdywgbG9va2luZyBhdCB0d2w0MDMwLWlycS5jCml0cyBjbGVhciBob3cgaSBzaG91bGQg aGF2ZSBkb25lIHRoaXMuIFRoYW5rcyEhCgo+ICAtIEFuZCB0aGUgQURDIHN5c2ZzIHN1cHBvcnQg aXNuJ3Qgc3VwcG9ydGluZyBod21vbiBtb2RlbHMuCj4gICAgKE5laXRoZXIgZG9lcyB0aGUgdHds NDAzMCBBREMgc3VwcG9ydCwgYnV0IHRoYXQncyBub3QgYmVlbgo+ICAgIHN1Ym1pdHRlZCBmb3Ig bWFpbmxpbmUgeWV0IGVpdGhlci4uLikKCkkgd2lsbCBsb29rIGludG8gdGhpcyBhZnRlciBpIGZp bmlzaCB0aGUgaXJxIHN0dWZmLgoKClRoaXMgaXMgYSBsb3Qgb2Ygc3R1ZmYsIHRoYW5rcyBmb3Ig dGhlIHJldmlldy4gOikKCi0tIApEYW5pZWwgUmliZWlybwoKCi0tLS0tLS0tLS0tLS0tLS0tLS0t LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0KVGhp cyBTRi5OZXQgZW1haWwgaXMgc3BvbnNvcmVkIGJ5IHRoZSBNb2JsaW4gWW91ciBNb3ZlIERldmVs b3BlcidzIGNoYWxsZW5nZQpCdWlsZCB0aGUgY29vbGVzdCBMaW51eCBiYXNlZCBhcHBsaWNhdGlv bnMgd2l0aCBNb2JsaW4gU0RLICYgd2luIGdyZWF0IHByaXplcwpHcmFuZCBwcml6ZSBpcyBhIHRy aXAgZm9yIHR3byB0byBhbiBPcGVuIFNvdXJjZSBldmVudCBhbnl3aGVyZSBpbiB0aGUgd29ybGQK aHR0cDovL21vYmxpbi1jb250ZXN0Lm9yZy9yZWRpcmVjdC5waHA/YmFubmVyX2lkPTEwMCZ1cmw9 LwpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpzcGktZGV2 ZWwtZ2VuZXJhbCBtYWlsaW5nIGxpc3QKc3BpLWRldmVsLWdlbmVyYWxAbGlzdHMuc291cmNlZm9y Z2UubmV0Cmh0dHBzOi8vbGlzdHMuc291cmNlZm9yZ2UubmV0L2xpc3RzL2xpc3RpbmZvL3NwaS1k ZXZlbC1nZW5lcmFsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757927AbYKWDjS (ORCPT ); Sat, 22 Nov 2008 22:39:18 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1755411AbYKWDjJ (ORCPT ); Sat, 22 Nov 2008 22:39:09 -0500 Received: from yx-out-2324.google.com ([74.125.44.29]:37559 "EHLO yx-out-2324.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755384AbYKWDjH (ORCPT ); Sat, 22 Nov 2008 22:39:07 -0500 DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=subject:from:to:cc:in-reply-to:references:content-type:date :message-id:mime-version:x-mailer:content-transfer-encoding; b=TRUFENIwMKyquQbYFy22dgRhd7zIE6Aj06NAKkPgwypVgf4fIMisudKVYp2s6k+26w tSKBWhiul714wmFFjS9EI3A2YhuekYzLb8SHrKVFV+8yX4FTervlM1QLQpxv++QODsdJ dMMdv3d614kOu4e8Vd4wr2dA00NO08PnuC8TI= Subject: Re: [spi-devel-general] [patch 05/14] mfd: PCAP2 driver From: Daniel Ribeiro To: David Brownell Cc: Stefan Schmidt , spi-devel-general@lists.sourceforge.net, Stefan Schmidt , eric.y.miao@gmail.com, linux-kernel@vger.kernel.org, sameo@openedhand.com, linux-arm-kernel@lists.arm.linux.org.uk In-Reply-To: <200811221819.53186.david-b@pacbell.net> References: <20081121160403.073751031@dodger.lab.datenfreihafen.org> <200811221558.03915.david-b@pacbell.net> <20081123003306.GE24437@datenfreihafen.org> <200811221819.53186.david-b@pacbell.net> Content-Type: text/plain; charset=UTF-8 Date: Sun, 23 Nov 2008 01:38:57 -0200 Message-Id: <1227411537.3148.22.camel@brutus> Mime-Version: 1.0 X-Mailer: Evolution 2.22.3.1 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Sáb, 2008-11-22 às 18:19 -0800, David Brownell escreveu: > SPI-specific bits: > > - I'd have to see the patch. The last one I saw still didn't > list a SPI_MASTER dependency for the core MFD module. The next one will. > - You should make ezx_pcap_write() and ezx_pcap_read() switch > over to spi_write_then_read(), to ensure it's never doing > DMA to/from the stack. I see a byte-order dependency too... I tried spi_write_then_read before, but it didn't work. I supposed it was because it was doing 2 transfers as the second transfer rx_buf always came zeroed. I see that commit f9b90e39cbc5c4d6ef60022fd1f25d541df0aad1 changed it to do a single transfer, so i will try it again. > - For general paranoia, the probe() should abort if pcap.spi is > already set ... and its cleanup path, plus remove(), should > null that pointer. Ok. > - If you're going to mark the probe() as __devinit, then mark > the remove() as __devexit and use __devexit_p() in the driver > struct. Ok. Shouldn't i use __init instead? > Other comments about the pcap2 core: > - Those show_regs/store_regs calls would IMO make more sense > in debugfs than in sysfs. I will just remove those for now. > - The set_vreg() stuff would seem to make more sense in a > regulator subdevice and driver ... but that's more of a > general driver structure thing, and might be fixed later. > > - Andrew seems to always want a comment explaining why the > IRQ handler for I2C and SPI devices needs to queue_work(). > Maybe the threaded IRQ stuff will help there... > > - The mask_event()/unmask_event() stuff looks like you're > more or less reinventing a baby "struct irq_chip", with > register_event() instead of request_irq(). > Re the IRQ stuff, this looks more like what i2c/chips/menelaus.c > did than mfd/twl4030-irq.c ... in general I think it's better to > pursue the latter approach, making genirq handle such stuff. > (Even though it's kind of awkward to use it for I2C or SPI based > interrupt controllers just now.) I didn't know i could use genirq with spi. In fact i was using genirq before, when the driver was using ssp.c. Now, looking at twl4030-irq.c its clear how i should have done this. Thanks!! > - And the ADC sysfs support isn't supporting hwmon models. > (Neither does the twl4030 ADC support, but that's not been > submitted for mainline yet either...) I will look into this after i finish the irq stuff. This is a lot of stuff, thanks for the review. :) -- Daniel Ribeiro