Re: [PATCH 1/2] mfd: Add Freescale's PMIC MC34708 support

[Lothar WaÃmann]

Hi,

Ying-Chun Liu (PaulLiu) writes:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@xxxxxxxxxx>
> 
> Freescale MC34708 is a PMIC which supports the following features:
>  * 6 multi-mode buck regulators
>  * Boost regulator for USB OTG.
>  * 8 regulators for thermal budget optimization
>  * 10-bit ADC
>  * Real time clock
> 
> Signed-off-by: Robin Gong <B38343@xxxxxxxxxxxxx>
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@xxxxxxxxxx>
> Cc: Samuel Ortiz <sameo@xxxxxxxxxxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> Cc: Shawn Guo <shawn.guo@xxxxxxxxxx>
> ---
>  Documentation/devicetree/bindings/mfd/mc34708.txt |   61 ++
>  drivers/mfd/Kconfig                               |   11 +
>  drivers/mfd/Makefile                              |    1 +
>  drivers/mfd/mc34708-core.c                        |  634 +++++++++++++++++++++
>  include/linux/mfd/mc34708.h                       |  134 +++++
>  5 files changed, 841 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/mc34708.txt
>  create mode 100644 drivers/mfd/mc34708-core.c
>  create mode 100644 include/linux/mfd/mc34708.h
> 
> diff --git a/Documentation/devicetree/bindings/mfd/mc34708.txt b/Documentation/devicetree/bindings/mfd/mc34708.txt
> new file mode 100644
> index 0000000..2bb5c9e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/mc34708.txt
> @@ -0,0 +1,61 @@
> +* Freescale MC34708 Power Management Integrated Circuit (PMIC)
> +
> +Required properties:
> +- compatible : Must be "fsl,mc34708"
> +
> +Optional properties:
> +- fsl,mc34708-uses-adc   : Indicate the ADC is being used
> +- fsl,mc34708-uses-rtc   : Indicate the RTC is being used
> +- fsl,mc34708-uses-ts    : Indicate the touchscreen controller is being used
> +
> +Sub-nodes:
> +- regulators : Contain the regulator nodes.  The MC34708 regulators are
> +  bound using their names as listed below for enabling.
> +
> +    mc34708__sw1a    : regulator SW1A
> +    mc34708__sw1b    : regulator SW1B
> +    mc34708__sw2     : regulator SW2
> +    mc34708__sw3     : regulator SW3
> +    mc34708__sw4A    : regulator SW4A
> +    mc34708__sw4b    : regulator SW4B
> +    mc34708__swbst   : regulator SWBST
> +    mc34708__vpll    : regulator VPLL
> +    mc34708__vrefddr : regulator VREFDDR
> +    mc34708__vusb    : regulator VUSB
> +    mc34708__vusb2   : regulator VUSB2
> +    mc34708__vdac    : regulator VDAC
> +    mc34708__vgen1   : regulator VGEN1
> +    mc34708__vgen2   : regulator VGEN2
> +
> +  The bindings details of individual regulator device can be found in:
> +  Documentation/devicetree/bindings/regulator/regulator.txt
> +
> +Examples:
> +
> +i2c@63fc8000 {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "fsl,imx53-i2c", "fsl,imx1-i2c";
> +	reg = <0x63fc8000 0x4000>;
> +	interrupts = <62>;
> +	status = "okay";
> +
> +	pmic: mc34708@8 {
> +		compatible = "fsl,mc34708";
> +		reg = <0x08>;
> +
> +		regulators {
> +			mc34708__sw1a {
> +				regulator-min-microvolt = <650000>;
> +				regulator-max-microvolt = <1437500>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			mc34708__vusb {
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +		};
> +	};
> +};
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 29f463c..17b6cc5 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -600,6 +600,17 @@ config MFD_MC13XXX
>  	  additional drivers must be enabled in order to use the
>  	  functionality of the device.
>  
> +config MFD_MC34708
> +	tristate "Support for Freescale's PMIC MC34708"
> +	depends on I2C
> +	depends on OF
> +	select MFD_CORE
> +	help
> +	  Support for the Freescale's PMIC MC34708
> +	  This driver provides common support for accessing the device,
> +	  additional drivers must be enabled in order to use the
> +	  functionality of the device.
> +
>  config ABX500_CORE
>  	bool "ST-Ericsson ABX500 Mixed Signal Circuit register functions"
>  	default y if ARCH_U300 || ARCH_U8500
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 05fa538..b98d943 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -54,6 +54,7 @@ obj-$(CONFIG_TWL6030_PWM)	+= twl6030-pwm.o
>  obj-$(CONFIG_TWL6040_CORE)	+= twl6040-core.o twl6040-irq.o
>  
>  obj-$(CONFIG_MFD_MC13XXX)	+= mc13xxx-core.o
> +obj-$(CONFIG_MFD_MC34708)       += mc34708-core.o
>  
other entries are using TAB for indentation!

>  obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
>  
> diff --git a/drivers/mfd/mc34708-core.c b/drivers/mfd/mc34708-core.c
> new file mode 100644
> index 0000000..54f469b
> --- /dev/null
> +++ b/drivers/mfd/mc34708-core.c
> @@ -0,0 +1,634 @@
> +/*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mutex.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/mc34708.h>
> +#include <linux/delay.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +
> +struct mc34708 {
> +	struct i2c_client *i2c_client;
> +	struct mutex lock;
> +	int irq;
> +
> +	irq_handler_t irqhandler[MC34708_NUM_IRQ];
> +	void *irqdata[MC34708_NUM_IRQ];
> +};
> +
> +/*!
> + * This is the enumeration of versions of PMIC
> + */
> +enum mc34708_id {
> +	MC_PMIC_ID_MC34708,
> +	MC_PMIC_ID_INVALID,
> +};
> +
> +struct mc34708_version_t {
> +	/*!
> +	 * PMIC version identifier.
> +	 */
> +	enum mc34708_id id;
> +	/*!
> +	 * Revision of the PMIC.
> +	 */
> +	int revision;
> +};
> +
> +#define PMIC_I2C_RETRY_TIMES		10
> +
> +static const struct i2c_device_id mc34708_device_id[] = {
> +	{"mc34708", MC_PMIC_ID_MC34708},
> +	{},
> +};
> +
> +static const char * const mc34708_chipname[] = {
> +	[MC_PMIC_ID_MC34708] = "mc34708",
> +};
> +
> +void mc34708_lock(struct mc34708 *mc_pmic)
> +{
> +	if (!mutex_trylock(&mc_pmic->lock)) {
> +		dev_dbg(&mc_pmic->i2c_client->dev, "wait for %s from %pf\n",
> +			__func__, __builtin_return_address(0));
> +
> +		mutex_lock(&mc_pmic->lock);
> +	}
> +	dev_dbg(&mc_pmic->i2c_client->dev, "%s from %pf\n",
> +		__func__, __builtin_return_address(0));
> +}
> +EXPORT_SYMBOL(mc34708_lock);
> +
> +void mc34708_unlock(struct mc34708 *mc_pmic)
> +{
> +	dev_dbg(&mc_pmic->i2c_client->dev, "%s from %pf\n",
> +		__func__, __builtin_return_address(0));
> +	mutex_unlock(&mc_pmic->lock);
> +}
> +EXPORT_SYMBOL(mc34708_unlock);
> +
> +static int mc34708_i2c_24bit_read(struct i2c_client *client,
> +				  unsigned int offset,
> +				  unsigned int *value)
> +{
> +	unsigned char buf[3];
> +	int ret;
> +	int i;
> +
> +	memset(buf, 0, 3);
> +	for (i = 0; i < PMIC_I2C_RETRY_TIMES; i++) {
> +		ret = i2c_smbus_read_i2c_block_data(client, offset, 3, buf);
> +		if (ret == 3)
> +			break;
> +		usleep_range(1000, 1500);
> +	}
> +
> +	if (ret == 3) {
> +		*value = buf[0] << 16 | buf[1] << 8 | buf[2];
> +		return ret;
> +	} else {
> +		dev_err(&client->dev, "24bit read error, ret = %d\n", ret);
> +		return -1;	/* return -1 on failure */
>
bogus return value. -1 means -EPERM! Please use a sensible error value
from errno.h. Also if ret < 0 it is an error code already, that should
be promoted to the caller.

> +	}
> +}
> +
> +int mc34708_reg_read(struct mc34708 *mc_pmic, unsigned int offset,
> +		     u32 *val)
> +{
> +	BUG_ON(!mutex_is_locked(&mc_pmic->lock));
> +
> +	if (offset > MC34708_NUMREGS)
> +		return -EINVAL;
> +
> +	if (mc34708_i2c_24bit_read(mc_pmic->i2c_client, offset, val) == -1)
> +		return -1;
>
Promote the error code returned by mc34708_i2c_24bit_read() instead
of inventing a new one.

> +	*val &= 0xffffff;
> +
> +	dev_vdbg(&mc_pmic->i2c_client->dev, "mc_pmic read [%02d] -> 0x%06x\n",
> +		 offset, *val);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mc34708_reg_read);
> +
> +static int mc34708_i2c_24bit_write(struct i2c_client *client,
> +				   unsigned int offset, unsigned int reg_val)
> +{
> +	char buf[3];
> +	int ret;
> +	int i;
> +
> +	buf[0] = (reg_val >> 16) & 0xff;
> +	buf[1] = (reg_val >> 8) & 0xff;
> +	buf[2] = (reg_val) & 0xff;
> +
> +	for (i = 0; i < PMIC_I2C_RETRY_TIMES; i++) {
> +		ret = i2c_smbus_write_i2c_block_data(client, offset, 3, buf);
> +		if (ret == 0)
> +			break;
> +		usleep_range(1000, 1500);
> +	}
> +	if (i == PMIC_I2C_RETRY_TIMES)
> +		dev_err(&client->dev, "24bit write error, ret = %d\n", ret);
> +
> +	return ret;
> +}
> +
> +int mc34708_reg_write(struct mc34708 *mc_pmic, unsigned int offset, u32 val)
> +{
> +	BUG_ON(!mutex_is_locked(&mc_pmic->lock));
> +
> +	if (offset > MC34708_NUMREGS)
> +		return -EINVAL;
> +
> +	if (mc34708_i2c_24bit_write(mc_pmic->i2c_client, offset, val))
> +		return -1;
>
dto.

> +	val &= 0xffffff;
> +
> +	dev_vdbg(&mc_pmic->i2c_client->dev, "mc_pmic write[%02d] -> 0x%06x\n",
> +		 offset, val);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mc34708_reg_write);
> +
> +int mc34708_reg_rmw(struct mc34708 *mc_pmic, unsigned int offset, u32 mask,
> +		    u32 val)
> +{
> +	int ret;
> +	u32 valread;
> +
> +	BUG_ON(val & ~mask);
> +
return -EINVAL instead of crashing the kernel?

> +	ret = mc34708_reg_read(mc_pmic, offset, &valread);
> +	if (ret)
> +		return ret;
> +
> +	valread = (valread & ~mask) | val;
> +
> +	return mc34708_reg_write(mc_pmic, offset, valread);
> +}
> +EXPORT_SYMBOL(mc34708_reg_rmw);
> +
> +int mc34708_irq_mask(struct mc34708 *mc_pmic, int irq)
> +{
> +	int ret;
> +	unsigned int offmask =
> +	    irq < 24 ? MC34708_REG_INT_MASK0 : MC34708_REG_INT_MASK1;
> +	u32 irqbit = 1 << (irq < 24 ? irq : irq - 24);
> +	u32 mask;
> +
> +	if (irq < 0 || irq >= MC34708_NUM_IRQ)
> +		return -EINVAL;
> +
> +	ret = mc34708_reg_read(mc_pmic, offmask, &mask);
> +	if (ret)
> +		return ret;
> +
> +	if (mask & irqbit)
> +		/* already masked */
> +		return 0;
> +
> +	return mc34708_reg_write(mc_pmic, offmask, mask | irqbit);
> +}
> +EXPORT_SYMBOL(mc34708_irq_mask);
> +
> +int mc34708_irq_unmask(struct mc34708 *mc_pmic, int irq)
> +{
> +	int ret;
> +	unsigned int offmask =
> +	    irq < 24 ? MC34708_REG_INT_MASK0 : MC34708_REG_INT_MASK1;
> +	u32 irqbit = 1 << (irq < 24 ? irq : irq - 24);
> +	u32 mask;
> +
> +	if (irq < 0 || irq >= MC34708_NUM_IRQ)
> +		return -EINVAL;
> +
> +	ret = mc34708_reg_read(mc_pmic, offmask, &mask);
> +	if (ret)
> +		return ret;
> +
> +	if (!(mask & irqbit))
> +		/* already unmasked */
> +		return 0;
> +
> +	return mc34708_reg_write(mc_pmic, offmask, mask & ~irqbit);
> +}
> +EXPORT_SYMBOL(mc34708_irq_unmask);
> +
> +int mc34708_irq_status(struct mc34708 *mc_pmic, int irq, int *enabled,
> +		       int *pending)
> +{
> +	int ret;
> +	unsigned int offmask =
> +	    irq < 24 ? MC34708_REG_INT_MASK0 : MC34708_REG_INT_MASK1;
> +	unsigned int offstat =
> +	    irq < 24 ? MC34708_REG_INT_STATUS0 : MC34708_REG_INT_STATUS1;
> +	u32 irqbit = 1 << (irq < 24 ? irq : irq - 24);
> +
> +	if (irq < 0 || irq >= MC34708_NUM_IRQ)
> +		return -EINVAL;
> +
> +	if (enabled) {
> +		u32 mask;
> +
> +		ret = mc34708_reg_read(mc_pmic, offmask, &mask);
> +		if (ret)
> +			return ret;
> +
> +		*enabled = mask & irqbit;
> +	}
> +
> +	if (pending) {
> +		u32 stat;
> +
> +		ret = mc34708_reg_read(mc_pmic, offstat, &stat);
> +		if (ret)
> +			return ret;
> +
> +		*pending = stat & irqbit;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mc34708_irq_status);
> +
> +int mc34708_irq_ack(struct mc34708 *mc_pmic, int irq)
> +{
> +	unsigned int offstat =
> +	    irq < 24 ? MC34708_REG_INT_STATUS0 : MC34708_REG_INT_STATUS1;
> +	unsigned int val = 1 << (irq < 24 ? irq : irq - 24);
> +
> +	BUG_ON(irq < 0 || irq >= MC34708_NUM_IRQ);
> +
> +	return mc34708_reg_write(mc_pmic, offstat, val);
> +}
> +EXPORT_SYMBOL(mc34708_irq_ack);
> +
> +int mc34708_irq_request_nounmask(struct mc34708 *mc_pmic, int irq,
> +				 irq_handler_t handler, const char *name,
> +				 void *dev)
> +{
> +	BUG_ON(!mutex_is_locked(&mc_pmic->lock));
> +	BUG_ON(!handler);
> +
> +	if (irq < 0 || irq >= MC34708_NUM_IRQ)
> +		return -EINVAL;
> +
> +	if (mc_pmic->irqhandler[irq])
> +		return -EBUSY;
> +
> +	mc_pmic->irqhandler[irq] = handler;
> +	mc_pmic->irqdata[irq] = dev;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mc34708_irq_request_nounmask);
> +
> +int mc34708_irq_request(struct mc34708 *mc_pmic, int irq,
> +			irq_handler_t handler, const char *name, void *dev)
> +{
> +	int ret;
> +
> +	ret = mc34708_irq_request_nounmask(mc_pmic, irq, handler, name, dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = mc34708_irq_unmask(mc_pmic, irq);
> +	if (ret) {
> +		mc_pmic->irqhandler[irq] = NULL;
> +		mc_pmic->irqdata[irq] = NULL;
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mc34708_irq_request);
> +
> +int mc34708_irq_free(struct mc34708 *mc_pmic, int irq, void *dev)
> +{
> +	int ret;
> +	BUG_ON(!mutex_is_locked(&mc_pmic->lock));
> +
> +	if (irq < 0 || irq >= MC34708_NUM_IRQ || !mc_pmic->irqhandler[irq] ||
> +	    mc_pmic->irqdata[irq] != dev)
> +		return -EINVAL;
> +
> +	ret = mc34708_irq_mask(mc_pmic, irq);
> +	if (ret)
> +		return ret;
> +
> +	mc_pmic->irqhandler[irq] = NULL;
> +	mc_pmic->irqdata[irq] = NULL;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mc34708_irq_free);
> +
> +static inline irqreturn_t mc34708_irqhandler(struct mc34708 *mc_pmic, int irq)
> +{
> +	return mc_pmic->irqhandler[irq] (irq, mc_pmic->irqdata[irq]);
> +}
> +
> +/*
> + * returns: number of handled irqs or negative error
> + * locking: holds mc_pmic->lock
> + */
> +static int mc34708_irq_handle(struct mc34708 *mc_pmic,
> +			      unsigned int offstat, unsigned int offmask,
> +			      int baseirq)
> +{
> +	u32 stat, mask;
> +	int ret = mc34708_reg_read(mc_pmic, offstat, &stat);
> +	int num_handled = 0;
> +
> +	if (ret)
> +		return ret;
> +
> +	ret = mc34708_reg_read(mc_pmic, offmask, &mask);
> +	if (ret)
> +		return ret;
> +
> +	while (stat & ~mask) {
> +		int irq = __ffs(stat & ~mask);
> +
> +		stat &= ~(1 << irq);
> +
> +		if (likely(mc_pmic->irqhandler[baseirq + irq])) {
> +			irqreturn_t handled;
> +
> +			handled = mc34708_irqhandler(mc_pmic, baseirq + irq);
> +			if (handled == IRQ_HANDLED)
> +				num_handled++;
> +		} else {
> +			dev_err(&mc_pmic->i2c_client->dev,
> +				"BUG: irq %u but no handler\n", baseirq + irq);
> +
> +			mask |= 1 << irq;
> +
> +			ret = mc34708_reg_write(mc_pmic, offmask, mask);
> +		}
> +	}
> +
> +	return num_handled;
> +}
> +
> +static irqreturn_t mc34708_irq_thread(int irq, void *data)
> +{
> +	struct mc34708 *mc_pmic = data;
> +	irqreturn_t ret;
> +	int handled = 0;
> +
> +	mc34708_lock(mc_pmic);
> +
> +	ret = mc34708_irq_handle(mc_pmic, MC34708_REG_INT_STATUS0,
> +				 MC34708_REG_INT_MASK0, 0);
> +	if (ret > 0)
> +		handled = 1;
> +
> +	ret = mc34708_irq_handle(mc_pmic, MC34708_REG_INT_STATUS1,
> +				 MC34708_REG_INT_MASK1, 24);
> +	if (ret > 0)
> +		handled = 1;
> +
> +	mc34708_unlock(mc_pmic);
> +
> +	return IRQ_RETVAL(handled);
> +}
> +
> +#define maskval(reg, mask)	(((reg) & (mask)) >> __ffs(mask))
> +static int mc34708_identify(struct mc34708 *mc_pmic,
> +			    struct mc34708_version_t *ver)
> +{
> +	int rev_id = 0;
> +	int rev1 = 0;
> +	int rev2 = 0;
> +	int finid = 0;
> +	int icid = 0;
> +	int ret;
> +	ret = mc34708_reg_read(mc_pmic, MC34708_REG_IDENTIFICATION, &rev_id);
> +	if (ret) {
> +		dev_dbg(&mc_pmic->i2c_client->dev, "read ID error!%x\n", ret);
> +		return ret;
> +	}
> +	rev1 = (rev_id & 0x018) >> 3;
> +	rev2 = (rev_id & 0x007);
> +	icid = (rev_id & 0x01C0) >> 6;
> +	finid = (rev_id & 0x01E00) >> 9;
> +	ver->id = MC_PMIC_ID_MC34708;
> +
> +	ver->revision = ((rev1 * 10) + rev2);
> +	dev_dbg(&mc_pmic->i2c_client->dev,
> +		"mc_pmic Rev %d.%d FinVer %x detected\n", rev1, rev2, finid);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id *i2c_match_id(const struct i2c_device_id *id,
> +						const struct i2c_client *client)
> +{
> +	while (id->name[0]) {
> +		if (strcmp(client->name, id->name) == 0)
> +			return id;
> +		id++;
> +	}
> +
> +	return NULL;
> +}
> +
> +static const struct i2c_device_id *i2c_get_device_id(const struct i2c_client
> +						     *idev)
> +{
> +	const struct i2c_driver *idrv = to_i2c_driver(idev->dev.driver);
> +
> +	return i2c_match_id(idrv->id_table, idev);
> +}
> +
> +static const char *mc34708_get_chipname(struct mc34708 *mc_pmic)
> +{
> +	const struct i2c_device_id *devid =
> +	    i2c_get_device_id(mc_pmic->i2c_client);
> +
> +	if (!devid)
> +		return NULL;
> +
> +	return mc34708_chipname[devid->driver_data];
> +}
> +
> +int mc34708_get_flags(struct mc34708 *mc_pmic)
> +{
> +	struct mc34708_platform_data *pdata =
> +	    dev_get_platdata(&mc_pmic->i2c_client->dev);
> +
> +	return pdata->flags;
> +}
> +EXPORT_SYMBOL(mc34708_get_flags);
> +
> +static int mc34708_add_subdevice_pdata(struct mc34708 *mc_pmic,
> +				       const char *format, void *pdata,
> +				       size_t pdata_size)
> +{
> +	char buf[30];
> +	const char *name = mc34708_get_chipname(mc_pmic);
> +
> +	struct mfd_cell cell = {
> +		.platform_data = pdata,
> +		.pdata_size = pdata_size,
> +	};
> +
> +	/* there is no asnprintf in the kernel :-( */
> +	if (snprintf(buf, sizeof(buf), format, name) > sizeof(buf))
> +		return -E2BIG;
> +
> +	cell.name = kmemdup(buf, strlen(buf) + 1, GFP_KERNEL);
> +	if (!cell.name)
> +		return -ENOMEM;
> +
> +	return mfd_add_devices(&mc_pmic->i2c_client->dev, -1, &cell, 1, NULL,
> +			       0);
> +}
> +
> +static int mc34708_add_subdevice(struct mc34708 *mc_pmic, const char *format)
> +{
> +	return mc34708_add_subdevice_pdata(mc_pmic, format, NULL, 0);
> +}
> +
> +static int mc34708_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct mc34708 *mc_pmic;
> +	struct mc34708_version_t version;
> +	struct mc34708_platform_data *pdata = client->dev.platform_data;
> +	struct device_node *np = client->dev.of_node;
> +	int ret;
> +
> +	mc_pmic = kzalloc(sizeof(*mc_pmic), GFP_KERNEL);
> +	if (!mc_pmic)
> +		return -ENOMEM;
> +	i2c_set_clientdata(client, mc_pmic);
> +	mc_pmic->i2c_client = client;
> +
> +	mutex_init(&mc_pmic->lock);
> +	mc34708_lock(mc_pmic);
> +	mc34708_identify(mc_pmic, &version);
> +	/* mask all irqs */
> +	ret = mc34708_reg_write(mc_pmic, MC34708_REG_INT_MASK0, 0x00ffffff);
> +	if (ret)
> +		goto err_mask;
> +
> +	ret = mc34708_reg_write(mc_pmic, MC34708_REG_INT_MASK1, 0x00ffffff);
> +	if (ret)
> +		goto err_mask;
> +
> +	ret = request_threaded_irq(client->irq, NULL, mc34708_irq_thread,
> +				   IRQF_ONESHOT | IRQF_TRIGGER_HIGH, "mc_pmic",
> +				   mc_pmic);
> +
> +	if (ret) {
> + err_mask:
> +		mc34708_unlock(mc_pmic);
> +		dev_set_drvdata(&client->dev, NULL);
> +		kfree(mc_pmic);
> +		return ret;
> +	}
It would be much clearer to put the error exit at the end of the
function.

> +
> +	mc34708_unlock(mc_pmic);
> +
> +	if (pdata && pdata->flags & MC34708_USE_ADC)
> +		mc34708_add_subdevice(mc_pmic, "%s-adc");
> +	else if (of_get_property(np, "fsl,mc34708-uses-adc", NULL))
> +		mc34708_add_subdevice(mc_pmic, "%s-adc");
> +
> +
> +	if (pdata && pdata->flags & MC34708_USE_REGULATOR) {
> +		struct mc34708_regulator_platform_data regulator_pdata = {
> +			.num_regulators = pdata->num_regulators,
> +			.regulators = pdata->regulators,
> +		};
> +
> +		mc34708_add_subdevice_pdata(mc_pmic, "%s-regulator",
> +					    &regulator_pdata,
> +					    sizeof(regulator_pdata));
> +	} else if (of_find_node_by_name(np, "regulators")) {
> +		mc34708_add_subdevice(mc_pmic, "%s-regulator");
> +	}
> +
> +	if (pdata && pdata->flags & MC34708_USE_RTC)
> +		mc34708_add_subdevice(mc_pmic, "%s-rtc");
> +	else if (of_get_property(np, "fsl,mc34708-uses-rtc", NULL))
> +		mc34708_add_subdevice(mc_pmic, "%s-rtc");
> +
> +	if (pdata && pdata->flags & MC34708_USE_TOUCHSCREEN)
> +		mc34708_add_subdevice(mc_pmic, "%s-ts");
> +	else if (of_get_property(np, "fsl,mc34708-uses-ts", NULL))
> +		mc34708_add_subdevice(mc_pmic, "%s-ts");
> +
I'd prefer to have the DT setup separated from the legacy setup via
pdata, so that at some time the legacy code can be removed easily.

> +	return 0;
> +}
> +
> +static int __devexit mc34708_remove(struct i2c_client *client)
> +{
> +	struct mc34708 *mc_pmic = dev_get_drvdata(&client->dev);
> +
> +	free_irq(mc_pmic->i2c_client->irq, mc_pmic);
> +
> +	mfd_remove_devices(&client->dev);
> +
> +	kfree(mc_pmic);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mc34708_dt_ids[] = {
> +	{ .compatible = "fsl,mc34708" },
> +	{ /* sentinel */ }
> +};
> +
> +static struct i2c_driver mc34708_driver = {
> +	.id_table = mc34708_device_id,
> +	.driver = {
> +		   .name = "mc34708",
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = mc34708_dt_ids,
> +		   },
strange indentation. The pattern:
| +	.driver = {
| +		.name = "mc34708",
| +		.owner = THIS_MODULE,
| +		.of_match_table = mc34708_dt_ids,
| +	},
seems to be more common in the kernel.


Lothar WaÃmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | PascalstraÃe 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
GeschÃftsfÃhrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@xxxxxxxxxxxxxxxxxxx
___________________________________________________________
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/