Re: [RFC PATCH] soundwire: bus: Add flag to mark DPN_BlockCtrl1 as readonly

[Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>]

On 3/9/20 12:37 PM, Srinivas Kandagatla wrote:
> According to SoundWire Specification Version 1.2.
> "A Data Port number X (in the range 0-14) which supports only one
> value of WordLength may implement the WordLength field in the
> DPX_BlockCtrl1 Register as Read-Only, returning the fixed value of
> WordLength in response to reads."

Interesting.

I think it's a spec issue that you want to bring to the MIPI Audio WG 
attention.

The note below says 'the DPx_BlockCtrl1 Register remains as Read-Write, 
but the value written to the Read-Only field is not used'

Ignoring a value and returning an error are two different behaviors indeed.

My recommendation would be to add a DisCo property stating the 
WordLength value can be used by the bus code but not written to the 
Slave device registers.

> As WSA881x interfaces in PDM mode making the only field "WordLength"
> in DPX_BlockCtrl1" fixed and read-only. Behaviour of writing to this
> register on WSA881x soundwire slave with Qualcomm Soundwire Controller
> is throwing up an error. Not sure how other controllers deal with
> writing to readonly registers, but this patch provides a way to avoid
> writes to DPN_BlockCtrl1 register by providing a ro_blockctrl1_reg
> flag in struct sdw_port_runtime.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> ---
>
> I will send patch for WSA881x to include this change once this patch
> is accepted.
>
>   drivers/soundwire/bus.h    |  2 ++
>   drivers/soundwire/stream.c | 17 ++++++++++-------
>   2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 204204a26db8..791e8d14093e 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -79,6 +79,7 @@ int sdw_find_col_index(int col);
>    * @num: Port number. For audio streams, valid port number ranges from
>    * [1,14]
>    * @ch_mask: Channel mask
> + * @ro_blockctrl1_reg: Read Only flag for DPN_BlockCtrl1 register
>    * @transport_params: Transport parameters
>    * @port_params: Port parameters
>    * @port_node: List node for Master or Slave port_list
> @@ -89,6 +90,7 @@ int sdw_find_col_index(int col);
>   struct sdw_port_runtime {
>   	int num;
>   	int ch_mask;
> +	bool ro_blockctrl1_reg;
>   	struct sdw_transport_params transport_params;
>   	struct sdw_port_params port_params;
>   	struct list_head port_node;
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 00348d1fc606..4491643aeb4a 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -167,13 +167,15 @@ static int sdw_program_slave_port_params(struct sdw_bus *bus,
>   		return ret;
>   	}
>   
> -	/* Program DPN_BlockCtrl1 register */
> -	ret = sdw_write(s_rt->slave, addr2, (p_params->bps - 1));
> -	if (ret < 0) {
> -		dev_err(&s_rt->slave->dev,
> -			"DPN_BlockCtrl1 register write failed for port %d\n",
> -			t_params->port_num);
> -		return ret;
> +	if (!p_rt->ro_blockctrl1_reg) {
> +		/* Program DPN_BlockCtrl1 register */
> +		ret = sdw_write(s_rt->slave, addr2, (p_params->bps - 1));
> +		if (ret < 0) {
> +			dev_err(&s_rt->slave->dev,
> +				"DPN_BlockCtrl1 register write failed for port %d\n",
> +				t_params->port_num);
> +			return ret;
> +		}
>   	}
>   
>   	/* Program DPN_SampleCtrl1 register */
> @@ -1195,6 +1197,7 @@ static struct sdw_port_runtime
>   
>   	p_rt->ch_mask = port_config[port_index].ch_mask;
>   	p_rt->num = port_config[port_index].num;
> +	p_rt->ro_blockctrl1_reg = port_config[port_index].ro_blockctrl1_reg;
>   
>   	return p_rt;
>   }