-
Notifications
You must be signed in to change notification settings - Fork 876
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
Conversation
2e07d57
to
fea0a01
Compare
This brings me one question... Before we had on 70ns offset and now we have 3ns. Isn't that an issue? |
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). |
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 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 |
fea0a01
to
65eaec9
Compare
True, I fixed the condition to be clk_gate_wf.duty_offset_ns > clk_gate_wf.period_length_ns which was actually what was described in the commit. |
drivers/iio/adc/ltc2387.c
Outdated
@@ -204,6 +204,9 @@ static int ltc2387_set_sampling_freq(struct ltc2387_dev *ltc, int freq) | |||
clk_gate_wf.duty_length_ns = ref_clk_period_ns * clk_en_time; | |||
clk_gate_wf.duty_offset_ns = LTC2387_T_FIRSTCLK_NS; | |||
|
|||
if (clk_gate_wf.duty_offset_ns > clk_gate_wf.period_length_ns) | |||
div64_u64_rem(clk_gate_wf.duty_offset_ns, clk_gate_wf.period_length_ns, &clk_gate_wf.duty_offset_ns); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line looks bigger than the 100col limit...
I guess I finally understand what you're doing here. I'm still not 100% convinced about this and it feels it is only solving this particular case but anyways, let's merge this. Please fix first the long line.
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]>
65eaec9
to
5ec5d54
Compare
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
PR Checklist