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...
