On Thu, Oct 19, 2017 at 08:33:22AM +0530, Vinod Koul wrote:
> +static bool sdw_get_page(struct sdw_slave *slave, struct sdw_msg *msg)
> +{
> + bool page = false, paging_support = false;
> +
> + if (slave && slave->prop.paging_support)
> + paging_support = true;
> +
> + /*
> + * Programme SCP page addr for:
> + * 1. addr_page1 and addr_page2 contains non-zero values.
> + * 2. Paging supported by Slave.
> + */
> + switch (msg->dev_num) {
> + case SDW_ENUM_DEV_NUM:
> + case SDW_BROADCAST_DEV_NUM:
> + break;
> +
> + default:
> + if (paging_support && ((msg->addr_page1) || (msg->addr_page2)))
> + page = true;
> + }
> +
> + return page;
So if a page was specified but we don't have paging support we silently
just write to the base pagee?
> +int sdw_transfer(struct sdw_bus *bus, struct sdw_slave *slave,
> + struct sdw_msg *msg)
> +{
> + bool page;
> + int ret;
> +
> + mutex_lock(&bus->msg_lock);
> +
> + page = sdw_get_page(slave, msg);
get_page() doesn't interact with the hardware at all so it could be
outside the lock.
> + ret = do_transfer(bus, msg, page);
> + if (ret != 0 && ret != -ENODATA) {
> + dev_err(bus->dev, "trf on Slave %d failed:%d\n",
> + msg->dev_num, ret);
> + goto error;
> + }
> +
> + if (page)
> + ret = sdw_reset_page(bus, msg->dev_num);
Wouldn't it be safer to reset the page even on error so future messages
go to the right place if the paging bit of the failed operation worked?
> +int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
> +{
> + struct sdw_msg msg;
> + int ret;
> +
> + pm_runtime_get_sync(slave->bus->dev);
No error check.
> + pm_runtime_get_sync(slave->bus->dev);
> +
> + sdw_fill_msg(&msg, addr, count, slave->dev_num, SDW_MSG_FLAG_WRITE,
> val);
The device doesn't need to be powered up for us to fill in the data
structures we're going to use.
signature.asc
Description: PGP signature
