RE: [PATCH V1 3/3] regulator: slg51000: add slg51000 regulator driver

[Eric Hyeung Dong Jeong]

On Wednesday, April 17, 2019 12:25 AM +0900, Mark Brown wrote:
 
> On Tue, Apr 16, 2019 at 01:37:27PM +0900, Eric Jeong wrote:
> 
> > +static int slg51000_regulator_is_enabled(struct regulator_dev *rdev)
> > +{
> > +   struct slg51000 *chip = rdev_get_drvdata(rdev);
> > +   int ret, id = rdev_get_id(rdev);
> > +   unsigned int state;
> > +
> > +   ret = regmap_read(chip->regmap, es_reg[id].sreg, );
> > +   if (ret < 0) {
> > +   dev_err(chip->dev, "Failed to read status register(%d)\n",
> > +   ret);
> > +   return ret;
> > +   }
> > +
> > +   if (!(state & SLG51000_STA_ILIM_FLAG_MASK) &&
> > +   (state & SLG51000_STA_VOUT_OK_FLAG_MASK))
> > +   return 1;
> > +   else
> > +   return 0;
> 
> This looks like it should be a get_status() operation as it's reading
> status bits rather than the command we sent to the device - for that
> just use regulator_is_enabled_regmap().

I thought that it needs to return current status of a regulator when the 
function is called.
I am wondering that the *is_enabled()* function is just to check 
If a regulator has been turned on or not rather than getting current status of 
the regulator.

Thanks
Eric

RE: [PATCH V3 2/4] mfd: pv88080: MFD core support

[Eric Hyeung Dong Jeong]

On Monday, November 21, 2016 10:09 PM, Lee Jones Wrote:

>
> On Fri, 18 Nov 2016, Eric Jeong wrote:
> 
> >
> > From: Eric Jeong 
> >
> > This patch adds supports for PV88080 MFD core device.
> >
> > It provides communication through the I2C interface.
> > It contains the following components:
> > - Regulators
> > - Configurable GPIOs
> >
> > Kconfig and Makefile are updated to reflect support for PV88080 PMIC.
> >
> > Signed-off-by: Eric Jeong 
> >
> > ---
> > This patch applies against linux-next and next-20161117
> >
> > Hi,
> >
> > This patch adds MFD core driver for PV88080 PMIC.
> > This is done as part of the existing PV88080 regulator driver by
> > expending the driver for GPIO function support.
> >
> > Change since PATCH V2
> >  - Make one file insted of usging core and i2c file
> >  - Use devm_ function to be managed resource automatically
> >  - Separated mfd_cell and regmap_irq_chip declaration for clarification.
> >  - Updated Kconfig to use OF and assign yes to I2C
> >
> > Change since PATCH V1
> >  - Patch separated from PATCH V1
> >
> > Regards,
> > Eric Jeong, Dialog Semiconductor Ltd.
> >
> >
> >  drivers/mfd/Kconfig |   12 ++
> >  drivers/mfd/Makefile|1 +
> >  drivers/mfd/pv88080.c   |  331 
> > +++
> >  include/linux/mfd/pv88080.h |  222 +
> >  4 files changed, 566 insertions(+)
> >  create mode 100644 drivers/mfd/pv88080.c  create mode 100644
> > include/linux/mfd/pv88080.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > 06dc9b0..75abf2d 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -792,6 +792,18 @@ config MFD_PM8921_CORE
> >   Say M here if you want to include support for PM8921 chip as a module.
> >   This will build a module called "pm8921-core".
> >
> > +config MFD_PV88080
> > +   tristate "Powerventure Semiconductor PV88080 PMIC Support"
> > +   select MFD_CORE
> > +   select REGMAP_I2C
> > +   select REGMAP_IRQ
> > +   depends on I2C=y && OF
> > +   help
> > + Say yes here for support for the Powerventure Semiconductor PV88080 
> > PMIC.
> > + This includes the I2C driver and core APIs.
> > + Additional drivers must be enabled in order to use the functionality
> > + of the device.
> > +
> >  config MFD_QCOM_RPM
> > tristate "Qualcomm Resource Power Manager (RPM)"
> > depends on ARCH_QCOM && OF
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > db39377..e9e16c6 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -173,6 +173,7 @@ obj-$(CONFIG_MFD_SI476X_CORE)   += si476x-core.o
> >  obj-$(CONFIG_MFD_CS5535)   += cs5535-mfd.o
> >  obj-$(CONFIG_MFD_OMAP_USB_HOST)+= omap-usb-host.o omap-usb-tll.o
> >  obj-$(CONFIG_MFD_PM8921_CORE)  += pm8921-core.o ssbi.o
> > +obj-$(CONFIG_MFD_PV88080)  += pv88080.o
> >  obj-$(CONFIG_MFD_QCOM_RPM) += qcom_rpm.o
> >  obj-$(CONFIG_MFD_SPMI_PMIC)+= qcom-spmi-pmic.o
> >  obj-$(CONFIG_TPS65911_COMPARATOR)  += tps65911-comparator.o
> > diff --git a/drivers/mfd/pv88080.c b/drivers/mfd/pv88080.c new file
> > mode 100644 index 000..518b44f
> > --- /dev/null
> > +++ b/drivers/mfd/pv88080.c
> > @@ -0,0 +1,331 @@
> > +/*
> > + * pv88080-i2c.c - I2C access driver for PV88080
> 
> Remove the filename.
> 
> They have a habit of becoming out of date (like now).

OK, I will do that.

> 
> > + * Copyright (C) 2016  Powerventure Semiconductor Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> Alphabetical.

OK, I see.

> 
> > +#include 
> 
> This doesn't need to be separated from the rest.

OK.

> 
> > +#definePV88080_REG_EVENT_A_OFFSET  0
> > +#definePV88080_REG_EVENT_B_OFFSET  1
> > +#definePV88080_REG_EVENT_C_OFFSET  2
> 
> Spaces after 'define'.

OK I will do that.

> 
> > +static const struct resource regulators_aa_resources[] = {
> > +   {
> > +   .name   = "VDD_TEMP_FAULT",
> > +   .start  = PV88080_AA_IRQ_VDD_FLT,
> > +   .end= PV88080_AA_IRQ_OVER_TEMP,
> > +   .flags  = IORESOURCE_IRQ,
> > +   },
> > +};
> > +
> > +static const struct resource regulators_ba_resources[] = {
> > +   {
> > +   .name   = "VDD_TEMP_FAULT",
> > +   .start  = PV88080_BA_IRQ_VDD_FLT,
> > +   

RE: [PATCH V3 2/4] mfd: pv88080: MFD core support

[Eric Hyeung Dong Jeong]

On Monday, November 21, 2016 10:09 PM, Lee Jones Wrote:

>
> On Fri, 18 Nov 2016, Eric Jeong wrote:
> 
> >
> > From: Eric Jeong 
> >
> > This patch adds supports for PV88080 MFD core device.
> >
> > It provides communication through the I2C interface.
> > It contains the following components:
> > - Regulators
> > - Configurable GPIOs
> >
> > Kconfig and Makefile are updated to reflect support for PV88080 PMIC.
> >
> > Signed-off-by: Eric Jeong 
> >
> > ---
> > This patch applies against linux-next and next-20161117
> >
> > Hi,
> >
> > This patch adds MFD core driver for PV88080 PMIC.
> > This is done as part of the existing PV88080 regulator driver by
> > expending the driver for GPIO function support.
> >
> > Change since PATCH V2
> >  - Make one file insted of usging core and i2c file
> >  - Use devm_ function to be managed resource automatically
> >  - Separated mfd_cell and regmap_irq_chip declaration for clarification.
> >  - Updated Kconfig to use OF and assign yes to I2C
> >
> > Change since PATCH V1
> >  - Patch separated from PATCH V1
> >
> > Regards,
> > Eric Jeong, Dialog Semiconductor Ltd.
> >
> >
> >  drivers/mfd/Kconfig |   12 ++
> >  drivers/mfd/Makefile|1 +
> >  drivers/mfd/pv88080.c   |  331 
> > +++
> >  include/linux/mfd/pv88080.h |  222 +
> >  4 files changed, 566 insertions(+)
> >  create mode 100644 drivers/mfd/pv88080.c  create mode 100644
> > include/linux/mfd/pv88080.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > 06dc9b0..75abf2d 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -792,6 +792,18 @@ config MFD_PM8921_CORE
> >   Say M here if you want to include support for PM8921 chip as a module.
> >   This will build a module called "pm8921-core".
> >
> > +config MFD_PV88080
> > +   tristate "Powerventure Semiconductor PV88080 PMIC Support"
> > +   select MFD_CORE
> > +   select REGMAP_I2C
> > +   select REGMAP_IRQ
> > +   depends on I2C=y && OF
> > +   help
> > + Say yes here for support for the Powerventure Semiconductor PV88080 
> > PMIC.
> > + This includes the I2C driver and core APIs.
> > + Additional drivers must be enabled in order to use the functionality
> > + of the device.
> > +
> >  config MFD_QCOM_RPM
> > tristate "Qualcomm Resource Power Manager (RPM)"
> > depends on ARCH_QCOM && OF
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > db39377..e9e16c6 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -173,6 +173,7 @@ obj-$(CONFIG_MFD_SI476X_CORE)   += si476x-core.o
> >  obj-$(CONFIG_MFD_CS5535)   += cs5535-mfd.o
> >  obj-$(CONFIG_MFD_OMAP_USB_HOST)+= omap-usb-host.o omap-usb-tll.o
> >  obj-$(CONFIG_MFD_PM8921_CORE)  += pm8921-core.o ssbi.o
> > +obj-$(CONFIG_MFD_PV88080)  += pv88080.o
> >  obj-$(CONFIG_MFD_QCOM_RPM) += qcom_rpm.o
> >  obj-$(CONFIG_MFD_SPMI_PMIC)+= qcom-spmi-pmic.o
> >  obj-$(CONFIG_TPS65911_COMPARATOR)  += tps65911-comparator.o
> > diff --git a/drivers/mfd/pv88080.c b/drivers/mfd/pv88080.c new file
> > mode 100644 index 000..518b44f
> > --- /dev/null
> > +++ b/drivers/mfd/pv88080.c
> > @@ -0,0 +1,331 @@
> > +/*
> > + * pv88080-i2c.c - I2C access driver for PV88080
> 
> Remove the filename.
> 
> They have a habit of becoming out of date (like now).

OK, I will do that.

> 
> > + * Copyright (C) 2016  Powerventure Semiconductor Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; either version 2
> > + * of the License, or (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> Alphabetical.

OK, I see.

> 
> > +#include 
> 
> This doesn't need to be separated from the rest.

OK.

> 
> > +#definePV88080_REG_EVENT_A_OFFSET  0
> > +#definePV88080_REG_EVENT_B_OFFSET  1
> > +#definePV88080_REG_EVENT_C_OFFSET  2
> 
> Spaces after 'define'.

OK I will do that.

> 
> > +static const struct resource regulators_aa_resources[] = {
> > +   {
> > +   .name   = "VDD_TEMP_FAULT",
> > +   .start  = PV88080_AA_IRQ_VDD_FLT,
> > +   .end= PV88080_AA_IRQ_OVER_TEMP,
> > +   .flags  = IORESOURCE_IRQ,
> > +   },
> > +};
> > +
> > +static const struct resource regulators_ba_resources[] = {
> > +   {
> > +   .name   = "VDD_TEMP_FAULT",
> > +   .start  = PV88080_BA_IRQ_VDD_FLT,
> > +   .end= PV88080_BA_IRQ_OVER_TEMP,
> > +   .flags  = 

RE: [PATCH V3 1/4] Documentation: pv88080: Move binding document

[Eric Hyeung Dong Jeong]

On Saturday, November 19, 2016 12:39 AM, Rob Herring wrote:

> On Fri, Nov 18, 2016 at 09:35:46AM +0900, Eric Jeong wrote:
> >
> > From: Eric Jeong 
> >
> > The change is to move pv88080 binding document from the regulator
> > directory to mfd binding directory for PV88080 PMIC MFD support.
> > And, GPIO properties are added.
> >
> >
> > Signed-off-by: Eric Jeong 
> >
> > ---
> > This patch applies against linux-next and next-20161117
> >
> > Hi,
> >
> > Change since PATCH V2
> >  - Added property and description for gpio
> >
> > Change since PATCH V1
> >  - Patch separated from PATCH V1
> >
> > Regards,
> > Eric Jeong, Dialog Semiconductor Ltd.
> >
> >
> >  .../bindings/{regulator => mfd}/pv88080.txt|   16 +++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)  rename
> > Documentation/devicetree/bindings/{regulator => mfd}/pv88080.txt (79%)
> >
> > diff --git a/Documentation/devicetree/bindings/regulator/pv88080.txt
> > b/Documentation/devicetree/bindings/mfd/pv88080.txt
> > similarity index 79%
> > rename from Documentation/devicetree/bindings/regulator/pv88080.txt
> > rename to Documentation/devicetree/bindings/mfd/pv88080.txt
> > index e6e4b9c8..7e24f95 100644
> > --- a/Documentation/devicetree/bindings/regulator/pv88080.txt
> > +++ b/Documentation/devicetree/bindings/mfd/pv88080.txt
> > @@ -1,4 +1,4 @@
> > -* Powerventure Semiconductor PV88080 Voltage Regulator
> > +* Powerventure Semiconductor PV88080 PMIC
> >
> >  Required properties:
> >  - compatible: Must be one of the following, depending on the @@ -16,8
> > +16,15 @@ Required properties:
> >standard binding for regulators; see regulator.txt.
> >BUCK1, BUCK2, BUCK3 and HVBUCK.
> >
> > +- gpio-controller: Marks the device node as a GPIO controller.
> > +- #gpio-cells: Should be 2. See gpio.txt in this directory
> > +  for a description of the cells format.
> > +
> >  Optional properties:
> > +- ngpios : Number of in-use slots of available slots for GPIO.
> > +  Maximum is 2.
> >  - Any optional property defined in regulator.txt
> > +  and gpio.txt for more information.
> >
> >  Example:
> >
> > @@ -27,6 +34,13 @@ Example:
> > interrupt-parent = <>;
> > interrupts = <24 24>;
> >
> > +   gpioex {
> 
> gpio-controller {
> 
> > +   compatible = "pvs,pv88080-gpio";
> 
> Is this documented?

No, It's not documented. I will update this document with the compatible string.

> 
> > +   gpio-controller;
> > +   #gpio-cells = <2>;
> > +   ngpios = <2>;
> > +   };
> > +
> > regulators {
> > BUCK1 {
> > regulator-name = "buck1";
> > --
> > end-of-patch for PATCH V3
> >

RE: [PATCH V3 1/4] Documentation: pv88080: Move binding document

[Eric Hyeung Dong Jeong]

On Saturday, November 19, 2016 12:39 AM, Rob Herring wrote:

> On Fri, Nov 18, 2016 at 09:35:46AM +0900, Eric Jeong wrote:
> >
> > From: Eric Jeong 
> >
> > The change is to move pv88080 binding document from the regulator
> > directory to mfd binding directory for PV88080 PMIC MFD support.
> > And, GPIO properties are added.
> >
> >
> > Signed-off-by: Eric Jeong 
> >
> > ---
> > This patch applies against linux-next and next-20161117
> >
> > Hi,
> >
> > Change since PATCH V2
> >  - Added property and description for gpio
> >
> > Change since PATCH V1
> >  - Patch separated from PATCH V1
> >
> > Regards,
> > Eric Jeong, Dialog Semiconductor Ltd.
> >
> >
> >  .../bindings/{regulator => mfd}/pv88080.txt|   16 +++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)  rename
> > Documentation/devicetree/bindings/{regulator => mfd}/pv88080.txt (79%)
> >
> > diff --git a/Documentation/devicetree/bindings/regulator/pv88080.txt
> > b/Documentation/devicetree/bindings/mfd/pv88080.txt
> > similarity index 79%
> > rename from Documentation/devicetree/bindings/regulator/pv88080.txt
> > rename to Documentation/devicetree/bindings/mfd/pv88080.txt
> > index e6e4b9c8..7e24f95 100644
> > --- a/Documentation/devicetree/bindings/regulator/pv88080.txt
> > +++ b/Documentation/devicetree/bindings/mfd/pv88080.txt
> > @@ -1,4 +1,4 @@
> > -* Powerventure Semiconductor PV88080 Voltage Regulator
> > +* Powerventure Semiconductor PV88080 PMIC
> >
> >  Required properties:
> >  - compatible: Must be one of the following, depending on the @@ -16,8
> > +16,15 @@ Required properties:
> >standard binding for regulators; see regulator.txt.
> >BUCK1, BUCK2, BUCK3 and HVBUCK.
> >
> > +- gpio-controller: Marks the device node as a GPIO controller.
> > +- #gpio-cells: Should be 2. See gpio.txt in this directory
> > +  for a description of the cells format.
> > +
> >  Optional properties:
> > +- ngpios : Number of in-use slots of available slots for GPIO.
> > +  Maximum is 2.
> >  - Any optional property defined in regulator.txt
> > +  and gpio.txt for more information.
> >
> >  Example:
> >
> > @@ -27,6 +34,13 @@ Example:
> > interrupt-parent = <>;
> > interrupts = <24 24>;
> >
> > +   gpioex {
> 
> gpio-controller {
> 
> > +   compatible = "pvs,pv88080-gpio";
> 
> Is this documented?

No, It's not documented. I will update this document with the compatible string.

> 
> > +   gpio-controller;
> > +   #gpio-cells = <2>;
> > +   ngpios = <2>;
> > +   };
> > +
> > regulators {
> > BUCK1 {
> > regulator-name = "buck1";
> > --
> > end-of-patch for PATCH V3
> >

RE: [PATCH V2 1/4] Documentation: pv88080: Move binding document

[Eric Hyeung Dong Jeong]

On Monday, October 31, 2016 1:44 PM, Rob Herring wrote:

> On Thu, Oct 27, 2016 at 10:03:14AM +0900, Eric Jeong wrote:
> >
> > From: Eric Jeong 
> >
> > The change is to move pv88080 binding document from the regulator
> > directory to mfd binding directory for PV88080 PMIC MFD support.
> >
> >
> > Signed-off-by: Eric Jeong 
> >
> > ---
> > This patch applies against linux-next and next-20161026
> >
> > Hi,
> >
> > Change since PATCH V1
> >  - Patch separated from PATCH V1
> >
> > Regards,
> > Eric Jeong, Dialog Semiconductor Ltd.
> >
> >
> >  Documentation/devicetree/bindings/mfd/pv88080.txt  |   63
> 
> >  .../devicetree/bindings/regulator/pv88080.txt  |   62 
> > ---
> >  2 files changed, 63 insertions(+), 62 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/mfd/pv88080.txt
> >  delete mode 100644
> > Documentation/devicetree/bindings/regulator/pv88080.txt
> 
> Please resend using git-format-patch -M option.
> 
> Rob

Thank you for the comments.
Maybe the document will be updated additionally.
I will send patch again.

Regards
Eric

RE: [PATCH V2 1/4] Documentation: pv88080: Move binding document

[Eric Hyeung Dong Jeong]

On Monday, October 31, 2016 1:44 PM, Rob Herring wrote:

> On Thu, Oct 27, 2016 at 10:03:14AM +0900, Eric Jeong wrote:
> >
> > From: Eric Jeong 
> >
> > The change is to move pv88080 binding document from the regulator
> > directory to mfd binding directory for PV88080 PMIC MFD support.
> >
> >
> > Signed-off-by: Eric Jeong 
> >
> > ---
> > This patch applies against linux-next and next-20161026
> >
> > Hi,
> >
> > Change since PATCH V1
> >  - Patch separated from PATCH V1
> >
> > Regards,
> > Eric Jeong, Dialog Semiconductor Ltd.
> >
> >
> >  Documentation/devicetree/bindings/mfd/pv88080.txt  |   63
> 
> >  .../devicetree/bindings/regulator/pv88080.txt  |   62 
> > ---
> >  2 files changed, 63 insertions(+), 62 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/mfd/pv88080.txt
> >  delete mode 100644
> > Documentation/devicetree/bindings/regulator/pv88080.txt
> 
> Please resend using git-format-patch -M option.
> 
> Rob

Thank you for the comments.
Maybe the document will be updated additionally.
I will send patch again.

Regards
Eric

RE: [PATCH V2 2/4] mfd: pv88080: MFD core support

[Eric Hyeung Dong Jeong]

On Friday, October 28, 2016 9:18 PM, Linus Walleij wrote:

> On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
>  wrote:
> 
> > +++ b/drivers/mfd/pv88080-i2c.c
> > +
> > +static const struct of_device_id pv88080_of_match_table[] = {
> > +   { .compatible = "pvs,pv88080",.data = (void *)TYPE_PV88080_AA },
> > +   { .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> > +   { .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> > +   { },
> > +};
> > +MODULE_DEVICE_TABLE(of, pv88080_of_match_table);
> 
> Actually you are doing something weird.
> 
> The only thing you put in your device tree is this I guess.
> 
> That means that the GPIO chip does *not* have a device tree entry, so you
> cannot reference the GPIOs there with  notation.
> 
> Please look around a bit to see how other OF-MFDs do it: they register and
> populate by struct mfd_cell using the .of_compatible member so that
> subdevices get their own DT nodes, which is necessary for nodes providing
> resources such as GPIOs, regulators and clocks, lest you cannot reference them
> in your device tree!
> 
> Therefore I think all your subdevices should instantiate from device tree with
> compatible matching as well, not as platform devices.
> 
> > +struct pv88080_pdata {
> > +   int (*init)(struct pv88080 *pv88080);
> > +   int irq_base;
> > +   int gpio_base;
> 
> NAK.
> 
> Get irq from the device tree, assign gpio base dynamically.
> 
> > +   struct regulator_init_data
> > + *regulators[PV88080_MAX_REGULATORS];
> 
> I suspect also this should come from the device tree.
> 
> Yours,
> Linus Walleij

Thank you for the comments.
I will send patch again.

Regards
Eric

RE: [PATCH V2 2/4] mfd: pv88080: MFD core support

[Eric Hyeung Dong Jeong]

On Friday, October 28, 2016 9:18 PM, Linus Walleij wrote:

> On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
>  wrote:
> 
> > +++ b/drivers/mfd/pv88080-i2c.c
> > +
> > +static const struct of_device_id pv88080_of_match_table[] = {
> > +   { .compatible = "pvs,pv88080",.data = (void *)TYPE_PV88080_AA },
> > +   { .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> > +   { .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> > +   { },
> > +};
> > +MODULE_DEVICE_TABLE(of, pv88080_of_match_table);
> 
> Actually you are doing something weird.
> 
> The only thing you put in your device tree is this I guess.
> 
> That means that the GPIO chip does *not* have a device tree entry, so you
> cannot reference the GPIOs there with  notation.
> 
> Please look around a bit to see how other OF-MFDs do it: they register and
> populate by struct mfd_cell using the .of_compatible member so that
> subdevices get their own DT nodes, which is necessary for nodes providing
> resources such as GPIOs, regulators and clocks, lest you cannot reference them
> in your device tree!
> 
> Therefore I think all your subdevices should instantiate from device tree with
> compatible matching as well, not as platform devices.
> 
> > +struct pv88080_pdata {
> > +   int (*init)(struct pv88080 *pv88080);
> > +   int irq_base;
> > +   int gpio_base;
> 
> NAK.
> 
> Get irq from the device tree, assign gpio base dynamically.
> 
> > +   struct regulator_init_data
> > + *regulators[PV88080_MAX_REGULATORS];
> 
> I suspect also this should come from the device tree.
> 
> Yours,
> Linus Walleij

Thank you for the comments.
I will send patch again.

Regards
Eric

RE: [PATCH V2 4/4] gpio: pv88080: Add GPIO function support

[Eric Hyeung Dong Jeong]

On Friday, October 28, 2016 9:11 PM, Linux Walleij wrote:

> On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
>  wrote:
> 
> > From: Eric Jeong 
> >
> > This patch adds support for PV88080 PMIC GPIOs.
> > PV88080 has two configurable GPIOs.
> >
> > Kconfig and Makefile are updated to reflect support for PV88080 PMIC
> > GPIO.
> >
> > Signed-off-by: Eric Jeong 
> (...)
> 
> > +static int pv88080_gpio_direction_input(struct gpio_chip *gc,
> > +   unsigned int offset) {
> > +   struct pv88080_gpio *priv = gpiochip_get_data(gc);
> > +   struct pv88080 *chip = priv->chip;
> > +   int ret;
> > +
> > +   /* Set the initial value */
> > +   ret = regmap_update_bits(chip->regmap, priv->gpio_base_reg + offset,
> > +   PV88080_GPIO_OUTPUT_MASK, 0);
> > +   if (ret)
> > +   return ret;
> 
> So you set the initial value when we change the pin to *input*...
> 
> > +
> > +   return regmap_update_bits(chip->regmap, priv->gpio_base_reg + 
> > offset,
> > +   PV88080_GPIO_DIRECTION_MASK, 0); }
> > +
> > +static int pv88080_gpio_direction_output(struct gpio_chip *gc,
> > +   unsigned int offset, int value) {
> > +   struct pv88080_gpio *priv = gpiochip_get_data(gc);
> > +   struct pv88080 *chip = priv->chip;
> > +   int ret;
> > +
> > +   ret = regmap_update_bits(chip->regmap, priv->gpio_base_reg + offset,
> > +   PV88080_GPIO_DIRECTION_MASK,
> > +   PV88080_GPIO_DIRECTION_MASK);
> 
> But do nothing when we change the pin to *output*?
> 
> It seems like you switched the two function implementations or something?
> 
> > +static int pv88080_gpio_get(struct gpio_chip *gc, unsigned int
> > +offset) {
> > +   struct pv88080_gpio *priv = gpiochip_get_data(gc);
> > +   struct pv88080 *chip = priv->chip;
> > +   unsigned int reg = 0, direction;
> > +   int ret;
> > +
> > +   ret = regmap_read(chip->regmap, priv->gpio_base_reg + offset, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   direction = (reg & PV88080_GPIO_DIRECTION_MASK);
> > +   if (direction == PV88080_PORT_DIRECTION_OUTPUT) {
> > +   if (reg & PV88080_GPIO_OUTPUT_EN)
> > +   return 1;
> > +   ret = 0;
> > +   } else {
> > +   ret = regmap_read(chip->regmap, priv->input_reg, );
> > +   if (ret < 0)
> > +   return ret;
> > +   ret = (reg & (PV88080_GPIO_INPUT_MASK << offset)) >>
> > + offset;
> 
> Isn't this what you want to do?
> 
> #include 
> 
> ret = !!(reg & BIT(offset));
> 
> The mask is 0x01. No point in making things more complicated than they are.
> 
> 
> > +static void pv88080_gpio_set(struct gpio_chip *gc, unsigned int offset,
> > +   int value) {
> > +   struct pv88080_gpio *priv = gpiochip_get_data(gc);
> > +   struct pv88080 *chip = priv->chip;
> > +
> > +   if (value)
> > +   regmap_update_bits(chip->regmap, priv->gpio_base_reg + 
> > offset,
> > +   PV88080_GPIO_OUTPUT_MASK,
> > +   PV88080_GPIO_OUTPUT_EN);
> > +   else
> > +   regmap_update_bits(chip->regmap, priv->gpio_base_reg + 
> > offset,
> > +   PV88080_GPIO_OUTPUT_MASK,
> > +   PV88080_GPIO_OUTPUT_DIS); }
> 
> Looks good, output is more complicated.
> 
> > +static const struct gpio_chip template_gpio = {
> > +   .label = "pv88080-gpio",
> > +   .owner = THIS_MODULE,
> > +   .get_direction = pv88080_gpio_get_direction,
> > +   .direction_input = pv88080_gpio_direction_input,
> > +   .direction_output = pv88080_gpio_direction_output,
> > +   .get = pv88080_gpio_get,
> > +   .set = pv88080_gpio_set,
> > +   .base = -1,
> > +   .ngpio = DEFAULT_PIN_NUMBER,
> > +};
> 
> Why even have a #define for DEFAULT_PIN_NUMBER?
> Just hardcode it here.
> 
> > +   priv->chip = chip;
> > +   priv->gpio_chip = template_gpio;
> > +   priv->gpio_chip.parent = chip->dev;
> 
> I slightly prefer that you fill in the priv->gpio_chip with code (one 
> assignment
> per line) rather than assigning a template like here, but it's your pick.
> 
> > +   if (pdata && pdata->gpio_base)
> > +   priv->gpio_chip.base = pdata->gpio_base;
> 
> Give me any good reason to support this. Please just drop this platform data.
> Use -1 like in the template and get dynamic assignment of GPIO numbers.
> 
> Yours,
> Linus Walleij

Thank you for the comments and recommendation.
I will try to updated the code like your recommendation and make it simple.  
Then, I will send patch again.

Regards
Eric

RE: [PATCH V2 2/4] mfd: pv88080: MFD core support

[Eric Hyeung Dong Jeong]

On Friday, October 28, 2016 5:11 AM, Linux Walleij wrote:

> On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
>  wrote:
> 
> > From: Eric Jeong 
> >
> > This patch adds supports for PV88080 MFD core device.
> >
> > It provides communication through the I2C interface.
> > It contains the following components:
> > - Regulators
> > - Configurable GPIOs
> >
> > Kconfig and Makefile are updated to reflect support for PV88080 PMIC.
> >
> > Signed-off-by: Eric Jeong 
> (...)
> >  drivers/mfd/pv88080-i2c.c   |   99 
> 
> This looks like a pure I2C driver.
> 
> Why is it not in drivers/i2c/busses/i2c-pv88080.c?
> 
> Yours,
> Linus Walleij

Ok. I agree with you. So, I will send patch again after combining the relevant 
files into one.
Thank you.

Regards
Eric

RE: [PATCH V2 4/4] gpio: pv88080: Add GPIO function support

[Eric Hyeung Dong Jeong]

On Friday, October 28, 2016 9:11 PM, Linux Walleij wrote:

> On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
>  wrote:
> 
> > From: Eric Jeong 
> >
> > This patch adds support for PV88080 PMIC GPIOs.
> > PV88080 has two configurable GPIOs.
> >
> > Kconfig and Makefile are updated to reflect support for PV88080 PMIC
> > GPIO.
> >
> > Signed-off-by: Eric Jeong 
> (...)
> 
> > +static int pv88080_gpio_direction_input(struct gpio_chip *gc,
> > +   unsigned int offset) {
> > +   struct pv88080_gpio *priv = gpiochip_get_data(gc);
> > +   struct pv88080 *chip = priv->chip;
> > +   int ret;
> > +
> > +   /* Set the initial value */
> > +   ret = regmap_update_bits(chip->regmap, priv->gpio_base_reg + offset,
> > +   PV88080_GPIO_OUTPUT_MASK, 0);
> > +   if (ret)
> > +   return ret;
> 
> So you set the initial value when we change the pin to *input*...
> 
> > +
> > +   return regmap_update_bits(chip->regmap, priv->gpio_base_reg + 
> > offset,
> > +   PV88080_GPIO_DIRECTION_MASK, 0); }
> > +
> > +static int pv88080_gpio_direction_output(struct gpio_chip *gc,
> > +   unsigned int offset, int value) {
> > +   struct pv88080_gpio *priv = gpiochip_get_data(gc);
> > +   struct pv88080 *chip = priv->chip;
> > +   int ret;
> > +
> > +   ret = regmap_update_bits(chip->regmap, priv->gpio_base_reg + offset,
> > +   PV88080_GPIO_DIRECTION_MASK,
> > +   PV88080_GPIO_DIRECTION_MASK);
> 
> But do nothing when we change the pin to *output*?
> 
> It seems like you switched the two function implementations or something?
> 
> > +static int pv88080_gpio_get(struct gpio_chip *gc, unsigned int
> > +offset) {
> > +   struct pv88080_gpio *priv = gpiochip_get_data(gc);
> > +   struct pv88080 *chip = priv->chip;
> > +   unsigned int reg = 0, direction;
> > +   int ret;
> > +
> > +   ret = regmap_read(chip->regmap, priv->gpio_base_reg + offset, );
> > +   if (ret)
> > +   return ret;
> > +
> > +   direction = (reg & PV88080_GPIO_DIRECTION_MASK);
> > +   if (direction == PV88080_PORT_DIRECTION_OUTPUT) {
> > +   if (reg & PV88080_GPIO_OUTPUT_EN)
> > +   return 1;
> > +   ret = 0;
> > +   } else {
> > +   ret = regmap_read(chip->regmap, priv->input_reg, );
> > +   if (ret < 0)
> > +   return ret;
> > +   ret = (reg & (PV88080_GPIO_INPUT_MASK << offset)) >>
> > + offset;
> 
> Isn't this what you want to do?
> 
> #include 
> 
> ret = !!(reg & BIT(offset));
> 
> The mask is 0x01. No point in making things more complicated than they are.
> 
> 
> > +static void pv88080_gpio_set(struct gpio_chip *gc, unsigned int offset,
> > +   int value) {
> > +   struct pv88080_gpio *priv = gpiochip_get_data(gc);
> > +   struct pv88080 *chip = priv->chip;
> > +
> > +   if (value)
> > +   regmap_update_bits(chip->regmap, priv->gpio_base_reg + 
> > offset,
> > +   PV88080_GPIO_OUTPUT_MASK,
> > +   PV88080_GPIO_OUTPUT_EN);
> > +   else
> > +   regmap_update_bits(chip->regmap, priv->gpio_base_reg + 
> > offset,
> > +   PV88080_GPIO_OUTPUT_MASK,
> > +   PV88080_GPIO_OUTPUT_DIS); }
> 
> Looks good, output is more complicated.
> 
> > +static const struct gpio_chip template_gpio = {
> > +   .label = "pv88080-gpio",
> > +   .owner = THIS_MODULE,
> > +   .get_direction = pv88080_gpio_get_direction,
> > +   .direction_input = pv88080_gpio_direction_input,
> > +   .direction_output = pv88080_gpio_direction_output,
> > +   .get = pv88080_gpio_get,
> > +   .set = pv88080_gpio_set,
> > +   .base = -1,
> > +   .ngpio = DEFAULT_PIN_NUMBER,
> > +};
> 
> Why even have a #define for DEFAULT_PIN_NUMBER?
> Just hardcode it here.
> 
> > +   priv->chip = chip;
> > +   priv->gpio_chip = template_gpio;
> > +   priv->gpio_chip.parent = chip->dev;
> 
> I slightly prefer that you fill in the priv->gpio_chip with code (one 
> assignment
> per line) rather than assigning a template like here, but it's your pick.
> 
> > +   if (pdata && pdata->gpio_base)
> > +   priv->gpio_chip.base = pdata->gpio_base;
> 
> Give me any good reason to support this. Please just drop this platform data.
> Use -1 like in the template and get dynamic assignment of GPIO numbers.
> 
> Yours,
> Linus Walleij

Thank you for the comments and recommendation.
I will try to updated the code like your recommendation and make it simple.  
Then, I will send patch again.

Regards
Eric

RE: [PATCH V2 2/4] mfd: pv88080: MFD core support

[Eric Hyeung Dong Jeong]

On Friday, October 28, 2016 5:11 AM, Linux Walleij wrote:

> On Thu, Oct 27, 2016 at 3:03 AM, Eric Jeong
>  wrote:
> 
> > From: Eric Jeong 
> >
> > This patch adds supports for PV88080 MFD core device.
> >
> > It provides communication through the I2C interface.
> > It contains the following components:
> > - Regulators
> > - Configurable GPIOs
> >
> > Kconfig and Makefile are updated to reflect support for PV88080 PMIC.
> >
> > Signed-off-by: Eric Jeong 
> (...)
> >  drivers/mfd/pv88080-i2c.c   |   99 
> 
> This looks like a pure I2C driver.
> 
> Why is it not in drivers/i2c/busses/i2c-pv88080.c?
> 
> Yours,
> Linus Walleij

Ok. I agree with you. So, I will send patch again after combining the relevant 
files into one.
Thank you.

Regards
Eric

RE: [PATCH V2 3/4] regulator: pv88080: Update Regulator driver for MFD support

[Eric Hyeung Dong Jeong]

On Thursday, October 27, 2016 8:04 PM, Mark Brown wrote:

> On Thu, Oct 27, 2016 at 10:03:14AM +0900, Eric Jeong wrote:
> 
> >  config REGULATOR_PV88080
> > -   tristate "Powerventure Semiconductor PV88080 regulator"
> > -   depends on I2C
> > -   select REGMAP_I2C
> > +   bool "Powerventure Semiconductor PV88080 regulator"
> > +   depends on MFD_PV88080
> 
> Forcing the driver to be built in looks like a regression, why would we want 
> to
> do that?
> 
> > +   irq = platform_get_irq_byname(pdev, "regulator-irq");
> > +   if (irq < 0) {
> > +   dev_err(>dev, "Failed to get IRQ.\n");
> > +   return irq;
> > }
> 
> What's the _byname() adding here given that the name is so generic?  It feels
> like if the name ever becomes important then this particular name is going to
> be a problem.
> 
> > -module_i2c_driver(pv88080_regulator_driver);
> > +static int __init pv88080_regulator_init(void) {
> > +   return platform_driver_register(_regulator_driver);
> > +}
> > +subsys_initcall(pv88080_regulator_init);
> 
> Why are you converting this to subsys_initcall()?  This looks like another
> regression.

Thank you for the comments.
There was an internal request to release driver based on Kernel 3.18 for GPIO 
and MFD support.
After that, the code has been kept. I will send a patch again after changes.

Regards
Eric

RE: [PATCH V2 3/4] regulator: pv88080: Update Regulator driver for MFD support

[Eric Hyeung Dong Jeong]

On Thursday, October 27, 2016 8:04 PM, Mark Brown wrote:

> On Thu, Oct 27, 2016 at 10:03:14AM +0900, Eric Jeong wrote:
> 
> >  config REGULATOR_PV88080
> > -   tristate "Powerventure Semiconductor PV88080 regulator"
> > -   depends on I2C
> > -   select REGMAP_I2C
> > +   bool "Powerventure Semiconductor PV88080 regulator"
> > +   depends on MFD_PV88080
> 
> Forcing the driver to be built in looks like a regression, why would we want 
> to
> do that?
> 
> > +   irq = platform_get_irq_byname(pdev, "regulator-irq");
> > +   if (irq < 0) {
> > +   dev_err(>dev, "Failed to get IRQ.\n");
> > +   return irq;
> > }
> 
> What's the _byname() adding here given that the name is so generic?  It feels
> like if the name ever becomes important then this particular name is going to
> be a problem.
> 
> > -module_i2c_driver(pv88080_regulator_driver);
> > +static int __init pv88080_regulator_init(void) {
> > +   return platform_driver_register(_regulator_driver);
> > +}
> > +subsys_initcall(pv88080_regulator_init);
> 
> Why are you converting this to subsys_initcall()?  This looks like another
> regression.

Thank you for the comments.
There was an internal request to release driver based on Kernel 3.18 for GPIO 
and MFD support.
After that, the code has been kept. I will send a patch again after changes.

Regards
Eric

RE: [PATCH V1] mfd: pv88080: Expand driver for GPIO function support.

[Eric Hyeung Dong Jeong]

On Tuesday, October 25, 2016 3:41 PM, Lee Jones wrote:

> On Tue, 25 Oct 2016, Eric Jeong wrote:
> 
> >
> > From: Eric Jeong 
> >
> > This patch adds support for the PV88080 PMIC.
> >
> > This pathch is done as part of the existing PV88080 regulator driver
> > by expanding the driver for GPIO function support.
> >
> > The MFD core driver provides communication through the I2C interface.
> > and contains the following components:
> >
> > - Regulators
> > - Configurable GPIOs
> >
> > Kconfig and Makefile are updated to reflect support for PV88080 PMIC.
> >
> > Signed-off-by: Eric Jeong 
> >
> > ---
> > This patch applies against linux-next and next-20161024
> >
> > Hi,
> >
> > This change is made as a single patch. Because, to ensure that kernel
> > builds and runs properly after this patch.
> >
> > The regulator device driver for PV88080 IC is submitted to Linux kernel.
> > And now, GPIO function is required. In order to add GPIO driver, MFD
> > driver is also required.
> >
> > Changes
> >  - Add MFD driver.
> >  - Add GPIO driver.
> >  - Update regulator driver to reflect the support.
> >  - Delete pv88080-regulator.h file.
> >  - Move binding document to mfd directory.
> >
> > Regards,
> > Eric Jeong, Dialog Semiconductor Ltd.
> >
> >
> >  Documentation/devicetree/bindings/mfd/pv88080.txt  |   63 +
> >  .../devicetree/bindings/regulator/pv88080.txt  |   62 -
> >  drivers/gpio/Kconfig   |   11 +
> >  drivers/gpio/Makefile  |1 +
> >  drivers/gpio/gpio-pv88080.c|  195 ++
> >  drivers/mfd/Kconfig|   12 +
> >  drivers/mfd/Makefile   |2 +
> >  drivers/mfd/pv88080-core.c |  270 
> > 
> >  drivers/mfd/pv88080-i2c.c  |   99 +++
> >  drivers/regulator/Kconfig  |5 +-
> >  drivers/regulator/pv88080-regulator.c  |  202 ++-
> >  drivers/regulator/pv88080-regulator.h  |  118 -
> >  include/linux/mfd/pv88080.h|  236 +
> >  13 files changed, 970 insertions(+), 306 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/mfd/pv88080.txt
> >  delete mode 100644
> > Documentation/devicetree/bindings/regulator/pv88080.txt
> >  create mode 100644 drivers/gpio/gpio-pv88080.c  create mode 100644
> > drivers/mfd/pv88080-core.c  create mode 100644
> > drivers/mfd/pv88080-i2c.c  delete mode 100644
> > drivers/regulator/pv88080-regulator.h
> >  create mode 100644 include/linux/mfd/pv88080.h
> 
> You're going to need to split this patch up as much as possible.
> 
> No one is going to want to review a 1200 line patch.
> 
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog

Sorry for inconvenience.
I will separate the patch and send again.

Regards
Eric

RE: [PATCH V1] mfd: pv88080: Expand driver for GPIO function support.

[Eric Hyeung Dong Jeong]

On Tuesday, October 25, 2016 3:41 PM, Lee Jones wrote:

> On Tue, 25 Oct 2016, Eric Jeong wrote:
> 
> >
> > From: Eric Jeong 
> >
> > This patch adds support for the PV88080 PMIC.
> >
> > This pathch is done as part of the existing PV88080 regulator driver
> > by expanding the driver for GPIO function support.
> >
> > The MFD core driver provides communication through the I2C interface.
> > and contains the following components:
> >
> > - Regulators
> > - Configurable GPIOs
> >
> > Kconfig and Makefile are updated to reflect support for PV88080 PMIC.
> >
> > Signed-off-by: Eric Jeong 
> >
> > ---
> > This patch applies against linux-next and next-20161024
> >
> > Hi,
> >
> > This change is made as a single patch. Because, to ensure that kernel
> > builds and runs properly after this patch.
> >
> > The regulator device driver for PV88080 IC is submitted to Linux kernel.
> > And now, GPIO function is required. In order to add GPIO driver, MFD
> > driver is also required.
> >
> > Changes
> >  - Add MFD driver.
> >  - Add GPIO driver.
> >  - Update regulator driver to reflect the support.
> >  - Delete pv88080-regulator.h file.
> >  - Move binding document to mfd directory.
> >
> > Regards,
> > Eric Jeong, Dialog Semiconductor Ltd.
> >
> >
> >  Documentation/devicetree/bindings/mfd/pv88080.txt  |   63 +
> >  .../devicetree/bindings/regulator/pv88080.txt  |   62 -
> >  drivers/gpio/Kconfig   |   11 +
> >  drivers/gpio/Makefile  |1 +
> >  drivers/gpio/gpio-pv88080.c|  195 ++
> >  drivers/mfd/Kconfig|   12 +
> >  drivers/mfd/Makefile   |2 +
> >  drivers/mfd/pv88080-core.c |  270 
> > 
> >  drivers/mfd/pv88080-i2c.c  |   99 +++
> >  drivers/regulator/Kconfig  |5 +-
> >  drivers/regulator/pv88080-regulator.c  |  202 ++-
> >  drivers/regulator/pv88080-regulator.h  |  118 -
> >  include/linux/mfd/pv88080.h|  236 +
> >  13 files changed, 970 insertions(+), 306 deletions(-)  create mode
> > 100644 Documentation/devicetree/bindings/mfd/pv88080.txt
> >  delete mode 100644
> > Documentation/devicetree/bindings/regulator/pv88080.txt
> >  create mode 100644 drivers/gpio/gpio-pv88080.c  create mode 100644
> > drivers/mfd/pv88080-core.c  create mode 100644
> > drivers/mfd/pv88080-i2c.c  delete mode 100644
> > drivers/regulator/pv88080-regulator.h
> >  create mode 100644 include/linux/mfd/pv88080.h
> 
> You're going to need to split this patch up as much as possible.
> 
> No one is going to want to review a 1200 line patch.
> 
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog

Sorry for inconvenience.
I will separate the patch and send again.

Regards
Eric

RE: [PATCH V1] regulator: pv88080: Update regulator for PV88080 BB silicon support

[Eric Hyeung Dong Jeong]

On September 25, 2016 3:23 AM, Mark Brown wrote:

> On Wed, Sep 21, 2016 at 01:44:39PM +0900, Eric Jeong wrote:
> 
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id pv88080_dt_ids[] = {
> > +   { .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> > +   { .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> > +   {},
> > +};
> > +MODULE_DEVICE_TABLE(of, pv88080_dt_ids); #endif
> 
> This doesn't list the old compatible string and therefore ends up breaking
> since the code defaults to assuming something it hasn't heard of is BB when
> it seems more likely that old DTs using the old compatible string will be for
> boards that also use the old silicon.  It would be much better practice to
> explicitly list the old compatible string and how we handle it, that won't 
> break
> in future and avoids this kind of error.
> 
> Is it really not possible to read the chip revision from the device using ID
> registers?  That'd be even better.

The address of ID register is different between each chip revision.
So, It is difficult to read the chip revision from the device using ID register.
And, I will send a patch with old compatible.

RE: [PATCH V1] regulator: pv88080: Update regulator for PV88080 BB silicon support

[Eric Hyeung Dong Jeong]

On September 25, 2016 3:23 AM, Mark Brown wrote:

> On Wed, Sep 21, 2016 at 01:44:39PM +0900, Eric Jeong wrote:
> 
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id pv88080_dt_ids[] = {
> > +   { .compatible = "pvs,pv88080-aa", .data = (void *)TYPE_PV88080_AA },
> > +   { .compatible = "pvs,pv88080-ba", .data = (void *)TYPE_PV88080_BA },
> > +   {},
> > +};
> > +MODULE_DEVICE_TABLE(of, pv88080_dt_ids); #endif
> 
> This doesn't list the old compatible string and therefore ends up breaking
> since the code defaults to assuming something it hasn't heard of is BB when
> it seems more likely that old DTs using the old compatible string will be for
> boards that also use the old silicon.  It would be much better practice to
> explicitly list the old compatible string and how we handle it, that won't 
> break
> in future and avoids this kind of error.
> 
> Is it really not possible to read the chip revision from the device using ID
> registers?  That'd be even better.

The address of ID register is different between each chip revision.
So, It is difficult to read the chip revision from the device using ID register.
And, I will send a patch with old compatible.