-
Notifications
You must be signed in to change notification settings - Fork 914
Dev ad9740 wip #3035
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
base: main
Are you sure you want to change the base?
Dev ad9740 wip #3035
Conversation
DDS is generating an apparently correct sine wave; in ramp mode a periodic signal is generated more similar to an asymetric PWM than to a ramp
machschmitt
left a comment
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.
Hi @sarpadi ,
Having a first look at this mess.
I'm not really getting what this wants to do with the iio-backend. I guess, it might make more sense to Nuno.
| /* Data format: true = 2's complement, false = offset binary */ | ||
| bool twos_complement; |
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.
comment doesn't make much sense and/or is not meaningful. What is "offset binary"?
| static int ad9740_read_raw(struct iio_dev *indio_dev, | ||
| struct iio_chan_spec const *chan, | ||
| int *val, int *val2, long mask) | ||
| { | ||
| return -EINVAL; | ||
| } |
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.
IIRC, this is not needed due to nothing can be read from the device
| struct iio_chan_spec const *chan, | ||
| int val, int val2, long mask) | ||
| { | ||
| return -EINVAL; |
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 should probably have a proper implementation
| dev_info(st->dev, "Buffer enable requested - starting DMA streaming\n"); | ||
| dev_info(st->dev, "========================================\n"); | ||
|
|
||
| mutex_lock(&st->lock); |
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.
can use guard() instead
| dev_info(st->dev, "Buffer disable requested - stopping DMA streaming\n"); | ||
| dev_info(st->dev, "========================================\n"); | ||
|
|
||
| mutex_lock(&st->lock); |
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.
guard
|
|
||
| dev_info(&pdev->dev, "Configuring IIO device structure\n"); | ||
| indio_dev->name = st->chip_info->name; | ||
| indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_HARDWARE; |
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.
INDIO_BUFFER_HARDWARE should be added within the proper buffer_setup() call
| /* Make a modifiable copy of the channel spec for backend extension */ | ||
| struct iio_chan_spec *channels; | ||
| const struct iio_chan_spec_ext_info *backend_ext_info; | ||
| struct iio_chan_spec_ext_info *merged_ext_info; | ||
| int num_our_ext_info, num_backend_ext_info, i, j; | ||
|
|
||
| dev_info(&pdev->dev, "Creating modifiable channel spec\n"); | ||
| channels = devm_kmemdup(&pdev->dev, st->chip_info->channels, | ||
| sizeof(struct iio_chan_spec) * st->chip_info->num_channels, | ||
| GFP_KERNEL); | ||
| if (!channels) { | ||
| dev_err(&pdev->dev, "Failed to allocate channel spec\n"); | ||
| return -ENOMEM; | ||
| } | ||
|
|
||
| /* Extend channel with DDS controls from backend */ | ||
| dev_info(&pdev->dev, "Extending channel spec with backend DDS controls\n"); | ||
| ret = iio_backend_extend_chan_spec(st->back, &channels[0]); | ||
| if (ret) { | ||
| dev_err(&pdev->dev, "Failed to extend channel spec: %d\n", ret); | ||
| return ret; | ||
| } | ||
|
|
||
| /* Merge our ext_info (data_source) with backend's ext_info (DDS) */ | ||
| backend_ext_info = channels[0].ext_info; | ||
|
|
||
| /* Count entries in both arrays */ | ||
| num_our_ext_info = ARRAY_SIZE(ad9740_ext_info) - 1; /* exclude terminator */ | ||
| num_backend_ext_info = 0; | ||
| if (backend_ext_info) { | ||
| while (backend_ext_info[num_backend_ext_info].name) | ||
| num_backend_ext_info++; | ||
| } | ||
|
|
||
| dev_info(&pdev->dev, "Merging ext_info: %d our attrs + %d backend attrs\n", | ||
| num_our_ext_info, num_backend_ext_info); | ||
|
|
||
| /* Allocate merged array: our entries + backend entries + terminator */ | ||
| merged_ext_info = devm_kcalloc(&pdev->dev, | ||
| num_our_ext_info + num_backend_ext_info + 1, | ||
| sizeof(*merged_ext_info), GFP_KERNEL); | ||
| if (!merged_ext_info) { | ||
| dev_err(&pdev->dev, "Failed to allocate merged ext_info\n"); | ||
| return -ENOMEM; | ||
| } | ||
|
|
||
| /* Copy our ext_info first */ | ||
| for (i = 0; i < num_our_ext_info; i++) | ||
| merged_ext_info[i] = ad9740_ext_info[i]; | ||
|
|
||
| /* Then append backend ext_info */ | ||
| for (j = 0; j < num_backend_ext_info; j++) | ||
| merged_ext_info[i + j] = backend_ext_info[j]; | ||
|
|
||
| /* Terminator is already zero from kcalloc */ | ||
| channels[0].ext_info = merged_ext_info; | ||
| dev_info(&pdev->dev, "Extended attributes merged successfully\n"); |
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.
i'm not sure about this channel juggling. Nuno might understand it better (or not?).
| /* Setup DAC and backend */ | ||
| ret = ad9740_setup(st); | ||
| if (ret) { | ||
| dev_err(&pdev->dev, "AD9740 setup failed: %d\n", ret); |
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.
dev_err_probe
|
|
||
| /* Register IIO device */ | ||
| dev_info(&pdev->dev, "Registering IIO device\n"); | ||
| ret = devm_iio_device_register(&pdev->dev, indio_dev); |
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.
i see there's a lot of debugging, but should probably just return devm_iio_... here when it gets well tested
| static const struct of_device_id axi_dac_of_match[] = { | ||
| { .compatible = "adi,axi-dac-9.1.b", .data = &dac_generic }, | ||
| { .compatible = "adi,axi-ad3552r", .data = &dac_ad3552r }, | ||
| { .compatible = "adi,axi-ad9740", .data = &dac_ad9740 }, |
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.
i haven't looked into the dac data sheet nor the HDL in details but, why can't this use the generic dac backend?
PR Description
necessary to understand them. List any dependencies required for this change.
any space), or simply check them after publishing the PR.
description and try to push all related PRs simultaneously.
PR Type
PR Checklist