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.
> >
> 
> Okay. I am working on that.

Thanks.

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