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

[Mark Brown]

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.

Attachment: signature.asc
Description: PGP signature