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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20140523111843.GW19747@lee--X1>
Date:	Fri, 23 May 2014 12:18:43 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Keerthy <a0393675@...com>
Cc:	Keerthy <j-keerthy@...com>, devicetree@...r.kernel.org,
	robh+dt@...nel.org, mark.rutland@....com, sameo@...ux.intel.com,
	grant.likely@...aro.org, ian@...mlogic.co.uk,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	broonie@...aro.org, linux-omap@...r.kernel.org
Subject: Re: [PATCH v3 3/4] mfd: tps65917: Add driver for the TPS65917 PMIC

> >>>>>>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@...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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/