linux-kernel - Re: [PATCH] ASoC: rt5682: split i2c driver into separate module

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]

Message-ID: <20200527205658.GM5308@sirena.org.uk>
Date:   Wed, 27 May 2020 21:56:58 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Akshu Agrawal <akshu.agrawal@....com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>,
        Oder Chiou <oder_chiou@...ltek.com>,
        Cezary Rojewski <cezary.rojewski@...el.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        linux-kernel@...r.kernel.org, alsa-devel@...a-project.org
Subject: Re: [PATCH] ASoC: rt5682: split i2c driver into separate module

On Wed, May 27, 2020 at 09:34:13PM +0200, Arnd Bergmann wrote:

> index 000000000000..c891aa80757f
> --- /dev/null
> +++ b/sound/soc/codecs/rt5682-i2c.c
> @@ -0,0 +1,341 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * rt5682.c  --  RT5682 ALSA SoC audio component driver
> + *

Please keep the entire comment C++ so things look more intentional.

> +static const char *rt5682_supply_names[RT5682_NUM_SUPPLIES] = {
> +	"AVDD",
> +	"MICVDD",
> +	"VBAT",
> +};

I'm *fairly* sure the device needs power even with Soundwire?

> +static void rt5682_jd_check_handler(struct work_struct *work)
> +{
> +	struct rt5682_priv *rt5682 = container_of(work, struct rt5682_priv,
> +		jd_check_work.work);

> +static int rt5682_parse_dt(struct rt5682_priv *rt5682, struct device *dev)
> +{
> +
> +	device_property_read_u32(dev, "realtek,dmic1-data-pin",
> +		&rt5682->pdata.dmic1_data_pin);
> +	device_property_read_u32(dev, "realtek,dmic1-clk-pin",
> +		&rt5682->pdata.dmic1_clk_pin);

This doesn't look very I2C specific either, nor do chunks of the rest of
the code.  The usual pattern with this stuff is to have the bus specific
code do bus specific stuff like setting up the regmap and then call into
a common init function for the shared parts of the chip.  I'd expect a
bit more unshared code here but not this much.

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)