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,
>
> > 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
> >
