A license mismatch is all I spotted.

On Tue, 2015-04-21 at 13:33 +0100, Richard Fitzgerald wrote:
> --- /dev/null
> +++ b/sound/soc/codecs/wm8998.c

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

This states the license is GPL v2.

> +MODULE_LICENSE("GPL");

And, according to include/linux/module.h, this states the license is GPL
v2 or later. So I think either the comment at the top of this file or
the license ident used in the MODULE_LICENSE() macro needs to change.

Paul Bolle

2015-04-23 02:14:54

by Brian Austin

[permalink] [raw]

Subject: Re: [alsa-devel] [PATCH 8/8] Documentation: Add WM8998/WM1814 device tree bindings

> On Apr 21, 2015, at 07:38, Richard Fitzgerald <[email protected]> wrote:
>
> Signed-off-by: Richard Fitzgerald <[email protected]>
> ---
> Documentation/devicetree/bindings/mfd/arizona.txt | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mfd/arizona.txt b/Documentation/devicetree/bindings/mfd/arizona.txt
> index 7665aa9..a3cb34b 100644
> --- a/Documentation/devicetree/bindings/mfd/arizona.txt
> +++ b/Documentation/devicetree/bindings/mfd/arizona.txt
> @@ -10,6 +10,9 @@ Required properties:
> "wlf,wm5110"
> "wlf,wm8280"
> "wlf,wm8997"
> + "wlf,WM8998"
> + "wlf,wm1814"
> +
Is there a reason for caps on the 8998?

-Brian -

2015-04-23 14:15:52

by Richard Fitzgerald

[permalink] [raw]

Subject: Re: [alsa-devel] [PATCH 6/8] extcon: arizona: Add support for WM8998 and WM1814

On Wed, Apr 22, 2015 at 07:20:09PM +0900, Chanwoo Choi wrote:
> On 04/22/2015 06:19 PM, Richard Fitzgerald wrote:
> > On Wed, Apr 22, 2015 at 02:53:42PM +0900, Chanwoo Choi wrote:
> >> Hi Richard,
> >>
> >>> @@ -1176,6 +1182,11 @@ static int arizona_extcon_probe(struct platform_device *pdev)
> >>> break;
> >>> }
> >>> break;
> >>> + case WM8998:
> >>> + case WM1814:
> >>> + info->micd_clamp = true;
> >>> + info->hpdet_ip = 2;
> >>
> >> What is meaning of '2'? I prefer to use the definition for '2'.
> >>
> >
> > '2' is the version number of the hpdet ip block in silicon. We're already using
> > it as a raw number '0', '1' or '2' all over extcon-arizona.c so changing it here
> > would mean making other patches to the file that aren't really part of adding
> > WM8998 support, so I'd prefer not to change that as a side-effect of adding WM8998.
>
> I think that just you can define following definitions and use HPDET_IP_VER_V2 instead of '2'.
>
> #define HPDET_IP_VER_V0 0
> #define HPDET_IP_VER_V1 1
> #define HPDET_IP_VER_V2 2
>

Can we deal with that as a separate patch from this series? Like I said,
the code already uses '0' '1' and '2' for the existing codecs so making a
change to use #define means patching the code for the other codecs. That
is not part of adding WM8998 support and I don't like patches that make
unexpected extra side-effect changes that are not relevant to the actual
functionality being added by the patch. It's specially annoying when
cherry-picking or reverting those patches if they included some extra
code change.

If we can get this series submitted I can look at making a later patch
to improve readbility, but since this really is just a version number I
think it would be enough to rename the variable to hpdet_ip_version rather
than effectively doing #define TWO 2

> _______________________________________________
> Alsa-devel mailing list
> [email protected]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

2015-04-23 14:28:31

by Richard Fitzgerald

[permalink] [raw]

Subject: Re: [PATCH 7/8] ASoC: wm8998: Initial WM8998 codec driver

On Wed, Apr 22, 2015 at 12:00:30PM +0100, Mark Brown wrote:
> On Tue, Apr 21, 2015 at 01:33:55PM +0100, Richard Fitzgerald wrote:
>
> > +static int wm8998_in1mux_ev(struct snd_soc_dapm_widget *w,
> > + struct snd_kcontrol *kcontrol,
> > + int event)
> > +{
> > + struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm);
> > + struct arizona *arizona = dev_get_drvdata(codec->dev->parent);
> > + unsigned int left_mux, right_mux, in1mode, old;
> > +
> > + switch (event) {
> > + case SND_SOC_DAPM_PRE_PMU:
> > + /* Validate the mux configuration */
> > + left_mux = snd_soc_read(codec, ARIZONA_ADC_DIGITAL_VOLUME_1L) &
> > + ARIZONA_IN1L_SRC_MASK;
> > + right_mux = snd_soc_read(codec, ARIZONA_ADC_DIGITAL_VOLUME_1R) &
> > + ARIZONA_IN1R_SRC_MASK;
> > +
> > + /* Only IN1A can be digital, IN1B is always analogue */
> > + in1mode = (arizona->pdata.inmode[0] & 2)
> > + << (ARIZONA_IN1_MODE_SHIFT - 1);
> > +
> > + if (in1mode != 0) {
> > + /* if IN1A is digital, the only valid mux configs
> > + * are both channels A or both channels B.
> > + */
> > + if (left_mux != right_mux) {
> > + dev_err(arizona->dev,
> > + "IN1=DMIC and IN1L Mux != IN1R Mux");
> > + return -EINVAL;
> > + }
>
> This (or at least some of it) seems like something we should be doing by
> registering different widgets and routing at device probe time.

I'm not sure this is really a routing or widget problem. Problem is that
if the muxed inputs are analogue we need to provide separate ALSA controls
for the left and right mux because it is allowed to use them as two
independant mono channels with separate muxing. But if they are
configured as digital the mux controls are ganged so must be set the
same. I don't feel good about registering a single mux control if its
digital and two if its analogue because that means the codec's control
list depends on a pdata option. I could implement the control pair
so that if the input is digital changing one control always changes
the other so they will always be forced to the same setting. Any
better options?

>
> > + old = snd_soc_read(codec, ARIZONA_IN1L_CONTROL) &
> > + ARIZONA_IN1_MODE_MASK;
>
> > + if (old != in1mode)
> > + snd_soc_update_bits(codec, ARIZONA_IN1L_CONTROL,
> > + ARIZONA_IN1_MODE_MASK, in1mode);
>
> Why bother with the read into old here? Suppressing redundant updates
> is part of the point of snd_soc_update_bits().

Agreed, probably a legacy of an earlier implementation that's now redundant.

2015-04-23 14:46:22

by Richard Fitzgerald

[permalink] [raw]

Subject: Re: [alsa-devel] [PATCH 8/8] Documentation: Add WM8998/WM1814 device tree bindings

On Thu, Apr 23, 2015 at 02:05:18AM +0000, Austin, Brian wrote:
>
>
> > On Apr 21, 2015, at 07:38, Richard Fitzgerald <[email protected]> wrote:
> >
> > Signed-off-by: Richard Fitzgerald <[email protected]>
> > ---
> > Documentation/devicetree/bindings/mfd/arizona.txt | 3 +++
> > 1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mfd/arizona.txt b/Documentation/devicetree/bindings/mfd/arizona.txt
> > index 7665aa9..a3cb34b 100644
> > --- a/Documentation/devicetree/bindings/mfd/arizona.txt
> > +++ b/Documentation/devicetree/bindings/mfd/arizona.txt
> > @@ -10,6 +10,9 @@ Required properties:
> > "wlf,wm5110"
> > "wlf,wm8280"
> > "wlf,wm8997"
> > + "wlf,WM8998"
> > + "wlf,wm1814"
> > +
> Is there a reason for caps on the 8998?
>
> -Brian

Yes, it's called "lack of attention to detail". Will be fixed in the respin.

> _______________________________________________
> Alsa-devel mailing list
> [email protected]
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

2015-04-23 17:55:12

On Tue, Apr 21, 2015 at 2:33 PM, Richard Fitzgerald
<[email protected]> wrote:

> Signed-off-by: Richard Fitzgerald <[email protected]>

Acked-by: Linus Walleij <[email protected]>

I guess this patch will go through the MFD tree?

Yours,
Linus Walleij