mbox series

[v3,0/5] MFD/ASoC: Add support for Intel Bay Trail boards with WM5102 codec

Message ID 20210117212252.206115-1-hdegoede@redhat.com
Headers show
Series MFD/ASoC: Add support for Intel Bay Trail boards with WM5102 codec | expand

Message

Hans de Goede Jan. 17, 2021, 9:22 p.m. UTC 
Hi All,

Here is v3 of my series to add support for Intel Bay Trail based devices
which use a WM5102 codec for audio output/input. New in v3 is that the
compile error when CONFIG_ACPI is unset should be gone. Everything else
is the same as in v2, see the cover-letter of v2 below:

This was developed and tested on a Lenovo Yoga Tablet 1051L.

The biggest difference from v1 is that, based on all the discussions about
the jack-detect stuff, I've decided to split this into 2 series.
One series adding the basic support for this setup, which is this series.

And a second series which reworks the extcon driver into an arizona-jackdet
library and then modifies the codec drivers to use that directly, replacing
the old separate extcon child-device and extcon-driver.

The are 2 reasons for the split:

1. With the new jack-det rework, the series really address 2 separate
(but related) enhancements.

2. I expect this series to be ready for merging, while the jack-det stuff
likely will need a couple more revisions.

Other then the split there are some minor changes addressing various review
comments, see the individual patch changelogs.

The MFD and ASoC parts can be merged independent from each-other (both must
be merged to get working sound on these boards, but that is only a runtime
dependency and a part missing won't have any bad side-effects). Or the
entire series could be merged through the MFD tree if people prefer that.

Regards,

Hans

Comments

Andy Shevchenko Jan. 18, 2021, 11:51 a.m. UTC | #1 
On Sun, Jan 17, 2021 at 11:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Replace the custom arizona_of_get_type() function with the generic
> device_get_match_data() helper. Besides being a nice cleanup this
> also makes it easier to add support for binding to ACPI enumerated
> devices.
>
> While at it also fix a possible NULL pointer deref of the id
> argument to the probe functions (this could happen on e.g. manual
> driver binding through sysfs).

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> - New patch in v2 of this patchset
> ---
>  drivers/mfd/arizona-core.c | 11 -----------
>  drivers/mfd/arizona-i2c.c  | 10 ++++++----
>  drivers/mfd/arizona-spi.c  | 10 ++++++----
>  drivers/mfd/arizona.h      |  9 ---------
>  4 files changed, 12 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 000cb82023e3..75f1bc671d59 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -797,17 +797,6 @@ const struct dev_pm_ops arizona_pm_ops = {
>  EXPORT_SYMBOL_GPL(arizona_pm_ops);
>
>  #ifdef CONFIG_OF
> -unsigned long arizona_of_get_type(struct device *dev)
> -{
> -       const struct of_device_id *id = of_match_device(arizona_of_match, dev);
> -
> -       if (id)
> -               return (unsigned long)id->data;
> -       else
> -               return 0;
> -}
> -EXPORT_SYMBOL_GPL(arizona_of_get_type);
> -
>  static int arizona_of_get_core_pdata(struct arizona *arizona)
>  {
>         struct arizona_pdata *pdata = &arizona->pdata;
> diff --git a/drivers/mfd/arizona-i2c.c b/drivers/mfd/arizona-i2c.c
> index 2a4a3a164d0a..5e83b730c4ce 100644
> --- a/drivers/mfd/arizona-i2c.c
> +++ b/drivers/mfd/arizona-i2c.c
> @@ -23,14 +23,16 @@
>  static int arizona_i2c_probe(struct i2c_client *i2c,
>                              const struct i2c_device_id *id)
>  {
> +       const void *match_data;
>         struct arizona *arizona;
>         const struct regmap_config *regmap_config = NULL;
> -       unsigned long type;
> +       unsigned long type = 0;
>         int ret;
>
> -       if (i2c->dev.of_node)
> -               type = arizona_of_get_type(&i2c->dev);
> -       else
> +       match_data = device_get_match_data(&i2c->dev);
> +       if (match_data)
> +               type = (unsigned long)match_data;
> +       else if (id)
>                 type = id->driver_data;
>
>         switch (type) {
> diff --git a/drivers/mfd/arizona-spi.c b/drivers/mfd/arizona-spi.c
> index 704f214d2614..798b88295c77 100644
> --- a/drivers/mfd/arizona-spi.c
> +++ b/drivers/mfd/arizona-spi.c
> @@ -23,14 +23,16 @@
>  static int arizona_spi_probe(struct spi_device *spi)
>  {
>         const struct spi_device_id *id = spi_get_device_id(spi);
> +       const void *match_data;
>         struct arizona *arizona;
>         const struct regmap_config *regmap_config = NULL;
> -       unsigned long type;
> +       unsigned long type = 0;
>         int ret;
>
> -       if (spi->dev.of_node)
> -               type = arizona_of_get_type(&spi->dev);
> -       else
> +       match_data = device_get_match_data(&spi->dev);
> +       if (match_data)
> +               type = (unsigned long)match_data;
> +       else if (id)
>                 type = id->driver_data;
>
>         switch (type) {
> diff --git a/drivers/mfd/arizona.h b/drivers/mfd/arizona.h
> index 995efc6d7f32..801cbbcd71cb 100644
> --- a/drivers/mfd/arizona.h
> +++ b/drivers/mfd/arizona.h
> @@ -50,13 +50,4 @@ int arizona_dev_exit(struct arizona *arizona);
>  int arizona_irq_init(struct arizona *arizona);
>  int arizona_irq_exit(struct arizona *arizona);
>
> -#ifdef CONFIG_OF
> -unsigned long arizona_of_get_type(struct device *dev);
> -#else
> -static inline unsigned long arizona_of_get_type(struct device *dev)
> -{
> -       return 0;
> -}
> -#endif
> -
>  #endif
> --
> 2.28.0
>
Mark Brown Jan. 18, 2021, 1:02 p.m. UTC | #2 
On Sun, Jan 17, 2021 at 10:22:50PM +0100, Hans de Goede wrote:

> +	/*
> +	 * Some DSDTs wrongly declare the IRQ trigger-type as IRQF_TRIGGER_FALLING
> +	 * The IRQ line will stay low when a new IRQ event happens between reading
> +	 * the IRQ status flags and acknowledging them. When the IRQ line stays
> +	 * low like this the IRQ will never trigger again when its type is set
> +	 * to IRQF_TRIGGER_FALLING. Correct the IRQ trigger-type to fix this.
> +	 */
> +	arizona->pdata.irq_flags = IRQF_TRIGGER_LOW;

Are you sure that all the relevant interrupt controllers support active
low interrupts?  There were issues on some systems with missing support
for active low interrupts (see the bodge in wm8994-irq.c to work around
them) - it's entirely likely that there are DSDTs that are just plain
buggy here but if someone's copying and pasting it smells like there may
be some systems that actually need an edge triggered interrupt that
they're getting it from.

> +
> +	/* Wait 200 ms after jack insertion */
> +	arizona->pdata.micd_detect_debounce = 200;
> +
> +	/* Use standard AOSP values for headset-button mappings */
> +	arizona->pdata.micd_ranges = arizona_micd_aosp_ranges;
> +	arizona->pdata.num_micd_ranges = ARRAY_SIZE(arizona_micd_aosp_ranges);
> +
> +	return 0;
> +}
> +
> +static const struct acpi_device_id arizona_acpi_match[] = {
> +	{
> +		.id = "WM510204",
> +		.driver_data = WM5102,
> +	},
> +	{
> +		.id = "WM510205",
> +		.driver_data = WM5102,
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, arizona_acpi_match);
> +#else
> +static int arizona_spi_acpi_probe(struct arizona *arizona)
> +{
> +	return -ENODEV;
> +}
> +#endif
> +
>  static int arizona_spi_probe(struct spi_device *spi)
>  {
>  	const struct spi_device_id *id = spi_get_device_id(spi);
> @@ -77,6 +191,12 @@ static int arizona_spi_probe(struct spi_device *spi)
>  	arizona->dev = &spi->dev;
>  	arizona->irq = spi->irq;
>  
> +	if (has_acpi_companion(&spi->dev)) {
> +		ret = arizona_spi_acpi_probe(arizona);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return arizona_dev_init(arizona);
>  }
>  
> @@ -104,6 +224,7 @@ static struct spi_driver arizona_spi_driver = {
>  		.name	= "arizona",
>  		.pm	= &arizona_pm_ops,
>  		.of_match_table	= of_match_ptr(arizona_of_match),
> +		.acpi_match_table = ACPI_PTR(arizona_acpi_match),
>  	},
>  	.probe		= arizona_spi_probe,
>  	.remove		= arizona_spi_remove,
> -- 
> 2.28.0
>
Hans de Goede Jan. 18, 2021, 1:13 p.m. UTC | #3 
Hi,

On 1/18/21 2:02 PM, Mark Brown wrote:
> On Sun, Jan 17, 2021 at 10:22:50PM +0100, Hans de Goede wrote:
> 
>> +	/*
>> +	 * Some DSDTs wrongly declare the IRQ trigger-type as IRQF_TRIGGER_FALLING
>> +	 * The IRQ line will stay low when a new IRQ event happens between reading
>> +	 * the IRQ status flags and acknowledging them. When the IRQ line stays
>> +	 * low like this the IRQ will never trigger again when its type is set
>> +	 * to IRQF_TRIGGER_FALLING. Correct the IRQ trigger-type to fix this.
>> +	 */
>> +	arizona->pdata.irq_flags = IRQF_TRIGGER_LOW;
> 
> Are you sure that all the relevant interrupt controllers support active
> low interrupts?  There were issues on some systems with missing support
> for active low interrupts (see the bodge in wm8994-irq.c to work around
> them) - it's entirely likely that there are DSDTs that are just plain
> buggy here but if someone's copying and pasting it smells like there may
> be some systems that actually need an edge triggered interrupt that
> they're getting it from.

I'm only aware of one series of devices / models which actually
use the combination of ACPI enumeration and the WM5102 codec, and that
is the Lenovo Yoga Tablet 2 series (in 8, 10 and 13 inch versions
shipping with both Windows and Android). These all use a Bay Trail
SoC which is capable of using active low interrupts.

More in general I'm not aware of any (recent-ish) x86 GPIO controllers
not being able to do active low interrupts. In theory we could hit this
code path on ARM devices using ACPI enumeration, but I don't think it
is likely we will see a combination of ARM + ACPI enumeration +
WM5102 + GPIO controller not capable of active-low interrupts.

This overriding of the flags definitely is necessary on the Lenovo
devices in question. I could add a
"if (dmi_name_in_vendors("LENOVO"))" guard around it, but that
seems unnecessary.

Regards,

Hans



> 
>> +
>> +	/* Wait 200 ms after jack insertion */
>> +	arizona->pdata.micd_detect_debounce = 200;
>> +
>> +	/* Use standard AOSP values for headset-button mappings */
>> +	arizona->pdata.micd_ranges = arizona_micd_aosp_ranges;
>> +	arizona->pdata.num_micd_ranges = ARRAY_SIZE(arizona_micd_aosp_ranges);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct acpi_device_id arizona_acpi_match[] = {
>> +	{
>> +		.id = "WM510204",
>> +		.driver_data = WM5102,
>> +	},
>> +	{
>> +		.id = "WM510205",
>> +		.driver_data = WM5102,
>> +	},
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, arizona_acpi_match);
>> +#else
>> +static int arizona_spi_acpi_probe(struct arizona *arizona)
>> +{
>> +	return -ENODEV;
>> +}
>> +#endif
>> +
>>  static int arizona_spi_probe(struct spi_device *spi)
>>  {
>>  	const struct spi_device_id *id = spi_get_device_id(spi);
>> @@ -77,6 +191,12 @@ static int arizona_spi_probe(struct spi_device *spi)
>>  	arizona->dev = &spi->dev;
>>  	arizona->irq = spi->irq;
>>  
>> +	if (has_acpi_companion(&spi->dev)) {
>> +		ret = arizona_spi_acpi_probe(arizona);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	return arizona_dev_init(arizona);
>>  }
>>  
>> @@ -104,6 +224,7 @@ static struct spi_driver arizona_spi_driver = {
>>  		.name	= "arizona",
>>  		.pm	= &arizona_pm_ops,
>>  		.of_match_table	= of_match_ptr(arizona_of_match),
>> +		.acpi_match_table = ACPI_PTR(arizona_acpi_match),
>>  	},
>>  	.probe		= arizona_spi_probe,
>>  	.remove		= arizona_spi_remove,
>> -- 
>> 2.28.0
>>
Mark Brown Jan. 18, 2021, 1:34 p.m. UTC | #4 
On Mon, Jan 18, 2021 at 02:13:50PM +0100, Hans de Goede wrote:

> More in general I'm not aware of any (recent-ish) x86 GPIO controllers
> not being able to do active low interrupts. In theory we could hit this
> code path on ARM devices using ACPI enumeration, but I don't think it
> is likely we will see a combination of ARM + ACPI enumeration +
> WM5102 + GPIO controller not capable of active-low interrupts.

I've not seen this issue on any ARM based systems.

> This overriding of the flags definitely is necessary on the Lenovo
> devices in question. I could add a
> "if (dmi_name_in_vendors("LENOVO"))" guard around it, but that
> seems unnecessary.

Possibly just an update to the comment to make it clear that some
firmwares might legitimately set the flag?
Hans de Goede Jan. 18, 2021, 1:38 p.m. UTC | #5 
Hi,

On 1/18/21 2:34 PM, Mark Brown wrote:
> On Mon, Jan 18, 2021 at 02:13:50PM +0100, Hans de Goede wrote:
> 
>> More in general I'm not aware of any (recent-ish) x86 GPIO controllers
>> not being able to do active low interrupts. In theory we could hit this
>> code path on ARM devices using ACPI enumeration, but I don't think it
>> is likely we will see a combination of ARM + ACPI enumeration +
>> WM5102 + GPIO controller not capable of active-low interrupts.
> 
> I've not seen this issue on any ARM based systems.
> 
>> This overriding of the flags definitely is necessary on the Lenovo
>> devices in question. I could add a
>> "if (dmi_name_in_vendors("LENOVO"))" guard around it, but that
>> seems unnecessary.
> 
> Possibly just an update to the comment to make it clear that some
> firmwares might legitimately set the flag
That seems sensible, I will wait a bit to so if you (or someone)
has any more review remarks to this series and then send out
a v4 with the comment updated.

Regards,

Hans
Charles Keepax Jan. 20, 2021, 10:02 a.m. UTC | #6 
On Sun, Jan 17, 2021 at 10:22:49PM +0100, Hans de Goede wrote:
> Replace the custom arizona_of_get_type() function with the generic
> device_get_match_data() helper. Besides being a nice cleanup this
> also makes it easier to add support for binding to ACPI enumerated
> devices.
> 
> While at it also fix a possible NULL pointer deref of the id
> argument to the probe functions (this could happen on e.g. manual
> driver binding through sysfs).
> 
> Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---

Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles
Andy Shevchenko Jan. 20, 2021, 7:59 p.m. UTC | #7 
On Wed, Jan 20, 2021 at 9:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 1/18/21 2:34 PM, Mark Brown wrote:
> > On Mon, Jan 18, 2021 at 02:13:50PM +0100, Hans de Goede wrote:
> >
> >> More in general I'm not aware of any (recent-ish) x86 GPIO controllers
> >> not being able to do active low interrupts. In theory we could hit this
> >> code path on ARM devices using ACPI enumeration, but I don't think it
> >> is likely we will see a combination of ARM + ACPI enumeration +
> >> WM5102 + GPIO controller not capable of active-low interrupts.
> >
> > I've not seen this issue on any ARM based systems.
> >
> >> This overriding of the flags definitely is necessary on the Lenovo
> >> devices in question. I could add a
> >> "if (dmi_name_in_vendors("LENOVO"))" guard around it, but that
> >> seems unnecessary.
> >
> > Possibly just an update to the comment to make it clear that some
> > firmwares might legitimately set the flag?
>
> Ok, I've extended the comment above the override of the irq-flags with
> the following paragraph for v4 of this patch-set:
>
>          * Note theoretically it is possible that some boards are not capable
>          * of handling active low level interrupts. In that case setting the
>          * flag to IRQF_TRIGGER_FALLING would not be a bug (and we would need
>          * to work around this) but sofar all known usages of IRQF_TRIGGER_FALLING

so far

>          * are a bug in the boards DSDT.

board's
Hans de Goede Jan. 20, 2021, 9:38 p.m. UTC | #8 
Hi,

On 1/20/21 8:59 PM, Andy Shevchenko wrote:
> On Wed, Jan 20, 2021 at 9:18 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 1/18/21 2:34 PM, Mark Brown wrote:
>>> On Mon, Jan 18, 2021 at 02:13:50PM +0100, Hans de Goede wrote:
>>>
>>>> More in general I'm not aware of any (recent-ish) x86 GPIO controllers
>>>> not being able to do active low interrupts. In theory we could hit this
>>>> code path on ARM devices using ACPI enumeration, but I don't think it
>>>> is likely we will see a combination of ARM + ACPI enumeration +
>>>> WM5102 + GPIO controller not capable of active-low interrupts.
>>>
>>> I've not seen this issue on any ARM based systems.
>>>
>>>> This overriding of the flags definitely is necessary on the Lenovo
>>>> devices in question. I could add a
>>>> "if (dmi_name_in_vendors("LENOVO"))" guard around it, but that
>>>> seems unnecessary.
>>>
>>> Possibly just an update to the comment to make it clear that some
>>> firmwares might legitimately set the flag?
>>
>> Ok, I've extended the comment above the override of the irq-flags with
>> the following paragraph for v4 of this patch-set:
>>
>>          * Note theoretically it is possible that some boards are not capable
>>          * of handling active low level interrupts. In that case setting the
>>          * flag to IRQF_TRIGGER_FALLING would not be a bug (and we would need
>>          * to work around this) but sofar all known usages of IRQF_TRIGGER_FALLING
> 
> so far
> 
>>          * are a bug in the boards DSDT.
> 
> board's
> 

Thank you for the quick review, I've fixed both spelling errors for the upcoming v4.

Regards,

Hans