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

[Srinivas Pandruvada <srinivas.pandruvada@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?
Added Jacob. 
If regmap_bulk_read is protected, we don't need to go through IIO. So
change looks fine to me.

Thanks,
Srinivas
> 
> > 
> > ---
> >  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