Re: [PATCH 2/2] mfd: intel_soc_pmic: Differentiate between Bay and Cherry Trail CRC variants

[Andy Shevchenko]

On Sat, Jul 1, 2017 at 1:13 PM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Both Bay and Cherry Trail devices may be used together with a Crystal Cove
> PMIC. Each platform has its own variant of the PMIC, which both use the
> same ACPI HID, but they are not 100% compatible.
>
> This commits makes the intel_soc_pmic_core code check the _HRV of the
> ACPI-firmware-node and selects intel_soc_pmic_config_byt_crc resp.
> intel_soc_pmic_config_cht_crc based on this.
>
> This fixes the Bay Trail specific ACPI OpRegion code causing problems
> on Cherry Trail devices. Specifically this was causing the external
> microsd slot on a Dell Venue 8 5855 (Cherry Trail version) to not work
> and the eMMC to become unreliable and throw lots of errors.
>

Fine to me
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>

> Reported-and-tested-by: russianneuromancer <russianneuromancer@xxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
>  drivers/mfd/Kconfig               |  4 ++--
>  drivers/mfd/intel_soc_pmic_core.c | 34 ++++++++++++++++++++++++++++------
>  2 files changed, 30 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 44e7164b5063..8533cb46a875 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -448,12 +448,12 @@ config LPC_SCH
>
>  config INTEL_SOC_PMIC
>         bool "Support for Crystal Cove PMIC"
> -       depends on HAS_IOMEM && I2C=y && GPIOLIB && COMMON_CLK
> +       depends on ACPI && HAS_IOMEM && I2C=y && GPIOLIB && COMMON_CLK
>         depends on X86 || COMPILE_TEST
>         select MFD_CORE
>         select REGMAP_I2C
>         select REGMAP_IRQ
> -       select I2C_DESIGNWARE_PLATFORM if ACPI
> +       select I2C_DESIGNWARE_PLATFORM
>         help
>           Select this option to enable support for Crystal Cove PMIC
>           on some Intel SoC systems. The PMIC provides ADC, GPIO,
> diff --git a/drivers/mfd/intel_soc_pmic_core.c b/drivers/mfd/intel_soc_pmic_core.c
> index 2234a847370a..36adf9e8153e 100644
> --- a/drivers/mfd/intel_soc_pmic_core.c
> +++ b/drivers/mfd/intel_soc_pmic_core.c
> @@ -16,6 +16,7 @@
>   * Author: Zhu, Lejun <lejun.zhu@xxxxxxxxxxxxxxx>
>   */
>
> +#include <linux/acpi.h>
>  #include <linux/module.h>
>  #include <linux/mfd/core.h>
>  #include <linux/i2c.h>
> @@ -28,6 +29,10 @@
>  #include <linux/pwm.h>
>  #include "intel_soc_pmic_core.h"
>
> +/* Crystal Cove PMIC shares same ACPI ID between different platforms */
> +#define BYT_CRC_HRV            2
> +#define CHT_CRC_HRV            3
> +
>  /* Lookup table for the Panel Enable/Disable line as GPIO signals */
>  static struct gpiod_lookup_table panel_gpio_table = {
>         /* Intel GFX is consumer */
> @@ -48,16 +53,33 @@ static int intel_soc_pmic_i2c_probe(struct i2c_client *i2c,
>                                     const struct i2c_device_id *i2c_id)
>  {
>         struct device *dev = &i2c->dev;
> -       const struct acpi_device_id *id;
>         struct intel_soc_pmic_config *config;
>         struct intel_soc_pmic *pmic;
> +       unsigned long long hrv;
> +       acpi_status status;
>         int ret;
>
> -       id = acpi_match_device(dev->driver->acpi_match_table, dev);
> -       if (!id || !id->driver_data)
> +       /*
> +        * There are 2 different Crystal Cove PMICs a Bay Trail and Cherry
> +        * Trail version, use _HRV to differentiate between the 2.
> +        */
> +       status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_HRV", NULL, &hrv);
> +       if (ACPI_FAILURE(status)) {
> +               dev_err(dev, "Failed to get PMIC hardware revision\n");
>                 return -ENODEV;
> -
> -       config = (struct intel_soc_pmic_config *)id->driver_data;
> +       }
> +
> +       switch (hrv) {
> +       case BYT_CRC_HRV:
> +               config = &intel_soc_pmic_config_byt_crc;
> +               break;
> +       case CHT_CRC_HRV:
> +               config = &intel_soc_pmic_config_cht_crc;
> +               break;
> +       default:
> +               dev_warn(dev, "Unknown hardware rev %llu, assuming BYT\n", hrv);
> +               config = &intel_soc_pmic_config_byt_crc;
> +       }
>
>         pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
>         if (!pmic)
> @@ -157,7 +179,7 @@ MODULE_DEVICE_TABLE(i2c, intel_soc_pmic_i2c_id);
>
>  #if defined(CONFIG_ACPI)
>  static const struct acpi_device_id intel_soc_pmic_acpi_match[] = {
> -       {"INT33FD", (kernel_ulong_t)&intel_soc_pmic_config_byt_crc},
> +       { "INT33FD" },
>         { },
>  };
>  MODULE_DEVICE_TABLE(acpi, intel_soc_pmic_acpi_match);
> --
> 2.13.0
>



-- 
With Best Regards,
Andy Shevchenko