Hi Markus,
On Sat, Jul 22, 2017 at 08:53:51AM +0200, codekipper@xxxxxxxxx wrote:
> From: Marcus Cooper <codekipper@xxxxxxxxx>
>
> In preparation for changing this driver to support newer SoC
> implementations then where needed there has been a switch from
> regmap_update_bits to regmap_field. Also included are adjustment
> variables although they are not set as no adjustment is required
> for the current support.
>
> Signed-off-by: Marcus Cooper <codekipper@xxxxxxxxx>
> ---
> sound/soc/sunxi/sun4i-i2s.c | 267 +++++++++++++++++++++++++++++++++-----------
> 1 file changed, 203 insertions(+), 64 deletions(-)
>
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index 62b307b0c846..1854405cbcb1 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -50,6 +50,8 @@
> #define SUN4I_I2S_FMT0_FMT_RIGHT_J (2 << 0)
> #define SUN4I_I2S_FMT0_FMT_LEFT_J (1 << 0)
> #define SUN4I_I2S_FMT0_FMT_I2S (0 << 0)
> +#define SUN4I_I2S_FMT0_POLARITY_INVERTED (1)
> +#define SUN4I_I2S_FMT0_POLARITY_NORMAL (0)
>
> #define SUN4I_I2S_FMT1_REG 0x08
> #define SUN4I_I2S_FIFO_TX_REG 0x0c
> @@ -72,7 +74,7 @@
> #define SUN4I_I2S_INT_STA_REG 0x20
>
> #define SUN4I_I2S_CLK_DIV_REG 0x24
> -#define SUN4I_I2S_CLK_DIV_MCLK_EN BIT(7)
> +#define SUN4I_I2S_CLK_DIV_MCLK_EN 7
> #define SUN4I_I2S_CLK_DIV_BCLK_MASK GENMASK(6, 4)
> #define SUN4I_I2S_CLK_DIV_BCLK(bclk) ((bclk) << 4)
> #define SUN4I_I2S_CLK_DIV_MCLK_MASK GENMASK(3, 0)
> @@ -82,15 +84,39 @@
> #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))
> +#define SUN4I_I2S_TX_CHAN_EN(num_chan) (((1 << num_chan) - 1))
>
> #define SUN4I_I2S_RX_CHAN_SEL_REG 0x38
> #define SUN4I_I2S_RX_CHAN_MAP_REG 0x3c
>
> +struct sun4i_i2s_quirks {
> + bool has_reset;
> + bool has_master_slave_sel;
I think both variants have a master and slave mode, so it's a bit
misleading.
You should also have a kerneldoc for that structure, to make it clear
what each quirk is supposed to be doing.
> + unsigned int reg_offset_txdata; /* TX FIFO */
> + unsigned int reg_offset_txchanmap;
> + unsigned int reg_offset_rxchanmap;
Is there any reason for txchanmap and rxchanmap to not be
regmap_fields too?
> + const struct regmap_config *sun4i_i2s_regmap;
> + unsigned int mclk_adjust;
> + unsigned int bclk_adjust;
> + unsigned int fmt_adjust;
I would replace adjust by offset
> + /* Register fields for i2s */
> + struct reg_field field_clkdiv_mclk_en;
> + struct reg_field field_fmt_set_wss;
> + struct reg_field field_fmt_set_sr;
> + struct reg_field field_fmt_set_bclk_polarity;
> + struct reg_field field_fmt_set_lrclk_polarity;
> + struct reg_field field_fmt_set_mode;
> + struct reg_field field_txchansel;
> + struct reg_field field_rxchansel;
> +};
> +
> struct sun4i_i2s {
> + struct device *dev;
You never use it outside of the probe function (and its callee), you
can just pass it directly as an argument
> struct clk *bus_clk;
> struct clk *mod_clk;
> struct regmap *regmap;
> @@ -100,6 +126,18 @@ 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_clkdiv_mclk_en;
> + struct regmap_field *field_fmt_set_wss;
> + struct regmap_field *field_fmt_set_sr;
> + struct regmap_field *field_fmt_set_bclk_polarity;
> + struct regmap_field *field_fmt_set_lrclk_polarity;
> + struct regmap_field *field_fmt_set_mode;
> + struct regmap_field *field_txchansel;
> + struct regmap_field *field_rxchansel;
> +
> + const struct sun4i_i2s_quirks *variant;
> };
>
> struct sun4i_i2s_clk_div {
> @@ -138,7 +176,7 @@ static int sun4i_i2s_get_bclk_div(struct sun4i_i2s *i2s,
> const struct sun4i_i2s_clk_div *bdiv = &sun4i_i2s_bclk_div[i];
>
> if (bdiv->div == div)
> - return bdiv->val;
> + return bdiv->val + i2s->variant->bclk_adjust;
> }
>
> return -EINVAL;
> @@ -156,7 +194,7 @@ static int sun4i_i2s_get_mclk_div(struct sun4i_i2s *i2s,
> const struct sun4i_i2s_clk_div *mdiv = &sun4i_i2s_mclk_div[i];
>
> if (mdiv->div == div)
> - return mdiv->val;
> + return mdiv->val + i2s->variant->mclk_adjust;
Could you split all that work to make it a bit easier to review?
You could do so by adding the various quirks in several steps, like
first the adjusts/offsets, then the regmap_fields then the TX FIFO
(and I guess RX too?), etc.
It looks much better otherwise, thanks!
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
