Hi,
Thanks for the patch, very good work. Looks mostly good, a few comments inline.
Also please run the patch through scripts/checkpatch.pl there are a couple
of whitespace issues here and there (extra space at the end of the line, etc.).
On 08/24/2016 03:53 PM, Gwenhael Goavec-Merou wrote:
> From: Gwenhael <gwe@xxxxxxxxxxxxxx>
>
> Add support for Analog Devices AD8801/AD8803, 8 channels 8bits, Digital to Analog
> converters.
>
> Signed-off-by: Gwenhael <gwe@xxxxxxxxxxxxxx>
Signed-off-by and from usually should be your full name.
[...]
> +
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
The driver does not seem to make use of the of.h or of_gpio.h APIs
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/sysfs.h>
> +#define AD8801_CHANNEL(chan) { \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .output = 1, \
> + .channel = (chan + 1), \
Channel numbers should start at 0, even if the hardware device starts
numbering at 1. There is the datasheet_field where you can store the name
listed in the datasheet. E.g. in this case "O1", "O2", ...
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> + .address = chan, \
> +}
> +
> +const struct iio_chan_spec ad8801_channels[] = {
Should be static since it is not used outside of the driver.
> + AD8801_CHANNEL(0),
> + AD8801_CHANNEL(1),
> + AD8801_CHANNEL(2),
> + AD8801_CHANNEL(3),
> + AD8801_CHANNEL(4),
> + AD8801_CHANNEL(5),
> + AD8801_CHANNEL(6),
> + AD8801_CHANNEL(7),
> +};
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
