Message-ID: <20161220103346.GA1318@imap.1und1.de>
Date:   Tue, 20 Dec 2016 11:33:47 +0100
From:   Andreas Klinger <ak@...klinger.de>
To:     Lars-Peter Clausen <lars@...afoo.de>
Cc:     devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, robh+dt@...nel.org,
        pawel.moll@....com, mark.rutland@....com,
        ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
        jic23@...nel.org, knaack.h@....de, pmeerw@...erw.net
Subject: Re: [PATCH v3 2/2] iio: adc: hx711: Add IIO driver for AVIA HX711

Hello Lars,

thank you for the thorough review.

I have some questions. See below.

Thanks,

Andreas


Lars-Peter Clausen <lars@...afoo.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);
> > +}
> 

Powered by blists - more mailing lists