linux-kernel - Re: [PATCH V4 1/9] 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]

Message-ID: <cd465394-5a19-239e-7aad-caa6f19b0918@amd.com>
Date:   Tue, 13 Jun 2023 11:12:44 +0530
From:   "Mukunda,Vijendar" <vijendar.mukunda@....com>
To:     Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        broonie@...nel.org
Cc:     alsa-devel@...a-project.org, Basavaraj.Hiregoudar@....com,
        Sunil-kumar.Dommati@....com, Mastan.Katragadda@....com,
        Arungopal.kondaveeti@....com, mario.limonciello@....com,
        Liam Girdwood <lgirdwood@...il.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Syed Saba Kareem <Syed.SabaKareem@....com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V4 1/9] ASoC: amd: ps: create platform devices based on
 acp config

On 12/06/23 23:39, Pierre-Louis Bossart wrote:
> =
>> +static int sdw_amd_scan_controller(struct device *dev)
>> +{
>> +	struct acp63_dev_data *acp_data;
>> +	struct fwnode_handle *link;
>> +	char name[32];
>> +	u32 sdw_manager_bitmap;
>> +	u8 count = 0;
>> +	u32 acp_sdw_power_mode = 0;
>> +	int index;
>> +	int ret;
>> +
>> +	acp_data = dev_get_drvdata(dev);
>> +	/*
>> +	 * Current implementation is based on MIPI DisCo 2.0 spec.
>> +	 * Found controller, find links supported.
>> +	 */
>> +	ret = fwnode_property_read_u32_array((acp_data->sdw_fw_node), "mipi-sdw-manager-list",
>> +					     &sdw_manager_bitmap, 1);
>> +
>> +	if (ret) {
>> +		dev_err(dev, "Failed to read mipi-sdw-manager-list: %d\n", ret);
>> +		return -EINVAL;
>> +	}
>> +	count = hweight32(sdw_manager_bitmap);
>> +	/* Check count is within bounds */
>> +	if (count > AMD_SDW_MAX_MANAGERS) {
>> +		dev_err(dev, "Manager count %d exceeds max %d\n", count, AMD_SDW_MAX_MANAGERS);
>> +		return -EINVAL;
>> +	}
> nit-pick: the count is not enough, you should also check that only bits
> 0 and 1 are set in mipi-sdw-manager-list...
As per our design for PS platform,
we will go with two bit map values as 0x03 and 0x01.
1. As per ACP PIN CONFIG, we support Single SDW Manager instance
which refers to SW0 manager instance. For this, we need to use bitmap
value as 0x01.
2. Other bit map value - 0x03 will be used to populate two SoundWire
manager instances.
We have extra sub property "amd-sdw-enable" to invoke the init sequence
for SoundWire manager.

As we are supporting two bit map value combinations here, it's not required
to check bit set value. count value is enough to know manager instance count.
It doesn't break anything.

>> +
>> +	if (!count) {
>> +		dev_dbg(dev, "No SoundWire Managers detected\n");
>> +		return -EINVAL;
>> +	}
>> +	dev_dbg(dev, "ACPI reports %d SoundWire Manager devices\n", count);
>> +	acp_data->sdw_manager_count = count;
>> +	for (index = 0; index < count; index++) {
>> +		snprintf(name, sizeof(name), "mipi-sdw-link-%d-subproperties", index);
> ... otherwise this will be wrong.
>
>> +		link = fwnode_get_named_child_node(acp_data->sdw_fw_node, name);
>> +		if (!link) {
>> +			dev_err(dev, "Manager node %s not found\n", name);
>> +			return -EIO;
>> +		}
>> +
>> +		ret = fwnode_property_read_u32(link, "amd-sdw-power-mode", &acp_sdw_power_mode);
>> +		if (ret)
>> +			return ret;
>> +		/*
>> +		 * when SoundWire configuration is selected from acp pin config,
>> +		 * based on manager instances count, acp init/de-init sequence should be
>> +		 * executed as part of PM ops only when Bus reset is applied for the active
>> +		 * SoundWire manager instances.
>> +		 */
>> +		if (acp_sdw_power_mode != AMD_SDW_POWER_OFF_MODE) {
>> +			acp_data->acp_reset = false;
>> +			return 0;
>> +		}
>> +	}
>> +	return 0;
>> +}