On Tue, Nov 6, 2018 at 10:34 AM Ardelean, Alexandru
<alexandru.Ardelean@xxxxxxxxxx> 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
> >
