On Thu, Jun 7, 2018 at 4:09 PM, Stefan Popa <stefan.popa@xxxxxxxxxx> wrote: > The AD5758 is a single channel DAC with 16-bit precision which uses the SPI > interface that operates at clock rates up to 50MHz. > > The output can be configured as voltage or current and is available on a > single terminal. > > Datasheet: > http://www.analog.com/media/en/technical-documentation/data-sheets/ad5758.pdf > +#include <linux/fs.h> > +#include <linux/device.h> > +#include <linux/module.h> > +#include <linux/kernel.h> > +#include <linux/slab.h> > +#include <linux/sysfs.h> > +#include <linux/spi/spi.h> > +#include <linux/delay.h> Can you keep the list in order? > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/property.h> Same, and property.h more generic if you wish than iio. > +/* AD5758_DIGITAL_DIAG_RESULTS */ > +#define AD5758_DIG_DIAG_RES_CAL_MEM_UNREFRESHED_MSK BIT(15) This name way too long. Also, check the rest near to this length and try to shorten them. > +#define AD5758_REG_WRITE(x) (0x80 | ((x) & 0x1F)) Name of macro is misleading. It rather some value preparations than "write". > +/* x^8 + x^2 + x^1 + x^0 */ > +#define AD5758_CRC8_POLY 0x07 Where is it used? > +enum ad5758_output_range { > + AD5758_RANGE_0V_5V, > + AD5758_RANGE_0V_10V, > + AD5758_RANGE_PLUSMINUS_5V, > + AD5758_RANGE_PLUSMINUS_10V, > + AD5758_RANGE_0mA_20mA = 8, Hmm... why it starts from 8? Perhaps some explanation on top of enum? > + AD5758_RANGE_0mA_24mA, > + AD5758_RANGE_4mA_24mA, > + AD5758_RANGE_PLUSMINUS_20mA, > + AD5758_RANGE_PLUSMINUS_24mA, > + AD5758_RANGE_MINUS_1mA_PLUS_22mA, > +}; > +static int ad5758_get_array_index(const int *array, unsigned int size, int val) > +{ > + int i; > + > + for (i = 0; i < size; i++) { > + if (val == array[i]) > + return i; > + } > + > + return -EINVAL; > +} bsearch? (lib/bsearch.c) > +static int ad5758_wait_for_task_complete(struct ad5758_state *st, > + unsigned int reg, > + unsigned int mask) > +{ > + unsigned int timeout; > + int ret; > + > + timeout = 4; > + do { > + ret = ad5758_spi_reg_read(st, reg); > + if (ret < 0) > + return ret; > + > + if (!(ret & mask)) > + return 0; > + > + mdelay(1); No way. CPU would be busy for 1ms at least! > + } while (--timeout); > + > + dev_err(&st->spi->dev, > + "Error reading bit 0x%x in 0x%x register\n", mask, reg); > + > + return -EIO; > +} > +static int ad5758_soft_reset(struct ad5758_state *st) > +{ > + int ret; > + > + ret = ad5758_spi_reg_write(st, AD5758_KEY, AD5758_KEY_CODE_RESET_1); > + if (ret < 0) > + return ret; > + > + ret = ad5758_spi_reg_write(st, AD5758_KEY, AD5758_KEY_CODE_RESET_2); > + > + mdelay(1); Explanation must be given for such a long delay without idling. > + > + return ret; > +} > + if (readval == NULL) { Why not positive check? if (readval) { ... } else { ... } > + ret = ad5758_spi_reg_write(st, reg, writeval); > + } else { > + ret = ad5758_spi_reg_read(st, reg); > + if (ret < 0) { > + mutex_unlock(&st->lock); > + return ret; > + } > + > + *readval = ret; > + ret = 0; > + } > + ret = strtobool(buf, &pwr_down); > + if (ret) > + return ret; kstrtobool(), please. > +static const struct iio_chan_spec_ext_info ad5758_ext_info[] = { > + { > + .name = "powerdown", > + .read = ad5758_read_powerdown, > + .write = ad5758_write_powerdown, > + .shared = IIO_SEPARATE, > + }, > + { }, Terminator is better without comma. > +}; > + st->data[0].d32 = > + cpu_to_be32( > + (AD5758_REG_WRITE(AD5758_DIGITAL_DIAG_CONFIG) << 24) | 0x5C3A); Either do this one line, or introduce a temporary variable for parameter of cpu_to_be32(). > + if (!device_property_read_u32(&st->spi->dev, "adi,range", &tmp)) { Would be better to have ret = device_property_...(); if (ret) { ... } else { ... } if it's optional and if (ret) { ... return -ERRNO; // goto cleanup; etc } > + for (i = 0; i < ARRAY_SIZE(ad5758_min_max_table); i++) { > + if (tmp == ad5758_min_max_table[i].reg) { > + st->out_range = i; > + break; > + } > + } > + > + if (i == ARRAY_SIZE(ad5758_min_max_table)) > + dev_warn(&st->spi->dev, > + "range invalid, using 0V to 5V\n"); > + } else { > + dev_dbg(&st->spi->dev, > + "Missing \"range\" property, using 0V to 5V\n"); > + } > + if (!device_property_read_u32_array(&st->spi->dev, "adi,slew", > + tmparray, 3)) { ... > + } else { > + dev_dbg(&st->spi->dev, > + "Missing \"slew\" property, using 16kHz and 4 LSB\n"); > + } Ditto. > + ad5758_parse_dt(st); No errors? Nothing fatal? -- With Best Regards, Andy Shevchenko -- 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
