Hello Lars, thank you for the thorough review. I have some questions. See below. Thanks, Andreas Lars-Peter Clausen <lars@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html