mbox series

[v3,0/9] ASoC: Mediatek: Add support for MT8192 SoC

Message ID 1603526339-15005-1-git-send-email-jiaxin.yu@mediatek.com
Headers show
Series ASoC: Mediatek: Add support for MT8192 SoC | expand

Message

Jiaxin Yu Oct. 24, 2020, 7:58 a.m. UTC
This series of patches adds support for Mediatek AFE for MT8192 SoC. At the same
time, the calibration function of MT6359 is completed with real machine driver.
The patch is based on broonie tree "for-next" branch.

Change since v2:
  - split the dai driver files as a seperate patch
  - fix dt-bindings to GPL-2.0-only License
  - remove unnecessary preperty descriptions such as 'maxItems'

Change since v1:
  - fixed some typos related to dt-bindings in [v1,3/5] and [v1,5/5]
  - add vendor prefix to the properties, such as "mediatek,apmixedsys"
  - add a dependency description to indicate the required header files

Jiaxin Yu (9):
  ASoC: mediatek: mt6359: add the calibration functions
  ASoC: mediatek: mt8192: add platform driver
  ASoC: mediatek: mt8192: support i2s in platform driver
  ASoC: mediatek: mt8192: support add in platform driver
  ASoC: mediatek: mt8192: support pcm in platform driver
  ASoC: mediatek: mt8192: support tdm in platform driver
  dt-bindings: mediatek: mt8192: add audio afe document
  ASoC: mediatek: mt8192: add machine driver with mt6359, rt1015 and
    rt5682
  dt-bindings: mediatek: mt8192: add mt8192-mt6358-rt1015-rt5682
    document

 .../bindings/sound/mt8192-afe-pcm.yaml        |  100 +
 .../sound/mt8192-mt6359-rt1015-rt5682.yaml    |   42 +
 sound/soc/codecs/mt6359.c                     |  110 +
 sound/soc/codecs/mt6359.h                     |    7 +
 sound/soc/mediatek/Kconfig                    |   23 +
 sound/soc/mediatek/Makefile                   |    1 +
 sound/soc/mediatek/common/mtk-afe-fe-dai.c    |   13 +-
 sound/soc/mediatek/common/mtk-base-afe.h      |    1 +
 sound/soc/mediatek/mt8192/Makefile            |   16 +
 sound/soc/mediatek/mt8192/mt8192-afe-clk.c    |  669 ++++
 sound/soc/mediatek/mt8192/mt8192-afe-clk.h    |  244 ++
 sound/soc/mediatek/mt8192/mt8192-afe-common.h |  170 +
 .../soc/mediatek/mt8192/mt8192-afe-control.c  |  163 +
 sound/soc/mediatek/mt8192/mt8192-afe-gpio.c   |  306 ++
 sound/soc/mediatek/mt8192/mt8192-afe-gpio.h   |   19 +
 sound/soc/mediatek/mt8192/mt8192-afe-pcm.c    | 2389 +++++++++++++
 sound/soc/mediatek/mt8192/mt8192-dai-adda.c   | 1489 ++++++++
 sound/soc/mediatek/mt8192/mt8192-dai-i2s.c    | 2139 +++++++++++
 sound/soc/mediatek/mt8192/mt8192-dai-pcm.c    |  409 +++
 sound/soc/mediatek/mt8192/mt8192-dai-tdm.c    |  778 ++++
 .../mediatek/mt8192/mt8192-interconnection.h  |   65 +
 .../mt8192/mt8192-mt6359-rt1015-rt5682.c      | 1058 ++++++
 sound/soc/mediatek/mt8192/mt8192-reg.h        | 3131 +++++++++++++++++
 23 files changed, 13338 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/mt8192-afe-pcm.yaml
 create mode 100644 Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
 create mode 100644 sound/soc/mediatek/mt8192/Makefile
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-clk.c
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-clk.h
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-common.h
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-control.c
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-gpio.c
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-gpio.h
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-afe-pcm.c
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-dai-adda.c
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-dai-i2s.c
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-dai-pcm.c
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-dai-tdm.c
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-interconnection.h
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-mt6359-rt1015-rt5682.c
 create mode 100644 sound/soc/mediatek/mt8192/mt8192-reg.h

Comments

Mark Brown Oct. 30, 2020, 2:37 p.m. UTC | #1
On Sat, Oct 24, 2020 at 03:58:53PM +0800, Jiaxin Yu wrote:

> +static const struct soc_enum mt8192_i2s_enum[] = {
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mt8192_i2s_hd_str),
> +			    mt8192_i2s_hd_str),
> +};

Why is this declared as a single element array?  It just makes all the
usages look odd for no obvious gain.

> +static int mtk_i2s_en_event(struct snd_soc_dapm_widget *w,
> +			    struct snd_kcontrol *kcontrol,
> +			    int event)

> +	dev_info(cmpnt->dev, "%s(), name %s, event 0x%x\n",
> +		 __func__, w->name, event);

This should be dev_dbg() at most, _info() will be too noisy in the logs.
Same for a lot of functions, including the stream callbacks.

> +static int mtk_i2s_hd_en_event(struct snd_soc_dapm_widget *w,
> +			       struct snd_kcontrol *kcontrol,
> +			       int event)
> +{
> +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> +
> +	dev_info(cmpnt->dev, "%s(), name %s, event 0x%x\n",
> +		 __func__, w->name, event);
> +
> +	return 0;
> +}

This should just be removed entirely, there's trace in the core if you
need logging in production systems - the tracepoints in particular are
good for just leaving on all the time without adding overhead.

> +	return (i2s_need_apll == cur_apll) ? 1 : 0;

Please write normal conditional statements to improve legiblity.

> +	if (rate == 44100)
> +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x001B9000);
> +	else if (rate == 32000)
> +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x140000);
> +	else
> +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x001E0000);

This would be better written as a switch statement.

> +	/* Calibration setting */
> +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON4, 0x00140000);
> +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON9, 0x00036000);
> +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON10, 0x0002FC00);
> +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON6, 0x00007EF4);
> +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON5, 0x00FF5986);

Are you sure this isn't system dependant?
Mark Brown Oct. 30, 2020, 3:13 p.m. UTC | #2
On Sat, Oct 24, 2020 at 03:58:54PM +0800, Jiaxin Yu wrote:

> +/* mtkaif dmic */
> +static const char * const mt8192_adda_off_on_str[] = {
> +	"Off", "On"
> +};
> +
> +static const struct soc_enum mt8192_adda_enum[] = {
> +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mt8192_adda_off_on_str),
> +			    mt8192_adda_off_on_str),
> +};

This should be a standard boolean control with a name ending in Switch
rather than an enum.
Mark Brown Oct. 30, 2020, 5:54 p.m. UTC | #3
On Sat, Oct 24, 2020 at 03:58:50PM +0800, Jiaxin Yu wrote:
> This series of patches adds support for Mediatek AFE for MT8192 SoC. At the same
> time, the calibration function of MT6359 is completed with real machine driver.
> The patch is based on broonie tree "for-next" branch.

I had some small comments but they were all pretty minor - overall this
is basically fine once those are addressed.
Jiaxin Yu Nov. 4, 2020, 6:50 a.m. UTC | #4
On Fri, 2020-10-30 at 14:37 +0000, Mark Brown wrote:
> On Sat, Oct 24, 2020 at 03:58:53PM +0800, Jiaxin Yu wrote:
> 
> > +static const struct soc_enum mt8192_i2s_enum[] = {
> > +	SOC_ENUM_SINGLE_EXT(ARRAY_SIZE(mt8192_i2s_hd_str),
> > +			    mt8192_i2s_hd_str),
> > +};
> 
> Why is this declared as a single element array?  It just makes all the
> usages look odd for no obvious gain.
> 
> > +static int mtk_i2s_en_event(struct snd_soc_dapm_widget *w,
> > +			    struct snd_kcontrol *kcontrol,
> > +			    int event)
> 
> > +	dev_info(cmpnt->dev, "%s(), name %s, event 0x%x\n",
> > +		 __func__, w->name, event);
> 
> This should be dev_dbg() at most, _info() will be too noisy in the logs.
> Same for a lot of functions, including the stream callbacks.
> 
> > +static int mtk_i2s_hd_en_event(struct snd_soc_dapm_widget *w,
> > +			       struct snd_kcontrol *kcontrol,
> > +			       int event)
> > +{
> > +	struct snd_soc_component *cmpnt = snd_soc_dapm_to_component(w->dapm);
> > +
> > +	dev_info(cmpnt->dev, "%s(), name %s, event 0x%x\n",
> > +		 __func__, w->name, event);
> > +
> > +	return 0;
> > +}
> 
> This should just be removed entirely, there's trace in the core if you
> need logging in production systems - the tracepoints in particular are
> good for just leaving on all the time without adding overhead.
> 
> > +	return (i2s_need_apll == cur_apll) ? 1 : 0;
> 
> Please write normal conditional statements to improve legiblity.
> 
> > +	if (rate == 44100)
> > +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x001B9000);
> > +	else if (rate == 32000)
> > +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x140000);
> > +	else
> > +		regmap_write(afe->regmap, AFE_ASRC_2CH_CON3, 0x001E0000);
> 
> This would be better written as a switch statement.
> 
> > +	/* Calibration setting */
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON4, 0x00140000);
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON9, 0x00036000);
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON10, 0x0002FC00);
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON6, 0x00007EF4);
> > +	regmap_write(afe->regmap, AFE_ASRC_2CH_CON5, 0x00FF5986);
> 
> Are you sure this isn't system dependant?
Hi Mark,
Yes, this is a system independent setting. And I fixed other comments
you pointed out then send "PATCH v4".