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

[Lee Jones <lee.jones@linaro.org>]

> >>>>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@ti.com>
> >>>>---
> >>>>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.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog