All of lore.kernel.org
 help / color / mirror / Atom feed
From: Chen-Yu Tsai <wens@csie.org>
To: Code Kipper <codekipper@gmail.com>
Cc: Maxime Ripard <maxime.ripard@free-electrons.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
	linux-sunxi <linux-sunxi@googlegroups.com>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Linux-ALSA <alsa-devel@alsa-project.org>,
	"Andrea Venturi (pers)" <be17068@iperbole.bo.it>
Subject: Re: [linux-sunxi] [PATCH v3 04/11] ASoC: sun4i-i2s: Add regmap fields for channels
Date: Sat, 12 Aug 2017 20:08:55 +0800	[thread overview]
Message-ID: <CAGb2v66z9D-Y-wzWeNdUEkxOQ9RDZJ_3CVK2ao4n66mq9DfEXQ@mail.gmail.com> (raw)
In-Reply-To: <20170812110059.5115-5-codekipper@gmail.com>

On Sat, Aug 12, 2017 at 7:00 PM,  <codekipper@gmail.com> wrote:
> From: Marcus Cooper <codekipper@gmail.com>
>
> 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 <codekipper@gmail.com>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  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 dfb794ffff92..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;

[...]

WARNING: multiple messages have this Message-ID (diff)
From: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
To: Code Kipper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Maxime Ripard
	<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>,
	linux-arm-kernel
	<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
	linux-sunxi <linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org>,
	Liam Girdwood <lgirdwood-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	linux-kernel
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Linux-ALSA <alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org>,
	"Andrea Venturi (pers)"
	<be17068-p0aYb1w59bq9tCD/VL7h6Q@public.gmane.org>
Subject: Re: [PATCH v3 04/11] ASoC: sun4i-i2s: Add regmap fields for channels
Date: Sat, 12 Aug 2017 20:08:55 +0800	[thread overview]
Message-ID: <CAGb2v66z9D-Y-wzWeNdUEkxOQ9RDZJ_3CVK2ao4n66mq9DfEXQ@mail.gmail.com> (raw)
In-Reply-To: <20170812110059.5115-5-codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On Sat, Aug 12, 2017 at 7:00 PM,  <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From: Marcus Cooper <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> 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 <codekipper-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
> ---
>  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 dfb794ffff92..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;

[...]

WARNING: multiple messages have this Message-ID (diff)
From: wens@csie.org (Chen-Yu Tsai)
To: linux-arm-kernel@lists.infradead.org
Subject: [linux-sunxi] [PATCH v3 04/11] ASoC: sun4i-i2s: Add regmap fields for channels
Date: Sat, 12 Aug 2017 20:08:55 +0800	[thread overview]
Message-ID: <CAGb2v66z9D-Y-wzWeNdUEkxOQ9RDZJ_3CVK2ao4n66mq9DfEXQ@mail.gmail.com> (raw)
In-Reply-To: <20170812110059.5115-5-codekipper@gmail.com>

On Sat, Aug 12, 2017 at 7:00 PM,  <codekipper@gmail.com> wrote:
> From: Marcus Cooper <codekipper@gmail.com>
>
> 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 <codekipper@gmail.com>
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  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 dfb794ffff92..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;

[...]

  reply	other threads:[~2017-08-12 12:09 UTC|newest]

Thread overview: 49+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-12 11:00 [PATCH v3 00/11] ASoC: Add I2S support for Allwinner H3 SoCs codekipper
2017-08-12 11:00 ` codekipper at gmail.com
2017-08-12 11:00 ` codekipper-Re5JQEeQqe8AvxtiuMwx3w
2017-08-12 11:00 ` [PATCH v3 01/11] ASoC: sun4i-i2s: Add clkdiv offsets to quirks codekipper
2017-08-12 11:00   ` codekipper at gmail.com
2017-08-12 11:00   ` codekipper-Re5JQEeQqe8AvxtiuMwx3w
2017-08-12 11:44   ` [linux-sunxi] " Chen-Yu Tsai
2017-08-12 11:44     ` Chen-Yu Tsai
2017-08-12 11:44     ` Chen-Yu Tsai
2017-08-14 16:43   ` Applied "ASoC: sun4i-i2s: Add clkdiv offsets to quirks" to the asoc tree Mark Brown
2017-08-14 16:43     ` Mark Brown
2017-08-14 16:43     ` Mark Brown
2017-08-12 11:00 ` [PATCH v3 02/11] ASoC: sun4i-i2s: Add regmap config to quirks codekipper
2017-08-12 11:00   ` codekipper at gmail.com
2017-08-12 11:00   ` codekipper-Re5JQEeQqe8AvxtiuMwx3w
2017-08-12 11:00 ` [PATCH v3 03/11] ASoC: sun4i-i2s: Add TX FIFO offset " codekipper
2017-08-12 11:00   ` codekipper at gmail.com
2017-08-12 11:00   ` codekipper-Re5JQEeQqe8AvxtiuMwx3w
2017-08-12 11:00 ` [PATCH v3 04/11] ASoC: sun4i-i2s: Add regmap fields for channels codekipper
2017-08-12 11:00   ` codekipper at gmail.com
2017-08-12 11:00   ` codekipper-Re5JQEeQqe8AvxtiuMwx3w
2017-08-12 12:08   ` Chen-Yu Tsai [this message]
2017-08-12 12:08     ` [linux-sunxi] " Chen-Yu Tsai
2017-08-12 12:08     ` Chen-Yu Tsai
2017-08-12 11:00 ` [PATCH v3 05/11] ASoC: sun4i-i2s: Add regfields for word size select and sample resolution codekipper
2017-08-12 11:00   ` codekipper at gmail.com
2017-08-12 11:00 ` [PATCH v3 06/11] ASoC: sun4i-i2s: bclk and lrclk polarity tidyup codekipper
2017-08-12 11:00   ` codekipper at gmail.com
2017-08-12 11:00   ` codekipper-Re5JQEeQqe8AvxtiuMwx3w
2017-08-12 11:00 ` [PATCH v3 07/11] ASoC: sun4i-i2s: Add mclk enable regmap field codekipper
2017-08-12 11:00   ` codekipper at gmail.com
2017-08-12 11:00   ` codekipper-Re5JQEeQqe8AvxtiuMwx3w
2017-08-12 11:00 ` [PATCH v3 08/11] ASoC: sun4i-i2s: Add regmap field to set DAI format codekipper
2017-08-12 11:00   ` codekipper at gmail.com
2017-08-12 11:00   ` codekipper-Re5JQEeQqe8AvxtiuMwx3w
2017-08-12 11:00 ` [PATCH v3 09/11] ASoC: sun4i-i2s: Check for slave select bit codekipper
2017-08-12 11:00   ` codekipper at gmail.com
2017-08-12 11:00   ` codekipper-Re5JQEeQqe8AvxtiuMwx3w
2017-08-12 11:00 ` [PATCH v3 10/11] ASoC: sun4i-i2s: Update global enable with bitmask codekipper
2017-08-12 11:00   ` codekipper at gmail.com
2017-08-12 11:00   ` codekipper-Re5JQEeQqe8AvxtiuMwx3w
2017-08-12 11:00 ` [PATCH v3 11/11] ASoC: sun4i-i2s: Add support for H3 codekipper
2017-08-12 11:00   ` codekipper at gmail.com
2017-08-12 11:00   ` codekipper-Re5JQEeQqe8AvxtiuMwx3w
2017-08-12 12:27   ` [linux-sunxi] " Chen-Yu Tsai
2017-08-12 12:27     ` Chen-Yu Tsai
2017-08-12 12:27     ` Chen-Yu Tsai
2017-08-15  7:23     ` Code Kipper
2017-08-15  7:23       ` Code Kipper

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGb2v66z9D-Y-wzWeNdUEkxOQ9RDZJ_3CVK2ao4n66mq9DfEXQ@mail.gmail.com \
    --to=wens@csie.org \
    --cc=alsa-devel@alsa-project.org \
    --cc=be17068@iperbole.bo.it \
    --cc=broonie@kernel.org \
    --cc=codekipper@gmail.com \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sunxi@googlegroups.com \
    --cc=maxime.ripard@free-electrons.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.