LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Maxime Ripard <maxime@cerno.tech>
To: 班涛 <fengzheng923@gmail.com>
Cc: lgirdwood@gmail.com, Mark Brown <broonie@kernel.org>,
	perex@perex.cz, tiwai@suse.com, wens@csie.org,
	jernej.skrabec@gmail.com, p.zabel@pengutronix.de,
	Samuel Holland <samuel@sholland.org>,
	krzk@kernel.org, linux-kernel@vger.kernel.org,
	alsa-devel@alsa-project.org,
	linux-arm-kernel@lists.infradead.org,
	linux-sunxi@lists.linux.dev
Subject: Re: [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver
Date: Tue, 17 Aug 2021 09:12:20 +0200	[thread overview]
Message-ID: <20210817071220.idoxpwzbpemdjqdz@gilmour> (raw)
In-Reply-To: <CAE=m61_=XfhtG9Q1r34McWWCUXt1KP67cjZ0ER62+YaGrG+b4w@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 3519 bytes --]

Hi,

On Sun, Aug 01, 2021 at 05:47:46PM +0800, 班涛 wrote:
> > > +     /* DMIC num is N+1 */
> > > +     regmap_update_bits(host->regmap, SUN50I_DMIC_CH_NUM,
> > > +                        SUN50I_DMIC_CH_NUM_N_MASK,
> > > +                        SUN50I_DMIC_CH_NUM_N(channels - 1));
> > > +     host->chan_en = (1 << channels) - 1;
> > > +     regmap_write(host->regmap, SUN50I_DMIC_HPF_CTRL, host->chan_en);
> >
> > Do we need to store the channels bitmask in the main structure? It looks
> > fairly easy to generate, so I guess we could just use a macro
> 
> I need to store channels bitmask and use it in sun50i_dmic_trigger function.

But you don't need it outside of hw_params. The channel setup is
typically done in hw_params, not in the trigger hook.

[...]

> > > +     /* Clocks */
> > > +     host->bus_clk = devm_clk_get(&pdev->dev, "bus");
> > > +     if (IS_ERR(host->bus_clk))
> > > +             return dev_err_probe(&pdev->dev, PTR_ERR(host->bus_clk),
> > > +                                  "failed to get bus clock.\n");
> > > +
> > > +     host->dmic_clk = devm_clk_get(&pdev->dev, "mod");
> > > +     if (IS_ERR(host->dmic_clk))
> > > +             return dev_err_probe(&pdev->dev, PTR_ERR(host->dmic_clk),
> > > +                                  "failed to get dmic clock.\n");
> > > +
> > > +     host->dma_params_rx.addr = res->start + SUN50I_DMIC_DATA;
> > > +     host->dma_params_rx.maxburst = 8;
> > > +
> > > +     platform_set_drvdata(pdev, host);
> > > +
> > > +     host->rst = devm_reset_control_get_optional_exclusive(&pdev->dev, NULL);
> > > +     if (IS_ERR(host->rst))
> > > +             return dev_err_probe(&pdev->dev, PTR_ERR(host->rst),
> > > +                                  "Failed to get reset.\n");
> >
> > Your binding states that the reset is mandatory so you don't need the
> > optional variant.
> >
> > > +     reset_control_deassert(host->rst);
> >
> > Can't this be moved to the runtime_pm hooks?
> 
> Is this necessary? I see that most of the driver files execute
> reset_control_deassert in the probe function, and reset_control_assert
> in the remove function.

Your driver seems to not rely on the fact that the device remains
powered between each run anyway, so yeah, that way you will completely
power it down.

> >
> > > +     ret = devm_snd_soc_register_component(&pdev->dev,
> > > +                             &sun50i_dmic_component, &sun50i_dmic_dai, 1);
> >
> > Your second line should be aligned on the opening parenthesis
> >
> > > +     if (ret)
> > > +             return dev_err_probe(&pdev->dev, ret,
> > > +                                  "failed to register component.\n");
> > > +
> > > +     pm_runtime_enable(&pdev->dev);
> > > +     if (!pm_runtime_enabled(&pdev->dev)) {
> > > +             ret = sun50i_dmic_runtime_resume(&pdev->dev);
> > > +             if (ret)
> > > +                     goto err_unregister;
> > > +     }
> >
> > We have a depends on PM on some drivers already, so I guess it would
> > just make sense to add one more here instead of dealing with whether
> > runtime_pm is compiled in or not.
> 
> I don't understand. I am referring to the sun4i-spdif.c file. Which
> driver files should I refer to?

That whole logic is there to enable the device is CONFIG_PM is not
enabled (and thus the calls to pm_runtime_* won't do anything).

Just add a depends on PM to your Kconfig entry, and call
pm_runtime_enable there.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

      reply	other threads:[~2021-08-17  7:12 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-11 12:20 fengzheng923
2021-07-15  7:47 ` Maxime Ripard
2021-08-01  9:47   ` 班涛
2021-08-17  7:12     ` Maxime Ripard [this message]

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=20210817071220.idoxpwzbpemdjqdz@gilmour \
    --to=maxime@cerno.tech \
    --cc=alsa-devel@alsa-project.org \
    --cc=broonie@kernel.org \
    --cc=fengzheng923@gmail.com \
    --cc=jernej.skrabec@gmail.com \
    --cc=krzk@kernel.org \
    --cc=lgirdwood@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sunxi@lists.linux.dev \
    --cc=p.zabel@pengutronix.de \
    --cc=perex@perex.cz \
    --cc=samuel@sholland.org \
    --cc=tiwai@suse.com \
    --cc=wens@csie.org \
    --subject='Re: [PATCH v6 1/2] ASoC: sunxi: Add Allwinner H6 Digital MIC driver' \
    /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

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox