Skip to content

iio: adc: ltc2387: Update calculation of duty_offset_ns value #2728

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

IuliaCMoldovan
Copy link
Contributor

@IuliaCMoldovan IuliaCMoldovan commented Mar 3, 2025

PR Description

For the situation when the maximum sampling is used, 15MHz.

Before: duty_offset_ns = 70, period_length_ns = 67 (rounded from 66.666)

That caused "ltc2387 setup failed", because the PWM framework has a condition
if wf->duty_offset_ns >= wf->period_length_ns, which is true => fail.

In the case of LTC2387 with max sample rate,
tFIRSTCLK >= 65ns and tCONV = 63ns (data sheet, page 5).
This doesn't go by the PWM framework condition, so the offset needs to be updated, affecting the timing of the cnv and clk_gate signals only at the max sample rate. Just the first time the conversion is affected, being delayed.

Now: duty_offset_ns = 3, period_length_ns = 67.
The first sample is junk anyway.

This doesn't affect the sampling, because it is started by the PWM core before the DMA buffer is enabled.

I tested this on a CN0577/Zed setup and it works (I can provide screenshots if necessary).

PR Type

  • Bug fix (a change that fixes an issue)
  • New feature (a change that adds new functionality)
  • Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

  • I have conducted a self-review of my own code changes
  • I have tested the changes on the relevant hardware
  • I have updated the documentation outside this repo accordingly (if there is the case)

For the situation when the maximum sampling is used, 15MHz.

Before: duty_offset_ns = 70, period_length_ns = 67 (rounded from 66.666)

That caused "ltc2387 setup failed", because the PWM framework has a
condition:
if wf->duty_offset_ns >= wf->period_length_ns, which is true => fail.

In the case of LTC2387 with max sample rate,
tFIRSTCLK >= 65ns and tCONV (offset) = 63ns (LTC2387-18 data sheet, p.5).
This doesn't go by the PWM framework condition, so the offset needs to be
updated, affecting the timing of the cnv and clk_gate signals only at
the max sample rate. Just the first time the conversion is affected,
being delayed.

Now: duty_offset_ns = 3, period_length_ns = 67.
The first sample is junk anyway.

This doesn't affect the sampling, because it is started by the PWM core
before the DMA buffer is enabled.

Signed-off-by: Iulia Moldovan <[email protected]>
@nunojsa
Copy link
Collaborator

nunojsa commented Mar 3, 2025

Now: duty_offset_ns = 3, period_length_ns = 67.
The first sample is junk anyway.

This brings me one question... Before we had on 70ns offset and now we have 3ns. Isn't that an issue?

@IuliaCMoldovan
Copy link
Contributor Author

Before, we had a period of 67ns and a 70ns delay so the start of the clk_gate_wf was aligned with the second clock of cnv_wf (and 3ns delay).
Now, it is aligned with the first clock of cnv_wf (and 3ns delay).
Although this leads to an unknown behavior for the first acquired sample, it doesn't affect us since it is discarded anyway, as the setup() function makes it to have permanent transactions. By the time the buffer is enabled, that first sample is already gone.

@nunojsa
Copy link
Collaborator

nunojsa commented Mar 5, 2025

Although this leads to an unknown behavior for the first acquired sample, it doesn't affect us since it is discarded anyway, as the setup() function makes it to have permanent transactions. By the time the buffer is enabled, that first sample is already gone.

The only thing that I do not like about that statement is that it looks like this is a change that works for the setup we have right now but a question mark for other possible setups. I also do not think that assuming that the first sample is going to be discarded anyway to be a very good solution... Also brings me the question of why using the duty_offset then? Would this work?

if (wf->duty_offset_ns >= wf->period_length_ns)
    wf->duty_offset_ns = wf->period_length_ns - 1;

If there's a real HW requirement for wf->duty_offset_ns to be equal to wf->period_length_ns, then we should propose that change to the PWM subsystem and maybe turn wf->duty_offset_ns >= wf->period_length_ns into wf->duty_offset_ns > wf->period_length_ns

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants