RE: [PATCH 1/2] platform/chrome: cros_ec_typec: Skip port partner check in configure_mux()

[Mani, Rajmohan]

Hi Prashant,

> -Original Message-
> From: Prashant Malani 
> Sent: Friday, February 05, 2021 12:07 PM
> To: Mani, Rajmohan 
> Cc: Benson Leung ; Enric Balletbo i Serra
> ; Guenter Roeck ;
> Linux Kernel Mailing List ; Heikki Krogerus
> 
> Subject: Re: [PATCH 1/2] platform/chrome: cros_ec_typec: Skip port partner
> check in configure_mux()
> 
> Hi Raj,
> 
> On Fri, Feb 5, 2021 at 11:52 AM Rajmohan Mani
>  wrote:
> >
> > For certain needs like updating the USB4 retimer firmware when no
> > device are connected, the Type-C ports require mux configuration, to
> > be able to communicate with the retimer. So removed the above check to
> > allow for mux configuration of Type-C ports, to enable retimer
> > communication.
> >
> > Signed-off-by: Rajmohan Mani 
> Reviewed-by: Prashant Malani 
> 

Thanks for the review of the patch series.

> > ---
> >  drivers/platform/chrome/cros_ec_typec.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c
> > b/drivers/platform/chrome/cros_ec_typec.c
> > index e724a5eaef1c..3d8ff3f8a514 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -536,9 +536,6 @@ static int cros_typec_configure_mux(struct
> cros_typec_data *typec, int port_num,
> > enum typec_orientation orientation;
> > int ret;
> >
> > -   if (!port->partner)
> > -   return 0;
> > -
> > if (mux_flags & USB_PD_MUX_POLARITY_INVERTED)
> > orientation = TYPEC_ORIENTATION_REVERSE;
> > else
> > --
> > 2.30.0
> >

RE: [PATCH 0/2] Add support for Type-C mux events without port partners

[Mani, Rajmohan]

Hi Benson,

> Subject: Re: [PATCH 0/2] Add support for Type-C mux events without port
> partners
> 
> Hi Rajmohan,
> 
> On Fri, 5 Feb 2021 11:51:11 -0800, Rajmohan Mani wrote:
> > There are cases, where support for Type-C mux events is needed, that
> > does not have port partners.
> > Enabling communication to a retimer connected to an USB4 port, when no
> > devices are attached, is a case that requires support for handling
> > Type-C mux events without port partners.
> >
> > The following patches[1] are needed on top of the mainline kernel to
> > be able to verify these patches.
> >
> > [...]
> 
> Applied, thanks!
> 

Thanks for the quick review / follow up.

...

RE: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Mani, Rajmohan]

Hi Greg,

> Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager
> (IOM) driver
> 
> On Fri, Aug 28, 2020 at 03:20:22PM +, Mani, Rajmohan wrote:
> > Hi Greg,
> >
> > > Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output
> > > Manager
> > > (IOM) driver
> > >
> > > Hi Greg,
> > >
> > > On Fri, Aug 28, 2020 at 09:43:59AM +0200, Greg Kroah-Hartman wrote:
> > > > I still find this crazy that a whole separate driver is created
> > > > just to read a single 32bit value.
> > > >
> > > > Why not put this logic in the driver that wants to read that value?
> > > > That would be much simpler, smaller, and more obvious.
> > >
> > > That would mean that we start maintaining something like DMI quirk
> > > table in those drivers. Unfortunately the IOM device is not available on
> every platform.
> > > Also, even on platforms that do have it, there is no guarantee that
> > > the device is always going to be mapped to the same address.
> > >
> > > Nevertheless, I was originally hoping that we could hide the
> > > handling of IOM somehow in ACPI without the need for an actual
> > > device object, but it now turns out that the other features of the
> > > IOM chip have created interest. At least our i915 guys probable have
> > > some use for it (I don't know exactly what they are planning to use it 
> > > for).
> > >
> > > So the fact that we may later need the device for something else, on
> > > top of the clumsiness and most importantly risks involved with using
> > > ACPI to take care of extra tasks (ASL tends to have bugs - bugs that
> > > may never ever get fixed), I think the IOM device object, and the
> > > driver that binds to it, do have a valid reason for existing.
> > >
> >
> > Intel PMC USB mux device is part of the PCH, while IOM is part of the SoC.
> 
> I have no idea what a "PCH" is, what "IOM" is, and how any of this relates to 
> a
> "SoC" :)
> 

I was just meaning to say IOM (Intel Output Manager) is a separate device, that
is not part of PCH (Platform Controller Hub) like PMC (Power Management 
Controller).

For the sake of completeness

PCH - Platform Controller Hub (usually that handles I/Os in Intel core 
platforms)
IOM - Input Output Manager (IOM) is part of the Tiger Lake SoC that handles 
Type-C
topology, configuration and PM functions of various Type-C devices connected
on the platform

> Don't impose arbritrary hardware "splits" to kernel code when the kernel has
> no such "partitioning" please.
> 

Ack.

> > This was another reason we had to have a separate ACPI device.
> 
> That sounds like a firmware issue you can solve in UEFI.
> 

Ack

> I think this is the most TLA-laden email I have ever written, and I used to 
> work
> at IBM :)

I thought it was only Intel where TLAs are abundantly used.

Thanks for the reviews and the direction on this topic.

> 
> greg k-h

RE: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Mani, Rajmohan]

> Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager
> (IOM) driver
> 
> On Fri, Aug 28, 2020 at 12:03:35PM +0200, Greg Kroah-Hartman wrote:
> > Handle the situation today, if, in the future, someone else
> > needs/wants this, _then_ work on splitting it out into separate
> > pieces.  Don't create additional complexity today, for no benefit
> > today.  It's already caused numerous review comments/complaints the way
> this is designed...
> 
> OK. We'll handle this in the mux driver for now.
> 

Ack. Will work with Heikki on this.

> 
> thanks,
> 
> --
> heikki

RE: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Mani, Rajmohan]

Hi Greg,

> Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager
> (IOM) driver
> 
> Hi Greg,
> 
> On Fri, Aug 28, 2020 at 09:43:59AM +0200, Greg Kroah-Hartman wrote:
> > I still find this crazy that a whole separate driver is created just
> > to read a single 32bit value.
> >
> > Why not put this logic in the driver that wants to read that value?
> > That would be much simpler, smaller, and more obvious.
> 
> That would mean that we start maintaining something like DMI quirk table in
> those drivers. Unfortunately the IOM device is not available on every 
> platform.
> Also, even on platforms that do have it, there is no guarantee that the 
> device is
> always going to be mapped to the same address.
> 
> Nevertheless, I was originally hoping that we could hide the handling of IOM
> somehow in ACPI without the need for an actual device object, but it now
> turns out that the other features of the IOM chip have created interest. At
> least our i915 guys probable have some use for it (I don't know exactly what
> they are planning to use it for).
> 
> So the fact that we may later need the device for something else, on top of 
> the
> clumsiness and most importantly risks involved with using ACPI to take care of
> extra tasks (ASL tends to have bugs - bugs that may never ever get fixed), I
> think the IOM device object, and the driver that binds to it, do have a valid
> reason for existing.
> 

Intel PMC USB mux device is part of the PCH, while IOM is part of the SoC.
This was another reason we had to have a separate ACPI device.

> 
> thanks,
> 
> --
> heikki

RE: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Mani, Rajmohan]

Hi Prashant,

...

> > > > +
> > > > +   reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN *
> > > port;
> > > > +
> > > > +   *status = ioread32(reg);
> > >
> > > Perhaps just inline reg within the parentheses?
> >
> > Kept this way to increase readability. Let me know if you feel
> > strongly towards inline reg.
> 
> I'd rather this be inlined, you save a couple lines from the variable 
> declaration,
> and IMO we're not gaining much in terms of readability by declaring this
> separately.
> 

Ack
(at least to me, it was more readable)

...

> >
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(intel_iom_port_status);
> > > > +
> > > > +static int intel_iom_probe(struct platform_device *pdev) {
> > > > +   void __iomem *addr;
> > > > +
> > > > +   /* only one IOM device is supported */
> > >
> > > Minor nit: s/only/Only
> >
> > And then I may need to end the comment with a period.
> > Let me know if you feel strongly.
> Yes, let's capitalize and add the period. Let's try to use the right 
> punctuation
> where possible.
> 

Ack
Will take care of this as part of v3.

RE: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager (IOM) driver

[Mani, Rajmohan]

Hi Prashant,

Thanks for the quick review.

> Subject: Re: [PATCH v2 1/3] platform/x86: Add Intel Input Output Manager
> (IOM) driver
> 
> Hi Rajmohan,
> 
> On Fri, Aug 21, 2020 at 09:05:06PM -0700, Rajmohan Mani wrote:
> > Input Output Manager (IOM) is part of the Tiger Lake SoC that
> > configures the Type-C Sub System (TCSS). IOM is a micro controller
> > that handles Type-C topology, configuration and PM functions of
> > various Type-C devices connected on the platform.
> >
> > This driver helps read relevant information such as Type-C port status
> > (whether a device is connected to a Type-C port or not) and the
> > activity type on the Type-C ports (such as USB, Display Port,
> > Thunderbolt), for consumption by other drivers.
> >
> > Currently intel_iom_port_status() API is exported by this driver, that
> > has information about the Type-C port status and port activity type.
> >
> > Signed-off-by: Rajmohan Mani 
> > ---
> 
> Perhaps include a version log of changes since v1?

Yes. It's there in the cover letter (patch v2 0/3).

> > diff --git a/drivers/platform/x86/intel_iom.c
> > b/drivers/platform/x86/intel_iom.c
> > new file mode 100644
> > index ..cda7716410c6
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel_iom.c
> > +int intel_iom_port_status(u8 port, u32 *status) {
> > +   void __iomem *reg;
> > +
> > +   if (!iom || !iom->dev || !iom->regbar)
> 
> Do we need to check for !iom->dev and !iom->regbar?

It's a good practice to have sanity checks on pointer members dereferenced.

So I can lose the check on iom->dev, but prefer to keep the check on regbar.
Let me know if you feel strongly about losing the check for regbar as well.

> Is there a valid situation
> where iom != NULL but iom->dev and/or iom->regbar == NULL?
> Sounds like it shouldn't, but I may be missing something.
> 

I think I am being conservative here.

> > +   return -ENODEV;
> > +
> > +   if (!status || (port > IOM_MAX_PORTS - 1))
> 
> I think parentheses around "port > IOM_MAX_PORT - 1" aren't required.

Ack

> > +   return -EINVAL;
> > +
> > +   reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN *
> port;
> > +
> > +   *status = ioread32(reg);
> 
> Perhaps just inline reg within the parentheses?

Kept this way to increase readability. Let me know if you feel strongly towards
inline reg.

> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(intel_iom_port_status);
> > +
> > +static int intel_iom_probe(struct platform_device *pdev) {
> > +   void __iomem *addr;
> > +
> > +   /* only one IOM device is supported */
> 
> Minor nit: s/only/Only

And then I may need to end the comment with a period.
Let me know if you feel strongly.

> > +   if (iom)
> > +   return -EBUSY;
> > +
> > +   iom = devm_kzalloc(>dev, sizeof(*iom), GFP_KERNEL);
> > +   if (!iom)
> > +   return -ENOMEM;
> > +
> > +   addr = devm_platform_ioremap_resource(pdev, 0);
> > +   if (IS_ERR(addr))
> > +   return PTR_ERR(addr);
> > +
> > +   iom->regbar = addr;
> > +   iom->dev = >dev;
> > +
> > +   return 0;
> > +}
> > +
> > +static const struct acpi_device_id intel_iom_acpi_ids[] = {
> > +   { "INTC1072" },
> > +   {}
> > +};
> > +MODULE_DEVICE_TABLE(acpi, intel_iom_acpi_ids);
> > +
> > +static struct platform_driver intel_iom_driver = {
> > +   .probe = intel_iom_probe,
> 
> nit: I generally see ".probe" listed below ".driver".

Ack

> > +   .driver = {
> > +   .name = "intel_iom",
> > +   .acpi_match_table = intel_iom_acpi_ids,
> > +   },
> > +};
> > +
> > +module_platform_driver_probe(intel_iom_driver, intel_iom_probe);
> > +
> > +MODULE_AUTHOR("Rajmohan Mani ");
> > +MODULE_DESCRIPTION("Intel IOM driver"); MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/platform_data/x86/intel_iom.h
> > b/include/linux/platform_data/x86/intel_iom.h
> > new file mode 100644
> > index ..e4c9a305e7a9
> > --- /dev/null
> > +++ b/include/linux/platform_data/x86/intel_iom.h
> > @@ -0,0 +1,49 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#ifndef _PLATFORM_DATA_X86_INTEL_IOM_H_ #define
> > +_PLATFORM_DATA_X86_INTEL_IOM_H_
> > +
> > +
> > +#define IOM_MAX_PORTS  4
> > +/* Register length in bytes */
> > +#define IOM_REG_LEN4
> 
> Do these two #define's need to be in the header, instead of directly in
> intel_iom.c ?
> 

Ack. These 2 can be moved to .c file.

> > +
> > +#ifdef CONFIG_ACPI
> > +
> > +int intel_iom_port_status(u8 port, u32 *status);
> > +
> > +#else
> > +
> > +int intel_iom_port_status(struct intel_iom *iom, u8 port, u32
> > +*status)
> 
> Should the function signature be the same as the #ifdef case?
> 

Thanks for catching this. I missed it.

RE: [PATCH 1/2] platform/x86: Add Intel Input Output Manager (IOM) driver

[Mani, Rajmohan]

Hi Greg,

> Subject: Re: [PATCH 1/2] platform/x86: Add Intel Input Output Manager (IOM)
> driver

...

> > > > +struct intel_iom {
> > > > +   struct device *dev;
> > > > +   void __iomem *regbar;
> > > > +};
> > > > +
> > > > +static struct intel_iom iom_dev;
> > >
> > > Why just one?  Why is this static?
> > >
> >
> > There is just one IOM device in the system.
> 
> For today, yes, no need to force yourself to have to change this in the 
> future.
> Just use a normal per-instance variable instead please, if you really need it.
> 

Ack

> > > > +
> > > > +   /* Prevent this driver from being unloaded while in use */
> > > > +   if (!try_module_get(dev->driver->owner)) {
> > >
> > > Why are you poking around in a random device's driver's owner?
> > >
> > > That's not ok.  And probably totally racy.
> > >
> > > Handle module reference counts properly, not like this.
> > >
> >
> > Ack. Will use THIS_MODULE here.
> 
> No, that is not the answer, and is always wrong to use :(
> 

This should not matter anymore, as I find reference counting may not be needed.

> > > And why does it even matter that you incremented the module
> > > reference count?  What is that "protecting" you from?
> > >
> >
> > To prevent this driver from being unloaded, while it is being used.
> 
> Why does that matter?  Shouldn't normal reference counting and
> dependancies be all that you need?
> 

I find just dependencies are enough to prevent the driver from being unloaded.

With Intel PMC USB mux control driver, not using 
intel_iom_get()/intel_iom_put(),
just invoking intel_iom_port_status() is enough for rmmod to report failure
(citing the use by intel_pmc_mux) in unloading the IOM driver.

> > > > +   put_device(iom_dev.dev);
> > > > +   return ERR_PTR(-EBUSY);
> > > > +   }
> > > > +
> > > > +   return _dev;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(intel_iom_get);
> > >
> > > Who calls this function?
> > >
> >
> > Intel PMC USB mux control driver, in this case.
> > The callers are expected to call intel_iom_get() before using the
> > intel_iom_port_status(), after which intel_iom_put() can be called to
> > release the IOM device instance.
> 
> Why not just have a single call if all this driver does is support one thing. 
>  The
> reference counting shouldn't be needed at all, right?
> 

Ack. That looks to be the case, based on my findings.

> > > > +/**
> > > > + * intel_iom_put() - Put IOM device instance
> > > > + * @iom: IOM device instance
> > > > + *
> > > > + * This function releases the IOM device instance created using
> > > > + * intel_iom_get() and allows the driver to be unloaded.
> > > > + *
> > > > + * Call intel_iom_put() to release the instance.
> > > > + */
> > > > +void intel_iom_put(struct intel_iom *iom) {
> > > > +   if (!iom)
> > > > +   return;
> > > > +
> > > > +   module_put(iom->dev->driver->owner);
> > >
> > > And if the device doesn't have a driver?  boom :(
> > >
> > > Don't do this.
> > >
> >
> > Ack. Will use THIS_MODULE here.
> 
> Again, no, that will be even more incorrect.
> 

This shouldn't be relevant anymore.

> > > > +   put_device(iom->dev);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(intel_iom_put);
> > > > +
> > > > +/**
> > > > + * intel_iom_port_status() - Get status bits for the Type-C port
> > > > + * @iom: IOM device instance
> > > > + * @port: Type-C port number
> > > > + * @status: pointer to receive the status bits
> > > > + *
> > > > + * Returns 0 on success, error otherwise.
> > > > + */
> > > > +int intel_iom_port_status(struct intel_iom *iom, u8 port, u32
> > > > +*status) {
> > > > +   void __iomem *reg;
> > > > +
> > > > +   if (!iom)
> > > > +   return -ENODEV;
> > > > +
> > > > +   if (!status || (port > IOM_MAX_PORTS - 1))
> > > > +   return -EINVAL;
> > > > +
> > > > +   reg = iom->regbar + IOM_PORT_STATUS_OFFSET + IOM_REG_LEN *
> > > port;
> > > > +
> > > > +   *status = ioread32(reg);
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(intel_iom_port_status);
> > >
> > > So the whole driver is here just to read, directly from memory, a
> > > single
> > > 32 bit value?
> >
> > Yes.
> 
> Ok, then this whole driver could be about 90% smaller and more obvious.
> Don't add the reference counting, the static variables and all the other stuff
> just to get a 32bit number.
> 

Ack

RE: [PATCH 2/2] usb: typec: intel_pmc_mux: Check the port status before connect

[Mani, Rajmohan]

Hi Greg,

> -Original Message-
> From: Greg Kroah-Hartman 
> Sent: Thursday, July 16, 2020 12:05 AM
> To: Mani, Rajmohan 
> Cc: Darren Hart ; Andy Shevchenko
> ; Mika Westerberg
> ; Dmitry Torokhov
> ; Lee Jones ; Ayman
> Bagabas ; Masahiro Yamada
> ; Joseph, Jithu ; Blaž
> Hrastnik ; Srinivas Pandruvada
> ; linux-kernel@vger.kernel.org;
> platform-driver-...@vger.kernel.org; Heikki Krogerus
> ; linux-...@vger.kernel.org;
> pmal...@chromium.org; ble...@chromium.org
> Subject: Re: [PATCH 2/2] usb: typec: intel_pmc_mux: Check the port status
> before connect
> 
> On Wed, Jul 15, 2020 at 05:33:10PM -0700, Rajmohan Mani wrote:
> > From: Heikki Krogerus 
> >
> > The PMC microcontroller that we use for configuration, does not supply
> > any status information back. For port status we need to talk to
> > another controller on the board called IOM (I/O manager).
> >
> > By checking the port status before configuring the muxes, we can make
> > sure that we do not reconfigure the port after bootup when the system
> > firmware (for example BIOS) has already configured it.
> >
> > Using the status information also to check if DisplayPort HPD is still
> > asserted when the cable plug is disconnected, and clearing it if it
> > is.
> >
> > Signed-off-by: Heikki Krogerus 
> 
> You can't just forward on patches from others without also adding your
> signed-off-by to them, right?
> 

Sorry I missed this.

> Please fix up this series and try again.
> 

Ack. Will fix this with v2.

> thanks,
> 
> greg k-h

RE: [PATCH 1/2] platform/x86: Add Intel Input Output Manager (IOM) driver

[Mani, Rajmohan]

Hi Greg,

Thanks for the reviews.

> -Original Message-
> From: Greg Kroah-Hartman 
> Sent: Thursday, July 16, 2020 12:10 AM
> To: Mani, Rajmohan 
> Cc: Darren Hart ; Andy Shevchenko
> ; Mika Westerberg
> ; Dmitry Torokhov
> ; Lee Jones ; Ayman
> Bagabas ; Masahiro Yamada
> ; Joseph, Jithu ; Blaž
> Hrastnik ; Srinivas Pandruvada
> ; linux-kernel@vger.kernel.org;
> platform-driver-...@vger.kernel.org; Heikki Krogerus
> ; linux-...@vger.kernel.org;
> pmal...@chromium.org; ble...@chromium.org
> Subject: Re: [PATCH 1/2] platform/x86: Add Intel Input Output Manager (IOM)
> driver
> 
> On Wed, Jul 15, 2020 at 05:33:09PM -0700, Rajmohan Mani wrote:
> > Input Output Manager (IOM) is part of the Tiger Lake SoC that
> > configures the Type-C Sub System (TCSS). IOM is a micro controller
> > that handles Type-C topology, configuration and PM functions of
> > various Type-C devices connected on the platform.
> >
> > This driver helps read relevant information such as Type-C port status
> > (whether a device is connected to a Type-C port or not) and the
> > activity type on the Type-C ports (such as USB, Display Port,
> > Thunderbolt), for consumption by other drivers.
> >
> > Currently intel_iom_port_status() API is exported by this driver, that
> > has information about the Type-C port status and port activity type.
> >
> > Signed-off-by: Rajmohan Mani 
> > ---
> >  drivers/platform/x86/Kconfig|  16 +++
> >  drivers/platform/x86/Makefile   |   1 +
> >  drivers/platform/x86/intel_iom.c| 133 
> >  include/linux/platform_data/x86/intel_iom.h |  62 +
> 
> Why do you need a .h file for a single .c file that no one else shares this 
> data?
> Just put it all in the .c file please.
> 

The APIs exported by this driver, are used by the caller (Intel PMC USB mux
control driver), hence the need for header file.

> >  4 files changed, 212 insertions(+)
> >  create mode 100644 drivers/platform/x86/intel_iom.c  create mode
> > 100644 include/linux/platform_data/x86/intel_iom.h
> >
> > diff --git a/drivers/platform/x86/Kconfig
> > b/drivers/platform/x86/Kconfig index 0581a54cf562..271feddb20ef 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -816,6 +816,22 @@ config INTEL_INT0002_VGPIO
> >   To compile this driver as a module, choose M here: the module will
> >   be called intel_int0002_vgpio.
> >
> > +config INTEL_IOM
> > +   tristate "Intel Input Output Manager (IOM) driver"
> > +   depends on ACPI && PCI
> > +   help
> > + This driver helps read relevant information such as Type-C port
> > + status (whether a device is connected to a Type-C port or not)
> > + and the activity type on the Type-C ports (such as USB, Display
> > + Port, Thunderbolt), for consumption by other drivers.
> > +
> > + Currently intel_iom_port_status() API is exported by this driver,
> > + that has information about the Type-C port status and port activity
> > + type.
> > +
> > + To compile this driver as a module, choose M here: the module will
> > + be called intel_iom.
> > +
> >  config INTEL_MENLOW
> > tristate "Thermal Management driver for Intel menlow platform"
> > depends on ACPI_THERMAL
> > diff --git a/drivers/platform/x86/Makefile
> > b/drivers/platform/x86/Makefile index 2b85852a1a87..d71e4620a7c6
> > 100644
> > --- a/drivers/platform/x86/Makefile
> > +++ b/drivers/platform/x86/Makefile
> > @@ -76,6 +76,7 @@ intel_cht_int33fe-objs:=
> intel_cht_int33fe_common.o \
> >intel_cht_int33fe_microb.o
> >  obj-$(CONFIG_INTEL_HID_EVENT)  += intel-hid.o
> >  obj-$(CONFIG_INTEL_INT0002_VGPIO)  += intel_int0002_vgpio.o
> > +obj-$(CONFIG_INTEL_IOM)+= intel_iom.o
> >  obj-$(CONFIG_INTEL_MENLOW) += intel_menlow.o
> >  obj-$(CONFIG_INTEL_OAKTRAIL)   += intel_oaktrail.o
> >  obj-$(CONFIG_INTEL_VBTN)   += intel-vbtn.o
> > diff --git a/drivers/platform/x86/intel_iom.c
> > b/drivers/platform/x86/intel_iom.c
> > new file mode 100644
> > index ..ece0fe720b2d
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel_iom.c
> > @@ -0,0 +1,133 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Intel Core SoC Input Output Manager (IOM) driver.
> > + *
> > + * This driver provides access to the Input Output Manager (IOM)
&

RE: [RFC PATCH 19/22] thunderbolt: Add support for Time Management Unit

[Mani, Rajmohan]

Hi Mika,

> -Original Message-
> From: Mika Westerberg [mailto:mika.westerb...@linux.intel.com]
> Sent: Tuesday, October 01, 2019 4:38 AM
> To: linux-...@vger.kernel.org
> Cc: Andreas Noever ; Jamet, Michael
> ; Mika Westerberg
> ; Yehezkel Bernat
> ; Mani, Rajmohan ;
> Nicholas Johnson ; Lukas
> Wunner ; Greg Kroah-Hartman
> ; Alan Stern ;
> mario.limoncie...@dell.com; Anthony Wong
> ; linux-kernel@vger.kernel.org
> Subject: [RFC PATCH 19/22] thunderbolt: Add support for Time Management
> Unit
> 
> From: Rajmohan Mani 
> 
> Time Management Unit (TMU) is included in each USB4 router. It is used to
> synchronize time across the USB4 fabric. By default when USB4 router is
> plugged to the domain, its TMU is turned off. This differs from Thunderbolt 
> (1,
> 2 and 3) devices whose TMU is by default configured to bi-directional HiFi
> mode. Since time synchronization is needed for proper Display Port tunneling
> this means we need to configure the TMU on
> USB4 compliant devices.
> 
> The USB4 spec allows some flexibility on how the TMU can be configured.
> This makes it possible to enable link power management states (CLx) in certain
> topologies, where for example DP tunneling is not used. TMU can also be re-
> configured dynamicaly depending on types of tunnels created over the USB4
> fabric.
> 
> In this patch we simply configure the TMU to be in bi-directional HiFi mode.
> This way we can tunnel any kind of traffic without need to perform complex
> steps to re-configure the domain dynamically. We can add more fine-grained
> TMU configuration later on when we start enabling CLx states.
> 
> Signed-off-by: Rajmohan Mani 
> Co-developed-by: Mika Westerberg 
> Signed-off-by: Mika Westerberg 
> ---
>  drivers/thunderbolt/Makefile  |   2 +-
>  drivers/thunderbolt/switch.c  |   4 +
>  drivers/thunderbolt/tb.c  |  29 +++
>  drivers/thunderbolt/tb.h  |  47 +
>  drivers/thunderbolt/tb_regs.h |  20 ++
>  drivers/thunderbolt/tmu.c | 380 ++
>  6 files changed, 481 insertions(+), 1 deletion(-)  create mode 100644
> drivers/thunderbolt/tmu.c
> 
> diff --git a/drivers/thunderbolt/Makefile b/drivers/thunderbolt/Makefile index
> c0b2fd73dfbd..2014bc840b06 100644
> --- a/drivers/thunderbolt/Makefile
> +++ b/drivers/thunderbolt/Makefile
> @@ -1,4 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only  obj-${CONFIG_THUNDERBOLT} :=
> thunderbolt.o  thunderbolt-objs := nhi.o nhi_ops.o ctl.o tb.o switch.o cap.o
> path.o tunnel.o eeprom.o -thunderbolt-objs += domain.o dma_port.o icm.o
> property.o xdomain.o lc.o usb4.o
> +thunderbolt-objs += domain.o dma_port.o icm.o property.o xdomain.o lc.o
> +tmu.o usb4.o
> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c index
> 2ccd1004920e..58e3f54ddbb9 100644
> --- a/drivers/thunderbolt/switch.c
> +++ b/drivers/thunderbolt/switch.c
> @@ -2278,6 +2278,10 @@ int tb_switch_add(struct tb_switch *sw)
>   ret = tb_switch_update_link_attributes(sw);
>   if (ret)
>   return ret;
> +
> + ret = tb_switch_tmu_init(sw);
> + if (ret)
> + return ret;
>   }
> 
>   ret = device_add(>dev);
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c index
> 24e37e47dc48..f2868c125637 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -161,6 +161,25 @@ static void tb_scan_xdomain(struct tb_port *port)
>   }
>  }
> 
> +static int tb_enable_tmu(struct tb_switch *sw) {
> + int ret;
> +
> + /* If it is already enabled in correct mode, don't touch it */
> + if (tb_switch_tmu_is_enabled(sw))
> + return 0;
> +
> + ret = tb_switch_tmu_disable(sw);
> + if (ret)
> + return ret;
> +
> + ret = tb_switch_tmu_post_time(sw);
> + if (ret)
> + return ret;
> +
> + return tb_switch_tmu_enable(sw);
> +}
> +
>  static void tb_scan_port(struct tb_port *port);
> 
>  /**
> @@ -263,6 +282,9 @@ static void tb_scan_port(struct tb_port *port)
>   if (tb_switch_lane_bonding_enable(sw))
>   tb_sw_warn(sw, "failed to enable lane bonding\n");
> 
> + if (tb_enable_tmu(sw))
> + tb_sw_warn(sw, "failed to enable TMU\n");
> +
>   tb_scan_switch(sw);
>  }
> 
> @@ -713,6 +735,7 @@ static void tb_handle_hotplug(struct work_struct
> *work)
>   tb_sw_set_unplugged(port->remote->sw);
>   tb_free_invalid_tunnels(tb);
>   tb_remove_dp_resources(port->remote->sw);
> +

RE: [PATCH] media: staging/intel-ipu3: Implement lock for stream on/off operations

[Mani, Rajmohan]

Hi Sakari,

[snip]

> > > > >  fail_stop_pipeline:
> > > > > @@ -543,6 +546,8 @@ static void ipu3_vb2_stop_streaming(struct
> > > > vb2_queue *vq)
> > > > >   dev_err(>pci_dev->dev,
> > > > >   "failed to stop subdev streaming\n");
> > > > >
> > > > > + mutex_lock(>streaming_lock);
> > > > > +
> > > > >   /* Was this the first node with streaming disabled? */
> > > > >   if (imgu->streaming && ipu3_all_nodes_streaming(imgu, node)) {
> > > > >   /* Yes, really stop streaming now */ @@ -552,6 +557,7 @@
> > > > static
> > > > > void ipu3_vb2_stop_streaming(struct vb2_queue *vq)
> > > > >   imgu->streaming = false;
> > > > >   }
> > > > >
> > > > > + mutex_unlock(>streaming_lock);
> > > > >   ipu3_return_all_buffers(imgu, node, VB2_BUF_STATE_ERROR);
> >
> > I'd also call ipu3_return_all_buffers() before releasing the lock: in
> > principle the user may have queued new buffers on the devices before
> > the driver marks the buffers as faulty.
> >

Ack
(Somehow I missed this comment.)

[snip]

RE: [PATCH] media: staging/intel-ipu3: Implement lock for stream on/off operations

[Mani, Rajmohan]

Hi Sakari,

> Subject: Re: [PATCH] media: staging/intel-ipu3: Implement lock for stream
> on/off operations
> 
> Hi Raj,
> 
> On Wed, Jan 30, 2019 at 05:17:15PM +0000, Mani, Rajmohan wrote:
> > Hi Sakari,
> >
> > > -Original Message-
> > > From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> > > Sent: Wednesday, January 30, 2019 12:59 AM
> > > To: Mani, Rajmohan 
> > > Cc: Mauro Carvalho Chehab ; Greg Kroah-Hartman
> > > ; linux-me...@vger.kernel.org;
> > > de...@driverdev.osuosl.org; linux-kernel@vger.kernel.org; Laurent
> > > Pinchart ; Jacopo Mondi
> > > ; Qiu, Tian Shu ; Cao,
> > > Bingbu ; z...@paasikivi.fi.intel.com; Zhi, Yong
> > > ; hverk...@xs4all.nl; tf...@chromium.org
> > > Subject: Re: [PATCH] media: staging/intel-ipu3: Implement lock for
> > > stream on/off operations
> > >
> > > Hi Rajmohan,
> > >
> > > On Tue, Jan 29, 2019 at 02:27:36PM -0800, Rajmohan Mani wrote:
> > > > Currently concurrent stream off operations on ImgU nodes are not
> > > > synchronized, leading to use-after-free bugs (as reported by KASAN).
> > > >
> > > > [  250.090724] BUG: KASAN: use-after-free in
> > > > ipu3_dmamap_free+0xc5/0x116 [ipu3_imgu] [  250.090726] Read of
> > > > size 8 at addr 888127b29bc0 by task yavta/18836 [  250.090731]
> > > > Hardware
> > > > name: HP Soraka/Soraka, BIOS Google_Soraka.10431.17.0 03/22/2018 [
> > > 250.090732] Call Trace:
> > > > [  250.090735]  dump_stack+0x6a/0xb1 [  250.090739]
> > > > print_address_description+0x8e/0x279
> > > > [  250.090743]  ? ipu3_dmamap_free+0xc5/0x116 [ipu3_imgu] [
> > > > 250.090746]  kasan_report+0x260/0x28a [  250.090750]
> > > > ipu3_dmamap_free+0xc5/0x116 [ipu3_imgu] [  250.090754]
> > > > ipu3_css_pool_cleanup+0x24/0x37 [ipu3_imgu] [  250.090759]
> > > > ipu3_css_pipeline_cleanup+0x61/0xb9 [ipu3_imgu] [  250.090763]
> > > > ipu3_css_stop_streaming+0x1f2/0x321 [ipu3_imgu] [  250.090768]
> > > > imgu_s_stream+0x94/0x443 [ipu3_imgu] [  250.090772]  ?
> > > > ipu3_vb2_buf_queue+0x280/0x280 [ipu3_imgu] [  250.090775]  ?
> > > > vb2_dma_sg_unmap_dmabuf+0x16/0x6f [videobuf2_dma_sg] [
> > > > 250.090778]
> > > ?
> > > > vb2_buffer_in_use+0x36/0x58 [videobuf2_common] [  250.090782]
> > > > ipu3_vb2_stop_streaming+0xf9/0x135 [ipu3_imgu]
> > > >
> > > > Implemented a lock to synchronize imgu stream on / off operations
> > > > and the modification of streaming flag (in struct imgu_device), to
> > > > prevent these issues.
> > > >
> > > > Reported-by: Laurent Pinchart 
> > > > Suggested-by: Laurent Pinchart 
> > > >
> > > > Signed-off-by: Rajmohan Mani 
> > > > ---
> > > >  drivers/staging/media/ipu3/ipu3-v4l2.c | 6 ++
> > > >  drivers/staging/media/ipu3/ipu3.c  | 3 +++
> > > >  drivers/staging/media/ipu3/ipu3.h  | 4 
> > > >  3 files changed, 13 insertions(+)
> > > >
> > > > diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c
> > > > b/drivers/staging/media/ipu3/ipu3-v4l2.c
> > > > index c7936032beb9..cf7e917cd0c8 100644
> > > > --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> > > > +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> > > > @@ -507,12 +507,15 @@ static int ipu3_vb2_start_streaming(struct
> > > vb2_queue *vq, unsigned int count)
> > > > goto fail_stop_pipeline;
> > > > }
> > > >
> > > > +   mutex_lock(>streaming_lock);
> > > > +
> > >
> > > You appear to be using imgu_device.lock (while searching buffers to
> > > queue to the device) as well as imgu_video_device.lock (qbuf, dqbuf)
> > > to serialise access to imgu_video_device.buffers list.
> >
> > Ack
> >
> > > The two locks may be acquired at the same time but each by different
> > > processes. That needs to be addressed, but probably not in this
> > > patch.
> > >
> >
> > The node specific locks will be used by different processes and all of
> > these processes will be competing commonly (and successfully) for the
> imgu_device lock.
> > I will look into this more.
> >
> > > I wonder if it'd be more simple to use imgu->lock here instead of
> > > adding a new one.
> > >
> >
> > Extending img

RE: [PATCH] media: staging/intel-ipu3: Implement lock for stream on/off operations

[Mani, Rajmohan]

Hi Sakari,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> Sent: Wednesday, January 30, 2019 12:59 AM
> To: Mani, Rajmohan 
> Cc: Mauro Carvalho Chehab ; Greg Kroah-Hartman
> ; linux-me...@vger.kernel.org;
> de...@driverdev.osuosl.org; linux-kernel@vger.kernel.org; Laurent Pinchart
> ; Jacopo Mondi ;
> Qiu, Tian Shu ; Cao, Bingbu
> ; z...@paasikivi.fi.intel.com; Zhi, Yong
> ; hverk...@xs4all.nl; tf...@chromium.org
> Subject: Re: [PATCH] media: staging/intel-ipu3: Implement lock for stream
> on/off operations
> 
> Hi Rajmohan,
> 
> On Tue, Jan 29, 2019 at 02:27:36PM -0800, Rajmohan Mani wrote:
> > Currently concurrent stream off operations on ImgU nodes are not
> > synchronized, leading to use-after-free bugs (as reported by KASAN).
> >
> > [  250.090724] BUG: KASAN: use-after-free in
> > ipu3_dmamap_free+0xc5/0x116 [ipu3_imgu] [  250.090726] Read of size 8
> > at addr 888127b29bc0 by task yavta/18836 [  250.090731] Hardware
> > name: HP Soraka/Soraka, BIOS Google_Soraka.10431.17.0 03/22/2018 [
> 250.090732] Call Trace:
> > [  250.090735]  dump_stack+0x6a/0xb1
> > [  250.090739]  print_address_description+0x8e/0x279
> > [  250.090743]  ? ipu3_dmamap_free+0xc5/0x116 [ipu3_imgu] [
> > 250.090746]  kasan_report+0x260/0x28a [  250.090750]
> > ipu3_dmamap_free+0xc5/0x116 [ipu3_imgu] [  250.090754]
> > ipu3_css_pool_cleanup+0x24/0x37 [ipu3_imgu] [  250.090759]
> > ipu3_css_pipeline_cleanup+0x61/0xb9 [ipu3_imgu] [  250.090763]
> > ipu3_css_stop_streaming+0x1f2/0x321 [ipu3_imgu] [  250.090768]
> > imgu_s_stream+0x94/0x443 [ipu3_imgu] [  250.090772]  ?
> > ipu3_vb2_buf_queue+0x280/0x280 [ipu3_imgu] [  250.090775]  ?
> > vb2_dma_sg_unmap_dmabuf+0x16/0x6f [videobuf2_dma_sg] [  250.090778]
> ?
> > vb2_buffer_in_use+0x36/0x58 [videobuf2_common] [  250.090782]
> > ipu3_vb2_stop_streaming+0xf9/0x135 [ipu3_imgu]
> >
> > Implemented a lock to synchronize imgu stream on / off operations and
> > the modification of streaming flag (in struct imgu_device), to prevent
> > these issues.
> >
> > Reported-by: Laurent Pinchart 
> > Suggested-by: Laurent Pinchart 
> >
> > Signed-off-by: Rajmohan Mani 
> > ---
> >  drivers/staging/media/ipu3/ipu3-v4l2.c | 6 ++
> >  drivers/staging/media/ipu3/ipu3.c  | 3 +++
> >  drivers/staging/media/ipu3/ipu3.h  | 4 
> >  3 files changed, 13 insertions(+)
> >
> > diff --git a/drivers/staging/media/ipu3/ipu3-v4l2.c
> > b/drivers/staging/media/ipu3/ipu3-v4l2.c
> > index c7936032beb9..cf7e917cd0c8 100644
> > --- a/drivers/staging/media/ipu3/ipu3-v4l2.c
> > +++ b/drivers/staging/media/ipu3/ipu3-v4l2.c
> > @@ -507,12 +507,15 @@ static int ipu3_vb2_start_streaming(struct
> vb2_queue *vq, unsigned int count)
> > goto fail_stop_pipeline;
> > }
> >
> > +   mutex_lock(>streaming_lock);
> > +
> 
> You appear to be using imgu_device.lock (while searching buffers to queue to
> the device) as well as imgu_video_device.lock (qbuf, dqbuf) to serialise 
> access
> to imgu_video_device.buffers list.

Ack

> The two locks may be acquired at the same
> time but each by different processes. That needs to be addressed, but
> probably not in this patch.
> 

The node specific locks will be used by different processes and all of these 
processes
will be competing commonly (and successfully) for the imgu_device lock.
I will look into this more.

> I wonder if it'd be more simple to use imgu->lock here instead of adding a new
> one.
> 

Extending imgu->lock here, does not work in this case, as imgu_queue_buffers()
will be stuck acquiring imgu->lock, which was already acquired by 
imgu_s_stream()
through ipu3_vb2_start_streaming().

> > /* Start streaming of the whole pipeline now */
> > dev_dbg(dev, "IMGU streaming is ready to start");
> > r = imgu_s_stream(imgu, true);
> > if (!r)
> > imgu->streaming = true;
> >
> > +   mutex_unlock(>streaming_lock);
> > return 0;
> >
> >  fail_stop_pipeline:
> > @@ -543,6 +546,8 @@ static void ipu3_vb2_stop_streaming(struct
> vb2_queue *vq)
> > dev_err(>pci_dev->dev,
> > "failed to stop subdev streaming\n");
> >
> > +   mutex_lock(>streaming_lock);
> > +
> > /* Was this the first node with streaming disabled? */
> > if (imgu->streaming && ipu3_all_nodes_streaming(imgu, node)) {
> > /* Yes, really stop streaming now */ @@ -552,6 +557,7 @@
> static
> > void ipu3_vb

RE: [PATCH v2 0/2] Add a property in at24.c

[Mani, Rajmohan]

Hi Bartosz,

> -Original Message-
> From: Yeh, Andy
> Sent: Tuesday, June 26, 2018 9:21 AM
> To: Andy Shevchenko ; Bartosz
> Golaszewski 
> Cc: Chiang, AlanX ; linux-i2c  i...@vger.kernel.org>; Sakari Ailus ; Mani,
> Rajmohan ; Andy Shevchenko
> ; Rob Herring ; Mark
> Rutland ; Arnd Bergmann ; Greg
> Kroah-Hartman ; Linux Kernel Mailing List
> 
> Subject: RE: [PATCH v2 0/2] Add a property in at24.c
> 
> > -Original Message-
> > From: Andy Shevchenko [mailto:andriy.shevche...@linux.intel.com]
> > Sent: Tuesday, June 26, 2018 11:48 PM
> > To: Bartosz Golaszewski 
> > Cc: Chiang, AlanX ; linux-i2c  > i...@vger.kernel.org>; Yeh, Andy ; Sakari Ailus
> > ; Mani, Rajmohan
> > ; Andy Shevchenko
> > ; Rob Herring ; Mark
> > Rutland ; Arnd Bergmann ; Greg
> > Kroah-Hartman ; Linux Kernel Mailing List
> > 
> > Subject: Re: [PATCH v2 0/2] Add a property in at24.c
> >
> > On Tue, 2018-06-26 at 15:30 +0200, Bartosz Golaszewski wrote:
> > > 2018-06-26 15:23 GMT+02:00 Andy Shevchenko
> >  > > tel.com>:
> > > > On Tue, 2018-06-26 at 14:36 +0200, Bartosz Golaszewski wrote:
> > > > > 2018-06-26 14:14 GMT+02:00 Andy Shevchenko
> > > > > :
> > > > > > On Tue, 2018-06-26 at 09:41 +0200, Bartosz Golaszewski wrote:
> > > > > > > What is your use case exactly? Do you have an EEPROM model
> > > > > > > that's not yet supported explicitly in the driver? Why would
> > > > > > > you need this option?
> > > > > >
> > > > > > The current at24 driver has no address width support,  thus,
> > > > > > reusing same
> > > > > > (allocated) IDs (non-DT case) is hard.
> > > >
> > > > ^
> > > >
> > > > > Every supported compatible has the width already specified in
> > > > > its corresponding chip data.
> > > >
> > > >
> > > > Please, read again carefully what I wrote before.
> > > >
> > >
> > > Ok makes sense in that case. Could you just point me towards an
> > > example model which has the address width different than the default
> > > for its type?
> >
> > AFAIK, it's a companion device inside the camera voice coil IC, i.e.
> > DONGWOON DW9714.
> >
> 
> Nope, actually it is DW9807 instead, which is used on a Samsung Chromebook.

M24C64S is one example, where reusing same id (non-DT case) is not possible,
since this model uses 16 bits as address width, as the driver supports only
8 bits address width as default.

Thanks
Raj

RE: [PATCH v2 0/2] Add a property in at24.c

[Mani, Rajmohan]

Hi Bartosz,

> -Original Message-
> From: Yeh, Andy
> Sent: Tuesday, June 26, 2018 9:21 AM
> To: Andy Shevchenko ; Bartosz
> Golaszewski 
> Cc: Chiang, AlanX ; linux-i2c  i...@vger.kernel.org>; Sakari Ailus ; Mani,
> Rajmohan ; Andy Shevchenko
> ; Rob Herring ; Mark
> Rutland ; Arnd Bergmann ; Greg
> Kroah-Hartman ; Linux Kernel Mailing List
> 
> Subject: RE: [PATCH v2 0/2] Add a property in at24.c
> 
> > -Original Message-
> > From: Andy Shevchenko [mailto:andriy.shevche...@linux.intel.com]
> > Sent: Tuesday, June 26, 2018 11:48 PM
> > To: Bartosz Golaszewski 
> > Cc: Chiang, AlanX ; linux-i2c  > i...@vger.kernel.org>; Yeh, Andy ; Sakari Ailus
> > ; Mani, Rajmohan
> > ; Andy Shevchenko
> > ; Rob Herring ; Mark
> > Rutland ; Arnd Bergmann ; Greg
> > Kroah-Hartman ; Linux Kernel Mailing List
> > 
> > Subject: Re: [PATCH v2 0/2] Add a property in at24.c
> >
> > On Tue, 2018-06-26 at 15:30 +0200, Bartosz Golaszewski wrote:
> > > 2018-06-26 15:23 GMT+02:00 Andy Shevchenko
> >  > > tel.com>:
> > > > On Tue, 2018-06-26 at 14:36 +0200, Bartosz Golaszewski wrote:
> > > > > 2018-06-26 14:14 GMT+02:00 Andy Shevchenko
> > > > > :
> > > > > > On Tue, 2018-06-26 at 09:41 +0200, Bartosz Golaszewski wrote:
> > > > > > > What is your use case exactly? Do you have an EEPROM model
> > > > > > > that's not yet supported explicitly in the driver? Why would
> > > > > > > you need this option?
> > > > > >
> > > > > > The current at24 driver has no address width support,  thus,
> > > > > > reusing same
> > > > > > (allocated) IDs (non-DT case) is hard.
> > > >
> > > > ^
> > > >
> > > > > Every supported compatible has the width already specified in
> > > > > its corresponding chip data.
> > > >
> > > >
> > > > Please, read again carefully what I wrote before.
> > > >
> > >
> > > Ok makes sense in that case. Could you just point me towards an
> > > example model which has the address width different than the default
> > > for its type?
> >
> > AFAIK, it's a companion device inside the camera voice coil IC, i.e.
> > DONGWOON DW9714.
> >
> 
> Nope, actually it is DW9807 instead, which is used on a Samsung Chromebook.

M24C64S is one example, where reusing same id (non-DT case) is not possible,
since this model uses 16 bits as address width, as the driver supports only
8 bits address width as default.

Thanks
Raj

RE: [PATCH v6 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Rafael,

> -Original Message-
> From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
> Sent: Tuesday, October 03, 2017 4:44 AM
> To: Sakari Ailus <sakari.ai...@iki.fi>
> Cc: Mani, Rajmohan <rajmohan.m...@intel.com>; linux-
> ker...@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones <lee.jo...@linaro.org>; Linus Walleij
> <linus.wall...@linaro.org>; Alexandre Courbot <gnu...@gmail.com>; Len
> Brown <l...@kernel.org>; Andy Shevchenko <andy.shevche...@gmail.com>
> Subject: Re: [PATCH v6 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Monday, October 2, 2017 6:54:03 PM CEST Sakari Ailus wrote:
> > Hi Rafael,
> >
> > On Mon, Aug 14, 2017 at 11:39:00PM +0300, Sakari Ailus wrote:
> > > On Fri, Jul 28, 2017 at 05:30:26PM -0700, Rajmohan Mani wrote:
> > > > The Kabylake platform coreboot (Chrome OS equivalent of
> > > > BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
> > > > These operation regions are to enable/disable voltage regulators,
> > > > configure voltage regulators, enable/disable clocks and to
> > > > configure clocks.
> > > >
> > > > This config adds ACPI operation region support for TI TPS68470 PMIC.
> > > > TPS68470 device is an advanced power management unit that powers a
> > > > Compact Camera Module (CCM), generates clocks for image sensors,
> > > > drives a dual LED for flash and incorporates two LED drivers for
> > > > general purpose indicators.
> > > > This driver enables ACPI operation region support to control
> > > > voltage regulators and clocks for the TPS68470 PMIC.
> > > >
> > > > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > >
> > > Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> >
> > The other two patches from the set are in v4.14-rc1.
> 
> Thanks!
> 
> I'll queue it up for 4.15 then.
> 

Thanks. This patch has landed in mainline kernel couple of days ago.

Raj

RE: [PATCH v6 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Rafael,

> -Original Message-
> From: Rafael J. Wysocki [mailto:r...@rjwysocki.net]
> Sent: Tuesday, October 03, 2017 4:44 AM
> To: Sakari Ailus 
> Cc: Mani, Rajmohan ; linux-
> ker...@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Len
> Brown ; Andy Shevchenko 
> Subject: Re: [PATCH v6 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Monday, October 2, 2017 6:54:03 PM CEST Sakari Ailus wrote:
> > Hi Rafael,
> >
> > On Mon, Aug 14, 2017 at 11:39:00PM +0300, Sakari Ailus wrote:
> > > On Fri, Jul 28, 2017 at 05:30:26PM -0700, Rajmohan Mani wrote:
> > > > The Kabylake platform coreboot (Chrome OS equivalent of
> > > > BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
> > > > These operation regions are to enable/disable voltage regulators,
> > > > configure voltage regulators, enable/disable clocks and to
> > > > configure clocks.
> > > >
> > > > This config adds ACPI operation region support for TI TPS68470 PMIC.
> > > > TPS68470 device is an advanced power management unit that powers a
> > > > Compact Camera Module (CCM), generates clocks for image sensors,
> > > > drives a dual LED for flash and incorporates two LED drivers for
> > > > general purpose indicators.
> > > > This driver enables ACPI operation region support to control
> > > > voltage regulators and clocks for the TPS68470 PMIC.
> > > >
> > > > Signed-off-by: Rajmohan Mani 
> > >
> > > Acked-by: Sakari Ailus 
> >
> > The other two patches from the set are in v4.14-rc1.
> 
> Thanks!
> 
> I'll queue it up for 4.15 then.
> 

Thanks. This patch has landed in mainline kernel couple of days ago.

Raj

RE: [PATCH v5 0/3] TPS68470 PMIC drivers

[Mani, Rajmohan]

Hi Rafael, Andy,

Just pinging to see if there are updates on ACPI / PMIC opregion patch...

Thanks
Raj

> -Original Message-
> From: Mani, Rajmohan
> Sent: Thursday, September 07, 2017 5:46 PM
> To: 'Rafael J. Wysocki' <raf...@kernel.org>
> Cc: 'Rafael J. Wysocki' <r...@rjwysocki.net>; 'Andy Shevchenko'
> <andy.shevche...@gmail.com>; 'linux-kernel@vger.kernel.org'  ker...@vger.kernel.org>; 'linux-g...@vger.kernel.org'  g...@vger.kernel.org>; 'linux-a...@vger.kernel.org'  a...@vger.kernel.org>; 'Lee Jones' <lee.jo...@linaro.org>; 'Linus Walleij'
> <linus.wall...@linaro.org>; 'Alexandre Courbot' <gnu...@gmail.com>; 'Len
> Brown' <l...@kernel.org>; 'sakari.ai...@linux.intel.com'
> <sakari.ai...@linux.intel.com>
> Subject: RE: [PATCH v5 0/3] TPS68470 PMIC drivers
> 
> Hi Rafael,
> 
> > Subject: RE: [PATCH v5 0/3] TPS68470 PMIC drivers
> >
> > Hi Rafael,
> >
> > > >> > >> > This is the patch series for TPS68470 PMIC that works as a
> > > >> > >> > camera
> > > PMIC.
> > > >> > >> >
> > > >> > >> > The patch series provide the following 3 drivers, to help
> > > >> > >> > configure the
> > > >> voltage regulators, clocks and GPIOs provided by the TPS68470
> > > >> PMIC, to be able to use the camera sensors connected to this PMIC.
> > > >> > >> >
> > > >> > >> > TPS68470 MFD driver:
> > > >> > >> > This is the multi function driver that initializes the
> > > >> > >> > TPS68470 PMIC and
> > > >> supports the GPIO and Op Region functions.
> > > >> > >> >
> > > >> > >> > TPS68470 GPIO driver:
> > > >> > >> > This is the PMIC GPIO driver that will be used by the OS
> > > >> > >> > GPIO layer,
> > > >> when the BIOS / firmware triggered GPIO access is done.
> > > >> > >> >
> > > >> > >> > TPS68470 Op Region driver:
> > > >> > >> > This is the driver that will be invoked, when the BIOS /
> > > >> > >> > firmware
> > > >> configures the voltage / clock for the sensors / vcm devices
> > > >> connected to the PMIC.
> > > >> > >> >
> > > >> > >>
> > > >> > >> All three patches are good to me (we did few rounds of
> > > >> > >> internal review before posting v4)
> > > >> > >>
> > > >> > >> Reviewed-by: Andy Shevchenko <andy.shevche...@gmail.com>
> > > >> > >
> > > >> > > OK, so how should they be routed?
> > > >> >
> > > >> > Good question. I don't know how last time PMIC drivers were
> > > >> > merged, here I think is just sane to route vi MFD with
> > > >> > immutable branch created.
> > > >>
> > > >> OK
> > > >>
> > > >> I will assume that the series will go in through MFD then.
> > > >>
> > > >
> > > > Now that the MFD and GPIO patches of v6 of this series have been
> > > > applied
> > > on respective trees, can you advise the next steps for the ACPI /
> > > PMIC Opregion driver?
> > >
> > > Well, it would have been better to route the whole series through one
> tree.
> > > Now it's better to wait until the two other trees get merged and
> > > then apply the opregion patch.
> > >
> >
> > Ack.
> > Let me get back once the other 2 trees are merged.
> >
> 
> Both MFD and GPIO patches of this series got merged upstream as of today.
> 
> Thanks
> Raj

RE: [PATCH v5 0/3] TPS68470 PMIC drivers

[Mani, Rajmohan]

Hi Rafael, Andy,

Just pinging to see if there are updates on ACPI / PMIC opregion patch...

Thanks
Raj

> -Original Message-
> From: Mani, Rajmohan
> Sent: Thursday, September 07, 2017 5:46 PM
> To: 'Rafael J. Wysocki' 
> Cc: 'Rafael J. Wysocki' ; 'Andy Shevchenko'
> ; 'linux-kernel@vger.kernel.org'  ker...@vger.kernel.org>; 'linux-g...@vger.kernel.org'  g...@vger.kernel.org>; 'linux-a...@vger.kernel.org'  a...@vger.kernel.org>; 'Lee Jones' ; 'Linus Walleij'
> ; 'Alexandre Courbot' ; 'Len
> Brown' ; 'sakari.ai...@linux.intel.com'
> 
> Subject: RE: [PATCH v5 0/3] TPS68470 PMIC drivers
> 
> Hi Rafael,
> 
> > Subject: RE: [PATCH v5 0/3] TPS68470 PMIC drivers
> >
> > Hi Rafael,
> >
> > > >> > >> > This is the patch series for TPS68470 PMIC that works as a
> > > >> > >> > camera
> > > PMIC.
> > > >> > >> >
> > > >> > >> > The patch series provide the following 3 drivers, to help
> > > >> > >> > configure the
> > > >> voltage regulators, clocks and GPIOs provided by the TPS68470
> > > >> PMIC, to be able to use the camera sensors connected to this PMIC.
> > > >> > >> >
> > > >> > >> > TPS68470 MFD driver:
> > > >> > >> > This is the multi function driver that initializes the
> > > >> > >> > TPS68470 PMIC and
> > > >> supports the GPIO and Op Region functions.
> > > >> > >> >
> > > >> > >> > TPS68470 GPIO driver:
> > > >> > >> > This is the PMIC GPIO driver that will be used by the OS
> > > >> > >> > GPIO layer,
> > > >> when the BIOS / firmware triggered GPIO access is done.
> > > >> > >> >
> > > >> > >> > TPS68470 Op Region driver:
> > > >> > >> > This is the driver that will be invoked, when the BIOS /
> > > >> > >> > firmware
> > > >> configures the voltage / clock for the sensors / vcm devices
> > > >> connected to the PMIC.
> > > >> > >> >
> > > >> > >>
> > > >> > >> All three patches are good to me (we did few rounds of
> > > >> > >> internal review before posting v4)
> > > >> > >>
> > > >> > >> Reviewed-by: Andy Shevchenko 
> > > >> > >
> > > >> > > OK, so how should they be routed?
> > > >> >
> > > >> > Good question. I don't know how last time PMIC drivers were
> > > >> > merged, here I think is just sane to route vi MFD with
> > > >> > immutable branch created.
> > > >>
> > > >> OK
> > > >>
> > > >> I will assume that the series will go in through MFD then.
> > > >>
> > > >
> > > > Now that the MFD and GPIO patches of v6 of this series have been
> > > > applied
> > > on respective trees, can you advise the next steps for the ACPI /
> > > PMIC Opregion driver?
> > >
> > > Well, it would have been better to route the whole series through one
> tree.
> > > Now it's better to wait until the two other trees get merged and
> > > then apply the opregion patch.
> > >
> >
> > Ack.
> > Let me get back once the other 2 trees are merged.
> >
> 
> Both MFD and GPIO patches of this series got merged upstream as of today.
> 
> Thanks
> Raj

RE: [PATCH v6 3/3] eeprom: at24: enable runtime pm support

[Mani, Rajmohan]

Adding Tomasz...

> -Original Message-
> From: Mohandass, Divagar
> Sent: Monday, September 04, 2017 3:29 AM
> To: robh...@kernel.org; mark.rutl...@arm.com; w...@the-dreams.de;
> sakari.ai...@iki.fi
> Cc: devicet...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Mani, Rajmohan <rajmohan.m...@intel.com>;
> Mohandass, Divagar <divagar.mohand...@intel.com>
> Subject: [PATCH v6 3/3] eeprom: at24: enable runtime pm support
> 
> Currently the device is kept in D0, there is an opportunity to save power by
> enabling runtime pm.
> 
> Device can be daisy chained from PMIC and we can't rely on I2C core for auto
> resume/suspend. Driver will decide when to resume/suspend.
> 
> Signed-off-by: Divagar Mohandass <divagar.mohand...@intel.com>
> ---
>  drivers/misc/eeprom/at24.c | 38
> ++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index
> 2199c42..d718a7a 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  /*
>   * I2C EEPROMs from most vendors are inexpensive and mostly
> interchangeable.
> @@ -501,11 +502,21 @@ static ssize_t at24_eeprom_write_i2c(struct
> at24_data *at24, const char *buf,  static int at24_read(void *priv, unsigned 
> int
> off, void *val, size_t count)  {
>   struct at24_data *at24 = priv;
> + struct i2c_client *client;
>   char *buf = val;
> + int ret;
> 
>   if (unlikely(!count))
>   return count;
> 
> + client = at24_translate_offset(at24, );
> +
> + ret = pm_runtime_get_sync(>dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(>dev);
> + return ret;
> + }
> +
>   /*
>* Read data from chip, protecting against concurrent updates
>* from this host, but not from other I2C masters.
> @@ -518,6 +529,7 @@ static int at24_read(void *priv, unsigned int off, void
> *val, size_t count)
>   status = at24->read_func(at24, buf, off, count);
>   if (status < 0) {
>   mutex_unlock(>lock);
> + pm_runtime_put(>dev);
>   return status;
>   }
>   buf += status;
> @@ -527,17 +539,29 @@ static int at24_read(void *priv, unsigned int off, void
> *val, size_t count)
> 
>   mutex_unlock(>lock);
> 
> + pm_runtime_put(>dev);
> +
>   return 0;
>  }
> 
>  static int at24_write(void *priv, unsigned int off, void *val, size_t count) 
>  {
>   struct at24_data *at24 = priv;
> + struct i2c_client *client;
>   char *buf = val;
> + int ret;
> 
>   if (unlikely(!count))
>   return -EINVAL;
> 
> + client = at24_translate_offset(at24, );
> +
> + ret = pm_runtime_get_sync(>dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(>dev);
> + return ret;
> + }
> +
>   /*
>* Write data to chip, protecting against concurrent updates
>* from this host, but not from other I2C masters.
> @@ -550,6 +574,7 @@ static int at24_write(void *priv, unsigned int off, void
> *val, size_t count)
>   status = at24->write_func(at24, buf, off, count);
>   if (status < 0) {
>   mutex_unlock(>lock);
> + pm_runtime_put(>dev);
>   return status;
>   }
>   buf += status;
> @@ -559,6 +584,8 @@ static int at24_write(void *priv, unsigned int off, void
> *val, size_t count)
> 
>   mutex_unlock(>lock);
> 
> + pm_runtime_put(>dev);
> +
>   return 0;
>  }
> 
> @@ -743,11 +770,17 @@ static int at24_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
> 
>   i2c_set_clientdata(client, at24);
> 
> + /* enable runtime pm */
> + pm_runtime_get_noresume(>dev);
> + pm_runtime_set_active(>dev);
> + pm_runtime_enable(>dev);
> +
>   /*
>* Perform a one-byte test read to verify that the
>* chip is functional.
>*/
>   err = at24_read(at24, 0, _byte, 1);
> + pm_runtime_put(>dev);
>   if (err) {
>   err = -ENODEV;
>   goto err_clients;
> @@ -795,6 +828,8 @@ static int at24_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
>   if (at24->client[i])
>   i2c_unregister_device(at24->client[i]);
> 
> + pm_runtime_disable(>dev);
> +
>   return err;
>  }
> 
> @@ -810,6 +845,9 @@ static int at24_remove(struct i2c_client *client)
>   for (i = 1; i < at24->num_addresses; i++)
>   i2c_unregister_device(at24->client[i]);
> 
> + pm_runtime_disable(>dev);
> + pm_runtime_set_suspended(>dev);
> +
>   return 0;
>  }
> 
> --
> 1.9.1

RE: [PATCH v6 3/3] eeprom: at24: enable runtime pm support

[Mani, Rajmohan]

Adding Tomasz...

> -Original Message-
> From: Mohandass, Divagar
> Sent: Monday, September 04, 2017 3:29 AM
> To: robh...@kernel.org; mark.rutl...@arm.com; w...@the-dreams.de;
> sakari.ai...@iki.fi
> Cc: devicet...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Mani, Rajmohan ;
> Mohandass, Divagar 
> Subject: [PATCH v6 3/3] eeprom: at24: enable runtime pm support
> 
> Currently the device is kept in D0, there is an opportunity to save power by
> enabling runtime pm.
> 
> Device can be daisy chained from PMIC and we can't rely on I2C core for auto
> resume/suspend. Driver will decide when to resume/suspend.
> 
> Signed-off-by: Divagar Mohandass 
> ---
>  drivers/misc/eeprom/at24.c | 38
> ++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index
> 2199c42..d718a7a 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
> 
>  /*
>   * I2C EEPROMs from most vendors are inexpensive and mostly
> interchangeable.
> @@ -501,11 +502,21 @@ static ssize_t at24_eeprom_write_i2c(struct
> at24_data *at24, const char *buf,  static int at24_read(void *priv, unsigned 
> int
> off, void *val, size_t count)  {
>   struct at24_data *at24 = priv;
> + struct i2c_client *client;
>   char *buf = val;
> + int ret;
> 
>   if (unlikely(!count))
>   return count;
> 
> + client = at24_translate_offset(at24, );
> +
> + ret = pm_runtime_get_sync(>dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(>dev);
> + return ret;
> + }
> +
>   /*
>* Read data from chip, protecting against concurrent updates
>* from this host, but not from other I2C masters.
> @@ -518,6 +529,7 @@ static int at24_read(void *priv, unsigned int off, void
> *val, size_t count)
>   status = at24->read_func(at24, buf, off, count);
>   if (status < 0) {
>   mutex_unlock(>lock);
> + pm_runtime_put(>dev);
>   return status;
>   }
>   buf += status;
> @@ -527,17 +539,29 @@ static int at24_read(void *priv, unsigned int off, void
> *val, size_t count)
> 
>   mutex_unlock(>lock);
> 
> + pm_runtime_put(>dev);
> +
>   return 0;
>  }
> 
>  static int at24_write(void *priv, unsigned int off, void *val, size_t count) 
>  {
>   struct at24_data *at24 = priv;
> + struct i2c_client *client;
>   char *buf = val;
> + int ret;
> 
>   if (unlikely(!count))
>   return -EINVAL;
> 
> + client = at24_translate_offset(at24, );
> +
> + ret = pm_runtime_get_sync(>dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(>dev);
> + return ret;
> + }
> +
>   /*
>* Write data to chip, protecting against concurrent updates
>* from this host, but not from other I2C masters.
> @@ -550,6 +574,7 @@ static int at24_write(void *priv, unsigned int off, void
> *val, size_t count)
>   status = at24->write_func(at24, buf, off, count);
>   if (status < 0) {
>   mutex_unlock(>lock);
> + pm_runtime_put(>dev);
>   return status;
>   }
>   buf += status;
> @@ -559,6 +584,8 @@ static int at24_write(void *priv, unsigned int off, void
> *val, size_t count)
> 
>   mutex_unlock(>lock);
> 
> + pm_runtime_put(>dev);
> +
>   return 0;
>  }
> 
> @@ -743,11 +770,17 @@ static int at24_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
> 
>   i2c_set_clientdata(client, at24);
> 
> + /* enable runtime pm */
> + pm_runtime_get_noresume(>dev);
> + pm_runtime_set_active(>dev);
> + pm_runtime_enable(>dev);
> +
>   /*
>* Perform a one-byte test read to verify that the
>* chip is functional.
>*/
>   err = at24_read(at24, 0, _byte, 1);
> + pm_runtime_put(>dev);
>   if (err) {
>   err = -ENODEV;
>   goto err_clients;
> @@ -795,6 +828,8 @@ static int at24_probe(struct i2c_client *client, const
> struct i2c_device_id *id)
>   if (at24->client[i])
>   i2c_unregister_device(at24->client[i]);
> 
> + pm_runtime_disable(>dev);
> +
>   return err;
>  }
> 
> @@ -810,6 +845,9 @@ static int at24_remove(struct i2c_client *client)
>   for (i = 1; i < at24->num_addresses; i++)
>   i2c_unregister_device(at24->client[i]);
> 
> + pm_runtime_disable(>dev);
> + pm_runtime_set_suspended(>dev);
> +
>   return 0;
>  }
> 
> --
> 1.9.1

RE: [PATCH v6 2/3] eeprom: at24: add support to fetch eeprom device property "size"

[Mani, Rajmohan]

Adding Tomasz...

> -Original Message-
> From: Mohandass, Divagar
> Sent: Monday, September 04, 2017 3:29 AM
> To: robh...@kernel.org; mark.rutl...@arm.com; w...@the-dreams.de;
> sakari.ai...@iki.fi
> Cc: devicet...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Mani, Rajmohan <rajmohan.m...@intel.com>;
> Mohandass, Divagar <divagar.mohand...@intel.com>
> Subject: [PATCH v6 2/3] eeprom: at24: add support to fetch eeprom device
> property "size"
> 
> Obtain the size of the EEPROM chip from DT if the "size" property is specified
> for the device.
> 
> Signed-off-by: Divagar Mohandass <divagar.mohand...@intel.com>
> ---
>  drivers/misc/eeprom/at24.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index
> 764ff5df..2199c42 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -570,6 +570,10 @@ static void at24_get_pdata(struct device *dev, struct
> at24_platform_data *chip)
>   if (device_property_present(dev, "read-only"))
>   chip->flags |= AT24_FLAG_READONLY;
> 
> + err = device_property_read_u32(dev, "size", );
> + if (!err)
> + chip->byte_len = val;
> +
>   err = device_property_read_u32(dev, "pagesize", );
>   if (!err) {
>   chip->page_size = val;
> --
> 1.9.1

RE: [PATCH v6 2/3] eeprom: at24: add support to fetch eeprom device property "size"

[Mani, Rajmohan]

Adding Tomasz...

> -Original Message-
> From: Mohandass, Divagar
> Sent: Monday, September 04, 2017 3:29 AM
> To: robh...@kernel.org; mark.rutl...@arm.com; w...@the-dreams.de;
> sakari.ai...@iki.fi
> Cc: devicet...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Mani, Rajmohan ;
> Mohandass, Divagar 
> Subject: [PATCH v6 2/3] eeprom: at24: add support to fetch eeprom device
> property "size"
> 
> Obtain the size of the EEPROM chip from DT if the "size" property is specified
> for the device.
> 
> Signed-off-by: Divagar Mohandass 
> ---
>  drivers/misc/eeprom/at24.c | 4 
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index
> 764ff5df..2199c42 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -570,6 +570,10 @@ static void at24_get_pdata(struct device *dev, struct
> at24_platform_data *chip)
>   if (device_property_present(dev, "read-only"))
>   chip->flags |= AT24_FLAG_READONLY;
> 
> + err = device_property_read_u32(dev, "size", );
> + if (!err)
> + chip->byte_len = val;
> +
>   err = device_property_read_u32(dev, "pagesize", );
>   if (!err) {
>   chip->page_size = val;
> --
> 1.9.1

RE: [PATCH v6 1/3] dt-bindings: add eeprom "size" property

[Mani, Rajmohan]

Adding Tomasz...

> -Original Message-
> From: Mohandass, Divagar
> Sent: Monday, September 04, 2017 3:29 AM
> To: robh...@kernel.org; mark.rutl...@arm.com; w...@the-dreams.de;
> sakari.ai...@iki.fi
> Cc: devicet...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Mani, Rajmohan <rajmohan.m...@intel.com>;
> Mohandass, Divagar <divagar.mohand...@intel.com>
> Subject: [PATCH v6 1/3] dt-bindings: add eeprom "size" property
> 
> This adds eeprom "size" as optional property for i2c eeproms.
> The "size" property allows explicitly specifying the size of the EEPROM chip 
> in
> bytes.
> 
> Signed-off-by: Divagar Mohandass <divagar.mohand...@intel.com>
> Acked-by: Rob Herring <r...@kernel.org>
> ---
>  Documentation/devicetree/bindings/eeprom/eeprom.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/eeprom/eeprom.txt
> b/Documentation/devicetree/bindings/eeprom/eeprom.txt
> index 5696eb5..1436569 100644
> --- a/Documentation/devicetree/bindings/eeprom/eeprom.txt
> +++ b/Documentation/devicetree/bindings/eeprom/eeprom.txt
> @@ -32,6 +32,8 @@ Optional properties:
> 
>- read-only: this parameterless property disables writes to the eeprom
> 
> +  - size: total eeprom size in bytes
> +
>  Example:
> 
>  eeprom@52 {
> --
> 1.9.1

RE: [PATCH v6 1/3] dt-bindings: add eeprom "size" property

[Mani, Rajmohan]

Adding Tomasz...

> -Original Message-
> From: Mohandass, Divagar
> Sent: Monday, September 04, 2017 3:29 AM
> To: robh...@kernel.org; mark.rutl...@arm.com; w...@the-dreams.de;
> sakari.ai...@iki.fi
> Cc: devicet...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Mani, Rajmohan ;
> Mohandass, Divagar 
> Subject: [PATCH v6 1/3] dt-bindings: add eeprom "size" property
> 
> This adds eeprom "size" as optional property for i2c eeproms.
> The "size" property allows explicitly specifying the size of the EEPROM chip 
> in
> bytes.
> 
> Signed-off-by: Divagar Mohandass 
> Acked-by: Rob Herring 
> ---
>  Documentation/devicetree/bindings/eeprom/eeprom.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/eeprom/eeprom.txt
> b/Documentation/devicetree/bindings/eeprom/eeprom.txt
> index 5696eb5..1436569 100644
> --- a/Documentation/devicetree/bindings/eeprom/eeprom.txt
> +++ b/Documentation/devicetree/bindings/eeprom/eeprom.txt
> @@ -32,6 +32,8 @@ Optional properties:
> 
>- read-only: this parameterless property disables writes to the eeprom
> 
> +  - size: total eeprom size in bytes
> +
>  Example:
> 
>  eeprom@52 {
> --
> 1.9.1

RE: [PATCH v6 0/3] enable eeprom "size" property and runtime pm

[Mani, Rajmohan]

Adding Tomasz...

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Tuesday, September 05, 2017 6:48 AM
> To: Mohandass, Divagar <divagar.mohand...@intel.com>
> Cc: robh...@kernel.org; mark.rutl...@arm.com; w...@the-dreams.de;
> devicet...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Mani, Rajmohan <rajmohan.m...@intel.com>
> Subject: Re: [PATCH v6 0/3] enable eeprom "size" property and runtime pm
> 
> Hi Divagar,
> 
> On Mon, Sep 04, 2017 at 03:58:45PM +0530, Divagar Mohandass wrote:
> > This series adds support for eeprom "size" property which will be read
> > by the driver for eeprom size. The existing ACPI has a different
> > default size which can be overridden with a DSD property value provided by
> the platform FW.
> >
> > This series also adds support for runtime PM. The eeprom driver
> > currently did not have support for runtime PM and the device was kept in D0
> throughout.
> >
> > [v1]
> > - Add support for eeprom "size" property.
> > - Add runtime PM support to the driver.
> >
> > [v2]
> > - Improved the patch subject.
> >
> > [v3]
> > - Addressed comments from Sakari Ailus.
> > - Improved patch description.
> > - Improved pm support patch.
> >
> > [v4]
> > - Improved runtime pm support.
> > - Addressed comments from Sakari Ailus.
> >
> > [v5]
> > - Addressed comments from Sakari Ailus.
> > - Improved error handling for PM support.
> >
> > [v6]
> > - Addressed comments from Sakari Ailus.
> >
> > Divagar Mohandass (3):
> >   dt-bindings: add eeprom "size" property
> >   eeprom: at24: add support to fetch eeprom device property "size"
> >   eeprom: at24: enable runtime pm support
> >
> >  .../devicetree/bindings/eeprom/eeprom.txt  |  2 ++
> >  drivers/misc/eeprom/at24.c | 42 
> > ++
> >  2 files changed, 44 insertions(+)
> 
> Thanks for the update!
> 
> For the set:
> 
> Reviewed-by: Sakari Ailus <sakari.ai...@linux.intel.com>
> 
> --
> Kind regards,
> 
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi

RE: [PATCH v6 0/3] enable eeprom "size" property and runtime pm

[Mani, Rajmohan]

Adding Tomasz...

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Tuesday, September 05, 2017 6:48 AM
> To: Mohandass, Divagar 
> Cc: robh...@kernel.org; mark.rutl...@arm.com; w...@the-dreams.de;
> devicet...@vger.kernel.org; linux-...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Mani, Rajmohan 
> Subject: Re: [PATCH v6 0/3] enable eeprom "size" property and runtime pm
> 
> Hi Divagar,
> 
> On Mon, Sep 04, 2017 at 03:58:45PM +0530, Divagar Mohandass wrote:
> > This series adds support for eeprom "size" property which will be read
> > by the driver for eeprom size. The existing ACPI has a different
> > default size which can be overridden with a DSD property value provided by
> the platform FW.
> >
> > This series also adds support for runtime PM. The eeprom driver
> > currently did not have support for runtime PM and the device was kept in D0
> throughout.
> >
> > [v1]
> > - Add support for eeprom "size" property.
> > - Add runtime PM support to the driver.
> >
> > [v2]
> > - Improved the patch subject.
> >
> > [v3]
> > - Addressed comments from Sakari Ailus.
> > - Improved patch description.
> > - Improved pm support patch.
> >
> > [v4]
> > - Improved runtime pm support.
> > - Addressed comments from Sakari Ailus.
> >
> > [v5]
> > - Addressed comments from Sakari Ailus.
> > - Improved error handling for PM support.
> >
> > [v6]
> > - Addressed comments from Sakari Ailus.
> >
> > Divagar Mohandass (3):
> >   dt-bindings: add eeprom "size" property
> >   eeprom: at24: add support to fetch eeprom device property "size"
> >   eeprom: at24: enable runtime pm support
> >
> >  .../devicetree/bindings/eeprom/eeprom.txt  |  2 ++
> >  drivers/misc/eeprom/at24.c | 42 
> > ++
> >  2 files changed, 44 insertions(+)
> 
> Thanks for the update!
> 
> For the set:
> 
> Reviewed-by: Sakari Ailus 
> 
> --
> Kind regards,
> 
> Sakari Ailus
> e-mail: sakari.ai...@iki.fi

RE: [PATCH v5 0/3] TPS68470 PMIC drivers

[Mani, Rajmohan]

Hi Rafael,

> Subject: RE: [PATCH v5 0/3] TPS68470 PMIC drivers
> 
> Hi Rafael,
> 
> > >> > >> > This is the patch series for TPS68470 PMIC that works as a
> > >> > >> > camera
> > PMIC.
> > >> > >> >
> > >> > >> > The patch series provide the following 3 drivers, to help
> > >> > >> > configure the
> > >> voltage regulators, clocks and GPIOs provided by the TPS68470 PMIC,
> > >> to be able to use the camera sensors connected to this PMIC.
> > >> > >> >
> > >> > >> > TPS68470 MFD driver:
> > >> > >> > This is the multi function driver that initializes the
> > >> > >> > TPS68470 PMIC and
> > >> supports the GPIO and Op Region functions.
> > >> > >> >
> > >> > >> > TPS68470 GPIO driver:
> > >> > >> > This is the PMIC GPIO driver that will be used by the OS
> > >> > >> > GPIO layer,
> > >> when the BIOS / firmware triggered GPIO access is done.
> > >> > >> >
> > >> > >> > TPS68470 Op Region driver:
> > >> > >> > This is the driver that will be invoked, when the BIOS /
> > >> > >> > firmware
> > >> configures the voltage / clock for the sensors / vcm devices
> > >> connected to the PMIC.
> > >> > >> >
> > >> > >>
> > >> > >> All three patches are good to me (we did few rounds of
> > >> > >> internal review before posting v4)
> > >> > >>
> > >> > >> Reviewed-by: Andy Shevchenko 
> > >> > >
> > >> > > OK, so how should they be routed?
> > >> >
> > >> > Good question. I don't know how last time PMIC drivers were
> > >> > merged, here I think is just sane to route vi MFD with immutable
> > >> > branch created.
> > >>
> > >> OK
> > >>
> > >> I will assume that the series will go in through MFD then.
> > >>
> > >
> > > Now that the MFD and GPIO patches of v6 of this series have been
> > > applied
> > on respective trees, can you advise the next steps for the ACPI / PMIC
> > Opregion driver?
> >
> > Well, it would have been better to route the whole series through one tree.
> > Now it's better to wait until the two other trees get merged and then
> > apply the opregion patch.
> >
> 
> Ack.
> Let me get back once the other 2 trees are merged.
> 

Both MFD and GPIO patches of this series got merged upstream as of today.

Thanks
Raj

RE: [PATCH v5 0/3] TPS68470 PMIC drivers

[Mani, Rajmohan]

Hi Rafael,

> Subject: RE: [PATCH v5 0/3] TPS68470 PMIC drivers
> 
> Hi Rafael,
> 
> > >> > >> > This is the patch series for TPS68470 PMIC that works as a
> > >> > >> > camera
> > PMIC.
> > >> > >> >
> > >> > >> > The patch series provide the following 3 drivers, to help
> > >> > >> > configure the
> > >> voltage regulators, clocks and GPIOs provided by the TPS68470 PMIC,
> > >> to be able to use the camera sensors connected to this PMIC.
> > >> > >> >
> > >> > >> > TPS68470 MFD driver:
> > >> > >> > This is the multi function driver that initializes the
> > >> > >> > TPS68470 PMIC and
> > >> supports the GPIO and Op Region functions.
> > >> > >> >
> > >> > >> > TPS68470 GPIO driver:
> > >> > >> > This is the PMIC GPIO driver that will be used by the OS
> > >> > >> > GPIO layer,
> > >> when the BIOS / firmware triggered GPIO access is done.
> > >> > >> >
> > >> > >> > TPS68470 Op Region driver:
> > >> > >> > This is the driver that will be invoked, when the BIOS /
> > >> > >> > firmware
> > >> configures the voltage / clock for the sensors / vcm devices
> > >> connected to the PMIC.
> > >> > >> >
> > >> > >>
> > >> > >> All three patches are good to me (we did few rounds of
> > >> > >> internal review before posting v4)
> > >> > >>
> > >> > >> Reviewed-by: Andy Shevchenko 
> > >> > >
> > >> > > OK, so how should they be routed?
> > >> >
> > >> > Good question. I don't know how last time PMIC drivers were
> > >> > merged, here I think is just sane to route vi MFD with immutable
> > >> > branch created.
> > >>
> > >> OK
> > >>
> > >> I will assume that the series will go in through MFD then.
> > >>
> > >
> > > Now that the MFD and GPIO patches of v6 of this series have been
> > > applied
> > on respective trees, can you advise the next steps for the ACPI / PMIC
> > Opregion driver?
> >
> > Well, it would have been better to route the whole series through one tree.
> > Now it's better to wait until the two other trees get merged and then
> > apply the opregion patch.
> >
> 
> Ack.
> Let me get back once the other 2 trees are merged.
> 

Both MFD and GPIO patches of this series got merged upstream as of today.

Thanks
Raj

RE: [PATCH v5 0/3] TPS68470 PMIC drivers

[Mani, Rajmohan]

Hi Rafael,

> >> > >> > This is the patch series for TPS68470 PMIC that works as a camera
> PMIC.
> >> > >> >
> >> > >> > The patch series provide the following 3 drivers, to help
> >> > >> > configure the
> >> voltage regulators, clocks and GPIOs provided by the TPS68470 PMIC,
> >> to be able to use the camera sensors connected to this PMIC.
> >> > >> >
> >> > >> > TPS68470 MFD driver:
> >> > >> > This is the multi function driver that initializes the
> >> > >> > TPS68470 PMIC and
> >> supports the GPIO and Op Region functions.
> >> > >> >
> >> > >> > TPS68470 GPIO driver:
> >> > >> > This is the PMIC GPIO driver that will be used by the OS GPIO
> >> > >> > layer,
> >> when the BIOS / firmware triggered GPIO access is done.
> >> > >> >
> >> > >> > TPS68470 Op Region driver:
> >> > >> > This is the driver that will be invoked, when the BIOS /
> >> > >> > firmware
> >> configures the voltage / clock for the sensors / vcm devices
> >> connected to the PMIC.
> >> > >> >
> >> > >>
> >> > >> All three patches are good to me (we did few rounds of internal
> >> > >> review before posting v4)
> >> > >>
> >> > >> Reviewed-by: Andy Shevchenko 
> >> > >
> >> > > OK, so how should they be routed?
> >> >
> >> > Good question. I don't know how last time PMIC drivers were merged,
> >> > here I think is just sane to route vi MFD with immutable branch
> >> > created.
> >>
> >> OK
> >>
> >> I will assume that the series will go in through MFD then.
> >>
> >
> > Now that the MFD and GPIO patches of v6 of this series have been applied
> on respective trees, can you advise the next steps for the ACPI / PMIC 
> Opregion
> driver?
> 
> Well, it would have been better to route the whole series through one tree.
> Now it's better to wait until the two other trees get merged and then apply 
> the
> opregion patch.
> 

Ack.
Let me get back once the other 2 trees are merged.

Thanks
Raj

RE: [PATCH v5 0/3] TPS68470 PMIC drivers

[Mani, Rajmohan]

Hi Rafael,

> >> > >> > This is the patch series for TPS68470 PMIC that works as a camera
> PMIC.
> >> > >> >
> >> > >> > The patch series provide the following 3 drivers, to help
> >> > >> > configure the
> >> voltage regulators, clocks and GPIOs provided by the TPS68470 PMIC,
> >> to be able to use the camera sensors connected to this PMIC.
> >> > >> >
> >> > >> > TPS68470 MFD driver:
> >> > >> > This is the multi function driver that initializes the
> >> > >> > TPS68470 PMIC and
> >> supports the GPIO and Op Region functions.
> >> > >> >
> >> > >> > TPS68470 GPIO driver:
> >> > >> > This is the PMIC GPIO driver that will be used by the OS GPIO
> >> > >> > layer,
> >> when the BIOS / firmware triggered GPIO access is done.
> >> > >> >
> >> > >> > TPS68470 Op Region driver:
> >> > >> > This is the driver that will be invoked, when the BIOS /
> >> > >> > firmware
> >> configures the voltage / clock for the sensors / vcm devices
> >> connected to the PMIC.
> >> > >> >
> >> > >>
> >> > >> All three patches are good to me (we did few rounds of internal
> >> > >> review before posting v4)
> >> > >>
> >> > >> Reviewed-by: Andy Shevchenko 
> >> > >
> >> > > OK, so how should they be routed?
> >> >
> >> > Good question. I don't know how last time PMIC drivers were merged,
> >> > here I think is just sane to route vi MFD with immutable branch
> >> > created.
> >>
> >> OK
> >>
> >> I will assume that the series will go in through MFD then.
> >>
> >
> > Now that the MFD and GPIO patches of v6 of this series have been applied
> on respective trees, can you advise the next steps for the ACPI / PMIC 
> Opregion
> driver?
> 
> Well, it would have been better to route the whole series through one tree.
> Now it's better to wait until the two other trees get merged and then apply 
> the
> opregion patch.
> 

Ack.
Let me get back once the other 2 trees are merged.

Thanks
Raj

RE: [PATCH v5 0/3] TPS68470 PMIC drivers

[Mani, Rajmohan]

Hi Andy,

> > >> > This is the patch series for TPS68470 PMIC that works as a camera PMIC.
> > >> >
> > >> > The patch series provide the following 3 drivers, to help configure the
> voltage regulators, clocks and GPIOs provided by the TPS68470 PMIC, to be
> able to use the camera sensors connected to this PMIC.
> > >> >
> > >> > TPS68470 MFD driver:
> > >> > This is the multi function driver that initializes the TPS68470 PMIC 
> > >> > and
> supports the GPIO and Op Region functions.
> > >> >
> > >> > TPS68470 GPIO driver:
> > >> > This is the PMIC GPIO driver that will be used by the OS GPIO layer,
> when the BIOS / firmware triggered GPIO access is done.
> > >> >
> > >> > TPS68470 Op Region driver:
> > >> > This is the driver that will be invoked, when the BIOS / firmware
> configures the voltage / clock for the sensors / vcm devices connected to the
> PMIC.
> > >> >
> > >>
> > >> All three patches are good to me (we did few rounds of internal
> > >> review before posting v4)
> > >>
> > >> Reviewed-by: Andy Shevchenko 
> > >
> > > OK, so how should they be routed?
> >
> > Good question. I don't know how last time PMIC drivers were merged,
> > here I think is just sane to route vi MFD with immutable branch
> > created.
> 
> OK
> 
> I will assume that the series will go in through MFD then.
> 

Now that the MFD and GPIO patches of v6 of this series have been applied on 
respective trees, can you advise the next steps for the ACPI / PMIC Opregion 
driver?

Thanks
Raj

RE: [PATCH v5 0/3] TPS68470 PMIC drivers

[Mani, Rajmohan]

Hi Andy,

> > >> > This is the patch series for TPS68470 PMIC that works as a camera PMIC.
> > >> >
> > >> > The patch series provide the following 3 drivers, to help configure the
> voltage regulators, clocks and GPIOs provided by the TPS68470 PMIC, to be
> able to use the camera sensors connected to this PMIC.
> > >> >
> > >> > TPS68470 MFD driver:
> > >> > This is the multi function driver that initializes the TPS68470 PMIC 
> > >> > and
> supports the GPIO and Op Region functions.
> > >> >
> > >> > TPS68470 GPIO driver:
> > >> > This is the PMIC GPIO driver that will be used by the OS GPIO layer,
> when the BIOS / firmware triggered GPIO access is done.
> > >> >
> > >> > TPS68470 Op Region driver:
> > >> > This is the driver that will be invoked, when the BIOS / firmware
> configures the voltage / clock for the sensors / vcm devices connected to the
> PMIC.
> > >> >
> > >>
> > >> All three patches are good to me (we did few rounds of internal
> > >> review before posting v4)
> > >>
> > >> Reviewed-by: Andy Shevchenko 
> > >
> > > OK, so how should they be routed?
> >
> > Good question. I don't know how last time PMIC drivers were merged,
> > here I think is just sane to route vi MFD with immutable branch
> > created.
> 
> OK
> 
> I will assume that the series will go in through MFD then.
> 

Now that the MFD and GPIO patches of v6 of this series have been applied on 
respective trees, can you advise the next steps for the ACPI / PMIC Opregion 
driver?

Thanks
Raj

RE: [PATCH v6 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

> -Original Message-
> From: Linus Walleij [mailto:linus.wall...@linaro.org]
> Sent: Monday, August 14, 2017 6:39 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; ACPI Devel
> Maling List <linux-a...@vger.kernel.org>; Lee Jones <lee.jo...@linaro.org>;
> Alexandre Courbot <gnu...@gmail.com>; Rafael J. Wysocki
> <r...@rjwysocki.net>; Len Brown <l...@kernel.org>;
> sakari.ai...@linux.intel.com; Andy Shevchenko
> <andy.shevche...@gmail.com>
> Subject: Re: [PATCH v6 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Sat, Jul 29, 2017 at 2:30 AM, Rajmohan Mani
> <rajmohan.m...@intel.com> wrote:
> 
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> >
> > The GPIOs are also provided with descriptive names.
> > However, the typical use case is that the OS GPIO driver will interact
> > with TPS68470 GPIO driver to configure these GPIOs, as requested by
> > the platform firmware.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > Reviewed-by: Linus Walleij <linus.wall...@linaro.org>
> 
> (...)
> 
> > +config GPIO_TPS68470
> > +   bool "TPS68470 GPIO"
> > +   depends on MFD_TPS68470
> 
> Guarded by this symbol, so:
> patch applied to the GPIO tree, thanks!
> 

Thanks for the reviews and update.
Raj

RE: [PATCH v6 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

> -Original Message-
> From: Linus Walleij [mailto:linus.wall...@linaro.org]
> Sent: Monday, August 14, 2017 6:39 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; ACPI Devel
> Maling List ; Lee Jones ;
> Alexandre Courbot ; Rafael J. Wysocki
> ; Len Brown ;
> sakari.ai...@linux.intel.com; Andy Shevchenko
> 
> Subject: Re: [PATCH v6 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Sat, Jul 29, 2017 at 2:30 AM, Rajmohan Mani
>  wrote:
> 
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> >
> > The GPIOs are also provided with descriptive names.
> > However, the typical use case is that the OS GPIO driver will interact
> > with TPS68470 GPIO driver to configure these GPIOs, as requested by
> > the platform firmware.
> >
> > Signed-off-by: Rajmohan Mani 
> > Reviewed-by: Linus Walleij 
> 
> (...)
> 
> > +config GPIO_TPS68470
> > +   bool "TPS68470 GPIO"
> > +   depends on MFD_TPS68470
> 
> Guarded by this symbol, so:
> patch applied to the GPIO tree, thanks!
> 

Thanks for the reviews and update.
Raj

RE: [PATCH v6 0/3] TPS68470 PMIC drivers

[Mani, Rajmohan]

Hi All,

Now that Lee has applied the MFD patch 
(https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-mfd-next=6a58f36b0c49c8179804bead579f5ff6304f0245),
 can someone help clarify the next steps for the GPIO and ACPI / PMIC OpRegion 
patches of this series?

Both these patches depend on the MFD patch (MFD_TPS68470 config option) to 
compile.

Thanks
Raj

> -Original Message-
> From: Mani, Rajmohan
> Sent: Friday, July 28, 2017 5:30 PM
> To: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org
> Cc: Lee Jones <lee.jo...@linaro.org>; Linus Walleij
> <linus.wall...@linaro.org>; Alexandre Courbot <gnu...@gmail.com>; Rafael
> J. Wysocki <r...@rjwysocki.net>; Len Brown <l...@kernel.org>;
> sakari.ai...@linux.intel.com; Andy Shevchenko
> <andy.shevche...@gmail.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>
> Subject: [PATCH v6 0/3] TPS68470 PMIC drivers
> 
> This is the patch series for TPS68470 PMIC that works as a camera PMIC.
> 
> The patch series provide the following 3 drivers, to help configure the 
> voltage
> regulators, clocks and GPIOs provided by the TPS68470 PMIC, to be able to
> use the camera sensors connected to this PMIC.
> 
> TPS68470 MFD driver:
> This is the multi function driver that initializes the TPS68470 PMIC and
> supports the GPIO and Op Region functions.
> 
> TPS68470 GPIO driver:
> This is the PMIC GPIO driver that will be used by the OS GPIO layer, when the
> BIOS / firmware triggered GPIO access is done.
> 
> TPS68470 Op Region driver:
> This is the driver that will be invoked, when the BIOS / firmware configures
> the voltage / clock for the sensors / vcm devices connected to the PMIC.
> 
> ---
> 
> Update on 2 GPIO chips implementation over 1:
>   - Attempted to implement 2 GPIO chips, but ran into couple of
> issues in the kernel, so we couldn't get it to work.
>   - It was decided to postpone this change, since it is not
> critical
> 
> Changes in v6:
>   - MFD driver:
>   - Used non-zero return values from regmap_write/read() calls
> to detect error conditions
> 
> Changes in v5:
>   - MFD driver:
>   - Fixed Kconfig description text
>   - Addressed other comments from Lee, related to formatting
> 
>   - GPIO driver:
>   - Formatted the file header text
> 
>   - Opregion driver:
>   - Formatted the file header text
> 
> Changes in v4:
>   - MFD driver:
>   - Removed board specific code and FIXME comment
>   - Moved i2c.h include from tps68470.h to tps68470.c
>   - Moved the TPS68470 REVID read code after PMIC reset
>   - Fixed typo in debug error message (on failure of
> devm_mfd_add_devices() )
>   - Enhanced dependency on I2C by changing it to I2C=y
> (to fix build errors if I2C is built as module
>  e.g tps68470.c:71: undefined reference to
> `__devm_regmap_init_i2c'
>  tps68470.c:117: undefined reference to `i2c_register_driver')
>   - Removed most of the unused header file definitions
>   - Moved devm_mfd_add_devices() after PMIC resett
>   - Used probe_new() and removed i2c_device_id table
> 
> The following patch from Andy is needed for the driver to be
> probed.
> http://marc.info/?l=linux-acpi=150030081523885=2
> 
>   - GPIO driver:
>   - Added newline at the end of Kconfig description
>   - Updated commit message about the descriptive
> names for the GPIOs and the typical usage model
> of the GPIO driver
> 
>   - Opregion driver:
>   - Added dependency on MFD_TPS68470
>   - Converted 2 liner into one line code
> 
> Changes in v3:
>   - MFD driver:
>   - Removed GPIO lookup table
>   - Reverted to probe() for consistency
>   - Addressed other comments from Andy
> 
>   - GPIO driver:
>   - Removed the code that initializes the default values
> of GPIOs to zeros
>   - Used gpiochip_get_data() to access data inside the gpio_chip
> 
> Changes in v2:
>   - MFD driver:
>   - Removed tps68470_* wrappers around regmap_* calls
>   - Removed "struct tps68470"
>   - used devm_mfd_add_devices and removed mutex in mfd driver
>   - Added reasoning about the need of having mfd driver
> as bool/builtin
> 
>   - Opregion driver:
>   - renamed opregion driver file / internal symbol names
> with tps68470_pmic*
>   - Made opregion driver tables as const
>   - Removed unused *handler_context in common handler
>   - Replaced "int" with "unsi

RE: [PATCH v6 0/3] TPS68470 PMIC drivers

[Mani, Rajmohan]

Hi All,

Now that Lee has applied the MFD patch 
(https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-mfd-next=6a58f36b0c49c8179804bead579f5ff6304f0245),
 can someone help clarify the next steps for the GPIO and ACPI / PMIC OpRegion 
patches of this series?

Both these patches depend on the MFD patch (MFD_TPS68470 config option) to 
compile.

Thanks
Raj

> -Original Message-
> From: Mani, Rajmohan
> Sent: Friday, July 28, 2017 5:30 PM
> To: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org
> Cc: Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael
> J. Wysocki ; Len Brown ;
> sakari.ai...@linux.intel.com; Andy Shevchenko
> ; Mani, Rajmohan
> 
> Subject: [PATCH v6 0/3] TPS68470 PMIC drivers
> 
> This is the patch series for TPS68470 PMIC that works as a camera PMIC.
> 
> The patch series provide the following 3 drivers, to help configure the 
> voltage
> regulators, clocks and GPIOs provided by the TPS68470 PMIC, to be able to
> use the camera sensors connected to this PMIC.
> 
> TPS68470 MFD driver:
> This is the multi function driver that initializes the TPS68470 PMIC and
> supports the GPIO and Op Region functions.
> 
> TPS68470 GPIO driver:
> This is the PMIC GPIO driver that will be used by the OS GPIO layer, when the
> BIOS / firmware triggered GPIO access is done.
> 
> TPS68470 Op Region driver:
> This is the driver that will be invoked, when the BIOS / firmware configures
> the voltage / clock for the sensors / vcm devices connected to the PMIC.
> 
> ---
> 
> Update on 2 GPIO chips implementation over 1:
>   - Attempted to implement 2 GPIO chips, but ran into couple of
> issues in the kernel, so we couldn't get it to work.
>   - It was decided to postpone this change, since it is not
> critical
> 
> Changes in v6:
>   - MFD driver:
>   - Used non-zero return values from regmap_write/read() calls
> to detect error conditions
> 
> Changes in v5:
>   - MFD driver:
>   - Fixed Kconfig description text
>   - Addressed other comments from Lee, related to formatting
> 
>   - GPIO driver:
>   - Formatted the file header text
> 
>   - Opregion driver:
>   - Formatted the file header text
> 
> Changes in v4:
>   - MFD driver:
>   - Removed board specific code and FIXME comment
>   - Moved i2c.h include from tps68470.h to tps68470.c
>   - Moved the TPS68470 REVID read code after PMIC reset
>   - Fixed typo in debug error message (on failure of
> devm_mfd_add_devices() )
>   - Enhanced dependency on I2C by changing it to I2C=y
> (to fix build errors if I2C is built as module
>  e.g tps68470.c:71: undefined reference to
> `__devm_regmap_init_i2c'
>  tps68470.c:117: undefined reference to `i2c_register_driver')
>   - Removed most of the unused header file definitions
>   - Moved devm_mfd_add_devices() after PMIC resett
>   - Used probe_new() and removed i2c_device_id table
> 
> The following patch from Andy is needed for the driver to be
> probed.
> http://marc.info/?l=linux-acpi=150030081523885=2
> 
>   - GPIO driver:
>   - Added newline at the end of Kconfig description
>   - Updated commit message about the descriptive
> names for the GPIOs and the typical usage model
> of the GPIO driver
> 
>   - Opregion driver:
>   - Added dependency on MFD_TPS68470
>   - Converted 2 liner into one line code
> 
> Changes in v3:
>   - MFD driver:
>   - Removed GPIO lookup table
>   - Reverted to probe() for consistency
>   - Addressed other comments from Andy
> 
>   - GPIO driver:
>   - Removed the code that initializes the default values
> of GPIOs to zeros
>   - Used gpiochip_get_data() to access data inside the gpio_chip
> 
> Changes in v2:
>   - MFD driver:
>   - Removed tps68470_* wrappers around regmap_* calls
>   - Removed "struct tps68470"
>   - used devm_mfd_add_devices and removed mutex in mfd driver
>   - Added reasoning about the need of having mfd driver
> as bool/builtin
> 
>   - Opregion driver:
>   - renamed opregion driver file / internal symbol names
> with tps68470_pmic*
>   - Made opregion driver tables as const
>   - Removed unused *handler_context in common handler
>   - Replaced "int" with "unsigned int"
>   - Changed to WARN macro to dev_warn()
>   - Destroyed mutex on error
>   - Added reasoning about the need of having Opregion driver
> as bool/bu

RE: [PATCH v6 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Thanks Lee.

Sorry for the noise, as some keystrokes sent out the empty response earlier.

Raj

> -Original Message-
> From: Mani, Rajmohan
> Sent: Tuesday, August 08, 2017 11:58 AM
> To: 'Lee Jones' <lee.jo...@linaro.org>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Linus Walleij <linus.wall...@linaro.org>; Alexandre
> Courbot <gnu...@gmail.com>; Rafael J. Wysocki <r...@rjwysocki.net>; Len
> Brown <l...@kernel.org>; sakari.ai...@linux.intel.com; Andy Shevchenko
> <andy.shevche...@gmail.com>
> Subject: RE: [PATCH v6 1/3] mfd: Add new mfd device TPS68470
> 
> 
> 
> > -Original Message-
> > From: Lee Jones [mailto:lee.jo...@linaro.org]
> > Sent: Tuesday, August 08, 2017 12:40 AM
> > To: Mani, Rajmohan <rajmohan.m...@intel.com>
> > Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> > a...@vger.kernel.org; Linus Walleij <linus.wall...@linaro.org>;
> > Alexandre Courbot <gnu...@gmail.com>; Rafael J. Wysocki
> > <r...@rjwysocki.net>; Len Brown <l...@kernel.org>;
> > sakari.ai...@linux.intel.com; Andy Shevchenko
> > <andy.shevche...@gmail.com>
> > Subject: Re: [PATCH v6 1/3] mfd: Add new mfd device TPS68470
> >
> > On Fri, 28 Jul 2017, Rajmohan Mani wrote:
> >
> > > The TPS68470 device is an advanced power management unit that
> powers
> > a
> > > Compact Camera Module (CCM), generates clocks for image sensors,
> > > drives a dual LED for Flash and incorporates two LED drivers for
> > > general purpose indicators.
> > >
> > > This patch adds support for TPS68470 mfd device.
> > >
> > > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > > ---
> > >  drivers/mfd/Kconfig  |  18 
> > >  drivers/mfd/Makefile |   1 +
> > >  drivers/mfd/tps68470.c   | 106
> > +++
> > >  include/linux/mfd/tps68470.h |  97
> > > +++
> > >  4 files changed, 222 insertions(+)
> > >  create mode 100644 drivers/mfd/tps68470.c  create mode 100644
> > > include/linux/mfd/tps68470.h
> >
> > Applied, thanks.
> >
> > --
> > Lee Jones
> > Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> > software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog

RE: [PATCH v6 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Thanks Lee.

Sorry for the noise, as some keystrokes sent out the empty response earlier.

Raj

> -Original Message-
> From: Mani, Rajmohan
> Sent: Tuesday, August 08, 2017 11:58 AM
> To: 'Lee Jones' 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Linus Walleij ; Alexandre
> Courbot ; Rafael J. Wysocki ; Len
> Brown ; sakari.ai...@linux.intel.com; Andy Shevchenko
> 
> Subject: RE: [PATCH v6 1/3] mfd: Add new mfd device TPS68470
> 
> 
> 
> > -Original Message-
> > From: Lee Jones [mailto:lee.jo...@linaro.org]
> > Sent: Tuesday, August 08, 2017 12:40 AM
> > To: Mani, Rajmohan 
> > Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> > a...@vger.kernel.org; Linus Walleij ;
> > Alexandre Courbot ; Rafael J. Wysocki
> > ; Len Brown ;
> > sakari.ai...@linux.intel.com; Andy Shevchenko
> > 
> > Subject: Re: [PATCH v6 1/3] mfd: Add new mfd device TPS68470
> >
> > On Fri, 28 Jul 2017, Rajmohan Mani wrote:
> >
> > > The TPS68470 device is an advanced power management unit that
> powers
> > a
> > > Compact Camera Module (CCM), generates clocks for image sensors,
> > > drives a dual LED for Flash and incorporates two LED drivers for
> > > general purpose indicators.
> > >
> > > This patch adds support for TPS68470 mfd device.
> > >
> > > Signed-off-by: Rajmohan Mani 
> > > ---
> > >  drivers/mfd/Kconfig  |  18 
> > >  drivers/mfd/Makefile |   1 +
> > >  drivers/mfd/tps68470.c   | 106
> > +++
> > >  include/linux/mfd/tps68470.h |  97
> > > +++
> > >  4 files changed, 222 insertions(+)
> > >  create mode 100644 drivers/mfd/tps68470.c  create mode 100644
> > > include/linux/mfd/tps68470.h
> >
> > Applied, thanks.
> >
> > --
> > Lee Jones
> > Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> > software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog

RE: [PATCH v6 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

> -Original Message-
> From: Lee Jones [mailto:lee.jo...@linaro.org]
> Sent: Tuesday, August 08, 2017 12:40 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Linus Walleij <linus.wall...@linaro.org>; Alexandre
> Courbot <gnu...@gmail.com>; Rafael J. Wysocki <r...@rjwysocki.net>; Len
> Brown <l...@kernel.org>; sakari.ai...@linux.intel.com; Andy Shevchenko
> <andy.shevche...@gmail.com>
> Subject: Re: [PATCH v6 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, 28 Jul 2017, Rajmohan Mani wrote:
> 
> > The TPS68470 device is an advanced power management unit that powers
> a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > ---
> >  drivers/mfd/Kconfig  |  18 
> >  drivers/mfd/Makefile |   1 +
> >  drivers/mfd/tps68470.c   | 106
> +++
> >  include/linux/mfd/tps68470.h |  97
> > +++
> >  4 files changed, 222 insertions(+)
> >  create mode 100644 drivers/mfd/tps68470.c  create mode 100644
> > include/linux/mfd/tps68470.h
> 
> Applied, thanks.
> 
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog

RE: [PATCH v6 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

> -Original Message-
> From: Lee Jones [mailto:lee.jo...@linaro.org]
> Sent: Tuesday, August 08, 2017 12:40 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Linus Walleij ; Alexandre
> Courbot ; Rafael J. Wysocki ; Len
> Brown ; sakari.ai...@linux.intel.com; Andy Shevchenko
> 
> Subject: Re: [PATCH v6 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, 28 Jul 2017, Rajmohan Mani wrote:
> 
> > The TPS68470 device is an advanced power management unit that powers
> a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> >
> > Signed-off-by: Rajmohan Mani 
> > ---
> >  drivers/mfd/Kconfig  |  18 
> >  drivers/mfd/Makefile |   1 +
> >  drivers/mfd/tps68470.c   | 106
> +++
> >  include/linux/mfd/tps68470.h |  97
> > +++
> >  4 files changed, 222 insertions(+)
> >  create mode 100644 drivers/mfd/tps68470.c  create mode 100644
> > include/linux/mfd/tps68470.h
> 
> Applied, thanks.
> 
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog

RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Tue, 25 Jul 2017, Mani, Rajmohan wrote:
> 
> > Hi Lee,
> >
> > > Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> > >
> > > On Fri, 21 Jul 2017, Mani, Rajmohan wrote:
> > > > > On Wed, 19 Jul 2017, Rajmohan Mani wrote:
> > > > >
> > > > > > The TPS68470 device is an advanced power management unit that
> > > > > > powers
> > > > > a
> > > > > > Compact Camera Module (CCM), generates clocks for image
> > > > > > sensors, drives a dual LED for Flash and incorporates two LED
> > > > > > drivers for general purpose indicators.
> > > > > >
> > > > > > This patch adds support for TPS68470 mfd device.
> > > > > >
> > > > > > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > > > > > ---
> > > > > >  drivers/mfd/Kconfig  |  18 +++
> > > > > >  drivers/mfd/Makefile |   1 +
> > > > > >  drivers/mfd/tps68470.c   | 110
> > > > > +++
> > > > > >  include/linux/mfd/tps68470.h |  97
> > > > > > ++
> > > > > >  4 files changed, 226 insertions(+)  create mode 100644
> > > > > > drivers/mfd/tps68470.c  create mode 100644
> > > > > > include/linux/mfd/tps68470.h
> > >
> > > [...]
> > >
> > > > > > +static const struct regmap_config tps68470_regmap_config = {
> > > > > > +   .reg_bits = 8,
> > > > > > +   .val_bits = 8,
> > > > > > +   .max_register = TPS68470_REG_MAX, };
> > > > > > +
> > > > > > +static int tps68470_chip_init(struct device *dev, struct
> > > > > > +regmap
> > > > > > +*regmap) {
> > > > > > +   unsigned int version;
> > > > > > +   int ret;
> > > > > > +
> > > > > > +   /* Force software reset */
> > > > > > +   ret = regmap_write(regmap, TPS68470_REG_RESET,
> > > > > TPS68470_REG_RESET_MASK);
> > > > > > +   if (ret < 0)
> > > > >
> > > > > Will 'if (!ret)' do?
> > > > >
> > > >
> > > > We intend to check error conditions and bail out. So, if (ret < 0)
> > > > works for
> > > this.
> > >
> > > Yes, 'if (!ret)' does that too.
> > >
> >
> > regmap_write() and regmap_read() functions return 0 on success. Hence
> we can not use 'if (!ret)' here, since we check for error conditions.
> 
> Sorry, this was a typo.
> 
> It should be 'if (ret)'.
> 
> What I'm trying to get at is the " < 0" is not required.
> 

Ack

> > > > > > +   return ret;
> > > > > > +
> > > > > > +   ret = regmap_read(regmap, TPS68470_REG_REVID,
> );
> > > > > > +   if (ret < 0) {
> > > > >
> > > > > Same
> > > > >
> > > >
> > > > We intend to check error conditions and bail out. So, if (ret < 0)
> > > > works for
> > > this.
> > >
> > > As above.
> > >
> >
> > Same as above
> >
> > > > > > +   dev_err(dev, "Failed to read revision register: %d\n",
> ret);
> > > > > > +   return ret;
> > > > > > +   }
> > > > > > +
> > > > > > +   dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
> > > > > > +
> > > > > > +   return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int tps68470_probe(struct i2c_client *client) {
> > > > > > +   struct device *dev = >dev;
> > > > > > +   struct regmap *regmap;
> > > > > > +   int ret;
> > > > > > +
> > > > > > +   regmap = devm_regmap_init_i2c(client,
> _regmap_config);
> > > > > > +   if (IS_ERR(regmap)) {
> > > > > > +   dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> > > > > > +   PTR_ERR(regmap));
> > > > > > +   return PTR_ERR(regmap);
> > > > > > +   }
> > > > > > +
> > > > > > +   i2c_set_clientdata(client, regmap);
> > > > > > +
> > > > > > +   ret = tps68470_chip_init(dev, regmap);
> > > > > > +   if (ret < 0) {
> > > > >
> > > > > Same
> > > > >
> > > >
> > > > We intend to check error conditions and bail out. So, if (ret < 0)
> > > > works for
> > > this.
> > >
> > > As above.
> > >
> >
> > Same as above
> 
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog

RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Tue, 25 Jul 2017, Mani, Rajmohan wrote:
> 
> > Hi Lee,
> >
> > > Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> > >
> > > On Fri, 21 Jul 2017, Mani, Rajmohan wrote:
> > > > > On Wed, 19 Jul 2017, Rajmohan Mani wrote:
> > > > >
> > > > > > The TPS68470 device is an advanced power management unit that
> > > > > > powers
> > > > > a
> > > > > > Compact Camera Module (CCM), generates clocks for image
> > > > > > sensors, drives a dual LED for Flash and incorporates two LED
> > > > > > drivers for general purpose indicators.
> > > > > >
> > > > > > This patch adds support for TPS68470 mfd device.
> > > > > >
> > > > > > Signed-off-by: Rajmohan Mani 
> > > > > > ---
> > > > > >  drivers/mfd/Kconfig  |  18 +++
> > > > > >  drivers/mfd/Makefile |   1 +
> > > > > >  drivers/mfd/tps68470.c   | 110
> > > > > +++
> > > > > >  include/linux/mfd/tps68470.h |  97
> > > > > > ++
> > > > > >  4 files changed, 226 insertions(+)  create mode 100644
> > > > > > drivers/mfd/tps68470.c  create mode 100644
> > > > > > include/linux/mfd/tps68470.h
> > >
> > > [...]
> > >
> > > > > > +static const struct regmap_config tps68470_regmap_config = {
> > > > > > +   .reg_bits = 8,
> > > > > > +   .val_bits = 8,
> > > > > > +   .max_register = TPS68470_REG_MAX, };
> > > > > > +
> > > > > > +static int tps68470_chip_init(struct device *dev, struct
> > > > > > +regmap
> > > > > > +*regmap) {
> > > > > > +   unsigned int version;
> > > > > > +   int ret;
> > > > > > +
> > > > > > +   /* Force software reset */
> > > > > > +   ret = regmap_write(regmap, TPS68470_REG_RESET,
> > > > > TPS68470_REG_RESET_MASK);
> > > > > > +   if (ret < 0)
> > > > >
> > > > > Will 'if (!ret)' do?
> > > > >
> > > >
> > > > We intend to check error conditions and bail out. So, if (ret < 0)
> > > > works for
> > > this.
> > >
> > > Yes, 'if (!ret)' does that too.
> > >
> >
> > regmap_write() and regmap_read() functions return 0 on success. Hence
> we can not use 'if (!ret)' here, since we check for error conditions.
> 
> Sorry, this was a typo.
> 
> It should be 'if (ret)'.
> 
> What I'm trying to get at is the " < 0" is not required.
> 

Ack

> > > > > > +   return ret;
> > > > > > +
> > > > > > +   ret = regmap_read(regmap, TPS68470_REG_REVID,
> );
> > > > > > +   if (ret < 0) {
> > > > >
> > > > > Same
> > > > >
> > > >
> > > > We intend to check error conditions and bail out. So, if (ret < 0)
> > > > works for
> > > this.
> > >
> > > As above.
> > >
> >
> > Same as above
> >
> > > > > > +   dev_err(dev, "Failed to read revision register: %d\n",
> ret);
> > > > > > +   return ret;
> > > > > > +   }
> > > > > > +
> > > > > > +   dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
> > > > > > +
> > > > > > +   return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int tps68470_probe(struct i2c_client *client) {
> > > > > > +   struct device *dev = >dev;
> > > > > > +   struct regmap *regmap;
> > > > > > +   int ret;
> > > > > > +
> > > > > > +   regmap = devm_regmap_init_i2c(client,
> _regmap_config);
> > > > > > +   if (IS_ERR(regmap)) {
> > > > > > +   dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> > > > > > +   PTR_ERR(regmap));
> > > > > > +   return PTR_ERR(regmap);
> > > > > > +   }
> > > > > > +
> > > > > > +   i2c_set_clientdata(client, regmap);
> > > > > > +
> > > > > > +   ret = tps68470_chip_init(dev, regmap);
> > > > > > +   if (ret < 0) {
> > > > >
> > > > > Same
> > > > >
> > > >
> > > > We intend to check error conditions and bail out. So, if (ret < 0)
> > > > works for
> > > this.
> > >
> > > As above.
> > >
> >
> > Same as above
> 
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source
> software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog

RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, Jul 28, 2017 at 3:30 AM, Mani, Rajmohan
> <rajmohan.m...@intel.com> wrote:
> >> On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones <lee.jo...@linaro.org>
> wrote:
> >> > On Tue, 25 Jul 2017, Andy Shevchenko wrote:
> >> >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones <lee.jo...@linaro.org>
> wrote:
> >>
> >> >> I briefly checked few ->read() and ->write() implementations and
> >> >> didn't find any evidence of positive numbers that can be returned.
> >> >> Documentation (kernel doc) doesn't shed a light on that. So, to me
> >> >> it sounds unspecified.
> >> >>
> >> >> So, for now (until documentation will be fixed) I would rely on if
> >> >> (ret < 0)
> >> >
> >> > It's not unspecified.  The regmap methods call into
> >> > regcache_write(), where the kerneldoc is clear.
> >>
> >
> > Since, we are interested in the regmap for the I2C bus here, I looked
> > into the implementation of
> >  __devm_regmap_init()
> > __regmap_init()
> > regcache_init()
> > for I2C bus.
> >
> > At the end of __devm_regmap_init() call from devm_regmap_init_i2c()
> inside tps68470_probe(), I see that both cache_bypass and defer_caching
> flags of i2c regmap struct are set. So, it looks regcache_write/read calls do
> not come into play here.
> >
> > So, regmap_write()
> > _regmap_write()
> > map->reg_write (drivers/base/regmap/regmap.c:1665) 
> > translates
> to
> > regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128)
> >
> > These checks in regmap_i2c_write() ensure all return values from
> i2c_master_send() other than the requested number of bytes to write, are
> converted into negative values.
> >
> > if (ret == count)
> > return 0;
> > else if (ret < 0)
> > return ret;
> > else
> > return -EIO;
> >
> > Similar argument goes for regmap_read() as well.
> > With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to 
> > be a
> better choice. Please see if I missed anything here.
> 
> It prooves exactly the Lee's point.
> 
> So, perhaps the best approach is to move to if (ret)  return ret;
> 
> ...if it will be a problem in the future, fix it accordingly.
> 

Ack.
We have spent enough time on this already.

RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, Jul 28, 2017 at 3:30 AM, Mani, Rajmohan
>  wrote:
> >> On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones 
> wrote:
> >> > On Tue, 25 Jul 2017, Andy Shevchenko wrote:
> >> >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones 
> wrote:
> >>
> >> >> I briefly checked few ->read() and ->write() implementations and
> >> >> didn't find any evidence of positive numbers that can be returned.
> >> >> Documentation (kernel doc) doesn't shed a light on that. So, to me
> >> >> it sounds unspecified.
> >> >>
> >> >> So, for now (until documentation will be fixed) I would rely on if
> >> >> (ret < 0)
> >> >
> >> > It's not unspecified.  The regmap methods call into
> >> > regcache_write(), where the kerneldoc is clear.
> >>
> >
> > Since, we are interested in the regmap for the I2C bus here, I looked
> > into the implementation of
> >  __devm_regmap_init()
> > __regmap_init()
> > regcache_init()
> > for I2C bus.
> >
> > At the end of __devm_regmap_init() call from devm_regmap_init_i2c()
> inside tps68470_probe(), I see that both cache_bypass and defer_caching
> flags of i2c regmap struct are set. So, it looks regcache_write/read calls do
> not come into play here.
> >
> > So, regmap_write()
> > _regmap_write()
> > map->reg_write (drivers/base/regmap/regmap.c:1665) 
> > translates
> to
> > regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128)
> >
> > These checks in regmap_i2c_write() ensure all return values from
> i2c_master_send() other than the requested number of bytes to write, are
> converted into negative values.
> >
> > if (ret == count)
> > return 0;
> > else if (ret < 0)
> > return ret;
> > else
> > return -EIO;
> >
> > Similar argument goes for regmap_read() as well.
> > With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to 
> > be a
> better choice. Please see if I missed anything here.
> 
> It prooves exactly the Lee's point.
> 
> So, perhaps the best approach is to move to if (ret)  return ret;
> 
> ...if it will be a problem in the future, fix it accordingly.
> 

Ack.
We have spent enough time on this already.

RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee, Andy,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones  wrote:
> > On Tue, 25 Jul 2017, Andy Shevchenko wrote:
> >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones  wrote:
> 
> >> I briefly checked few ->read() and ->write() implementations and
> >> didn't find any evidence of positive numbers that can be returned.
> >> Documentation (kernel doc) doesn't shed a light on that. So, to me it
> >> sounds unspecified.
> >>
> >> So, for now (until documentation will be fixed) I would rely on if
> >> (ret < 0)
> >
> > It's not unspecified.  The regmap methods call into regcache_write(),
> > where the kerneldoc is clear.
> 

Since, we are interested in the regmap for the I2C bus here, I looked into the 
implementation of
 __devm_regmap_init()
__regmap_init()
regcache_init()
for I2C bus.

At the end of __devm_regmap_init() call from devm_regmap_init_i2c() inside 
tps68470_probe(), I see that both cache_bypass and defer_caching flags of i2c 
regmap struct are set. So, it looks regcache_write/read calls do not come into 
play here.

So, regmap_write()
_regmap_write()
map->reg_write (drivers/base/regmap/regmap.c:1665) translates to
regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128)

These checks in regmap_i2c_write() ensure all return values from 
i2c_master_send() other than the requested number of bytes to write, are 
converted into negative values.

if (ret == count)
return 0;
else if (ret < 0)
return ret;
else
return -EIO;

Similar argument goes for regmap_read() as well.
With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to be a 
better choice. Please see if I missed anything here.

> drivers/base/regmap/regcache.c:266
> 
> " * Return a negative value on failure, 0 on success."
> 
> I can hardly find this any cleaner than "unspecified".
> 
> >  Perhaps we can also change the regmap kerneldoc headers too to match,
> > which might lessen the disparity.
> 
> I'm not familiar with the guts of regmap API, so, I would stick with if (ret 
> < 0)
> for now until documentation specifies positive return values.
> 

Ack

RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee, Andy,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Wed, Jul 26, 2017 at 11:23 AM, Lee Jones  wrote:
> > On Tue, 25 Jul 2017, Andy Shevchenko wrote:
> >> On Tue, Jul 25, 2017 at 12:10 PM, Lee Jones  wrote:
> 
> >> I briefly checked few ->read() and ->write() implementations and
> >> didn't find any evidence of positive numbers that can be returned.
> >> Documentation (kernel doc) doesn't shed a light on that. So, to me it
> >> sounds unspecified.
> >>
> >> So, for now (until documentation will be fixed) I would rely on if
> >> (ret < 0)
> >
> > It's not unspecified.  The regmap methods call into regcache_write(),
> > where the kerneldoc is clear.
> 

Since, we are interested in the regmap for the I2C bus here, I looked into the 
implementation of
 __devm_regmap_init()
__regmap_init()
regcache_init()
for I2C bus.

At the end of __devm_regmap_init() call from devm_regmap_init_i2c() inside 
tps68470_probe(), I see that both cache_bypass and defer_caching flags of i2c 
regmap struct are set. So, it looks regcache_write/read calls do not come into 
play here.

So, regmap_write()
_regmap_write()
map->reg_write (drivers/base/regmap/regmap.c:1665) translates to
regmap_i2c_write(drivers/base/regmap/regmap-i2c.c:128)

These checks in regmap_i2c_write() ensure all return values from 
i2c_master_send() other than the requested number of bytes to write, are 
converted into negative values.

if (ret == count)
return 0;
else if (ret < 0)
return ret;
else
return -EIO;

Similar argument goes for regmap_read() as well.
With that, for regmap over I2C bus, it sounds like 'if (ret < 0)' looks to be a 
better choice. Please see if I missed anything here.

> drivers/base/regmap/regcache.c:266
> 
> " * Return a negative value on failure, 0 on success."
> 
> I can hardly find this any cleaner than "unspecified".
> 
> >  Perhaps we can also change the regmap kerneldoc headers too to match,
> > which might lessen the disparity.
> 
> I'm not familiar with the guts of regmap API, so, I would stick with if (ret 
> < 0)
> for now until documentation specifies positive return values.
> 

Ack

RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, 21 Jul 2017, Mani, Rajmohan wrote:
> > > On Wed, 19 Jul 2017, Rajmohan Mani wrote:
> > >
> > > > The TPS68470 device is an advanced power management unit that
> > > > powers
> > > a
> > > > Compact Camera Module (CCM), generates clocks for image sensors,
> > > > drives a dual LED for Flash and incorporates two LED drivers for
> > > > general purpose indicators.
> > > >
> > > > This patch adds support for TPS68470 mfd device.
> > > >
> > > > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > > > ---
> > > >  drivers/mfd/Kconfig  |  18 +++
> > > >  drivers/mfd/Makefile |   1 +
> > > >  drivers/mfd/tps68470.c   | 110
> > > +++
> > > >  include/linux/mfd/tps68470.h |  97
> > > > ++
> > > >  4 files changed, 226 insertions(+)  create mode 100644
> > > > drivers/mfd/tps68470.c  create mode 100644
> > > > include/linux/mfd/tps68470.h
> 
> [...]
> 
> > > > +static const struct regmap_config tps68470_regmap_config = {
> > > > +   .reg_bits = 8,
> > > > +   .val_bits = 8,
> > > > +   .max_register = TPS68470_REG_MAX, };
> > > > +
> > > > +static int tps68470_chip_init(struct device *dev, struct regmap
> > > > +*regmap) {
> > > > +   unsigned int version;
> > > > +   int ret;
> > > > +
> > > > +   /* Force software reset */
> > > > +   ret = regmap_write(regmap, TPS68470_REG_RESET,
> > > TPS68470_REG_RESET_MASK);
> > > > +   if (ret < 0)
> > >
> > > Will 'if (!ret)' do?
> > >
> >
> > We intend to check error conditions and bail out. So, if (ret < 0) works for
> this.
> 
> Yes, 'if (!ret)' does that too.
> 

regmap_write() and regmap_read() functions return 0 on success. Hence we can 
not use 'if (!ret)' here, since we check for error conditions.

> > > > +   return ret;
> > > > +
> > > > +   ret = regmap_read(regmap, TPS68470_REG_REVID, );
> > > > +   if (ret < 0) {
> > >
> > > Same
> > >
> >
> > We intend to check error conditions and bail out. So, if (ret < 0) works for
> this.
> 
> As above.
> 

Same as above

> > > > +   dev_err(dev, "Failed to read revision register: %d\n", 
> > > > ret);
> > > > +   return ret;
> > > > +   }
> > > > +
> > > > +   dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +static int tps68470_probe(struct i2c_client *client) {
> > > > +   struct device *dev = >dev;
> > > > +   struct regmap *regmap;
> > > > +   int ret;
> > > > +
> > > > +   regmap = devm_regmap_init_i2c(client, _regmap_config);
> > > > +   if (IS_ERR(regmap)) {
> > > > +   dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> > > > +   PTR_ERR(regmap));
> > > > +   return PTR_ERR(regmap);
> > > > +   }
> > > > +
> > > > +   i2c_set_clientdata(client, regmap);
> > > > +
> > > > +   ret = tps68470_chip_init(dev, regmap);
> > > > +   if (ret < 0) {
> > >
> > > Same
> > >
> >
> > We intend to check error conditions and bail out. So, if (ret < 0) works for
> this.
> 
> As above.
> 

Same as above

RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee,

> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, 21 Jul 2017, Mani, Rajmohan wrote:
> > > On Wed, 19 Jul 2017, Rajmohan Mani wrote:
> > >
> > > > The TPS68470 device is an advanced power management unit that
> > > > powers
> > > a
> > > > Compact Camera Module (CCM), generates clocks for image sensors,
> > > > drives a dual LED for Flash and incorporates two LED drivers for
> > > > general purpose indicators.
> > > >
> > > > This patch adds support for TPS68470 mfd device.
> > > >
> > > > Signed-off-by: Rajmohan Mani 
> > > > ---
> > > >  drivers/mfd/Kconfig  |  18 +++
> > > >  drivers/mfd/Makefile |   1 +
> > > >  drivers/mfd/tps68470.c   | 110
> > > +++
> > > >  include/linux/mfd/tps68470.h |  97
> > > > ++
> > > >  4 files changed, 226 insertions(+)  create mode 100644
> > > > drivers/mfd/tps68470.c  create mode 100644
> > > > include/linux/mfd/tps68470.h
> 
> [...]
> 
> > > > +static const struct regmap_config tps68470_regmap_config = {
> > > > +   .reg_bits = 8,
> > > > +   .val_bits = 8,
> > > > +   .max_register = TPS68470_REG_MAX, };
> > > > +
> > > > +static int tps68470_chip_init(struct device *dev, struct regmap
> > > > +*regmap) {
> > > > +   unsigned int version;
> > > > +   int ret;
> > > > +
> > > > +   /* Force software reset */
> > > > +   ret = regmap_write(regmap, TPS68470_REG_RESET,
> > > TPS68470_REG_RESET_MASK);
> > > > +   if (ret < 0)
> > >
> > > Will 'if (!ret)' do?
> > >
> >
> > We intend to check error conditions and bail out. So, if (ret < 0) works for
> this.
> 
> Yes, 'if (!ret)' does that too.
> 

regmap_write() and regmap_read() functions return 0 on success. Hence we can 
not use 'if (!ret)' here, since we check for error conditions.

> > > > +   return ret;
> > > > +
> > > > +   ret = regmap_read(regmap, TPS68470_REG_REVID, );
> > > > +   if (ret < 0) {
> > >
> > > Same
> > >
> >
> > We intend to check error conditions and bail out. So, if (ret < 0) works for
> this.
> 
> As above.
> 

Same as above

> > > > +   dev_err(dev, "Failed to read revision register: %d\n", 
> > > > ret);
> > > > +   return ret;
> > > > +   }
> > > > +
> > > > +   dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +static int tps68470_probe(struct i2c_client *client) {
> > > > +   struct device *dev = >dev;
> > > > +   struct regmap *regmap;
> > > > +   int ret;
> > > > +
> > > > +   regmap = devm_regmap_init_i2c(client, _regmap_config);
> > > > +   if (IS_ERR(regmap)) {
> > > > +   dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> > > > +   PTR_ERR(regmap));
> > > > +   return PTR_ERR(regmap);
> > > > +   }
> > > > +
> > > > +   i2c_set_clientdata(client, regmap);
> > > > +
> > > > +   ret = tps68470_chip_init(dev, regmap);
> > > > +   if (ret < 0) {
> > >
> > > Same
> > >
> >
> > We intend to check error conditions and bail out. So, if (ret < 0) works for
> this.
> 
> As above.
> 

Same as above

RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee,

> > > + * 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 version 2.
> > > + *
> > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > > + * kind, whether express or implied; without even the implied
> > > + warranty
> > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> the
> > > + * GNU General Public License for more details.
> >
> > Can you use the short version?
> >
> 
> Ack
> I will update the other patches of this series too.
> 

I just confirmed that we are recommended to use the above long version of 
headers. So, these headers will stay as such.

Thanks
Raj

RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee,

> > > + * 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 version 2.
> > > + *
> > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > > + * kind, whether express or implied; without even the implied
> > > + warranty
> > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See
> the
> > > + * GNU General Public License for more details.
> >
> > Can you use the short version?
> >
> 
> Ack
> I will update the other patches of this series too.
> 

I just confirmed that we are recommended to use the above long version of 
headers. So, these headers will stay as such.

Thanks
Raj

RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee,

Thanks for the reviews.

> -Original Message-
> From: Lee Jones [mailto:lee.jo...@linaro.org]
> Sent: Thursday, July 20, 2017 2:03 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Linus Walleij <linus.wall...@linaro.org>; Alexandre
> Courbot <gnu...@gmail.com>; Rafael J. Wysocki <r...@rjwysocki.net>; Len
> Brown <l...@kernel.org>; sakari.ai...@linux.intel.com; Andy Shevchenko
> <andy.shevche...@gmail.com>
> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Wed, 19 Jul 2017, Rajmohan Mani wrote:
> 
> > The TPS68470 device is an advanced power management unit that powers
> a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > ---
> >  drivers/mfd/Kconfig  |  18 +++
> >  drivers/mfd/Makefile |   1 +
> >  drivers/mfd/tps68470.c   | 110
> +++
> >  include/linux/mfd/tps68470.h |  97
> > ++
> >  4 files changed, 226 insertions(+)
> >  create mode 100644 drivers/mfd/tps68470.c  create mode 100644
> > include/linux/mfd/tps68470.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > 3eb5c93..960be43 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1311,6 +1311,24 @@ config MFD_TPS65217
> >   This driver can also be built as a module.  If so, the module
> >   will be called tps65217.
> >
> > +config MFD_TPS68470
> > +   bool "TI TPS68470 Power Management / LED chips"
> > +   depends on ACPI && I2C=y
> > +   select MFD_CORE
> > +   select REGMAP_I2C
> > +   select I2C_DESIGNWARE_PLATFORM
> > +   help
> > + If you say yes here you get support for the TPS68470 series of
> > + Power Management / LED chips.
> > +
> > + These include voltage regulators, led and other features
> 
> LED(s)
> 

Ack

> > + that are often used in portable devices.
> > +
> > + This option is a bool as it provides an ACPI operation
> > + region, which must be available before any of the devices
> > + using this, are probed. This option also configures the
> 
> Remove the ','.
> 

Ack

> > + designware-i2c driver to be builtin, for the same reason.
> 
> built-in
> 

Ack

> > +
> >  config MFD_TI_LP873X
> > tristate "TI LP873X Power Management IC"
> > depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > c16bf1e..6dd2b94 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -82,6 +82,7 @@ obj-$(CONFIG_MFD_TPS65910)+= tps65910.o
> >  obj-$(CONFIG_MFD_TPS65912) += tps65912-core.o
> >  obj-$(CONFIG_MFD_TPS65912_I2C) += tps65912-i2c.o
> >  obj-$(CONFIG_MFD_TPS65912_SPI)  += tps65912-spi.o
> > +obj-$(CONFIG_MFD_TPS68470) += tps68470.o
> >  obj-$(CONFIG_MFD_TPS80031) += tps80031.o
> >  obj-$(CONFIG_MENELAUS) += menelaus.o
> >
> > diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c new file
> > mode 100644 index 000..a692af7
> > --- /dev/null
> > +++ b/drivers/mfd/tps68470.c
> > @@ -0,0 +1,110 @@
> > +/*
> > + * TPS68470 chip family multi-function driver
> 
> Does it really cover a family?  If so 'TPS68470' seems very specific.
> 
> "Multi-Function Driver" or even better "Core" or "Parent" driver.
> 

No. This is just for TPS68470.
I picked "Parent" driver

> > + * Copyright (C) 2017 Intel Corporation
> 
> '\n' here.
> 

Ack

> > + * Authors:
> > + * Rajmohan Mani <rajmohan.m...@intel.com>
> > + * Tianshu Qiu <tian.shu@intel.com>
> > + * Jian Xu Zheng <jian.xu.zh...@intel.com>
> > + * Yuning Pu <yuning...@intel.com>
> 
> Tab these out:
> 
>  * Authors:
>  *Rajmohan Mani <rajmohan.m...@intel.com>
>  *Tianshu Qiu <tian.shu@intel.com>
>  *Jian Xu Zheng <jian.xu.zh...@intel.com>
>  *Yuning Pu <yuning...@intel.com>
> 

Ack

> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * pu

RE: [PATCH v4 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee,

Thanks for the reviews.

> -Original Message-
> From: Lee Jones [mailto:lee.jo...@linaro.org]
> Sent: Thursday, July 20, 2017 2:03 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Linus Walleij ; Alexandre
> Courbot ; Rafael J. Wysocki ; Len
> Brown ; sakari.ai...@linux.intel.com; Andy Shevchenko
> 
> Subject: Re: [PATCH v4 1/3] mfd: Add new mfd device TPS68470
> 
> On Wed, 19 Jul 2017, Rajmohan Mani wrote:
> 
> > The TPS68470 device is an advanced power management unit that powers
> a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> >
> > Signed-off-by: Rajmohan Mani 
> > ---
> >  drivers/mfd/Kconfig  |  18 +++
> >  drivers/mfd/Makefile |   1 +
> >  drivers/mfd/tps68470.c   | 110
> +++
> >  include/linux/mfd/tps68470.h |  97
> > ++
> >  4 files changed, 226 insertions(+)
> >  create mode 100644 drivers/mfd/tps68470.c  create mode 100644
> > include/linux/mfd/tps68470.h
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index
> > 3eb5c93..960be43 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1311,6 +1311,24 @@ config MFD_TPS65217
> >   This driver can also be built as a module.  If so, the module
> >   will be called tps65217.
> >
> > +config MFD_TPS68470
> > +   bool "TI TPS68470 Power Management / LED chips"
> > +   depends on ACPI && I2C=y
> > +   select MFD_CORE
> > +   select REGMAP_I2C
> > +   select I2C_DESIGNWARE_PLATFORM
> > +   help
> > + If you say yes here you get support for the TPS68470 series of
> > + Power Management / LED chips.
> > +
> > + These include voltage regulators, led and other features
> 
> LED(s)
> 

Ack

> > + that are often used in portable devices.
> > +
> > + This option is a bool as it provides an ACPI operation
> > + region, which must be available before any of the devices
> > + using this, are probed. This option also configures the
> 
> Remove the ','.
> 

Ack

> > + designware-i2c driver to be builtin, for the same reason.
> 
> built-in
> 

Ack

> > +
> >  config MFD_TI_LP873X
> > tristate "TI LP873X Power Management IC"
> > depends on I2C
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index
> > c16bf1e..6dd2b94 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -82,6 +82,7 @@ obj-$(CONFIG_MFD_TPS65910)+= tps65910.o
> >  obj-$(CONFIG_MFD_TPS65912) += tps65912-core.o
> >  obj-$(CONFIG_MFD_TPS65912_I2C) += tps65912-i2c.o
> >  obj-$(CONFIG_MFD_TPS65912_SPI)  += tps65912-spi.o
> > +obj-$(CONFIG_MFD_TPS68470) += tps68470.o
> >  obj-$(CONFIG_MFD_TPS80031) += tps80031.o
> >  obj-$(CONFIG_MENELAUS) += menelaus.o
> >
> > diff --git a/drivers/mfd/tps68470.c b/drivers/mfd/tps68470.c new file
> > mode 100644 index 000..a692af7
> > --- /dev/null
> > +++ b/drivers/mfd/tps68470.c
> > @@ -0,0 +1,110 @@
> > +/*
> > + * TPS68470 chip family multi-function driver
> 
> Does it really cover a family?  If so 'TPS68470' seems very specific.
> 
> "Multi-Function Driver" or even better "Core" or "Parent" driver.
> 

No. This is just for TPS68470.
I picked "Parent" driver

> > + * Copyright (C) 2017 Intel Corporation
> 
> '\n' here.
> 

Ack

> > + * Authors:
> > + * Rajmohan Mani 
> > + * Tianshu Qiu 
> > + * Jian Xu Zheng 
> > + * Yuning Pu 
> 
> Tab these out:
> 
>  * Authors:
>  *Rajmohan Mani 
>  *Tianshu Qiu 
>  *Jian Xu Zheng 
>  *Yuning Pu 
> 

Ack

> > + * 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 version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied
> > + warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> 
> Can you use the short version?
> 

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee,

> > On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> >
> > > Hi Andy,
> > >
> > > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > > <rajmohan.m...@intel.com> wrote:
> > > > > > The TPS68470 device is an advanced power management unit that
> > > > > > powers a Compact Camera Module (CCM), generates clocks for
> > > > > > image sensors, drives a dual LED for Flash and incorporates
> > > > > > two LED drivers for general purpose indicators.
> > > > > >
> > > > > > This patch adds support for TPS68470 mfd device.
> > > > >
> > > > > I dunno why you decide to send this out now, see my comments
> below.
> > > > >
> > > > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > > > +   unsigned int version;
> > > > > > +   int ret;
> > > > >
> > > > > > +   /* FIXME: configure these dynamically */
> > > > >
> > > > > So, what prevents you to fix this?
> > > >
> > > > Nothing I suppose. They're however not needed right now and can be
> > > > implemented later on if they're ever needed.
> > > >
> > >
> > > Ack
> >
> > What does this mean?  Is the plan to fix it or not?  I don't want
> > FIXMEs in the code that a) can be fixed right away or b) might never be
> fixed.
> >
> 
> I meant that this can be implemented later on, if there's a need.
> I will look into this and see how this can be fixed.
> 

Thanks for your patience.
This is fixed with v4 of this series.

Raj

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee,

> > On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> >
> > > Hi Andy,
> > >
> > > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > >  wrote:
> > > > > > The TPS68470 device is an advanced power management unit that
> > > > > > powers a Compact Camera Module (CCM), generates clocks for
> > > > > > image sensors, drives a dual LED for Flash and incorporates
> > > > > > two LED drivers for general purpose indicators.
> > > > > >
> > > > > > This patch adds support for TPS68470 mfd device.
> > > > >
> > > > > I dunno why you decide to send this out now, see my comments
> below.
> > > > >
> > > > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > > > +   unsigned int version;
> > > > > > +   int ret;
> > > > >
> > > > > > +   /* FIXME: configure these dynamically */
> > > > >
> > > > > So, what prevents you to fix this?
> > > >
> > > > Nothing I suppose. They're however not needed right now and can be
> > > > implemented later on if they're ever needed.
> > > >
> > >
> > > Ack
> >
> > What does this mean?  Is the plan to fix it or not?  I don't want
> > FIXMEs in the code that a) can be fixed right away or b) might never be
> fixed.
> >
> 
> I meant that this can be implemented later on, if there's a need.
> I will look into this and see how this can be fixed.
> 

Thanks for your patience.
This is fixed with v4 of this series.

Raj

RE: [PATCH v2 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy and all,

Thanks for your patience.

> >
> > Main points (some I already told in an answer to Sakari's mail):
> > 1. Consider 2 GPIO chips over 1.
> 
> I am looking into this.
> 

For the second GPIO chip, the call to acpi_attach_data() fails with 
AE_ALREADY_EXISTS, as below with 4.12 rc3 kernel.

acpi_gpiochip_add()
acpi_attach_data() 
acpi_ns_attach_data() fails with AE_ALREADY_EXISTS for the 
second GPIO chip.
/* We only allow one attachment per handler */ 
drivers/acpi/acpica/nsobject.c

Also, I tried the following experiment, since we know the number of GPIOs for 
the second GPIO chip is 3, just to see if I can get further along after a 
successful call to acpi_attach_data().

Since the acpi_gpio_chip_dh is used as the handler in all acpi_gpiochip_* 
calls, I tried to use another handler (acpi_gpio_chip_dh2). But now, as 
expected, acpi_gpiochip_request_regions() failed inside acpi_gpiochip_add(), as 
only one OpRegion handler can be installed for a given OpRegion code. Due to 
this, when I try to set the GPIO 9 (which is now GPIO 2 of the second GPIO chip 
of 2 GPIO chips implementation), the OpRegion handler is called with the 
information / region_context pointer of the first GPIO chip, which is not 
desired.

It sounds like there are 2 issues with the 2 GPIO chips implementation.

1) How to get acpi_ns_attach_data() done successfully for the second GPIO chip, 
given the observation above and

2) How to get the GPIO OpRegion handler to be called with the right GPIO chip 
information / region_context pointer

I need pointers to make further progress here.

> > 2. Fix FIXME(s).
> 
> Leaving this on, until I see how this can be fixed.
> 

These FIXME code will be removed from the driver, as we are working to get this 
done through platform firmware.

> > 3. If there is hardware bug we should work around it must be clarified.
> 
> Ack
> If this is about initializing the GPIOs with zero, I have removed this code 
> for
> now.
> 
> > 4. You missed Linus' comments here (switch to the data pointer inside
> > GPIO chip and remove platform driver data stuff from the driver).
> >
> 
> I have fixed this with v3

RE: [PATCH v2 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy and all,

Thanks for your patience.

> >
> > Main points (some I already told in an answer to Sakari's mail):
> > 1. Consider 2 GPIO chips over 1.
> 
> I am looking into this.
> 

For the second GPIO chip, the call to acpi_attach_data() fails with 
AE_ALREADY_EXISTS, as below with 4.12 rc3 kernel.

acpi_gpiochip_add()
acpi_attach_data() 
acpi_ns_attach_data() fails with AE_ALREADY_EXISTS for the 
second GPIO chip.
/* We only allow one attachment per handler */ 
drivers/acpi/acpica/nsobject.c

Also, I tried the following experiment, since we know the number of GPIOs for 
the second GPIO chip is 3, just to see if I can get further along after a 
successful call to acpi_attach_data().

Since the acpi_gpio_chip_dh is used as the handler in all acpi_gpiochip_* 
calls, I tried to use another handler (acpi_gpio_chip_dh2). But now, as 
expected, acpi_gpiochip_request_regions() failed inside acpi_gpiochip_add(), as 
only one OpRegion handler can be installed for a given OpRegion code. Due to 
this, when I try to set the GPIO 9 (which is now GPIO 2 of the second GPIO chip 
of 2 GPIO chips implementation), the OpRegion handler is called with the 
information / region_context pointer of the first GPIO chip, which is not 
desired.

It sounds like there are 2 issues with the 2 GPIO chips implementation.

1) How to get acpi_ns_attach_data() done successfully for the second GPIO chip, 
given the observation above and

2) How to get the GPIO OpRegion handler to be called with the right GPIO chip 
information / region_context pointer

I need pointers to make further progress here.

> > 2. Fix FIXME(s).
> 
> Leaving this on, until I see how this can be fixed.
> 

These FIXME code will be removed from the driver, as we are working to get this 
done through platform firmware.

> > 3. If there is hardware bug we should work around it must be clarified.
> 
> Ack
> If this is about initializing the GPIOs with zero, I have removed this code 
> for
> now.
> 
> > 4. You missed Linus' comments here (switch to the data pointer inside
> > GPIO chip and remove platform driver data stuff from the driver).
> >
> 
> I have fixed this with v3

RE: [PATCH v3 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

Thanks for your comments.

> On Mon, Jun 12, 2017 at 11:26 AM, Rajmohan Mani
>  wrote:
> 
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> >
> > Signed-off-by: Rajmohan Mani 
> 
> This is some fine code.
> Reviewed-by: Linus Walleij 
> 
> Feel free to merge this through MFD or tell me if I should merge it if there 
> are
> no compile-time dependencies.

I still have the following 3 outstanding comments from Andy (that includes one 
from Lee) on this patch series.

Main points (some I already told in an answer to Sakari's mail):

1. Consider 2 GPIO chips over 1.(Andy on GPIO driver)
 I will look into this week and get back on the issue that I find with using 2 
GPIO chips.

2. Fix FIXME(s) (Andy and Lee on MFD driver)
I will work on this once I am done with 1.

+   /* FIXME: configure these dynamically */
+   /* Enable Daisy Chain LDO and configure relevant GPIOs as output */
+   ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 2);
+   if (ret < 0)
+   return ret;
+
+   ret = regmap_write(regmap, TPS68470_REG_GPCTL4A, 2);
+   if (ret < 0)
+   return ret;
+
+   ret = regmap_write(regmap, TPS68470_REG_GPCTL5A, 2);
+   if (ret < 0)
+   return ret;
+
+   ret = regmap_write(regmap, TPS68470_REG_GPCTL6A, 2);
+   if (ret < 0)
+   return ret;
+
+   /*
+* When SDA and SCL are routed to GPIO1 and GPIO2, the mode
+* for these GPIOs must be configured using their respective
+* GPCTLxA registers as inputs with no pull-ups.
+*/
+   ret = regmap_write(regmap, TPS68470_REG_GPCTL1A, 0);
+   if (ret < 0)
+   return ret;
+
+   ret = regmap_write(regmap, TPS68470_REG_GPCTL2A, 0);
+   if (ret < 0)
+   return ret;
+
+   /* Enable daisy chain */
+   ret = regmap_update_bits(regmap, TPS68470_REG_S_I2C_CTL, 1, 1);
+   if (ret < 0)
+   return ret;

3. Move  from tps68470.h to tps68470.c (Andy on MFD driver)
This is done, but waiting on 1 and 2

Thanks
Raj

RE: [PATCH v3 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

Thanks for your comments.

> On Mon, Jun 12, 2017 at 11:26 AM, Rajmohan Mani
>  wrote:
> 
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> >
> > Signed-off-by: Rajmohan Mani 
> 
> This is some fine code.
> Reviewed-by: Linus Walleij 
> 
> Feel free to merge this through MFD or tell me if I should merge it if there 
> are
> no compile-time dependencies.

I still have the following 3 outstanding comments from Andy (that includes one 
from Lee) on this patch series.

Main points (some I already told in an answer to Sakari's mail):

1. Consider 2 GPIO chips over 1.(Andy on GPIO driver)
 I will look into this week and get back on the issue that I find with using 2 
GPIO chips.

2. Fix FIXME(s) (Andy and Lee on MFD driver)
I will work on this once I am done with 1.

+   /* FIXME: configure these dynamically */
+   /* Enable Daisy Chain LDO and configure relevant GPIOs as output */
+   ret = regmap_write(regmap, TPS68470_REG_S_I2C_CTL, 2);
+   if (ret < 0)
+   return ret;
+
+   ret = regmap_write(regmap, TPS68470_REG_GPCTL4A, 2);
+   if (ret < 0)
+   return ret;
+
+   ret = regmap_write(regmap, TPS68470_REG_GPCTL5A, 2);
+   if (ret < 0)
+   return ret;
+
+   ret = regmap_write(regmap, TPS68470_REG_GPCTL6A, 2);
+   if (ret < 0)
+   return ret;
+
+   /*
+* When SDA and SCL are routed to GPIO1 and GPIO2, the mode
+* for these GPIOs must be configured using their respective
+* GPCTLxA registers as inputs with no pull-ups.
+*/
+   ret = regmap_write(regmap, TPS68470_REG_GPCTL1A, 0);
+   if (ret < 0)
+   return ret;
+
+   ret = regmap_write(regmap, TPS68470_REG_GPCTL2A, 0);
+   if (ret < 0)
+   return ret;
+
+   /* Enable daisy chain */
+   ret = regmap_update_bits(regmap, TPS68470_REG_S_I2C_CTL, 1, 1);
+   if (ret < 0)
+   return ret;

3. Move  from tps68470.h to tps68470.c (Andy on MFD driver)
This is done, but waiting on 1 and 2

Thanks
Raj

RE: [PATCH v3 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

> On Mon, Jun 12, 2017 at 12:26 PM, Rajmohan Mani
>  wrote:
> > The TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> 
> > +#include 
> 
> If you are going to ignore reviewers' comments you may delay the patch
> upstreaming for quite a bit.
> 

Sorry. The changes were in my git repo, but didn't make it to the v3 patch list 
(since I didn't commit and amend the patch).
I will hold onto this for now (instead of pushing v4 with just this change).

> P.S. For now I don't see any priority to review v3.
> I noted more issues that weren't addressed.
> 
> Sorry.
> 
> --
> With Best Regards,
> Andy Shevchenko

RE: [PATCH v3 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

> On Mon, Jun 12, 2017 at 12:26 PM, Rajmohan Mani
>  wrote:
> > The TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> 
> > +#include 
> 
> If you are going to ignore reviewers' comments you may delay the patch
> upstreaming for quite a bit.
> 

Sorry. The changes were in my git repo, but didn't make it to the v3 patch list 
(since I didn't commit and amend the patch).
I will hold onto this for now (instead of pushing v4 with just this change).

> P.S. For now I don't see any priority to review v3.
> I noted more issues that weren't addressed.
> 
> Sorry.
> 
> --
> With Best Regards,
> Andy Shevchenko

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Sakari,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> Hi Rajmohan and Andy,
> 
> On Sun, Jun 11, 2017 at 03:49:18AM +, Mani, Rajmohan wrote:
> > Hi Andy,
> >
> > Thanks for the reviews and patience.
> >
> > >
> > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > <rajmohan.m...@intel.com>
> > > wrote:
> > > > This patch adds support for TPS68470 GPIOs.
> > > > There are 7 GPIOs and a few sensor related GPIOs.
> > > > These GPIOs can be requested and configured as appropriate.
> > >
> > > Besides my below comments, just put it here that I recommended
> > > earlier to provide 2 GPIO chips (one per bank of GPIOs).
> > > It's up to Linus to decide since you didn't follow the recommendation.
> > >
> >
> > Ack.
> > Did you mean to add this in Kconfig or this source file?
> >
> > Here's some more details on these GPIOs.
> > Each of these 7 GPIOs has 2 registers to control the mode, level, drive
> strength, polarity, hysteresis control among other things. Also there are GPDI
> and GPDO registers to control the input and output values of these 7 GPIOs.
> These GPIOs are numbered 0 through 6.
> > The remaining 3 GPIOs are more of special purpose GPIOs that are output
> only, with one register to control all of their output values and drive 
> strengths.
> These GPIOs are named with a special purpose (ENABLE, IDLE and RESET of the
> sensor).
> >
> > > > +#include 
> > > > +#include 
> > >
> > > These shouldn't be in the driver.
> > > Instead use
> > > #include 
> > >
> >
> > Ack
> >
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > >
> > > > +   if (offset >= TPS68470_N_REGULAR_GPIO) {
> > > > +   offset -= TPS68470_N_REGULAR_GPIO;
> > > > +   reg = TPS68470_REG_SGPO;
> > > > +   }
> > >
> > > Two GPIO chips makes this gone.
> 
> Again, I'm not really worried about this driver, but the ACPI tables. How does
> the difference show there?
> 
> The outputs (s_enable, s_idle and s_resetn) are not numbered in the
> documentation. There grouped, though, but the order in that grouping varies.
> 
> > > > +struct gpiod_lookup_table gpios_table = {
> > > > +   .dev_id = NULL,
> > > > +   .table = {
> > > > + GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> > > GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 8, "s_idle",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> > > GPIO_ACTIVE_HIGH),
> > > > + {},
> > > > +   },
> > > > +};
> > >
> > > This doesn't belong to the driver.
> > >
> >
> > Ack
> > I have moved this code to the MFD driver
> 
> This information should come from the ACPI tables, it should not be present in
> drivers. Why do you need it?
> 

I have removed the GPIO lookup table code for now.

> > > > +   /*
> > > > +* Initialize all the GPIOs to 0, just to make sure all
> > > > +* GPIOs start with known default values. This protects against
> > > > +* any GPIOs getting set with a value of 1, after TPS68470
> > > > + reset
> > >
> > > So, this is hardware bug. Right? Or misconfiguration of the chip we may
> avoid?
> > >
> >
> > The tps68470 PMIC upon reset, does not have the "power on reset" values.
> > We just initialize the GPIOs with their known default values.
> 
> If that was the case, I bet you could expect interesting behaviour from the
> hardware connected to these pins.
> 
> For what it's worth, the chip documentation states that the reset value for 
> the
> SGPO and GPDO registers is zero, as well as that GPIOs are configured as input
> and the reset value of the s_* outputs is low.
> 
> In other words, I don't think that explicitly setting the values to zero has 
> an
> effect.
>

For now, I have removed this code that sets the GPIOs to zeros. If there are 
enough findings / reasons, this code may come back.

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Sakari,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> Hi Rajmohan and Andy,
> 
> On Sun, Jun 11, 2017 at 03:49:18AM +, Mani, Rajmohan wrote:
> > Hi Andy,
> >
> > Thanks for the reviews and patience.
> >
> > >
> > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > 
> > > wrote:
> > > > This patch adds support for TPS68470 GPIOs.
> > > > There are 7 GPIOs and a few sensor related GPIOs.
> > > > These GPIOs can be requested and configured as appropriate.
> > >
> > > Besides my below comments, just put it here that I recommended
> > > earlier to provide 2 GPIO chips (one per bank of GPIOs).
> > > It's up to Linus to decide since you didn't follow the recommendation.
> > >
> >
> > Ack.
> > Did you mean to add this in Kconfig or this source file?
> >
> > Here's some more details on these GPIOs.
> > Each of these 7 GPIOs has 2 registers to control the mode, level, drive
> strength, polarity, hysteresis control among other things. Also there are GPDI
> and GPDO registers to control the input and output values of these 7 GPIOs.
> These GPIOs are numbered 0 through 6.
> > The remaining 3 GPIOs are more of special purpose GPIOs that are output
> only, with one register to control all of their output values and drive 
> strengths.
> These GPIOs are named with a special purpose (ENABLE, IDLE and RESET of the
> sensor).
> >
> > > > +#include 
> > > > +#include 
> > >
> > > These shouldn't be in the driver.
> > > Instead use
> > > #include 
> > >
> >
> > Ack
> >
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > >
> > > > +   if (offset >= TPS68470_N_REGULAR_GPIO) {
> > > > +   offset -= TPS68470_N_REGULAR_GPIO;
> > > > +   reg = TPS68470_REG_SGPO;
> > > > +   }
> > >
> > > Two GPIO chips makes this gone.
> 
> Again, I'm not really worried about this driver, but the ACPI tables. How does
> the difference show there?
> 
> The outputs (s_enable, s_idle and s_resetn) are not numbered in the
> documentation. There grouped, though, but the order in that grouping varies.
> 
> > > > +struct gpiod_lookup_table gpios_table = {
> > > > +   .dev_id = NULL,
> > > > +   .table = {
> > > > + GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> > > GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 8, "s_idle",
> GPIO_ACTIVE_HIGH),
> > > > + GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> > > GPIO_ACTIVE_HIGH),
> > > > + {},
> > > > +   },
> > > > +};
> > >
> > > This doesn't belong to the driver.
> > >
> >
> > Ack
> > I have moved this code to the MFD driver
> 
> This information should come from the ACPI tables, it should not be present in
> drivers. Why do you need it?
> 

I have removed the GPIO lookup table code for now.

> > > > +   /*
> > > > +* Initialize all the GPIOs to 0, just to make sure all
> > > > +* GPIOs start with known default values. This protects against
> > > > +* any GPIOs getting set with a value of 1, after TPS68470
> > > > + reset
> > >
> > > So, this is hardware bug. Right? Or misconfiguration of the chip we may
> avoid?
> > >
> >
> > The tps68470 PMIC upon reset, does not have the "power on reset" values.
> > We just initialize the GPIOs with their known default values.
> 
> If that was the case, I bet you could expect interesting behaviour from the
> hardware connected to these pins.
> 
> For what it's worth, the chip documentation states that the reset value for 
> the
> SGPO and GPDO registers is zero, as well as that GPIOs are configured as input
> and the reset value of the s_* outputs is low.
> 
> In other words, I don't think that explicitly setting the values to zero has 
> an
> effect.
>

For now, I have removed this code that sets the GPIOs to zeros. If there are 
enough findings / reasons, this code may come back.

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee,

> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> 
> > Hi Andy,
> >
> > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > > <rajmohan.m...@intel.com> wrote:
> > > > > The TPS68470 device is an advanced power management unit that
> > > > > powers a Compact Camera Module (CCM), generates clocks for image
> > > > > sensors, drives a dual LED for Flash and incorporates two LED
> > > > > drivers for general purpose indicators.
> > > > >
> > > > > This patch adds support for TPS68470 mfd device.
> > > >
> > > > I dunno why you decide to send this out now, see my comments below.
> > > >
> > > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > > +   unsigned int version;
> > > > > +   int ret;
> > > >
> > > > > +   /* FIXME: configure these dynamically */
> > > >
> > > > So, what prevents you to fix this?
> > >
> > > Nothing I suppose. They're however not needed right now and can be
> > > implemented later on if they're ever needed.
> > >
> >
> > Ack
> 
> What does this mean?  Is the plan to fix it or not?  I don't want FIXMEs in 
> the
> code that a) can be fixed right away or b) might never be fixed.
> 

I meant that this can be implemented later on, if there's a need.
I will look into this and see how this can be fixed.

Thanks
Raj

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Lee,

> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Fri, 09 Jun 2017, Mani, Rajmohan wrote:
> 
> > Hi Andy,
> >
> > > On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
> > >  wrote:
> > > > > The TPS68470 device is an advanced power management unit that
> > > > > powers a Compact Camera Module (CCM), generates clocks for image
> > > > > sensors, drives a dual LED for Flash and incorporates two LED
> > > > > drivers for general purpose indicators.
> > > > >
> > > > > This patch adds support for TPS68470 mfd device.
> > > >
> > > > I dunno why you decide to send this out now, see my comments below.
> > > >
> > > > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > > > +   unsigned int version;
> > > > > +   int ret;
> > > >
> > > > > +   /* FIXME: configure these dynamically */
> > > >
> > > > So, what prevents you to fix this?
> > >
> > > Nothing I suppose. They're however not needed right now and can be
> > > implemented later on if they're ever needed.
> > >
> >
> > Ack
> 
> What does this mean?  Is the plan to fix it or not?  I don't want FIXMEs in 
> the
> code that a) can be fixed right away or b) might never be fixed.
> 

I meant that this can be implemented later on, if there's a need.
I will look into this and see how this can be fixed.

Thanks
Raj

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy, Sakari,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Sun, Jun 11, 2017 at 7:50 PM, Sakari Ailus  wrote:
> > On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
> >> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus  wrote:
> 
> >> > Again, I'm not really worried about this driver, but the ACPI
> >> > tables. How does the difference show there?
> >>
> >> Same way. You will have common numbering over the chip [0, 9]. It
> >> will be just an abstraction inside the driver.
> >
> > Oh, in that case that should be a non-issue.
> 
> >> Above states the opposite, so, it's clear to me that abstraction of 2
> >> GPIO chips over 1 can be utilized here.
> >
> > Sounds fine to me, taken that this does not add complications to ACPI
> > tables.
> 
> They just need to share the same ACPI_HANDLE (it might require to do this in
> generic way in gpiolib) and have a continuous numbering (easy to achieve with
> carefully chosen bases).
> 

Few clarifications...

Are you implying new kernel changes are needed in gpiolib to accommodate 2 GPIO 
chips?
Does it need changes in platform firmware or is it expected to work just with 
the gpiolib changes that you described above?

Thanks
Raj

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy, Sakari,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Sun, Jun 11, 2017 at 7:50 PM, Sakari Ailus  wrote:
> > On Sun, Jun 11, 2017 at 04:40:16PM +0300, Andy Shevchenko wrote:
> >> On Sun, Jun 11, 2017 at 2:30 PM, Sakari Ailus  wrote:
> 
> >> > Again, I'm not really worried about this driver, but the ACPI
> >> > tables. How does the difference show there?
> >>
> >> Same way. You will have common numbering over the chip [0, 9]. It
> >> will be just an abstraction inside the driver.
> >
> > Oh, in that case that should be a non-issue.
> 
> >> Above states the opposite, so, it's clear to me that abstraction of 2
> >> GPIO chips over 1 can be utilized here.
> >
> > Sounds fine to me, taken that this does not add complications to ACPI
> > tables.
> 
> They just need to share the same ACPI_HANDLE (it might require to do this in
> generic way in gpiolib) and have a continuous numbering (easy to achieve with
> carefully chosen bases).
> 

Few clarifications...

Are you implying new kernel changes are needed in gpiolib to accommodate 2 GPIO 
chips?
Does it need changes in platform firmware or is it expected to work just with 
the gpiolib changes that you described above?

Thanks
Raj

RE: [PATCH v2 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy,

> Subject: Re: [PATCH v2 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Sun, Jun 11, 2017 at 9:09 AM, Rajmohan Mani
>  wrote:
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> 
> Main points (some I already told in an answer to Sakari's mail):
> 1. Consider 2 GPIO chips over 1.

I am looking into this.

> 2. Fix FIXME(s).

Leaving this on, until I see how this can be fixed.

> 3. If there is hardware bug we should work around it must be clarified.

Ack
If this is about initializing the GPIOs with zero, I have removed this code for 
now.

> 4. You missed Linus' comments here (switch to the data pointer inside GPIO
> chip and remove platform driver data stuff from the driver).
> 

I have fixed this with v3

RE: [PATCH v2 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy,

> Subject: Re: [PATCH v2 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Sun, Jun 11, 2017 at 9:09 AM, Rajmohan Mani
>  wrote:
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> 
> Main points (some I already told in an answer to Sakari's mail):
> 1. Consider 2 GPIO chips over 1.

I am looking into this.

> 2. Fix FIXME(s).

Leaving this on, until I see how this can be fixed.

> 3. If there is hardware bug we should work around it must be clarified.

Ack
If this is about initializing the GPIOs with zero, I have removed this code for 
now.

> 4. You missed Linus' comments here (switch to the data pointer inside GPIO
> chip and remove platform driver data stuff from the driver).
> 

I have fixed this with v3

RE: [PATCH v2 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews.

> Subject: Re: [PATCH v2 1/3] mfd: Add new mfd device TPS68470
> 
> On Sun, Jun 11, 2017 at 9:09 AM, Rajmohan Mani
>  wrote:
> > The TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> 
> Thanks! This looks much better, though see my few comments below.
> 
> > +/*
> > + * This lookup table for the TPS68470 GPIOs, lists
> > + * the 7 GPIOs (that can be configured as input or output
> > + * as appropriate) and 3 special purpose GPIOs that are
> > + * "output only". Exporting these GPIOs in a system mounted
> > + * with the TPS68470, in conjunction with the gpio-tps68470
> > + * driver, allows the platform firmware to configure these
> > + * GPIOs appropriately, through the ACPI operation region.
> > + * These 7 configurable GPIOs can be connected to power rails,
> > + * sensor control (e.g sensor reset), while the 3 GPIOs can
> > + * be used for sensor control.
> > + */
> 
> > +struct gpiod_lookup_table gpios_table = {
> > +   .dev_id = NULL,
> 
> Why dev_id is NULL?
> 

I have removed the GPIO lookup tables in the driver.

> > +   .table = {
> > + GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> GPIO_ACTIVE_HIGH),
> > + {},
> > +   },
> > +};
> 
> I don't remember if I asked already why this table exists at all in the 
> driver.
> Shouldn't it be provided by ACPI _DSD?
> 

Ack. I have removed the GPIO lookup tables in the driver.

> > +static int tps68470_chip_init(struct device *dev, struct regmap
> > +*regmap) {
> > +   unsigned int version;
> > +   int ret;
> > +
> > +   ret = regmap_read(regmap, TPS68470_REG_REVID, );
> > +   if (ret < 0) {
> > +   dev_err(dev, "Failed to read revision register: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> 
> > +   dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
> 
> This will confuse user when probe fails. Should be printed only when we return
> 0 for sure.
> 

Ack.

> > +   ret = regmap_write(regmap, TPS68470_REG_RESET, 0xff);
> > +   if (ret < 0)
> > +   return ret;
> > +
> 
> > +   /* FIXME: configure these dynamically */
> 
> Please, either fix or remove this comment.
> 

Will keep this comment, until I see how this can be fixed.

> > +   /* Enable daisy chain */
> > +   ret = regmap_update_bits(regmap, TPS68470_REG_S_I2C_CTL, 1, 1);
> > +   if (ret < 0)
> > +   return ret;
> > +
> 
> > +   usleep_range(TPS68470_DAISY_CHAIN_DELAY_US,
> > +   TPS68470_DAISY_CHAIN_DELAY_US + 10);
> 
> This might require a comment, though I'm fine with it as long as it close to
> previous excerpt.
> 

Ack. Added comment.

> > +   return 0;
> > +}
> 
> > +static int tps68470_probe(struct i2c_client *client) {
> > +   struct device *dev = >dev;
> > +   struct regmap *regmap;
> > +   int ret;
> > +
> > +   regmap = devm_regmap_init_i2c(client, _regmap_config);
> > +   if (IS_ERR(regmap)) {
> 
> > +   dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> > +   PTR_ERR(regmap));
> 
> 1. Indentation.

Ack

> 2. Do we really need this message?
> 

Since the driver probe fails, it would be useful to know more info on that.

> > +   return PTR_ERR(regmap);
> > +   }
> > +
> > +   i2c_set_clientdata(client, regmap);
> > +
> > +   gpiod_add_lookup_table(_table);
> > +
> 
> > +   ret = devm_mfd_add_devices(dev, -1, tps68470s,
> > + ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> 
> -1 has a definition for such case, use it instead.
> 

Ack

> > +   if (ret < 0) {
> > +   dev_err(dev, "mfd_add_devices failed: %d\n", ret);
> > +   return ret;
> > +   }
> 
> > +static const struct i2c_device_id tps68470_id_table[] = {
> > +   {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, tps68470_id_table);
> 
> Either choose ->probe() over 

RE: [PATCH v2 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews.

> Subject: Re: [PATCH v2 1/3] mfd: Add new mfd device TPS68470
> 
> On Sun, Jun 11, 2017 at 9:09 AM, Rajmohan Mani
>  wrote:
> > The TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> 
> Thanks! This looks much better, though see my few comments below.
> 
> > +/*
> > + * This lookup table for the TPS68470 GPIOs, lists
> > + * the 7 GPIOs (that can be configured as input or output
> > + * as appropriate) and 3 special purpose GPIOs that are
> > + * "output only". Exporting these GPIOs in a system mounted
> > + * with the TPS68470, in conjunction with the gpio-tps68470
> > + * driver, allows the platform firmware to configure these
> > + * GPIOs appropriately, through the ACPI operation region.
> > + * These 7 configurable GPIOs can be connected to power rails,
> > + * sensor control (e.g sensor reset), while the 3 GPIOs can
> > + * be used for sensor control.
> > + */
> 
> > +struct gpiod_lookup_table gpios_table = {
> > +   .dev_id = NULL,
> 
> Why dev_id is NULL?
> 

I have removed the GPIO lookup tables in the driver.

> > +   .table = {
> > + GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> GPIO_ACTIVE_HIGH),
> > + {},
> > +   },
> > +};
> 
> I don't remember if I asked already why this table exists at all in the 
> driver.
> Shouldn't it be provided by ACPI _DSD?
> 

Ack. I have removed the GPIO lookup tables in the driver.

> > +static int tps68470_chip_init(struct device *dev, struct regmap
> > +*regmap) {
> > +   unsigned int version;
> > +   int ret;
> > +
> > +   ret = regmap_read(regmap, TPS68470_REG_REVID, );
> > +   if (ret < 0) {
> > +   dev_err(dev, "Failed to read revision register: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> 
> > +   dev_info(dev, "TPS68470 REVID: 0x%x\n", version);
> 
> This will confuse user when probe fails. Should be printed only when we return
> 0 for sure.
> 

Ack.

> > +   ret = regmap_write(regmap, TPS68470_REG_RESET, 0xff);
> > +   if (ret < 0)
> > +   return ret;
> > +
> 
> > +   /* FIXME: configure these dynamically */
> 
> Please, either fix or remove this comment.
> 

Will keep this comment, until I see how this can be fixed.

> > +   /* Enable daisy chain */
> > +   ret = regmap_update_bits(regmap, TPS68470_REG_S_I2C_CTL, 1, 1);
> > +   if (ret < 0)
> > +   return ret;
> > +
> 
> > +   usleep_range(TPS68470_DAISY_CHAIN_DELAY_US,
> > +   TPS68470_DAISY_CHAIN_DELAY_US + 10);
> 
> This might require a comment, though I'm fine with it as long as it close to
> previous excerpt.
> 

Ack. Added comment.

> > +   return 0;
> > +}
> 
> > +static int tps68470_probe(struct i2c_client *client) {
> > +   struct device *dev = >dev;
> > +   struct regmap *regmap;
> > +   int ret;
> > +
> > +   regmap = devm_regmap_init_i2c(client, _regmap_config);
> > +   if (IS_ERR(regmap)) {
> 
> > +   dev_err(dev, "devm_regmap_init_i2c Error %ld\n",
> > +   PTR_ERR(regmap));
> 
> 1. Indentation.

Ack

> 2. Do we really need this message?
> 

Since the driver probe fails, it would be useful to know more info on that.

> > +   return PTR_ERR(regmap);
> > +   }
> > +
> > +   i2c_set_clientdata(client, regmap);
> > +
> > +   gpiod_add_lookup_table(_table);
> > +
> 
> > +   ret = devm_mfd_add_devices(dev, -1, tps68470s,
> > + ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> 
> -1 has a definition for such case, use it instead.
> 

Ack

> > +   if (ret < 0) {
> > +   dev_err(dev, "mfd_add_devices failed: %d\n", ret);
> > +   return ret;
> > +   }
> 
> > +static const struct i2c_device_id tps68470_id_table[] = {
> > +   {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, tps68470_id_table);
> 
> Either choose ->probe() over ->probe_new() or remove above.

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

> > +static inline struct tps68470_gpio_data *to_gpio_data(struct
> > +gpio_chip *gpiochp) {
> > +   return container_of(gpiochp, struct tps68470_gpio_data, gc); }
> 
> Please use the data pointe inside gpio_chip.
> 

My bad. I completely missed the following line. Will fix it v3.

> struct tps68470_gpio_data *foo = gpiochip_get_data(gc);
> 

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

> > +static inline struct tps68470_gpio_data *to_gpio_data(struct
> > +gpio_chip *gpiochp) {
> > +   return container_of(gpiochp, struct tps68470_gpio_data, gc); }
> 
> Please use the data pointe inside gpio_chip.
> 

My bad. I completely missed the following line. Will fix it v3.

> struct tps68470_gpio_data *foo = gpiochip_get_data(gc);
> 

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

Thanks for the reviews.

> -Original Message-
> From: Linus Walleij [mailto:linus.wall...@linaro.org]
> Sent: Friday, June 09, 2017 4:15 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; ACPI Devel
> Maling List <linux-a...@vger.kernel.org>; Lee Jones <lee.jo...@linaro.org>;
> Alexandre Courbot <gnu...@gmail.com>; Rafael J. Wysocki
> <r...@rjwysocki.net>; Len Brown <l...@kernel.org>
> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Tue, Jun 6, 2017 at 1:55 PM, Rajmohan Mani <rajmohan.m...@intel.com>
> wrote:
> 
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> 
> Same comments as Andy already sent, plus:
> 
> > +static inline struct tps68470_gpio_data *to_gpio_data(struct
> > +gpio_chip *gpiochp) {
> > +   return container_of(gpiochp, struct tps68470_gpio_data, gc); }
> 
> Please use the data pointe inside gpio_chip.
> 

This is not clear to me.
The driver already gets the data pointer inside gpio_chip (which is gpiochp)
Is the name gpiochp misleading? 
I had to get rid of "i" in gpiochip, to meet 80 char limit.

> struct tps68470_gpio_data *foo = gpiochip_get_data(gc);
> 
> > +   ret = tps68470_reg_read(tps, reg, );
> (...)
> > +   tps68470_update_bits(tps, reg, BIT(offset), value ?
> > + BIT(offset) : 0);
> (...)
> > +   return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> > +TPS68470_GPIO_MODE_MASK,
> > +TPS68470_GPIO_MODE_OUT_CMOS);
> (...)
> > +   return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> > +  TPS68470_GPIO_MODE_MASK, 0x00);
> 
> This looks like a reimplementation of regmap. Is it not possible to create a
> regmap in the MFD driver and pass that around instead?
> 

Ack
Will be addressed in v2 of this patch series.

> > +struct gpiod_lookup_table gpios_table = {
> > +   .dev_id = NULL,
> > +   .table = {
> > + GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> GPIO_ACTIVE_HIGH),
> > + {},
> > +   },
> > +};
> 
> Hm that's why you include  I guess.
> 

Ack

> This warrants a big comment above it explaining why this is done like that and
> what the use is inside any system with this chip mounted.
> 

Ack
I have added comments in the MFD driver, inside which the lookup table has 
moved.

> > +   tps68470_gpio->gc.label = "tps68470-gpio";
> > +   tps68470_gpio->gc.owner = THIS_MODULE;
> > +   tps68470_gpio->gc.direction_input = tps68470_gpio_input;
> > +   tps68470_gpio->gc.direction_output = tps68470_gpio_output;
> 
> Please implement .get_direction()
> 

Ack

> > +   tps68470_gpio->gc.get = tps68470_gpio_get;
> > +   tps68470_gpio->gc.set = tps68470_gpio_set;
> > +   tps68470_gpio->gc.can_sleep = true;
> > +   tps68470_gpio->gc.ngpio = TPS68470_N_GPIO;
> > +   tps68470_gpio->gc.base = -1;
> > +   tps68470_gpio->gc.parent = >dev;
> 
> Consider assigning the .names() array some sensible line names.
> 

Ack

> > +static int tps68470_gpio_remove(struct platform_device *pdev) {
> > +   struct tps68470_gpio_data *tps68470_gpio =
> > +platform_get_drvdata(pdev);
> > +
> > +   gpiod_remove_lookup_table(_table);
> > +   gpiochip_remove(_gpio->gc);
> 
> You can't use devm_gpiochip_add()?
> 

Originally I couldn't, because the driver can not remove the lookup table upon 
exit/removal. I moved this code inside the MFD driver, which is modified to use 
.shutdown() to remove the lookup table, so it can use dev_mfd_* call.

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Linus,

Thanks for the reviews.

> -Original Message-
> From: Linus Walleij [mailto:linus.wall...@linaro.org]
> Sent: Friday, June 09, 2017 4:15 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; ACPI Devel
> Maling List ; Lee Jones ;
> Alexandre Courbot ; Rafael J. Wysocki
> ; Len Brown 
> Subject: Re: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs
> 
> On Tue, Jun 6, 2017 at 1:55 PM, Rajmohan Mani 
> wrote:
> 
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> >
> > Signed-off-by: Rajmohan Mani 
> 
> Same comments as Andy already sent, plus:
> 
> > +static inline struct tps68470_gpio_data *to_gpio_data(struct
> > +gpio_chip *gpiochp) {
> > +   return container_of(gpiochp, struct tps68470_gpio_data, gc); }
> 
> Please use the data pointe inside gpio_chip.
> 

This is not clear to me.
The driver already gets the data pointer inside gpio_chip (which is gpiochp)
Is the name gpiochp misleading? 
I had to get rid of "i" in gpiochip, to meet 80 char limit.

> struct tps68470_gpio_data *foo = gpiochip_get_data(gc);
> 
> > +   ret = tps68470_reg_read(tps, reg, );
> (...)
> > +   tps68470_update_bits(tps, reg, BIT(offset), value ?
> > + BIT(offset) : 0);
> (...)
> > +   return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> > +TPS68470_GPIO_MODE_MASK,
> > +TPS68470_GPIO_MODE_OUT_CMOS);
> (...)
> > +   return tps68470_update_bits(tps, TPS68470_GPIO_CTL_REG_A(offset),
> > +  TPS68470_GPIO_MODE_MASK, 0x00);
> 
> This looks like a reimplementation of regmap. Is it not possible to create a
> regmap in the MFD driver and pass that around instead?
> 

Ack
Will be addressed in v2 of this patch series.

> > +struct gpiod_lookup_table gpios_table = {
> > +   .dev_id = NULL,
> > +   .table = {
> > + GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> GPIO_ACTIVE_HIGH),
> > + {},
> > +   },
> > +};
> 
> Hm that's why you include  I guess.
> 

Ack

> This warrants a big comment above it explaining why this is done like that and
> what the use is inside any system with this chip mounted.
> 

Ack
I have added comments in the MFD driver, inside which the lookup table has 
moved.

> > +   tps68470_gpio->gc.label = "tps68470-gpio";
> > +   tps68470_gpio->gc.owner = THIS_MODULE;
> > +   tps68470_gpio->gc.direction_input = tps68470_gpio_input;
> > +   tps68470_gpio->gc.direction_output = tps68470_gpio_output;
> 
> Please implement .get_direction()
> 

Ack

> > +   tps68470_gpio->gc.get = tps68470_gpio_get;
> > +   tps68470_gpio->gc.set = tps68470_gpio_set;
> > +   tps68470_gpio->gc.can_sleep = true;
> > +   tps68470_gpio->gc.ngpio = TPS68470_N_GPIO;
> > +   tps68470_gpio->gc.base = -1;
> > +   tps68470_gpio->gc.parent = >dev;
> 
> Consider assigning the .names() array some sensible line names.
> 

Ack

> > +static int tps68470_gpio_remove(struct platform_device *pdev) {
> > +   struct tps68470_gpio_data *tps68470_gpio =
> > +platform_get_drvdata(pdev);
> > +
> > +   gpiod_remove_lookup_table(_table);
> > +   gpiochip_remove(_gpio->gc);
> 
> You can't use devm_gpiochip_add()?
> 

Originally I couldn't, because the driver can not remove the lookup table upon 
exit/removal. I moved this code inside the MFD driver, which is modified to use 
.shutdown() to remove the lookup table, so it can use dev_mfd_* call.

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> wrote:
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> 
> Besides my below comments, just put it here that I recommended earlier to
> provide 2 GPIO chips (one per bank of GPIOs).
> It's up to Linus to decide since you didn't follow the recommendation.
> 

Ack.
Did you mean to add this in Kconfig or this source file?

Here's some more details on these GPIOs.
Each of these 7 GPIOs has 2 registers to control the mode, level, drive 
strength, polarity, hysteresis control among other things. Also there are GPDI 
and GPDO registers to control the input and output values of these 7 GPIOs. 
These GPIOs are numbered 0 through 6.
The remaining 3 GPIOs are more of special purpose GPIOs that are output only, 
with one register to control all of their output values and drive strengths. 
These GPIOs are named with a special purpose (ENABLE, IDLE and RESET of the 
sensor).

> > +#include 
> > +#include 
> 
> These shouldn't be in the driver.
> Instead use
> #include 
> 

Ack

> > +#include 
> > +#include 
> > +#include 
> 
> > +   if (offset >= TPS68470_N_REGULAR_GPIO) {
> > +   offset -= TPS68470_N_REGULAR_GPIO;
> > +   reg = TPS68470_REG_SGPO;
> > +   }
> 
> Two GPIO chips makes this gone.
> 

Ack.
On a closer look, creating two GPIO chips makes things clearer. 
But, this comes at the cost of a new set of gpio_get/set, 
gpio_output/input functions for the new GPIO chip. This results in 
new code for the second GPIO chip, which is pretty much 
going to be the copy of first GPIO chip, except for initializing 
the "reg" variable with GPDO or SGPO register. If we decide to 
reuse the existing code of the first GPIO chip for the new/second 
GPIO chip, we would still need to add a check, which would be 
effectively the same as the existing code, with the only advantage 
of not having to initialize the "offset" variable (which holds the 
GPIO offset). Given the above, it seems ok to retain the existing 
model of a single chip with the adjustments for offset, reg 
variables per the GPIO offset, to keep the whole picture simple.

> > +struct gpiod_lookup_table gpios_table = {
> > +   .dev_id = NULL,
> > +   .table = {
> > + GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> GPIO_ACTIVE_HIGH),
> > + {},
> > +   },
> > +};
> 
> This doesn't belong to the driver.
> 

Ack
I have moved this code to the MFD driver

> > +static int tps68470_gpio_probe(struct platform_device *pdev) {
> > +   struct tps68470 *tps68470 = dev_get_drvdata(pdev->dev.parent);
> > +   struct tps68470_gpio_data *tps68470_gpio;
> 
> > +   int i, ret;
> 
> unsingned int i;
> 
> > +   ret = gpiochip_add(_gpio->gc);
> 
> devm_ ?
> 

Ack

> > +   gpiod_add_lookup_table(_table);
> 
> Doesn't belong to the driver either.
> I suppose it's a part of MFD (patch 1)
> 

Ack

> > +   /*
> > +* Initialize all the GPIOs to 0, just to make sure all
> > +* GPIOs start with known default values. This protects against
> > +* any GPIOs getting set with a value of 1, after TPS68470
> > + reset
> 
> So, this is hardware bug. Right? Or misconfiguration of the chip we may avoid?
> 

The tps68470 PMIC upon reset, does not have the "power on reset" values.
We just initialize the GPIOs with their known default values.

> > +*/
> > +   for (i = 0; i < tps68470_gpio->gc.ngpio; i++)
> > +   tps68470_gpio_set(_gpio->gc, i, 0);
> > +
> > +   return ret;
> > +}
> 
> > +
> > +static int tps68470_gpio_remove(struct platform_device *pdev) {
> > +   struct tps68470_gpio_data *tps68470_gpio =
> > +platform_get_drvdata(pdev);
> > +
> > +   gpiod_remove_lookup_table(_table);
> > +   gpiochip_remove(_gpio->gc);
> > +
> > +   return 0;
> > +}
> 
> Should gone after devm_ in use.
> 

Ack

RE: [PATCH v1 2/3] gpio: Add support for TPS68470 GPIOs

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> wrote:
> > This patch adds support for TPS68470 GPIOs.
> > There are 7 GPIOs and a few sensor related GPIOs.
> > These GPIOs can be requested and configured as appropriate.
> 
> Besides my below comments, just put it here that I recommended earlier to
> provide 2 GPIO chips (one per bank of GPIOs).
> It's up to Linus to decide since you didn't follow the recommendation.
> 

Ack.
Did you mean to add this in Kconfig or this source file?

Here's some more details on these GPIOs.
Each of these 7 GPIOs has 2 registers to control the mode, level, drive 
strength, polarity, hysteresis control among other things. Also there are GPDI 
and GPDO registers to control the input and output values of these 7 GPIOs. 
These GPIOs are numbered 0 through 6.
The remaining 3 GPIOs are more of special purpose GPIOs that are output only, 
with one register to control all of their output values and drive strengths. 
These GPIOs are named with a special purpose (ENABLE, IDLE and RESET of the 
sensor).

> > +#include 
> > +#include 
> 
> These shouldn't be in the driver.
> Instead use
> #include 
> 

Ack

> > +#include 
> > +#include 
> > +#include 
> 
> > +   if (offset >= TPS68470_N_REGULAR_GPIO) {
> > +   offset -= TPS68470_N_REGULAR_GPIO;
> > +   reg = TPS68470_REG_SGPO;
> > +   }
> 
> Two GPIO chips makes this gone.
> 

Ack.
On a closer look, creating two GPIO chips makes things clearer. 
But, this comes at the cost of a new set of gpio_get/set, 
gpio_output/input functions for the new GPIO chip. This results in 
new code for the second GPIO chip, which is pretty much 
going to be the copy of first GPIO chip, except for initializing 
the "reg" variable with GPDO or SGPO register. If we decide to 
reuse the existing code of the first GPIO chip for the new/second 
GPIO chip, we would still need to add a check, which would be 
effectively the same as the existing code, with the only advantage 
of not having to initialize the "offset" variable (which holds the 
GPIO offset). Given the above, it seems ok to retain the existing 
model of a single chip with the adjustments for offset, reg 
variables per the GPIO offset, to keep the whole picture simple.

> > +struct gpiod_lookup_table gpios_table = {
> > +   .dev_id = NULL,
> > +   .table = {
> > + GPIO_LOOKUP("tps68470-gpio", 0, "gpio.0", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 1, "gpio.1", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 2, "gpio.2", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 3, "gpio.3", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 4, "gpio.4", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 5, "gpio.5", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 6, "gpio.6", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 7, "s_enable",
> GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", 
> > GPIO_ACTIVE_HIGH),
> > + GPIO_LOOKUP("tps68470-gpio", 9, "s_resetn",
> GPIO_ACTIVE_HIGH),
> > + {},
> > +   },
> > +};
> 
> This doesn't belong to the driver.
> 

Ack
I have moved this code to the MFD driver

> > +static int tps68470_gpio_probe(struct platform_device *pdev) {
> > +   struct tps68470 *tps68470 = dev_get_drvdata(pdev->dev.parent);
> > +   struct tps68470_gpio_data *tps68470_gpio;
> 
> > +   int i, ret;
> 
> unsingned int i;
> 
> > +   ret = gpiochip_add(_gpio->gc);
> 
> devm_ ?
> 

Ack

> > +   gpiod_add_lookup_table(_table);
> 
> Doesn't belong to the driver either.
> I suppose it's a part of MFD (patch 1)
> 

Ack

> > +   /*
> > +* Initialize all the GPIOs to 0, just to make sure all
> > +* GPIOs start with known default values. This protects against
> > +* any GPIOs getting set with a value of 1, after TPS68470
> > + reset
> 
> So, this is hardware bug. Right? Or misconfiguration of the chip we may avoid?
> 

The tps68470 PMIC upon reset, does not have the "power on reset" values.
We just initialize the GPIOs with their known default values.

> > +*/
> > +   for (i = 0; i < tps68470_gpio->gc.ngpio; i++)
> > +   tps68470_gpio_set(_gpio->gc, i, 0);
> > +
> > +   return ret;
> > +}
> 
> > +
> > +static int tps68470_gpio_remove(struct platform_device *pdev) {
> > +   struct tps68470_gpio_data *tps68470_gpio =
> > +platform_get_drvdata(pdev);
> > +
> > +   gpiod_remove_lookup_table(_table);
> > +   gpiochip_remove(_gpio->gc);
> > +
> > +   return 0;
> > +}
> 
> Should gone after devm_ in use.
> 

Ack

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Sakari, Andy,

> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Wed, Jun 07, 2017 at 11:40:27PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 7, 2017 at 11:10 PM, Sakari Ailus  wrote:
> > > On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote:
> > >> On Wed, Jun 7, 2017 at 3:15 PM, Sakari Ailus  wrote:
> > >> > On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote:
> >
> > >> >> Follow the pattern, please, I suppose ti_pmic_tps68470.c
> > >>
> > >> Would it work for you?
> > >
> > > This is still a different driver from the tps68470 driver which is
> > > an MFD driver. For clarity, I'd keep pmic as part of the name (and
> > > I'd use tps68470_pmic_ prefix for internal symbols, too).
> > >
> > > As PMICs are typically linked to the kernel (vs. being modules),
> > > there's no issue with the module name. I would suppose few if any
> > > PMICs will be compiled as modules in general.
> > >
> > > It's not a big deal though. I'm fine either way.
> >
> > Okay, let's agree on  tps68470_pmic for internal prefix and for file name?
> 
> Ack. Thanks!
> 

Ack

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Sakari, Andy,

> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Wed, Jun 07, 2017 at 11:40:27PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 7, 2017 at 11:10 PM, Sakari Ailus  wrote:
> > > On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote:
> > >> On Wed, Jun 7, 2017 at 3:15 PM, Sakari Ailus  wrote:
> > >> > On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote:
> >
> > >> >> Follow the pattern, please, I suppose ti_pmic_tps68470.c
> > >>
> > >> Would it work for you?
> > >
> > > This is still a different driver from the tps68470 driver which is
> > > an MFD driver. For clarity, I'd keep pmic as part of the name (and
> > > I'd use tps68470_pmic_ prefix for internal symbols, too).
> > >
> > > As PMICs are typically linked to the kernel (vs. being modules),
> > > there's no issue with the module name. I would suppose few if any
> > > PMICs will be compiled as modules in general.
> > >
> > > It's not a big deal though. I'm fine either way.
> >
> > Okay, let's agree on  tps68470_pmic for internal prefix and for file name?
> 
> Ack. Thanks!
> 

Ack

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Wed, Jun 7, 2017 at 3:07 PM, Sakari Ailus  wrote:
> 
> >> +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
> >> +int bitmask, u64 *value) {
> >> + int data;
> >
> > Shouldn't you use unsigned int here? Same in the functions below.
> 
> +1, regmap_read() returns unsigned int.
> 

Ack

> >> +static acpi_status ti_pmic_common_handler(u32 function,
> > +   acpi_physical_address address,
> > +   u32 bits, u64 *value,
> > +   void *handler_context,
> 
> > handler_context is unused.
> 

Ack

> >> +  int, int, u64 *),
> >> +   int (*update)(struct regmap *,
> >> + int, int, u64),
> >> +   struct ti_pmic_table *table,
> >> +   int table_size)
> 
> I would even split this to have separate update() and get() paths instead of
> having such a monster of parameters.
> 

I have responded on top of Sakari's response.

> >> +static acpi_status ti_pmic_clk_freq_handler(u32 function,
> >> + acpi_physical_address address,
> >> + u32 bits, u64 *value,
> >> + void *handler_context,
> >> + void *region_context) {
> >> + return ti_pmic_common_handler(function, address, bits, value,
> >> + handler_context, region_context,
> >> + ti_tps68470_pmic_get_clk_freq,
> >> + ti_tps68470_regmap_update_bits,
> >> + (struct ti_pmic_table *)
> >> +_freq_table,
> >
> > You shouldn't use an explicit cast here. Instead make the function
> > argument const as well and you're fine.
> 
> +1.
> 

Ack

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Wed, Jun 7, 2017 at 3:07 PM, Sakari Ailus  wrote:
> 
> >> +static int ti_tps68470_pmic_get_power(struct regmap *regmap, int reg,
> >> +int bitmask, u64 *value) {
> >> + int data;
> >
> > Shouldn't you use unsigned int here? Same in the functions below.
> 
> +1, regmap_read() returns unsigned int.
> 

Ack

> >> +static acpi_status ti_pmic_common_handler(u32 function,
> > +   acpi_physical_address address,
> > +   u32 bits, u64 *value,
> > +   void *handler_context,
> 
> > handler_context is unused.
> 

Ack

> >> +  int, int, u64 *),
> >> +   int (*update)(struct regmap *,
> >> + int, int, u64),
> >> +   struct ti_pmic_table *table,
> >> +   int table_size)
> 
> I would even split this to have separate update() and get() paths instead of
> having such a monster of parameters.
> 

I have responded on top of Sakari's response.

> >> +static acpi_status ti_pmic_clk_freq_handler(u32 function,
> >> + acpi_physical_address address,
> >> + u32 bits, u64 *value,
> >> + void *handler_context,
> >> + void *region_context) {
> >> + return ti_pmic_common_handler(function, address, bits, value,
> >> + handler_context, region_context,
> >> + ti_tps68470_pmic_get_clk_freq,
> >> + ti_tps68470_regmap_update_bits,
> >> + (struct ti_pmic_table *)
> >> +_freq_table,
> >
> > You shouldn't use an explicit cast here. Instead make the function
> > argument const as well and you're fine.
> 
> +1.
> 

Ack

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Sakari, Andy,

> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Wed, Jun 07, 2017 at 04:37:12PM +0300, Andy Shevchenko wrote:
> > >> +static acpi_status ti_pmic_common_handler(u32 function,
> > > +   acpi_physical_address address,
> > > +   u32 bits, u64 *value,
> > > +   void *handler_context,
> >
> > > handler_context is unused.
> >
> > >> +  int, int, u64 *),
> > >> +   int (*update)(struct regmap *,
> > >> + int, int, u64),
> > >> +   struct ti_pmic_table *table,
> > >> +   int table_size)
> >
> > I would even split this to have separate update() and get() paths
> > instead of having such a monster of parameters.
> 
> I'm not really worried about the two callbacks --- you have the compexity,
> which is agruably rather manageable, split into a number of caller functions. 
> I'd
> rather keep it as-is.
> 

Ack

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Sakari, Andy,

> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Wed, Jun 07, 2017 at 04:37:12PM +0300, Andy Shevchenko wrote:
> > >> +static acpi_status ti_pmic_common_handler(u32 function,
> > > +   acpi_physical_address address,
> > > +   u32 bits, u64 *value,
> > > +   void *handler_context,
> >
> > > handler_context is unused.
> >
> > >> +  int, int, u64 *),
> > >> +   int (*update)(struct regmap *,
> > >> + int, int, u64),
> > >> +   struct ti_pmic_table *table,
> > >> +   int table_size)
> >
> > I would even split this to have separate update() and get() paths
> > instead of having such a monster of parameters.
> 
> I'm not really worried about the two callbacks --- you have the compexity,
> which is agruably rather manageable, split into a number of caller functions. 
> I'd
> rather keep it as-is.
> 

Ack

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Sakari,

Thanks for the reviews.

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Wednesday, June 07, 2017 5:08 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones <lee.jo...@linaro.org>; Linus Walleij
> <linus.wall...@linaro.org>; Alexandre Courbot <gnu...@gmail.com>; Rafael J.
> Wysocki <r...@rjwysocki.net>; Len Brown <l...@kernel.org>
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> Hi Rajmohan,
> 
> Thanks for removing the redundant struct definition. A couple more comments
> below. Not really necessarily bugs but a few things to clean things up a bit.
> 

Ack

> On Tue, Jun 06, 2017 at 04:55:18AM -0700, Rajmohan Mani wrote:
> > The Kabylake platform coreboot (Chrome OS equivalent of
> > BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
> > These operation regions are to enable/disable voltage regulators,
> > configure voltage regulators, enable/disable clocks and to configure
> > clocks.
> >
> > This config adds ACPI operation region support for TI TPS68470 PMIC.
> > TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for flash and incorporates two LED drivers for
> > general purpose indicators.
> > This driver enables ACPI operation region support to control voltage
> > regulators and clocks for the TPS68470 PMIC.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > ---
> >  drivers/acpi/Kconfig  |  12 +
> >  drivers/acpi/Makefile |   2 +
> >  drivers/acpi/pmic/pmic_tps68470.c | 454
> > ++
> >  3 files changed, 468 insertions(+)
> >  create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
> >
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index
> > 1ce52f8..218d22d 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -535,4 +535,16 @@ if ARM64
> >  source "drivers/acpi/arm64/Kconfig"
> >  endif
> >
> > +config TPS68470_PMIC_OPREGION
> > +   bool "ACPI operation region support for TPS68470 PMIC"
> > +   help
> > + This config adds ACPI operation region support for TI TPS68470 PMIC.
> > + TPS68470 device is an advanced power management unit that powers
> > + a Compact Camera Module (CCM), generates clocks for image sensors,
> > + drives a dual LED for flash and incorporates two LED drivers for
> > + general purpose indicators.
> > + This driver enables ACPI operation region support control voltage
> > + regulators and clocks.
> > +
> 
> Extra newline.
> 

Ack

> > +
> >  endif  # ACPI
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index
> > b1aacfc..7113d05 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) +=
> > pmic/intel_pmic_chtwc.o
> >
> >  obj-$(CONFIG_ACPI_CONFIGFS)+= acpi_configfs.o
> >
> > +obj-$(CONFIG_TPS68470_PMIC_OPREGION)   += pmic/pmic_tps68470.o
> > +
> >  video-objs += acpi_video.o video_detect.o
> >  obj-y  += dptf/
> >
> > diff --git a/drivers/acpi/pmic/pmic_tps68470.c
> > b/drivers/acpi/pmic/pmic_tps68470.c
> > new file mode 100644
> > index 000..b2d608b
> > --- /dev/null
> > +++ b/drivers/acpi/pmic/pmic_tps68470.c
> > @@ -0,0 +1,454 @@
> > +/*
> > + * TI TPS68470 PMIC operation region driver
> > + *
> > + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> > + * Author: Rajmohan Mani <rajmohan.m...@intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > +version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * 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.
> > + *
> > + * Based on drivers/acpi/pmic/intel_pmic* drivers
> > + *
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Sakari,

Thanks for the reviews.

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Wednesday, June 07, 2017 5:08 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> Hi Rajmohan,
> 
> Thanks for removing the redundant struct definition. A couple more comments
> below. Not really necessarily bugs but a few things to clean things up a bit.
> 

Ack

> On Tue, Jun 06, 2017 at 04:55:18AM -0700, Rajmohan Mani wrote:
> > The Kabylake platform coreboot (Chrome OS equivalent of
> > BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
> > These operation regions are to enable/disable voltage regulators,
> > configure voltage regulators, enable/disable clocks and to configure
> > clocks.
> >
> > This config adds ACPI operation region support for TI TPS68470 PMIC.
> > TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for flash and incorporates two LED drivers for
> > general purpose indicators.
> > This driver enables ACPI operation region support to control voltage
> > regulators and clocks for the TPS68470 PMIC.
> >
> > Signed-off-by: Rajmohan Mani 
> > ---
> >  drivers/acpi/Kconfig  |  12 +
> >  drivers/acpi/Makefile |   2 +
> >  drivers/acpi/pmic/pmic_tps68470.c | 454
> > ++
> >  3 files changed, 468 insertions(+)
> >  create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
> >
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index
> > 1ce52f8..218d22d 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -535,4 +535,16 @@ if ARM64
> >  source "drivers/acpi/arm64/Kconfig"
> >  endif
> >
> > +config TPS68470_PMIC_OPREGION
> > +   bool "ACPI operation region support for TPS68470 PMIC"
> > +   help
> > + This config adds ACPI operation region support for TI TPS68470 PMIC.
> > + TPS68470 device is an advanced power management unit that powers
> > + a Compact Camera Module (CCM), generates clocks for image sensors,
> > + drives a dual LED for flash and incorporates two LED drivers for
> > + general purpose indicators.
> > + This driver enables ACPI operation region support control voltage
> > + regulators and clocks.
> > +
> 
> Extra newline.
> 

Ack

> > +
> >  endif  # ACPI
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index
> > b1aacfc..7113d05 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) +=
> > pmic/intel_pmic_chtwc.o
> >
> >  obj-$(CONFIG_ACPI_CONFIGFS)+= acpi_configfs.o
> >
> > +obj-$(CONFIG_TPS68470_PMIC_OPREGION)   += pmic/pmic_tps68470.o
> > +
> >  video-objs += acpi_video.o video_detect.o
> >  obj-y  += dptf/
> >
> > diff --git a/drivers/acpi/pmic/pmic_tps68470.c
> > b/drivers/acpi/pmic/pmic_tps68470.c
> > new file mode 100644
> > index 000..b2d608b
> > --- /dev/null
> > +++ b/drivers/acpi/pmic/pmic_tps68470.c
> > @@ -0,0 +1,454 @@
> > +/*
> > + * TI TPS68470 PMIC operation region driver
> > + *
> > + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> > + * Author: Rajmohan Mani 
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > +version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * 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.
> > + *
> > + * Based on drivers/acpi/pmic/intel_pmic* drivers
> > + *
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +struct ti_pmic_table {
> > +   u32 address;/* operation region address */
> > +   u32 reg;/* corresponding register */
>

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Hans,

> >
> > As PMICs are typically linked to the kernel (vs. being modules),
> > there's no issue with the module name. I would suppose few if any
> > PMICs will be compiled as modules in general.
> 
> Good point about the OpRegion driver usually being built-in, in my experience 
> it
> MUST always be built-in, so the Kconfig option should be a bool. Note this is
> useless unless the mfd driver is also a bool (I would advice to go that
> route) and the mfd driver's Kconfig should select the right i2c bus driver to
> make sure that is built-in too, see for example:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-
> mfd-next=2f91ded5f8f4fdd67d8daae514b0d434c98ab1e0
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-
> mfd-next=c5065d8625ebdc164199b99d838ac0636faa7f0b
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-
> mfd-next=5f125f1f570568a29edf783fba1ebb606d5c6b24
> 
> Which are all recent commits from me dealing with making the mfd driver built-
> in / selecting the i2c bus driver.
> 

Thanks for these links.
I will update the Kconfig and commit messages with relevant description around 
this.

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Hans,

> >
> > As PMICs are typically linked to the kernel (vs. being modules),
> > there's no issue with the module name. I would suppose few if any
> > PMICs will be compiled as modules in general.
> 
> Good point about the OpRegion driver usually being built-in, in my experience 
> it
> MUST always be built-in, so the Kconfig option should be a bool. Note this is
> useless unless the mfd driver is also a bool (I would advice to go that
> route) and the mfd driver's Kconfig should select the right i2c bus driver to
> make sure that is built-in too, see for example:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-
> mfd-next=2f91ded5f8f4fdd67d8daae514b0d434c98ab1e0
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-
> mfd-next=c5065d8625ebdc164199b99d838ac0636faa7f0b
> https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git/commit/?h=for-
> mfd-next=5f125f1f570568a29edf783fba1ebb606d5c6b24
> 
> Which are all recent commits from me dealing with making the mfd driver built-
> in / selecting the i2c bus driver.
> 

Thanks for these links.
I will update the Kconfig and commit messages with relevant description around 
this.

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Sakari, Andy,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Wednesday, June 07, 2017 2:13 PM
> To: Andy Shevchenko <andy.shevche...@gmail.com>
> Cc: Mani, Rajmohan <rajmohan.m...@intel.com>; Hans de Goede
> <hdego...@redhat.com>; linux-kernel@vger.kernel.org; linux-
> g...@vger.kernel.org; linux-a...@vger.kernel.org; Lee Jones
> <lee.jo...@linaro.org>; Linus Walleij <linus.wall...@linaro.org>; Alexandre
> Courbot <gnu...@gmail.com>; Rafael J. Wysocki <r...@rjwysocki.net>; Len
> Brown <l...@kernel.org>
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Wed, Jun 07, 2017 at 11:40:27PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 7, 2017 at 11:10 PM, Sakari Ailus <sakari.ai...@iki.fi> wrote:
> > > On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote:
> > >> On Wed, Jun 7, 2017 at 3:15 PM, Sakari Ailus <sakari.ai...@iki.fi> wrote:
> > >> > On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote:
> >
> > >> >> Follow the pattern, please, I suppose ti_pmic_tps68470.c
> > >>
> > >> Would it work for you?
> > >
> > > This is still a different driver from the tps68470 driver which is
> > > an MFD driver. For clarity, I'd keep pmic as part of the name (and
> > > I'd use tps68470_pmic_ prefix for internal symbols, too).
> > >
> > > As PMICs are typically linked to the kernel (vs. being modules),
> > > there's no issue with the module name. I would suppose few if any
> > > PMICs will be compiled as modules in general.
> > >
> > > It's not a big deal though. I'm fine either way.
> >
> > Okay, let's agree on  tps68470_pmic for internal prefix and for file name?
> 
> Ack. Thanks!
> 

Ack

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Sakari, Andy,

> -Original Message-
> From: Sakari Ailus [mailto:sakari.ai...@iki.fi]
> Sent: Wednesday, June 07, 2017 2:13 PM
> To: Andy Shevchenko 
> Cc: Mani, Rajmohan ; Hans de Goede
> ; linux-kernel@vger.kernel.org; linux-
> g...@vger.kernel.org; linux-a...@vger.kernel.org; Lee Jones
> ; Linus Walleij ; Alexandre
> Courbot ; Rafael J. Wysocki ; Len
> Brown 
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> On Wed, Jun 07, 2017 at 11:40:27PM +0300, Andy Shevchenko wrote:
> > On Wed, Jun 7, 2017 at 11:10 PM, Sakari Ailus  wrote:
> > > On Wed, Jun 07, 2017 at 04:40:13PM +0300, Andy Shevchenko wrote:
> > >> On Wed, Jun 7, 2017 at 3:15 PM, Sakari Ailus  wrote:
> > >> > On Tue, Jun 06, 2017 at 05:23:56PM +0300, Andy Shevchenko wrote:
> >
> > >> >> Follow the pattern, please, I suppose ti_pmic_tps68470.c
> > >>
> > >> Would it work for you?
> > >
> > > This is still a different driver from the tps68470 driver which is
> > > an MFD driver. For clarity, I'd keep pmic as part of the name (and
> > > I'd use tps68470_pmic_ prefix for internal symbols, too).
> > >
> > > As PMICs are typically linked to the kernel (vs. being modules),
> > > there's no issue with the module name. I would suppose few if any
> > > PMICs will be compiled as modules in general.
> > >
> > > It's not a big deal though. I'm fine either way.
> >
> > Okay, let's agree on  tps68470_pmic for internal prefix and for file name?
> 
> Ack. Thanks!
> 

Ack

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Hans,

> -Original Message-
> From: Hans de Goede [mailto:hdego...@redhat.com]
> Sent: Tuesday, June 06, 2017 8:22 AM
> To: Andy Shevchenko <andy.shevche...@gmail.com>; Mani, Rajmohan
> <rajmohan.m...@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones <lee.jo...@linaro.org>; Linus Walleij
> <linus.wall...@linaro.org>; Alexandre Courbot <gnu...@gmail.com>; Rafael J.
> Wysocki <r...@rjwysocki.net>; Len Brown <l...@kernel.org>
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> Hi,
> 
> On 06/06/2017 04:23 PM, Andy Shevchenko wrote:
> > +Cc Hans (that's why didn't delete anything from original mail, just
> > adding my comments).
> >
> > Hans, if you have few minutes it would be appreciated to glance on the
> > below for some issues if any since you did pass quite a good quest
> > with other PMIC drivers.
> 
> I've gone over this driver, nothing stands out in a bad way to me, IOW this
> seems like a normal PMIC OpRegion handler to me.
> 

Thanks for the reviews and time.

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Hans,

> -Original Message-
> From: Hans de Goede [mailto:hdego...@redhat.com]
> Sent: Tuesday, June 06, 2017 8:22 AM
> To: Andy Shevchenko ; Mani, Rajmohan
> 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> Hi,
> 
> On 06/06/2017 04:23 PM, Andy Shevchenko wrote:
> > +Cc Hans (that's why didn't delete anything from original mail, just
> > adding my comments).
> >
> > Hans, if you have few minutes it would be appreciated to glance on the
> > below for some issues if any since you did pass quite a good quest
> > with other PMIC drivers.
> 
> I've gone over this driver, nothing stands out in a bad way to me, IOW this
> seems like a normal PMIC OpRegion handler to me.
> 

Thanks for the reviews and time.

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Tuesday, June 06, 2017 7:24 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>; Hans de Goede
> <hdego...@redhat.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones <lee.jo...@linaro.org>; Linus Walleij
> <linus.wall...@linaro.org>; Alexandre Courbot <gnu...@gmail.com>; Rafael J.
> Wysocki <r...@rjwysocki.net>; Len Brown <l...@kernel.org>
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> +Cc Hans (that's why didn't delete anything from original mail, just
> adding my comments).
> 
> Hans, if you have few minutes it would be appreciated to glance on the below
> for some issues if any since you did pass quite a good quest with other PMIC
> drivers.
> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.m...@intel.com>
> wrote:
> > The Kabylake platform coreboot (Chrome OS equivalent of
> > BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
> > These operation regions are to enable/disable voltage regulators,
> > configure voltage regulators, enable/disable clocks and to configure
> > clocks.
> >
> > This config adds ACPI operation region support for TI TPS68470 PMIC.
> > TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for flash and incorporates two LED drivers for
> > general purpose indicators.
> > This driver enables ACPI operation region support to control voltage
> > regulators and clocks for the TPS68470 PMIC.
> >
> > Signed-off-by: Rajmohan Mani <rajmohan.m...@intel.com>
> > ---
> >  drivers/acpi/Kconfig  |  12 +
> >  drivers/acpi/Makefile |   2 +
> 
> >  drivers/acpi/pmic/pmic_tps68470.c | 454
> > ++
> 
> Follow the pattern, please, I suppose
> ti_pmic_tps68470.c
> 

I will reply to this, on the later threads on the same subject

> >  3 files changed, 468 insertions(+)
> >  create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
> >
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index
> > 1ce52f8..218d22d 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -535,4 +535,16 @@ if ARM64
> >  source "drivers/acpi/arm64/Kconfig"
> >  endif
> >
> > +config TPS68470_PMIC_OPREGION
> > +   bool "ACPI operation region support for TPS68470 PMIC"
> > +   help
> > + This config adds ACPI operation region support for TI TPS68470 
> > PMIC.
> > + TPS68470 device is an advanced power management unit that powers
> > + a Compact Camera Module (CCM), generates clocks for image sensors,
> > + drives a dual LED for flash and incorporates two LED drivers for
> > + general purpose indicators.
> > + This driver enables ACPI operation region support control voltage
> > + regulators and clocks.
> > +
> 
> > +
> 
> Extra line, remove.
> 

Ack

> >  endif  # ACPI
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index
> > b1aacfc..7113d05 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) +=
> > pmic/intel_pmic_chtwc.o
> >
> >  obj-$(CONFIG_ACPI_CONFIGFS)+= acpi_configfs.o
> >
> > +obj-$(CONFIG_TPS68470_PMIC_OPREGION)   += pmic/pmic_tps68470.o
> > +
> >  video-objs += acpi_video.o video_detect.o
> >  obj-y  += dptf/
> >
> > diff --git a/drivers/acpi/pmic/pmic_tps68470.c
> > b/drivers/acpi/pmic/pmic_tps68470.c
> > new file mode 100644
> > index 000..b2d608b
> > --- /dev/null
> > +++ b/drivers/acpi/pmic/pmic_tps68470.c
> > @@ -0,0 +1,454 @@
> > +/*
> > + * TI TPS68470 PMIC operation region driver
> > + *
> > + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> > + * Author: Rajmohan Mani <rajmohan.m...@intel.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > +version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WIT

RE: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation region driver

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Tuesday, June 06, 2017 7:24 AM
> To: Mani, Rajmohan ; Hans de Goede
> 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 3/3] ACPI / PMIC: Add TI PMIC TPS68470 operation
> region driver
> 
> +Cc Hans (that's why didn't delete anything from original mail, just
> adding my comments).
> 
> Hans, if you have few minutes it would be appreciated to glance on the below
> for some issues if any since you did pass quite a good quest with other PMIC
> drivers.
> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> wrote:
> > The Kabylake platform coreboot (Chrome OS equivalent of
> > BIOS) has defined 4 operation regions for the TI TPS68470 PMIC.
> > These operation regions are to enable/disable voltage regulators,
> > configure voltage regulators, enable/disable clocks and to configure
> > clocks.
> >
> > This config adds ACPI operation region support for TI TPS68470 PMIC.
> > TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for flash and incorporates two LED drivers for
> > general purpose indicators.
> > This driver enables ACPI operation region support to control voltage
> > regulators and clocks for the TPS68470 PMIC.
> >
> > Signed-off-by: Rajmohan Mani 
> > ---
> >  drivers/acpi/Kconfig  |  12 +
> >  drivers/acpi/Makefile |   2 +
> 
> >  drivers/acpi/pmic/pmic_tps68470.c | 454
> > ++
> 
> Follow the pattern, please, I suppose
> ti_pmic_tps68470.c
> 

I will reply to this, on the later threads on the same subject

> >  3 files changed, 468 insertions(+)
> >  create mode 100644 drivers/acpi/pmic/pmic_tps68470.c
> >
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig index
> > 1ce52f8..218d22d 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -535,4 +535,16 @@ if ARM64
> >  source "drivers/acpi/arm64/Kconfig"
> >  endif
> >
> > +config TPS68470_PMIC_OPREGION
> > +   bool "ACPI operation region support for TPS68470 PMIC"
> > +   help
> > + This config adds ACPI operation region support for TI TPS68470 
> > PMIC.
> > + TPS68470 device is an advanced power management unit that powers
> > + a Compact Camera Module (CCM), generates clocks for image sensors,
> > + drives a dual LED for flash and incorporates two LED drivers for
> > + general purpose indicators.
> > + This driver enables ACPI operation region support control voltage
> > + regulators and clocks.
> > +
> 
> > +
> 
> Extra line, remove.
> 

Ack

> >  endif  # ACPI
> > diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile index
> > b1aacfc..7113d05 100644
> > --- a/drivers/acpi/Makefile
> > +++ b/drivers/acpi/Makefile
> > @@ -106,6 +106,8 @@ obj-$(CONFIG_CHT_WC_PMIC_OPREGION) +=
> > pmic/intel_pmic_chtwc.o
> >
> >  obj-$(CONFIG_ACPI_CONFIGFS)+= acpi_configfs.o
> >
> > +obj-$(CONFIG_TPS68470_PMIC_OPREGION)   += pmic/pmic_tps68470.o
> > +
> >  video-objs += acpi_video.o video_detect.o
> >  obj-y  += dptf/
> >
> > diff --git a/drivers/acpi/pmic/pmic_tps68470.c
> > b/drivers/acpi/pmic/pmic_tps68470.c
> > new file mode 100644
> > index 000..b2d608b
> > --- /dev/null
> > +++ b/drivers/acpi/pmic/pmic_tps68470.c
> > @@ -0,0 +1,454 @@
> > +/*
> > + * TI TPS68470 PMIC operation region driver
> > + *
> > + * Copyright (C) 2017 Intel Corporation. All rights reserved.
> > + * Author: Rajmohan Mani 
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > +version
> > + * 2 as published by the Free Software Foundation.
> > + *
> > + * 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.
> > + *
> > + * Based on drivers/acpi/pmic/intel_pmic* drivers
> > + *
> &g

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

> On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
>  wrote:
> > > The TPS68470 device is an advanced power management unit that powers
> > > a Compact Camera Module (CCM), generates clocks for image sensors,
> > > drives a dual LED for Flash and incorporates two LED drivers for
> > > general purpose indicators.
> > >
> > > This patch adds support for TPS68470 mfd device.
> >
> > I dunno why you decide to send this out now, see my comments below.
> >
> > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > +   unsigned int version;
> > > +   int ret;
> >
> > > +   /* FIXME: configure these dynamically */
> >
> > So, what prevents you to fix this?
> 
> Nothing I suppose. They're however not needed right now and can be
> implemented later on if they're ever needed.
> 

Ack

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

> On Tue, Jun 06, 2017 at 03:59:49PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani
>  wrote:
> > > The TPS68470 device is an advanced power management unit that powers
> > > a Compact Camera Module (CCM), generates clocks for image sensors,
> > > drives a dual LED for Flash and incorporates two LED drivers for
> > > general purpose indicators.
> > >
> > > This patch adds support for TPS68470 mfd device.
> >
> > I dunno why you decide to send this out now, see my comments below.
> >
> > > +static int tps68470_chip_init(struct tps68470 *tps) {
> > > +   unsigned int version;
> > > +   int ret;
> >
> > > +   /* FIXME: configure these dynamically */
> >
> > So, what prevents you to fix this?
> 
> Nothing I suppose. They're however not needed right now and can be
> implemented later on if they're ever needed.
> 

Ack

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Tuesday, June 06, 2017 6:00 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones <lee.jo...@linaro.org>; Linus Walleij
> <linus.wall...@linaro.org>; Alexandre Courbot <gnu...@gmail.com>; Rafael J.
> Wysocki <r...@rjwysocki.net>; Len Brown <l...@kernel.org>
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani <rajmohan.m...@intel.com>
> wrote:
> > The TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> 
> I dunno why you decide to send this out now, see my comments below.
> 

We decided to go with the submission of these drivers for upstream review 
sooner rather than later.

> > +static int tps68470_chip_init(struct tps68470 *tps) {
> > +   unsigned int version;
> > +   int ret;
> 
> > +   /* FIXME: configure these dynamically */
> 
> So, what prevents you to fix this?
> 

I will respond on top of Sakari's response.

> > +   /* Enable Daisy Chain LDO and configure relevant GPIOs as
> > + output */
> 
> > +}
> 
> > +static int tps68470_probe(struct i2c_client *client) {
> > +   struct tps68470 *tps;
> > +   int ret;
> > +
> > +   tps = devm_kzalloc(>dev, sizeof(*tps), GFP_KERNEL);
> > +   if (!tps)
> > +   return -ENOMEM;
> > +
> > +   mutex_init(>lock);
> > +   i2c_set_clientdata(client, tps);
> > +   tps->dev = >dev;
> > +
> > +   tps->regmap = devm_regmap_init_i2c(client,
> _regmap_config);
> > +   if (IS_ERR(tps->regmap)) {
> > +   dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
> > +   return PTR_ERR(tps->regmap);
> > +   }
> > +
> 
> > +   ret = mfd_add_devices(tps->dev, -1, tps68470s,
> > + ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> 
> devm_?
> 

Ack

> > +   if (ret < 0) {
> > +   dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   ret = tps68470_chip_init(tps);
> > +   if (ret < 0) {
> > +   dev_err(tps->dev, "TPS68470 Init Error %d\n", ret);
> > +   goto fail;
> > +   }
> > +
> > +   return 0;
> 
> > +fail:
> > +   mutex_lock(>lock);
> 
> I'm not sure you need this mutex to be held here.
> Otherwise your code has a bug with locking.
> 

Repeating the response to Heikki here

I had this following question from Alan Cox on the original code without these 
wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not 
in parallel issuing a tps68470_ operation when the device goes away (eg if I 
down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks 
around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the 
same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver 
just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these 
wrappers around regmap_* calls.

> > +   mfd_remove_devices(tps->dev);
> > +   mutex_unlock(>lock);
> > +
> > +   return ret;
> 
> Taking above into consideration I suggest to clarify your locking scheme.
> 

Same as above.

> > +}
> > +
> > +static int tps68470_remove(struct i2c_client *client) {
> > +   struct tps68470 *tps = i2c_get_clientdata(client);
> > +
> 
> > +   mutex_lock(>lock);
> > +   mfd_remove_devices(tps->dev);
> > +   mutex_unlock(>lock);
> 
> Ditto.
> 

Same as above

> > +   return 0;
> > +}
> 
> > +/**
> > + * struct tps68470 - tps68470 sub-driver chip access routines
> > + *
> 
> kbuild bot will be unhappy. You need to file a description per field.
> 

Ack
It looks like this structure will go away, once I implement the feedback from 
Heikki.

> > + * Device data may be used to access the TPS68470 chip */
> > +
> > +struct tps68470 {
> > +   struct device *dev;
> > +   struct regmap *regmap;
> 
> > +   /*
> > +* Used to synchronize access to tps68470_ operations
> > +* and addition and removal of mfd devices
> > +*/
> 
> Move it to kernel-doc above.
> 

Same as above

> > +   struct mutex lock;
> > +};
> 
> --
> With Best Regards,
> Andy Shevchenko

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Andy,

Thanks for the reviews and patience.

> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Tuesday, June 06, 2017 6:00 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> On Tue, Jun 6, 2017 at 2:55 PM, Rajmohan Mani 
> wrote:
> > The TPS68470 device is an advanced power management unit that powers a
> > Compact Camera Module (CCM), generates clocks for image sensors,
> > drives a dual LED for Flash and incorporates two LED drivers for
> > general purpose indicators.
> >
> > This patch adds support for TPS68470 mfd device.
> 
> I dunno why you decide to send this out now, see my comments below.
> 

We decided to go with the submission of these drivers for upstream review 
sooner rather than later.

> > +static int tps68470_chip_init(struct tps68470 *tps) {
> > +   unsigned int version;
> > +   int ret;
> 
> > +   /* FIXME: configure these dynamically */
> 
> So, what prevents you to fix this?
> 

I will respond on top of Sakari's response.

> > +   /* Enable Daisy Chain LDO and configure relevant GPIOs as
> > + output */
> 
> > +}
> 
> > +static int tps68470_probe(struct i2c_client *client) {
> > +   struct tps68470 *tps;
> > +   int ret;
> > +
> > +   tps = devm_kzalloc(>dev, sizeof(*tps), GFP_KERNEL);
> > +   if (!tps)
> > +   return -ENOMEM;
> > +
> > +   mutex_init(>lock);
> > +   i2c_set_clientdata(client, tps);
> > +   tps->dev = >dev;
> > +
> > +   tps->regmap = devm_regmap_init_i2c(client,
> _regmap_config);
> > +   if (IS_ERR(tps->regmap)) {
> > +   dev_err(tps->dev, "devm_regmap_init_i2c Error %d\n", ret);
> > +   return PTR_ERR(tps->regmap);
> > +   }
> > +
> 
> > +   ret = mfd_add_devices(tps->dev, -1, tps68470s,
> > + ARRAY_SIZE(tps68470s), NULL, 0, NULL);
> 
> devm_?
> 

Ack

> > +   if (ret < 0) {
> > +   dev_err(tps->dev, "mfd_add_devices failed: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   ret = tps68470_chip_init(tps);
> > +   if (ret < 0) {
> > +   dev_err(tps->dev, "TPS68470 Init Error %d\n", ret);
> > +   goto fail;
> > +   }
> > +
> > +   return 0;
> 
> > +fail:
> > +   mutex_lock(>lock);
> 
> I'm not sure you need this mutex to be held here.
> Otherwise your code has a bug with locking.
> 

Repeating the response to Heikki here

I had this following question from Alan Cox on the original code without these 
wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not 
in parallel issuing a tps68470_ operation when the device goes away (eg if I 
down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks 
around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the 
same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver 
just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these 
wrappers around regmap_* calls.

> > +   mfd_remove_devices(tps->dev);
> > +   mutex_unlock(>lock);
> > +
> > +   return ret;
> 
> Taking above into consideration I suggest to clarify your locking scheme.
> 

Same as above.

> > +}
> > +
> > +static int tps68470_remove(struct i2c_client *client) {
> > +   struct tps68470 *tps = i2c_get_clientdata(client);
> > +
> 
> > +   mutex_lock(>lock);
> > +   mfd_remove_devices(tps->dev);
> > +   mutex_unlock(>lock);
> 
> Ditto.
> 

Same as above

> > +   return 0;
> > +}
> 
> > +/**
> > + * struct tps68470 - tps68470 sub-driver chip access routines
> > + *
> 
> kbuild bot will be unhappy. You need to file a description per field.
> 

Ack
It looks like this structure will go away, once I implement the feedback from 
Heikki.

> > + * Device data may be used to access the TPS68470 chip */
> > +
> > +struct tps68470 {
> > +   struct device *dev;
> > +   struct regmap *regmap;
> 
> > +   /*
> > +* Used to synchronize access to tps68470_ operations
> > +* and addition and removal of mfd devices
> > +*/
> 
> Move it to kernel-doc above.
> 

Same as above

> > +   struct mutex lock;
> > +};
> 
> --
> With Best Regards,
> Andy Shevchenko

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Heikki,

Thanks for the reviews and patience.

> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: Tuesday, June 06, 2017 5:49 AM
> To: Mani, Rajmohan <rajmohan.m...@intel.com>
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones <lee.jo...@linaro.org>; Linus Walleij
> <linus.wall...@linaro.org>; Alexandre Courbot <gnu...@gmail.com>; Rafael J.
> Wysocki <r...@rjwysocki.net>; Len Brown <l...@kernel.org>
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> Hi Rajmohan,
> 
> On Tue, Jun 06, 2017 at 04:55:16AM -0700, Rajmohan Mani wrote:
> > +/*
> > + * tps68470_reg_read: Read a single tps68470 register.
> > + *
> > + * @tps: Device to read from.
> > + * @reg: Register to read.
> > + * @val: Contains the value
> > + */
> > +int tps68470_reg_read(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int *val)
> > +{
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_read(tps->regmap, reg, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_reg_read);
> > +
> > +/*
> > + * tps68470_reg_write: Write a single tps68470 register.
> > + *
> > + * @tps68470: Device to write to.
> > + * @reg: Register to write to.
> > + * @val: Value to write.
> > + */
> > +int tps68470_reg_write(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int val)
> > +{
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_write(tps->regmap, reg, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_reg_write);
> > +
> > +/*
> > + * tps68470_update_bits: Modify bits w.r.t mask and val.
> > + *
> > + * @tps68470: Device to write to.
> > + * @reg: Register to read-write to.
> > + * @mask: Mask.
> > + * @val: Value to write.
> > + */
> > +int tps68470_update_bits(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int mask, unsigned int val) {
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_update_bits(tps->regmap, reg, mask, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_update_bits);
> 
> I'm not sure you need those above wrappers at all, regmap is handling locking 
> in
> any case.
> 

I had this following question from Alan Cox on the original code without these 
wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not 
in parallel issuing a tps68470_ operation when the device goes away (eg if I 
down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks 
around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the 
same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver 
just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these 
wrappers around regmap_* calls.

> > +static const struct regmap_config tps68470_regmap_config = {
> > +   .reg_bits = 8,
> > +   .val_bits = 8,
> > +   .max_register = TPS68470_REG_MAX,
> > +};
> > +
> > +static int tps68470_chip_init(struct tps68470 *tps) {
> > +   unsigned int version;
> > +   int ret;
> > +
> > +   ret = tps68470_reg_read(tps, TPS68470_REG_REVID, );
> > +   if (ret < 0) {
> > +   dev_err(tps->dev,
> > +   "Failed to read revision register: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   dev_info(tps->dev, "TPS68470 REVID: 0x%x\n", version);
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_RESET, 0xff);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* FIXME: configure these dynamically */
> > +   /* Enable Daisy Chain LDO and configure relevant GPIOs as output */
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_S_I2C_CTL, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL4A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL5A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps6847

RE: [PATCH v1 1/3] mfd: Add new mfd device TPS68470

[Mani, Rajmohan]

Hi Heikki,

Thanks for the reviews and patience.

> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: Tuesday, June 06, 2017 5:49 AM
> To: Mani, Rajmohan 
> Cc: linux-kernel@vger.kernel.org; linux-g...@vger.kernel.org; linux-
> a...@vger.kernel.org; Lee Jones ; Linus Walleij
> ; Alexandre Courbot ; Rafael J.
> Wysocki ; Len Brown 
> Subject: Re: [PATCH v1 1/3] mfd: Add new mfd device TPS68470
> 
> Hi Rajmohan,
> 
> On Tue, Jun 06, 2017 at 04:55:16AM -0700, Rajmohan Mani wrote:
> > +/*
> > + * tps68470_reg_read: Read a single tps68470 register.
> > + *
> > + * @tps: Device to read from.
> > + * @reg: Register to read.
> > + * @val: Contains the value
> > + */
> > +int tps68470_reg_read(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int *val)
> > +{
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_read(tps->regmap, reg, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_reg_read);
> > +
> > +/*
> > + * tps68470_reg_write: Write a single tps68470 register.
> > + *
> > + * @tps68470: Device to write to.
> > + * @reg: Register to write to.
> > + * @val: Value to write.
> > + */
> > +int tps68470_reg_write(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int val)
> > +{
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_write(tps->regmap, reg, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_reg_write);
> > +
> > +/*
> > + * tps68470_update_bits: Modify bits w.r.t mask and val.
> > + *
> > + * @tps68470: Device to write to.
> > + * @reg: Register to read-write to.
> > + * @mask: Mask.
> > + * @val: Value to write.
> > + */
> > +int tps68470_update_bits(struct tps68470 *tps, unsigned int reg,
> > +   unsigned int mask, unsigned int val) {
> > +   int ret;
> > +
> > +   mutex_lock(>lock);
> > +   ret = regmap_update_bits(tps->regmap, reg, mask, val);
> > +   mutex_unlock(>lock);
> > +   return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tps68470_update_bits);
> 
> I'm not sure you need those above wrappers at all, regmap is handling locking 
> in
> any case.
> 

I had this following question from Alan Cox on the original code without these 
wrappers.

"What is the model for insuring that no interrupt or thread of a driver is not 
in parallel issuing a tps68470_ operation when the device goes away (eg if I 
down the i2c controller) ?"

To address the above concerns, I got extra cautious and implemented locks 
around the regmap_* calls.
Now, I have been asked from more than one reviewer about the necessity of the 
same.
With the use of devm_* calls, tps68470_remove() goes away and leaves the driver 
just with regmap_* calls.
Unless I hear from Alan or other reviewers otherwise, I will drop these 
wrappers around regmap_* calls.

> > +static const struct regmap_config tps68470_regmap_config = {
> > +   .reg_bits = 8,
> > +   .val_bits = 8,
> > +   .max_register = TPS68470_REG_MAX,
> > +};
> > +
> > +static int tps68470_chip_init(struct tps68470 *tps) {
> > +   unsigned int version;
> > +   int ret;
> > +
> > +   ret = tps68470_reg_read(tps, TPS68470_REG_REVID, );
> > +   if (ret < 0) {
> > +   dev_err(tps->dev,
> > +   "Failed to read revision register: %d\n", ret);
> > +   return ret;
> > +   }
> > +
> > +   dev_info(tps->dev, "TPS68470 REVID: 0x%x\n", version);
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_RESET, 0xff);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /* FIXME: configure these dynamically */
> > +   /* Enable Daisy Chain LDO and configure relevant GPIOs as output */
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_S_I2C_CTL, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL4A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL5A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   ret = tps68470_reg_write(tps, TPS68470_REG_GPCTL6A, 2);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > +   /*
> > +* When SDA and SCL are r