Re: [PATCH 03/14] mfd: arizona: Add support for ACPI enumeration of WM5102 connected over SPI

[Andy Shevchenko]

On Sun, Dec 27, 2020 at 11:16 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
>   */
>
> +#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