Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <59da8a18-752a-bc8c-c8c5-19d75d8c87fb@linux.intel.com>
Date:   Thu, 15 Jun 2023 18:23:46 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.11.0
Subject: Re: [PATCH V4 1/9] ASoC: amd: ps: create platform devices based on
 acp config
Content-Language: en-US
To:     "Mukunda,Vijendar" <vijendar.mukunda@amd.com>, broonie@kernel.org
Cc:     alsa-devel@alsa-project.org, Basavaraj.Hiregoudar@amd.com,
        Sunil-kumar.Dommati@amd.com, Mastan.Katragadda@amd.com,
        Arungopal.kondaveeti@amd.com, mario.limonciello@amd.com,
        Liam Girdwood <lgirdwood@gmail.com>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Syed Saba Kareem <Syed.SabaKareem@amd.com>,
        open list <linux-kernel@vger.kernel.org>
References: <20230612095903.2113464-1-Vijendar.Mukunda@amd.com>
 <20230612095903.2113464-2-Vijendar.Mukunda@amd.com>
 <5899c2b8-e984-fda9-5e12-190b0c9fd3b2@linux.intel.com>
 <cd465394-5a19-239e-7aad-caa6f19b0918@amd.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <cd465394-5a19-239e-7aad-caa6f19b0918@amd.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 6/13/23 07:42, Mukunda,Vijendar wrote:
> 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.

Not a blocker but you underestimate the creativity of UEFI BIOS writers...