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

[Arnd Bergmann]

On Wed, May 27, 2020 at 10:57 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> 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.

That's the kind of change I was trying to avoid by just moving
things around, but I've done that now in both files.

> > +static const char *rt5682_supply_names[RT5682_NUM_SUPPLIES] = {
> > +     "AVDD",
> > +     "MICVDD",
> > +     "VBAT",
> > +};
>
> I'm *fairly* sure the device needs power even with Soundwire?

I have no idea how this is done with ACPI. I'm moving it
back as an exported symbol. There should probably be
some abstraction that handles this in the common part of
the driver along with some of the other bits of
rt5682_i2c_probe, but really don't want to shake things up
too much and would let this be done by whoever needs to add
DT support to the soundwire driver in the future and is able
to test the changes.

> > +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.

Right, I was surprised the soundwire portion does not tie into
device tree at all, and none of the other soundwire codecs seem
to either and no dts files reference it, though there is some code
for the subsystem and a binding.

     Arnd