| Message ID | 20171027010841.28624-2-tpiepho@impinj.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 26, 2017 at 06:08:38PM -0700, Trent Piepho wrote: > The driver will fail to load if no gpio chip selects are specified, > this patch changes this so that it no longer fails. > > It's possible to use all native chip selects, in which case there is > no reason to have a gpio chip select array. This is what happens if > the *optional* device tree property "cs-gpios" is omitted. Do the native chip selects actually work usefully on this hardware? There used to be problems with it wanting to do things like bounce the chip select on every word which made it extremely difficult to use with Linux.
On Tue, 2017-10-31 at 11:19 +0000, Mark Brown wrote: > On Thu, Oct 26, 2017 at 06:08:38PM -0700, Trent Piepho wrote: > > The driver will fail to load if no gpio chip selects are specified, > > this patch changes this so that it no longer fails. > > > > It's possible to use all native chip selects, in which case there is > > no reason to have a gpio chip select array. This is what happens if > > the *optional* device tree property "cs-gpios" is omitted. > > Do the native chip selects actually work usefully on this hardware? > There used to be problems with it wanting to do things like bounce the > chip select on every word which made it extremely difficult to use with > Linux. Still are annoying, but on the device we have connected to it, it ends up working as desired. I've not thoroughly investigated this hardware to find the details. IIRC, the designware SPI on Altera SoCFPGA had the same issue, but it was a flaw in the driver and I was able to fix it. I've come to expect it, as every new SPI master I use doesn't work properly in some different way.
On Tue, Oct 31, 2017 at 04:57:42PM +0000, Trent Piepho wrote: > On Tue, 2017-10-31 at 11:19 +0000, Mark Brown wrote: > > Do the native chip selects actually work usefully on this hardware? > > There used to be problems with it wanting to do things like bounce the > > chip select on every word which made it extremely difficult to use with > > Linux. > Still are annoying, but on the device we have connected to it, it ends > up working as desired. > I've not thoroughly investigated this hardware to find the details. > IIRC, the designware SPI on Altera SoCFPGA had the same issue, but it > was a flaw in the driver and I was able to fix it. I've come to expect > it, as every new SPI master I use doesn't work properly in some > different way. It's one of the reasons why I'm suspicous of making the GPIO optional, lots of chips use GPIO chipselects because a lot of the breakage is confined to chip select handling and I remember the i.MX as being especially far from useful.
T24gVGh1LCAyMDE3LTExLTAyIGF0IDE1OjE0ICswMDAwLCBNYXJrIEJyb3duIHdyb3RlOg0KPiBP biBUdWUsIE9jdCAzMSwgMjAxNyBhdCAwNDo1Nzo0MlBNICswMDAwLCBUcmVudCBQaWVwaG8gd3Jv dGU6DQo+ID4gT24gVHVlLCAyMDE3LTEwLTMxIGF0IDExOjE5ICswMDAwLCBNYXJrIEJyb3duIHdy b3RlOg0KPiA+ID4gRG8gdGhlIG5hdGl2ZSBjaGlwIHNlbGVjdHMgYWN0dWFsbHkgd29yayB1c2Vm dWxseSBvbiB0aGlzIGhhcmR3YXJlPw0KPiA+ID4gVGhlcmUgdXNlZCB0byBiZSBwcm9ibGVtcyB3 aXRoIGl0IHdhbnRpbmcgdG8gZG8gdGhpbmdzIGxpa2UgYm91bmNlIHRoZQ0KPiA+ID4gY2hpcCBz ZWxlY3Qgb24gZXZlcnkgd29yZCB3aGljaCBtYWRlIGl0IGV4dHJlbWVseSBkaWZmaWN1bHQgdG8g dXNlIHdpdGgNCj4gPiA+IExpbnV4Lg0KPiA+IFN0aWxsIGFyZSBhbm5veWluZywgYnV0IG9uIHRo ZSBkZXZpY2Ugd2UgaGF2ZSBjb25uZWN0ZWQgdG8gaXQsIGl0IGVuZHMNCj4gPiB1cCB3b3JraW5n IGFzIGRlc2lyZWQuDQo+ID4gSSd2ZSBub3QgdGhvcm91Z2hseSBpbnZlc3RpZ2F0ZWQgdGhpcyBo YXJkd2FyZSB0byBmaW5kIHRoZSBkZXRhaWxzLiANCj4gPiBJSVJDLCB0aGUgZGVzaWdud2FyZSBT UEkgb24gQWx0ZXJhIFNvQ0ZQR0EgaGFkIHRoZSBzYW1lIGlzc3VlLCBidXQgaXQNCj4gPiB3YXMg YSBmbGF3IGluIHRoZSBkcml2ZXIgYW5kIEkgd2FzIGFibGUgdG8gZml4IGl0LiAgSSd2ZSBjb21l IHRvIGV4cGVjdA0KPiA+IGl0LCBhcyBldmVyeSBuZXcgU1BJIG1hc3RlciBJIHVzZSBkb2Vzbid0 IHdvcmsgcHJvcGVybHkgaW4gc29tZQ0KPiA+IGRpZmZlcmVudCB3YXkuDQo+IA0KPiBJdCdzIG9u ZSBvZiB0aGUgcmVhc29ucyB3aHkgSSdtIHN1c3BpY291cyBvZiBtYWtpbmcgdGhlIEdQSU8gb3B0 aW9uYWwsDQo+IGxvdHMgb2YgY2hpcHMgdXNlIEdQSU8gY2hpcHNlbGVjdHMgYmVjYXVzZSBhIGxv dCBvZiB0aGUgYnJlYWthZ2UgaXMNCj4gY29uZmluZWQgdG8gY2hpcCBzZWxlY3QgaGFuZGxpbmcg YW5kIEkgcmVtZW1iZXIgdGhlIGkuTVggYXMgYmVpbmcNCj4gZXNwZWNpYWxseSBmYXIgZnJvbSB1 c2VmdWwuDQoNCkp1c3QgdG8gYmUgY2xlYXIsIG9uZSBkb2Vzbid0IG5lZWQgdG8gdXNlIEdQSU9z IHdpdGggdGhlIGRyaXZlciBhcyBpcy4gDQpCdXQgdGhlIGJpbmRpbmdzIHRvIGRvIHRoYXQgYXJl IG5vbi1zdGFuZGFyZCBhbmQgdGhlc2UgcGF0Y2hlcyBtYWtlIHRoZQ0KZHJpdmVyIGZvbGxvdyB0 aGUgc3RhbmRhcmQuIChhbmQgZG9uJ3QgYnJlYWsgYW55b25lKS4NCg0KSXQncyB1bmZvcnR1bmF0 ZSB0aGF0IHRoaXMsIGFuZCBpbiBteSBleHBlcmllbmNlIGV2ZXJ5LCBTUEkgbWFzdGVyDQpkb2Vz bid0IGFsd2F5cyAob3IgZXZlcikgZ2VuZXJhdGUgdGhlIHdhdmVmb3JtcyBpdCBzaG91bGQgYWNj b3JkaW5nIHRvDQp0aGUgTGludXggQVBJLiAgQnV0IEkgZG9uJ3QgdGhpbmsgbm90IGZvbGxvd2lu ZyB0aGUgc3BlY2lmaWNhdGlvbiBmb3INCnRoZSBkZXZpY2UgdHJlZSBiaW5kaW5ncyBpcyBhIG1p dGlnYXRpb24gb2YgdGhpcy4gIElmIGFueXRoaW5nLCBpdCBqdXN0DQpjcmVhdGVzIG1vcmUgcHJv YmxlbXMuDQoNCkFzIHNvbWVvbmUgd2hvIGhhcyBkb25lIGJyaW5ndXAgYSBtb3JlIHRoYW4gYSBm ZXcgZGV2aWNlcywgSSBrbm93IHdoYXQNCkkgd2FudCB0aGUgaGFyZHdhcmUgdG8gZG8sIGFuZCBJ IGtub3cgdGhlIHByb3BlciBiaW5kaW5ncyB0byBkbyB0aGF0LiANCkJ1dCBzb21lb25lIGNvbWVz IGJhY2sgYW5kIHNheXMgdGhleSBkb24ndCB3b3JrIGFuZCBub3cgSSBuZWVkIHRvIGRpZw0KaW50 byB0aGUgZHJpdmVyIHNvdXJjZSBhbmQgZmlndXJlIG91dCB3aGF0IGl0cyBwYXJ0aWN1bGFyIGZs YXZvciBvZg0Kbm9uLXN0YW5kYXJkIGJlaGF2aW9yIGlzLiAgSSBkb24ndCB0aGluayB0aGF0IGhl bHBlZCBtZSBhbnkuICBJdCBkaWRuJ3QNCnRlbGwgd2hhdCB0aGUgaGFyZHdhcmUvZHJpdmVyJ3Mg cXVpcmtzIGFyZSB3LnIudC4gYWJpbGl0eSB0byBmb2xsb3cgdGhlDQpMaW51eCBTUEkgQVBJIGVp dGhlci4NCg0KVGhpcyBhbHNvIGhhcHBlbnMgYWZ0ZXIgdGhlIGhhcmR3YXJlIGlzIGRlc2lnbmVk IGFuZCBidWlsdCwgc28gaXQncyBhDQpsaXR0bGUgbGF0ZSB0byBjaG9vc2UgYW5vdGhlciBwaW4u DQoNCklmIHRoZSBnb2FsIGlzIHRvIGRvY3VtZW50IHRoZSBoYXJkd2FyZSBxdWlya3MsIHRoZW4g c2hvdWxkbid0IHRoaXMgYmUNCmRvbmUgdGhyb3VnaCBkb2N1bWVudGF0aW9uPyAgQSBub3RlIG9y IHBvaW50ZXIgaW4gdGhlIGtjb25maWcsDQpzb21ldGhpbmcgaW4gdGhlIHNvdXJjZSwgYSBiZXR0 ZXIgZGVzY3JpcHRpb24gaW4gRG9jdW1lbnRpb24vDQpzb21ld2hlcmUsIGV0Yy4gIFRoYXQgd291 bGQgaGF2ZSBhIGJldHRlciBjaGFuY2Ugb2YgYmVpbmcgc2VlbiBiZWZvcmUNCmhhcmR3YXJlIGlz IGRlc2lnbmVkLCBhbmQgd291bGQgZXhwbGFpbiB0aGUgaXNzdWUgdG9vLCBpbnN0ZWFkIG9mIGp1 c3QNCmFwcGVhcmluZyBhcyBhbm90aGVyIHF1aXJrIGluIGRldmljZSB0cmVlIGJpbmRpbmdzLg0K DQpBbHNvIGNvbnNpZGVyIHRoZXJlIGlzIGEgbG90IG9mIHJlLWltcGxlbWVudGVkIGNvZGUgaW4g ZWFjaCBkcml2ZXINCnJlbGF0aW5nIHRvIGRldmljZSB0cmVlIHBhcnNpbmcgYW5kIGFsc28gR1BJ TyBDUy4gIE5vbi1zdGFuZGFyZA0KYmVoYXZpb3IgaW4gYSBkcml2ZXIgZG9lc24ndCBoZWxwIGFu eSByZWZhY3RvcmluZyBlZmZvcnQu -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 03, 2017 at 05:53:59PM +0000, Trent Piepho wrote: > Just to be clear, one doesn't need to use GPIOs with the driver as is. > But the bindings to do that are non-standard and these patches make the > driver follow the standard. (and don't break anyone). If there are non-standard bindings then mark them as deprecated. I can't immediately find *any* binding documentation for this controller. The last commit looks like it was more attempting to work round broken board bindings and do something sensible than add a new binding, at least that's what I remember my sense of it being. > It's unfortunate that this, and in my experience every, SPI master > doesn't always (or ever) generate the waveforms it should according to > the Linux API. But I don't think not following the specification for > the device tree bindings is a mitigation of this. If anything, it just > creates more problems. If the hardware is as broken as these controllers always were in the past and there are workarounds which work in all practical situations (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins) documenting something as supported is just going to make people miserable. The reason I know about this breakage is that I had to go through the process of working out that the native chip select support didn't work on a system. > If the goal is to document the hardware quirks, then shouldn't this be > done through documentation? A note or pointer in the kconfig, > something in the source, a better description in Documention/ > somewhere, etc. That would have a better chance of being seen before > hardware is designed, and would explain the issue too, instead of just > appearing as another quirk in device tree bindings. Yes, better documentation would be great.
T24gRnJpLCAyMDE3LTExLTAzIGF0IDE4OjM3ICswMDAwLCBNYXJrIEJyb3duIHdyb3RlOg0KPiBP biBGcmksIE5vdiAwMywgMjAxNyBhdCAwNTo1Mzo1OVBNICswMDAwLCBUcmVudCBQaWVwaG8gd3Jv dGU6DQo+IA0KPiA+IEp1c3QgdG8gYmUgY2xlYXIsIG9uZSBkb2Vzbid0IG5lZWQgdG8gdXNlIEdQ SU9zIHdpdGggdGhlIGRyaXZlciBhcyBpcy4gDQo+ID4gQnV0IHRoZSBiaW5kaW5ncyB0byBkbyB0 aGF0IGFyZSBub24tc3RhbmRhcmQgYW5kIHRoZXNlIHBhdGNoZXMgbWFrZSB0aGUNCj4gPiBkcml2 ZXIgZm9sbG93IHRoZSBzdGFuZGFyZC4gKGFuZCBkb24ndCBicmVhayBhbnlvbmUpLg0KPiANCj4g SWYgdGhlcmUgYXJlIG5vbi1zdGFuZGFyZCBiaW5kaW5ncyB0aGVuIG1hcmsgdGhlbSBhcyBkZXBy ZWNhdGVkLiAgSQ0KPiBjYW4ndCBpbW1lZGlhdGVseSBmaW5kICphbnkqIGJpbmRpbmcgZG9jdW1l bnRhdGlvbiBmb3IgdGhpcyBjb250cm9sbGVyLg0KPiBUaGUgbGFzdCBjb21taXQgbG9va3MgbGlr ZSBpdCB3YXMgbW9yZSBhdHRlbXB0aW5nIHRvIHdvcmsgcm91bmQgYnJva2VuDQo+IGJvYXJkIGJp bmRpbmdzIGFuZCBkbyBzb21ldGhpbmcgc2Vuc2libGUgdGhhbiBhZGQgYSBuZXcgYmluZGluZywg YXQNCj4gbGVhc3QgdGhhdCdzIHdoYXQgSSByZW1lbWJlciBteSBzZW5zZSBvZiBpdCBiZWluZy4N Cg0KVGhlIG5vbi1zdGFuZGFyZCBwYXJ0IGlzIG5lZWRpbmcgdG8gYWRkIGNzLWdwaW9zID0gPDA+ IHRvIGdldCBhIG5hdGl2ZQ0KY2hpcCBzZWxlY3Qgd2hlbiB0aGF0IGlzIGRvY3VtZW50ZWQgYXMg YmVpbmcgb3B0aW9uYWwuICBJdCBkb2Vzbid0DQpmb2xsb3cgdGhlIHNwZWMuICBJdCBkb2Vzbid0 IG1hdGNoIG90aGVyIGRyaXZlcnMgKGFuZCBkdy1zcGkgaXMgZXF1YWxseQ0KYXMgYnJva2VuIGlu IHRoZSBzYW1lIG1hbm5lciwgcHJvYmFibHkgb3RoZXJzIHRvbykgdGhhdCBkbyBmb2xsb3cgdGhl DQpzcGVjLg0KDQo+IElmIHRoZSBoYXJkd2FyZSBpcyBhcyBicm9rZW4gYXMgdGhlc2UgY29udHJv bGxlcnMgYWx3YXlzIHdlcmUgaW4gdGhlDQo+IHBhc3QgYW5kIHRoZXJlIGFyZSB3b3JrYXJvdW5k cyB3aGljaCB3b3JrIGluIGFsbCBwcmFjdGljYWwgc2l0dWF0aW9ucw0KPiAoQUZBSUsgYWxsIHRo ZSByZWxldmFudCBTb0NzIHBlcm1pdCBHUElPIHVzYWdlIG9uIHRoZSBjaGlwIHNlbGVjdCBwaW5z KQ0KDQpDb21tZW50cyBpbiB0aGUgZHJpdmVyIGluZGljYXRlIHRoYXQgc29tZSBTb0NzIGRvIG5v dCBhbGxvdyBHUElPIHVzYWdlDQpvbiBhbGwgY2hpcCBzZWxlY3QgcGlucy4NCg0KPiBkb2N1bWVu dGluZyBzb21ldGhpbmcgYXMgc3VwcG9ydGVkIGlzIGp1c3QgZ29pbmcgdG8gbWFrZSBwZW9wbGUN Cj4gbWlzZXJhYmxlLiAgVGhlIHJlYXNvbiBJIGtub3cgYWJvdXQgdGhpcyBicmVha2FnZSBpcyB0 aGF0IEkgaGFkIHRvIGdvDQo+IHRocm91Z2ggdGhlIHByb2Nlc3Mgb2Ygd29ya2luZyBvdXQgdGhh dCB0aGUgbmF0aXZlIGNoaXAgc2VsZWN0IHN1cHBvcnQNCj4gZGlkbid0IHdvcmsgb24gYSBzeXN0 ZW0uDQoNCkkganVzdCBkb24ndCBzZWUgaG93IG5vdCBmb2xsb3dpbmcgdGhlIGRldmljZSB0cmVl IGJpbmRpbmcNCnNwZWNpZmljYXRpb24gZG9jdW1lbnRzIHRoZSBoYXJkd2FyZSBmbGF3LCBvciBo b3cgZm9sbG93aW5nIHRoZSBzcGVjDQpkb2N1bWVudHMgdGhhdCB0aGUgZmxhdyBkb2VzIG5vdCBl eGlzdC4NCg0KPiA+IElmIHRoZSBnb2FsIGlzIHRvIGRvY3VtZW50IHRoZSBoYXJkd2FyZSBxdWly a3MsIHRoZW4gc2hvdWxkbid0IHRoaXMgYmUNCj4gPiBkb25lIHRocm91Z2ggZG9jdW1lbnRhdGlv bj8gIEEgbm90ZSBvciBwb2ludGVyIGluIHRoZSBrY29uZmlnLA0KPiA+IHNvbWV0aGluZyBpbiB0 aGUgc291cmNlLCBhIGJldHRlciBkZXNjcmlwdGlvbiBpbiBEb2N1bWVudGlvbi8NCj4gPiBzb21l d2hlcmUsIGV0Yy4gIFRoYXQgd291bGQgaGF2ZSBhIGJldHRlciBjaGFuY2Ugb2YgYmVpbmcgc2Vl biBiZWZvcmUNCj4gPiBoYXJkd2FyZSBpcyBkZXNpZ25lZCwgYW5kIHdvdWxkIGV4cGxhaW4gdGhl IGlzc3VlIHRvbywgaW5zdGVhZCBvZiBqdXN0DQo+ID4gYXBwZWFyaW5nIGFzIGFub3RoZXIgcXVp cmsgaW4gZGV2aWNlIHRyZWUgYmluZGluZ3MuDQo+IA0KPiBZZXMsIGJldHRlciBkb2N1bWVudGF0 aW9uIHdvdWxkIGJlIGdyZWF0Lg0KDQpIb3cgYWJvdXQgSSBhZGQgc29tZXRoaW5nIHRvIERvY3Vt ZW50YXRpb24vc3BpIGFuZCBhZGQgYSBub3RlIGluDQpLY29uZmlnLCBidXQgbWFrZSB0aGUgZHJp dmVyIHN0YW5kYXJkIGNvbXBsaWFudCBpbiBpdHMgZGV2aWNlIHRyZWUNCmJpbmRpbmdzPw0KDQpU aGUgZ29hbCBoZXJlIGlzIHRoYXQgZXZlcnlvbmUgZG9lc24ndCBoYXZlIHRvIHN0aWNrIGEgc2Nv cGUgb24gdGhlDQpib2FyZCBhbmQgcmUtZGlzY292ZXIgdGhhdCB0aGUgbmF0aXZlIENTIGRvZXNu J3QgZG8gd2hhdCB0aGV5IHdhbnQsDQpyaWdodD8gQmVlbiB0aGVyZSwgZG9uZSB0aGF0LCBhbmQg Y2FuIGFwcHJlY2lhdGUgdGhlIHNlbnRpbWVudC4NCg0KSSBkb24ndCB0aGluayBub3QgZm9sbG93 aW5nIHRoZSBzdGFuZGFyZCBpcyBhbiBlZmZlY3RpdmUgd2F5IHRvIGRvDQp0aGF0LiAgRnJvbSBh IHNhbXBsZSBvZiB0aHJlZSBkZXZlbG9wZXJzLCB0d28gY2FtZSB1cCB3aXRoIGR0IGJpbmRpbmdz DQp0byB1c2UgbmF0aXZlIGNzIGFuZCBkZWNpZGVkIHRoZXkgYXBwYXJlbnRseSBtaXN1bmRlcnN0 b29kIHRoZSBzcGkgZHQNCnNwZWMgdG8gZXhwbGFpbiB3aHkgd2hhdCBzaG91bGQgaGF2ZSB3b3Jr ZWQgZGlkIG5vdC4gIFRoZSB0aGlyZCAobWUpDQpsb29rZWQgaW50byB0aGUgZHJpdmVyIHNvdXJj ZSB0byBmaW5kIHRoZSB3aHksIGFuZCBldmVuIHRoZW4gaXQgd2Fzbid0DQpjbGVhciB0aGUgYmVo YXZpb3Igd2FzIGludGVudGlvbmFsIG9yIGFuIG92ZXJzaWdodC4gIEkgdGhpbmsNCmRvY3VtZW50 aW5nIHRoZSBrbm93biBmbGF3cyB3b3VsZCBiZSBhIGJldHRlciBhbmQgbW9yZSBlZmZlY3RpdmUg d2F5IHRvDQpnZXQgdGhhdCBpbmZvcm1hdGlvbiBhY3Jvc3Mu -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote: > On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote: > > If there are non-standard bindings then mark them as deprecated. I > > can't immediately find *any* binding documentation for this controller. > > The last commit looks like it was more attempting to work round broken > > board bindings and do something sensible than add a new binding, at > > least that's what I remember my sense of it being. > The non-standard part is needing to add cs-gpios = <0> to get a native > chip select when that is documented as being optional. It doesn't > follow the spec. It doesn't match other drivers (and dw-spi is equally > as broken in the same manner, probably others too) that do follow the > spec. Is there any documentation of the bindings for this driver at all? I wasn't able to find it. > > If the hardware is as broken as these controllers always were in the > > past and there are workarounds which work in all practical situations > > (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins) > Comments in the driver indicate that some SoCs do not allow GPIO usage > on all chip select pins. Ah, that's an issue. We will need support for the hardware chip selects then. > > documenting something as supported is just going to make people > > miserable. The reason I know about this breakage is that I had to go > > through the process of working out that the native chip select support > > didn't work on a system. > I just don't see how not following the device tree binding > specification documents the hardware flaw, or how following the spec > documents that the flaw does not exist. Hardware chip selects aren't present in all controllers and at times have entertaining enumerations in the hardware, the details on them need to be covered in the device specific binding (which like I say seems to be missing for this controller). If they just don't work well the kindest thing may be to not support them and document the binding that way. > > Yes, better documentation would be great. > How about I add something to Documentation/spi and add a note in > Kconfig, but make the driver standard compliant in its device tree > bindings? Writing a binding document for the controller would probably cover it.
T24gRnJpLCAyMDE3LTExLTAzIGF0IDE5OjM2ICswMDAwLCBNYXJrIEJyb3duIHdyb3RlOg0KPiBP biBGcmksIE5vdiAwMywgMjAxNyBhdCAwNzoxODo1NlBNICswMDAwLCBUcmVudCBQaWVwaG8gd3Jv dGU6DQo+ID4gT24gRnJpLCAyMDE3LTExLTAzIGF0IDE4OjM3ICswMDAwLCBNYXJrIEJyb3duIHdy b3RlOg0KPiA+ID4gSWYgdGhlcmUgYXJlIG5vbi1zdGFuZGFyZCBiaW5kaW5ncyB0aGVuIG1hcmsg dGhlbSBhcyBkZXByZWNhdGVkLiAgSQ0KPiA+ID4gY2FuJ3QgaW1tZWRpYXRlbHkgZmluZCAqYW55 KiBiaW5kaW5nIGRvY3VtZW50YXRpb24gZm9yIHRoaXMgY29udHJvbGxlci4NCj4gPiA+IFRoZSBs YXN0IGNvbW1pdCBsb29rcyBsaWtlIGl0IHdhcyBtb3JlIGF0dGVtcHRpbmcgdG8gd29yayByb3Vu ZCBicm9rZW4NCj4gPiA+IGJvYXJkIGJpbmRpbmdzIGFuZCBkbyBzb21ldGhpbmcgc2Vuc2libGUg dGhhbiBhZGQgYSBuZXcgYmluZGluZywgYXQNCj4gPiA+IGxlYXN0IHRoYXQncyB3aGF0IEkgcmVt ZW1iZXIgbXkgc2Vuc2Ugb2YgaXQgYmVpbmcuDQo+ID4gVGhlIG5vbi1zdGFuZGFyZCBwYXJ0IGlz IG5lZWRpbmcgdG8gYWRkIGNzLWdwaW9zID0gPDA+IHRvIGdldCBhIG5hdGl2ZQ0KPiA+IGNoaXAg c2VsZWN0IHdoZW4gdGhhdCBpcyBkb2N1bWVudGVkIGFzIGJlaW5nIG9wdGlvbmFsLiAgSXQgZG9l c24ndA0KPiA+IGZvbGxvdyB0aGUgc3BlYy4gIEl0IGRvZXNuJ3QgbWF0Y2ggb3RoZXIgZHJpdmVy cyAoYW5kIGR3LXNwaSBpcyBlcXVhbGx5DQo+ID4gYXMgYnJva2VuIGluIHRoZSBzYW1lIG1hbm5l ciwgcHJvYmFibHkgb3RoZXJzIHRvbykgdGhhdCBkbyBmb2xsb3cgdGhlDQo+ID4gc3BlYy4NCj4g DQo+IElzIHRoZXJlIGFueSBkb2N1bWVudGF0aW9uIG9mIHRoZSBiaW5kaW5ncyBmb3IgdGhpcyBk cml2ZXIgYXQgYWxsPyAgSQ0KPiB3YXNuJ3QgYWJsZSB0byBmaW5kIGl0Lg0KDQpEb2N1bWVudGF0 aW9uL2RldmljZXRyZWUvYmluZGluZ3Mvc3BpL2ZzbC1pbXgtY3NwaS50eHQNCg0KRG9lc24ndCBu b3RlIGFueXRoaW5nIHNwZWNpYWwgYWJvdXQgdGhlIG5hdGl2ZSBjaGlwIHNlbGVjdHMuDQoNCj4g PiA+IElmIHRoZSBoYXJkd2FyZSBpcyBhcyBicm9rZW4gYXMgdGhlc2UgY29udHJvbGxlcnMgYWx3 YXlzIHdlcmUgaW4gdGhlDQo+ID4gPiBwYXN0IGFuZCB0aGVyZSBhcmUgd29ya2Fyb3VuZHMgd2hp Y2ggd29yayBpbiBhbGwgcHJhY3RpY2FsIHNpdHVhdGlvbnMNCj4gPiA+IChBRkFJSyBhbGwgdGhl IHJlbGV2YW50IFNvQ3MgcGVybWl0IEdQSU8gdXNhZ2Ugb24gdGhlIGNoaXAgc2VsZWN0IHBpbnMp DQo+ID4gQ29tbWVudHMgaW4gdGhlIGRyaXZlciBpbmRpY2F0ZSB0aGF0IHNvbWUgU29DcyBkbyBu b3QgYWxsb3cgR1BJTyB1c2FnZQ0KPiA+IG9uIGFsbCBjaGlwIHNlbGVjdCBwaW5zLg0KPiANCj4g QWgsIHRoYXQncyBhbiBpc3N1ZS4gIFdlIHdpbGwgbmVlZCBzdXBwb3J0IGZvciB0aGUgaGFyZHdh cmUgY2hpcCBzZWxlY3RzDQo+IHRoZW4uDQoNCkFuZCB0aGV5IGFyZSBhbHJlYWR5IHN1cHBvcnRl ZCB0b28uICBUaGUgaXNzdWUgSSB3YW50IHRvIGZpeCBpcyB0aGUNCm5lZWQgZm9yIG5vbi1zdGFu ZGFyZCBiaW5kaW5ncyB0byB1c2UgdGhlbS4NCg0KPiA+ID4gZG9jdW1lbnRpbmcgc29tZXRoaW5n IGFzIHN1cHBvcnRlZCBpcyBqdXN0IGdvaW5nIHRvIG1ha2UgcGVvcGxlDQo+ID4gPiBtaXNlcmFi bGUuICBUaGUgcmVhc29uIEkga25vdyBhYm91dCB0aGlzIGJyZWFrYWdlIGlzIHRoYXQgSSBoYWQg dG8gZ28NCj4gPiA+IHRocm91Z2ggdGhlIHByb2Nlc3Mgb2Ygd29ya2luZyBvdXQgdGhhdCB0aGUg bmF0aXZlIGNoaXAgc2VsZWN0IHN1cHBvcnQNCj4gPiA+IGRpZG4ndCB3b3JrIG9uIGEgc3lzdGVt Lg0KPiA+IEkganVzdCBkb24ndCBzZWUgaG93IG5vdCBmb2xsb3dpbmcgdGhlIGRldmljZSB0cmVl IGJpbmRpbmcNCj4gPiBzcGVjaWZpY2F0aW9uIGRvY3VtZW50cyB0aGUgaGFyZHdhcmUgZmxhdywg b3IgaG93IGZvbGxvd2luZyB0aGUgc3BlYw0KPiA+IGRvY3VtZW50cyB0aGF0IHRoZSBmbGF3IGRv ZXMgbm90IGV4aXN0Lg0KPiANCj4gSGFyZHdhcmUgY2hpcCBzZWxlY3RzIGFyZW4ndCBwcmVzZW50 IGluIGFsbCBjb250cm9sbGVycyBhbmQgYXQgdGltZXMNCj4gaGF2ZSBlbnRlcnRhaW5pbmcgZW51 bWVyYXRpb25zIGluIHRoZSBoYXJkd2FyZSwgdGhlIGRldGFpbHMgb24gdGhlbSBuZWVkDQo+IHRv IGJlIGNvdmVyZWQgaW4gdGhlIGRldmljZSBzcGVjaWZpYyBiaW5kaW5nICh3aGljaCBsaWtlIEkg c2F5IHNlZW1zIHRvDQo+IGJlIG1pc3NpbmcgZm9yIHRoaXMgY29udHJvbGxlcikuICBJZiB0aGV5 IGp1c3QgZG9uJ3Qgd29yayB3ZWxsIHRoZQ0KPiBraW5kZXN0IHRoaW5nIG1heSBiZSB0byBub3Qg c3VwcG9ydCB0aGVtIGFuZCBkb2N1bWVudCB0aGUgYmluZGluZyB0aGF0DQo+IHdheS4NCj4gDQo+ ID4gPiBZZXMsIGJldHRlciBkb2N1bWVudGF0aW9uIHdvdWxkIGJlIGdyZWF0Lg0KPiA+IEhvdyBh Ym91dCBJIGFkZCBzb21ldGhpbmcgdG8gRG9jdW1lbnRhdGlvbi9zcGkgYW5kIGFkZCBhIG5vdGUg aW4NCj4gPiBLY29uZmlnLCBidXQgbWFrZSB0aGUgZHJpdmVyIHN0YW5kYXJkIGNvbXBsaWFudCBp biBpdHMgZGV2aWNlIHRyZWUNCj4gPiBiaW5kaW5ncz8NCj4gDQo+IFdyaXRpbmcgYSBiaW5kaW5n IGRvY3VtZW50IGZvciB0aGUgY29udHJvbGxlciB3b3VsZCBwcm9iYWJseSBjb3ZlciBpdC4NCg0K T2ssIEknbGwgYWRqdXN0IHRoZSBzZXJpZXMgdG8gZG9jdW1lbnQgdGhlIHJlYWwgaXNzdWUu -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 03, 2017 at 08:09:19PM +0000, Trent Piepho wrote: > On Fri, 2017-11-03 at 19:36 +0000, Mark Brown wrote: > > Is there any documentation of the bindings for this driver at all? I > > wasn't able to find it. > Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt Ah, good - for some reason I'd thought that was a different IP block (SPI controllers are like serial ports, endless room for innovation!).
On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote: > On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote: > > On Fri, Nov 03, 2017 at 05:53:59PM +0000, Trent Piepho wrote: > > > > > Just to be clear, one doesn't need to use GPIOs with the driver as is. > > > But the bindings to do that are non-standard and these patches make the > > > driver follow the standard. (and don't break anyone). > > > > If there are non-standard bindings then mark them as deprecated. I > > can't immediately find *any* binding documentation for this controller. > > The last commit looks like it was more attempting to work round broken > > board bindings and do something sensible than add a new binding, at > > least that's what I remember my sense of it being. > > The non-standard part is needing to add cs-gpios = <0> to get a native > chip select when that is documented as being optional. It doesn't > follow the spec. It doesn't match other drivers (and dw-spi is equally > as broken in the same manner, probably others too) that do follow the > spec. > > > If the hardware is as broken as these controllers always were in the > > past and there are workarounds which work in all practical situations > > (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins) > > Comments in the driver indicate that some SoCs do not allow GPIO usage > on all chip select pins. Which comments do you mean? I remember there were comments, but I can't find any in the driver. The only SoC which has dedicated SPI CS pins which can't be configured as GPIOs is the i.MX31. Unless you have an i.MX31 you shouldn't be forced to use native chip selects. Sascha
On Mon, 2017-11-06 at 08:32 +0100, Sascha Hauer wrote: > On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote: > > > > Comments in the driver indicate that some SoCs do not allow GPIO usage > > on all chip select pins. > > Which comments do you mean? I remember there were comments, but I can't > find any in the driver. It's in arch/arm/plat-mxc/include/mach/spi.h. > The only SoC which has dedicated SPI CS pins which can't be configured > as GPIOs is the i.MX31. Unless you have an i.MX31 you shouldn't be > forced to use native chip selects. Indeed I'm not forced to, but native is more efficient and faster, and since I'm lucky enough that they work for me, I'd like to be able to use them. And I can already, but the DT bindings are non-standard and I don't see any value in that. DT bindings are complex enough without drivers adding quirks. And my experience was no one connected the non- standard bindings with native CS not working well. I think explicit documentation will be more effective at that.
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index babb15f07995..07e6250f2dad 100644 --- a/drivers/spi/spi-imx.c +++ b/drivers/spi/spi-imx.c @@ -1457,22 +1457,19 @@ static int spi_imx_probe(struct platform_device *pdev) goto out_clk_put; } - if (!master->cs_gpios) { - dev_err(&pdev->dev, "No CS GPIOs available\n"); - ret = -EINVAL; - goto out_clk_put; - } - - for (i = 0; i < master->num_chipselect; i++) { - if (!gpio_is_valid(master->cs_gpios[i])) - continue; - - ret = devm_gpio_request(&pdev->dev, master->cs_gpios[i], - DRIVER_NAME); - if (ret) { - dev_err(&pdev->dev, "Can't get CS GPIO %i\n", - master->cs_gpios[i]); - goto out_clk_put; + /* Request GPIO CS lines, if any */ + if (master->cs_gpios) { + for (i = 0; i < master->num_chipselect; i++) { + if (!gpio_is_valid(master->cs_gpios[i])) + continue; + + ret = devm_gpio_request(&pdev->dev, master->cs_gpios[i], + DRIVER_NAME); + if (ret) { + dev_err(&pdev->dev, "Can't get CS GPIO %i\n", + master->cs_gpios[i]); + goto out_clk_put; + } } }