All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 2/2] iio: adc: hx711: Add IIO driver for AVIA HX711
@ 2016-12-14 16:17 ` Andreas Klinger
  0 siblings, 0 replies; 8+ messages in thread
From: Andreas Klinger @ 2016-12-14 16:17 UTC (permalink / raw)
  To: devicetree, linux-iio
  Cc: linux-kernel, robh+dt, pawel.moll, mark.rutland, ijc+devicetree,
	galak, jic23, knaack.h, lars, pmeerw, ak

This is the IIO driver for AVIA HX711 ADC which ist mostly used in weighting
cells.

The protocol is quite simple and using GPIOs:
One GPIO is used as clock (SCK) while another GPIO is read (DOUT)

The raw value read from the chip is delivered. 
To get a weight one needs to subtract the zero offset and scale it.

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/adc/Kconfig  |  18 +++
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/hx711.c  | 292 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 311 insertions(+)
 create mode 100644 drivers/iio/adc/hx711.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 932de1f9d1e7..918f582288c9 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -205,6 +205,24 @@ config HI8435
 	  This driver can also be built as a module. If so, the module will be
 	  called hi8435.
 
+config HX711
+	tristate "AVIA HX711 ADC for weight cells"
+	depends on GPIOLIB
+	help
+	  If you say yes here you get support for AVIA HX711 ADC which is used
+	  for weight cells
+
+	  This driver uses two GPIOs, one for setting the clock and the other
+	  one for getting the data
+
+	  Currently the raw value is read from the chip and delivered.
+	  For getting an actual weight one needs to subtract the
+	  zero offset and multiply by a scale factor.
+	  This should be done in userspace.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called hx711.
+
 config INA2XX_ADC
 	tristate "Texas Instruments INA2xx Power Monitors IIO driver"
 	depends on I2C && !SENSORS_INA2XX
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index b1aa456e6af3..d46e289900ef 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
 obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
 obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
 obj-$(CONFIG_HI8435) += hi8435.o
+obj-$(CONFIG_HX711) += hx711.o
 obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
 obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
diff --git a/drivers/iio/adc/hx711.c b/drivers/iio/adc/hx711.c
new file mode 100644
index 000000000000..700281932ff0
--- /dev/null
+++ b/drivers/iio/adc/hx711.c
@@ -0,0 +1,292 @@
+/*
+ * HX711: analog to digital converter for weight sensor module
+ *
+ * Copyright (c) 2016 Andreas Klinger <ak@it-klinger.de>
+ *
+ * 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.
+ *
+ */
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/sched.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define HX711_GAIN_32		2	/* gain = 32 for channel B  */
+#define HX711_GAIN_64		3	/* gain = 64 for channel A  */
+#define HX711_GAIN_128		1	/* gain = 128 for channel A */
+
+struct hx711_data {
+	struct device		*dev;
+	struct gpio_desc	*gpiod_sck;
+	struct gpio_desc	*gpiod_dout;
+	int			gain_pulse;
+	struct mutex		lock;
+};
+
+static int hx711_reset(struct hx711_data *hx711_data)
+{
+	int i, val;
+
+	val = gpiod_get_value(hx711_data->gpiod_dout);
+	if (val) {
+		gpiod_set_value(hx711_data->gpiod_sck, 1);
+		udelay(80);
+		gpiod_set_value(hx711_data->gpiod_sck, 0);
+
+		for (i = 0; i < 1000; i++) {
+			val = gpiod_get_value(hx711_data->gpiod_dout);
+			if (!val)
+				break;
+			/* sleep at least 1 ms */
+			msleep(1);
+		}
+	}
+
+	return val;
+}
+
+static int hx711_cycle(struct hx711_data *hx711_data)
+{
+	int val;
+
+	/* if preempted for more then 60us while SCK is high:
+	 * hx711 is going in reset
+	 * ==> measuring is false
+	 */
+	preempt_disable();
+	gpiod_set_value(hx711_data->gpiod_sck, 1);
+	val = gpiod_get_value(hx711_data->gpiod_dout);
+	gpiod_set_value(hx711_data->gpiod_sck, 0);
+	preempt_enable();
+
+	return val;
+}
+
+static int hx711_read(struct hx711_data *hx711_data)
+{
+	int i, ret;
+	int value = 0;
+
+	mutex_lock(&hx711_data->lock);
+
+	if (hx711_reset(hx711_data)) {
+		dev_err(hx711_data->dev, "reset failed!");
+		mutex_unlock(&hx711_data->lock);
+		return -1;
+	}
+
+	for (i = 0; i < 24; i++) {
+		value <<= 1;
+		ret = hx711_cycle(hx711_data);
+		if (ret)
+			value++;
+	}
+
+	value ^= 0x800000;
+
+	for (i = 0; i < hx711_data->gain_pulse; i++)
+		ret = hx711_cycle(hx711_data);
+
+	mutex_unlock(&hx711_data->lock);
+
+	return value;
+}
+
+static int hx711_read_raw(struct iio_dev *iio_dev,
+				const struct iio_chan_spec *chan,
+				int *val, int *val2, long mask)
+{
+	struct hx711_data *hx711_data = iio_priv(iio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*val = hx711_read(hx711_data);
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t hx711_gain_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct hx711_data *hx711_data = iio_priv(dev_to_iio_dev(dev));
+	int val;
+
+	switch (hx711_data->gain_pulse) {
+	case HX711_GAIN_32:
+		val = 32;
+		break;
+	case HX711_GAIN_64:
+		val = 64;
+		break;
+	default:
+		val = 128;
+	}
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t hx711_gain_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t len)
+{
+	struct hx711_data *hx711_data = iio_priv(dev_to_iio_dev(dev));
+	int ret, val;
+	int gain_save = hx711_data->gain_pulse;
+
+	ret = kstrtoint((const char *) buf, 10, &val);
+	if (ret)
+		return -EINVAL;
+
+	switch (val) {
+	case 32:
+		hx711_data->gain_pulse = HX711_GAIN_32;
+		break;
+	case 64:
+		hx711_data->gain_pulse = HX711_GAIN_64;
+		break;
+	case 128:
+		hx711_data->gain_pulse = HX711_GAIN_128;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	dev_dbg(hx711_data->dev, "gain_pulse: %d\n", hx711_data->gain_pulse);
+
+	/* if gain has changed do a fake read for the new gain to be set
+	 *   for the next read
+	 */
+	if (gain_save != hx711_data->gain_pulse)
+		hx711_read(hx711_data);
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(gain, S_IRUGO | S_IWUSR,
+	hx711_gain_show, hx711_gain_store, 0);
+
+static struct attribute *hx711_attributes[] = {
+	&iio_dev_attr_gain.dev_attr.attr,
+	NULL,
+};
+
+static struct attribute_group hx711_attribute_group = {
+	.attrs = hx711_attributes,
+};
+
+static const struct iio_info hx711_iio_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= hx711_read_raw,
+	.attrs			= &hx711_attribute_group,
+};
+
+static const struct iio_chan_spec hx711_chan_spec[] = {
+	{ .type = IIO_VOLTAGE,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW),
+	},
+};
+
+static int hx711_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hx711_data *hx711_data = NULL;
+	struct iio_dev *iio;
+	int ret = 0;
+
+	iio = devm_iio_device_alloc(dev, sizeof(struct hx711_data));
+	if (!iio) {
+		dev_err(dev, "failed to allocate IIO device\n");
+		return -ENOMEM;
+	}
+
+	hx711_data = iio_priv(iio);
+	hx711_data->dev = dev;
+
+	mutex_init(&hx711_data->lock);
+
+	hx711_data->gpiod_sck = devm_gpiod_get(dev, "sck", GPIOD_OUT_HIGH);
+	if (IS_ERR(hx711_data->gpiod_sck)) {
+		dev_err(dev, "failed to get sck-gpiod: err=%ld\n",
+					PTR_ERR(hx711_data->gpiod_sck));
+		return PTR_ERR(hx711_data->gpiod_sck);
+	}
+
+	hx711_data->gpiod_dout = devm_gpiod_get(dev, "dout", GPIOD_OUT_HIGH);
+	if (IS_ERR(hx711_data->gpiod_dout)) {
+		dev_err(dev, "failed to get dout-gpiod: err=%ld\n",
+					PTR_ERR(hx711_data->gpiod_dout));
+		return PTR_ERR(hx711_data->gpiod_dout);
+	}
+
+	ret = gpiod_direction_input(hx711_data->gpiod_dout);
+	if (ret < 0) {
+		dev_err(hx711_data->dev, "gpiod_direction_input: %d\n", ret);
+		return ret;
+	}
+
+	ret = gpiod_direction_output(hx711_data->gpiod_sck, 0);
+	if (ret < 0) {
+		dev_err(hx711_data->dev, "gpiod_direction_output: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, iio);
+
+	iio->name = pdev->name;
+	iio->dev.parent = &pdev->dev;
+	iio->info = &hx711_iio_info;
+	iio->modes = INDIO_DIRECT_MODE;
+	iio->channels = hx711_chan_spec;
+	iio->num_channels = ARRAY_SIZE(hx711_chan_spec);
+
+	return devm_iio_device_register(dev, iio);
+}
+
+static const struct of_device_id of_hx711_match[] = {
+	{ .compatible = "avia,hx711", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, of_hx711_match);
+
+static struct platform_driver hx711_driver = {
+	.probe		= hx711_probe,
+	.driver		= {
+		.name		= "hx711-gpio",
+		.of_match_table	= of_hx711_match,
+	},
+};
+
+module_platform_driver(hx711_driver);
+
+MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
+MODULE_DESCRIPTION("HX711 bitbanging driver - ADC for weight cells");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:hx711-gpio");
+
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v3 2/2] iio: adc: hx711: Add IIO driver for AVIA HX711
@ 2016-12-14 16:17 ` Andreas Klinger
  0 siblings, 0 replies; 8+ messages in thread
From: Andreas Klinger @ 2016-12-14 16:17 UTC (permalink / raw)
  To: devicetree-u79uwXL29TY76Z2rM5mHXA, linux-iio-u79uwXL29TY76Z2rM5mHXA
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, pawel.moll-5wv7dgnIgG8,
	mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
	galak-sgV2jX0FEOL9JmXXK+q4OQ, jic23-DgEjT+Ai2ygdnm+yROfE0A,
	knaack.h-Mmb7MZpHnFY, lars-Qo5EllUWu/uELgA04lAiVw,
	pmeerw-jW+XmwGofnusTnJN9+BGXg, ak-n176/SwNRljddJNmlsFzeA

This is the IIO driver for AVIA HX711 ADC which ist mostly used in weighting
cells.

The protocol is quite simple and using GPIOs:
One GPIO is used as clock (SCK) while another GPIO is read (DOUT)

The raw value read from the chip is delivered. 
To get a weight one needs to subtract the zero offset and scale it.

Signed-off-by: Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>
---
 drivers/iio/adc/Kconfig  |  18 +++
 drivers/iio/adc/Makefile |   1 +
 drivers/iio/adc/hx711.c  | 292 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 311 insertions(+)
 create mode 100644 drivers/iio/adc/hx711.c

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 932de1f9d1e7..918f582288c9 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -205,6 +205,24 @@ config HI8435
 	  This driver can also be built as a module. If so, the module will be
 	  called hi8435.
 
+config HX711
+	tristate "AVIA HX711 ADC for weight cells"
+	depends on GPIOLIB
+	help
+	  If you say yes here you get support for AVIA HX711 ADC which is used
+	  for weight cells
+
+	  This driver uses two GPIOs, one for setting the clock and the other
+	  one for getting the data
+
+	  Currently the raw value is read from the chip and delivered.
+	  For getting an actual weight one needs to subtract the
+	  zero offset and multiply by a scale factor.
+	  This should be done in userspace.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called hx711.
+
 config INA2XX_ADC
 	tristate "Texas Instruments INA2xx Power Monitors IIO driver"
 	depends on I2C && !SENSORS_INA2XX
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index b1aa456e6af3..d46e289900ef 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
 obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
 obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
 obj-$(CONFIG_HI8435) += hi8435.o
+obj-$(CONFIG_HX711) += hx711.o
 obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
 obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
 obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
diff --git a/drivers/iio/adc/hx711.c b/drivers/iio/adc/hx711.c
new file mode 100644
index 000000000000..700281932ff0
--- /dev/null
+++ b/drivers/iio/adc/hx711.c
@@ -0,0 +1,292 @@
+/*
+ * HX711: analog to digital converter for weight sensor module
+ *
+ * Copyright (c) 2016 Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>
+ *
+ * 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.
+ *
+ */
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/property.h>
+#include <linux/slab.h>
+#include <linux/sched.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define HX711_GAIN_32		2	/* gain = 32 for channel B  */
+#define HX711_GAIN_64		3	/* gain = 64 for channel A  */
+#define HX711_GAIN_128		1	/* gain = 128 for channel A */
+
+struct hx711_data {
+	struct device		*dev;
+	struct gpio_desc	*gpiod_sck;
+	struct gpio_desc	*gpiod_dout;
+	int			gain_pulse;
+	struct mutex		lock;
+};
+
+static int hx711_reset(struct hx711_data *hx711_data)
+{
+	int i, val;
+
+	val = gpiod_get_value(hx711_data->gpiod_dout);
+	if (val) {
+		gpiod_set_value(hx711_data->gpiod_sck, 1);
+		udelay(80);
+		gpiod_set_value(hx711_data->gpiod_sck, 0);
+
+		for (i = 0; i < 1000; i++) {
+			val = gpiod_get_value(hx711_data->gpiod_dout);
+			if (!val)
+				break;
+			/* sleep at least 1 ms */
+			msleep(1);
+		}
+	}
+
+	return val;
+}
+
+static int hx711_cycle(struct hx711_data *hx711_data)
+{
+	int val;
+
+	/* if preempted for more then 60us while SCK is high:
+	 * hx711 is going in reset
+	 * ==> measuring is false
+	 */
+	preempt_disable();
+	gpiod_set_value(hx711_data->gpiod_sck, 1);
+	val = gpiod_get_value(hx711_data->gpiod_dout);
+	gpiod_set_value(hx711_data->gpiod_sck, 0);
+	preempt_enable();
+
+	return val;
+}
+
+static int hx711_read(struct hx711_data *hx711_data)
+{
+	int i, ret;
+	int value = 0;
+
+	mutex_lock(&hx711_data->lock);
+
+	if (hx711_reset(hx711_data)) {
+		dev_err(hx711_data->dev, "reset failed!");
+		mutex_unlock(&hx711_data->lock);
+		return -1;
+	}
+
+	for (i = 0; i < 24; i++) {
+		value <<= 1;
+		ret = hx711_cycle(hx711_data);
+		if (ret)
+			value++;
+	}
+
+	value ^= 0x800000;
+
+	for (i = 0; i < hx711_data->gain_pulse; i++)
+		ret = hx711_cycle(hx711_data);
+
+	mutex_unlock(&hx711_data->lock);
+
+	return value;
+}
+
+static int hx711_read_raw(struct iio_dev *iio_dev,
+				const struct iio_chan_spec *chan,
+				int *val, int *val2, long mask)
+{
+	struct hx711_data *hx711_data = iio_priv(iio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			*val = hx711_read(hx711_data);
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t hx711_gain_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct hx711_data *hx711_data = iio_priv(dev_to_iio_dev(dev));
+	int val;
+
+	switch (hx711_data->gain_pulse) {
+	case HX711_GAIN_32:
+		val = 32;
+		break;
+	case HX711_GAIN_64:
+		val = 64;
+		break;
+	default:
+		val = 128;
+	}
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t hx711_gain_store(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t len)
+{
+	struct hx711_data *hx711_data = iio_priv(dev_to_iio_dev(dev));
+	int ret, val;
+	int gain_save = hx711_data->gain_pulse;
+
+	ret = kstrtoint((const char *) buf, 10, &val);
+	if (ret)
+		return -EINVAL;
+
+	switch (val) {
+	case 32:
+		hx711_data->gain_pulse = HX711_GAIN_32;
+		break;
+	case 64:
+		hx711_data->gain_pulse = HX711_GAIN_64;
+		break;
+	case 128:
+		hx711_data->gain_pulse = HX711_GAIN_128;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	dev_dbg(hx711_data->dev, "gain_pulse: %d\n", hx711_data->gain_pulse);
+
+	/* if gain has changed do a fake read for the new gain to be set
+	 *   for the next read
+	 */
+	if (gain_save != hx711_data->gain_pulse)
+		hx711_read(hx711_data);
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(gain, S_IRUGO | S_IWUSR,
+	hx711_gain_show, hx711_gain_store, 0);
+
+static struct attribute *hx711_attributes[] = {
+	&iio_dev_attr_gain.dev_attr.attr,
+	NULL,
+};
+
+static struct attribute_group hx711_attribute_group = {
+	.attrs = hx711_attributes,
+};
+
+static const struct iio_info hx711_iio_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= hx711_read_raw,
+	.attrs			= &hx711_attribute_group,
+};
+
+static const struct iio_chan_spec hx711_chan_spec[] = {
+	{ .type = IIO_VOLTAGE,
+		.info_mask_separate =
+			BIT(IIO_CHAN_INFO_RAW),
+	},
+};
+
+static int hx711_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hx711_data *hx711_data = NULL;
+	struct iio_dev *iio;
+	int ret = 0;
+
+	iio = devm_iio_device_alloc(dev, sizeof(struct hx711_data));
+	if (!iio) {
+		dev_err(dev, "failed to allocate IIO device\n");
+		return -ENOMEM;
+	}
+
+	hx711_data = iio_priv(iio);
+	hx711_data->dev = dev;
+
+	mutex_init(&hx711_data->lock);
+
+	hx711_data->gpiod_sck = devm_gpiod_get(dev, "sck", GPIOD_OUT_HIGH);
+	if (IS_ERR(hx711_data->gpiod_sck)) {
+		dev_err(dev, "failed to get sck-gpiod: err=%ld\n",
+					PTR_ERR(hx711_data->gpiod_sck));
+		return PTR_ERR(hx711_data->gpiod_sck);
+	}
+
+	hx711_data->gpiod_dout = devm_gpiod_get(dev, "dout", GPIOD_OUT_HIGH);
+	if (IS_ERR(hx711_data->gpiod_dout)) {
+		dev_err(dev, "failed to get dout-gpiod: err=%ld\n",
+					PTR_ERR(hx711_data->gpiod_dout));
+		return PTR_ERR(hx711_data->gpiod_dout);
+	}
+
+	ret = gpiod_direction_input(hx711_data->gpiod_dout);
+	if (ret < 0) {
+		dev_err(hx711_data->dev, "gpiod_direction_input: %d\n", ret);
+		return ret;
+	}
+
+	ret = gpiod_direction_output(hx711_data->gpiod_sck, 0);
+	if (ret < 0) {
+		dev_err(hx711_data->dev, "gpiod_direction_output: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, iio);
+
+	iio->name = pdev->name;
+	iio->dev.parent = &pdev->dev;
+	iio->info = &hx711_iio_info;
+	iio->modes = INDIO_DIRECT_MODE;
+	iio->channels = hx711_chan_spec;
+	iio->num_channels = ARRAY_SIZE(hx711_chan_spec);
+
+	return devm_iio_device_register(dev, iio);
+}
+
+static const struct of_device_id of_hx711_match[] = {
+	{ .compatible = "avia,hx711", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, of_hx711_match);
+
+static struct platform_driver hx711_driver = {
+	.probe		= hx711_probe,
+	.driver		= {
+		.name		= "hx711-gpio",
+		.of_match_table	= of_hx711_match,
+	},
+};
+
+module_platform_driver(hx711_driver);
+
+MODULE_AUTHOR("Andreas Klinger <ak-n176/SwNRljddJNmlsFzeA@public.gmane.org>");
+MODULE_DESCRIPTION("HX711 bitbanging driver - ADC for weight cells");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:hx711-gpio");
+
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] iio: adc: hx711: Add IIO driver for AVIA HX711
  2016-12-14 16:17 ` Andreas Klinger
@ 2016-12-19 16:28   ` Lars-Peter Clausen
  -1 siblings, 0 replies; 8+ messages in thread
From: Lars-Peter Clausen @ 2016-12-19 16:28 UTC (permalink / raw)
  To: Andreas Klinger, devicetree, linux-iio
  Cc: linux-kernel, robh+dt, pawel.moll, mark.rutland, ijc+devicetree,
	galak, jic23, knaack.h, pmeerw

On 12/14/2016 05:17 PM, Andreas Klinger wrote:
[...]
> +#include <linux/err.h>
> +#include <linux/gpio.h>

Since you used the consumer API linux/gpio.h is not needed.

> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/sched.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define HX711_GAIN_32		2	/* gain = 32 for channel B  */
> +#define HX711_GAIN_64		3	/* gain = 64 for channel A  */
> +#define HX711_GAIN_128		1	/* gain = 128 for channel A */
> +
> +struct hx711_data {
> +	struct device		*dev;
> +	struct gpio_desc	*gpiod_sck;
> +	struct gpio_desc	*gpiod_dout;
> +	int			gain_pulse;
> +	struct mutex		lock;
> +};
> +
> +static int hx711_read(struct hx711_data *hx711_data)
> +{
> +	int i, ret;
> +	int value = 0;
> +
> +	mutex_lock(&hx711_data->lock);
> +
> +	if (hx711_reset(hx711_data)) {

If you reset the device before each conversion wont this clear the channel
and gain selection? Wouldn't the driver always read from channel A at 128
gain regardless of what has been selected?

> +		dev_err(hx711_data->dev, "reset failed!");
> +		mutex_unlock(&hx711_data->lock);
> +		return -1;

If there is an error it should be propagated to the higher layers. At the
moment you only return a bogus conversion value.

> +	}
> +
> +	for (i = 0; i < 24; i++) {
> +		value <<= 1;
> +		ret = hx711_cycle(hx711_data);
> +		if (ret)
> +			value++;
> +	}
> +
> +	value ^= 0x800000;
> +
> +	for (i = 0; i < hx711_data->gain_pulse; i++)
> +		ret = hx711_cycle(hx711_data);
> +
> +	mutex_unlock(&hx711_data->lock);
> +
> +	return value;
> +}
> +
> +static int hx711_read_raw(struct iio_dev *iio_dev,
> +				const struct iio_chan_spec *chan,
> +				int *val, int *val2, long mask)
> +{
> +	struct hx711_data *hx711_data = iio_priv(iio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = hx711_read(hx711_data);
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
[...]
> +static struct attribute *hx711_attributes[] = {
> +	&iio_dev_attr_gain.dev_attr.attr,

For IIO devices the gain is typically expressed through the scale attribute.
Which is kind of the inverse of gain. It would be good if this driver
follows this standard notation. The scale is the value of 1LSB in mV. So
this includes the resolution of the ADC, the reference voltage and any gain
that is applied to the input signal.

The possible values can be listed in the scale_available attribute.

> +	NULL,
> +};
> +
> +static struct attribute_group hx711_attribute_group = {
> +	.attrs = hx711_attributes,
> +};
> +
> +static const struct iio_info hx711_iio_info = {
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= hx711_read_raw,
> +	.attrs			= &hx711_attribute_group,
> +};
> +
> +static const struct iio_chan_spec hx711_chan_spec[] = {
> +	{ .type = IIO_VOLTAGE,
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_RAW),

Given that there are two separate physical input channels this should be
expressed here and there should be two IIO channels for the device.

> +	},
> +};
> +
> +static int hx711_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct hx711_data *hx711_data = NULL;

The = NULL is not needed as it is overwritten a few lines below.

> +	struct iio_dev *iio;
> +	int ret = 0;

Again = 0 no needed.

> +
> +	iio = devm_iio_device_alloc(dev, sizeof(struct hx711_data));
> +	if (!iio) {
> +		dev_err(dev, "failed to allocate IIO device\n");
> +		return -ENOMEM;
> +	}
> +
> +	hx711_data = iio_priv(iio);
> +	hx711_data->dev = dev;
> +
> +	mutex_init(&hx711_data->lock);
> +
> +	hx711_data->gpiod_sck = devm_gpiod_get(dev, "sck", GPIOD_OUT_HIGH);
> +	if (IS_ERR(hx711_data->gpiod_sck)) {
> +		dev_err(dev, "failed to get sck-gpiod: err=%ld\n",
> +					PTR_ERR(hx711_data->gpiod_sck));
> +		return PTR_ERR(hx711_data->gpiod_sck);
> +	}
> +
> +	hx711_data->gpiod_dout = devm_gpiod_get(dev, "dout", GPIOD_OUT_HIGH);
> +	if (IS_ERR(hx711_data->gpiod_dout)) {
> +		dev_err(dev, "failed to get dout-gpiod: err=%ld\n",
> +					PTR_ERR(hx711_data->gpiod_dout));
> +		return PTR_ERR(hx711_data->gpiod_dout);
> +	}
> +
> +	ret = gpiod_direction_input(hx711_data->gpiod_dout);

If dout is used as a input GPIO you should request it with GPIOD_IN. In that
case you can remove the gpiod_direction_input() call.

> +	if (ret < 0) {
> +		dev_err(hx711_data->dev, "gpiod_direction_input: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = gpiod_direction_output(hx711_data->gpiod_sck, 0);

Similar to above. If you want this to be a output GPIO with the default
value of 0 request it with GPIOD_OUT_LOW.

> +	if (ret < 0) {
> +		dev_err(hx711_data->dev, "gpiod_direction_output: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, iio);

There is no matching platform_get_drvdata() so this can probably be removed.

> +
> +	iio->name = pdev->name;

This should be the part name. E.g. "hx711" in this case.

> +	iio->dev.parent = &pdev->dev;
> +	iio->info = &hx711_iio_info;
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->channels = hx711_chan_spec;
> +	iio->num_channels = ARRAY_SIZE(hx711_chan_spec);
> +
> +	return devm_iio_device_register(dev, iio);
> +}

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] iio: adc: hx711: Add IIO driver for AVIA HX711
@ 2016-12-19 16:28   ` Lars-Peter Clausen
  0 siblings, 0 replies; 8+ messages in thread
From: Lars-Peter Clausen @ 2016-12-19 16:28 UTC (permalink / raw)
  To: Andreas Klinger, devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-iio-u79uwXL29TY76Z2rM5mHXA
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, pawel.moll-5wv7dgnIgG8,
	mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
	galak-sgV2jX0FEOL9JmXXK+q4OQ, jic23-DgEjT+Ai2ygdnm+yROfE0A,
	knaack.h-Mmb7MZpHnFY, pmeerw-jW+XmwGofnusTnJN9+BGXg

On 12/14/2016 05:17 PM, Andreas Klinger wrote:
[...]
> +#include <linux/err.h>
> +#include <linux/gpio.h>

Since you used the consumer API linux/gpio.h is not needed.

> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/sched.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define HX711_GAIN_32		2	/* gain = 32 for channel B  */
> +#define HX711_GAIN_64		3	/* gain = 64 for channel A  */
> +#define HX711_GAIN_128		1	/* gain = 128 for channel A */
> +
> +struct hx711_data {
> +	struct device		*dev;
> +	struct gpio_desc	*gpiod_sck;
> +	struct gpio_desc	*gpiod_dout;
> +	int			gain_pulse;
> +	struct mutex		lock;
> +};
> +
> +static int hx711_read(struct hx711_data *hx711_data)
> +{
> +	int i, ret;
> +	int value = 0;
> +
> +	mutex_lock(&hx711_data->lock);
> +
> +	if (hx711_reset(hx711_data)) {

If you reset the device before each conversion wont this clear the channel
and gain selection? Wouldn't the driver always read from channel A at 128
gain regardless of what has been selected?

> +		dev_err(hx711_data->dev, "reset failed!");
> +		mutex_unlock(&hx711_data->lock);
> +		return -1;

If there is an error it should be propagated to the higher layers. At the
moment you only return a bogus conversion value.

> +	}
> +
> +	for (i = 0; i < 24; i++) {
> +		value <<= 1;
> +		ret = hx711_cycle(hx711_data);
> +		if (ret)
> +			value++;
> +	}
> +
> +	value ^= 0x800000;
> +
> +	for (i = 0; i < hx711_data->gain_pulse; i++)
> +		ret = hx711_cycle(hx711_data);
> +
> +	mutex_unlock(&hx711_data->lock);
> +
> +	return value;
> +}
> +
> +static int hx711_read_raw(struct iio_dev *iio_dev,
> +				const struct iio_chan_spec *chan,
> +				int *val, int *val2, long mask)
> +{
> +	struct hx711_data *hx711_data = iio_priv(iio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = hx711_read(hx711_data);
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
[...]
> +static struct attribute *hx711_attributes[] = {
> +	&iio_dev_attr_gain.dev_attr.attr,

For IIO devices the gain is typically expressed through the scale attribute.
Which is kind of the inverse of gain. It would be good if this driver
follows this standard notation. The scale is the value of 1LSB in mV. So
this includes the resolution of the ADC, the reference voltage and any gain
that is applied to the input signal.

The possible values can be listed in the scale_available attribute.

> +	NULL,
> +};
> +
> +static struct attribute_group hx711_attribute_group = {
> +	.attrs = hx711_attributes,
> +};
> +
> +static const struct iio_info hx711_iio_info = {
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= hx711_read_raw,
> +	.attrs			= &hx711_attribute_group,
> +};
> +
> +static const struct iio_chan_spec hx711_chan_spec[] = {
> +	{ .type = IIO_VOLTAGE,
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_RAW),

Given that there are two separate physical input channels this should be
expressed here and there should be two IIO channels for the device.

> +	},
> +};
> +
> +static int hx711_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct hx711_data *hx711_data = NULL;

The = NULL is not needed as it is overwritten a few lines below.

> +	struct iio_dev *iio;
> +	int ret = 0;

Again = 0 no needed.

> +
> +	iio = devm_iio_device_alloc(dev, sizeof(struct hx711_data));
> +	if (!iio) {
> +		dev_err(dev, "failed to allocate IIO device\n");
> +		return -ENOMEM;
> +	}
> +
> +	hx711_data = iio_priv(iio);
> +	hx711_data->dev = dev;
> +
> +	mutex_init(&hx711_data->lock);
> +
> +	hx711_data->gpiod_sck = devm_gpiod_get(dev, "sck", GPIOD_OUT_HIGH);
> +	if (IS_ERR(hx711_data->gpiod_sck)) {
> +		dev_err(dev, "failed to get sck-gpiod: err=%ld\n",
> +					PTR_ERR(hx711_data->gpiod_sck));
> +		return PTR_ERR(hx711_data->gpiod_sck);
> +	}
> +
> +	hx711_data->gpiod_dout = devm_gpiod_get(dev, "dout", GPIOD_OUT_HIGH);
> +	if (IS_ERR(hx711_data->gpiod_dout)) {
> +		dev_err(dev, "failed to get dout-gpiod: err=%ld\n",
> +					PTR_ERR(hx711_data->gpiod_dout));
> +		return PTR_ERR(hx711_data->gpiod_dout);
> +	}
> +
> +	ret = gpiod_direction_input(hx711_data->gpiod_dout);

If dout is used as a input GPIO you should request it with GPIOD_IN. In that
case you can remove the gpiod_direction_input() call.

> +	if (ret < 0) {
> +		dev_err(hx711_data->dev, "gpiod_direction_input: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = gpiod_direction_output(hx711_data->gpiod_sck, 0);

Similar to above. If you want this to be a output GPIO with the default
value of 0 request it with GPIOD_OUT_LOW.

> +	if (ret < 0) {
> +		dev_err(hx711_data->dev, "gpiod_direction_output: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, iio);

There is no matching platform_get_drvdata() so this can probably be removed.

> +
> +	iio->name = pdev->name;

This should be the part name. E.g. "hx711" in this case.

> +	iio->dev.parent = &pdev->dev;
> +	iio->info = &hx711_iio_info;
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->channels = hx711_chan_spec;
> +	iio->num_channels = ARRAY_SIZE(hx711_chan_spec);
> +
> +	return devm_iio_device_register(dev, iio);
> +}

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] iio: adc: hx711: Add IIO driver for AVIA HX711
  2016-12-14 16:17 ` Andreas Klinger
  (?)
  (?)
@ 2016-12-19 20:49 ` Jonathan Cameron
  -1 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2016-12-19 20:49 UTC (permalink / raw)
  To: Andreas Klinger, devicetree, linux-iio
  Cc: linux-kernel, robh+dt, pawel.moll, mark.rutland, ijc+devicetree,
	galak, knaack.h, lars, pmeerw

On 14/12/16 16:17, Andreas Klinger wrote:
> This is the IIO driver for AVIA HX711 ADC which ist mostly used in weighting
> cells.
> 
> The protocol is quite simple and using GPIOs:
> One GPIO is used as clock (SCK) while another GPIO is read (DOUT)
Youch. Controlling the next conversion via the number of clocks is hideous!
Oh well, guess it's one solution that limits the number of wires needed.

Still not as hideous as some ;) (sht15 I'm looking at you :)

Few comments inline.

Jonathan

> 
> The raw value read from the chip is delivered. 
> To get a weight one needs to subtract the zero offset and scale it.
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
>  drivers/iio/adc/Kconfig  |  18 +++
>  drivers/iio/adc/Makefile |   1 +
>  drivers/iio/adc/hx711.c  | 292 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 311 insertions(+)
>  create mode 100644 drivers/iio/adc/hx711.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 932de1f9d1e7..918f582288c9 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -205,6 +205,24 @@ config HI8435
>  	  This driver can also be built as a module. If so, the module will be
>  	  called hi8435.
>  
> +config HX711
> +	tristate "AVIA HX711 ADC for weight cells"
> +	depends on GPIOLIB
> +	help
> +	  If you say yes here you get support for AVIA HX711 ADC which is used
> +	  for weight cells
Typically just called weigh cells rather than weight cells.
One of those ugly bits of English.
> +
> +	  This driver uses two GPIOs, one for setting the clock and the other
> +	  one for getting the data
This driver uses two GPIOs, one acts as the clock and controls the channel
selection and gain, the other is used for the measurement data
(or something like that).
> +
> +	  Currently the raw value is read from the chip and delivered.
> +	  For getting an actual weight one needs to subtract the
To get an actual weight...
> +	  zero offset and multiply by a scale factor.
> +	  This should be done in userspace.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called hx711.
> +
>  config INA2XX_ADC
>  	tristate "Texas Instruments INA2xx Power Monitors IIO driver"
>  	depends on I2C && !SENSORS_INA2XX
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index b1aa456e6af3..d46e289900ef 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_CC10001_ADC) += cc10001_adc.o
>  obj-$(CONFIG_DA9150_GPADC) += da9150-gpadc.o
>  obj-$(CONFIG_EXYNOS_ADC) += exynos_adc.o
>  obj-$(CONFIG_HI8435) += hi8435.o
> +obj-$(CONFIG_HX711) += hx711.o
>  obj-$(CONFIG_IMX7D_ADC) += imx7d_adc.o
>  obj-$(CONFIG_INA2XX_ADC) += ina2xx-adc.o
>  obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
> diff --git a/drivers/iio/adc/hx711.c b/drivers/iio/adc/hx711.c
> new file mode 100644
> index 000000000000..700281932ff0
> --- /dev/null
> +++ b/drivers/iio/adc/hx711.c
> @@ -0,0 +1,292 @@
> +/*
> + * HX711: analog to digital converter for weight sensor module
> + *
> + * Copyright (c) 2016 Andreas Klinger <ak@it-klinger.de>
> + *
> + * 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.
> + *
No need for this blank line.
> + */
> +#include <linux/err.h>
> +#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +#include <linux/slab.h>
> +#include <linux/sched.h>
> +#include <linux/delay.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define HX711_GAIN_32		2	/* gain = 32 for channel B  */
> +#define HX711_GAIN_64		3	/* gain = 64 for channel A  */
> +#define HX711_GAIN_128		1	/* gain = 128 for channel A */
> +
> +struct hx711_data {
> +	struct device		*dev;
> +	struct gpio_desc	*gpiod_sck;
> +	struct gpio_desc	*gpiod_dout;
> +	int			gain_pulse;
> +	struct mutex		lock;
> +};
> +
> +static int hx711_reset(struct hx711_data *hx711_data)
> +{
> +	int i, val;
> +
> +	val = gpiod_get_value(hx711_data->gpiod_dout);
> +	if (val) {
> +		gpiod_set_value(hx711_data->gpiod_sck, 1);
> +		udelay(80);
a comment here on why 80 would be good (it's bigger than 60?)
> +		gpiod_set_value(hx711_data->gpiod_sck, 0);
> +
> +		for (i = 0; i < 1000; i++) {
> +			val = gpiod_get_value(hx711_data->gpiod_dout);
> +			if (!val)
> +				break;
> +			/* sleep at least 1 ms */
> +			msleep(1);
> +		}
> +	}
> +
> +	return val;
> +}
> +
> +static int hx711_cycle(struct hx711_data *hx711_data)
> +{
> +	int val;
> +
/*
 * if pre...
> +	/* if preempted for more then 60us while SCK is high:
> +	 * hx711 is going in reset
> +	 * ==> measuring is false
> +	 */
> +	preempt_disable();
> +	gpiod_set_value(hx711_data->gpiod_sck, 1);
I'm reading the datasheet as suggesting you need to wait at least 0.1 microsecs
here...
> +	val = gpiod_get_value(hx711_data->gpiod_dout);
> +	gpiod_set_value(hx711_data->gpiod_sck, 0);
> +	preempt_enable();
> +
> +	return val;
> +}
> +
> +static int hx711_read(struct hx711_data *hx711_data)
> +{
> +	int i, ret;
> +	int value = 0;
> +
> +	mutex_lock(&hx711_data->lock);
> +
> +	if (hx711_reset(hx711_data)) {
> +		dev_err(hx711_data->dev, "reset failed!");
> +		mutex_unlock(&hx711_data->lock);
> +		return -1;
> +	}
> +
> +	for (i = 0; i < 24; i++) {
> +		value <<= 1;
> +		ret = hx711_cycle(hx711_data);
> +		if (ret)
> +			value++;
> +	}
> +
> +	value ^= 0x800000;
> +
> +	for (i = 0; i < hx711_data->gain_pulse; i++)
> +		ret = hx711_cycle(hx711_data);
> +
> +	mutex_unlock(&hx711_data->lock);
> +
> +	return value;
> +}
> +
> +static int hx711_read_raw(struct iio_dev *iio_dev,
> +				const struct iio_chan_spec *chan,
> +				int *val, int *val2, long mask)
> +{
> +	struct hx711_data *hx711_data = iio_priv(iio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_VOLTAGE:
> +			*val = hx711_read(hx711_data);
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static ssize_t hx711_gain_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct hx711_data *hx711_data = iio_priv(dev_to_iio_dev(dev));
> +	int val;
> +
> +	switch (hx711_data->gain_pulse) {
> +	case HX711_GAIN_32:
> +		val = 32;
> +		break;
> +	case HX711_GAIN_64:
> +		val = 64;
> +		break;
> +	default:
> +		val = 128;
> +	}
> +
> +	return sprintf(buf, "%d\n", val);
> +}
> +
> +static ssize_t hx711_gain_store(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf, size_t len)
> +{
> +	struct hx711_data *hx711_data = iio_priv(dev_to_iio_dev(dev));
> +	int ret, val;
> +	int gain_save = hx711_data->gain_pulse;
> +
> +	ret = kstrtoint((const char *) buf, 10, &val);
> +	if (ret)
> +		return -EINVAL;
> +
> +	switch (val) {
> +	case 32:
> +		hx711_data->gain_pulse = HX711_GAIN_32;
> +		break;
> +	case 64:
> +		hx711_data->gain_pulse = HX711_GAIN_64;
> +		break;
> +	case 128:
> +		hx711_data->gain_pulse = HX711_GAIN_128;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(hx711_data->dev, "gain_pulse: %d\n", hx711_data->gain_pulse);
> +
> +	/* if gain has changed do a fake read for the new gain to be set
> +	 *   for the next read
> +	 */
> +	if (gain_save != hx711_data->gain_pulse)
> +		hx711_read(hx711_data);
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(gain, S_IRUGO | S_IWUSR,
> +	hx711_gain_show, hx711_gain_store, 0);
> +
> +static struct attribute *hx711_attributes[] = {
> +	&iio_dev_attr_gain.dev_attr.attr,
> +	NULL,
> +};
> +
As Lars suggested, please use standard ABI (easier if you use the info_mask
elements and do it through read raw...
> +static struct attribute_group hx711_attribute_group = {
> +	.attrs = hx711_attributes,
> +};
> +
> +static const struct iio_info hx711_iio_info = {
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= hx711_read_raw,
> +	.attrs			= &hx711_attribute_group,
> +};
> +
> +static const struct iio_chan_spec hx711_chan_spec[] = {
> +	{
new line here is slightly nicer to read.
>.type = IIO_VOLTAGE,
> +		.info_mask_separate =
> +			BIT(IIO_CHAN_INFO_RAW),
> +	},
> +};
> +
> +static int hx711_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct hx711_data *hx711_data = NULL;
> +	struct iio_dev *iio;
> +	int ret = 0;
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(struct hx711_data));
> +	if (!iio) {
> +		dev_err(dev, "failed to allocate IIO device\n");
> +		return -ENOMEM;
> +	}
> +
> +	hx711_data = iio_priv(iio);
> +	hx711_data->dev = dev;
> +
> +	mutex_init(&hx711_data->lock);
> +
> +	hx711_data->gpiod_sck = devm_gpiod_get(dev, "sck", GPIOD_OUT_HIGH);
> +	if (IS_ERR(hx711_data->gpiod_sck)) {
> +		dev_err(dev, "failed to get sck-gpiod: err=%ld\n",
> +					PTR_ERR(hx711_data->gpiod_sck));
> +		return PTR_ERR(hx711_data->gpiod_sck);
> +	}
> +
> +	hx711_data->gpiod_dout = devm_gpiod_get(dev, "dout", GPIOD_OUT_HIGH);
If there is a reason for getting an input as an output then it wants a comment!
> +	if (IS_ERR(hx711_data->gpiod_dout)) {
> +		dev_err(dev, "failed to get dout-gpiod: err=%ld\n",
> +					PTR_ERR(hx711_data->gpiod_dout));
> +		return PTR_ERR(hx711_data->gpiod_dout);
> +	}
> +
> +	ret = gpiod_direction_input(hx711_data->gpiod_dout);
> +	if (ret < 0) {
> +		dev_err(hx711_data->dev, "gpiod_direction_input: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = gpiod_direction_output(hx711_data->gpiod_sck, 0);
Doesn't the flag above already mean we are in output mode for this pin?
> +	if (ret < 0) {
> +		dev_err(hx711_data->dev, "gpiod_direction_output: %d\n", ret);
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, iio);
> +
> +	iio->name = pdev->name;
> +	iio->dev.parent = &pdev->dev;
> +	iio->info = &hx711_iio_info;
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->channels = hx711_chan_spec;
> +	iio->num_channels = ARRAY_SIZE(hx711_chan_spec);
> +
> +	return devm_iio_device_register(dev, iio);
> +}
> +
> +static const struct of_device_id of_hx711_match[] = {
> +	{ .compatible = "avia,hx711", },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, of_hx711_match);
> +
> +static struct platform_driver hx711_driver = {
> +	.probe		= hx711_probe,
> +	.driver		= {
> +		.name		= "hx711-gpio",
> +		.of_match_table	= of_hx711_match,
> +	},
> +};
> +
> +module_platform_driver(hx711_driver);
> +
> +MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
> +MODULE_DESCRIPTION("HX711 bitbanging driver - ADC for weight cells");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:hx711-gpio");
> +
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] iio: adc: hx711: Add IIO driver for AVIA HX711
@ 2016-12-20 10:33     ` Andreas Klinger
  0 siblings, 0 replies; 8+ messages in thread
From: Andreas Klinger @ 2016-12-20 10:33 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: devicetree, linux-iio, linux-kernel, robh+dt, pawel.moll,
	mark.rutland, ijc+devicetree, galak, jic23, knaack.h, pmeerw

Hello Lars,

thank you for the thorough review.

I have some questions. See below.

Thanks,

Andreas


Lars-Peter Clausen <lars@metafoo.de> schrieb am Mon, 19. Dec 17:28:
> On 12/14/2016 05:17 PM, Andreas Klinger wrote:
> [...]
> > +#include <linux/err.h>
> > +#include <linux/gpio.h>
> 
> Since you used the consumer API linux/gpio.h is not needed.
> 
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/sched.h>
> > +#include <linux/delay.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#define HX711_GAIN_32		2	/* gain = 32 for channel B  */
> > +#define HX711_GAIN_64		3	/* gain = 64 for channel A  */
> > +#define HX711_GAIN_128		1	/* gain = 128 for channel A */
> > +
> > +struct hx711_data {
> > +	struct device		*dev;
> > +	struct gpio_desc	*gpiod_sck;
> > +	struct gpio_desc	*gpiod_dout;
> > +	int			gain_pulse;
> > +	struct mutex		lock;
> > +};
> > +
> > +static int hx711_read(struct hx711_data *hx711_data)
> > +{
> > +	int i, ret;
> > +	int value = 0;
> > +
> > +	mutex_lock(&hx711_data->lock);
> > +
> > +	if (hx711_reset(hx711_data)) {
> 
> If you reset the device before each conversion wont this clear the channel
> and gain selection? Wouldn't the driver always read from channel A at 128
> gain regardless of what has been selected?
>

This is a bug, i need to fix. Thank you.

> > +		dev_err(hx711_data->dev, "reset failed!");
> > +		mutex_unlock(&hx711_data->lock);
> > +		return -1;
> 
> If there is an error it should be propagated to the higher layers. At the
> moment you only return a bogus conversion value.
> 
> > +	}
> > +
> > +	for (i = 0; i < 24; i++) {
> > +		value <<= 1;
> > +		ret = hx711_cycle(hx711_data);
> > +		if (ret)
> > +			value++;
> > +	}
> > +
> > +	value ^= 0x800000;
> > +
> > +	for (i = 0; i < hx711_data->gain_pulse; i++)
> > +		ret = hx711_cycle(hx711_data);
> > +
> > +	mutex_unlock(&hx711_data->lock);
> > +
> > +	return value;
> > +}
> > +
> > +static int hx711_read_raw(struct iio_dev *iio_dev,
> > +				const struct iio_chan_spec *chan,
> > +				int *val, int *val2, long mask)
> > +{
> > +	struct hx711_data *hx711_data = iio_priv(iio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		switch (chan->type) {
> > +		case IIO_VOLTAGE:
> > +			*val = hx711_read(hx711_data);
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> [...]
> > +static struct attribute *hx711_attributes[] = {
> > +	&iio_dev_attr_gain.dev_attr.attr,
> 
> For IIO devices the gain is typically expressed through the scale attribute.
> Which is kind of the inverse of gain. It would be good if this driver
> follows this standard notation. The scale is the value of 1LSB in mV. So
> this includes the resolution of the ADC, the reference voltage and any gain
> that is applied to the input signal.
> 
> The possible values can be listed in the scale_available attribute.
> 

The reference voltage is in the hardware. 
Should i use a DT entry for the reference voltage? 
Or is it better to use a buildin scale and make it changeable?


> > +	NULL,
> > +};
> > +
> > +static struct attribute_group hx711_attribute_group = {
> > +	.attrs = hx711_attributes,
> > +};
> > +
> > +static const struct iio_info hx711_iio_info = {
> > +	.driver_module		= THIS_MODULE,
> > +	.read_raw		= hx711_read_raw,
> > +	.attrs			= &hx711_attribute_group,
> > +};
> > +
> > +static const struct iio_chan_spec hx711_chan_spec[] = {
> > +	{ .type = IIO_VOLTAGE,
> > +		.info_mask_separate =
> > +			BIT(IIO_CHAN_INFO_RAW),
> 
> Given that there are two separate physical input channels this should be
> expressed here and there should be two IIO channels for the device.
> 

One who is toggling between channel A and B will cause a dummy read
additional to every normal read. 

Should i offer a "toggling mode" which means that after reading
channel A the channel B is selected for the next read and after
reading channel B channel A is selected? Simply expecting the channel
is toggled on every read. If it's not toggled there need to be a dummy 
read, of course. This should be an attribute, right?


> > +	},
> > +};
> > +
> > +static int hx711_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct hx711_data *hx711_data = NULL;
> 
> The = NULL is not needed as it is overwritten a few lines below.
> 
> > +	struct iio_dev *iio;
> > +	int ret = 0;
> 
> Again = 0 no needed.
> 
> > +
> > +	iio = devm_iio_device_alloc(dev, sizeof(struct hx711_data));
> > +	if (!iio) {
> > +		dev_err(dev, "failed to allocate IIO device\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	hx711_data = iio_priv(iio);
> > +	hx711_data->dev = dev;
> > +
> > +	mutex_init(&hx711_data->lock);
> > +
> > +	hx711_data->gpiod_sck = devm_gpiod_get(dev, "sck", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(hx711_data->gpiod_sck)) {
> > +		dev_err(dev, "failed to get sck-gpiod: err=%ld\n",
> > +					PTR_ERR(hx711_data->gpiod_sck));
> > +		return PTR_ERR(hx711_data->gpiod_sck);
> > +	}
> > +
> > +	hx711_data->gpiod_dout = devm_gpiod_get(dev, "dout", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(hx711_data->gpiod_dout)) {
> > +		dev_err(dev, "failed to get dout-gpiod: err=%ld\n",
> > +					PTR_ERR(hx711_data->gpiod_dout));
> > +		return PTR_ERR(hx711_data->gpiod_dout);
> > +	}
> > +
> > +	ret = gpiod_direction_input(hx711_data->gpiod_dout);
> 
> If dout is used as a input GPIO you should request it with GPIOD_IN. In that
> case you can remove the gpiod_direction_input() call.
> 
> > +	if (ret < 0) {
> > +		dev_err(hx711_data->dev, "gpiod_direction_input: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = gpiod_direction_output(hx711_data->gpiod_sck, 0);
> 
> Similar to above. If you want this to be a output GPIO with the default
> value of 0 request it with GPIOD_OUT_LOW.
> 
> > +	if (ret < 0) {
> > +		dev_err(hx711_data->dev, "gpiod_direction_output: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, iio);
> 
> There is no matching platform_get_drvdata() so this can probably be removed.
> 
> > +
> > +	iio->name = pdev->name;
> 
> This should be the part name. E.g. "hx711" in this case.
> 
> > +	iio->dev.parent = &pdev->dev;
> > +	iio->info = &hx711_iio_info;
> > +	iio->modes = INDIO_DIRECT_MODE;
> > +	iio->channels = hx711_chan_spec;
> > +	iio->num_channels = ARRAY_SIZE(hx711_chan_spec);
> > +
> > +	return devm_iio_device_register(dev, iio);
> > +}
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] iio: adc: hx711: Add IIO driver for AVIA HX711
@ 2016-12-20 10:33     ` Andreas Klinger
  0 siblings, 0 replies; 8+ messages in thread
From: Andreas Klinger @ 2016-12-20 10:33 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-iio-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, pawel.moll-5wv7dgnIgG8,
	mark.rutland-5wv7dgnIgG8, ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg,
	galak-sgV2jX0FEOL9JmXXK+q4OQ, jic23-DgEjT+Ai2ygdnm+yROfE0A,
	knaack.h-Mmb7MZpHnFY, pmeerw-jW+XmwGofnusTnJN9+BGXg

Hello Lars,

thank you for the thorough review.

I have some questions. See below.

Thanks,

Andreas


Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> schrieb am Mon, 19. Dec 17:28:
> On 12/14/2016 05:17 PM, Andreas Klinger wrote:
> [...]
> > +#include <linux/err.h>
> > +#include <linux/gpio.h>
> 
> Since you used the consumer API linux/gpio.h is not needed.
> 
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/property.h>
> > +#include <linux/slab.h>
> > +#include <linux/sched.h>
> > +#include <linux/delay.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#define HX711_GAIN_32		2	/* gain = 32 for channel B  */
> > +#define HX711_GAIN_64		3	/* gain = 64 for channel A  */
> > +#define HX711_GAIN_128		1	/* gain = 128 for channel A */
> > +
> > +struct hx711_data {
> > +	struct device		*dev;
> > +	struct gpio_desc	*gpiod_sck;
> > +	struct gpio_desc	*gpiod_dout;
> > +	int			gain_pulse;
> > +	struct mutex		lock;
> > +};
> > +
> > +static int hx711_read(struct hx711_data *hx711_data)
> > +{
> > +	int i, ret;
> > +	int value = 0;
> > +
> > +	mutex_lock(&hx711_data->lock);
> > +
> > +	if (hx711_reset(hx711_data)) {
> 
> If you reset the device before each conversion wont this clear the channel
> and gain selection? Wouldn't the driver always read from channel A at 128
> gain regardless of what has been selected?
>

This is a bug, i need to fix. Thank you.

> > +		dev_err(hx711_data->dev, "reset failed!");
> > +		mutex_unlock(&hx711_data->lock);
> > +		return -1;
> 
> If there is an error it should be propagated to the higher layers. At the
> moment you only return a bogus conversion value.
> 
> > +	}
> > +
> > +	for (i = 0; i < 24; i++) {
> > +		value <<= 1;
> > +		ret = hx711_cycle(hx711_data);
> > +		if (ret)
> > +			value++;
> > +	}
> > +
> > +	value ^= 0x800000;
> > +
> > +	for (i = 0; i < hx711_data->gain_pulse; i++)
> > +		ret = hx711_cycle(hx711_data);
> > +
> > +	mutex_unlock(&hx711_data->lock);
> > +
> > +	return value;
> > +}
> > +
> > +static int hx711_read_raw(struct iio_dev *iio_dev,
> > +				const struct iio_chan_spec *chan,
> > +				int *val, int *val2, long mask)
> > +{
> > +	struct hx711_data *hx711_data = iio_priv(iio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		switch (chan->type) {
> > +		case IIO_VOLTAGE:
> > +			*val = hx711_read(hx711_data);
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> [...]
> > +static struct attribute *hx711_attributes[] = {
> > +	&iio_dev_attr_gain.dev_attr.attr,
> 
> For IIO devices the gain is typically expressed through the scale attribute.
> Which is kind of the inverse of gain. It would be good if this driver
> follows this standard notation. The scale is the value of 1LSB in mV. So
> this includes the resolution of the ADC, the reference voltage and any gain
> that is applied to the input signal.
> 
> The possible values can be listed in the scale_available attribute.
> 

The reference voltage is in the hardware. 
Should i use a DT entry for the reference voltage? 
Or is it better to use a buildin scale and make it changeable?


> > +	NULL,
> > +};
> > +
> > +static struct attribute_group hx711_attribute_group = {
> > +	.attrs = hx711_attributes,
> > +};
> > +
> > +static const struct iio_info hx711_iio_info = {
> > +	.driver_module		= THIS_MODULE,
> > +	.read_raw		= hx711_read_raw,
> > +	.attrs			= &hx711_attribute_group,
> > +};
> > +
> > +static const struct iio_chan_spec hx711_chan_spec[] = {
> > +	{ .type = IIO_VOLTAGE,
> > +		.info_mask_separate =
> > +			BIT(IIO_CHAN_INFO_RAW),
> 
> Given that there are two separate physical input channels this should be
> expressed here and there should be two IIO channels for the device.
> 

One who is toggling between channel A and B will cause a dummy read
additional to every normal read. 

Should i offer a "toggling mode" which means that after reading
channel A the channel B is selected for the next read and after
reading channel B channel A is selected? Simply expecting the channel
is toggled on every read. If it's not toggled there need to be a dummy 
read, of course. This should be an attribute, right?


> > +	},
> > +};
> > +
> > +static int hx711_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct hx711_data *hx711_data = NULL;
> 
> The = NULL is not needed as it is overwritten a few lines below.
> 
> > +	struct iio_dev *iio;
> > +	int ret = 0;
> 
> Again = 0 no needed.
> 
> > +
> > +	iio = devm_iio_device_alloc(dev, sizeof(struct hx711_data));
> > +	if (!iio) {
> > +		dev_err(dev, "failed to allocate IIO device\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	hx711_data = iio_priv(iio);
> > +	hx711_data->dev = dev;
> > +
> > +	mutex_init(&hx711_data->lock);
> > +
> > +	hx711_data->gpiod_sck = devm_gpiod_get(dev, "sck", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(hx711_data->gpiod_sck)) {
> > +		dev_err(dev, "failed to get sck-gpiod: err=%ld\n",
> > +					PTR_ERR(hx711_data->gpiod_sck));
> > +		return PTR_ERR(hx711_data->gpiod_sck);
> > +	}
> > +
> > +	hx711_data->gpiod_dout = devm_gpiod_get(dev, "dout", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(hx711_data->gpiod_dout)) {
> > +		dev_err(dev, "failed to get dout-gpiod: err=%ld\n",
> > +					PTR_ERR(hx711_data->gpiod_dout));
> > +		return PTR_ERR(hx711_data->gpiod_dout);
> > +	}
> > +
> > +	ret = gpiod_direction_input(hx711_data->gpiod_dout);
> 
> If dout is used as a input GPIO you should request it with GPIOD_IN. In that
> case you can remove the gpiod_direction_input() call.
> 
> > +	if (ret < 0) {
> > +		dev_err(hx711_data->dev, "gpiod_direction_input: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = gpiod_direction_output(hx711_data->gpiod_sck, 0);
> 
> Similar to above. If you want this to be a output GPIO with the default
> value of 0 request it with GPIOD_OUT_LOW.
> 
> > +	if (ret < 0) {
> > +		dev_err(hx711_data->dev, "gpiod_direction_output: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	platform_set_drvdata(pdev, iio);
> 
> There is no matching platform_get_drvdata() so this can probably be removed.
> 
> > +
> > +	iio->name = pdev->name;
> 
> This should be the part name. E.g. "hx711" in this case.
> 
> > +	iio->dev.parent = &pdev->dev;
> > +	iio->info = &hx711_iio_info;
> > +	iio->modes = INDIO_DIRECT_MODE;
> > +	iio->channels = hx711_chan_spec;
> > +	iio->num_channels = ARRAY_SIZE(hx711_chan_spec);
> > +
> > +	return devm_iio_device_register(dev, iio);
> > +}
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v3 2/2] iio: adc: hx711: Add IIO driver for AVIA HX711
  2016-12-20 10:33     ` Andreas Klinger
  (?)
@ 2016-12-20 18:55     ` Lars-Peter Clausen
  -1 siblings, 0 replies; 8+ messages in thread
From: Lars-Peter Clausen @ 2016-12-20 18:55 UTC (permalink / raw)
  To: Andreas Klinger
  Cc: devicetree, linux-iio, linux-kernel, robh+dt, pawel.moll,
	mark.rutland, ijc+devicetree, galak, jic23, knaack.h, pmeerw

On 12/20/2016 11:33 AM, Andreas Klinger wrote:
>>> +static struct attribute *hx711_attributes[] = {
>>> +	&iio_dev_attr_gain.dev_attr.attr,
>>
>> For IIO devices the gain is typically expressed through the scale attribute.
>> Which is kind of the inverse of gain. It would be good if this driver
>> follows this standard notation. The scale is the value of 1LSB in mV. So
>> this includes the resolution of the ADC, the reference voltage and any gain
>> that is applied to the input signal.
>>
>> The possible values can be listed in the scale_available attribute.
>>
> 
> The reference voltage is in the hardware. 
> Should i use a DT entry for the reference voltage? 
> Or is it better to use a buildin scale and make it changeable?

Typically the reference voltage is specified through the devicetree using a
regulator. Have a look at e.g. ad7291.c.

> 
> 
>>> +	NULL,
>>> +};
>>> +
>>> +static struct attribute_group hx711_attribute_group = {
>>> +	.attrs = hx711_attributes,
>>> +};
>>> +
>>> +static const struct iio_info hx711_iio_info = {
>>> +	.driver_module		= THIS_MODULE,
>>> +	.read_raw		= hx711_read_raw,
>>> +	.attrs			= &hx711_attribute_group,
>>> +};
>>> +
>>> +static const struct iio_chan_spec hx711_chan_spec[] = {
>>> +	{ .type = IIO_VOLTAGE,
>>> +		.info_mask_separate =
>>> +			BIT(IIO_CHAN_INFO_RAW),
>>
>> Given that there are two separate physical input channels this should be
>> expressed here and there should be two IIO channels for the device.
>>
> 
> One who is toggling between channel A and B will cause a dummy read
> additional to every normal read. 
> 
> Should i offer a "toggling mode" which means that after reading
> channel A the channel B is selected for the next read and after
> reading channel B channel A is selected? Simply expecting the channel
> is toggled on every read. If it's not toggled there need to be a dummy 
> read, of course. This should be an attribute, right?

I don't think that is necessary. On one hand when doing these kind of single
shot conversions there is often not a predictable pattern on the other hand
such custom device specific attributes are difficult to handle in a generic
framework. We try to establish standard semantics so a generic application
is able to talk to a wide range of devices. When a device implements custom
attributes that becomes more difficult.

And there is also buffered mode. In buffered mode the application can select
the channels which the converter should cycle through and connect a trigger
like a timer to schedule the conversions. If switching between A and B in a
circular way is required I'd recommend to implement this by using buffered mode.

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2016-12-20 18:55 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-14 16:17 [PATCH v3 2/2] iio: adc: hx711: Add IIO driver for AVIA HX711 Andreas Klinger
2016-12-14 16:17 ` Andreas Klinger
2016-12-19 16:28 ` Lars-Peter Clausen
2016-12-19 16:28   ` Lars-Peter Clausen
2016-12-20 10:33   ` Andreas Klinger
2016-12-20 10:33     ` Andreas Klinger
2016-12-20 18:55     ` Lars-Peter Clausen
2016-12-19 20:49 ` Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.