diff mbox series

[v3,3/5] mfd: arizona: Add support for ACPI enumeration of WM5102 connected over SPI

Message ID 20210117212252.206115-4-hdegoede@redhat.com (mailing list archive)
State Superseded
Headers show
Series MFD/ASoC: Add support for Intel Bay Trail boards with WM5102 codec | expand

Commit Message

Hans de Goede Jan. 17, 2021, 9:22 p.m. UTC 
The Intel Bay Trail (x86/ACPI) based Lenovo Yoga Tablet 2 series use
a WM5102 codec connected over SPI.

Add support for ACPI enumeration to arizona-spi so that arizona-spi can
bind to the codec on these tablets.

This is loosely based on an earlier attempt (for Android-x86) at this by
Christian Hartmann, combined with insights in things like the speaker GPIO
from the android-x86 android port for the Lenovo Yoga Tablet 2 1051F/L [1].

[1] https://github.com/Kitsune2222/Android_Yoga_Tablet_2-1051F_Kernel

Cc: Christian Hartmann <cornogle@googlemail.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v3:
- Fix compilation error when CONFIG_ACPI is not set

Changes in v2:
- Minor coding style tweaks
- Use memcpy instead of for loop to copy gpiod_lookup-s
- Log a warning when the ACPI "CLKE" call fails
- Drop addition of acpi_device_get_match_data() call, as the code was
  moved over to use the generic device_get_match_data() helper in a
  (new in v2) preparation patch
---
 drivers/mfd/arizona-spi.c | 121 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

Comments

Mark Brown Jan. 18, 2021, 1:02 p.m. UTC | #1 
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 | #2 
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 | #3 
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 | #4 
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
Hans de Goede Jan. 20, 2021, 7:18 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?

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
         * are a bug in the boards DSDT.

Regards,

Hans
Andy Shevchenko Jan. 20, 2021, 7:59 p.m. UTC | #6 
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 | #7 
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
diff mbox series

Patch

diff --git a/drivers/mfd/arizona-spi.c b/drivers/mfd/arizona-spi.c
index 798b88295c77..c652b733b2cb 100644
--- a/drivers/mfd/arizona-spi.c
+++ b/drivers/mfd/arizona-spi.c
@@ -7,7 +7,10 @@ 
  * Author: Mark Brown <broonie@opensource.wolfsonmicro.com>
  */
 
+#include <linux/acpi.h>
 #include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/machine.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
@@ -15,11 +18,122 @@ 
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
 #include <linux/of.h>
+#include <uapi/linux/input-event-codes.h>
 
 #include <linux/mfd/arizona/core.h>
 
 #include "arizona.h"
 
+#ifdef CONFIG_ACPI
+const struct acpi_gpio_params reset_gpios = { 1, 0, false };
+const struct acpi_gpio_params ldoena_gpios = { 2, 0, false };
+
+static const struct acpi_gpio_mapping arizona_acpi_gpios[] = {
+	{ "reset-gpios", &reset_gpios, 1, },
+	{ "wlf,ldoena-gpios", &ldoena_gpios, 1 },
+	{ }
+};
+
+/*
+ * The ACPI resources for the device only describe external GPIO-s. They do
+ * not provide mappings for the GPIO-s coming from the Arizona codec itself.
+ */
+static const struct gpiod_lookup arizona_soc_gpios[] = {
+	{ "arizona", 2, "wlf,spkvdd-ena", 0, GPIO_ACTIVE_HIGH },
+	{ "arizona", 4, "wlf,micd-pol", 0, GPIO_ACTIVE_LOW },
+};
+
+/*
+ * The AOSP 3.5 mm Headset: Accessory Specification gives the following values:
+ * Function A Play/Pause:           0 ohm
+ * Function D Voice assistant:    135 ohm
+ * Function B Volume Up           240 ohm
+ * Function C Volume Down         470 ohm
+ * Minimum Mic DC resistance     1000 ohm
+ * Minimum Ear speaker impedance   16 ohm
+ * Note the first max value below must be less then the min. speaker impedance,
+ * to allow CTIA/OMTP detection to work. The other max values are the closest
+ * value from extcon-arizona.c:arizona_micd_levels halfway 2 button resistances.
+ */
+static const struct arizona_micd_range arizona_micd_aosp_ranges[] = {
+	{ .max =  11, .key = KEY_PLAYPAUSE },
+	{ .max = 186, .key = KEY_VOICECOMMAND },
+	{ .max = 348, .key = KEY_VOLUMEUP },
+	{ .max = 752, .key = KEY_VOLUMEDOWN },
+};
+
+static void arizona_spi_acpi_remove_lookup(void *lookup)
+{
+	gpiod_remove_lookup_table(lookup);
+}
+
+static int arizona_spi_acpi_probe(struct arizona *arizona)
+{
+	struct gpiod_lookup_table *lookup;
+	acpi_status status;
+	int ret;
+
+	/* Add mappings for the 2 ACPI declared GPIOs used for reset and ldo-ena */
+	devm_acpi_dev_add_driver_gpios(arizona->dev, arizona_acpi_gpios);
+
+	/* Add lookups for the SoCs own GPIOs used for micdet-polarity and spkVDD-enable */
+	lookup = devm_kzalloc(arizona->dev,
+			      struct_size(lookup, table, ARRAY_SIZE(arizona_soc_gpios) + 1),
+			      GFP_KERNEL);
+	if (!lookup)
+		return -ENOMEM;
+
+	lookup->dev_id = dev_name(arizona->dev);
+	memcpy(lookup->table, arizona_soc_gpios, sizeof(arizona_soc_gpios));
+
+	gpiod_add_lookup_table(lookup);
+	ret = devm_add_action_or_reset(arizona->dev, arizona_spi_acpi_remove_lookup, lookup);
+	if (ret)
+		return ret;
+
+	/* Enable 32KHz clock from SoC to codec for jack-detect */
+	status = acpi_evaluate_object(ACPI_HANDLE(arizona->dev), "CLKE", NULL, NULL);
+	if (ACPI_FAILURE(status))
+		dev_warn(arizona->dev, "Failed to enable 32KHz clk ACPI error %d\n", status);
+
+	/*
+	 * 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;
+
+	/* 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,