Re: [PATCH 1/4] mfd: add support for AMS AS3722 PMIC

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2113 bytes --]

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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]