23 Apr
2018
23 Apr
'18
4:27 p.m.
On 4/21/18 10:53 AM, Vinod Koul wrote:
On Sat, Apr 21, 2018 at 06:47:24AM -0700, Pierre-Louis Bossart wrote:
+static int sdw_enable_disable_slave_ports(struct sdw_bus *bus,
struct sdw_slave_runtime *s_rt,struct sdw_port_runtime *p_rt, bool en)+{
- struct sdw_transport_params *t_params = &p_rt->transport_params;
- u32 addr;
- int ret;
- if (bus->params.next_bank)
addr = SDW_DPN_CHANNELEN_B1(p_rt->num);- else
addr = SDW_DPN_CHANNELEN_B0(p_rt->num);- /*
* Since bus doesn't support sharing a port across two streams,* it is safe to reset this register*/- if (en)
ret = sdw_update(s_rt->slave, addr, 0xFF, p_rt->ch_mask);- else
ret = sdw_update(s_rt->slave, addr, 0xFF, 0x0);- if (ret < 0)
dev_err(&s_rt->slave->dev,"Slave chn_en reg write failed:%d port:%d",ret, t_params->port_num);- return ret;
+}
Add a clarification that this routine only sets the enable/disable bits in the relevant bank, the actual enable/disable is done with a bank switch.
Isn't that something spec tells the reader? this is a SDW concept that we program bank and then switch to have the effect, so why would i add comment for that?
To make sure people understand your code and follow the recommendation from the spec. It's legal to write directly in enable bits but it's not recommended at all. A standard spec is always interpreted in different ways, adding a comment helps.