linux-kernel - Re: [PATCH 1/4] mfd: add support for AMS AS3722 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130917112444.GU21013@sirena.org.uk>
Date:	Tue, 17 Sep 2013 12:24:44 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Laxman Dewangan <ldewangan@...dia.com>
Cc:	lee.jones@...aro.org, sameo@...ux.intel.com,
	linus.walleij@...aro.org, akpm@...ux-foundation.org,
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
	rtc-linux@...glegroups.com, rob.herring@...xeda.com,
	mark.rutland@....com, pawel.moll@....com, swarren@...dotorg.org,
	rob@...dley.net, ijc+devicetree@...lion.org.uk,
	grant.likely@...aro.org, florian.lobmaier@....com
Subject: Re: [PATCH 1/4] mfd: add support for AMS AS3722 PMIC

On Tue, Sep 17, 2013 at 12:15:35PM +0530, Laxman Dewangan wrote:

> +MFD driver adds following mfd devices with their compatible values:
> +as3722-gpio: The compatible value of this as3722 gpio driver is
> +	     "ams,as3722-gpio";
> +as3722-regulator: The compatible value of this as3722 regulator driver is
> +	     "ams,as3722-regulator";
> +as3722-rtc: The compatible value of this as3722 rtc driver is
> +	     "ams,as3722-rtc";
> +as3722-adc: The compatible value of this as3722 adc driver is
> +	     "ams,as3722-adc";
> +as3722-power-off: he compatible value of this as3722 power off driver is
> +	     "ams,as3722-power-off".

Personally I find this to be exposing implementation details of Linux -
unless there is something reusable about the binding that'd allow it to
be used to describe the contents of the chip the subnodes really aren't 
adding any information that wasn't present from just knowing the parent
chip.  If there were relocatable IPs it'd be a bit different.

> +#define reg_range(min, max)	{.range_min = min, .range_max = max,}
> +static const struct regmap_range as3722_readable_ranges[] = {
> +	reg_range(AS3722_SD0_VOLTAGE_REG, AS3722_SD6_VOLTAGE_REG),
> +	reg_range(AS3722_GPIO0_CONTROL_REG, AS3722_LDO7_VOLTAGE_REG),

If this macro is useful it should be a generic thing in the header, not
done driver local.

> +static const struct regmap_access_table as3722_volatile_table = {
> +	.no_ranges = as3722_voltaile_ranges,
> +	.n_no_ranges = ARRAY_SIZE(as3722_voltaile_ranges),
> +};

This looks wrong - the registers listed in _volatile_ranges are in fact
the ones that are not volatile.  I'd rename _volatile_ranges to
something like _cacheable_ranges.

> +static int __init as3722_i2c_init(void)
> +{
> +	return i2c_add_driver(&as3722_i2c_driver);
> +}
> +subsys_initcall(as3722_i2c_init);
> +
> +static void __exit as3722_i2c_exit(void)
> +{
> +	i2c_del_driver(&as3722_i2c_driver);
> +}
> +module_exit(as3722_i2c_exit);

Modern systems should be able to use module_i2c_driver() and deferred
probing - is this not working well?

Download attachment "signature.asc" of type "application/pgp-signature" (837 bytes)