linux-kernel - Re: [PATCH 01/19] ASoC: amd: ps: create platform devices based on acp config

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 11 Jan 2023 19:43:27 +0530
From:   "Mukunda,Vijendar" <vijendar.mukunda@....com>
To:     Amadeusz Sławiński 
        <amadeuszx.slawinski@...ux.intel.com>, broonie@...nel.org,
        vkoul@...nel.org, alsa-devel@...a-project.org
Cc:     Mastan.Katragadda@....com, Sunil-kumar.Dommati@....com,
        open list <linux-kernel@...r.kernel.org>,
        Basavaraj.Hiregoudar@....com, Takashi Iwai <tiwai@...e.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Nathan Chancellor <nathan@...nel.org>,
        Mario.Limonciello@....com, arungopal.kondaveeti@....com,
        Sanyog Kale <sanyog.r.kale@...el.com>,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        Syed Saba Kareem <Syed.SabaKareem@....com>
Subject: Re: [PATCH 01/19] ASoC: amd: ps: create platform devices based on acp
 config

On 11/01/23 18:57, Amadeusz Sławiński wrote:
> On 1/11/2023 10:02 AM, Vijendar Mukunda wrote:
>> Create platform devices for sdw controllers and PDM controller
>> based on ACP pin config selection and ACPI fw handle for
>> pink sardine platform.
>>
>> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@....com>
>> Signed-off-by: Mastan Katragadda <Mastan.Katragadda@....com>
>> ---
>>   include/linux/soundwire/sdw_amd.h |  18 +++
>>   sound/soc/amd/ps/acp63.h          |  24 ++-
>>   sound/soc/amd/ps/pci-ps.c         | 248 ++++++++++++++++++++++++++++--
>>   3 files changed, 277 insertions(+), 13 deletions(-)
>>   create mode 100644 include/linux/soundwire/sdw_amd.h
>>
>
> ...
>
>> diff --git a/sound/soc/amd/ps/pci-ps.c b/sound/soc/amd/ps/pci-ps.c
>> index e86f23d97584..85154cf0b2a2 100644
>> --- a/sound/soc/amd/ps/pci-ps.c
>> +++ b/sound/soc/amd/ps/pci-ps.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/interrupt.h>
>>   #include <sound/pcm_params.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/soundwire/sdw_amd.h>
>>     #include "acp63.h"
>>   @@ -134,12 +135,68 @@ static irqreturn_t acp63_irq_handler(int irq, void *dev_id)
>>       return IRQ_NONE;
>>   }
>>   -static void get_acp63_device_config(u32 config, struct pci_dev *pci,
>> -                    struct acp63_dev_data *acp_data)
>> +static int sdw_amd_scan_controller(struct device *dev)
>> +{
>> +    struct acp63_dev_data *acp_data;
>> +    struct fwnode_handle *link;
>> +    char name[32];
>> +    u8 count = 0;
>> +    u32 acp_sdw_power_mode = 0;
>> +    int index;
>> +    int ret;
>> +
>> +    acp_data = dev_get_drvdata(dev);
>> +    acp_data->acp_sdw_power_off = true;
>> +    /* Found controller, find links supported */
>> +    ret = fwnode_property_read_u8_array((acp_data->sdw_fw_node),
>> +                        "mipi-sdw-master-count", &count, 1);
>> +
>> +    if (ret) {
>> +        dev_err(dev,
>> +            "Failed to read mipi-sdw-master-count: %d\n", ret);
>> +        return -EINVAL;
>> +    }
>> +
>> +    /* Check count is within bounds */
>> +    if (count > AMD_SDW_MAX_CONTROLLERS) {
>> +        dev_err(dev, "Controller count %d exceeds max %d\n",
>> +            count, AMD_SDW_MAX_CONTROLLERS);
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (!count) {
>> +        dev_warn(dev, "No SoundWire controllers detected\n");
>> +        return -EINVAL;
>> +    }
>> +    dev_dbg(dev, "ACPI reports %d Soundwire Controller devices\n", count);
>> +    acp_data->sdw_master_count  = count;
>
> Double space before '='.
> will fix it.
>> +    for (index = 0; index < count; index++) {
>> +        snprintf(name, sizeof(name), "mipi-sdw-link-%d-subproperties", index);
>> +        link = fwnode_get_named_child_node(acp_data->sdw_fw_node, name);
>> +        if (!link) {
>> +            dev_err(dev, "Master node %s not found\n", name);
>> +            return -EIO;
>> +        }
>> +
>> +        fwnode_property_read_u32(link, "amd-sdw-power-mode",
>> +                     &acp_sdw_power_mode);
>> +        if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE)
>> +            acp_data->acp_sdw_power_off = false;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int get_acp63_device_config(u32 config, struct pci_dev *pci, struct acp63_dev_data *acp_data)
>>   {
>>       struct acpi_device *dmic_dev;
>> +    struct acpi_device *sdw_dev;
>> +    struct device *dev;
>>       const union acpi_object *obj;
>>       bool is_dmic_dev = false;
>> +    bool is_sdw_dev = false;
>> +    int ret;
>> +
>> +    dev = &pci->dev;
>>         dmic_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_DMIC_ADDR, 0);
>
> If you set dev above, you might as well use it throughout the function context? Like above in ACPI_COMPANION? 

> will use pci->dev throughtout the function context.
>>       if (dmic_dev) {
>> @@ -149,22 +206,84 @@ static void get_acp63_device_config(u32 config, struct pci_dev *pci,
>>               is_dmic_dev = true;
>>       }
>>   +    sdw_dev = acpi_find_child_device(ACPI_COMPANION(&pci->dev), ACP63_SDW_ADDR, 0);
>> +    if (sdw_dev) {
>> +        is_sdw_dev = true;
>> +        acp_data->sdw_fw_node = acpi_fwnode_handle(sdw_dev);
>> +        ret = sdw_amd_scan_controller(dev);
>
> Or just use &pci->dev here, so there is no need for separate variable?

> will remove the "dev" local variable.
>
>