Hello Lars,
thank you for the thorough review.
I have some questions. See below.
Thanks,
Andreas
Lars-Peter Clausen <l...@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);
> > +}
>
