diff mbox series

[RFC,24/40] soundwire: cadence_master: use BIOS defaults for frame shape

Message ID 20190725234032.21152-25-pierre-louis.bossart@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series soundwire: updates for 5.4 | expand

Commit Message

Pierre-Louis Bossart July 25, 2019, 11:40 p.m. UTC 
Remove hard-coding and use BIOS values. If they are wrong use default
48x2 frame shape.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/cadence_master.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

Comments

Cezary Rojewski July 26, 2019, 10:20 a.m. UTC | #1 
On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
> +static u32 cdns_set_default_frame_shape(int n_rows, int n_cols)
> +{
> +	u32 val;
> +	int c;
> +	int r;
> +
> +	r = sdw_find_row_index(n_rows);
> +	c = sdw_find_col_index(n_cols);
> +
> +	val = (r << 3) | c;
> +
> +	return val;
> +}
> +
>   /**
>    * sdw_cdns_init() - Cadence initialization
>    * @cdns: Cadence instance
> @@ -977,7 +990,9 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
>   	cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
>   
>   	/* Set the default frame shape */
> -	cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, CDNS_DEFAULT_FRAME_SHAPE);
> +	val = cdns_set_default_frame_shape(prop->default_row,
> +					   prop->default_col);
> +	cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, val);
>   
>   	/* Set SSP interval to default value */
>   	cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL);
> 

Suggestion:
declare "generic" _get_frame_frame(rows, cols) instead and let it do the 
bitwise operations for you. Pretty sure this won't be the only place in 
code where reg value for frame_shape needs to be prepared.

Said function could be as simple as:
return (row << 3) | cols;
+inline flag

i.e. could be even a macro..

Otherwise modify _set_default_frame_shape to simply:
return (r << 3) | c

without involving additional local val variable (I don't really see the 
point for any locals there though).
Pierre-Louis Bossart July 26, 2019, 2:22 p.m. UTC | #2 
On 7/26/19 5:20 AM, Cezary Rojewski wrote:
> On 2019-07-26 01:40, Pierre-Louis Bossart wrote:
>> +static u32 cdns_set_default_frame_shape(int n_rows, int n_cols)
>> +{
>> +    u32 val;
>> +    int c;
>> +    int r;
>> +
>> +    r = sdw_find_row_index(n_rows);
>> +    c = sdw_find_col_index(n_cols);
>> +
>> +    val = (r << 3) | c;
>> +
>> +    return val;
>> +}
>> +
>>   /**
>>    * sdw_cdns_init() - Cadence initialization
>>    * @cdns: Cadence instance
>> @@ -977,7 +990,9 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
>>       cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
>>       /* Set the default frame shape */
>> -    cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, 
>> CDNS_DEFAULT_FRAME_SHAPE);
>> +    val = cdns_set_default_frame_shape(prop->default_row,
>> +                       prop->default_col);
>> +    cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, val);
>>       /* Set SSP interval to default value */
>>       cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL);
>>
> 
> Suggestion:
> declare "generic" _get_frame_frame(rows, cols) instead and let it do the 
> bitwise operations for you. Pretty sure this won't be the only place in 
> code where reg value for frame_shape needs to be prepared.
> 
> Said function could be as simple as:
> return (row << 3) | cols;
> +inline flag
> 
> i.e. could be even a macro..
> 
> Otherwise modify _set_default_frame_shape to simply:
> return (r << 3) | c
> 
> without involving additional local val variable (I don't really see the 
> point for any locals there though).

what this function does is take the standard-defined offsets for row and 
column and stores them in a Cadence-defined register. I think we can 
probably use a macro to remove the magic '3' value, but there are limits 
to what we can simplify. I should probably add comments so that people 
figure it out.
Vinod Koul Aug. 2, 2019, 5:10 p.m. UTC | #3 
On 25-07-19, 18:40, Pierre-Louis Bossart wrote:
> Remove hard-coding and use BIOS values. If they are wrong use default

BIOS :) this is cadence, am sure this can be used outside BIOS :D

It would be better to say firmware (ACPI/DT)

> 48x2 frame shape.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/cadence_master.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index 442f78c00f09..d84344e29f71 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -175,7 +175,6 @@
>  /* Driver defaults */
>  
>  #define CDNS_DEFAULT_CLK_DIVIDER		0
> -#define CDNS_DEFAULT_FRAME_SHAPE		0x30
>  #define CDNS_DEFAULT_SSP_INTERVAL		0x18
>  #define CDNS_TX_TIMEOUT				2000
>  
> @@ -954,6 +953,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
>  }
>  EXPORT_SYMBOL(sdw_cdns_pdi_init);
>  
> +static u32 cdns_set_default_frame_shape(int n_rows, int n_cols)
> +{
> +	u32 val;
> +	int c;
> +	int r;

This can be in single line!

> +
> +	r = sdw_find_row_index(n_rows);
> +	c = sdw_find_col_index(n_cols);
> +
> +	val = (r << 3) | c;

Magic 3?
Pierre-Louis Bossart Aug. 2, 2019, 5:24 p.m. UTC | #4 
On 8/2/19 12:10 PM, Vinod Koul wrote:
> On 25-07-19, 18:40, Pierre-Louis Bossart wrote:
>> Remove hard-coding and use BIOS values. If they are wrong use default
> 
> BIOS :) this is cadence, am sure this can be used outside BIOS :D
> 
> It would be better to say firmware (ACPI/DT)

yes

> 
>> 48x2 frame shape.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> ---
>>   drivers/soundwire/cadence_master.c | 19 +++++++++++++++++--
>>   1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
>> index 442f78c00f09..d84344e29f71 100644
>> --- a/drivers/soundwire/cadence_master.c
>> +++ b/drivers/soundwire/cadence_master.c
>> @@ -175,7 +175,6 @@
>>   /* Driver defaults */
>>   
>>   #define CDNS_DEFAULT_CLK_DIVIDER		0
>> -#define CDNS_DEFAULT_FRAME_SHAPE		0x30
>>   #define CDNS_DEFAULT_SSP_INTERVAL		0x18
>>   #define CDNS_TX_TIMEOUT				2000
>>   
>> @@ -954,6 +953,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
>>   }
>>   EXPORT_SYMBOL(sdw_cdns_pdi_init);
>>   
>> +static u32 cdns_set_default_frame_shape(int n_rows, int n_cols)
>> +{
>> +	u32 val;
>> +	int c;
>> +	int r;
> 
> This can be in single line!

one line per variable is what I prefer.

> 
>> +
>> +	r = sdw_find_row_index(n_rows);
>> +	c = sdw_find_col_index(n_cols);
>> +
>> +	val = (r << 3) | c;
> 
> Magic 3?

yes fixed already.
Sanyog Kale Aug. 5, 2019, 10:01 a.m. UTC | #5 
On Thu, Jul 25, 2019 at 06:40:16PM -0500, Pierre-Louis Bossart wrote:
> Remove hard-coding and use BIOS values. If they are wrong use default
> 48x2 frame shape.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  drivers/soundwire/cadence_master.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
> index 442f78c00f09..d84344e29f71 100644
> --- a/drivers/soundwire/cadence_master.c
> +++ b/drivers/soundwire/cadence_master.c
> @@ -175,7 +175,6 @@
>  /* Driver defaults */
>  
>  #define CDNS_DEFAULT_CLK_DIVIDER		0
> -#define CDNS_DEFAULT_FRAME_SHAPE		0x30
>  #define CDNS_DEFAULT_SSP_INTERVAL		0x18
>  #define CDNS_TX_TIMEOUT				2000
>  
> @@ -954,6 +953,20 @@ int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
>  }
>  EXPORT_SYMBOL(sdw_cdns_pdi_init);
>  
> +static u32 cdns_set_default_frame_shape(int n_rows, int n_cols)
> +{
> +	u32 val;
> +	int c;
> +	int r;
> +
> +	r = sdw_find_row_index(n_rows);
> +	c = sdw_find_col_index(n_cols);
> +

Now i get why you need above functions to be exported, please ignore my
previous comment.

> +	val = (r << 3) | c;
> +
> +	return val;
> +}
> +
>  /**
>   * sdw_cdns_init() - Cadence initialization
>   * @cdns: Cadence instance
> @@ -977,7 +990,9 @@ int sdw_cdns_init(struct sdw_cdns *cdns)
>  	cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
>  
>  	/* Set the default frame shape */
> -	cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, CDNS_DEFAULT_FRAME_SHAPE);
> +	val = cdns_set_default_frame_shape(prop->default_row,
> +					   prop->default_col);
> +	cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, val);
>  
>  	/* Set SSP interval to default value */
>  	cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL);
> -- 
> 2.20.1
>
diff mbox series

Patch

diff --git a/drivers/soundwire/cadence_master.c b/drivers/soundwire/cadence_master.c
index 442f78c00f09..d84344e29f71 100644
--- a/drivers/soundwire/cadence_master.c
+++ b/drivers/soundwire/cadence_master.c
@@ -175,7 +175,6 @@ 
 /* Driver defaults */
 
 #define CDNS_DEFAULT_CLK_DIVIDER		0
-#define CDNS_DEFAULT_FRAME_SHAPE		0x30
 #define CDNS_DEFAULT_SSP_INTERVAL		0x18
 #define CDNS_TX_TIMEOUT				2000
 
@@ -954,6 +953,20 @@  int sdw_cdns_pdi_init(struct sdw_cdns *cdns,
 }
 EXPORT_SYMBOL(sdw_cdns_pdi_init);
 
+static u32 cdns_set_default_frame_shape(int n_rows, int n_cols)
+{
+	u32 val;
+	int c;
+	int r;
+
+	r = sdw_find_row_index(n_rows);
+	c = sdw_find_col_index(n_cols);
+
+	val = (r << 3) | c;
+
+	return val;
+}
+
 /**
  * sdw_cdns_init() - Cadence initialization
  * @cdns: Cadence instance
@@ -977,7 +990,9 @@  int sdw_cdns_init(struct sdw_cdns *cdns)
 	cdns_writel(cdns, CDNS_MCP_CLK_CTRL0, val);
 
 	/* Set the default frame shape */
-	cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, CDNS_DEFAULT_FRAME_SHAPE);
+	val = cdns_set_default_frame_shape(prop->default_row,
+					   prop->default_col);
+	cdns_writel(cdns, CDNS_MCP_FRAME_SHAPE_INIT, val);
 
 	/* Set SSP interval to default value */
 	cdns_writel(cdns, CDNS_MCP_SSP_CTRL0, CDNS_DEFAULT_SSP_INTERVAL);