Re: [PATCH V3 2/4] mfd: pv88080: MFD core support
On Fri, 25 Nov 2016, Eric Hyeung Dong Jeong wrote:
> 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
It's a good idea to cut out all of the code/comments that is either
not relevant, or you are not providing comment (besides "will do")
on.
> > > +struct pv88080 {
> > > + struct device *dev;
> > > + struct regmap *regmap;
> > > + unsigned long type;
> >
> > Does this really need to be in here?
>
> The *type* member is used for separating silicon type.
> And, regulator and gpio driver also use the member to check the type
> for proper configuration without additional code.
> That is the reason that the member is added in the structure.
I don't see how this is being used, so assuming the other Maintainers
are happy with the implementation it's okay for this to live here.
However, please consider changing to something better than "value" or
"type". Perhaps "varian"t or "model" or similar would be better?
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
RE: [PATCH V3 2/4] mfd: pv88080: MFD core support
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 2/4] mfd: pv88080: MFD core support
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).
> + * 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.
> +#include
This doesn't need to be separated from the rest.
> +#define PV88080_REG_EVENT_A_OFFSET 0
> +#define PV88080_REG_EVENT_B_OFFSET 1
> +#define PV88080_REG_EVENT_C_OFFSET 2
Spaces after 'define'.
> +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 = IORESOURCE_IRQ,
> + },
> +};
Use the DEFINE_RES_* macros.
> +static const struct mfd_cell pv88080_aa_cells[] = {
> + {
> + .name = "pv88080-regulator",
> + .num_resources = ARRAY_SIZE(regulators_aa_resources),
> + .resources = regulators_aa_resources,
> + .
