Re: [linux-sunxi] [PATCH v3 04/11] ASoC: sun4i-i2s: Add regmap fields for channels

[Chen-Yu Tsai]

On Sat, Aug 12, 2017 at 7:00 PM,   wrote:
> From: Marcus Cooper 
>
> On the original i2s block the channel mapping and selection were
> configured for stereo audio by default: This is not the case with
> the newer SoCs and they are also located at different offsets.
>
> To support the newer SoC then regmap fields have been added to the
> quirks and these are initialised to their correct settings during
> probing.
>
> Signed-off-by: Marcus Cooper 
> Reviewed-by: Chen-Yu Tsai 
> ---
>  sound/soc/sunxi/sun4i-i2s.c | 84 
> -
>  1 file changed, 76 insertions(+), 8 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index dfb79492..a65dcb013247 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c

[...]

> @@ -681,14 +701,56 @@ static const struct sun4i_i2s_quirks 
> sun4i_a10_i2s_quirks = {
> .has_reset  = false,
> .reg_offset_txdata  = SUN4I_I2S_FIFO_TX_REG,
> .sun4i_i2s_regmap   = &sun4i_i2s_regmap_config,
> +   .field_txchanmap= REG_FIELD(SUN4I_I2S_TX_CHAN_MAP_REG, 0, 31),
> +   .field_rxchanmap= REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31),
> +   .field_txchansel= REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2),
> +   .field_rxchansel= REG_FIELD(SUN4I_I2S_RX_CHAN_SEL_REG, 0, 2),
>  };
>
>  static const struct sun4i_i2s_quirks sun6i_a31_i2s_quirks = {
> .has_reset  = true,
> .reg_offset_txdata  = SUN4I_I2S_FIFO_TX_REG,
> .sun4i_i2s_regmap   = &sun4i_i2s_regmap_config,
> +   .field_txchanmap= REG_FIELD(SUN4I_I2S_TX_CHAN_MAP_REG, 0, 31),
> +   .field_rxchanmap= REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31),
> +   .field_txchansel= REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2),
> +   .field_rxchansel= REG_FIELD(SUN4I_I2S_RX_CHAN_SEL_REG, 0, 2),
>  };
>
> +static int sun4i_i2s_init_regmap_fields(struct device *dev,
> +   struct sun4i_i2s *i2s)
> +{
> +   int ret;
> +
> +   i2s->field_txchanmap =
> +   devm_regmap_field_alloc(dev, i2s->regmap,
> +   i2s->variant->field_txchanmap);
> +   ret = PTR_ERR_OR_ZERO(i2s->field_txchanmap);
> +
> +   if (!ret) {
> +   i2s->field_rxchanmap =
> +   devm_regmap_field_alloc(dev, i2s->regmap,
> +   
> i2s->variant->field_rxchanmap);
> +   ret = PTR_ERR_OR_ZERO(i2s->field_rxchanmap);
> +   }
> +
> +   if (!ret) {
> +   i2s->field_txchansel =
> +   devm_regmap_field_alloc(dev, i2s->regmap,
> +   
> i2s->variant->field_txchansel);
> +   ret = PTR_ERR_OR_ZERO(i2s->field_txchansel);
> +   }
> +
> +   if (!ret) {
> +   i2s->field_rxchansel =
> +   devm_regmap_field_alloc(dev, i2s->regmap,
> +   
> i2s->variant->field_rxchansel);
> +   ret = PTR_ERR_OR_ZERO(i2s->field_rxchansel);

I'm quite sure this was not what we meant when we recommended
the usage of PTR_ERR_OR_ZERO...

Rather you should have kept the

if (PTR_ERR(...))
return PTR_ERR(...);

for all but the last conditional blocks. For the last one just

return PTR_ERR_OR_ZERO(...);

Returning early, as opposed to what you have here, is also
easier to read. People looking at it will notice early on
that if something wrong happens, you bail out. Instead with
this structure, people have to read down a long list of ifs
that aren't doing anything. Not to mention that you have to
indent every code block except the first one by one tab,
reducing any screen real estate you have.

ChenYu

> +   }
> +
> +   return ret;
> +}
> +
>  static int sun4i_i2s_probe(struct platform_device *pdev)
>  {
> struct sun4i_i2s *i2s;

[...]

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] [PATCH v3 04/11] ASoC: sun4i-i2s: Add regmap fields for channels

[codekipper]

From: Marcus Cooper 

On the original i2s block the channel mapping and selection were
configured for stereo audio by default: This is not the case with
the newer SoCs and they are also located at different offsets.

To support the newer SoC then regmap fields have been added to the
quirks and these are initialised to their correct settings during
probing.

Signed-off-by: Marcus Cooper 
Reviewed-by: Chen-Yu Tsai 
---
 sound/soc/sunxi/sun4i-i2s.c | 84 -
 1 file changed, 76 insertions(+), 8 deletions(-)

diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
index dfb79492..a65dcb013247 100644
--- a/sound/soc/sunxi/sun4i-i2s.c
+++ b/sound/soc/sunxi/sun4i-i2s.c
@@ -82,7 +82,7 @@
 #define SUN4I_I2S_TX_CNT_REG   0x2c
 
 #define SUN4I_I2S_TX_CHAN_SEL_REG  0x30
-#define SUN4I_I2S_TX_CHAN_SEL(num_chan)(((num_chan) - 1) << 0)
+#define SUN4I_I2S_CHAN_SEL(num_chan)   (((num_chan) - 1) << 0)
 
 #define SUN4I_I2S_TX_CHAN_MAP_REG  0x34
 #define SUN4I_I2S_TX_CHAN_MAP(chan, sample)((sample) << (chan << 2))
@@ -98,6 +98,10 @@
  * @sun4i_i2s_regmap: regmap config to use.
  * @mclk_offset: Value by which mclkdiv needs to be adjusted.
  * @bclk_offset: Value by which bclkdiv needs to be adjusted.
+ * @field_txchanmap: location of the tx channel mapping register.
+ * @field_rxchanmap: location of the rx channel mapping register.
+ * @field_txchansel: location of the tx channel select bit fields.
+ * @field_rxchansel: location of the rx channel select bit fields.
  */
 struct sun4i_i2s_quirks {
boolhas_reset;
@@ -105,6 +109,12 @@ struct sun4i_i2s_quirks {
const struct regmap_config  *sun4i_i2s_regmap;
unsigned intmclk_offset;
unsigned intbclk_offset;
+
+   /* Register fields for i2s */
+   struct reg_fieldfield_txchanmap;
+   struct reg_fieldfield_rxchanmap;
+   struct reg_fieldfield_txchansel;
+   struct reg_fieldfield_rxchansel;
 };
 
 struct sun4i_i2s {
@@ -118,6 +128,12 @@ struct sun4i_i2s {
struct snd_dmaengine_dai_dma_data   capture_dma_data;
struct snd_dmaengine_dai_dma_data   playback_dma_data;
 
+   /* Register fields for i2s */
+   struct regmap_field *field_txchanmap;
+   struct regmap_field *field_rxchanmap;
+   struct regmap_field *field_txchansel;
+   struct regmap_field *field_rxchansel;
+
const struct sun4i_i2s_quirks   *variant;
 };
 
@@ -268,6 +284,17 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream 
*substream,
if (params_channels(params) != 2)
return -EINVAL;
 
+   /* Map the channels for playback and capture */
+   regmap_field_write(i2s->field_txchanmap, 0x76543210);
+   regmap_field_write(i2s->field_rxchanmap, 0x3210);
+
+   /* Configure the channels */
+   regmap_field_write(i2s->field_txchansel,
+  SUN4I_I2S_CHAN_SEL(params_channels(params)));
+
+   regmap_field_write(i2s->field_rxchansel,
+  SUN4I_I2S_CHAN_SEL(params_channels(params)));
+
switch (params_physical_width(params)) {
case 16:
width = DMA_SLAVE_BUSWIDTH_2_BYTES;
@@ -490,13 +517,6 @@ static int sun4i_i2s_startup(struct snd_pcm_substream 
*substream,
   SUN4I_I2S_CTRL_SDO_EN_MASK,
   SUN4I_I2S_CTRL_SDO_EN(0));
 
-   /* Enable the first two channels */
-   regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_SEL_REG,
-SUN4I_I2S_TX_CHAN_SEL(2));
-
-   /* Map them to the two first samples coming in */
-   regmap_write(i2s->regmap, SUN4I_I2S_TX_CHAN_MAP_REG,
-SUN4I_I2S_TX_CHAN_MAP(0, 0) | SUN4I_I2S_TX_CHAN_MAP(1, 1));
 
return clk_prepare_enable(i2s->mod_clk);
 }
@@ -681,14 +701,56 @@ static const struct sun4i_i2s_quirks sun4i_a10_i2s_quirks 
= {
.has_reset  = false,
.reg_offset_txdata  = SUN4I_I2S_FIFO_TX_REG,
.sun4i_i2s_regmap   = &sun4i_i2s_regmap_config,
+   .field_txchanmap= REG_FIELD(SUN4I_I2S_TX_CHAN_MAP_REG, 0, 31),
+   .field_rxchanmap= REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31),
+   .field_txchansel= REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2),
+   .field_rxchansel= REG_FIELD(SUN4I_I2S_RX_CHAN_SEL_REG, 0, 2),
 };
 
 static const struct sun4i_i2s_quirks sun6i_a31_i2s_quirks = {
.has_reset  = true,
.reg_offset_txdata  = SUN4I_I2S_FIFO_TX_REG,
.sun4i_i2s_regmap   = &sun4i_i2s_regmap_config,
+   .field_txchanmap= REG_FIELD(SUN4I_I2S_TX_CHAN_MAP_REG, 0, 31),
+   .field_rxchanmap= REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31),
+   .field_txchansel= REG_FIELD(SUN4I_I2S_TX_CHAN_SEL