Message-ID: <alpine.DEB.2.02.1606211451550.22249@pmeerw.net>
Date:	Tue, 21 Jun 2016 15:12:29 +0200 (CEST)
From:	Peter Meerwald-Stadler <pmeerw@...erw.net>
To:	Florian Lobmaier <Florian.Lobmaier@....com>
cc:	"jic23@...nel.org" <jic23@...nel.org>,
	Elitsa Polizoeva <Elitsa.Polizoeva@....com>,
	"knaack.h@....de" <knaack.h@....de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
	Lars-Peter Clausen <lars@...afoo.de>
Subject: RE: [PATCH V1 1/1] iio: as6200: add AS6200 temperature sensor driver
 from ams AG


> Finally we got a Datasheet which we can release and share with you. 
> Thanks for the feedback, we tried to integrate most of the feedback in 
> the driver. Some proprietary API is still present as all features of the 
> chip should be reachable. If there are any features we could use from 
> the iio framework we overlooked, please let us know.

can you provide a link to the datasheet? I almost missed the attached 
document...

more comments below, there still seems to be many issues; I stopped 
reviewing at some point where I noticed that previous comments have not 
beed addressed

thanks, regards, p.

> Signed-off-by: Elitsa Polizoeva <elitsa.polizoeva@....com>
> Signed-off-by: Florian Lobmaier <florian.lobmaier@....com>
> ---
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index 21feaa4..d82e80f 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -3,6 +3,17 @@
>  #
>  menu "Temperature sensors"
>  
> +config AS6200
> +	tristate "ams AG AS6200 temperature sensor"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	  If you say yes here you get support for the AS6200 temperature
> +	  sensor from ams AG.
> +
> +	  This driver can also be built as a module. If so, the module will
> +	  be called as6200.
> +
>  config MLX90614
>  	tristate "MLX90614 contact-less infrared sensor"
>  	depends on I2C
> diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
> index 40710a8..c0c9a9a 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -2,5 +2,6 @@
>  # Makefile for industrial I/O temperature drivers
>  #
>  
> +obj-$(CONFIG_AS6200) += as6200.o
>  obj-$(CONFIG_MLX90614) += mlx90614.o
>  obj-$(CONFIG_TMP006) += tmp006.o
> diff --git a/drivers/iio/temperature/as6200.c b/drivers/iio/temperature/as6200.c
> new file mode 100644
> index 0000000..5e37227
> --- /dev/null
> +++ b/drivers/iio/temperature/as6200.c
> @@ -0,0 +1,808 @@

it is customary to put the GPL license statement here

> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/types.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +#include <linux/regmap.h>
> +
> +/* to support regmap define in version 3.12 */
> +/* supported officially in kernel 3.13 and later */

please remove these comments, not relevant for mainline and not a proper 
multi-line comment

> +#define regmap_reg_range(low, high) { .range_min = low, .range_max = high, }

as6200_ prefix needed

> +
> +static const int as6200_conv_rates[4][2] = { {4, 0}, {1, 0},
> +					{0, 250000}, {0, 125000} };
> +
> +static const char * const as6200_consec_faults[] = { "1", "2", "4", "6" };
> +
> +/* AS6200 registers */
> +#define AS6200_REG_TVAL		0x00
> +#define AS6200_REG_CONFIG	0x01
> +#define AS6200_REG_TLOW		0x02
> +#define AS6200_REG_THIGH	0x03
> +#define AS6200_MAX_REGISTER	0x03
> +
> +#define AS6200_CONFIG_DW_MASK	0x0010

could use GENMASK() or BIT() macros to make it clearer which bits are 
relevant

> +#define AS6200_CONFIG_DW_SHIFT	4
> +#define AS6200_CONFIG_AL_MASK	0x0020
> +#define AS6200_CONFIG_AL_SHIFT	5
> +#define AS6200_CONFIG_CR_MASK	0x00C0
> +#define AS6200_CONFIG_CR_SHIFT	6
> +#define AS6200_CONFIG_SM_MASK	0x0100
> +#define AS6200_CONFIG_SM_SHIFT	8
> +#define AS6200_CONFIG_IM_MASK	0x0200
> +#define AS6200_CONFIG_IM_SHIFT	9
> +#define AS6200_CONFIG_POL_MASK	0x0400
> +#define AS6200_CONFIG_POL_SHIFT	10
> +#define AS6200_CONFIG_CF_MASK	0x1800
> +#define AS6200_CONFIG_CF_SHIFT	11
> +#define AS6200_CONFIG_SS_MASK	0x8000
> +#define AS6200_CONFIG_SS_SHIFT	15
> +
> +struct as6200_data {
> +	struct mutex update_lock;

mutex used anywhere?

> +	struct i2c_client *client;
> +	int irqn;
> +	unsigned long irq_flags;

irq_flags not used

> +	int gpio;
> +	char valid; /* !=0 if following fields are valid */
> +	struct regmap *regmap;
> +};
> +

drop newline

> +
> +static const struct regmap_range as6200_readable_ranges[] = {
> +	regmap_reg_range(AS6200_REG_TVAL, AS6200_REG_THIGH),
> +};

add newline (here and below: one newline please)

> +static const struct regmap_access_table as6200_readable_table = {
> +	.yes_ranges = as6200_readable_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(as6200_readable_ranges),
> +};
> +
> +static const struct regmap_range as6200_writable_ranges[] = {
> +	regmap_reg_range(AS6200_REG_CONFIG, AS6200_REG_THIGH),
> +};
> +static const struct regmap_access_table as6200_writable_table = {
> +	.yes_ranges = as6200_writable_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(as6200_writable_ranges),
> +};
> +
> +static const struct regmap_range as6200_volatile_ranges[] = {
> +	regmap_reg_range(AS6200_REG_TVAL, AS6200_REG_TVAL),
> +};
> +static const struct regmap_access_table as6200_volatile_table = {
> +	.yes_ranges = as6200_volatile_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(as6200_volatile_ranges),
> +};
> +static const struct regmap_config as6200_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 16,
> +	.max_register = AS6200_MAX_REGISTER,
> +	.cache_type = REGCACHE_RBTREE,
> +	.rd_table = &as6200_readable_table,
> +	.wr_table = &as6200_writable_table,
> +	.volatile_table = &as6200_volatile_table,
> +};
> +
> +
> +static int as6200_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *channel, int *val,
> +			    int *val2, long mask)
> +{
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int err = 0;
> +	unsigned int reg_val = 0;
> +	int cr = 0;
> +	s32 ret = 0;

try to avoid unnecessary initialization

> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (channel->type == IIO_TEMP) {
> +			err = regmap_read(data->regmap, AS6200_REG_TVAL,
> +						&reg_val);

error checking?

> +			ret = reg_val;
> +			*val = sign_extend32(ret, 15) >> 4;
> +		} else {
> +			break;
> +		}
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_PROCESSED:
> +		if (channel->type == IIO_TEMP) {
> +			err = regmap_read(data->regmap, AS6200_REG_TVAL,
> +						&reg_val);
> +			ret = sign_extend32(ret, 15) >> 4;
> +			*val = (ret * 625) / 10000;
> +		} else {
> +			break;
> +		}
> +		return IIO_VAL_INT;

I suggest to drop _INFO_PROCESSED as _RAW + _SCALE offers equivalent 
information; _PROCESSED is not in the chan_spec?!

otherwise, the code path are rather similar and could be combined

> +	case IIO_CHAN_INFO_SCALE:
> +		if (channel->type == IIO_TEMP) {
> +			*val = 62;
> +			*val2 = 500000;
> +		} else {
> +			break;
> +		}
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_SAMP_FREQ:
> +		if (channel->type == IIO_TEMP) {
> +			dev_info(&client->dev, "get CR from reg");

debug message, please remove

> +			err = regmap_read(data->regmap, AS6200_REG_CONFIG,
> +						&reg_val);
> +			cr = (reg_val & AS6200_CONFIG_CR_MASK)
> +				>> AS6200_CONFIG_CR_SHIFT;
> +			*val = as6200_conv_rates[cr][0];
> +			*val2 = as6200_conv_rates[cr][1];
> +		} else {
> +			break;
> +		}
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		break;
> +	}
> +	return -EINVAL;
> +}
> +
> +static int as6200_write_raw(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int val,
> +			int val2,
> +			long mask)
> +{
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	int i;
> +	unsigned int config_val;
> +
> +	if (mask != IIO_CHAN_INFO_SAMP_FREQ)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(as6200_conv_rates); i++) {
> +		if ((val == as6200_conv_rates[i][0]) &&
> +			(val2 == as6200_conv_rates[i][1])) {
> +			regmap_read(data->regmap,
> +				AS6200_REG_CONFIG, &config_val);
> +			config_val &= ~AS6200_CONFIG_CR_MASK;
> +			config_val |= i << AS6200_CONFIG_CR_SHIFT;
> +
> +			return regmap_write(data->regmap, AS6200_REG_CONFIG,
> +							config_val);
> +		}
> +	}
> +	return -EINVAL;
> +}
> +
> +static irqreturn_t as6200_alert_isr(int irq, void *dev_id)
> +{
> +	struct iio_dev *indio_dev = dev_id;
> +
> +	iio_push_event(indio_dev,
> +		IIO_UNMOD_EVENT_CODE(IIO_TEMP,
> +					0,
> +					IIO_EV_TYPE_THRESH,
> +					IIO_EV_DIR_EITHER),
> +					iio_get_time_ns());
> +	return IRQ_HANDLED;
> +}
> +
> +static int as6200_setup_irq(struct iio_dev *indio_dev, bool set_gpio, u8 pol)
> +{
> +	int err;
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	struct device *dev = &data->client->dev;
> +	int gpio = -1;
> +	int irq_num;
> +	int irq_trig;
> +
> +	if (pol == 1)
> +		irq_trig = IRQF_TRIGGER_RISING;
> +	else
> +		irq_trig = IRQF_TRIGGER_FALLING;
> +
> +	if (set_gpio) {
> +		gpio = of_get_named_gpio_flags(dev->of_node,
> +			"as6200,irq-gpio", 0, 0);
> +		err = gpio_request(gpio, "as6200_irq");
> +		if (err) {
> +			dev_err(dev, "requesting gpio %d failed\n", gpio);
> +			return err;
> +		}
> +		err = gpio_direction_input(gpio);
> +		if (err) {
> +			dev_err(dev, "gpio %d cannot apply direction\n",
> +					gpio);
> +			return err;
> +		}
> +	}
> +	data->gpio = gpio;
> +	irq_num = gpio_to_irq(gpio);
> +	err = request_irq(irq_num, as6200_alert_isr, irq_trig,
> +			"as6200", dev);
> +	if (err) {
> +		dev_err(dev, "error requesting irq %d\n", err);
> +		return err;
> +	}
> +	data->irqn = irq_num;
> +
> +	return 0;
> +}
> +
> +static ssize_t as6200_show_thigh(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val = 0;
> +	int err = 0;
> +	int val;
> +
> +	err = regmap_read(data->regmap, AS6200_REG_THIGH, &reg_val);
> +	reg_val = reg_val >> 4;
> +	val = (625 * reg_val) / 10000;
> +	return sprintf(buf, "%d%cC\n", val, (unsigned char)(248));
> +}
> +
> +static ssize_t as6200_show_tlow(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val = 0x00;
> +	int err = 0;
> +	int val;
> +
> +	err = regmap_read(data->regmap, AS6200_REG_TLOW, &reg_val);
> +	reg_val = reg_val >> 4;
> +	val = (625 * reg_val) / 10000;
> +	return sprintf(buf, "%d%cC\n", val, (unsigned char)(248));
> +}
> +
> +static ssize_t as6200_show_thigh_reg(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val = 0;
> +	int err = 0;
> +
> +	err = regmap_read(data->regmap, AS6200_REG_THIGH, &reg_val);
> +	return sprintf(buf, "%hX\n", reg_val);
> +}
> +
> +static ssize_t as6200_show_tlow_reg(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val = 0x00;
> +	int err = 0;
> +
> +	err = regmap_read(data->regmap, AS6200_REG_TLOW, &reg_val);
> +	return sprintf(buf, "%hX\n", reg_val);
> +}
> +
> +static ssize_t as6200_show_config(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val = 0x00;
> +	int err = 0;
> +
> +	err = regmap_read(data->regmap, AS6200_REG_CONFIG, &reg_val);
> +	return sprintf(buf, "%hX\n", reg_val);
> +}
> +
> +static ssize_t as6200_show_al(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val = 0x00;
> +	int err = 0;
> +	u16 pol = 0;
> +
> +	err = regmap_read(data->regmap, AS6200_REG_CONFIG, &reg_val);
> +	pol = (reg_val & AS6200_CONFIG_POL_MASK) >> AS6200_CONFIG_POL_SHIFT;
> +	reg_val = (reg_val & AS6200_CONFIG_AL_MASK) >> AS6200_CONFIG_AL_SHIFT;
> +	if (pol == 1) {
> +		if (reg_val == 0)
> +			return sprintf(buf, "off\n");
> +		else if (reg_val == 1)
> +			return sprintf(buf, "on\n");
> +		else
> +			return sprintf(buf, "invalid\n");
> +	} else if (pol == 0) {
> +		if (reg_val == 0)
> +			return sprintf(buf, "on\n");
> +		else if (reg_val == 1)
> +			return sprintf(buf, "off\n");
> +		else
> +			return sprintf(buf, "invalid\n");
> +	} else
> +		return sprintf(buf, "invalid\n");
> +}
> +
> +static ssize_t as6200_show_sm(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val = 0x00;
> +	int err = 0;
> +
> +	err = regmap_read(data->regmap, AS6200_REG_CONFIG, &reg_val);
> +	return sprintf(buf, "%hX\n", (reg_val & AS6200_CONFIG_SM_MASK)
> +					>> AS6200_CONFIG_SM_SHIFT);
> +}
> +
> +static ssize_t as6200_show_im(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val = 0x00;
> +	int err = 0;
> +
> +	err = regmap_read(data->regmap, AS6200_REG_CONFIG, &reg_val);
> +	return sprintf(buf, "%hX\n", (reg_val & AS6200_CONFIG_IM_MASK)
> +					>> AS6200_CONFIG_IM_SHIFT);
> +}
> +
> +static ssize_t as6200_show_pol(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val = 0x00;
> +	int err = 0;
> +
> +	err = regmap_read(data->regmap, AS6200_REG_CONFIG, &reg_val);
> +	return sprintf(buf, "%hX\n", (reg_val & AS6200_CONFIG_POL_MASK)
> +					 >> AS6200_CONFIG_POL_SHIFT);
> +}
> +
> +static ssize_t as6200_show_cf(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val = 0x00;
> +	int err = 0;
> +
> +	err = regmap_read(data->regmap, AS6200_REG_CONFIG, &reg_val);
> +	reg_val = (reg_val & AS6200_CONFIG_CF_MASK) >> AS6200_CONFIG_CF_SHIFT;
> +	switch (reg_val) {
> +	case 0:
> +		return sprintf(buf, "1\n");
> +	case 1:
> +		return sprintf(buf, "2\n");
> +	case 2:
> +		return sprintf(buf, "4\n");
> +	case 3:
> +		return sprintf(buf, "6\n");
> +	}
> +	return sprintf(buf, "invalid\n");
> +}
> +
> +static ssize_t as6200_show_ss(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	unsigned int reg_val = 0x00;
> +	int err = 0;
> +
> +	err = regmap_read(data->regmap, AS6200_REG_CONFIG, &reg_val);
> +	return sprintf(buf, "%hX\n", (reg_val & AS6200_CONFIG_SS_MASK)
> +					>> AS6200_CONFIG_SS_SHIFT);
> +}
> +
> +static ssize_t as6200_set_thigh(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	int err = 0;
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	s16 reg_val;
> +	int val;
> +
> +	err = kstrtoint(buf, 0, &val);
> +	if (err == 0) {
> +		if ((val < -40) | (val > 150))
> +			return count;
> +		val = (val * 10000) / 625;
> +		reg_val = val << 4;
> +		err = regmap_write(data->regmap, AS6200_REG_THIGH, reg_val);
> +	} else
> +		dev_info(&client->dev,
> +			"Converting value for THIGH failed, err = %hx",
> +			err);
> +	return count;
> +}
> +
> +static ssize_t as6200_set_tlow(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int err = 0;
> +	s16 reg_val;
> +	int val;
> +
> +	err = kstrtoint(buf, 0, &val);
> +	if (err == 0) {
> +		if ((val < -40) | (val > 150)) {
> +			dev_info(&client->dev,
> +				"Value for THIGH is invalid min = -40%cC, max = 150°C, val = %d°C",
> +				(unsigned char)(248), val);
> +			return count;
> +		}
> +		val = (val * 10000) / 625;
> +		reg_val = val << 4;
> +		err = regmap_write(data->regmap, AS6200_REG_TLOW, reg_val);
> +	} else
> +		dev_info(&client->dev,
> +			"Converting value for TLOW failed, err = %hx",
> +			err);
> +	return count;
> +}
> +
> +static ssize_t as6200_set_thigh_reg(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	int err = 0;
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	u16 reg_val;
> +
> +	err = kstrtou16(buf, 0, &reg_val);
> +	if (err == 0)
> +		err = regmap_write(data->regmap, AS6200_REG_THIGH, reg_val);
> +	else
> +		dev_info(&client->dev,
> +			"Converting value for THIGH failed, err = %hx",
> +			err);
> +	return count;
> +}
> +
> +static ssize_t as6200_set_tlow_reg(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int err = 0;
> +	u16 reg_val;
> +
> +	err = kstrtou16(buf, 0, &reg_val);
> +	if (err == 0)
> +		err = regmap_write(data->regmap, AS6200_REG_TLOW, reg_val);
> +	else
> +		dev_info(&client->dev,
> +			"Converting value for TLOW failed, err = %hx",
> +			err);
> +	return count;
> +}
> +
> +static ssize_t as6200_set_configreg(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int err = 0;
> +	u16 reg_val;
> +
> +	err = kstrtou16(buf, 0, &reg_val);
> +	if (err == 0)
> +		err = regmap_write(data->regmap, AS6200_REG_CONFIG, reg_val);
> +	else
> +		dev_info(&client->dev,
> +			"Converting value for CONFIG failed, err = %hx",
> +			err);
> +	return count;
> +}
> +
> +
> +static ssize_t as6200_set_sm(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int err;
> +	u16 val;
> +
> +	err = kstrtou16(buf, 0, &val);
> +	if (err == 0) {
> +		err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> +			AS6200_CONFIG_SM_MASK, val << AS6200_CONFIG_SM_SHIFT);
> +	} else
> +		dev_info(&client->dev,
> +			"Converting value for SM field failed, err = %hx",
> +			err);
> +	return count;
> +}
> +
> +static ssize_t as6200_set_im(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int err;
> +	u16 val;
> +
> +	err = kstrtou16(buf, 0, &val);
> +	if (err == 0) {
> +		err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> +			AS6200_CONFIG_IM_MASK, val << AS6200_CONFIG_IM_SHIFT);
> +	} else
> +		dev_info(&client->dev,
> +			"Converting value for IM field failed, err = %hx",
> +			err);
> +	return count;
> +}
> +
> +static ssize_t as6200_set_pol(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int err;
> +	int irq_num = data->irqn;
> +	u16 val;
> +
> +	err = kstrtou16(buf, 0, &val);
> +	if (err == 0) {
> +		free_irq(irq_num, dev);
> +		err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> +			AS6200_CONFIG_POL_MASK,
> +			val << AS6200_CONFIG_POL_SHIFT);
> +		as6200_setup_irq(indio_dev, false, val);
> +	} else
> +		dev_info(&client->dev,
> +			"Converting value for POL field failed, err = %hx",
> +			err);
> +	return count;
> +}
> +
> +static ssize_t as6200_set_cf(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int err;
> +	u16 val;
> +
> +	err = kstrtou16(buf, 0, &val);
> +	if (err == 0) {
> +		switch (val) {
> +		case 1:
> +		case 2:
> +		case 4:
> +		case 6:
> +			err = regmap_update_bits(data->regmap,
> +				AS6200_REG_CONFIG, AS6200_CONFIG_CF_MASK,
> +				val << AS6200_CONFIG_CF_SHIFT);
> +			break;
> +		default:
> +			dev_info(&client->dev,
> +				"Value for CF field invalid, val = %hx", val);
> +			return count;
> +		}
> +	} else
> +		dev_info(&client->dev,
> +			"Converting value for CF field failed, err = %hx",
> +			err);
> +	return count;
> +}
> +
> +static ssize_t as6200_set_ss(struct device *dev,
> +	struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct as6200_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int err;
> +	u16 val;
> +
> +	err = kstrtou16(buf, 0, &val);
> +	if (err == 0) {
> +		err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> +			AS6200_CONFIG_SS_MASK, val << AS6200_CONFIG_SS_SHIFT);
> +	} else
> +		dev_info(&client->dev,
> +			"Converting value for SS field failed, err = %hx",
> +			err);
> +	return count;
> +}
> +
> +static IIO_DEVICE_ATTR(thigh, S_IWUSR | S_IRUGO, as6200_show_thigh,
> +	as6200_set_thigh, 0);
> +static IIO_DEVICE_ATTR(tlow, S_IWUSR | S_IRUGO, as6200_show_tlow,
> +	as6200_set_tlow, 0);
> +static IIO_DEVICE_ATTR(thigh_reg, S_IWUSR | S_IRUGO, as6200_show_thigh_reg,
> +	as6200_set_thigh_reg, 0);
> +static IIO_DEVICE_ATTR(tlow_reg, S_IWUSR | S_IRUGO, as6200_show_tlow_reg,
> +	as6200_set_tlow_reg, 0);
> +static IIO_DEVICE_ATTR(config, S_IWUSR | S_IRUGO, as6200_show_config,
> +	as6200_set_configreg, 0);
> +static IIO_DEVICE_ATTR(al, S_IRUGO, as6200_show_al, NULL, 0);
> +static IIO_DEVICE_ATTR(sm, S_IWUSR | S_IRUGO, as6200_show_sm,
> +	as6200_set_sm, 0);
> +static IIO_DEVICE_ATTR(im, S_IWUSR | S_IRUGO, as6200_show_im,
> +	as6200_set_im, 0);
> +static IIO_DEVICE_ATTR(pol, S_IWUSR | S_IRUGO, as6200_show_pol,
> +	as6200_set_pol, 0);
> +static IIO_DEVICE_ATTR(cf, S_IWUSR | S_IRUGO, as6200_show_cf,
> +	as6200_set_cf, 0);
> +static IIO_DEVICE_ATTR(ss, S_IWUSR | S_IRUGO, as6200_show_ss,
> +	as6200_set_ss, 0);
> +
> +static IIO_CONST_ATTR(sampling_frequency_available, "4 1 0.25 0.125");
> +
> +static struct attribute *as6200_attrs[] = {
> +	&iio_dev_attr_thigh.dev_attr.attr,
> +	&iio_dev_attr_tlow.dev_attr.attr,
> +	&iio_dev_attr_thigh_reg.dev_attr.attr,
> +	&iio_dev_attr_tlow_reg.dev_attr.attr,
> +	&iio_dev_attr_config.dev_attr.attr,
> +	&iio_dev_attr_al.dev_attr.attr,
> +	&iio_dev_attr_sm.dev_attr.attr,
> +	&iio_dev_attr_im.dev_attr.attr,
> +	&iio_dev_attr_pol.dev_attr.attr,
> +	&iio_dev_attr_cf.dev_attr.attr,
> +	&iio_dev_attr_ss.dev_attr.attr,
> +	&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group as6200_attr_group = {
> +	.attrs = as6200_attrs,
> +};
> +
> +static const struct iio_chan_spec as6200_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +	}
> +};
> +
> +static const struct iio_info as6200_info = {
> +	.read_raw = as6200_read_raw,
> +	.write_raw = as6200_write_raw,
> +	.attrs = &as6200_attr_group,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int as6200_i2c_of_probe(struct i2c_client *i2c,
> +				struct as6200_data *as6200)
> +{
> +	struct device_node *np = i2c->dev.of_node;
> +	struct irq_data *irq_data;
> +
> +	if (!np) {
> +		dev_err(&i2c->dev, "Device Tree not found\n");
> +		return -EINVAL;
> +	}
> +
> +	irq_data = irq_get_irq_data(i2c->irq);

what is this good for?

> +	if (!irq_data) {
> +		dev_err(&i2c->dev, "Invalid IRQ: %d\n", i2c->irq);
> +		return -EINVAL;
> +	}
> +
> +	as6200->irq_flags = irqd_get_trigger_type(irq_data);
> +	dev_dbg(&i2c->dev, "IRQ flags are 0x%08lx\n", as6200->irq_flags);
> +	return 0;
> +}
> +
> +static int as6200_i2c_probe(struct i2c_client *client,
> +	const struct i2c_device_id *id)
> +{
> +	struct iio_dev *indio_dev;
> +	struct as6200_data *data;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +
> +	ret = as6200_i2c_of_probe(client, data);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->regmap = devm_regmap_init_i2c(client, &as6200_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		ret = PTR_ERR(data->regmap);
> +		dev_err(&client->dev, "regmap init failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->name = id->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &as6200_info;
> +
> +	indio_dev->channels = as6200_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(as6200_channels);
> +
> +	mutex_init(&data->update_lock);
> +	ret = as6200_setup_irq(indio_dev, true, 0);
> +

no newline here

> +	if (ret < 0)
> +		goto cleanup_gpio_irq;
> +
> +	return iio_device_register(indio_dev);
> +
> +cleanup_gpio_irq:
> +	free_irq(data->irqn, &client->dev);
> +	gpio_free(data->gpio);
> +
> +	return ret;
> +}
> +
> +static int as6200_i2c_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev;
> +	struct as6200_data *data;
> +
> +	indio_dev = i2c_get_clientdata(client);
> +	data = iio_priv(indio_dev);

most drivers do that via initialization, not assignment for conciseness

> +
> +	iio_device_unregister(indio_dev);
> +	free_irq(data->irqn, &client->dev);
> +	gpio_free(data->gpio);
> +	return 0;
> +}
> +
> +static const struct of_device_id as6200_of_match[] = {
> +	{ .compatible = "ams,as6200", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, as6200_of_match);
> +
> +
> +static const struct i2c_device_id as6200_i2c_id[] = {
> +	{ "as6200", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, as6200_id);
> +
> +static struct i2c_driver as6200_i2c_driver = {
> +	.driver = {
> +		.name = "as6200",
> +		.owner = THIS_MODULE,
> +		.of_match_table = as6200_of_match,
> +	},
> +	.probe = as6200_i2c_probe,
> +	.remove = as6200_i2c_remove,
> +	.id_table = as6200_i2c_id,
> +};
> +
> +module_i2c_driver(as6200_i2c_driver);
> +
> +MODULE_DESCRIPTION("ams AS6200 temperature sensor");
> +MODULE_AUTHOR("Elitsa Polizoeva <elitsa.polizoeva@....com>");
> +MODULE_AUTHOR("Florian Lobmaier <florian.lobmaier@....com>");
> +MODULE_LICENSE("GPL");
> 
> -----Original Message-----
> From: Lars-Peter Clausen [mailto:lars@...afoo.de] 
> Sent: Dienstag, 10. November 2015 12:52
> To: Florian Lobmaier <Florian.Lobmaier@....com>; jic23@...nel.org
> Cc: Elitsa Polizoeva <Elitsa.Polizoeva@....com>; knaack.h@....de; pmeerw@...erw.net; linux-kernel@...r.kernel.org; linux-iio@...r.kernel.org
> Subject: Re: [PATCH V1 1/1] iio: as6200: add AS6200 temperature sensor driver from ams AG
> 
> On 11/10/2015 10:38 AM, Florian Lobmaier wrote:
> > The AS6200 is a compact temperature sensor chip with I2C interface.
> > 
> > Add a driver to support the AS6200 temperature sensor.
> > 
> > Signed-off-by: Elitsa Polizoeva <elitsa.polizoeva@....com>
> > Signed-off-by: Florian Lobmaier <florian.lobmaier@....com>
> 
> Hi,
> 
> Thanks for the patch.
> 
> As Peter already said this patch introduces a lot of custom ABI, none of which is documented and most of which is probably not acceptable since. The whole idea of a framework is that you expose the capabilities of a device through a standard device independent ABI, this allows to write generic applications and libraries that can manage the devices through this ABI.
> This allows to leverage existing developments rather than starting from scratch each time. If your device uses a complete custom ABI there is not much point of having a driver in the first place since any application needs device specific knowledge anyway to talk to the driver, in this case you could just directly implement the I2C communication in the application.
> 
> Please also consider reading and following Documentation/CodingStyle
> 
> [...]
> > +static int as6200_read_reg(struct i2c_client *client,
> > +u8 reg_num, u16 *reg_val)
> > +{
> > +int err = 0;
> > +char tx_buf[1];
> > +char rx_buf[2];
> > +
> > +if ((reg_num >= 0) & (reg_num <= 3)) { tx_buf[0] = reg_num; err = 
> > +i2c_master_send(client, tx_buf, 1); if (err == 1) err = 
> > +i2c_master_recv(client, rx_buf, 2);
> 
> This is not thread safe. Another thread could interrupt between
> i2c_master_send() and i2c_master_recv() and cause undefined behavior. Use
> i2c_smbus_read_word_swapped() which makes sure that the I2C communication happens atomically.
> 
> > +if (err == 2) {
> > +*reg_val = rx_buf[0];
> > +*reg_val = *reg_val << 8;
> > +*reg_val = *reg_val | rx_buf[1];
> > +}
> > +return err;
> > +} else {
> > +return -EINVAL;
> > +}
> > +}
> [...]
> > +
> > +static irqreturn_t alert_isr(int irq, void *dev_id) { 
> > +dev_warn(dev_id, "Temperature outside of limits!");
> 
> Please use IIO threshold events for this. Such out-of-band communication is really not acceptable.
> 
> > +return IRQ_HANDLED;
> > +}
> > +
> > +static int setupIRQ(struct iio_dev *indio_dev, bool set_gpio, u8 pol) 
> > +{ int err; struct as6200_data *data = iio_priv(indio_dev); struct 
> > +device *dev = &data->client->dev; int gpio = -1; int irq_num; int 
> > +irq_trig;
> > +
> > +if (pol == 1)
> > +irq_trig = IRQF_TRIGGER_RISING;
> > +else
> > +irq_trig = IRQF_TRIGGER_FALLING;
> > +
> > +if (set_gpio) {
> > +gpio = of_get_named_gpio_flags(dev->of_node,
> > +"as6200,irq-gpio", 0, 0);
> > +err = gpio_request(gpio, "as6200_irq"); if (err) { dev_info(dev, "%s: 
> > +requesting gpio %d failed\n", as6200_id[0].name, gpio); return err; } 
> > +err = gpio_direction_input(gpio); if (err) { dev_info(dev, "%s: gpio 
> > +%d cannot apply direction\n", as6200_id[0].name, gpio); return err; } 
> > +} irq_num = gpio_to_irq(gpio); dev_info(dev, "%s: registering for IRQ 
> > +%d\n", as6200_id[0].name, irq_num);
> 
> Please drop all these dev_info(). That's just noise in the boot log, imagine every driver did this you wouldn't be able to spot the critical information.
> 
> > +err = request_irq(irq_num, alert_isr, irq_trig, as6200_id[0].name, 
> > +dev);
> 
> Don't do all the GPIO translation. This pin is only used as a interrupt, so specify it directly as an interrupt and use it that way as well.
> 
> > +if (err) {
> > +dev_info(dev, "%s: error requesting irq %d\n", as6200_id[0].name, 
> > +err);
> 
> For errors use dev_err. Also the as6200_id[0].name is redundant since dev_info/dev_err already prefixes the message with the device name.
> 
> > +return err;
> > +}
> > +dev_info(dev, "%s: registered for IRQ %d\n", as6200_id[0].name, 
> > +irq_num); mutex_lock(&data->update_lock);
> > +data->irqn = irq_num;
> > +mutex_unlock(&data->update_lock);
> 
> What exactly is protect by that mutex here?
> 
> > +
> > +return 0;
> > +}
> > +
> [...]
> > +if (err == 0) {
> > +if ((val < -40) | (val > 150)) {
> > +dev_info(&client->dev,
> > +"Value for THIGH is invalid min = -40%cC, max = 150°C, val = %d°C", 
> > +(unsigned char)(248), val);
> 
> Please no out-of-band error reporting.
> 
> > +return count;
> > +}
> > +val = (val * 10000) / 625;
> > +reg_val = val << 4;
> [...]
> > +static int as6200_probe(struct i2c_client *client, const struct 
> > +i2c_device_id *id) { struct iio_dev *indio_dev = NULL; struct 
> > +as6200_data *data = NULL;
> 
> No need to initialize those here with dummy, this will just hide warnings in case you forgot to initialize them with actual data.
> 
> > +
> > +indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +
> > +if (!indio_dev)
> > +return -ENOMEM;
> > +
> > +data = iio_priv(indio_dev);
> > +i2c_set_clientdata(client, indio_dev);
> > +data->client = client;
> > +
> > +indio_dev->dev.parent = &client->dev; indio_dev->name = 
> > +dev_name(&client->dev);
> 
> use id->name for this. dev_name() contains things like the I2C bus and device address, etc. Whereas the IIO device name should describe the type of device.
> 
> > +indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->info = &as6200_info;
> > +
> > +indio_dev->channels = as6200_channels; indio_dev->num_channels = 
> > +ARRAY_SIZE(as6200_channels);
> > +
> > +initClientData(data);
> > +mutex_init(&data->update_lock);
> > +setupIRQ(indio_dev, true, 0);
> > +
> > +return iio_device_register(indio_dev);
> 
> Error handling is missing here, you need to free the resources that were acquired in case of an error.
> 
> > +}
> > +
> [...]
> 
> 
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Powered by blists - more mailing lists