Conversation
|
Can drivers/iio/adc/adrv904x/initdata.c and drivers/iio/adc/adrv904x/initdata.h are API files right? the rules are at https://github.com/analogdevicesinc/linux/blob/ci/ci/build.sh#L35 The license.pdf is a no no, please remove All files should have unix endings, even api ones, csv, etc, use I will now take a look on the |
|
The api is generating calls to thanks! |
|
0001-API-dirty-adrv904x-use-math64-and-gcd.patch |
3c1012d to
a37e455
Compare
|
@gastmaier thank you for your review. See the changelog below:
|
5fb6eed to
fa6d679
Compare
|
Hi, is rule |
|
Adding new comment to fixup my last comment, you did fix the line endings at but checkpatch checks per commit What you can do, if you don't want to force push, is to create a fixup commit with Those commits are intentionally auto squashed during the ci |
fa6d679 to
1405cc2
Compare
Yes, adrv904x/platforms contains the platform abstraction layer (adi_library_types.h, adi_library.c). Thanks for adding the board rule! |
|
@gastmaier Thank you again for the review! Could you take another look and let me know if there are any remaining topics? |
|
Friendly reminder: please don't forget to review this PR. |
There was a problem hiding this comment.
Do we have documentation on the dt bindings .yaml? How will you document how to use?
Please resolve the clang/gcc analyzer caught issues, they are not static analyzer and are mostly always correct, either directly correct, or surface deeper structure issues.
The issues are, for convenience, shown as annotations, in the changed files; you can also now filter the commits shows in the gui, so de-select the api and fw commits, and focus on your files.
Thanks!
| break; | ||
| } | ||
|
|
||
| ret = adrv904x_gainindex_to_gain(phy, chan->channel, |
There was a problem hiding this comment.
this ret value is never read.
should you error check? or discard explicitly
Same in all locations
| adrv904x_add_debugfs_entry(phy, "bist_tone", DBGFS_BIST_TONE); | ||
|
|
||
| for (i = 0; i < phy->adrv904x_debugfs_entry_index; i++) | ||
| d = debugfs_create_file(phy->debugfs_entry[i].propname, 0644, |
| case OBS_SAMPL_CLK: | ||
| phy->orx_iqRate_kHz = phy->kororDevice->initExtract.jesdSetting.framerSetting[1].iqRate_kHz; | ||
| init.ops = &bb_clk_ops; | ||
| clk_priv->rate = phy->orx_iqRate_kHz;; |
| adrv904x_TxLinkSamplingRateFind(phy->kororDevice, | ||
| ADI_ADRV904X_DEFRAMER_0, | ||
| &rate); | ||
| init.ops = &bb_clk_ops; | ||
| clk_priv->rate = rate; |
There was a problem hiding this comment.
Error checking before storing value? Does rate really get a value on all branches?
If deframerSel selects more than one, it never writes iqRate_kHz.
But in general just always error check.
do you have a wrapper to convert recoveryAction like device->common.errPtr->errDebugInfo.highestPriorityAction, in a more palatable format?
1405cc2 to
db3c42e
Compare
|
changelog v3: Fixed unused/overwritten values:
Fixed unused variables:
Fixed syntax issues:
Added dt-bindings documentation:
|
ef097a1 to
3e64a1d
Compare
2e6cda5 to
8f0c2f0
Compare
705a5eb to
cd6524b
Compare
|
changelog v5:
|
7059b80 to
1e34bc2
Compare
|
chagelog v6:
|
1e34bc2 to
9e138a1
Compare
nunojsa
left a comment
There was a problem hiding this comment.
Here it goes my 2 cents
| CONFIG_DEVTMPFS=y | ||
| CONFIG_DEVTMPFS_MOUNT=y | ||
| CONFIG_EXTRA_FIRMWARE="ad9467_intbypass_ad9517.stp ad9517.stp ad9144_fmc_ebz_ad9516.stp Mykonos_M3.bin TaliseStream.bin TaliseTDDArmFirmware.bin TaliseTxArmFirmware.bin TaliseRxArmFirmware.bin adau1761.bin Navassa_EvaluationFw.bin RxGainTable.csv RxGainTable_GainCompensated.csv ORxGainTable.csv TxAttenTable.csv Navassa_Stream.bin Navassa_CMOS_profile.json Navassa_LVDS_profile.json Navassa_CMOS_profile_adrv9003.json Navassa_LVDS_profile_adrv9003.json Navassa_LVDS_init_cals.bin Navassa_CMOS_init_cals.bin Navassa_CMOS_init_cals_adrv9003.bin Navassa_LVDS_init_cals_adrv9003.bin Navassa_CMOS_profile_adrv9004.json Navassa_LVDS_profile_adrv9004.json Navassa_CMOS_profile_adrv9005.json Navassa_LVDS_profile_adrv9005.json Navassa_CMOS_profile_adrv9006.json Navassa_LVDS_profile_adrv9006.json ADRV9025_DPDCORE_FW.bin ADRV9025_FW.bin ADRV9025_RxGainTable.csv ADRV9025_TxAttenTable.csv stream_image_6E3E00EFB74FE7D465FA88A171B81B8F.bin ActiveUseCase.profile ActiveUtilInit.profile ActiveUseCase_NLS.profile ActiveUseCase_204C.profile ADRV9030_FW.bin ADRV9030_DeviceProfileTest_M4.bin ADRV9030_stream_image.bin ADRV9030_RxGainTable.csv ADRV9030_RxGainTable_GainCompensated.csv ADRV9030_RxGainTable_HB.csv" | ||
| CONFIG_EXTRA_FIRMWARE="ad9467_intbypass_ad9517.stp ad9517.stp ad9144_fmc_ebz_ad9516.stp Mykonos_M3.bin TaliseStream.bin TaliseTDDArmFirmware.bin TaliseTxArmFirmware.bin TaliseRxArmFirmware.bin adau1761.bin Navassa_EvaluationFw.bin RxGainTable.csv RxGainTable_GainCompensated.csv ORxGainTable.csv TxAttenTable.csv Navassa_Stream.bin Navassa_CMOS_profile.json Navassa_LVDS_profile.json Navassa_CMOS_profile_adrv9003.json Navassa_LVDS_profile_adrv9003.json Navassa_LVDS_init_cals.bin Navassa_CMOS_init_cals.bin Navassa_CMOS_init_cals_adrv9003.bin Navassa_LVDS_init_cals_adrv9003.bin Navassa_CMOS_profile_adrv9004.json Navassa_LVDS_profile_adrv9004.json Navassa_CMOS_profile_adrv9005.json Navassa_LVDS_profile_adrv9005.json Navassa_CMOS_profile_adrv9006.json Navassa_LVDS_profile_adrv9006.json ADRV9025_DPDCORE_FW.bin ADRV9025_FW.bin ADRV9025_RxGainTable.csv ADRV9025_TxAttenTable.csv stream_image_6E3E00EFB74FE7D465FA88A171B81B8F.bin ActiveUseCase.profile ActiveUtilInit.profile ActiveUseCase_NLS.profile ActiveUseCase_204C.profile ADRV9030_FW.bin ADRV9030_DeviceProfileTest_M4.bin ADRV9030_stream_image.bin ADRV9030_RxGainTable.csv ADRV9030_RxGainTable_GainCompensated.csv ADRV9030_RxGainTable_HB.csv ADRV9040_DFE_CALS_FW.bin DeviceProfileTest.bin DeviceProfileTest_NLS.bin stream_image.bin ADRV9040_FW.bin ADRV9040_RxGainTable.csv" |
There was a problem hiding this comment.
Could you please clarify what you mean? The ADRV9040 firmware entries are already in a dedicated commit. Did you mean splitting the defconfig change from the firmware file additions?
|
|
||
| typedef struct { | ||
| void *value; | ||
| } thread_to_value_t; |
There was a problem hiding this comment.
I would say this platform file is code "owned" by us/linux to some extent. So I would use kernel standards as much as possible ine here
| size_t total_size; | ||
|
|
||
| total_size = size * nmemb; | ||
| return kzalloc(total_size, GFP_KERNEL); |
| { | ||
| int ret; | ||
|
|
||
| FILE *stream = devm_kzalloc(&hal->spi->dev, sizeof(*stream), GFP_KERNEL); |
There was a problem hiding this comment.
No new line and this devm_kzalloc() is very questionable. I would just do the typical pair "kzalloc() + kfree()"
|
|
||
| ADI_API adi_hal_Err_e linux_adrv904x_MutexUnlock(adi_hal_mutex_t *mutex) | ||
| { | ||
| (void)mutex; |
| struct adrv904x_rf_phy *phy = priv->phy; | ||
| adi_adrv904x_TxAtten_t txAttenuation[1]; | ||
| const u32 ALL_CHANNELS_MASK = 0xFFU; | ||
| const u32 NUM_TRACKING_CALS = 7U; |
There was a problem hiding this comment.
I think plain GENMASK() in the code would be more readable
|
|
||
| if (dev_clk > 0 && ((dev_clk / 1000) == | ||
| deviceClockScaled_kHz)) { | ||
| clk_set_rate(phy->dev_clk, (unsigned long)dev_clk); |
There was a problem hiding this comment.
Should check for error codes
| _parent_name[0] = | ||
| adrv904x_clk_set_dev_name(phy, p_name[0], parent_name); | ||
| _parent_name[1] = | ||
| adrv904x_clk_set_dev_name(phy, p_name[1], parent_name2); |
There was a problem hiding this comment.
I would not do the line break
| else | ||
| orxchan_en &= ~(ADI_ADRV904X_RX0 << chan->channel); | ||
| } | ||
| } |
There was a problem hiding this comment.
In the above seems that there's room to properly use linux bit handling! I would use it
| * in Hz. Using scale is a bit ugly. | ||
| */ | ||
| _ADRV904X_EXT_LO_INFO("frequency", LOEXT_FREQ), | ||
| {}, |
There was a problem hiding this comment.
I would drop the comma! We have the NULL terminator entry
Add Koror API 2.15.0.5. Signed-off-by: George Mois <george.mois@analog.com>
Add binaries to be used with the ADRV904X driver. New adrv904x firmware to reflect the two available use cases, without ORX (default profile) and with ORX (NLS profile) Signed-off-by: George Mois <george.mois@analog.com> Signed-off-by: Andrei Dragomir <andrei.dragomir@analog.com> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Add the platform adaptation layer for the ADRV904X driver, including Linux-specific HAL implementation and platform abstraction functions. Signed-off-by: George Mois <george.mois@analog.com> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Add driver files for ADRV904X and add option to makefiles. Signed-off-by: George Mois <george.mois@analog.com> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Add adrv904x_1_00_a_info core info and the "adi,axi-adrv904x-tx-1.0" compatible string to support the ADRV904X DAC. Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Add info table for observation receiver for the ADRV904X family. Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Make sure that the ADRV904X driver is built. Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Add dts file for adrv904x (Koror). Signed-off-by: George Mois <george.mois@analog.com>
New adrv904x dts to implement the use case with ORX in NLS mode Signed-off-by: AndrDragomir <andrei.dragomir@analog.com>
Direct 64-bit division on ARM32 causes undefined references to __aeabi_uldivmod and __aeabi_ldivmod. Add platform abstraction wrappers in adi_library_types.h for division operations and use them throughout the driver. Use GCD reduction before squaring in scale_with_squared_ratio() to prevent overflow in power threshold calculations. Co-authored-by: Jorge Marques <jorge.marques@analog.com> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Add device tree bindings documentation for the Analog Devices ADRV904X RF transceiver family. The ADRV904X is a highly integrated RF transceiver supporting 8 transmitter and 8 receiver channels with observation receivers, designed for cellular infrastructure applications. Signed-off-by: Stefan Popa <stefan.popa@analog.com>
…e_dpd The prototype for adi_adrv904x_WBBufSegConfigGet() in adi_adrv904x_dfe_dpd.h is conditionally compiled under in adi_adrv904x_dfe_dpd.c lacks the same guard. When ADI_LIBRARY_RM_FLOATS is defined the compiler sees no prior declaration for the function and emits -Wmissing-prototypes, which is promoted to an error by -Werror, breaking the build. Wrap the function definition with the same guard as its prototype. Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Fix variable-length array (VLA) build errors in vendor API code. Replace const local variables used as array dimensions with #define macros so sizes are true compile-time constants. Affected files: adi_adrv904x_utilities.c, adi_adrv904x_cpu.c, adrv904x_carrier_reconfigure.c Signed-off-by: Stefan Popa <stefan.popa@analog.com>
adrv904x_dfe_vswr.c defines adrv904x_VswrPlaybackDatNumOfSamplesInit but does not include its own header adrv904x_dfe_vswr.h, where the function prototype is declared. This causes a -Werror=missing-prototypes build failure since the compiler sees the definition before any declaration. Add the missing self-include to fix the build error. Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Rename all symbols in the adrv904x vendor common library and platform files that clash with the identical copies in adrv903x when both drivers are built-in (=y). This follows the same approach used for adrv9104 in commit 6cf76da. Renamed symbols use the adrv904x_ prefix. Compatibility #defines are added in the headers so that vendor API code calling the old names continues to compile unchanged. Also rename the initdata.c globals (utilityInit, deviceInitStruct, etc.) to adrv904x_-prefixed names and update references in adrv904x.c. Signed-off-by: Stefan Popa <stefan.popa@analog.com>
9e138a1 to
a16087c
Compare
|
@nunojsa thank you for the review! Here's chagelog v7:
|
PR Description
This PR adds initial Linux kernel driver support for the Analog Devices ADRV904X (Koror) family of wideband transceivers.
PR Type
PR Checklist