Question about max frequency in ad2s90

Question about max frequency in ad2s90

[Matheus Tavares Bernardino]

Hello everyone,

While working on the ad2s90 driver, I had a question about the maximum
frequency set. Any feedback will be much appreciated.

The ad2s90 driver sets its maximum speed to 830000hz (or 0.83Mhz), but
the datasheet [1] specifies a maximum of 2Mhz. Above the max_speed_hz
setup, there's a commentary saying  "need 600ns between CS and the
first falling edge of SCLK", which is also said in the datasheet.

Is max_speed_hz set to 0.83Mhz, not 2Mhz, due to the required minimum
delay of 600ns, somehow? If not:
  1) is 0.83Mhz really correct? and
  2) is the minimum delay of 600ns being respected?

Thanks,
Matheus.

[1] https://www.analog.com/media/en/technical-documentation/data-sheets/AD2S90.pdf

Re: Question about max frequency in ad2s90

[Ardelean, Alexandru]

T24gTW9uLCAyMDE4LTExLTA1IGF0IDIzOjExIC0wMjAwLCBNYXRoZXVzIFRhdmFyZXMgQmVybmFy
ZGlubyB3cm90ZToNCj4gSGVsbG8gZXZlcnlvbmUsDQo+IA0KDQpIZXksDQoNCj4gV2hpbGUgd29y
a2luZyBvbiB0aGUgYWQyczkwIGRyaXZlciwgSSBoYWQgYSBxdWVzdGlvbiBhYm91dCB0aGUgbWF4
aW11bQ0KPiBmcmVxdWVuY3kgc2V0LiBBbnkgZmVlZGJhY2sgd2lsbCBiZSBtdWNoIGFwcHJlY2lh
dGVkLg0KPiANCg0KSnVzdCBhIG5vdGUvcmVtaW5kZXIgZm9yIHlvdTogdGhhdCBkcml2ZXIgbG9v
a3MgcHJldHR5IG9sZCBhbmQgW3NpbmNlIGl0J3MNCnN0YWdpbmcgYXMgd2VsbF0geW91IHNob3Vs
ZCBhbHNvIGNvbnNpZGVyIHRoYXQgaXQncyBsaWtlbHkgdGhhdCBzb21lIFtnb29kXQ0KcHJhY3Rp
Y2VzIHdlcmUgbm90IHlldCBleGlzdGluZyBhdCB0aGUgdGltZSBbYWJvdXQgaG93IHRvIGRvIHRo
aW5nc10uDQoNCj4gVGhlIGFkMnM5MCBkcml2ZXIgc2V0cyBpdHMgbWF4aW11bSBzcGVlZCB0byA4
MzAwMDBoeiAob3IgMC44M01oeiksIGJ1dA0KPiB0aGUgZGF0YXNoZWV0IFsxXSBzcGVjaWZpZXMg
YSBtYXhpbXVtIG9mIDJNaHouIEFib3ZlIHRoZSBtYXhfc3BlZWRfaHoNCj4gc2V0dXAsIHRoZXJl
J3MgYSBjb21tZW50YXJ5IHNheWluZyAgIm5lZWQgNjAwbnMgYmV0d2VlbiBDUyBhbmQgdGhlDQo+
IGZpcnN0IGZhbGxpbmcgZWRnZSBvZiBTQ0xLIiwgd2hpY2ggaXMgYWxzbyBzYWlkIGluIHRoZSBk
YXRhc2hlZXQuDQo+IA0KPiBJcyBtYXhfc3BlZWRfaHogc2V0IHRvIDAuODNNaHosIG5vdCAyTWh6
LCBkdWUgdG8gdGhlIHJlcXVpcmVkIG1pbmltdW0NCj4gZGVsYXkgb2YgNjAwbnMsIHNvbWVob3c/
IElmIG5vdDoNCj4gICAxKSBpcyAwLjgzTWh6IHJlYWxseSBjb3JyZWN0PyBhbmQNCj4gICAyKSBp
cyB0aGUgbWluaW11bSBkZWxheSBvZiA2MDBucyBiZWluZyByZXNwZWN0ZWQ/DQo+IA0KDQpUaGUg
MC44MyBNaHogbGltaXRhdGlvbiBsb29rcyB2ZXJ5IG11Y2ggbGlrZSBpdCB3YXMgYWRkZWQgdG8g
c2F0aXNmeSB0aGUNCjYwMCBucyBsaW1pdGF0aW9uLiBJIHF1aWNrbHkgZGlkIHRoZSBjb21wdXRh
dGlvbiBhbmQgaXQgcm91Z2hseSBlbmRzIHVwDQpiZWluZyAwLjgzIE1oeiBmb3IgNjAwIG5zLiAo
aS5lLiAoMSAvIDAsMDAwNjAwKSAvIDIgPSBoYWxmIGEgcGVyaW9kICkNCg0KVGhhdCBsaW1pdGF0
aW9uIChmb3IgdGhlIGRlbGF5IENTIGFuZCB0aGUgZmlyc3QgZGF0YSkgaXMgaW1wb3NlZCBieSB0
aGUNCmRhdGFzaGVldCA7IHdoaWxlIHRoZSBjaGlwIGNvdWxkIG9wZXJhdGUgYXQgYSBsb3dlciBk
ZWxheSAoYW5kIHN1YnNlcXVlbnRseQ0KaGlnaGVyIGZyZXF1ZW5jeSB1cCB0byAyIE1oeiksIGl0
J3Mgc2FmZXIgdG8ga2VlcCBpdCBzb21laG93IGltcGxlbWVudGVkLg0KDQpJIHRvb2sgYSBsb29r
IGluIHRoZSBTUEkgY29kZSBhbmQgaXQgZG9lc24ndCBsb29rIGxpa2UgaXQgaW1wbGVtZW50cyBz
b21lDQpzb3J0IG9mIGRlbGF5IGJldHdlZW4gQ1MgYW5kIGZpcnN0IGRhdGEgYmVpbmcgcmVhZC4g
SSBtYXkgaGF2ZSBtaXNzZWQgc3R1ZmYNCnRob3VnaC4NCg0KSW4gYW55IGNhc2UsIEkgZG8gc3Vn
Z2VzdCByZW1vdmluZyBhbGwgdGhvc2UgNCBsaW5lcyBmb3IgdGhlIFNQSSBzZXR1cA0KY29kZS4N
CmkuZS4NCg0KICAgICAgICAvKiBuZWVkIDYwMG5zIGJldHdlZW4gQ1MgYW5kIHRoZSBmaXJzdCBm
YWxsaW5nIGVkZ2Ugb2YgU0NMSyAqLw0KICAgICAgICBzcGktPm1heF9zcGVlZF9oeiA9IDgzMDAw
MDsNCiAgICAgICAgc3BpLT5tb2RlID0gU1BJX01PREVfMzsNCiAgICAgICAgc3BpX3NldHVwKHNw
aSk7DQoNClRoaXMgc2hvdWxkIGJlIGhhbmRsZWQgdmlhIGRldmljZS10cmVlLg0KDQpJbnN0ZWFk
LCB3aGF0IGNvdWxkIGJlIGRvbmUgaW4gdGhlIGFkMnM5MF9wcm9iZSgpIGZ1bmN0aW9uLCBpcyB0
byBhZGQNCihlYXJseSBpbiB0aGUgY29kZSkgYSBjaGVjayBmb3IgdGhlIG1heCBmcmVxdWVuY3ku
DQoNClNvbWV0aGluZyBsaWtlOg0KDQojZGVmaW5lIEFEMlM5MF9NQVhfU1BJX0ZSRVFfSFogIDIw
MDAwMDANCg0KICAgICAgIGlmIChzcGktPm1heF9zcGVlZF9oeiA+IEFEMlM5MF9NQVhfU1BJX0ZS
RVFfSFopIHsNCiAgICAgICAgICAgICAgICBkZXZfZXJyKCZzcGktPmRldiwgIlNQSSBDTEssICVk
IEh6IGV4Y2VlZHMgMiBNSHpcbiIsDQogICAgICAgICAgICAgICAgICAgICAgICBzcGktPm1heF9z
cGVlZF9oeik7DQogICAgICAgICAgICAgICAgcmV0dXJuIC1FSU5WQUw7DQogICAgICAgIH0NCg0K
T3IsIHlvdSBjb3VsZCBtYWtlIEFEMlM5MF9NQVhfU1BJX0ZSRVFfSFogPSA4MzAwMDAgDQoNCk9y
LCB5b3UgY291bGQganVzdCBwcmludCBhIHdhcm5pbmcgZm9yID4gODMwMDAwLg0KU29tZXRoaW5n
IGxpa2U6DQoNCiNkZWZpbmUgQUQyUzkwX01BWF9TUElfU0FGRV9GUkVRX0haICA4MzAwMDANCg0K
ICAgICAgIGlmIChzcGktPm1heF9zcGVlZF9oeiA+IEFEMlM5MF9NQVhfU1BJX1NBRkVfRlJFUV9I
WikNCiAgICAgICAgICAgICAgICBkZXZfd2Fybigmc3BpLT5kZXYsICJTUEkgQ0xLLCAlZCBIeiA+
IDAuODMgTUh6LCBkZXZpY2UgbWF5DQpub3Qgb3BlcmF0ZSBjb3JyZWN0bHlcbiIsDQogICAgICAg
ICAgICAgICAgICAgICAgICBzcGktPm1heF9zcGVlZF9oeik7DQoNCg0KSSdtIG5vdCBzdXJlIHdo
YXQgd291bGQgYmUgcmVjb21tZW5kZWQgaGVyZS4gTWF5YmUgc29tZW9uZSBlbHNlIGhhcyBhDQpz
dHJvbmdlciBvcGluaW9uIGFib3V0IHRoaXMuDQoNClRoYW5rcw0KQWxleA0KDQo+IFRoYW5rcywN
Cj4gTWF0aGV1cy4NCj4gDQo+IFsxXSANCj4gaHR0cHM6Ly93d3cuYW5hbG9nLmNvbS9tZWRpYS9l
bi90ZWNobmljYWwtZG9jdW1lbnRhdGlvbi9kYXRhLXNoZWV0cy9BRDJTOTAucGRmDQo+IA0K

Re: Question about max frequency in ad2s90

[Matheus Tavares Bernardino]

On Tue, Nov 6, 2018 at 10:34 AM Ardelean, Alexandru
<alexandru.Ardelean@analog.com> wrote:
>
> On Mon, 2018-11-05 at 23:11 -0200, Matheus Tavares Bernardino wrote:
> > Hello everyone,
> >
>
> Hey,
>

Hi, Alex

> > While working on the ad2s90 driver, I had a question about the maximum
> > frequency set. Any feedback will be much appreciated.
> >
>
> Just a note/reminder for you: that driver looks pretty old and [since it's
> staging as well] you should also consider that it's likely that some [good]
> practices were not yet existing at the time [about how to do things].
>

Thanks for the reminder. Victor and I are trying to find what needs to
be updated/modified to move this driver out of staging. We've sent
some patches and now I'm working on the device tree support, which is
the last thing to be done in a list Jonathan helped us to enumerate.
So I take this opportunity to ask you to, please, let us know if
there's anything else you see that needs to be done before moving it
out of staging.

> > The ad2s90 driver sets its maximum speed to 830000hz (or 0.83Mhz), but
> > the datasheet [1] specifies a maximum of 2Mhz. Above the max_speed_hz
> > setup, there's a commentary saying  "need 600ns between CS and the
> > first falling edge of SCLK", which is also said in the datasheet.
> >
> > Is max_speed_hz set to 0.83Mhz, not 2Mhz, due to the required minimum
> > delay of 600ns, somehow? If not:
> >   1) is 0.83Mhz really correct? and
> >   2) is the minimum delay of 600ns being respected?
> >
>
> The 0.83 Mhz limitation looks very much like it was added to satisfy the
> 600 ns limitation. I quickly did the computation and it roughly ends up
> being 0.83 Mhz for 600 ns. (i.e. (1 / 0,000600) / 2 = half a period )
>

Thanks. Now I better understood the calculation made to use 0.83Mhz.

> That limitation (for the delay CS and the first data) is imposed by the
> datasheet ; while the chip could operate at a lower delay (and subsequently
> higher frequency up to 2 Mhz), it's safer to keep it somehow implemented.
>
> I took a look in the SPI code and it doesn't look like it implements some
> sort of delay between CS and first data being read. I may have missed stuff
> though.
>

Ok. So just to check if I got it right: The chip could work at 2Mhz
but needs a delay of 600ns between CS and the first data bit
retrieval. The driver could implement the delay and then clock at
2Mhz, but the SPI code doesn't have a way to implement this delay, so
we have to stay at a lower frequency (of maximum 0.83Mhz), to satisfy
the 600ns delay. Is that right?

So there's nothing to do about it, right?

> In any case, I do suggest removing all those 4 lines for the SPI setup
> code.
> i.e.
>
>         /* need 600ns between CS and the first falling edge of SCLK */
>         spi->max_speed_hz = 830000;
>         spi->mode = SPI_MODE_3;
>         spi_setup(spi);
>
> This should be handled via device-tree.
>

Right! Thanks for pointing it out. I'm going to address those changes
in the next patch set I'm working on, regarding device tree support.

> Instead, what could be done in the ad2s90_probe() function, is to add
> (early in the code) a check for the max frequency.
>
> Something like:
>
> #define AD2S90_MAX_SPI_FREQ_HZ  2000000
>
>        if (spi->max_speed_hz > AD2S90_MAX_SPI_FREQ_HZ) {
>                 dev_err(&spi->dev, "SPI CLK, %d Hz exceeds 2 MHz\n",
>                         spi->max_speed_hz);
>                 return -EINVAL;
>         }
>
> Or, you could make AD2S90_MAX_SPI_FREQ_HZ = 830000
>
> Or, you could just print a warning for > 830000.
> Something like:
>
> #define AD2S90_MAX_SPI_SAFE_FREQ_HZ  830000
>
>        if (spi->max_speed_hz > AD2S90_MAX_SPI_SAFE_FREQ_HZ)
>                 dev_warn(&spi->dev, "SPI CLK, %d Hz > 0.83 MHz, device may
> not operate correctly\n",
>                         spi->max_speed_hz);
>
>
> I'm not sure what would be recommended here. Maybe someone else has a
> stronger opinion about this.
>

Got it. Maybe we could join both, adding the error for freq > 2Mhz and
the warning for freq > 0.83 Mhz? Anyway, more opinions on this would
be nice, as you said.

Thanks for all the feedback.
Matheus

> Thanks
> Alex
>
> > Thanks,
> > Matheus.
> >
> > [1]
> > https://www.analog.com/media/en/technical-documentation/data-sheets/AD2S90.pdf
> >

Re: Question about max frequency in ad2s90

[Ardelean, Alexandru]

T24gVHVlLCAyMDE4LTExLTA2IGF0IDEzOjEyIC0wMjAwLCBNYXRoZXVzIFRhdmFyZXMgQmVybmFy
ZGlubyB3cm90ZToNCj4gT24gVHVlLCBOb3YgNiwgMjAxOCBhdCAxMDozNCBBTSBBcmRlbGVhbiwg
QWxleGFuZHJ1DQo+IDxhbGV4YW5kcnUuQXJkZWxlYW5AYW5hbG9nLmNvbT4gd3JvdGU6DQo+ID4g
DQo+ID4gT24gTW9uLCAyMDE4LTExLTA1IGF0IDIzOjExIC0wMjAwLCBNYXRoZXVzIFRhdmFyZXMg
QmVybmFyZGlubyB3cm90ZToNCj4gPiA+IEhlbGxvIGV2ZXJ5b25lLA0KPiA+ID4gDQo+ID4gDQo+
ID4gSGV5LA0KPiA+IA0KPiANCj4gSGksIEFsZXgNCj4gDQo+ID4gPiBXaGlsZSB3b3JraW5nIG9u
IHRoZSBhZDJzOTAgZHJpdmVyLCBJIGhhZCBhIHF1ZXN0aW9uIGFib3V0IHRoZQ0KPiA+ID4gbWF4
aW11bQ0KPiA+ID4gZnJlcXVlbmN5IHNldC4gQW55IGZlZWRiYWNrIHdpbGwgYmUgbXVjaCBhcHBy
ZWNpYXRlZC4NCj4gPiA+IA0KPiA+IA0KPiA+IEp1c3QgYSBub3RlL3JlbWluZGVyIGZvciB5b3U6
IHRoYXQgZHJpdmVyIGxvb2tzIHByZXR0eSBvbGQgYW5kIFtzaW5jZQ0KPiA+IGl0J3MNCj4gPiBz
dGFnaW5nIGFzIHdlbGxdIHlvdSBzaG91bGQgYWxzbyBjb25zaWRlciB0aGF0IGl0J3MgbGlrZWx5
IHRoYXQgc29tZQ0KPiA+IFtnb29kXQ0KPiA+IHByYWN0aWNlcyB3ZXJlIG5vdCB5ZXQgZXhpc3Rp
bmcgYXQgdGhlIHRpbWUgW2Fib3V0IGhvdyB0byBkbyB0aGluZ3NdLg0KPiA+IA0KPiANCj4gVGhh
bmtzIGZvciB0aGUgcmVtaW5kZXIuIFZpY3RvciBhbmQgSSBhcmUgdHJ5aW5nIHRvIGZpbmQgd2hh
dCBuZWVkcyB0bw0KPiBiZSB1cGRhdGVkL21vZGlmaWVkIHRvIG1vdmUgdGhpcyBkcml2ZXIgb3V0
IG9mIHN0YWdpbmcuIFdlJ3ZlIHNlbnQNCj4gc29tZSBwYXRjaGVzIGFuZCBub3cgSSdtIHdvcmtp
bmcgb24gdGhlIGRldmljZSB0cmVlIHN1cHBvcnQsIHdoaWNoIGlzDQo+IHRoZSBsYXN0IHRoaW5n
IHRvIGJlIGRvbmUgaW4gYSBsaXN0IEpvbmF0aGFuIGhlbHBlZCB1cyB0byBlbnVtZXJhdGUuDQo+
IFNvIEkgdGFrZSB0aGlzIG9wcG9ydHVuaXR5IHRvIGFzayB5b3UgdG8sIHBsZWFzZSwgbGV0IHVz
IGtub3cgaWYNCj4gdGhlcmUncyBhbnl0aGluZyBlbHNlIHlvdSBzZWUgdGhhdCBuZWVkcyB0byBi
ZSBkb25lIGJlZm9yZSBtb3ZpbmcgaXQNCj4gb3V0IG9mIHN0YWdpbmcuDQo+IA0KPiA+ID4gVGhl
IGFkMnM5MCBkcml2ZXIgc2V0cyBpdHMgbWF4aW11bSBzcGVlZCB0byA4MzAwMDBoeiAob3IgMC44
M01oeiksDQo+ID4gPiBidXQNCj4gPiA+IHRoZSBkYXRhc2hlZXQgWzFdIHNwZWNpZmllcyBhIG1h
eGltdW0gb2YgMk1oei4gQWJvdmUgdGhlIG1heF9zcGVlZF9oeg0KPiA+ID4gc2V0dXAsIHRoZXJl
J3MgYSBjb21tZW50YXJ5IHNheWluZyAgIm5lZWQgNjAwbnMgYmV0d2VlbiBDUyBhbmQgdGhlDQo+
ID4gPiBmaXJzdCBmYWxsaW5nIGVkZ2Ugb2YgU0NMSyIsIHdoaWNoIGlzIGFsc28gc2FpZCBpbiB0
aGUgZGF0YXNoZWV0Lg0KPiA+ID4gDQo+ID4gPiBJcyBtYXhfc3BlZWRfaHogc2V0IHRvIDAuODNN
aHosIG5vdCAyTWh6LCBkdWUgdG8gdGhlIHJlcXVpcmVkIG1pbmltdW0NCj4gPiA+IGRlbGF5IG9m
IDYwMG5zLCBzb21laG93PyBJZiBub3Q6DQo+ID4gPiAgIDEpIGlzIDAuODNNaHogcmVhbGx5IGNv
cnJlY3Q/IGFuZA0KPiA+ID4gICAyKSBpcyB0aGUgbWluaW11bSBkZWxheSBvZiA2MDBucyBiZWlu
ZyByZXNwZWN0ZWQ/DQo+ID4gPiANCj4gPiANCj4gPiBUaGUgMC44MyBNaHogbGltaXRhdGlvbiBs
b29rcyB2ZXJ5IG11Y2ggbGlrZSBpdCB3YXMgYWRkZWQgdG8gc2F0aXNmeQ0KPiA+IHRoZQ0KPiA+
IDYwMCBucyBsaW1pdGF0aW9uLiBJIHF1aWNrbHkgZGlkIHRoZSBjb21wdXRhdGlvbiBhbmQgaXQg
cm91Z2hseSBlbmRzIHVwDQo+ID4gYmVpbmcgMC44MyBNaHogZm9yIDYwMCBucy4gKGkuZS4gKDEg
LyAwLDAwMDYwMCkgLyAyID0gaGFsZiBhIHBlcmlvZCApDQo+ID4gDQo+IA0KPiBUaGFua3MuIE5v
dyBJIGJldHRlciB1bmRlcnN0b29kIHRoZSBjYWxjdWxhdGlvbiBtYWRlIHRvIHVzZSAwLjgzTWh6
Lg0KPiANCj4gPiBUaGF0IGxpbWl0YXRpb24gKGZvciB0aGUgZGVsYXkgQ1MgYW5kIHRoZSBmaXJz
dCBkYXRhKSBpcyBpbXBvc2VkIGJ5IHRoZQ0KPiA+IGRhdGFzaGVldCA7IHdoaWxlIHRoZSBjaGlw
IGNvdWxkIG9wZXJhdGUgYXQgYSBsb3dlciBkZWxheSAoYW5kDQo+ID4gc3Vic2VxdWVudGx5DQo+
ID4gaGlnaGVyIGZyZXF1ZW5jeSB1cCB0byAyIE1oeiksIGl0J3Mgc2FmZXIgdG8ga2VlcCBpdCBz
b21laG93DQo+ID4gaW1wbGVtZW50ZWQuDQo+ID4gDQo+ID4gSSB0b29rIGEgbG9vayBpbiB0aGUg
U1BJIGNvZGUgYW5kIGl0IGRvZXNuJ3QgbG9vayBsaWtlIGl0IGltcGxlbWVudHMNCj4gPiBzb21l
DQo+ID4gc29ydCBvZiBkZWxheSBiZXR3ZWVuIENTIGFuZCBmaXJzdCBkYXRhIGJlaW5nIHJlYWQu
IEkgbWF5IGhhdmUgbWlzc2VkDQo+ID4gc3R1ZmYNCj4gPiB0aG91Z2guDQo+ID4gDQo+IA0KPiBP
ay4gU28ganVzdCB0byBjaGVjayBpZiBJIGdvdCBpdCByaWdodDogVGhlIGNoaXAgY291bGQgd29y
ayBhdCAyTWh6DQo+IGJ1dCBuZWVkcyBhIGRlbGF5IG9mIDYwMG5zIGJldHdlZW4gQ1MgYW5kIHRo
ZSBmaXJzdCBkYXRhIGJpdA0KPiByZXRyaWV2YWwuIFRoZSBkcml2ZXIgY291bGQgaW1wbGVtZW50
IHRoZSBkZWxheSBhbmQgdGhlbiBjbG9jayBhdA0KPiAyTWh6LCBidXQgdGhlIFNQSSBjb2RlIGRv
ZXNuJ3QgaGF2ZSBhIHdheSB0byBpbXBsZW1lbnQgdGhpcyBkZWxheSwgc28NCj4gd2UgaGF2ZSB0
byBzdGF5IGF0IGEgbG93ZXIgZnJlcXVlbmN5IChvZiBtYXhpbXVtIDAuODNNaHopLCB0byBzYXRp
c2Z5DQo+IHRoZSA2MDBucyBkZWxheS4gSXMgdGhhdCByaWdodD8NCg0KWWVzLCB0aGF0J3MgY29y
cmVjdC4NCg0KPiANCj4gU28gdGhlcmUncyBub3RoaW5nIHRvIGRvIGFib3V0IGl0LCByaWdodD8N
Cj4gDQoNClllcywgdGhhdCdzIGFsc28gY29ycmVjdC4NClsgV2VsbCwgd2Ugd291bGQgbmVlZCB0
byB1cGRhdGUgdGhlIFNQSSBkcml2ZXIsIGJ1dCBJJ20gbm90IHN1cmUgaWYgaXQncw0Kd29ydGgg
dGhlIGNoYW5nZSBqdXN0IGZvciB0aGlzIGNoaXAsIHdoaWNoIGlzIHByZXR0eSBvbGQgOyBuZXdl
ciBjaGlwcw0Kc2hvdWxkbid0IGhhdmUgdGhpcyBwcm9ibGVtIF0uDQoNCj4gPiBJbiBhbnkgY2Fz
ZSwgSSBkbyBzdWdnZXN0IHJlbW92aW5nIGFsbCB0aG9zZSA0IGxpbmVzIGZvciB0aGUgU1BJIHNl
dHVwDQo+ID4gY29kZS4NCj4gPiBpLmUuDQo+ID4gDQo+ID4gICAgICAgICAvKiBuZWVkIDYwMG5z
IGJldHdlZW4gQ1MgYW5kIHRoZSBmaXJzdCBmYWxsaW5nIGVkZ2Ugb2YgU0NMSyAqLw0KPiA+ICAg
ICAgICAgc3BpLT5tYXhfc3BlZWRfaHogPSA4MzAwMDA7DQo+ID4gICAgICAgICBzcGktPm1vZGUg
PSBTUElfTU9ERV8zOw0KPiA+ICAgICAgICAgc3BpX3NldHVwKHNwaSk7DQo+ID4gDQo+ID4gVGhp
cyBzaG91bGQgYmUgaGFuZGxlZCB2aWEgZGV2aWNlLXRyZWUuDQo+ID4gDQo+IA0KPiBSaWdodCEg
VGhhbmtzIGZvciBwb2ludGluZyBpdCBvdXQuIEknbSBnb2luZyB0byBhZGRyZXNzIHRob3NlIGNo
YW5nZXMNCj4gaW4gdGhlIG5leHQgcGF0Y2ggc2V0IEknbSB3b3JraW5nIG9uLCByZWdhcmRpbmcg
ZGV2aWNlIHRyZWUgc3VwcG9ydC4NCj4gDQo+ID4gSW5zdGVhZCwgd2hhdCBjb3VsZCBiZSBkb25l
IGluIHRoZSBhZDJzOTBfcHJvYmUoKSBmdW5jdGlvbiwgaXMgdG8gYWRkDQo+ID4gKGVhcmx5IGlu
IHRoZSBjb2RlKSBhIGNoZWNrIGZvciB0aGUgbWF4IGZyZXF1ZW5jeS4NCj4gPiANCj4gPiBTb21l
dGhpbmcgbGlrZToNCj4gPiANCj4gPiAjZGVmaW5lIEFEMlM5MF9NQVhfU1BJX0ZSRVFfSFogIDIw
MDAwMDANCj4gPiANCj4gPiAgICAgICAgaWYgKHNwaS0+bWF4X3NwZWVkX2h6ID4gQUQyUzkwX01B
WF9TUElfRlJFUV9IWikgew0KPiA+ICAgICAgICAgICAgICAgICBkZXZfZXJyKCZzcGktPmRldiwg
IlNQSSBDTEssICVkIEh6IGV4Y2VlZHMgMiBNSHpcbiIsDQo+ID4gICAgICAgICAgICAgICAgICAg
ICAgICAgc3BpLT5tYXhfc3BlZWRfaHopOw0KPiA+ICAgICAgICAgICAgICAgICByZXR1cm4gLUVJ
TlZBTDsNCj4gPiAgICAgICAgIH0NCj4gPiANCj4gPiBPciwgeW91IGNvdWxkIG1ha2UgQUQyUzkw
X01BWF9TUElfRlJFUV9IWiA9IDgzMDAwMA0KPiA+IA0KPiA+IE9yLCB5b3UgY291bGQganVzdCBw
cmludCBhIHdhcm5pbmcgZm9yID4gODMwMDAwLg0KPiA+IFNvbWV0aGluZyBsaWtlOg0KPiA+IA0K
PiA+ICNkZWZpbmUgQUQyUzkwX01BWF9TUElfU0FGRV9GUkVRX0haICA4MzAwMDANCj4gPiANCj4g
PiAgICAgICAgaWYgKHNwaS0+bWF4X3NwZWVkX2h6ID4gQUQyUzkwX01BWF9TUElfU0FGRV9GUkVR
X0haKQ0KPiA+ICAgICAgICAgICAgICAgICBkZXZfd2Fybigmc3BpLT5kZXYsICJTUEkgQ0xLLCAl
ZCBIeiA+IDAuODMgTUh6LCBkZXZpY2UNCj4gPiBtYXkNCj4gPiBub3Qgb3BlcmF0ZSBjb3JyZWN0
bHlcbiIsDQo+ID4gICAgICAgICAgICAgICAgICAgICAgICAgc3BpLT5tYXhfc3BlZWRfaHopOw0K
PiA+IA0KPiA+IA0KPiA+IEknbSBub3Qgc3VyZSB3aGF0IHdvdWxkIGJlIHJlY29tbWVuZGVkIGhl
cmUuIE1heWJlIHNvbWVvbmUgZWxzZSBoYXMgYQ0KPiA+IHN0cm9uZ2VyIG9waW5pb24gYWJvdXQg
dGhpcy4NCj4gPiANCj4gDQo+IEdvdCBpdC4gTWF5YmUgd2UgY291bGQgam9pbiBib3RoLCBhZGRp
bmcgdGhlIGVycm9yIGZvciBmcmVxID4gMk1oeiBhbmQNCj4gdGhlIHdhcm5pbmcgZm9yIGZyZXEg
PiAwLjgzIE1oej8gQW55d2F5LCBtb3JlIG9waW5pb25zIG9uIHRoaXMgd291bGQNCj4gYmUgbmlj
ZSwgYXMgeW91IHNhaWQuDQo+IA0KPiBUaGFua3MgZm9yIGFsbCB0aGUgZmVlZGJhY2suDQo+IE1h
dGhldXMNCj4gDQo+ID4gVGhhbmtzDQo+ID4gQWxleA0KPiA+IA0KPiA+ID4gVGhhbmtzLA0KPiA+
ID4gTWF0aGV1cy4NCj4gPiA+IA0KPiA+ID4gWzFdDQo+ID4gPiANCmh0dHBzOi8vd3d3LmFuYWxv
Zy5jb20vbWVkaWEvZW4vdGVjaG5pY2FsLWRvY3VtZW50YXRpb24vZGF0YS1zaGVldHMvQUQyUzkw
LnBkZg0KPiA+ID4gDQo+IA0KPiANCg==

Re: Question about max frequency in ad2s90

[Matheus Tavares Bernardino]

On Tue, Nov 6, 2018 at 10:34 AM Ardelean, Alexandru
<alexandru.Ardelean@analog.com> wrote:
>
> On Mon, 2018-11-05 at 23:11 -0200, Matheus Tavares Bernardino wrote:
> > Hello everyone,
> >
>
> Hey,
>
> > While working on the ad2s90 driver, I had a question about the maximum
> > frequency set. Any feedback will be much appreciated.
> >
>
> Just a note/reminder for you: that driver looks pretty old and [since it's
> staging as well] you should also consider that it's likely that some [good]
> practices were not yet existing at the time [about how to do things].
>
> > The ad2s90 driver sets its maximum speed to 830000hz (or 0.83Mhz), but
> > the datasheet [1] specifies a maximum of 2Mhz. Above the max_speed_hz
> > setup, there's a commentary saying  "need 600ns between CS and the
> > first falling edge of SCLK", which is also said in the datasheet.
> >
> > Is max_speed_hz set to 0.83Mhz, not 2Mhz, due to the required minimum
> > delay of 600ns, somehow? If not:
> >   1) is 0.83Mhz really correct? and
> >   2) is the minimum delay of 600ns being respected?
> >
>
> The 0.83 Mhz limitation looks very much like it was added to satisfy the
> 600 ns limitation. I quickly did the computation and it roughly ends up
> being 0.83 Mhz for 600 ns. (i.e. (1 / 0,000600) / 2 = half a period )
>
> That limitation (for the delay CS and the first data) is imposed by the
> datasheet ; while the chip could operate at a lower delay (and subsequently
> higher frequency up to 2 Mhz), it's safer to keep it somehow implemented.
>
> I took a look in the SPI code and it doesn't look like it implements some
> sort of delay between CS and first data being read. I may have missed stuff
> though.
>
> In any case, I do suggest removing all those 4 lines for the SPI setup
> code.
> i.e.
>
>         /* need 600ns between CS and the first falling edge of SCLK */
>         spi->max_speed_hz = 830000;
>         spi->mode = SPI_MODE_3;
>         spi_setup(spi);
>
> This should be handled via device-tree.
>
> Instead, what could be done in the ad2s90_probe() function, is to add
> (early in the code) a check for the max frequency.
>
> Something like:
>
> #define AD2S90_MAX_SPI_FREQ_HZ  2000000
>
>        if (spi->max_speed_hz > AD2S90_MAX_SPI_FREQ_HZ) {
>                 dev_err(&spi->dev, "SPI CLK, %d Hz exceeds 2 MHz\n",
>                         spi->max_speed_hz);
>                 return -EINVAL;
>         }
>
> Or, you could make AD2S90_MAX_SPI_FREQ_HZ = 830000
>
> Or, you could just print a warning for > 830000.
> Something like:
>
> #define AD2S90_MAX_SPI_SAFE_FREQ_HZ  830000
>
>        if (spi->max_speed_hz > AD2S90_MAX_SPI_SAFE_FREQ_HZ)
>                 dev_warn(&spi->dev, "SPI CLK, %d Hz > 0.83 MHz, device may
> not operate correctly\n",
>                         spi->max_speed_hz);
>

I'm adding these suggestions in a patch set, but I don't really know
what's the right way of crediting the authorship in this case. Should
I set you as the author and add my S-o-B?

Thanks
Matheus

>
> I'm not sure what would be recommended here. Maybe someone else has a
> stronger opinion about this.
>
> Thanks
> Alex
>
> > Thanks,
> > Matheus.
> >
> > [1]
> > https://www.analog.com/media/en/technical-documentation/data-sheets/AD2S90.pdf
> >

Re: Question about max frequency in ad2s90

[Ardelean, Alexandru]

T24gV2VkLCAyMDE4LTExLTA3IGF0IDE2OjA1IC0wMjAwLCBNYXRoZXVzIFRhdmFyZXMgQmVybmFy
ZGlubyB3cm90ZToNCj4gT24gVHVlLCBOb3YgNiwgMjAxOCBhdCAxMDozNCBBTSBBcmRlbGVhbiwg
QWxleGFuZHJ1DQo+IDxhbGV4YW5kcnUuQXJkZWxlYW5AYW5hbG9nLmNvbT4gd3JvdGU6DQo+ID4g
DQo+ID4gT24gTW9uLCAyMDE4LTExLTA1IGF0IDIzOjExIC0wMjAwLCBNYXRoZXVzIFRhdmFyZXMg
QmVybmFyZGlubyB3cm90ZToNCj4gPiA+IEhlbGxvIGV2ZXJ5b25lLA0KPiA+ID4gDQo+ID4gDQo+
ID4gSGV5LA0KPiA+IA0KPiA+ID4gV2hpbGUgd29ya2luZyBvbiB0aGUgYWQyczkwIGRyaXZlciwg
SSBoYWQgYSBxdWVzdGlvbiBhYm91dCB0aGUNCj4gPiA+IG1heGltdW0NCj4gPiA+IGZyZXF1ZW5j
eSBzZXQuIEFueSBmZWVkYmFjayB3aWxsIGJlIG11Y2ggYXBwcmVjaWF0ZWQuDQo+ID4gPiANCj4g
PiANCj4gPiBKdXN0IGEgbm90ZS9yZW1pbmRlciBmb3IgeW91OiB0aGF0IGRyaXZlciBsb29rcyBw
cmV0dHkgb2xkIGFuZCBbc2luY2UNCj4gPiBpdCdzDQo+ID4gc3RhZ2luZyBhcyB3ZWxsXSB5b3Ug
c2hvdWxkIGFsc28gY29uc2lkZXIgdGhhdCBpdCdzIGxpa2VseSB0aGF0IHNvbWUNCj4gPiBbZ29v
ZF0NCj4gPiBwcmFjdGljZXMgd2VyZSBub3QgeWV0IGV4aXN0aW5nIGF0IHRoZSB0aW1lIFthYm91
dCBob3cgdG8gZG8gdGhpbmdzXS4NCj4gPiANCj4gPiA+IFRoZSBhZDJzOTAgZHJpdmVyIHNldHMg
aXRzIG1heGltdW0gc3BlZWQgdG8gODMwMDAwaHogKG9yIDAuODNNaHopLA0KPiA+ID4gYnV0DQo+
ID4gPiB0aGUgZGF0YXNoZWV0IFsxXSBzcGVjaWZpZXMgYSBtYXhpbXVtIG9mIDJNaHouIEFib3Zl
IHRoZSBtYXhfc3BlZWRfaHoNCj4gPiA+IHNldHVwLCB0aGVyZSdzIGEgY29tbWVudGFyeSBzYXlp
bmcgICJuZWVkIDYwMG5zIGJldHdlZW4gQ1MgYW5kIHRoZQ0KPiA+ID4gZmlyc3QgZmFsbGluZyBl
ZGdlIG9mIFNDTEsiLCB3aGljaCBpcyBhbHNvIHNhaWQgaW4gdGhlIGRhdGFzaGVldC4NCj4gPiA+
IA0KPiA+ID4gSXMgbWF4X3NwZWVkX2h6IHNldCB0byAwLjgzTWh6LCBub3QgMk1oeiwgZHVlIHRv
IHRoZSByZXF1aXJlZCBtaW5pbXVtDQo+ID4gPiBkZWxheSBvZiA2MDBucywgc29tZWhvdz8gSWYg
bm90Og0KPiA+ID4gICAxKSBpcyAwLjgzTWh6IHJlYWxseSBjb3JyZWN0PyBhbmQNCj4gPiA+ICAg
MikgaXMgdGhlIG1pbmltdW0gZGVsYXkgb2YgNjAwbnMgYmVpbmcgcmVzcGVjdGVkPw0KPiA+ID4g
DQo+ID4gDQo+ID4gVGhlIDAuODMgTWh6IGxpbWl0YXRpb24gbG9va3MgdmVyeSBtdWNoIGxpa2Ug
aXQgd2FzIGFkZGVkIHRvIHNhdGlzZnkNCj4gPiB0aGUNCj4gPiA2MDAgbnMgbGltaXRhdGlvbi4g
SSBxdWlja2x5IGRpZCB0aGUgY29tcHV0YXRpb24gYW5kIGl0IHJvdWdobHkgZW5kcyB1cA0KPiA+
IGJlaW5nIDAuODMgTWh6IGZvciA2MDAgbnMuIChpLmUuICgxIC8gMCwwMDA2MDApIC8gMiA9IGhh
bGYgYSBwZXJpb2QgKQ0KPiA+IA0KPiA+IFRoYXQgbGltaXRhdGlvbiAoZm9yIHRoZSBkZWxheSBD
UyBhbmQgdGhlIGZpcnN0IGRhdGEpIGlzIGltcG9zZWQgYnkgdGhlDQo+ID4gZGF0YXNoZWV0IDsg
d2hpbGUgdGhlIGNoaXAgY291bGQgb3BlcmF0ZSBhdCBhIGxvd2VyIGRlbGF5IChhbmQNCj4gPiBz
dWJzZXF1ZW50bHkNCj4gPiBoaWdoZXIgZnJlcXVlbmN5IHVwIHRvIDIgTWh6KSwgaXQncyBzYWZl
ciB0byBrZWVwIGl0IHNvbWVob3cNCj4gPiBpbXBsZW1lbnRlZC4NCj4gPiANCj4gPiBJIHRvb2sg
YSBsb29rIGluIHRoZSBTUEkgY29kZSBhbmQgaXQgZG9lc24ndCBsb29rIGxpa2UgaXQgaW1wbGVt
ZW50cw0KPiA+IHNvbWUNCj4gPiBzb3J0IG9mIGRlbGF5IGJldHdlZW4gQ1MgYW5kIGZpcnN0IGRh
dGEgYmVpbmcgcmVhZC4gSSBtYXkgaGF2ZSBtaXNzZWQNCj4gPiBzdHVmZg0KPiA+IHRob3VnaC4N
Cj4gPiANCj4gPiBJbiBhbnkgY2FzZSwgSSBkbyBzdWdnZXN0IHJlbW92aW5nIGFsbCB0aG9zZSA0
IGxpbmVzIGZvciB0aGUgU1BJIHNldHVwDQo+ID4gY29kZS4NCj4gPiBpLmUuDQo+ID4gDQo+ID4g
ICAgICAgICAvKiBuZWVkIDYwMG5zIGJldHdlZW4gQ1MgYW5kIHRoZSBmaXJzdCBmYWxsaW5nIGVk
Z2Ugb2YgU0NMSyAqLw0KPiA+ICAgICAgICAgc3BpLT5tYXhfc3BlZWRfaHogPSA4MzAwMDA7DQo+
ID4gICAgICAgICBzcGktPm1vZGUgPSBTUElfTU9ERV8zOw0KPiA+ICAgICAgICAgc3BpX3NldHVw
KHNwaSk7DQo+ID4gDQo+ID4gVGhpcyBzaG91bGQgYmUgaGFuZGxlZCB2aWEgZGV2aWNlLXRyZWUu
DQo+ID4gDQo+ID4gSW5zdGVhZCwgd2hhdCBjb3VsZCBiZSBkb25lIGluIHRoZSBhZDJzOTBfcHJv
YmUoKSBmdW5jdGlvbiwgaXMgdG8gYWRkDQo+ID4gKGVhcmx5IGluIHRoZSBjb2RlKSBhIGNoZWNr
IGZvciB0aGUgbWF4IGZyZXF1ZW5jeS4NCj4gPiANCj4gPiBTb21ldGhpbmcgbGlrZToNCj4gPiAN
Cj4gPiAjZGVmaW5lIEFEMlM5MF9NQVhfU1BJX0ZSRVFfSFogIDIwMDAwMDANCj4gPiANCj4gPiAg
ICAgICAgaWYgKHNwaS0+bWF4X3NwZWVkX2h6ID4gQUQyUzkwX01BWF9TUElfRlJFUV9IWikgew0K
PiA+ICAgICAgICAgICAgICAgICBkZXZfZXJyKCZzcGktPmRldiwgIlNQSSBDTEssICVkIEh6IGV4
Y2VlZHMgMiBNSHpcbiIsDQo+ID4gICAgICAgICAgICAgICAgICAgICAgICAgc3BpLT5tYXhfc3Bl
ZWRfaHopOw0KPiA+ICAgICAgICAgICAgICAgICByZXR1cm4gLUVJTlZBTDsNCj4gPiAgICAgICAg
IH0NCj4gPiANCj4gPiBPciwgeW91IGNvdWxkIG1ha2UgQUQyUzkwX01BWF9TUElfRlJFUV9IWiA9
IDgzMDAwMA0KPiA+IA0KPiA+IE9yLCB5b3UgY291bGQganVzdCBwcmludCBhIHdhcm5pbmcgZm9y
ID4gODMwMDAwLg0KPiA+IFNvbWV0aGluZyBsaWtlOg0KPiA+IA0KPiA+ICNkZWZpbmUgQUQyUzkw
X01BWF9TUElfU0FGRV9GUkVRX0haICA4MzAwMDANCj4gPiANCj4gPiAgICAgICAgaWYgKHNwaS0+
bWF4X3NwZWVkX2h6ID4gQUQyUzkwX01BWF9TUElfU0FGRV9GUkVRX0haKQ0KPiA+ICAgICAgICAg
ICAgICAgICBkZXZfd2Fybigmc3BpLT5kZXYsICJTUEkgQ0xLLCAlZCBIeiA+IDAuODMgTUh6LCBk
ZXZpY2UNCj4gPiBtYXkNCj4gPiBub3Qgb3BlcmF0ZSBjb3JyZWN0bHlcbiIsDQo+ID4gICAgICAg
ICAgICAgICAgICAgICAgICAgc3BpLT5tYXhfc3BlZWRfaHopOw0KPiA+IA0KPiANCj4gSSdtIGFk
ZGluZyB0aGVzZSBzdWdnZXN0aW9ucyBpbiBhIHBhdGNoIHNldCwgYnV0IEkgZG9uJ3QgcmVhbGx5
IGtub3cNCj4gd2hhdCdzIHRoZSByaWdodCB3YXkgb2YgY3JlZGl0aW5nIHRoZSBhdXRob3JzaGlw
IGluIHRoaXMgY2FzZS4gU2hvdWxkDQo+IEkgc2V0IHlvdSBhcyB0aGUgYXV0aG9yIGFuZCBhZGQg
bXkgUy1vLUI/DQoNCkZlZWwgZnJlZSB0byBub3QgaW5jbHVkZSBtZSBpbiB0aGUgcGF0Y2gtc2V0
IGluIGFueSB3YXkgOikNClRoaXMgZW1haWwgc2hvdWxkIHNlcnZlIGFzIGEgbm90ZS9tZW50aW9u
IHRoYXQgSSBhbSBmaW5lIHdpdGggdGhpcy4NCg0KV2hhdCBJIHByb3ZpZGVkIHdhcyBtb3JlIG9m
IGEgQyBwc2V1ZG8tY29kZSA7IHlvdXIgUy1vLUIgaW1wbGllcyB0aGF0IHlvdQ0KZGlkIHRoZSBm
aW5hbCB3b3JrIGFuZCB0ZXN0aW5nIG9mIHRoZSBwYXRjaC4NCg0KVGhhbmtzDQpBbGV4DQo+IA0K
PiBUaGFua3MNCj4gTWF0aGV1cw0KPiANCj4gPiANCj4gPiBJJ20gbm90IHN1cmUgd2hhdCB3b3Vs
ZCBiZSByZWNvbW1lbmRlZCBoZXJlLiBNYXliZSBzb21lb25lIGVsc2UgaGFzIGENCj4gPiBzdHJv
bmdlciBvcGluaW9uIGFib3V0IHRoaXMuDQo+ID4gDQo+ID4gVGhhbmtzDQo+ID4gQWxleA0KPiA+
IA0KPiA+ID4gVGhhbmtzLA0KPiA+ID4gTWF0aGV1cy4NCj4gPiA+IA0KPiA+ID4gWzFdDQo+ID4g
PiANCmh0dHBzOi8vd3d3LmFuYWxvZy5jb20vbWVkaWEvZW4vdGVjaG5pY2FsLWRvY3VtZW50YXRp
b24vZGF0YS1zaGVldHMvQUQyUzkwLnBkZg0KPiA+ID4gDQo+IA0KPiANCg==