Re: [PATCH 1/5] lmp92001: mfd: Add support TI LMP92001

From: Lee Jones
Date: Mon Sep 04 2017 - 11:46:36 EST

Next message: Joe Perches: "Re: [PATCH] checkpatch: simplify the output of --list-types"
Previous message: Andreas FÃrber: "Re: [PATCH v2] clk: mb86s7x: Drop non-building driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, 31 Aug 2017, s.abhisit@xxxxxxxxx wrote:

> From: Abhisit Sangjan <s.abhisit@xxxxxxxxx>
>
> TI LMP92001 Analog System Monitor and Controller
>
> 8-bit GPIOs.
> 12 DACs with 12-bit resolution.
>
> The GPIOs and DACs are shared port function with Cy function pin to
> take control the pin suddenly from external hardware.
> DAC's referance voltage selectable for Internal/External.
>
> 16 + 1 ADCs with 12-bit resolution.
>
> Built-in internal Temperature Sensor on channel 17.
> Window Comparator Function is supported on channel 1-3 and 9-11 for
> monitoring with interrupt signal (pending to implement for interrupt).
> ADC's referance voltage selectable for Internal/External.
>
> Signed-off-by: Abhisit Sangjan <s.abhisit@xxxxxxxxx>
> ---
> drivers/mfd/Kconfig | 12 ++
> drivers/mfd/Makefile | 4 +
> drivers/mfd/lmp92001-core.c | 307 +++++++++++++++++++++++++++++++++++++
> drivers/mfd/lmp92001-debug.c | 64 ++++++++
> drivers/mfd/lmp92001-i2c.c | 211 +++++++++++++++++++++++++
> include/linux/mfd/lmp92001/core.h | 135 ++++++++++++++++
> include/linux/mfd/lmp92001/debug.h | 27 ++++
> 7 files changed, 760 insertions(+)
> create mode 100644 drivers/mfd/lmp92001-core.c
> create mode 100644 drivers/mfd/lmp92001-debug.c
> create mode 100644 drivers/mfd/lmp92001-i2c.c
> create mode 100644 include/linux/mfd/lmp92001/core.h
> create mode 100644 include/linux/mfd/lmp92001/debug.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 94ad2c1c3d90..a20389a30cf8 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1528,6 +1528,18 @@ config MFD_LM3533
> additional drivers must be enabled in order to use the LED,
> backlight or ambient-light-sensor functionality of the device.
>
> +config MFD_LMP92001
> + tristate "TI LMP92001 Analog System Monitor and Controller"
> + depends on I2C
> + select MFD_CORE
> + select REGMAP_I2C
> + help
> + Say yes here to enable support for TI LMP92001 Analog System
> + Monitor and Controller
> +
> + This driver provide support for 16 ADC, 12 DAC, Voltage Reference,
> + Analog Temperature Sensor and 8-bit GPIO Port.
> +
> config MFD_TIMBERDALE
> tristate "Timberdale FPGA"
> select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 080793b3fd0e..20d2e65d503a 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -199,6 +199,10 @@ obj-$(CONFIG_MFD_RN5T618) += rn5t618.o
> obj-$(CONFIG_MFD_SEC_CORE) += sec-core.o sec-irq.o
> obj-$(CONFIG_MFD_SYSCON) += syscon.o
> obj-$(CONFIG_MFD_LM3533) += lm3533-core.o lm3533-ctrlbank.o
> +
> +lmp92001-objs := lmp92001-core.o lmp92001-i2c.o lmp92001-debug.o
> +obj-$(CONFIG_MFD_LMP92001) += lmp92001.o
> +
> obj-$(CONFIG_MFD_VEXPRESS_SYSREG) += vexpress-sysreg.o
> obj-$(CONFIG_MFD_RETU) += retu-mfd.o
> obj-$(CONFIG_MFD_AS3711) += as3711.o
> diff --git a/drivers/mfd/lmp92001-core.c b/drivers/mfd/lmp92001-core.c
> new file mode 100644
> index 000000000000..5ee315ce61bb
> --- /dev/null
> +++ b/drivers/mfd/lmp92001-core.c
> @@ -0,0 +1,307 @@
> +/*
> + * lmp92001-core.c - Device access for TI LMP92001

Please remove the filename from the header comment.

> + * Copyright 2016-2017 Celestica Ltd.
> + *
> + * Author: Abhisit Sangjan <s.abhisit@xxxxxxxxx>
> + *
> + * Inspired by wm831x driver.
> + *
> + * 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.

Can you use the short licence?

> + */
> +
> +#include <linux/mfd/lmp92001/core.h>
> +#include <linux/mfd/lmp92001/debug.h>

I don't see a requirement to create a new directory for this driver.

> +static const struct mfd_cell lmp92001_devs[] = {
> + {
> + .name = "lmp92001-gpio",
> + .of_compatible = "ti,lmp92001-gpio",
> + },
> + {

Place these two lines on a single one.

> + .name = "lmp92001-adc",
> + .of_compatible = "ti,lmp92001-adc",
> + },
> + {

... and here.

> + .name = "lmp92001-dac",
> + .of_compatible = "ti,lmp92001-dac",
> + },
> +};
> +
> +static struct reg_default lmp92001_defaults[] = {
> + { LMP92001_SGEN, 0x40 },
> + { LMP92001_SHIL, 0x00 },
> + { LMP92001_SLOL, 0x00 },
> + { LMP92001_CGEN, 0x00 },
> + { LMP92001_CDAC, 0x03 },
> + { LMP92001_CGPO, 0xFF },
> + { LMP92001_CINH, 0x00 },
> + { LMP92001_CINL, 0x00 },
> + { LMP92001_CAD1, 0x00 },
> + { LMP92001_CAD2, 0x00 },
> + { LMP92001_CAD3, 0x00 },
> + { LMP92001_CTRIG, 0x00 },
> + { LMP92001_CREF, 0x07 },
> + { LMP92001_ADC1, 0x0000 },
> + { LMP92001_ADC2, 0x0000 },
> + { LMP92001_ADC3, 0x0000 },
> + { LMP92001_ADC4, 0x0000 },
> + { LMP92001_ADC5, 0x0000 },
> + { LMP92001_ADC6, 0x0000 },
> + { LMP92001_ADC7, 0x0000 },
> + { LMP92001_ADC8, 0x0000 },
> + { LMP92001_ADC9, 0x0000 },
> + { LMP92001_ADC10, 0x0000 },
> + { LMP92001_ADC11, 0x0000 },
> + { LMP92001_ADC12, 0x0000 },
> + { LMP92001_ADC13, 0x0000 },
> + { LMP92001_ADC14, 0x0000 },
> + { LMP92001_ADC15, 0x0000 },
> + { LMP92001_ADC16, 0x0000 },
> + { LMP92001_LIH1, 0x0FFF },
> + { LMP92001_LIH2, 0x0FFF },
> + { LMP92001_LIH3, 0x0FFF },
> + { LMP92001_LIH9, 0x0FFF },
> + { LMP92001_LIH10, 0x0FFF },
> + { LMP92001_LIH11, 0x0FFF },
> + { LMP92001_LIL1, 0x0000 },
> + { LMP92001_LIL2, 0x0000 },
> + { LMP92001_LIL3, 0x0000 },
> + { LMP92001_LIL9, 0x0000 },
> + { LMP92001_LIL10, 0x0000 },
> + { LMP92001_LIL11, 0x0000 },
> + { LMP92001_DAC1, 0x0000 },
> + { LMP92001_DAC2, 0x0000 },
> + { LMP92001_DAC3, 0x0000 },
> + { LMP92001_DAC4, 0x0000 },
> + { LMP92001_DAC5, 0x0000 },
> + { LMP92001_DAC6, 0x0000 },
> + { LMP92001_DAC7, 0x0000 },
> + { LMP92001_DAC8, 0x0000 },
> + { LMP92001_DAC9, 0x0000 },
> + { LMP92001_DAC10, 0x0000 },
> + { LMP92001_DAC11, 0x0000 },
> + { LMP92001_DAC12, 0x0000 },
> + { LMP92001_DALL, 0x0000 },
> +};
> +
> +static bool lmp92001_reg_readable(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case LMP92001_ID:
> + case LMP92001_VER:
> + case LMP92001_SGEN:
> + case LMP92001_SGPI:
> + case LMP92001_SHIL:
> + case LMP92001_SLOL:
> + case LMP92001_CGEN:
> + case LMP92001_CDAC:
> + case LMP92001_CGPO:
> + case LMP92001_CINH:
> + case LMP92001_CINL:
> + case LMP92001_CAD1:
> + case LMP92001_CAD2:
> + case LMP92001_CAD3:
> + case LMP92001_ADC1:
> + case LMP92001_ADC2:
> + case LMP92001_ADC3:
> + case LMP92001_ADC4:
> + case LMP92001_ADC5:
> + case LMP92001_ADC6:
> + case LMP92001_ADC7:
> + case LMP92001_ADC8:
> + case LMP92001_ADC9:
> + case LMP92001_ADC10:
> + case LMP92001_ADC11:
> + case LMP92001_ADC12:
> + case LMP92001_ADC13:
> + case LMP92001_ADC14:
> + case LMP92001_ADC15:
> + case LMP92001_ADC16:
> + case LMP92001_ADC17:
> + case LMP92001_LIH1:
> + case LMP92001_LIH2:
> + case LMP92001_LIH3:
> + case LMP92001_LIH9:
> + case LMP92001_LIH10:
> + case LMP92001_LIH11:
> + case LMP92001_LIL1:
> + case LMP92001_LIL2:
> + case LMP92001_LIL3:
> + case LMP92001_LIL9:
> + case LMP92001_LIL10:
> + case LMP92001_LIL11:
> + case LMP92001_CREF:
> + case LMP92001_DAC1:
> + case LMP92001_DAC2:
> + case LMP92001_DAC3:
> + case LMP92001_DAC4:
> + case LMP92001_DAC5:
> + case LMP92001_DAC6:
> + case LMP92001_DAC7:
> + case LMP92001_DAC8:
> + case LMP92001_DAC9:
> + case LMP92001_DAC10:
> + case LMP92001_DAC11:
> + case LMP92001_DAC12:
> + case LMP92001_BLK0:
> + case LMP92001_BLK1:
> + case LMP92001_BLK2:
> + case LMP92001_BLK3:
> + case LMP92001_BLK4:
> + case LMP92001_BLK5:

Any reason why you're not using ranges?

Listing every single register is a drag.

> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static bool lmp92001_reg_writeable(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case LMP92001_CGEN:
> + case LMP92001_CDAC:
> + case LMP92001_CGPO:
> + case LMP92001_CINH:
> + case LMP92001_CINL:
> + case LMP92001_CAD1:
> + case LMP92001_CAD2:
> + case LMP92001_CAD3:
> + case LMP92001_CTRIG:
> + case LMP92001_LIH1:
> + case LMP92001_LIH2:
> + case LMP92001_LIH3:
> + case LMP92001_LIH9:
> + case LMP92001_LIH10:
> + case LMP92001_LIH11:
> + case LMP92001_LIL1:
> + case LMP92001_LIL2:
> + case LMP92001_LIL3:
> + case LMP92001_LIL9:
> + case LMP92001_LIL10:
> + case LMP92001_LIL11:
> + case LMP92001_CREF:
> + case LMP92001_DAC1:
> + case LMP92001_DAC2:
> + case LMP92001_DAC3:
> + case LMP92001_DAC4:
> + case LMP92001_DAC5:
> + case LMP92001_DAC6:
> + case LMP92001_DAC7:
> + case LMP92001_DAC8:
> + case LMP92001_DAC9:
> + case LMP92001_DAC10:
> + case LMP92001_DAC11:
> + case LMP92001_DAC12:
> + case LMP92001_DALL:
> + case LMP92001_BLK0:
> + case LMP92001_BLK1:
> + case LMP92001_BLK4:
> + case LMP92001_BLK5:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static bool lmp92001_reg_volatile(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case LMP92001_SGEN:
> + case LMP92001_SGPI:
> + case LMP92001_SHIL:
> + case LMP92001_SLOL:
> + case LMP92001_CGEN:
> + case LMP92001_ADC1:
> + case LMP92001_ADC2:
> + case LMP92001_ADC3:
> + case LMP92001_ADC4:
> + case LMP92001_ADC5:
> + case LMP92001_ADC6:
> + case LMP92001_ADC7:
> + case LMP92001_ADC8:
> + case LMP92001_ADC9:
> + case LMP92001_ADC10:
> + case LMP92001_ADC11:
> + case LMP92001_ADC12:
> + case LMP92001_ADC13:
> + case LMP92001_ADC14:
> + case LMP92001_ADC15:
> + case LMP92001_ADC16:
> + case LMP92001_ADC17:
> + case LMP92001_BLK2:
> + case LMP92001_BLK3:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +struct regmap_config lmp92001_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 16,
> +

Remove the line breaks in this struct.

> + .cache_type = REGCACHE_RBTREE,
> +
> + .reg_defaults = lmp92001_defaults,
> + .num_reg_defaults = ARRAY_SIZE(lmp92001_defaults),
> +
> + .max_register = LMP92001_BLK5,
> + .readable_reg = lmp92001_reg_readable,
> + .writeable_reg = lmp92001_reg_writeable,
> + .volatile_reg = lmp92001_reg_volatile,
> +};
> +
> +int lmp92001_device_init(struct lmp92001 *lmp92001, unsigned long id, int irq)

Static?

> +{
> + int ret;
> + unsigned int comid, ver;
> +
> + dev_set_drvdata(lmp92001->dev, lmp92001);
> +
> + ret = regmap_read(lmp92001->regmap, LMP92001_ID, &comid);
> + if (ret < 0) {
> + dev_err(lmp92001->dev, "failed to read Company ID: %d\n", ret);

Company ID? That's a new one on me. What is this?

> + goto exit;
> + }
> +
> + ret = regmap_read(lmp92001->regmap, LMP92001_VER, &ver);
> + if (ret < 0) {
> + dev_err(lmp92001->dev, "failed to read Version: %d\n", ret);
> + goto exit;
> + }
> +
> + dev_info(lmp92001->dev, "Company ID 0x%.2x, Version 0x%.2x\n",
> + comid, ver);
> +
> + ret = mfd_add_devices(lmp92001->dev, PLATFORM_DEVID_AUTO,
> + lmp92001_devs, ARRAY_SIZE(lmp92001_devs),
> + NULL, 0, NULL);
> + if (ret != 0) {

if (ret) {

> + dev_err(lmp92001->dev, "failed to add children\n");

Try to be user friendly.

"Failed to register child devices" is probably more helpful.

> + goto exit;

Just return here.

> + }
> +
> + ret = lmp92001_debug_init(lmp92001);
> + if (ret < 0) {

if (ret) {

> + dev_err(lmp92001->dev, "failed to initial debug fs.\n");

This makes no sense.

"Failed to initialise DebugFS"

Why does this function even exist?

> + goto exit;

Drop this.

> + }
> +
> +exit:

Drop this.

> + return ret;
> +}
> +
> +void lmp92001_device_exit(struct lmp92001 *lmp92001)
> +{
> + lmp92001_debug_exit(lmp92001);
> + mfd_remove_devices(lmp92001->dev);

Any reason not to use devm_*?

> +}
> diff --git a/drivers/mfd/lmp92001-debug.c b/drivers/mfd/lmp92001-debug.c
> new file mode 100644
> index 000000000000..b1746dc810b3
> --- /dev/null
> +++ b/drivers/mfd/lmp92001-debug.c

This file probably isn't needed at all.

> @@ -0,0 +1,64 @@
> +/*
> + * lmp92001-debug.c - Debug file system for TI LMP92001

Same here.

> + * Copyright 2016-2017 Celestica Ltd.
> + *
> + * Author: Abhisit Sangjan <s.abhisit@xxxxxxxxx>
> + *
> + * Inspired by wm831x driver.
> + *
> + * 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.

And here.

> + */
> +
> +#include <linux/kernel.h>
> +
> +#include <linux/mfd/lmp92001/core.h>
> +
> +static ssize_t lmp92001_id_ver_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct lmp92001 *lmp92001 = dev_get_drvdata(dev);
> + int ret;
> + unsigned int comid, ver;
> +
> + ret = regmap_read(lmp92001->regmap, LMP92001_ID, &comid);
> + if (ret < 0) {
> + dev_err(lmp92001->dev, "failed to read Company ID: %d\n", ret);
> + return ret;
> + }
> +
> + ret = regmap_read(lmp92001->regmap, LMP92001_VER, &ver);
> + if (ret < 0) {
> + dev_err(lmp92001->dev, "failed to read Version: %d\n", ret);
> + return ret;
> + }

This is duplicated code. You did this once already.

> + return sprintf(buf, "Company ID 0x%02x (%d), Version 0x%02x (%d)\n",
> + comid, comid, ver, ver);
> +}
> +static DEVICE_ATTR(lmp92001_id_ver, 0444, lmp92001_id_ver_show, NULL);

Why don't you just print this to the kernel log (actually I think you
did already) and remove this function/file entirely.

> +int lmp92001_debug_init(struct lmp92001 *lmp92001)
> +{
> + int ret;
> +
> + ret = device_create_file(lmp92001->dev, &dev_attr_lmp92001_id_ver);
> + if (ret != 0)
> + dev_err(lmp92001->dev,
> + "unique ID attribute is not created: %d\n", ret);
> +
> + return ret;
> +}
> +
> +void lmp92001_debug_exit(struct lmp92001 *lmp92001)
> +{
> + device_remove_file(lmp92001->dev, &dev_attr_lmp92001_id_ver);
> +}
> diff --git a/drivers/mfd/lmp92001-i2c.c b/drivers/mfd/lmp92001-i2c.c
> new file mode 100644
> index 000000000000..e22ea88f5820
> --- /dev/null
> +++ b/drivers/mfd/lmp92001-i2c.c
> @@ -0,0 +1,211 @@
> +/*
> + * lmp92001-i2c.c - I2C access for TI LMP92001

Same here.

> + * Copyright 2016-2017 Celestica Ltd.
> + *
> + * Author: Abhisit Sangjan <s.abhisit@xxxxxxxxx>
> + *
> + * Inspired by wm831x driver.
> + *
> + * 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.

And here.

> + */
> +
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <linux/mfd/lmp92001/core.h>
> +
> +static const unsigned short lmp92001_i2c_addresses[] = {
> + 0x40, 0x42, 0x44, 0x46, 0x48, 0x4A, 0x4C, 0x4E, 0x50, I2C_CLIENT_END
> +};
> +
> +/* TODO: To read/write block access, it may need to re-ordering endianness! */

Why don't you just do this right away?

When are you realistically going to implement this?

> +static int lmp92001_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> + struct device *dev = context;
> + struct i2c_client *i2c = to_i2c_client(dev);
> + int ret;
> +
> + if (reg > 0xff)

No magic numbers. Please define the 0xff.

> + return -EINVAL;
> +
> + switch (reg) {
> + case LMP92001_ID ... LMP92001_CTRIG:
> + case LMP92001_CREF:
> + *val = ret = i2c_smbus_read_byte_data(i2c, reg);

Only assign *val if ret is not an error.

> + break;
> + case LMP92001_ADC1 ... LMP92001_LIL11:
> + case LMP92001_DAC1 ... LMP92001_DALL:
> + *val = ret = i2c_smbus_read_word_swapped(i2c, reg);
> + break;
> + case LMP92001_BLK0 ... LMP92001_BLK5:
> + ret = i2c_smbus_read_block_data(i2c, reg,
> + (u8 *)((uintptr_t)*val));
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (ret < 0)
> + return ret;
> +
> + return 0;

Remove the 4 lines above and just return ret.

> +}
> +
> +static int lmp92001_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> + struct device *dev = context;
> + struct i2c_client *i2c = to_i2c_client(dev);
> + int ret;
> +
> + if (reg > 0xff)
> + return -EINVAL;
> +
> + switch (reg) {
> + case LMP92001_ID ... LMP92001_CTRIG:
> + case LMP92001_CREF:
> + ret = i2c_smbus_write_byte_data(i2c, reg, val);
> + break;
> + case LMP92001_ADC1 ... LMP92001_LIL11:
> + case LMP92001_DAC1 ... LMP92001_DALL:
> + ret = i2c_smbus_write_word_swapped(i2c, reg, val);
> + break;
> + /* To call this function/case, must be passed val as pointer */
> + case LMP92001_BLK0:
> + case LMP92001_BLK4:
> + ret = i2c_smbus_write_block_data(i2c, reg, 24,

No magic numbers. Please define them all.

> + (u8 *)((uintptr_t)val));

Why is this type of casting required?

> + break;
> + case LMP92001_BLK1:
> + case LMP92001_BLK5:
> + ret = i2c_smbus_write_block_data(i2c, reg, 12,
> + (u8 *)((uintptr_t)val));
> + break;
> + case LMP92001_BLK2:
> + ret = i2c_smbus_write_block_data(i2c, reg, 34,
> + (u8 *)((uintptr_t)val));
> + break;
> + case LMP92001_BLK3:
> + ret = i2c_smbus_write_block_data(i2c, reg, 18,
> + (u8 *)((uintptr_t)val));
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return ret;
> +}
> +
> +static int lmp92001_i2c_probe(struct i2c_client *i2c,

^
Line break here instead.

> + const struct i2c_device_id *id)
> +{
> + struct lmp92001 *lmp92001;
> + int ret;
> +
> + lmp92001 = devm_kzalloc(&i2c->dev, sizeof(struct lmp92001), GFP_KERNEL);
> + if (!lmp92001)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(i2c, lmp92001);
> + lmp92001->dev = &i2c->dev;
> +
> + lmp92001_regmap_config.reg_read = lmp92001_reg_read;
> + lmp92001_regmap_config.reg_write = lmp92001_reg_write;
> +
> + lmp92001->regmap = devm_regmap_init(&i2c->dev, NULL, &i2c->dev,
> + &lmp92001_regmap_config);
> + if (IS_ERR(lmp92001->regmap)) {
> + ret = PTR_ERR(lmp92001->regmap);
> + dev_err(lmp92001->dev, "failed to allocate register map: %d\n",
> + ret);
> + return ret;
> + }
> +
> + return lmp92001_device_init(lmp92001, id->driver_data, i2c->irq);
> +}
> +
> +static int lmp92001_i2c_remove(struct i2c_client *i2c)
> +{
> + struct lmp92001 *lmp92001 = i2c_get_clientdata(i2c);
> +
> + lmp92001_device_exit(lmp92001);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id lmp92001_dt_ids[] = {
> + { .compatible = "ti,lmp92001", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, lmp92001_dt_ids);
> +#endif
> +
> +static const struct i2c_device_id lmp92001_i2c_ids[] = {
> + { "lmp92001" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(i2c, lmp92001_i2c_ids);
> +
> +static int lmp92001_i2c_detect(struct i2c_client *i2c,
> + struct i2c_board_info *info)
> +{
> + struct i2c_adapter *adapter = i2c->adapter;
> + s32 comid, ver;
> +
> + if (!i2c_check_functionality(adapter,
> + I2C_FUNC_SMBUS_BYTE_DATA |
> + I2C_FUNC_SMBUS_WORD_DATA |
> + I2C_FUNC_SMBUS_BLOCK_DATA))
> + return -ENODEV;
> +
> + comid = i2c_smbus_read_byte_data(i2c, LMP92001_ID);
> + ver = i2c_smbus_read_byte_data(i2c, LMP92001_VER);
> +
> + if (comid != 0x01 || ver != 0x10)

No magic numbers.

> + return -ENODEV;
> +
> + return 0;
> +}
> +
> +static struct i2c_driver lmp92001_i2c_driver = {
> + .driver = {
> + .name = "lmp92001",
> + .of_match_table = of_match_ptr(lmp92001_dt_ids),
> + },
> + .probe = lmp92001_i2c_probe,
> + .remove = lmp92001_i2c_remove,
> + .id_table = lmp92001_i2c_ids,
> + .detect = lmp92001_i2c_detect,
> + .address_list = lmp92001_i2c_addresses,
> +};
> +
> +static int __init lmp92001_i2c_init(void)
> +{
> + return i2c_add_driver(&lmp92001_i2c_driver);
> +}
> +subsys_initcall(lmp92001_i2c_init);
> +
> +static void __exit lmp92001_i2c_exit(void)
> +{
> + i2c_del_driver(&lmp92001_i2c_driver);
> +}
> +module_exit(lmp92001_i2c_exit);
> +
> +MODULE_DESCRIPTION("i2c driver for TI LMP92001");
> +MODULE_AUTHOR("Abhisit Sangjan <s.abhisit@xxxxxxxxx>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/lmp92001/core.h b/include/linux/mfd/lmp92001/core.h
> new file mode 100644
> index 000000000000..81aabf8b8e7a
> --- /dev/null
> +++ b/include/linux/mfd/lmp92001/core.h
> @@ -0,0 +1,135 @@
> +/*
> + * include/linux/mfd/lmp92001/core.h - Core interface for TI LMP92001

Same here.

> + * Copyright 2016-2017 Celestica Ltd.
> + *
> + * Author: Abhisit Sangjan <s.abhisit@xxxxxxxxx>
> + *
> + * Inspired by wm831x driver.
> + *
> + * 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.

Same here.

> + */
> +
> +#ifndef __MFD_LMP92001_CORE_H__
> +#define __MFD_LMP92001_CORE_H__
> +
> +#include <linux/device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +
> +/*
> + * Register values.
> + */
> +/* ID */
> +#define LMP92001_ID 0x0E
> +#define LMP92001_VER 0x0F
> +/* STATUS */
> +#define LMP92001_SGEN 0x10
> +#define LMP92001_SGPI 0x11
> +#define LMP92001_SHIL 0x12
> +#define LMP92001_SLOL 0x13
> +/* CONTROL */
> +#define LMP92001_CGEN 0x14
> +#define LMP92001_CDAC 0x15
> +#define LMP92001_CGPO 0x16
> +#define LMP92001_CINH 0x17
> +#define LMP92001_CINL 0x18
> +#define LMP92001_CAD1 0x19
> +#define LMP92001_CAD2 0x1A
> +#define LMP92001_CAD3 0x1B
> +#define LMP92001_CTRIG 0x1C
> +/* ADC OUTPUT DATA */
> +#define LMP92001_ADC1 0x20
> +#define LMP92001_ADC2 0x21
> +#define LMP92001_ADC3 0x22
> +#define LMP92001_ADC4 0x23
> +#define LMP92001_ADC5 0x24
> +#define LMP92001_ADC6 0x25
> +#define LMP92001_ADC7 0x26
> +#define LMP92001_ADC8 0x27
> +#define LMP92001_ADC9 0x28
> +#define LMP92001_ADC10 0x29
> +#define LMP92001_ADC11 0x2A
> +#define LMP92001_ADC12 0x2B
> +#define LMP92001_ADC13 0x2C
> +#define LMP92001_ADC14 0x2D
> +#define LMP92001_ADC15 0x2E
> +#define LMP92001_ADC16 0x2F
> +#define LMP92001_ADC17 0x30
> +/* ADC WINDOW COMPARATOR LIMITS */
> +#define LMP92001_LIH1 0x40
> +#define LMP92001_LIH2 0x41
> +#define LMP92001_LIH3 0x42
> +#define LMP92001_LIH9 0x43
> +#define LMP92001_LIH10 0x44
> +#define LMP92001_LIH11 0x45
> +#define LMP92001_LIL1 0x46
> +#define LMP92001_LIL2 0x47
> +#define LMP92001_LIL3 0x48
> +#define LMP92001_LIL9 0x49
> +#define LMP92001_LIL10 0x4A
> +#define LMP92001_LIL11 0x4B
> +/* INTERNAL REFERENCE CONTROL */
> +#define LMP92001_CREF 0x66
> +/* DAC INPUT DATA */
> +#define LMP92001_DAC1 0x80
> +#define LMP92001_DAC2 0x81
> +#define LMP92001_DAC3 0x82
> +#define LMP92001_DAC4 0x83
> +#define LMP92001_DAC5 0x84
> +#define LMP92001_DAC6 0x85
> +#define LMP92001_DAC7 0x86
> +#define LMP92001_DAC8 0x87
> +#define LMP92001_DAC9 0x88
> +#define LMP92001_DAC10 0x89
> +#define LMP92001_DAC11 0x8A
> +#define LMP92001_DAC12 0x8B
> +#define LMP92001_DALL 0x90
> +/* MEMORY MAPPED BLOCK COMMANDS */
> +#define LMP92001_BLK0 0xF0
> +#define LMP92001_BLK1 0xF1
> +#define LMP92001_BLK2 0xF2
> +#define LMP92001_BLK3 0xF3
> +#define LMP92001_BLK4 0xF4
> +#define LMP92001_BLK5 0xF5
> +
> +/*
> + * Helpful macros
> + */
> +/* DAC */
> +#define CDAC_OFF BIT(0)
> +#define CDAC_OLVL BIT(1)
> +#define CDAC_GANG BIT(2)
> +/* INT. REFERANCE ENABLE */
> +#define CREF_DEXT BIT(0)
> +#define CREF_AEXT BIT(1)
> +/* GENERAL */
> +#define CGEN_STRT BIT(0)
> +#define CGEN_LCK BIT(1)
> +#define CGEN_RST BIT(7)
> +/* STATUS: GENERNAL */
> +#define SGEN_GPI BIT(0)
> +
> +struct lmp92001 {
> + struct device *dev;
> + struct regmap *regmap;
> +
> + struct mutex adc_lock;
> + struct mutex dac_lock;
> +};
> +
> +extern struct regmap_config lmp92001_regmap_config;
> +
> +int lmp92001_device_init(struct lmp92001 *lmp92001, unsigned long id, int irq);
> +void lmp92001_device_exit(struct lmp92001 *lmp92001);
> +
> +#endif /* __MFD_LMP92001_CORE_H__ */
> diff --git a/include/linux/mfd/lmp92001/debug.h b/include/linux/mfd/lmp92001/debug.h
> new file mode 100644
> index 000000000000..e6fda7efc917
> --- /dev/null
> +++ b/include/linux/mfd/lmp92001/debug.h
> @@ -0,0 +1,27 @@
> +/*
> + * include/linux/mfd/lmp92001/debug.h - Debug interface for TI 92001
> + *
> + * Copyright 2016-2017 Celestica Ltd.
> + *
> + * Author: Abhisit Sangjan <s.abhisit@xxxxxxxxx>
> + *
> + * Inspired by wm831x driver.
> + *
> + * 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.
> + */
> +
> +#ifndef __MFD_LMP92001_DEBUG_H__
> +#define __MFD_LMP92001_DEBUG_H__
> +
> +int lmp92001_debug_init(struct lmp92001 *lmp92001);
> +void lmp92001_debug_exit(struct lmp92001 *lmp92001);
> +
> +#endif /* __MFD_LMP92001_DEBUG_H__ */

--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Next message: Joe Perches: "Re: [PATCH] checkpatch: simplify the output of --list-types"
Previous message: Andreas FÃrber: "Re: [PATCH v2] clk: mb86s7x: Drop non-building driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]