linux-kernel - RE: [PATCH] ASoC: max98373: Added Amplifier Driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 25 Dec 2017 14:30:23 +0000
From:   Ryan Lee <RyanS.Lee@...imintegrated.com>
To:     Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>
CC:     "lgirdwood@...il.com" <lgirdwood@...il.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "perex@...ex.cz" <perex@...ex.cz>,
        "tiwai@...e.com" <tiwai@...e.com>, "arnd@...db.de" <arnd@...db.de>,
        "afd@...com" <afd@...com>,
        "robert.jarzmik@...e.fr" <robert.jarzmik@...e.fr>,
        "supercraig0719@...il.com" <supercraig0719@...il.com>,
        "jbrunet@...libre.com" <jbrunet@...libre.com>,
        "dannenberg@...com" <dannenberg@...com>,
        "romain.perier@...labora.com" <romain.perier@...labora.com>,
        "bryce.ferguson@...kwellcollins.com" 
        <bryce.ferguson@...kwellcollins.com>,
        "m-stecklein@...com" <m-stecklein@...com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "ryan.lee.maxim@...il.com" <ryan.lee.maxim@...il.com>
Subject: RE: [PATCH] ASoC: max98373: Added Amplifier Driver

Hi Kuninori Miromoto,

>-----Original Message-----
>From: Kuninori Morimoto [mailto:kuninori.morimoto.gx@...esas.com]
>Sent: Thursday, December 21, 2017 6:24 PM
>To: Ryan Lee <RyanS.Lee@...imintegrated.com>
>Cc: lgirdwood@...il.com; broonie@...nel.org; robh+dt@...nel.org;
>mark.rutland@....com; perex@...ex.cz; tiwai@...e.com; arnd@...db.de;
>afd@...com; robert.jarzmik@...e.fr; supercraig0719@...il.com;
>jbrunet@...libre.com; dannenberg@...com; romain.perier@...labora.com;
>bryce.ferguson@...kwellcollins.com; m-stecklein@...com; alsa-devel@...a-
>project.org; devicetree@...r.kernel.org; linux-kernel@...r.kernel.org;
>ryan.lee.maxim@...il.com
>Subject: Re: [PATCH] ASoC: max98373: Added Amplifier Driver
>
>EXTERNAL EMAIL
>
>
>
>Hi Ryan
>
>> Signed-off-by: Ryan Lee <ryans.lee@...imintegrated.com>
>> ---
>>
>> Created max98373 amplifier driver.
>>
>>  .../devicetree/bindings/sound/max98373.txt         |  43 +
>>  sound/soc/codecs/Kconfig                           |   5 +
>>  sound/soc/codecs/Makefile                          |   2 +
>>  sound/soc/codecs/max98373.c                        | 996
>+++++++++++++++++++++
>>  sound/soc/codecs/max98373.h                        | 225 +++++
>>  5 files changed, 1271 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/sound/max98373.txt
>>  create mode 100644 sound/soc/codecs/max98373.c  create mode 100644
>> sound/soc/codecs/max98373.h
>(snip)
>> +struct max98373_priv {
>> +     struct regmap *regmap;
>> +     struct snd_soc_codec *codec;
>> +     unsigned int sysclk;
>> +     unsigned int v_slot;
>> +     unsigned int i_slot;
>> +     unsigned int spkfb_slot;
>> +     bool interleave_mode;
>> +     unsigned int ch_size;
>> +     unsigned int iface;
>> +     bool tdm_mode;
>> +};
>
>About this max98373->codec.
>This user is only max98373_set_clock(), and it is called from
>max98373_dai_hw_params().
>You are getting *codec from dai->codec in this function, and max98373 is
>came from it.
>This means, we can remove max98373->codec ?

Thanks for your feedback.
I will remove max98373->codec and change related things.

>
>About max98373->regmap.
>You are using devm_regmap_init_i2c(), and keeping it on max98373.
>Can you check snd_soc_component_add() which is called from
>snd_soc_add_component().
>It will set component->regmap if you called devm_regmap_init_i2c().
>I think you can use snd_soc_component_read/write instead of
>regmap_read/write.
>Then, we can remove max98373->regmap too ?
>Ahh, you want to check Revision ID.
>then, snd_soc_component_init_regmap() can help you ?

I'm sorry but I don't fully understand the benefit of this.
Keeping regmap information in private driver data is very common and I can see many ASoC drivers are using it.
I was able to see only a few driver use ' snd_soc_component_read'.
I would like to keep regmap_read/write if it is still acceptable.

>
>Best regards
>---
>Kuninori Morimoto