[PATCH 1/2] mfd: Add Freescale's PMIC MC34708 support
Lothar Waßmann
LW at KARO-electronics.de
Fri Apr 20 02:35:51 EDT 2012
Hi,
Ying-Chun Liu (PaulLiu) writes:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu at linaro.org>
>
> 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 at freescale.com>
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu at linaro.org>
> Cc: Samuel Ortiz <sameo at linux.intel.com>
> Cc: Mark Brown <broonie at opensource.wolfsonmicro.com>
> Cc: Shawn Guo <shawn.guo at linaro.org>
> ---
> 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 at 63fc8000 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "fsl,imx53-i2c", "fsl,imx1-i2c";
> + reg = <0x63fc8000 0x4000>;
> + interrupts = <62>;
> + status = "okay";
> +
> + pmic: mc34708 at 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",
> + ®ulator_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 at karo-electronics.de
___________________________________________________________
More information about the linux-arm-kernel
mailing list