Message-ID: <20170904154623.ba6rhqn6cijqfmbk@dell>
Date:   Mon, 4 Sep 2017 16:46:23 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     s.abhisit@...il.com
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] lmp92001: mfd: Add support TI LMP92001

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

> From: Abhisit Sangjan <s.abhisit@...il.com>
> 
> 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@...il.com>
> ---
>  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@...il.com>
> + *
> + * 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@...il.com>
> + *
> + * 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@...il.com>
> + *
> + * 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@...il.com>");
> +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@...il.com>
> + *
> + * 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@...il.com>
> + *
> + * 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

Powered by blists - more mailing lists