Messages in this thread | | | Subject | Re: [alsa-devel] [PATCH v4 08/15] soundwire: Add Slave status handling helpers | From | Pierre-Louis Bossart <> | Date | Sun, 3 Dec 2017 21:07:29 -0600 |
| |
On 12/3/17 11:08 AM, Vinod Koul wrote: > On Fri, Dec 01, 2017 at 05:36:47PM -0600, Pierre-Louis Bossart wrote: > >>> +/* called with bus_lock held */ >>> +static int sdw_get_device_num(struct sdw_slave *slave) >>> +{ >>> + int bit; >>> + >>> + bit = find_first_zero_bit(slave->bus->assigned, SDW_MAX_DEVICES); >>> + if (bit == SDW_MAX_DEVICES) { >>> + bit = -ENODEV; >>> + goto err; >> >> My brain is starting to fry but is this correct? Bit11 seems like a valid >> value. Should it be bit > 15 (assuming bit 12,13,14 are set to avoid using >> groups and master)? > > this is correct. You are confusing SDW concept and API return types! > That should be hint for you to start weekend if you didn't do so :D > > This API returns max value it was provided (last arg) if it doesn't > find free bit. That's an indication to caller that we ran out of devices > hence ENODEV error!
Can you just make sure bit11 is included?
> >>> +static int sdw_program_device_num(struct sdw_bus *bus) >>> +{ >>> + u8 buf[SDW_NUM_DEV_ID_REGISTERS] = {0}; >>> + struct sdw_slave *slave, *_s; >>> + struct sdw_slave_id id; >>> + struct sdw_msg msg; >>> + bool found = false; >>> + int count = 0, ret; >>> + u64 addr; >>> + >>> + /* No Slave, so use raw xfer api */ >>> + ret = sdw_fill_msg(&msg, NULL, SDW_SCP_DEVID_0, >>> + SDW_NUM_DEV_ID_REGISTERS, 0, SDW_MSG_FLAG_READ, buf); >>> + if (ret < 0) >>> + return ret; >>> + >>> + do { >>> + ret = sdw_transfer(bus, NULL, &msg); >>> + if (ret == -ENODATA) { /* end of device id reads */ >>> + ret = 0; >>> + break; >>> + } >>> + if (ret < 0) { >>> + dev_err(bus->dev, "DEVID read fail:%d\n", ret); >>> + break; >>> + } >>> + >>> + /* >>> + * Construct the addr and extract. Cast the higher shift >>> + * bits to avoid truncation due to size limit. >>> + */ >>> + addr = buf[5] | (buf[4] << 8) | (buf[3] << 16) | >>> + (buf[2] << 24) | ((unsigned long long)buf[1] << 32) | >>> + ((unsigned long long)buf[0] << 40); >>> + >>> + sdw_extract_slave_id(bus, addr, &id); >>> + >>> + /* Now compare with entries */ >>> + list_for_each_entry_safe(slave, _s, &bus->slaves, node) { >>> + if (sdw_compare_devid(slave, id) == 0) { >>> + found = true; >>> + >>> + /* >>> + * Assign a new dev_num to this Slave and >>> + * not mark it present. It will be marked >>> + * present after it reports ATTACHED on new >>> + * dev_num >>> + */ >>> + ret = sdw_assign_device_num(slave); >>> + if (ret) { >>> + dev_err(slave->bus->dev, >>> + "Assign dev_num failed:%d", >>> + ret); >>> + return ret; >>> + } >>> + >>> + break; >>> + } >>> + } >>> + >>> + if (found == false) { >>> + /* TODO: Park this device in Group 13 */ >>> + dev_err(bus->dev, "Slave Entry not found"); >>> + } >>> + >>> + count++; >>> + >>> + } while (ret == 0 && count < (SDW_MAX_DEVICES * 2)); >> >> explain that the last condition is intentional - this is not a bug -, some >> devices can drop off during enumeration and rejoin so might be counted >> twice. > > ok will add >
| |