Re: [PATCH 7/8] soundwire: fix master properties

[Vinod Koul <vkoul@kernel.org>]

On 03-05-19, 19:29, Pierre-Louis Bossart wrote:
> The master-count is only defined for a controller or a Slave in the
> MIPI DisCo for SoundWire document.

... so remove it

> rename all fields with 'freq' as 'clk_freq' to follow the MIPI
> specification and avoid confusion between bus clock and audio clocks.

That sounds good to me.

> fix support for clock_stop_mode0 and 1. The existing code uses a
> bitmask between enums, one of which being zero. Or'ing with zero is
> not very useful in general...Fix by or-ing with a BIT dependent on the
> enum value.
> 
> Fix additional comments to align with MIPI spec

Ideally these should be different patches...


> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/bus.c        |  4 ++--
>  drivers/soundwire/intel.c      | 11 ++++++-----
>  drivers/soundwire/mipi_disco.c | 27 ++++++++++++++-------------
>  drivers/soundwire/stream.c     |  2 +-
>  include/linux/soundwire/sdw.h  | 20 +++++++++-----------
>  5 files changed, 32 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index aac35fc3cf22..96e42df8f458 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -87,7 +87,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
>  
>  	/*
>  	 * Initialize clock values based on Master properties. The max
> -	 * frequency is read from max_freq property. Current assumption
> +	 * frequency is read from max_clk_freq property. Current assumption
>  	 * is that the bus will start at highest clock frequency when
>  	 * powered on.
>  	 *
> @@ -95,7 +95,7 @@ int sdw_add_bus_master(struct sdw_bus *bus)
>  	 * to start with bank 0 (Table 40 of Spec)
>  	 */
>  	prop = &bus->prop;
> -	bus->params.max_dr_freq = prop->max_freq * SDW_DOUBLE_RATE_FACTOR;
> +	bus->params.max_dr_freq = prop->max_clk_freq * SDW_DOUBLE_RATE_FACTOR;
>  	bus->params.curr_dr_freq = bus->params.max_dr_freq;
>  	bus->params.curr_bank = SDW_BANK0;
>  	bus->params.next_bank = SDW_BANK1;
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index 31336b0271b0..4ac141730b13 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -796,13 +796,14 @@ static int intel_prop_read(struct sdw_bus *bus)
>  	sdw_master_read_prop(bus);
>  
>  	/* BIOS is not giving some values correctly. So, lets override them */
> -	bus->prop.num_freq = 1;
> -	bus->prop.freq = devm_kcalloc(bus->dev, bus->prop.num_freq,
> -				      sizeof(*bus->prop.freq), GFP_KERNEL);
> -	if (!bus->prop.freq)
> +	bus->prop.num_clk_freq = 1;
> +	bus->prop.clk_freq = devm_kcalloc(bus->dev, bus->prop.num_clk_freq,
> +					  sizeof(*bus->prop.clk_freq),
> +					  GFP_KERNEL);
> +	if (!bus->prop.clk_freq)
>  		return -ENOMEM;
>  
> -	bus->prop.freq[0] = bus->prop.max_freq;
> +	bus->prop.clk_freq[0] = bus->prop.max_clk_freq;
>  	bus->prop.err_threshold = 5;
>  
>  	return 0;
> diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
> index f6b1be920a19..7db816691393 100644
> --- a/drivers/soundwire/mipi_disco.c
> +++ b/drivers/soundwire/mipi_disco.c
> @@ -50,39 +50,40 @@ int sdw_master_read_prop(struct sdw_bus *bus)
>  
>  	if (fwnode_property_read_bool(link,
>  				      "mipi-sdw-clock-stop-mode0-supported"))
> -		prop->clk_stop_mode = SDW_CLK_STOP_MODE0;
> +		prop->clk_stop_modes |= BIT(SDW_CLK_STOP_MODE0);
>  
>  	if (fwnode_property_read_bool(link,
>  				      "mipi-sdw-clock-stop-mode1-supported"))
> -		prop->clk_stop_mode |= SDW_CLK_STOP_MODE1;
> +		prop->clk_stop_modes |= BIT(SDW_CLK_STOP_MODE1);
>  
>  	fwnode_property_read_u32(link,
>  				 "mipi-sdw-max-clock-frequency",
> -				 &prop->max_freq);
> +				 &prop->max_clk_freq);
>  
>  	nval = fwnode_property_read_u32_array(link,
>  			"mipi-sdw-clock-frequencies-supported", NULL, 0);
>  	if (nval > 0) {
> -		prop->num_freq = nval;
> -		prop->freq = devm_kcalloc(bus->dev, prop->num_freq,
> -					  sizeof(*prop->freq), GFP_KERNEL);
> -		if (!prop->freq)
> +		prop->num_clk_freq = nval;
> +		prop->clk_freq = devm_kcalloc(bus->dev, prop->num_clk_freq,
> +					      sizeof(*prop->clk_freq),
> +					      GFP_KERNEL);
> +		if (!prop->clk_freq)
>  			return -ENOMEM;
>  
>  		fwnode_property_read_u32_array(link,
>  				"mipi-sdw-clock-frequencies-supported",
> -				prop->freq, prop->num_freq);
> +				prop->clk_freq, prop->num_clk_freq);
>  	}
>  
>  	/*
>  	 * Check the frequencies supported. If FW doesn't provide max
>  	 * freq, then populate here by checking values.
>  	 */
> -	if (!prop->max_freq && prop->freq) {
> -		prop->max_freq = prop->freq[0];
> -		for (i = 1; i < prop->num_freq; i++) {
> -			if (prop->freq[i] > prop->max_freq)
> -				prop->max_freq = prop->freq[i];
> +	if (!prop->max_clk_freq && prop->clk_freq) {
> +		prop->max_clk_freq = prop->clk_freq[0];
> +		for (i = 1; i < prop->num_clk_freq; i++) {
> +			if (prop->clk_freq[i] > prop->max_clk_freq)
> +				prop->max_clk_freq = prop->clk_freq[i];
>  		}
>  	}
>  
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index d01060dbee96..89edc897b8eb 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1474,7 +1474,7 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
>  		memcpy(&params, &bus->params, sizeof(params));
>  
>  		/* TODO: Support Asynchronous mode */
> -		if ((prop->max_freq % stream->params.rate) != 0) {
> +		if ((prop->max_clk_freq % stream->params.rate) != 0) {
>  			dev_err(bus->dev, "Async mode not supported\n");
>  			return -EINVAL;
>  		}
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 80584e9d5970..89c51838b7ec 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -364,29 +364,27 @@ struct sdw_slave_prop {
>  /**
>   * struct sdw_master_prop - Master properties
>   * @revision: MIPI spec version of the implementation
> - * @master_count: Number of masters
> - * @clk_stop_mode: Bitmap for Clock Stop modes supported
> - * @max_freq: Maximum Bus clock frequency, in Hz
> + * @clk_stop_modes: Bitmap, bit N set when clock-stop-modeN supported
> + * @max_clk_freq: Maximum Bus clock frequency, in Hz
>   * @num_clk_gears: Number of clock gears supported
>   * @clk_gears: Clock gears supported
> - * @num_freq: Number of clock frequencies supported, in Hz
> - * @freq: Clock frequencies supported, in Hz
> + * @num_clk_freq: Number of clock frequencies supported, in Hz
> + * @clk_freq: Clock frequencies supported, in Hz
>   * @default_frame_rate: Controller default Frame rate, in Hz
>   * @default_row: Number of rows
>   * @default_col: Number of columns
> - * @dynamic_frame: Dynamic frame supported
> + * @dynamic_frame: Dynamic frame shape supported
>   * @err_threshold: Number of times that software may retry sending a single
>   * command
>   */
>  struct sdw_master_prop {
>  	u32 revision;
> -	u32 master_count;
> -	enum sdw_clk_stop_mode clk_stop_mode;
> -	u32 max_freq;
> +	u32 clk_stop_modes;
> +	u32 max_clk_freq;
>  	u32 num_clk_gears;
>  	u32 *clk_gears;
> -	u32 num_freq;
> -	u32 *freq;
> +	u32 num_clk_freq;
> +	u32 *clk_freq;
>  	u32 default_frame_rate;
>  	u32 default_row;
>  	u32 default_col;
> -- 
> 2.17.1

-- 
~Vinod