On Wed, 2018-11-07 at 16:05 -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,
> >
> > > 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?
Feel free to not include me in the patch-set in any way :)
This email should serve as a note/mention that I am fine with this.
What I provided was more of a C pseudo-code ; your S-o-B implies that you
did the final work and testing of the patch.
Thanks
Alex
>
> 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
> > >
>
>
