| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VesAo9-GGCVyGcQuNLG8KOLcB_S+bokcxJTfeDn7sb0Bg@mail.gmail.com>
Date: Mon, 28 Dec 2020 16:14:27 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Hans de Goede <hdegoede@...hat.com>
Cc: Lee Jones <lee.jones@...aro.org>,
MyungJoo Ham <myungjoo.ham@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>,
Cezary Rojewski <cezary.rojewski@...el.com>,
Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
Jie Yang <yang.jie@...ux.intel.com>,
Mark Brown <broonie@...nel.org>, patches@...nsource.cirrus.com,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
ALSA Development Mailing List <alsa-devel@...a-project.org>,
Christian Hartmann <cornogle@...glemail.com>
Subject: Re: [PATCH 03/14] mfd: arizona: Add support for ACPI enumeration of
WM5102 connected over SPI
On Sun, Dec 27, 2020 at 11:16 PM Hans de Goede <hdegoede@...hat.com> wrote:
>
> 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].
Few nitpicks here and there, but the most important bit that hits me
is device_get_match_data().
> [1] https://github.com/Kitsune2222/Android_Yoga_Tablet_2-1051F_Kernel
>
> Cc: Christian Hartmann <cornogle@...glemail.com>
> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
> ---
> drivers/mfd/arizona-spi.c | 120 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 120 insertions(+)
>
> diff --git a/drivers/mfd/arizona-spi.c b/drivers/mfd/arizona-spi.c
> index 704f214d2614..bcdbd72fefb5 100644
> --- a/drivers/mfd/arizona-spi.c
> +++ b/drivers/mfd/arizona-spi.c
> @@ -7,7 +7,10 @@
> * Author: Mark Brown <broonie@...nsource.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,119 @@
> #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;
> + int i, 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);
> + for (i = 0; i < ARRAY_SIZE(arizona_soc_gpios); i++)
> + lookup->table[i] = arizona_soc_gpios[i];
Would memcpy() do the same at one pass?
> + 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 */
> + acpi_evaluate_object(ACPI_HANDLE(arizona->dev), "CLKE", NULL, NULL);
No error check?
> + /*
> + * 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,
> + },
> + { },
No need for comma here.
> +};
> +MODULE_DEVICE_TABLE(acpi, arizona_acpi_match);
> +#else
> +static void arizona_spi_acpi_probe(struct arizona *arizona)
> +{
> +}
Can be one line?
> +#endif
> +
> static int arizona_spi_probe(struct spi_device *spi)
> {
> const struct spi_device_id *id = spi_get_device_id(spi);
> @@ -30,6 +141,8 @@ static int arizona_spi_probe(struct spi_device *spi)
>
> if (spi->dev.of_node)
> type = arizona_of_get_type(&spi->dev);
> + else if (ACPI_COMPANION(&spi->dev))
> + type = (unsigned long)acpi_device_get_match_data(&spi->dev);
Can we rather get rid of these and use device_get_match_data() directly?
> else
> type = id->driver_data;
>
> @@ -75,6 +188,12 @@ static int arizona_spi_probe(struct spi_device *spi)
> arizona->dev = &spi->dev;
> arizona->irq = spi->irq;
>
> + if (ACPI_COMPANION(&spi->dev)) {
has_acpi_companion() ?
> + ret = arizona_spi_acpi_probe(arizona);
> + if (ret)
> + return ret;
> + }
> +
> return arizona_dev_init(arizona);
> }
>
> @@ -102,6 +221,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
>
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists