Re: [PATCH v3 3/4] mfd: tps65917: Add driver for the TPS65917 PMIC

[Keerthy]

On Friday 23 May 2014 03:36 PM, Lee Jones wrote:
> > > > > > The TPS65917 chip is a power management IC for Portable Navigation Systems
> > > > > > and Tablet Computing devices. It contains the following components:
> > > > > >
> > > > > >   - Regulators.
> > > > > >   - Over Temperature warning and Shut down.
> > > > > >
> > > > > > This patch adds support for tps65917 mfd device. At this time only
> > > > > > the regulator functionality is made available.
> > > > > >
> > > > > > Signed-off-by: Keerthy <j-keerthy@xxxxxx>
> > > > > > ---
> > > > > > v3 Changes:
> > > > > >
> > > > > >   * Header file formating
> > > > > >   * Changed the cache_type to REGCACHE_RBTREE
> > > > > >   * removed unnecessary code
> > > > > >   * Corrected documentation style
> > > > > >   * Added pm_power_off function
> > > > > >
> > > > > >   v2 Changes:
> > > > > >
> > > > > >   * Added volatile register check as some of the registers
> > > > > >     in the set are volatile.
> > > > > > drivers/mfd/Kconfig          |   12 +
> > > > > >   drivers/mfd/Makefile         |    1 +
> > > > > >   drivers/mfd/tps65917.c       |  594 +++++++++++++++++
> > > > > >   include/linux/mfd/tps65917.h | 1485 ++++++++++++++++++++++++++++++++++++++++++
> > > > > >   4 files changed, 2092 insertions(+)
> > > > > >   create mode 100644 drivers/mfd/tps65917.c
> > > > > >   create mode 100644 include/linux/mfd/tps65917.h
> [...]
>
> > > > > > +		ret = regmap_read(tps65917->regmap[slave], addr, &reg);
> > > > > > +		if (ret)
> > > > > > +			goto err_irq;
> > > > > > +	}i
> > > > > What does the read do?  You're not doing anything with the value.
> > > > This pad1 and pad2 stuff is not needed. I will remove this.
> > > Then why is it in here?
> > >
> > > Did you copy this code from somewhere, if so, where?
> > >
> > > Okay, I just answered my own question.  There is so much common code
> > > in between this and palmas, there is no way I'm going to accept this
> > > driver.  Please merge it in with the palmas driver!
> > The chip is more like a subset of palmas with lot of register offset changes
> > and register bit field changes. Adding this would make it clumsy.
> > There could
> > be lot of checks. That is why i chose to write a new driver.
> >
> > Palmas driver already supports palmas variants and tps659038. Merging
> > this would mean more and more checks :-/.
> Then find an elegant way of representing the variants.  I'm not
> prepared to carry that much duplicated code in MFD.  It's already
> overladened and in need of an overhaul.  This will exacerbate the
> matter.

Okay. I am working on that.

Regards,
Keerthy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/