Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750974AbdGYFw6 (ORCPT ); Tue, 25 Jul 2017 01:52:58 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:40040 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750783AbdGYFw5 (ORCPT ); Tue, 25 Jul 2017 01:52:57 -0400 Date: Tue, 25 Jul 2017 07:52:55 +0200 From: Maxime Ripard To: codekipper@gmail.com Cc: linux-arm-kernel@lists.infradead.org, linux-sunxi@googlegroups.com, lgirdwood@gmail.com, broonie@kernel.org, linux-kernel@vger.kernel.org, alsa-devel@alsa-project.org, be17068@iperbole.bo.it Subject: Re: [PATCH v2 1/2] ASoC: sun4i-i2s: Add more quirks for newer SoCs Message-ID: <20170725055255.tahwdluwhklyqedu@flea> References: <20170722065352.7635-1-codekipper@gmail.com> <20170722065352.7635-2-codekipper@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="mlyqlexrsjlharpf" Content-Disposition: inline In-Reply-To: <20170722065352.7635-2-codekipper@gmail.com> User-Agent: NeoMutt/20170714 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5975 Lines: 175 --mlyqlexrsjlharpf Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Markus, On Sat, Jul 22, 2017 at 08:53:51AM +0200, codekipper@gmail.com wrote: > From: Marcus Cooper >=20 > 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. >=20 > Signed-off-by: Marcus Cooper > --- > sound/soc/sunxi/sun4i-i2s.c | 267 +++++++++++++++++++++++++++++++++-----= ------ > 1 file changed, 203 insertions(+), 64 deletions(-) >=20 > 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) > =20 > #define SUN4I_I2S_FMT1_REG 0x08 > #define SUN4I_I2S_FIFO_TX_REG 0x0c > @@ -72,7 +74,7 @@ > #define SUN4I_I2S_INT_STA_REG 0x20 > =20 > #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 > =20 > #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) > =20 > #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)) > =20 > #define SUN4I_I2S_RX_CHAN_SEL_REG 0x38 > #define SUN4I_I2S_RX_CHAN_MAP_REG 0x3c > =20 > +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 { > =20 > 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; > }; > =20 > struct sun4i_i2s_clk_div { > @@ -138,7 +176,7 @@ static int sun4i_i2s_get_bclk_div(struct sun4i_i2s *i= 2s, > const struct sun4i_i2s_clk_div *bdiv =3D &sun4i_i2s_bclk_div[i]; > =20 > if (bdiv->div =3D=3D div) > - return bdiv->val; > + return bdiv->val + i2s->variant->bclk_adjust; > } > =20 > return -EINVAL; > @@ -156,7 +194,7 @@ static int sun4i_i2s_get_mclk_div(struct sun4i_i2s *i= 2s, > const struct sun4i_i2s_clk_div *mdiv =3D &sun4i_i2s_mclk_div[i]; > =20 > if (mdiv->div =3D=3D 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 --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --mlyqlexrsjlharpf Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBAgAGBQJZdty3AAoJEBx+YmzsjxAgLUgQALNkR5f3ns6Zmh3KH+edsfkf nMLEyIxskxKYWJ/oxAO4+Egb23HvjBjptq/gW+hX5effLWE5olLhmghgM3EbwFqP pT4DLfYvfneDN/MweGQplNb2+TiK6UzYtE5uQt+JEp0+3+rHp4qtV/VYAcuoEbn7 CYdmM0Tmo6O4wlPlRfG4q29WHXlDse6625DlcAo9H4RsDfit4KnAifKz87aK4CeE ZswN5eaOa3nQJmDf2/b9I1EsSx241F4mlCo5gkRqkREaOfUuLhhPjOKYmmuq/Z4A +SrVUt15s8eYk9cOWQCrkATFlp9s8/aOjdnB6PAu1HOQ8WVztaEgM96vDkZyCHui Yw4+ENHFy+t2y+TAVwuOzgOGQz/5TSf/lJLIe7wg98X+rEsC6CtGgTEPsgBfN4fW bs/HxIdg4iV4E0gXBaLTQtTwsQ2mH2NHMfdUf9aYVbbV63kvI/kCBQzUcXtDsVuM 1IGP42EuDdM2WHXaTyYUQ0akAOC0EFRF5SLSDYnbA6y4yWRlva9MAFgVD/7cJDve zweMXDzjmlKpSj2iZWQIaN3+Hx2g2dePjPrs0+iGy2BJmdwLnsm1HT5LXVM2oFNe Cun/NFYVgBKH0kXMU/0T93d6htVuM0y2cp75IgFnlOzIkUMZ8nPiZ9UemRMj4IeE o0Rf5djuQaUdD0GaQ1JA =cGmq -----END PGP SIGNATURE----- --mlyqlexrsjlharpf--