On Tue, 2018-11-06 at 13:12 -0200, Matheus Tavares Bernardino wrote:
> 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?
Yes, that's correct.
>
> So there's nothing to do about it, right?
>
Yes, that's also correct.
[ Well, we would need to update the SPI driver, but I'm not sure if it's
worth the change just for this chip, which is pretty old ; newer chips
shouldn't have this problem ].
> > 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
> > >
>
>
