Hi Hans,
On 12/4/2023 9:00 PM, Hans de Goede wrote:
> Hi,
>
> On 11/29/23 10:13, Ma Jun wrote:
>> Due to electrical and mechanical constraints in certain platform designs
>> there may be likely interference of relatively high-powered harmonics of
>> the (G-)DDR memory clocks with local radio module frequency bands used
>> by Wifi 6/6e/7.
>>
>> To mitigate this, AMD has introduced a mechanism that devices can use to
>> notify active use of particular frequencies so that other devices can make
>> relative internal adjustments as necessary to avoid this resonance.
>>
>> Co-developed-by: Evan Quan <quanliangl@xxxxxxxxxxx>
>> Signed-off-by: Evan Quan <quanliangl@xxxxxxxxxxx>
>> Signed-off-by: Ma Jun <Jun.Ma2@xxxxxxx>
>>
>> --
>> v11:
>> - fix typo(Simon)
>> v12:
>> - Fix the code logic (Rafael)
>> - Move amd_wbrf.c to drivers/platform/x86/amd/wbrf.c
>> - Updated Evan's email because he's no longer at AMD.Thanks
>> for his work in earlier versions.
>> v13:
>> - Fix the format issue (IIpo Jarvinen)
>> - Add comment for some functions
>> v14:
>> - Use the apci_check_dsm and acpi_evaluate_dsm (Hans de Goede)
>
> Thank you this is much better.
>
> I notice that the #define ACPI_AMD_WBRF_METHOD "\\WBRF"
> still exists though and that this is still used in
> static bool acpi_amd_wbrf_supported_system(void).
>
> I think it might be better to just remove
> these 2 all together.
>
> Checking if a DSM with the expected GUID is present
> and if that has the correct bits set in its supported
> mask should be enough.
>
> And on future systems the implementer may decide to
> not have a WBRF helper function at all and instead
> handle everything in the _DSM method.
>
> So the "\\WBRF" check seems to be checking for
> what really is an implementation detail.
>
Yes,you are right. I will fix these issues.
Thanks for your review and suggestion.
Regards,
Ma Jun
> 2 other very small remark
>
>> +/**
>> + * acpi_amd_wbrf_supported_producer - determine if the WBRF can be enabled
>> + * for the device as a producer
>> + *
>> + * @dev: device pointer
>> + *
>> + * Check if the platform equipped with necessary implementations to
>> + * support WBRF for the device as a producer.
>> + *
>> + * Return:
>> + * true if WBRF is supported, otherwise returns false
>> + */
>> +bool acpi_amd_wbrf_supported_producer(struct device *dev)
>> +{
>> + struct acpi_device *adev;
>> +
>> + adev = ACPI_COMPANION(dev);
>> + if (!adev)
>> + return false;
>> +
>> + if (!acpi_amd_wbrf_supported_system())
>> + return false;
>> +
>> +
>> + return acpi_check_dsm(adev->handle, &wifi_acpi_dsm_guid,
>> + WBRF_REVISION, BIT(WBRF_RECORD));
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_amd_wbrf_supported_producer);
>
> Please don't use double empty lines, one empty line to separate things
> is enough.
>
>> +
>> +/**
>> + * acpi_amd_wbrf_supported_consumer - determine if the WBRF can be enabled
>> + * for the device as a consumer
>> + *
>> + * @dev: device pointer
>> + *
>> + * Determine if the platform equipped with necessary implementations to
>> + * support WBRF for the device as a consumer.
>> + *
>> + * Return:
>> + * true if WBRF is supported, otherwise returns false.
>> + */
>> +bool acpi_amd_wbrf_supported_consumer(struct device *dev)
>> +{
>> + struct acpi_device *adev;
>> +
>> + adev = ACPI_COMPANION(dev);
>> + if (!adev)
>> + return false;
>> +
>> + if (!acpi_amd_wbrf_supported_system())
>> + return false;
>> +
>> + return acpi_check_dsm(adev->handle, &wifi_acpi_dsm_guid,
>> + WBRF_REVISION, BIT(WBRF_RETRIEVE));
>> +}
>> +EXPORT_SYMBOL_GPL(acpi_amd_wbrf_supported_consumer);
>> +
>> +/**
>> + * amd_wbrf_retrieve_freq_band - retrieve current active frequency
>> + * bands
>
> You may go a bit over the 80 chars limit, please just make this
> a single line:
>
> * amd_wbrf_retrieve_freq_band - retrieve current active frequency bands
>
>> + *
>> + * @dev: device pointer
>> + * @out: output structure containing all the active frequency bands
>> + *
>> + * Retrieve the current active frequency bands which were broadcasted
>> + * by other producers. The consumer who calls this API should take
>> + * proper actions if any of the frequency band may cause RFI with its
>> + * own frequency band used.
>> + *
>> + * Return:
>> + * 0 for getting wifi freq band successfully.
>> + * Returns a negative error code for failure.
>> + */
>
> Regards,
>
> Hans
>
