| Message ID | 20191016070740.121435-3-codekipper@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: sun4i-i2s: Updates to the driver | expand |
Hi, On Wed, Oct 16, 2019 at 09:07:35AM +0200, codekipper@gmail.com wrote: > From: Marcus Cooper <codekipper@gmail.com> > > Newer SoCs like the H6 have the channel offset bits in a different > position to what is on the H3. As we will eventually add multi- > channel support then create function calls as opposed to regmap > fields to add support for different devices. > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > --- > sound/soc/sunxi/sun4i-i2s.c | 31 +++++++++++++++++++++++++------ > 1 file changed, 25 insertions(+), 6 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index f1a80973c450..875567881f30 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -157,6 +157,8 @@ struct sun4i_i2s_quirks { > int (*set_chan_cfg)(const struct sun4i_i2s *, > const struct snd_pcm_hw_params *); > int (*set_fmt)(struct sun4i_i2s *, unsigned int); > + void (*set_txchanoffset)(const struct sun4i_i2s *, int); > + void (*set_rxchanoffset)(const struct sun4i_i2s *); The point of removing the regmap_field was that because having a one-size-fits-all function with regmap_field sort of making the abstraction was becoming more and more of a burden to maintain. Having functions for each and every register access is exactly the same as using regmap_field here, and the issue we adressed is not with regmap_fields in itself. If the H6 has a different register layout, then so be it, create a new set_chan_cfg or set_fmt function for the H6. Maxime
On Wed, 16 Oct 2019 at 10:06, Maxime Ripard <mripard@kernel.org> wrote: > > Hi, > > On Wed, Oct 16, 2019 at 09:07:35AM +0200, codekipper@gmail.com wrote: > > From: Marcus Cooper <codekipper@gmail.com> > > > > Newer SoCs like the H6 have the channel offset bits in a different > > position to what is on the H3. As we will eventually add multi- > > channel support then create function calls as opposed to regmap > > fields to add support for different devices. > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > --- > > sound/soc/sunxi/sun4i-i2s.c | 31 +++++++++++++++++++++++++------ > > 1 file changed, 25 insertions(+), 6 deletions(-) > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > index f1a80973c450..875567881f30 100644 > > --- a/sound/soc/sunxi/sun4i-i2s.c > > +++ b/sound/soc/sunxi/sun4i-i2s.c > > @@ -157,6 +157,8 @@ struct sun4i_i2s_quirks { > > int (*set_chan_cfg)(const struct sun4i_i2s *, > > const struct snd_pcm_hw_params *); > > int (*set_fmt)(struct sun4i_i2s *, unsigned int); > > + void (*set_txchanoffset)(const struct sun4i_i2s *, int); > > + void (*set_rxchanoffset)(const struct sun4i_i2s *); > > The point of removing the regmap_field was that because having a > one-size-fits-all function with regmap_field sort of making the > abstraction was becoming more and more of a burden to maintain. > > Having functions for each and every register access is exactly the > same as using regmap_field here, and the issue we adressed is not with > regmap_fields in itself. > > If the H6 has a different register layout, then so be it, create a new > set_chan_cfg or set_fmt function for the H6. The H3 and the H6 have a similar register layout but the issue here is that sooner rather than later we would want to be supporting multi-channel audio which requires the offset to be applied to each TX channel channel select register(8chan PCM for HDMI requires 4 Tx channels). Currently we're only using one. BR, CK > > Maxime
On Wed, Oct 16, 2019 at 10:25:29AM +0200, Code Kipper wrote: > On Wed, 16 Oct 2019 at 10:06, Maxime Ripard <mripard@kernel.org> wrote: > > > > Hi, > > > > On Wed, Oct 16, 2019 at 09:07:35AM +0200, codekipper@gmail.com wrote: > > > From: Marcus Cooper <codekipper@gmail.com> > > > > > > Newer SoCs like the H6 have the channel offset bits in a different > > > position to what is on the H3. As we will eventually add multi- > > > channel support then create function calls as opposed to regmap > > > fields to add support for different devices. > > > > > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > > > --- > > > sound/soc/sunxi/sun4i-i2s.c | 31 +++++++++++++++++++++++++------ > > > 1 file changed, 25 insertions(+), 6 deletions(-) > > > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > > index f1a80973c450..875567881f30 100644 > > > --- a/sound/soc/sunxi/sun4i-i2s.c > > > +++ b/sound/soc/sunxi/sun4i-i2s.c > > > @@ -157,6 +157,8 @@ struct sun4i_i2s_quirks { > > > int (*set_chan_cfg)(const struct sun4i_i2s *, > > > const struct snd_pcm_hw_params *); > > > int (*set_fmt)(struct sun4i_i2s *, unsigned int); > > > + void (*set_txchanoffset)(const struct sun4i_i2s *, int); > > > + void (*set_rxchanoffset)(const struct sun4i_i2s *); > > > > The point of removing the regmap_field was that because having a > > one-size-fits-all function with regmap_field sort of making the > > abstraction was becoming more and more of a burden to maintain. > > > > Having functions for each and every register access is exactly the > > same as using regmap_field here, and the issue we adressed is not with > > regmap_fields in itself. > > > > If the H6 has a different register layout, then so be it, create a new > > set_chan_cfg or set_fmt function for the H6. > > The H3 and the H6 have a similar register layout but the issue here > is that sooner rather than later we would want to be supporting > multi-channel audio which requires the offset to be applied to each > TX channel channel select register(8chan PCM for HDMI requires 4 Tx > channels). Currently we're only using one. So, as it turns out, they do not have the same register layout? Maxime
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index f1a80973c450..875567881f30 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -157,6 +157,8 @@ struct sun4i_i2s_quirks { int (*set_chan_cfg)(const struct sun4i_i2s *, const struct snd_pcm_hw_params *); int (*set_fmt)(struct sun4i_i2s *, unsigned int); + void (*set_txchanoffset)(const struct sun4i_i2s *, int); + void (*set_rxchanoffset)(const struct sun4i_i2s *); }; struct sun4i_i2s { @@ -467,6 +469,23 @@ static int sun8i_i2s_set_chan_cfg(const struct sun4i_i2s *i2s, return 0; } +static void sun8i_i2s_set_txchanoffset(const struct sun4i_i2s *i2s, int output) +{ + if (output >= 0 && output < 4) + regmap_update_bits(i2s->regmap, + SUN8I_I2S_TX_CHAN_SEL_REG + (output * 4), + SUN8I_I2S_TX_CHAN_OFFSET_MASK, + SUN8I_I2S_TX_CHAN_OFFSET(i2s->offset)); +} + +static void sun8i_i2s_set_rxchanoffset(const struct sun4i_i2s *i2s) +{ + regmap_update_bits(i2s->regmap, + SUN8I_I2S_RX_CHAN_SEL_REG, + SUN8I_I2S_TX_CHAN_OFFSET_MASK, + SUN8I_I2S_TX_CHAN_OFFSET(i2s->offset)); +} + static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream, struct snd_pcm_hw_params *params, struct snd_soc_dai *dai) @@ -661,12 +680,10 @@ static int sun8i_i2s_set_soc_fmt(struct sun4i_i2s *i2s, regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG, SUN8I_I2S_CTRL_MODE_MASK, mode); - regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG, - SUN8I_I2S_TX_CHAN_OFFSET_MASK, - SUN8I_I2S_TX_CHAN_OFFSET(i2s->offset)); - regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG, - SUN8I_I2S_TX_CHAN_OFFSET_MASK, - SUN8I_I2S_TX_CHAN_OFFSET(i2s->offset)); + if (i2s->variant->set_txchanoffset) + i2s->variant->set_txchanoffset(i2s, 0); + if (i2s->variant->set_rxchanoffset) + i2s->variant->set_rxchanoffset(i2s); /* DAI clock master masks */ switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { @@ -1136,6 +1153,8 @@ static const struct sun4i_i2s_quirks sun8i_h3_i2s_quirks = { .get_wss = sun8i_i2s_get_sr_wss, .set_chan_cfg = sun8i_i2s_set_chan_cfg, .set_fmt = sun8i_i2s_set_soc_fmt, + .set_txchanoffset = sun8i_i2s_set_txchanoffset, + .set_rxchanoffset = sun8i_i2s_set_rxchanoffset, }; static const struct sun4i_i2s_quirks sun50i_a64_codec_i2s_quirks = {