Re: [PATCH v4 2/2] ACPI / PMIC: Stop xpower OPRegion handler relying on IIO

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Wed, 2017-04-19 at 22:18 +0200, Rafael J. Wysocki wrote:
> On Wed, Apr 19, 2017 at 3:07 PM, Hans de Goede <hdegoede@redhat.com>
> wrote:
> > The intel_pmic_xpower code provides an OPRegion handler, which must
> > be
> > available before other drivers using it are loaded, which can only
> > be
> > ensured if both the mfd and opregion drivers are built in, which is
> > why
> > the Kconfig option for intel_pmic_xpower is a bool.
> > 
> > The use of IIO is causing trouble for generic distro configs here as
> > distros will typically want to build IIO drivers as modules and
> > there
> > really is no reason to use IIO here. The reading of the ADC value is
> > a
> > single regmap_bulk_read, which is already protected against races by
> > the regmap-lock.
> > 
> > This commit removes the use of IIO, allowing distros to enable the
> > driver without needing to built IIO in and also actually simplifies
> > the code.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Andy, Srinivas, any concerns?

I would prefer to have this magic shifts in a common header between
drivers, but I'm not insisting (I admit it might take much more work for
no clear benefit, except keeping ADC bits in one place).

So, I'm fine with this change.

> 
> > ---
> >  drivers/acpi/Kconfig                  |  2 +-
> >  drivers/acpi/pmic/intel_pmic_xpower.c | 21 ++++-----------------
> >  2 files changed, 5 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index 4f12fe0..842530f 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -506,7 +506,7 @@ config CRC_PMIC_OPREGION
> > 
> >  config XPOWER_PMIC_OPREGION
> >         bool "ACPI operation region support for XPower AXP288 PMIC"
> > -       depends on AXP288_ADC = y
> > +       depends on MFD_AXP20X_I2C
> >         help
> >           This config adds ACPI operation region support for XPower
> > AXP288 PMIC.
> > 
> > diff --git a/drivers/acpi/pmic/intel_pmic_xpower.c
> > b/drivers/acpi/pmic/intel_pmic_xpower.c
> > index e6e991a..55f5111 100644
> > --- a/drivers/acpi/pmic/intel_pmic_xpower.c
> > +++ b/drivers/acpi/pmic/intel_pmic_xpower.c
> > @@ -18,7 +18,6 @@
> >  #include <linux/mfd/axp20x.h>
> >  #include <linux/regmap.h>
> >  #include <linux/platform_device.h>
> > -#include <linux/iio/consumer.h>
> >  #include "intel_pmic.h"
> > 
> >  #define XPOWER_GPADC_LOW       0x5b
> > @@ -186,28 +185,16 @@ static int
> > intel_xpower_pmic_update_power(struct regmap *regmap, int reg,
> >   * @regmap: regmap of the PMIC device
> >   * @reg: register to get the reading
> >   *
> > - * We could get the sensor value by manipulating the HW regs here,
> > but since
> > - * the axp288 IIO driver may also access the same regs at the same
> > time, the
> > - * APIs provided by IIO subsystem are used here instead to avoid
> > problems. As
> > - * a result, the two passed in params are of no actual use.
> > - *
> >   * Return a positive value on success, errno on failure.
> >   */
> >  static int intel_xpower_pmic_get_raw_temp(struct regmap *regmap,
> > int reg)
> >  {
> > -       struct iio_channel *gpadc_chan;
> > -       int ret, val;
> > -
> > -       gpadc_chan = iio_channel_get(NULL, "axp288-system-temp");
> > -       if (IS_ERR_OR_NULL(gpadc_chan))
> > -               return -EACCES;
> > +       u8 buf[2];
> > 
> > -       ret = iio_read_channel_raw(gpadc_chan, &val);
> > -       if (ret < 0)
> > -               val = ret;
> > +       if (regmap_bulk_read(regmap, AXP288_GP_ADC_H, buf, 2))
> > +               return -EIO;
> > 
> > -       iio_channel_release(gpadc_chan);
> > -       return val;
> > +       return (buf[0] << 4) + ((buf[1] >> 4) & 0x0F);
> >  }
> > 
> >  static struct intel_pmic_opregion_data
> > intel_xpower_pmic_opregion_data = {
> > --
> > 2.9.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy