Re: [alsa-devel] [PATCH 08/14] soundwire: Add Slave status handling helpers

[Vinod Koul <vinod.koul@intel.com>]

On Thu, Oct 19, 2017 at 03:44:12PM +0200, Takashi Iwai wrote:
> On Thu, 19 Oct 2017 05:03:24 +0200,
> Vinod Koul wrote:

Sorry looks like I missed replying on this one, my apologies

> > +static struct sdw_slave *sdw_get_slave(struct sdw_bus *bus, int i)
> > +{
> > +	struct sdw_slave *slave;
> > +
> > +	list_for_each_entry(slave, &bus->slaves, node) {
> > +		if (slave->dev_num == i)
> > +			return slave;
> > +	}
> > +
> > +	return NULL;
> 
> Is this performed always in bus_lock, right?
> Better to document it.

Thanks we need to have lock here, fixed

> > +static int sdw_compare_devid(struct sdw_slave *slave, struct sdw_slave_id id)
> > +{
> > +
> > +	if ((slave->id.unique_id != id.unique_id) ||
> > +			(slave->id.mfg_id != id.mfg_id) ||
> > +			(slave->id.part_id != id.part_id) ||
> > +			(slave->id.class_id != id.class_id))
> 
> Align indentations.

sure

> > +static int sdw_get_device_num(struct sdw_slave *slave)
> > +{
> > +	bool assigned = false;
> > +	int i;
> > +
> > +	mutex_lock(&slave->bus->bus_lock);
> > +	for (i = 1; i <= SDW_MAX_DEVICES; i++) {
> > +		if (slave->bus->assigned[i] == true)
> > +			continue;
> > +
> > +		slave->bus->assigned[i] = true;
> > +		assigned = true;
> > +
> > +		/*
> > +		 * Do not update dev_num in Slave data structure here,
> > +		 * Update once program dev_num is successful
> > +		 */
> > +		break;
> 
> With the bitmap, it's easier, you can use find_next_zero_bit() :)

yes already done

> > +static int sdw_program_device_num(struct sdw_bus *bus)
> > +{
> > +	u8 buf[SDW_NUM_DEV_ID_REGISTERS] = {0};
> > +	unsigned long long addr;
> 
> Use u64.

yes fixed

> > +	struct sdw_slave *slave;
> > +	struct sdw_slave_id id;
> > +	struct sdw_msg msg;
> > +	bool found = false;
> > +	int ret;
> > +
> > +	/* No Slave, so use raw xfer api */
> > +	sdw_fill_msg(&msg, SDW_SCP_DEVID_0, SDW_NUM_DEV_ID_REGISTERS,
> > +					0, SDW_MSG_FLAG_READ, buf);
> > +
> > +	do {
> > +		ret = sdw_transfer(bus, NULL, &msg);
> > +		if (ret == -ENODATA)
> > +			break;
> > +		if (ret < 0) {
> > +			dev_err(bus->dev, "DEVID read fail:%d\n", ret);
> > +			break;
> 
> So here we break, and the function returns zero.  Is this the expected
> behavior?

nope, thats a bug fixed now

> > +		if (found == false) {
> > +			/* TODO: Park this device in Group 13 */
> > +			dev_err(bus->dev, "Slave Entry not found");
> 
> No break here?  Otherwise...

Thats intentional. We want to still read next device that show up

> 
> > +		}
> > +
> > +	} while (ret == 0);
> 
> ... the outer loop may go endlessly.
> This condition doesn't look effective.

not really. We cant keep reading successfully. At some point all slaves will
ignore and return ENODATA and we exit. Bus errors will also make it exit

BUT given that we have seen stuff i am inclined to add a counter, we cant
have more than 11 device so that's a sane value to use :)

-- 
~Vinod