| Message ID | 20211111110043.101891-2-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | LTC2688 support | expand |
On Thu, Nov 11, 2021 at 1:01 PM Nuno Sá <nuno.sa@analog.com> wrote: > > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated > precission reference. It is guaranteed monotonic and has built in precision > rail-to-rail output buffers that can source or sink up to 20 mA. It's in good shape, several comments below, though. ... > +enum { > + LTC2688_SPAN_RANGE_0V_5V, > + LTC2688_SPAN_RANGE_M10V_10V, > + LTC2688_SPAN_RANGE_M15V_15V, > + /* > + * These 2 were deliberately re-arranged to be the last items as they > + * have the same fs and we will just return 1 in read_available. WTH "fs" mean? > + */ > + LTC2688_SPAN_RANGE_0V_10V, > + LTC2688_SPAN_RANGE_M5V_5V, > + LTC2688_SPAN_RANGE_MAX > +}; ... > +static const int ltc2688_off_tbl[LTC2688_SPAN_RANGE_MAX] = { > + [LTC2688_SPAN_RANGE_0V_5V] = 0, > + [LTC2688_SPAN_RANGE_M10V_10V] = -32768, > + [LTC2688_SPAN_RANGE_M15V_15V] = -32768, > + [LTC2688_SPAN_RANGE_0V_10V] = 0, > + [LTC2688_SPAN_RANGE_M5V_5V] = -32768 + Comma Isn't it more natural to move them up by two lines? > +}; > + > +static const int ltc2688_reg_val[LTC2688_SPAN_RANGE_MAX] = { > + [LTC2688_SPAN_RANGE_0V_5V] = 0, > + [LTC2688_SPAN_RANGE_M10V_10V] = 3, > + [LTC2688_SPAN_RANGE_M15V_15V] = 4, > + [LTC2688_SPAN_RANGE_0V_10V] = 1, > + [LTC2688_SPAN_RANGE_M5V_5V] = 2 As per above. > +}; ... > +static int ltc2688_span_set(const struct ltc2688_state *st, int c, int span) > +{ > + const struct ltc2688_chan *chan = &st->channels[c]; > + u32 i = ARRAY_SIZE(chan->span_tbl); > + int off = ltc2688_offset_avail[chan->offset_idx]; > + while (i--) { Can ARRAY_SIZE() give 0? If not, do {} while (--i); looks more natural. > + if (span == chan->span_tbl[i][1]) { > + /* > + * The selected scale needs to fit the current offset. > + * Note that for [0V 10V] and [-5V 5V], as the scale is > + * the same, we will always succeed here. Hence if > + * someone really wants [0 10V] and does not set the > + * offset to 0, then :man-shrugging: > + */ > + if (off == ltc2688_off_tbl[i]) { > + break; > + } else if (i < LTC2688_SPAN_RANGE_0V_10V) { Redundant 'else'. > + /* > + * At this point, only one offset is valid so we > + * already can assume error. > + */ > + dev_err(&st->spi->dev, "Offset(%d) not valid for current scale(0.%09d)\n", > + off, span); > + return -EPERM; > + } > + continue; Redundant, see below. > + } > + if (!i) { > + dev_err(&st->spi->dev, "span(%d) not available\n", span); > + return -EINVAL; > + } This is invariant to the loop. > + } > + > + return regmap_update_bits(st->regmap, LTC2688_CMD_CH_SETTING(c), > + LTC2688_CH_SPAN_MSK, > + FIELD_PREP(LTC2688_CH_SPAN_MSK, ltc2688_reg_val[i])); > +}; ... > + if (off != ltc2688_offset_avail[0] && off != ltc2688_offset_avail[1]) > + return -EINVAL; Extra condition, See below. > + if (off == ltc2688_offset_avail[0]) > + chan->offset_idx = 0; > + else else if > + chan->offset_idx = 1; else return -EINVAL; ... > + ret = regmap_read(st->regmap, reg, ®val); > + if (ret < 0) Is it the first time you tested explicitly for a negative result? > + return ret; ... > + return sprintf(buf, "%u\n", !!(regval & BIT(chan->channel))); sysfs_emit() ? > +} ... > + struct ltc2688_state *st = iio_priv(indio_dev); > + bool en; > + int ret; > + u32 val = 0, reg; Reversed xmas tree order ? > + ret = strtobool(buf, &en); kstrtobool() ... > + if (ret) > + return ret; > + break; What does this mean? ... > + "4", > + "8", > + "16", > + "32", > + "64", One line? ... > + "0", > + "90", > + "180", > + "270", Ditto. ... > +enum { > + LTC2688_CHAN_MODE_TOGGLE, > + LTC2688_CHAN_MODE_DITHER + Comma. > +}; ... > +static const char * const ltc2688_clk_names[LTC2688_CHAN_TD_MAX] = { > + "TGP1", "TGP2", "TGP3" In this notation, + comma. > +}; ... > + for_each_set_bit(bit, &mask, ARRAY_SIZE(ltc2688_clk_names)) { > + struct clk *clk; + blank line > + /* > + * If a TGP pin is set, then we need to provide a valid clock at > + * the pin. > + */ > + } ... > + fwnode_for_each_available_child_node(fwnode, child) { > + struct ltc2688_chan *chan; > + > + ret = fwnode_property_read_u32(child, "reg", ®); > + if (ret) { > + fwnode_handle_put(child); > + return dev_err_probe(&st->spi->dev, ret, > + "Failed to get reg property\n"); One line. > + } else if (reg >= LTC2688_DAC_CHANNELS) { Redundant 'else'. > + fwnode_handle_put(child); > + return dev_err_probe(&st->spi->dev, -EINVAL, > + "reg >= %d\n", LTC2688_DAC_CHANNELS); The message is cryptic. Decrypt it for the user. > + } > + > + chan = &st->channels[reg]; > + ret = fwnode_property_read_u32(child, "adi,mode", &mode); > + if (!ret) { > + if (mode > LTC2688_CHAN_MODE_DITHER) { > + fwnode_handle_put(child); > + return dev_err_probe(&st->spi->dev, -EINVAL, > + "chan mode inv value(%d)\n", > + mode); > + } > + > + chan->dither_toggle = true; > + if (mode == LTC2688_CHAN_MODE_TOGGLE) { > + ltc2688_channels[reg].ext_info = ltc2688_toggle_ext_info; > + } else { > + ltc2688_channels[reg].ext_info = ltc2688_dither_ext_info; > + /* enable dither mode */ > + mask = LTC2688_CH_MODE_MSK; > + val = BIT(11); > + } > + } > + > + ret = fwnode_property_read_u32(child, "adi,toggle-dither-input", > + &clk_input); > + if (!ret) { if (ret) { if ... } else { ... } > + if (clk_input > LTC2688_CHAN_TD_TGP3) { > + fwnode_handle_put(child); > + return dev_err_probe(&st->spi->dev, -EINVAL, > + "toggle-dither-input inv value(%d)\n", > + clk_input); > + } > + > + clk_msk |= BIT(clk_input); > + mask |= LTC2688_CH_TD_SEL_MSK; > + /* > + * 0 means software toggle which we are not supporting > + * for now. Hence the +1 In all multi-line comments miseed (grammatical) period. > + */ > + val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1); > + } else if (chan->dither_toggle) { > + /* > + * As sw_toggle is not supported, we need to make sure > + * a valid input is selected if toggle/dither mode is > + * requested > + */ > + return dev_err_probe(&st->spi->dev, -EINVAL, > + "toggle-dither set but no toggle-dither-input\n"); > + } > + chan->overrange = fwnode_property_read_bool(child, > + "adi,overrange"); One line? > + if (chan->overrange) { > + val |= LTC2688_CH_OVERRANGE_MSK; > + mask |= BIT(3); > + } > + > + if (!mask) > + continue; > + > + ret = regmap_update_bits(st->regmap, > + LTC2688_CMD_CH_SETTING(reg), mask, > + val); > + if (ret) { > + fwnode_handle_put(child); > + return dev_err_probe(&st->spi->dev, -EINVAL, > + "failed to set chan settings\n"); > + } > + > + mask = 0; > + val = 0; > + } ... > + int fs = ltc2688_span_helper[idx][1] - ltc2688_span_helper[idx][0]; > + > + if (chan->overrange) > + fs = mult_frac(fs, 105, 100); > + > + return fs; if (...) return ... ? ... > + u32 c, i; In one case you use int or unsigned int if I'm not mistaken, here out of a sudden u32. Why? Please, be consistent over the code. ... > + /* we will return IIO_VAL_INT_PLUS_NANO */ > + s = DIV_ROUND_CLOSEST_ULL(st->vref * fs * 1000000000ULL, > + 4096 * 1 << 16); Can you make use of one of the SI prefixes here? (See unit.h) ... > + gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", GPIOD_OUT_HIGH); > + if (IS_ERR(gpio)) > + return dev_err_probe(&st->spi->dev, PTR_ERR(gpio), > + "Failed to get reset gpio"); > + Redundant blank line. > + if (gpio) { > + usleep_range(1000, 1200); > + /* bring device out of reset */ > + gpiod_set_value_cansleep(gpio, 0); > + } else { I'm wondering why 'else'? Can't it be both? Why not? > + ret = regmap_update_bits(st->regmap, LTC2688_CMD_CONFIG_REG, > + LTC2688_CONFIG_RST, > + LTC2688_CONFIG_RST); > + if (ret < 0) > + return ret; > + } > + > + usleep_range(10000, 12000); + blank line, so the reader won't be confused about the function of this sleep. > + ret = ltc2688_channel_config(st); > + if (ret < 0) > + return ret; > + > + if (vref) { > + ret = regmap_update_bits(st->regmap, LTC2688_CMD_CONFIG_REG, > + LTC2688_CONFIG_EXT_REF, BIT(1)); > + if (ret < 0) These ' < 0' parts are inconsistent over the code. > + return ret; > + } > + > + ltc2688_span_avail_compute(st); > + return 0; > +} ... > + /* ignoring the no op command */ > + .max_register = U16_MAX - 1 Really?! Have you ever considered what burden you bring with this to one who accidentally or on purpose tries to dump registers via debugfs? ... > + st->regmap = devm_regmap_init(&spi->dev, NULL, st, <c2688_regmap_config); I'm wondering why it's not a regmap SPI? > + if (IS_ERR(st->regmap)) > + return dev_err_probe(&spi->dev, PTR_ERR(st->regmap), > + "Failed to init regmap"); ... > + ret = ltc2688_regulator_supply_get(st, "vcc"); > + if (ret) > + return ret; > + > + ret = ltc2688_regulator_supply_get(st, "iovcc"); > + if (ret) > + return ret; Can bulk regulators be used? ... > + st->vref = ret / 1000; Make use of unit.h -- With Best Regards, Andy Shevchenko
Hi Andy, Thanks for the preliminary review... > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Thursday, November 11, 2021 2:49 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron > <jic23@kernel.org> > Subject: Re: [RFC PATCH 1/1] iio: dac: add support for ltc2688 > > [External] > > On Thu, Nov 11, 2021 at 1:01 PM Nuno Sá <nuno.sa@analog.com> > wrote: > > > > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated > > precission reference. It is guaranteed monotonic and has built in > > precision > > > rail-to-rail output buffers that can source or sink up to 20 mA. > > It's in good shape, several comments below, though. > > ... > > > +enum { > > + LTC2688_SPAN_RANGE_0V_5V, > > + LTC2688_SPAN_RANGE_M10V_10V, > > + LTC2688_SPAN_RANGE_M15V_15V, > > + /* > > + * These 2 were deliberately re-arranged to be the last items as > they > > + * have the same fs and we will just return 1 in read_available. > > WTH "fs" mean? full scale... will spell it out > > + */ > > + LTC2688_SPAN_RANGE_0V_10V, > > + LTC2688_SPAN_RANGE_M5V_5V, > > + LTC2688_SPAN_RANGE_MAX > > +}; > > ... > > > +static const int ltc2688_off_tbl[LTC2688_SPAN_RANGE_MAX] = { > > + [LTC2688_SPAN_RANGE_0V_5V] = 0, > > + [LTC2688_SPAN_RANGE_M10V_10V] = -32768, > > + [LTC2688_SPAN_RANGE_M15V_15V] = -32768, > > > + [LTC2688_SPAN_RANGE_0V_10V] = 0, > > + [LTC2688_SPAN_RANGE_M5V_5V] = -32768 > > + Comma > > Isn't it more natural to move them up by two lines? There's a reason for this to be ordered like this. Anyways, as I said in the cover letter I will probably remove all of these things used to validate scale + offset pairs. I think is a bit overdone... > > +}; > > + > > +static const int ltc2688_reg_val[LTC2688_SPAN_RANGE_MAX] = { > > + [LTC2688_SPAN_RANGE_0V_5V] = 0, > > + [LTC2688_SPAN_RANGE_M10V_10V] = 3, > > + [LTC2688_SPAN_RANGE_M15V_15V] = 4, > > + [LTC2688_SPAN_RANGE_0V_10V] = 1, > > + [LTC2688_SPAN_RANGE_M5V_5V] = 2 > > As per above. > > > +}; > > ... > > > +static int ltc2688_span_set(const struct ltc2688_state *st, int c, int > span) > > +{ > > + const struct ltc2688_chan *chan = &st->channels[c]; > > + u32 i = ARRAY_SIZE(chan->span_tbl); > > + int off = ltc2688_offset_avail[chan->offset_idx]; > > > + while (i--) { > > Can ARRAY_SIZE() give 0? nope > If not, do {} while (--i); looks more natural. can do like that > > + if (span == chan->span_tbl[i][1]) { > > + /* > > + * The selected scale needs to fit the current offset. > > + * Note that for [0V 10V] and [-5V 5V], as the scale is > > + * the same, we will always succeed here. Hence if > > + * someone really wants [0 10V] and does not set the > > + * offset to 0, then :man-shrugging: > > + */ > > + if (off == ltc2688_off_tbl[i]) { > > + break; > > > + } else if (i < LTC2688_SPAN_RANGE_0V_10V) { > > Redundant 'else'. Why? Note that I really (where possible) want to make a distinction between the errors... Again, I will probably remove this part and only match 'if (span == chan->span_tbl[i][1])' > > + /* > > + * At this point, only one offset is valid so we > > + * already can assume error. > > + */ > > + dev_err(&st->spi->dev, "Offset(%d) not valid for > current scale(0.%09d)\n", > > + off, span); > > + return -EPERM; > > + } > > > + continue; > > Redundant, see below. true, but at this point I already know that doing the next if (!i) is not really needed. But yeah, less lines of code > > > + } > > > + if (!i) { > > + dev_err(&st->spi->dev, "span(%d) not available\n", > span); > > + return -EINVAL; > > + } > > This is invariant to the loop. > > > + } > > + > > + return regmap_update_bits(st->regmap, > LTC2688_CMD_CH_SETTING(c), > > + LTC2688_CH_SPAN_MSK, > > + FIELD_PREP(LTC2688_CH_SPAN_MSK, > ltc2688_reg_val[i])); > > +}; > > ... > > > + if (off != ltc2688_offset_avail[0] && off != > ltc2688_offset_avail[1]) > > + return -EINVAL; > > Extra condition, See below. > > > + if (off == ltc2688_offset_avail[0]) > > + chan->offset_idx = 0; > > + else > > else if > > > + chan->offset_idx = 1; > > else > return -EINVAL; sure... > ... > > > + ret = regmap_read(st->regmap, reg, ®val); > > + if (ret < 0) > > Is it the first time you tested explicitly for a negative result? should not be like this... > > + return ret; > > ... > > > + return sprintf(buf, "%u\n", !!(regval & BIT(chan->channel))); > > sysfs_emit() ? definetly... > > +} > > ... > > > + struct ltc2688_state *st = iio_priv(indio_dev); > > + bool en; > > + int ret; > > + u32 val = 0, reg; > > Reversed xmas tree order ? sure > > > + ret = strtobool(buf, &en); > > kstrtobool() sure > > ... > > > + if (ret) > > + return ret; > > + break; > > What does this mean? means I was sleeping... > ... > > > + "4", > > + "8", > > + "16", > > + "32", > > + "64", > > One line? yes > > ... > > > + "0", > > + "90", > > + "180", > > + "270", > > Ditto. > > ... > > > +enum { > > + LTC2688_CHAN_MODE_TOGGLE, > > + LTC2688_CHAN_MODE_DITHER > > + Comma. > > > +}; > > ... > > > +static const char * const > ltc2688_clk_names[LTC2688_CHAN_TD_MAX] = { > > + "TGP1", "TGP2", "TGP3" > > In this notation, + comma. > > > +}; > > ... > > > + for_each_set_bit(bit, &mask, ARRAY_SIZE(ltc2688_clk_names)) > { > > + struct clk *clk; > > + blank line > > > + /* > > + * If a TGP pin is set, then we need to provide a valid clock at > > + * the pin. > > + */ > > > + } > > ... > > > + fwnode_for_each_available_child_node(fwnode, child) { > > + struct ltc2688_chan *chan; > > + > > + ret = fwnode_property_read_u32(child, "reg", ®); > > + if (ret) { > > + fwnode_handle_put(child); > > > + return dev_err_probe(&st->spi->dev, ret, > > + "Failed to get reg property\n"); > > One line. > > > + } else if (reg >= LTC2688_DAC_CHANNELS) { > > Redundant 'else'. Do you mean? if (ret) ... if (reg >= LTC2688_DAC_CHANNELS) .... > > + fwnode_handle_put(child); > > + return dev_err_probe(&st->spi->dev, -EINVAL, > > > + "reg >= %d\n", LTC2688_DAC_CHANNELS); > > The message is cryptic. Decrypt it for the user. > > > + } > > + > > + chan = &st->channels[reg]; > > + ret = fwnode_property_read_u32(child, "adi,mode", > &mode); > > + if (!ret) { > > + if (mode > LTC2688_CHAN_MODE_DITHER) { > > + fwnode_handle_put(child); > > + return dev_err_probe(&st->spi->dev, -EINVAL, > > + "chan mode inv value(%d)\n", > > + mode); > > + } > > + > > + chan->dither_toggle = true; > > + if (mode == LTC2688_CHAN_MODE_TOGGLE) { > > + ltc2688_channels[reg].ext_info = > ltc2688_toggle_ext_info; > > + } else { > > + ltc2688_channels[reg].ext_info = > ltc2688_dither_ext_info; > > + /* enable dither mode */ > > + mask = LTC2688_CH_MODE_MSK; > > + val = BIT(11); > > + } > > + } > > + > > + ret = fwnode_property_read_u32(child, "adi,toggle-dither- > input", > > + &clk_input); > > > + if (!ret) { > > if (ret) { > if ... > } else { > ... > } sure > > + if (clk_input > LTC2688_CHAN_TD_TGP3) { > > + fwnode_handle_put(child); > > + return dev_err_probe(&st->spi->dev, -EINVAL, > > + "toggle-dither-input inv value(%d)\n", > > + clk_input); > > + } > > + > > + clk_msk |= BIT(clk_input); > > + mask |= LTC2688_CH_TD_SEL_MSK; > > + /* > > + * 0 means software toggle which we are not supporting > > + * for now. Hence the +1 > > In all multi-line comments miseed (grammatical) period. > > > + */ > > + val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input > + 1); > > + } else if (chan->dither_toggle) { > > + /* > > + * As sw_toggle is not supported, we need to make sure > > + * a valid input is selected if toggle/dither mode is > > + * requested > > + */ > > + return dev_err_probe(&st->spi->dev, -EINVAL, > > + "toggle-dither set but no toggle-dither- > input\n"); > > + } > > > + chan->overrange = fwnode_property_read_bool(child, > > + "adi,overrange"); > > One line? It will pass the 80 col limit. AFAIR, Jonathan prefers to keep it when it does not hurt readability... > > + if (chan->overrange) { > > + val |= LTC2688_CH_OVERRANGE_MSK; > > + mask |= BIT(3); > > + } > > + > > + if (!mask) > > + continue; > > + > > + ret = regmap_update_bits(st->regmap, > > + LTC2688_CMD_CH_SETTING(reg), mask, > > + val); > > + if (ret) { > > + fwnode_handle_put(child); > > + return dev_err_probe(&st->spi->dev, -EINVAL, > > + "failed to set chan settings\n"); > > + } > > + > > + mask = 0; > > + val = 0; > > + } > > ... > > > + int fs = ltc2688_span_helper[idx][1] - > ltc2688_span_helper[idx][0]; > > + > > + if (chan->overrange) > > + fs = mult_frac(fs, 105, 100); > > + > > + return fs; > > if (...) > return ... > ? Yes... > ... > > > + u32 c, i; > > In one case you use int or unsigned int if I'm not mistaken, here out > of a sudden u32. Why? Please, be consistent over the code. I will check that... > ... > > > + /* we will return IIO_VAL_INT_PLUS_NANO */ > > + s = DIV_ROUND_CLOSEST_ULL(st->vref * fs * > 1000000000ULL, > > + 4096 * 1 << 16); > > Can you make use of one of the SI prefixes here? (See unit.h) Sure > ... > > > + gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", > GPIOD_OUT_HIGH); > > + if (IS_ERR(gpio)) > > + return dev_err_probe(&st->spi->dev, PTR_ERR(gpio), > > + "Failed to get reset gpio"); > > > + > > Redundant blank line. > > > + if (gpio) { > > + usleep_range(1000, 1200); > > + /* bring device out of reset */ > > + gpiod_set_value_cansleep(gpio, 0); > > > + } else { > > I'm wondering why 'else'? Can't it be both? Why not? Well, if we have the reset pin, then we reset using it. If there's no pin, we use the SW reset. It's a common pattern that we already use for example in the ADIS devices. > > + ret = regmap_update_bits(st->regmap, > LTC2688_CMD_CONFIG_REG, > > + LTC2688_CONFIG_RST, > > + LTC2688_CONFIG_RST); > > + if (ret < 0) > > + return ret; > > + } > > + > > + usleep_range(10000, 12000); > > + blank line, so the reader won't be confused about the function of > this sleep. > > > + ret = ltc2688_channel_config(st); > > + if (ret < 0) > > + return ret; > > + > > + if (vref) { > > + ret = regmap_update_bits(st->regmap, > LTC2688_CMD_CONFIG_REG, > > + LTC2688_CONFIG_EXT_REF, BIT(1)); > > + if (ret < 0) > > These ' < 0' parts are inconsistent over the code. Yes, I will fix that... > > + return ret; > > + } > > + > > + ltc2688_span_avail_compute(st); > > + return 0; > > +} > > ... > > > + /* ignoring the no op command */ > > + .max_register = U16_MAX - 1 > > Really?! Have you ever considered what burden you bring with this to > one who accidentally or on purpose tries to dump registers via > debugfs? Ups, this is completely wrong!! It should be U8_MAX... > ... > > > + st->regmap = devm_regmap_init(&spi->dev, NULL, st, > <c2688_regmap_config); > > I'm wondering why it's not a regmap SPI? The problem is on the read side... In the first transfer we write the command/register to read, then we need to release the CS pin so that the device executes the command, and only then we read the data. AFAIK, the regmap spi implementation won't work like this. I think CS is kept asserted the whole time... > > + if (IS_ERR(st->regmap)) > > + return dev_err_probe(&spi->dev, PTR_ERR(st->regmap), > > + "Failed to init regmap"); > > ... > > > + ret = ltc2688_regulator_supply_get(st, "vcc"); > > + if (ret) > > + return ret; > > + > > + ret = ltc2688_regulator_supply_get(st, "iovcc"); > > + if (ret) > > + return ret; > > Can bulk regulators be used? Hmm, I need to check but I think so... There's no ordering requirement here > ... > > > + st->vref = ret / 1000; > > Make use of unit.h > - Nuno Sá
On Thu, Nov 11, 2021 at 4:30 PM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Sent: Thursday, November 11, 2021 2:49 PM > > On Thu, Nov 11, 2021 at 1:01 PM Nuno Sá <nuno.sa@analog.com> > > wrote: ... > > > +static const int ltc2688_off_tbl[LTC2688_SPAN_RANGE_MAX] = { > > > + [LTC2688_SPAN_RANGE_0V_5V] = 0, > > > + [LTC2688_SPAN_RANGE_M10V_10V] = -32768, > > > + [LTC2688_SPAN_RANGE_M15V_15V] = -32768, > > > > > + [LTC2688_SPAN_RANGE_0V_10V] = 0, > > > + [LTC2688_SPAN_RANGE_M5V_5V] = -32768 > > > > + Comma > > > > Isn't it more natural to move them up by two lines? > > There's a reason for this to be ordered like this. I understand that for enum, but here it doesn't make any sense to me since you are addressing them by indices. > Anyways, > as I said in the cover letter I will probably remove all of these things > used to validate scale + offset pairs. I think is a bit overdone... > > > +}; ... > > > + if (off == ltc2688_off_tbl[i]) { > > > + break; > > > > > + } else if (i < LTC2688_SPAN_RANGE_0V_10V) { > > > > Redundant 'else'. > > Why? Note that I really (where possible) want to make a distinction > between the errors... while () { if () ... break; } else { ... } } Isn't it obvious? Same with if () return else ... > > > + } ... > > > + ret = fwnode_property_read_u32(child, "reg", ®); > > > + if (ret) { > > > + fwnode_handle_put(child); > > > > > + return dev_err_probe(&st->spi->dev, ret, > > > + "Failed to get reg property\n"); > > > > One line. > > > > > + } else if (reg >= LTC2688_DAC_CHANNELS) { > > > > Redundant 'else'. > > Do you mean? > > if (ret) > ... > > if (reg >= LTC2688_DAC_CHANNELS) > .... Yes. > > > + } ... > > > + chan->overrange = fwnode_property_read_bool(child, > > > + "adi,overrange"); > > > > One line? > > It will pass the 80 col limit. AFAIR, Jonathan prefers to keep it when it > does not hurt readability... I believe it will increase readability being located on one line. ... > > > + if (gpio) { > > > + usleep_range(1000, 1200); > > > + /* bring device out of reset */ > > > + gpiod_set_value_cansleep(gpio, 0); > > > > > + } else { > > > > I'm wondering why 'else'? Can't it be both? Why not? > > Well, if we have the reset pin, then we reset using it. If there's > no pin, we use the SW reset. It's a common pattern that we already use > for example in the ADIS devices. Perhaps a comment in the code is needed. > > > + ret = regmap_update_bits(st->regmap, > > LTC2688_CMD_CONFIG_REG, > > > + LTC2688_CONFIG_RST, > > > + LTC2688_CONFIG_RST); > > > + if (ret < 0) > > > + return ret; > > > + } ... > > > + /* ignoring the no op command */ > > > + .max_register = U16_MAX - 1 > > > > Really?! Have you ever considered what burden you bring with this to > > one who accidentally or on purpose tries to dump registers via > > debugfs? > > Ups, this is completely wrong!! It should be U8_MAX... Ah, that explains! ... > > > + st->regmap = devm_regmap_init(&spi->dev, NULL, st, > > <c2688_regmap_config); > > > > I'm wondering why it's not a regmap SPI? > > The problem is on the read side... In the first transfer we write the command/register > to read, then we need to release the CS pin so that the device executes the command, > and only then we read the data. AFAIK, the regmap spi implementation won't work like > this. I think CS is kept asserted the whole time... I believe it's configurable, no? Like the cs_change flag somewhere. Can you double check?
> -----Original Message----- > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Sent: Thursday, November 11, 2021 3:42 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron > <jic23@kernel.org> > Subject: Re: [RFC PATCH 1/1] iio: dac: add support for ltc2688 > > [External] > > On Thu, Nov 11, 2021 at 4:30 PM Sa, Nuno <Nuno.Sa@analog.com> > wrote: > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > > Sent: Thursday, November 11, 2021 2:49 PM > > > On Thu, Nov 11, 2021 at 1:01 PM Nuno Sá <nuno.sa@analog.com> > > > wrote: > > ... > > > > > +static const int ltc2688_off_tbl[LTC2688_SPAN_RANGE_MAX] = > { > > > > + [LTC2688_SPAN_RANGE_0V_5V] = 0, > > > > + [LTC2688_SPAN_RANGE_M10V_10V] = -32768, > > > > + [LTC2688_SPAN_RANGE_M15V_15V] = -32768, > > > > > > > + [LTC2688_SPAN_RANGE_0V_10V] = 0, > > > > + [LTC2688_SPAN_RANGE_M5V_5V] = -32768 > > > > > > + Comma > > > > > > Isn't it more natural to move them up by two lines? > > > > There's a reason for this to be ordered like this. > > I understand that for enum, but here it doesn't make any sense to me > since you are addressing them by indices. Yeah, right... If we keep this, I will do as you suggested > > Anyways, > > as I said in the cover letter I will probably remove all of these things > > used to validate scale + offset pairs. I think is a bit overdone... > > > > > +}; > > ... > > > > > + if (off == ltc2688_off_tbl[i]) { > > > > + break; > > > > > > > + } else if (i < LTC2688_SPAN_RANGE_0V_10V) { > > > > > > Redundant 'else'. > > > > Why? Note that I really (where possible) want to make a distinction > > between the errors... > > while () { > if () > ... > break; > } else { > ... > } > } > > Isn't it obvious? Ah ok, I see know what you mean (and yes, it is obvious; for my shame)... if () break if () return -EPERM As we do the break, the else becomes irrelevant > Same with > > if () > return > else > ... yeah yeah... > > > > + } > > ... > > > > > + ret = fwnode_property_read_u32(child, "reg", ®); > > > > + if (ret) { > > > > + fwnode_handle_put(child); > > > > > > > + return dev_err_probe(&st->spi->dev, ret, > > > > + "Failed to get reg property\n"); > > > > > > One line. > > > > > > > + } else if (reg >= LTC2688_DAC_CHANNELS) { > > > > > > Redundant 'else'. > > > > Do you mean? > > > > if (ret) > > ... > > > > if (reg >= LTC2688_DAC_CHANNELS) > > .... > > Yes. > > > > > + } > > ... > > > > > + chan->overrange = fwnode_property_read_bool(child, > > > > + "adi,overrange"); > > > > > > One line? > > > > It will pass the 80 col limit. AFAIR, Jonathan prefers to keep it when it > > does not hurt readability... > > I believe it will increase readability being located on one line. I mean, this is perfectly aligned with the open "(", so it's a pretty normal pattern. Anyways, I'm more than happy to move this into a one liner and just use the 100 limit. But let's see what Jonathan has to say because I do not want to move back and forward... > ... > > > > > + if (gpio) { > > > > + usleep_range(1000, 1200); > > > > + /* bring device out of reset */ > > > > + gpiod_set_value_cansleep(gpio, 0); > > > > > > > + } else { > > > > > > I'm wondering why 'else'? Can't it be both? Why not? > > > > Well, if we have the reset pin, then we reset using it. If there's > > no pin, we use the SW reset. It's a common pattern that we already > use > > for example in the ADIS devices. > > Perhaps a comment in the code is needed. can do that... > > > > + ret = regmap_update_bits(st->regmap, > > > LTC2688_CMD_CONFIG_REG, > > > > + LTC2688_CONFIG_RST, > > > > + LTC2688_CONFIG_RST); > > > > + if (ret < 0) > > > > + return ret; > > > > + } > > ... > > > > > + /* ignoring the no op command */ > > > > + .max_register = U16_MAX - 1 > > > > > > Really?! Have you ever considered what burden you bring with this > to > > > one who accidentally or on purpose tries to dump registers via > > > debugfs? > > > > Ups, this is completely wrong!! It should be U8_MAX... > > Ah, that explains! > > ... > > > > > + st->regmap = devm_regmap_init(&spi->dev, NULL, st, > > > <c2688_regmap_config); > > > > > > I'm wondering why it's not a regmap SPI? > > > > The problem is on the read side... In the first transfer we write the > command/register > > to read, then we need to release the CS pin so that the device > executes the command, > > and only then we read the data. AFAIK, the regmap spi > implementation won't work like > > this. I think CS is kept asserted the whole time... > > I believe it's configurable, no? Like the cs_change flag somewhere. > Can you double check? Don't think we can... The read part just calls: https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-spi.c#L98 and has no control over the spi transfer bits... - Nuno Sá
> > > > > > > + chan->overrange = fwnode_property_read_bool(child, > > > > > + "adi,overrange"); > > > > > > > > One line? > > > > > > It will pass the 80 col limit. AFAIR, Jonathan prefers to keep it when it > > > does not hurt readability... > > > > I believe it will increase readability being located on one line. > > I mean, this is perfectly aligned with the open "(", so it's a pretty > normal pattern. Anyways, I'm more than happy to move this into a one > liner and just use the 100 limit. But let's see what Jonathan has to say > because I do not want to move back and forward... Here it happens to be particularly ugly because of the short first parameter, so I'm fine with a longer line for this one. > > > > > + st->regmap = devm_regmap_init(&spi->dev, NULL, st, > > > > <c2688_regmap_config); > > > > > > > > I'm wondering why it's not a regmap SPI? > > > > > > The problem is on the read side... In the first transfer we write the > > command/register > > > to read, then we need to release the CS pin so that the device > > executes the command, > > > and only then we read the data. AFAIK, the regmap spi > > implementation won't work like > > > this. I think CS is kept asserted the whole time... > > > > I believe it's configurable, no? Like the cs_change flag somewhere. > > Can you double check? > > Don't think we can... The read part just calls: > https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap-spi.c#L98 > and has no control over the spi transfer bits... Feature to add then or a custom regmap_bus if you want to keep it in the driver. J > > - Nuno Sá
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Friday, November 12, 2021 4:23 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>; linux-iio > <linux-iio@vger.kernel.org> > Subject: Re: [RFC PATCH 1/1] iio: dac: add support for ltc2688 > > [External] > > > > > > > > > > > + chan->overrange = > fwnode_property_read_bool(child, > > > > > > + "adi,overrange"); > > > > > > > > > > One line? > > > > > > > > It will pass the 80 col limit. AFAIR, Jonathan prefers to keep it > when it > > > > does not hurt readability... > > > > > > I believe it will increase readability being located on one line. > > > > I mean, this is perfectly aligned with the open "(", so it's a pretty > > normal pattern. Anyways, I'm more than happy to move this into a > one > > liner and just use the 100 limit. But let's see what Jonathan has to say > > because I do not want to move back and forward... > > Here it happens to be particularly ugly because of the short first > parameter, > so I'm fine with a longer line for this one. > Ok then... > > > > > > > + st->regmap = devm_regmap_init(&spi->dev, NULL, st, > > > > > <c2688_regmap_config); > > > > > > > > > > I'm wondering why it's not a regmap SPI? > > > > > > > > The problem is on the read side... In the first transfer we write > the > > > command/register > > > > to read, then we need to release the CS pin so that the device > > > executes the command, > > > > and only then we read the data. AFAIK, the regmap spi > > > implementation won't work like > > > > this. I think CS is kept asserted the whole time... > > > > > > I believe it's configurable, no? Like the cs_change flag somewhere. > > > Can you double check? > > > > Don't think we can... The read part just calls: > > > https://urldefense.com/v3/__https://elixir.bootlin.com/linux/latest/s > ource/drivers/base/regmap/regmap- > spi.c*L98__;Iw!!A3Ni8CS0y2Y!orMauLgKQzcdnc3Mh7DBQY9nGq9sgf8jl > KRBR8IOHhKMT2tdkLj9AyvIHDPYmA$ > > and has no control over the spi transfer bits... > > Feature to add then or a custom regmap_bus if you want to keep it in > the driver. Hmm I see what you mean with the custom bus. We can actually make use of more regmap infrastructure if I just define my regmap_bus in the driver. Thanks for the tip! - Nuno Sá
diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c new file mode 100644 index 000000000000..b394701b044e --- /dev/null +++ b/drivers/iio/dac/ltc2688.c @@ -0,0 +1,995 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * LTC2688 16 channel, 16 bit Voltage Output SoftSpan DAC driver + * + * Copyright 2021 Analog Devices Inc. + */ +#include <linux/bitfield.h> +#include <linux/bits.h> +#include <linux/clk.h> +#include <linux/device.h> +#include <linux/gpio/consumer.h> +#include <linux/limits.h> +#include <linux/iio/iio.h> +#include <linux/module.h> +#include <linux/kernel.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/spi/spi.h> +#include <linux/unaligned/be_byteshift.h> + +#define LTC2688_DAC_CHANNELS 16 + +#define LTC2688_CMD_CH_CODE(x) (0x00 + (x)) +#define LTC2688_CMD_CH_SETTING(x) (0x10 + (x)) +#define LTC2688_CMD_CH_OFFSET(x) (0X20 + (x)) +#define LTC2688_CMD_CH_GAIN(x) (0x30 + (x)) +#define LTC2688_CMD_CH_CODE_UPDATE(x) (0x40 + (x)) + +#define LTC2688_CMD_CONFIG_REG 0x70 +#define LTC2688_CMD_POWERDOWN_REG 0x71 +#define LTC2688_CMD_A_B_SELECT_REG 0x72 +#define LTC2688_CMD_TOGGLE_DITHER_EN_REG 0x74 +#define LTC2688_CMD_UPDATE_ALL 0x7C +#define LTC2688_CMD_NOOP 0xFF + +#define LTC2688_READ_OPERATION 0x80 + +/* Channel Settings */ +#define LTC2688_CH_SPAN_MSK GENMASK(2, 0) +#define LTC2688_CH_OVERRANGE_MSK BIT(3) +#define LTC2688_CH_TD_SEL_MSK GENMASK(5, 4) +#define LTC2688_CH_DIT_PER_MSK GENMASK(8, 6) +#define LTC2688_CH_DIT_PH_MSK GENMASK(10, 9) +#define LTC2688_CH_MODE_MSK BIT(11) + +/* Configuration register */ +#define LTC2688_CONFIG_RST BIT(15) +#define LTC2688_CONFIG_EXT_REF BIT(1) + +enum { + LTC2688_SPAN_RANGE_0V_5V, + LTC2688_SPAN_RANGE_M10V_10V, + LTC2688_SPAN_RANGE_M15V_15V, + /* + * These 2 were deliberately re-arranged to be the last items as they + * have the same fs and we will just return 1 in read_available. + */ + LTC2688_SPAN_RANGE_0V_10V, + LTC2688_SPAN_RANGE_M5V_5V, + LTC2688_SPAN_RANGE_MAX +}; + +struct ltc2688_chan { + int span_tbl[LTC2688_SPAN_RANGE_MAX][2]; + bool overrange; + bool dither_toggle; + u8 offset_idx; +}; + +/* + * Helper tables to make sure the selected span is valid for the current offset + * and to translate the reg value to set for a specific span... + */ +static const int ltc2688_off_tbl[LTC2688_SPAN_RANGE_MAX] = { + [LTC2688_SPAN_RANGE_0V_5V] = 0, + [LTC2688_SPAN_RANGE_M10V_10V] = -32768, + [LTC2688_SPAN_RANGE_M15V_15V] = -32768, + [LTC2688_SPAN_RANGE_0V_10V] = 0, + [LTC2688_SPAN_RANGE_M5V_5V] = -32768 +}; + +static const int ltc2688_reg_val[LTC2688_SPAN_RANGE_MAX] = { + [LTC2688_SPAN_RANGE_0V_5V] = 0, + [LTC2688_SPAN_RANGE_M10V_10V] = 3, + [LTC2688_SPAN_RANGE_M15V_15V] = 4, + [LTC2688_SPAN_RANGE_0V_10V] = 1, + [LTC2688_SPAN_RANGE_M5V_5V] = 2 +}; + +struct ltc2688_state { + struct spi_device *spi; + struct regmap *regmap; + struct ltc2688_chan channels[LTC2688_DAC_CHANNELS]; + int vref; + u8 tx_data[6] ____cacheline_aligned; + u8 rx_data[3]; +}; + +static int ltc2688_spi_read(void *context, unsigned int cmd, unsigned int *val) +{ + struct ltc2688_state *st = context; + struct spi_transfer xfers[] = { + { + .tx_buf = st->tx_data, + .bits_per_word = 8, + .len = 3, + .cs_change = 1, + }, { + .tx_buf = st->tx_data + 3, + .rx_buf = st->rx_data, + .bits_per_word = 8, + .len = 3, + }, + }; + int ret; + + st->tx_data[0] = cmd | LTC2688_READ_OPERATION; + st->tx_data[3] = LTC2688_CMD_NOOP; + + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers)); + if (ret) + return ret; + + /* first byte is to be ignored */ + *val = get_unaligned_be16(&st->rx_data[1]); + + return 0; +} + +static int ltc2688_spi_write(void *context, unsigned int cmd, unsigned int val) +{ + struct ltc2688_state *st = context; + + st->tx_data[0] = cmd; + put_unaligned_be16(val, &st->tx_data[1]); + + return spi_write(st->spi, st->tx_data, 3); +} + +static int ltc2688_span_get(const struct ltc2688_state *st, int c, int *val2) +{ + const struct ltc2688_chan *chan = &st->channels[c]; + int ret, reg, span; + u32 i; + + ret = regmap_read(st->regmap, LTC2688_CMD_CH_SETTING(c), ®); + if (ret) + return ret; + + span = FIELD_GET(LTC2688_CH_SPAN_MSK, reg); + /* sanity check to make sure we don't get any weird value from the HW */ + if (span >= LTC2688_SPAN_RANGE_MAX) + return -EIO; + + /* look for correct span */ + for (i = 0; i < ARRAY_SIZE(ltc2688_reg_val); i++) + if (span == ltc2688_reg_val[i]) + break; + + *val2 = chan->span_tbl[i][1]; + + return IIO_VAL_INT_PLUS_NANO; +} + +static const int ltc2688_offset_avail[] = { -32768, 0 }; + +static int ltc2688_span_set(const struct ltc2688_state *st, int c, int span) +{ + const struct ltc2688_chan *chan = &st->channels[c]; + u32 i = ARRAY_SIZE(chan->span_tbl); + int off = ltc2688_offset_avail[chan->offset_idx]; + + while (i--) { + if (span == chan->span_tbl[i][1]) { + /* + * The selected scale needs to fit the current offset. + * Note that for [0V 10V] and [-5V 5V], as the scale is + * the same, we will always succeed here. Hence if + * someone really wants [0 10V] and does not set the + * offset to 0, then :man-shrugging: + */ + if (off == ltc2688_off_tbl[i]) { + break; + } else if (i < LTC2688_SPAN_RANGE_0V_10V) { + /* + * At this point, only one offset is valid so we + * already can assume error. + */ + dev_err(&st->spi->dev, "Offset(%d) not valid for current scale(0.%09d)\n", + off, span); + return -EPERM; + } + + continue; + } + + if (!i) { + dev_err(&st->spi->dev, "span(%d) not available\n", span); + return -EINVAL; + } + } + + return regmap_update_bits(st->regmap, LTC2688_CMD_CH_SETTING(c), + LTC2688_CH_SPAN_MSK, + FIELD_PREP(LTC2688_CH_SPAN_MSK, ltc2688_reg_val[i])); +}; + +static int ltc2688_offset_set(struct ltc2688_state *st, int c, int off) +{ + struct ltc2688_chan *chan = &st->channels[c]; + + if (off != ltc2688_offset_avail[0] && off != ltc2688_offset_avail[1]) + return -EINVAL; + + if (off == ltc2688_offset_avail[0]) + chan->offset_idx = 0; + else + chan->offset_idx = 1; + + return 0; +} + +static int ltc2688_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int **vals, int *type, int *length, long m) +{ + struct ltc2688_state *st = iio_priv(indio_dev); + int c = chan->channel; + + switch (m) { + case IIO_CHAN_INFO_SCALE: + *vals = (const int *)st->channels[c].span_tbl; + *length = ARRAY_SIZE(st->channels[c].span_tbl) * 2 - 2; + *type = IIO_VAL_INT_PLUS_NANO; + return IIO_AVAIL_LIST; + case IIO_CHAN_INFO_OFFSET: + *vals = ltc2688_offset_avail; + *length = ARRAY_SIZE(ltc2688_offset_avail); + *type = IIO_VAL_INT; + return IIO_AVAIL_LIST; + default: + return -EINVAL; + }; +} + +static int ltc2688_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long m) +{ + struct ltc2688_state *st = iio_priv(indio_dev); + int ret; + u8 offset; + + switch (m) { + case IIO_CHAN_INFO_RAW: + ret = regmap_read(st->regmap, + LTC2688_CMD_CH_CODE(chan->address), val); + if (ret) + return ret; + + return IIO_VAL_INT; + case IIO_CHAN_INFO_OFFSET: + offset = st->channels[chan->channel].offset_idx; + *val = ltc2688_offset_avail[offset]; + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + *val = 0; + return ltc2688_span_get(st, chan->channel, val2); + case IIO_CHAN_INFO_CALIBBIAS: + ret = regmap_read(st->regmap, + LTC2688_CMD_CH_OFFSET(chan->channel), val); + if (ret) + return ret; + + return IIO_VAL_INT; + case IIO_CHAN_INFO_CALIBSCALE: + ret = regmap_read(st->regmap, + LTC2688_CMD_CH_GAIN(chan->channel), val); + if (ret) + return ret; + + return IIO_VAL_INT; + default: + return -EINVAL; + } +} + +static int ltc2688_write_raw_get_fmt(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_RAW: + case IIO_CHAN_INFO_CALIBSCALE: + case IIO_CHAN_INFO_CALIBBIAS: + case IIO_CHAN_INFO_OFFSET: + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + return IIO_VAL_INT_PLUS_NANO; + default: + return -EINVAL; + } +} + +static int ltc2688_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, + int val2, long mask) +{ + struct ltc2688_state *st = iio_priv(indio_dev); + int cmd; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + if (val > U16_MAX || val < 0) + return -EINVAL; + /* + * If in dither/toggle mode the dac should be updated by an + * external signal and not here + */ + if (st->channels[chan->channel].dither_toggle) + cmd = LTC2688_CMD_CH_CODE(chan->channel); + else + cmd = LTC2688_CMD_CH_CODE_UPDATE(chan->channel); + + return regmap_write(st->regmap, cmd, val); + case IIO_CHAN_INFO_SCALE: + if (val) + return -EINVAL; + + return ltc2688_span_set(st, chan->channel, val2); + case IIO_CHAN_INFO_OFFSET: + return ltc2688_offset_set(st, chan->channel, val); + case IIO_CHAN_INFO_CALIBBIAS: + return regmap_write(st->regmap, + LTC2688_CMD_CH_OFFSET(chan->channel), val); + case IIO_CHAN_INFO_CALIBSCALE: + return regmap_write(st->regmap, + LTC2688_CMD_CH_GAIN(chan->channel), val); + default: + return -EINVAL; + } +} + +enum { + LTC2688_DITHER_TOGGLE_ENABLE, + LTC2688_POWERDOWN, +}; + +static ssize_t ltc2688_read_ext(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + char *buf) +{ + struct ltc2688_state *st = iio_priv(indio_dev); + int ret, regval; + u32 reg; + + switch (private) { + case LTC2688_DITHER_TOGGLE_ENABLE: + reg = LTC2688_CMD_TOGGLE_DITHER_EN_REG; + break; + case LTC2688_POWERDOWN: + reg = LTC2688_CMD_POWERDOWN_REG; + break; + default: + return -EINVAL; + } + + ret = regmap_read(st->regmap, reg, ®val); + if (ret < 0) + return ret; + + return sprintf(buf, "%u\n", !!(regval & BIT(chan->channel))); +} + +static ssize_t ltc2688_write_ext(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + const char *buf, size_t len) +{ + struct ltc2688_state *st = iio_priv(indio_dev); + bool en; + int ret; + u32 val = 0, reg; + + ret = strtobool(buf, &en); + if (ret) + return ret; + + switch (private) { + case LTC2688_DITHER_TOGGLE_ENABLE: + reg = LTC2688_CMD_TOGGLE_DITHER_EN_REG; + break; + case LTC2688_POWERDOWN: + reg = LTC2688_CMD_POWERDOWN_REG; + break; + default: + return -EINVAL; + } + + if (en) + val = BIT(chan->channel); + + ret = regmap_update_bits(st->regmap, reg, BIT(chan->channel), val); + if (ret) + return ret; + + return len; +} + +static int ltc2688_get_dither_period(struct iio_dev *dev, + const struct iio_chan_spec *chan) +{ + struct ltc2688_state *st = iio_priv(dev); + int ret, regval; + + ret = regmap_read(st->regmap, LTC2688_CMD_CH_SETTING(chan->channel), + ®val); + if (ret) + return ret; + + return FIELD_GET(LTC2688_CH_DIT_PER_MSK, regval); +} + +static int ltc2688_set_dither_period(struct iio_dev *dev, + const struct iio_chan_spec *chan, + unsigned int period) +{ + struct ltc2688_state *st = iio_priv(dev); + + return regmap_update_bits(st->regmap, + LTC2688_CMD_CH_SETTING(chan->channel), + LTC2688_CH_DIT_PER_MSK, + FIELD_PREP(LTC2688_CH_DIT_PER_MSK, period)); +} + +static int ltc2688_get_dither_phase(struct iio_dev *dev, + const struct iio_chan_spec *chan) +{ + struct ltc2688_state *st = iio_priv(dev); + int ret, regval; + + ret = regmap_read(st->regmap, LTC2688_CMD_CH_SETTING(chan->channel), + ®val); + if (ret) + return ret; + + return FIELD_GET(LTC2688_CH_DIT_PH_MSK, regval); +} + +static int ltc2688_set_dither_phase(struct iio_dev *dev, + const struct iio_chan_spec *chan, + unsigned int phase) +{ + struct ltc2688_state *st = iio_priv(dev); + + return regmap_update_bits(st->regmap, + LTC2688_CMD_CH_SETTING(chan->channel), + LTC2688_CH_DIT_PH_MSK, + FIELD_PREP(LTC2688_CH_DIT_PH_MSK, phase)); +} + +static int ltc2688_get_reg_select(struct iio_dev *dev, + const struct iio_chan_spec *chan) +{ + struct ltc2688_state *st = iio_priv(dev); + int ret, regval; + + ret = regmap_read(st->regmap, LTC2688_CMD_A_B_SELECT_REG, ®val); + if (ret < 0) + return ret; + + return !!(regval & BIT(chan->channel)); +} + +static int ltc2688_set_reg_select(struct iio_dev *dev, + const struct iio_chan_spec *chan, + unsigned int reg) +{ + struct ltc2688_state *st = iio_priv(dev); + + return regmap_update_bits(st->regmap, LTC2688_CMD_A_B_SELECT_REG, + BIT(chan->channel), reg << chan->channel); +} + +static int ltc2688_reg_access(struct iio_dev *indio_dev, + unsigned int reg, + unsigned int writeval, + unsigned int *readval) +{ + struct ltc2688_state *st = iio_priv(indio_dev); + + if (readval) + return regmap_read(st->regmap, reg, readval); + else + return regmap_write(st->regmap, reg, writeval); +} + +static const char * const ltc2688_input_register[] = { + "input_a", + "input_b", +}; + +static const struct iio_enum ltc2688_input_reg_enum = { + .items = ltc2688_input_register, + .num_items = ARRAY_SIZE(ltc2688_input_register), + .set = ltc2688_set_reg_select, + .get = ltc2688_get_reg_select, +}; + +static const char * const ltc2688_dither_period[] = { + "4", + "8", + "16", + "32", + "64", +}; + +static const struct iio_enum ltc2688_dither_period_enum = { + .items = ltc2688_dither_period, + .num_items = ARRAY_SIZE(ltc2688_dither_period), + .set = ltc2688_set_dither_period, + .get = ltc2688_get_dither_period, +}; + +static const char * const ltc2688_dither_phase[] = { + "0", + "90", + "180", + "270", +}; + +static const struct iio_enum ltc2688_dither_phase_enum = { + .items = ltc2688_dither_phase, + .num_items = ARRAY_SIZE(ltc2688_dither_phase), + .set = ltc2688_set_dither_phase, + .get = ltc2688_get_dither_phase, +}; + +#define LTC2688_CHAN_EXT_INFO(_name, _what, _shared) { \ + .name = _name, \ + .read = ltc2688_read_ext, \ + .write = ltc2688_write_ext, \ + .private = _what, \ + .shared = _shared, \ +} + +#define IIO_ENUM_AVAILABLE_SHARED(_name, _shared, _e) { \ + .name = (_name "_available"), \ + .shared = _shared, \ + .read = iio_enum_available_read, \ + .private = (uintptr_t)(_e), \ +} + +static const struct iio_chan_spec_ext_info ltc2688_dither_ext_info[] = { + IIO_ENUM("input_register", IIO_SEPARATE, <c2688_input_reg_enum), + IIO_ENUM_AVAILABLE_SHARED("input_register", + IIO_SHARED_BY_TYPE, <c2688_input_reg_enum), + IIO_ENUM("dither_period", IIO_SEPARATE, <c2688_dither_period_enum), + IIO_ENUM_AVAILABLE_SHARED("dither_period", + IIO_SHARED_BY_TYPE, <c2688_dither_period_enum), + IIO_ENUM("dither_phase", IIO_SEPARATE, <c2688_dither_phase_enum), + IIO_ENUM_AVAILABLE_SHARED("dither_phase", + IIO_SHARED_BY_TYPE, <c2688_dither_phase_enum), + LTC2688_CHAN_EXT_INFO("dither_en", LTC2688_DITHER_TOGGLE_ENABLE, + IIO_SEPARATE), + LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE), + {} +}; + +static const struct iio_chan_spec_ext_info ltc2688_toggle_ext_info[] = { + IIO_ENUM("input_register", IIO_SEPARATE, <c2688_input_reg_enum), + IIO_ENUM_AVAILABLE_SHARED("input_register", + IIO_SHARED_BY_TYPE, <c2688_input_reg_enum), + LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_DITHER_TOGGLE_ENABLE, + IIO_SEPARATE), + LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE), + {} +}; + +static const struct iio_chan_spec_ext_info ltc2688_default_ext_info[] = { + LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_POWERDOWN, IIO_SEPARATE), + {} +}; + +#define LTC2688_CHANNEL(_chan) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .output = 1, \ + .channel = (_chan), \ + .info_mask_separate = BIT(IIO_CHAN_INFO_CALIBSCALE) | \ + BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET) | \ + BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE), \ + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_OFFSET),\ + .ext_info = ltc2688_default_ext_info \ +} + +struct iio_chan_spec ltc2688_channels[] = { + LTC2688_CHANNEL(0), + LTC2688_CHANNEL(1), + LTC2688_CHANNEL(2), + LTC2688_CHANNEL(3), + LTC2688_CHANNEL(4), + LTC2688_CHANNEL(5), + LTC2688_CHANNEL(6), + LTC2688_CHANNEL(7), + LTC2688_CHANNEL(8), + LTC2688_CHANNEL(9), + LTC2688_CHANNEL(10), + LTC2688_CHANNEL(11), + LTC2688_CHANNEL(12), + LTC2688_CHANNEL(13), + LTC2688_CHANNEL(14), + LTC2688_CHANNEL(15), +}; + +enum { + LTC2688_CHAN_MODE_TOGGLE, + LTC2688_CHAN_MODE_DITHER +}; + +enum { + LTC2688_CHAN_TD_TGP1, + LTC2688_CHAN_TD_TGP2, + LTC2688_CHAN_TD_TGP3, + LTC2688_CHAN_TD_MAX +}; + +static const char * const ltc2688_clk_names[LTC2688_CHAN_TD_MAX] = { + "TGP1", "TGP2", "TGP3" +}; + +static void ltc2688_clk_disable(void *clk) +{ + clk_disable_unprepare(clk); +} + +static int ltc2688_clk_setup(const struct ltc2688_state *st, unsigned long mask) +{ + u32 bit; + int ret; + + for_each_set_bit(bit, &mask, ARRAY_SIZE(ltc2688_clk_names)) { + struct clk *clk; + /* + * If a TGP pin is set, then we need to provide a valid clock at + * the pin. + */ + clk = devm_clk_get(&st->spi->dev, ltc2688_clk_names[bit]); + if (IS_ERR(clk)) + return dev_err_probe(&st->spi->dev, PTR_ERR(clk), + "failed to get clk: %s\n", + ltc2688_clk_names[bit]); + + ret = clk_prepare_enable(clk); + if (ret) + return dev_err_probe(&st->spi->dev, ret, + "failed to enable clk: %s\n", + ltc2688_clk_names[bit]); + + ret = devm_add_action_or_reset(&st->spi->dev, + ltc2688_clk_disable, clk); + if (ret) + return ret; + } + + return 0; +} + +static int ltc2688_channel_config(struct ltc2688_state *st) +{ + struct fwnode_handle *fwnode = dev_fwnode(&st->spi->dev), *child; + u32 reg, clk_input, mode, val, mask; + unsigned long clk_msk = 0; + int ret; + + fwnode_for_each_available_child_node(fwnode, child) { + struct ltc2688_chan *chan; + + ret = fwnode_property_read_u32(child, "reg", ®); + if (ret) { + fwnode_handle_put(child); + return dev_err_probe(&st->spi->dev, ret, + "Failed to get reg property\n"); + } else if (reg >= LTC2688_DAC_CHANNELS) { + fwnode_handle_put(child); + return dev_err_probe(&st->spi->dev, -EINVAL, + "reg >= %d\n", LTC2688_DAC_CHANNELS); + } + + chan = &st->channels[reg]; + ret = fwnode_property_read_u32(child, "adi,mode", &mode); + if (!ret) { + if (mode > LTC2688_CHAN_MODE_DITHER) { + fwnode_handle_put(child); + return dev_err_probe(&st->spi->dev, -EINVAL, + "chan mode inv value(%d)\n", + mode); + } + + chan->dither_toggle = true; + if (mode == LTC2688_CHAN_MODE_TOGGLE) { + ltc2688_channels[reg].ext_info = ltc2688_toggle_ext_info; + } else { + ltc2688_channels[reg].ext_info = ltc2688_dither_ext_info; + /* enable dither mode */ + mask = LTC2688_CH_MODE_MSK; + val = BIT(11); + } + } + + ret = fwnode_property_read_u32(child, "adi,toggle-dither-input", + &clk_input); + if (!ret) { + if (clk_input > LTC2688_CHAN_TD_TGP3) { + fwnode_handle_put(child); + return dev_err_probe(&st->spi->dev, -EINVAL, + "toggle-dither-input inv value(%d)\n", + clk_input); + } + + clk_msk |= BIT(clk_input); + mask |= LTC2688_CH_TD_SEL_MSK; + /* + * 0 means software toggle which we are not supporting + * for now. Hence the +1 + */ + val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1); + } else if (chan->dither_toggle) { + /* + * As sw_toggle is not supported, we need to make sure + * a valid input is selected if toggle/dither mode is + * requested + */ + return dev_err_probe(&st->spi->dev, -EINVAL, + "toggle-dither set but no toggle-dither-input\n"); + } + + chan->overrange = fwnode_property_read_bool(child, + "adi,overrange"); + if (chan->overrange) { + val |= LTC2688_CH_OVERRANGE_MSK; + mask |= BIT(3); + } + + if (!mask) + continue; + + ret = regmap_update_bits(st->regmap, + LTC2688_CMD_CH_SETTING(reg), mask, + val); + if (ret) { + fwnode_handle_put(child); + return dev_err_probe(&st->spi->dev, -EINVAL, + "failed to set chan settings\n"); + } + + mask = 0; + val = 0; + } + + return ltc2688_clk_setup(st, clk_msk); +} + +static const int ltc2688_span_helper[LTC2688_SPAN_RANGE_MAX][2] = { + {0, 5000}, {-10000, 10000}, {-15000, 15000}, {0, 10000}, {-5000, 5000}, +}; + +static int ltc2688_get_full_scale(const struct ltc2688_chan *chan, int idx) +{ + int fs = ltc2688_span_helper[idx][1] - ltc2688_span_helper[idx][0]; + + if (chan->overrange) + fs = mult_frac(fs, 105, 100); + + return fs; +} + +static void ltc2688_span_avail_compute(struct ltc2688_state *st) +{ + u32 c, i; + + for (c = 0; c < ARRAY_SIZE(st->channels); c++) { + struct ltc2688_chan *chan = &st->channels[c]; + + for (i = 0; i < ARRAY_SIZE(chan->span_tbl); i++) { + u32 fs, s; + + fs = ltc2688_get_full_scale(chan, i); + /* we will return IIO_VAL_INT_PLUS_NANO */ + s = DIV_ROUND_CLOSEST_ULL(st->vref * fs * 1000000000ULL, + 4096 * 1 << 16); + + chan->span_tbl[i][1] = s; + /* default offset is 0 */ + chan->offset_idx = 1; + } + } +} + +static int ltc2688_setup(struct ltc2688_state *st, struct regulator *vref) +{ + struct gpio_desc *gpio; + int ret; + + gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(gpio)) + return dev_err_probe(&st->spi->dev, PTR_ERR(gpio), + "Failed to get reset gpio"); + + if (gpio) { + usleep_range(1000, 1200); + /* bring device out of reset */ + gpiod_set_value_cansleep(gpio, 0); + } else { + ret = regmap_update_bits(st->regmap, LTC2688_CMD_CONFIG_REG, + LTC2688_CONFIG_RST, + LTC2688_CONFIG_RST); + if (ret < 0) + return ret; + } + + usleep_range(10000, 12000); + ret = ltc2688_channel_config(st); + if (ret < 0) + return ret; + + if (vref) { + ret = regmap_update_bits(st->regmap, LTC2688_CMD_CONFIG_REG, + LTC2688_CONFIG_EXT_REF, BIT(1)); + if (ret < 0) + return ret; + } + + ltc2688_span_avail_compute(st); + return 0; +} + +static void ltc2688_disable_regulator(void *regulator) +{ + regulator_disable(regulator); +} + +static int ltc2688_regulator_supply_get(const struct ltc2688_state *st, + const char *name) +{ + struct regulator *reg; + int ret; + + reg = devm_regulator_get(&st->spi->dev, name); + if (IS_ERR(reg)) + return dev_err_probe(&st->spi->dev, PTR_ERR(reg), + "Failed to get %s regulator\n", name); + + ret = regulator_enable(reg); + if (ret) + return dev_err_probe(&st->spi->dev, ret, + "Failed to enable %s regulator\n", name); + + return devm_add_action_or_reset(&st->spi->dev, + ltc2688_disable_regulator, reg); +} + +static bool ltc2688_reg_readable(struct device *dev, unsigned int cmd) +{ + switch (cmd | LTC2688_READ_OPERATION) { + case 0x80 ... 0xBF: + return true; + case 0xF0 ... 0xFE: + return true; + default: + return false; + } +} + +static bool ltc2688_reg_writable(struct device *dev, unsigned int cmd) +{ + if (cmd <= LTC2688_CMD_UPDATE_ALL) + return true; + + return false; +} + +static const struct regmap_config ltc2688_regmap_config = { + .reg_read = ltc2688_spi_read, + .reg_write = ltc2688_spi_write, + .readable_reg = ltc2688_reg_readable, + .writeable_reg = ltc2688_reg_writable, + /* ignoring the no op command */ + .max_register = U16_MAX - 1 +}; + +static const struct iio_info ltc2688_info = { + .write_raw = ltc2688_write_raw, + .read_raw = ltc2688_read_raw, + .write_raw_get_fmt = ltc2688_write_raw_get_fmt, + .read_avail = ltc2688_read_avail, + .debugfs_reg_access = ltc2688_reg_access, +}; + +static int ltc2688_probe(struct spi_device *spi) +{ + struct ltc2688_state *st; + struct iio_dev *indio_dev; + struct regulator *vref_reg; + int ret; + + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st)); + if (!indio_dev) + return -ENOMEM; + + st = iio_priv(indio_dev); + st->spi = spi; + + st->regmap = devm_regmap_init(&spi->dev, NULL, st, <c2688_regmap_config); + if (IS_ERR(st->regmap)) + return dev_err_probe(&spi->dev, PTR_ERR(st->regmap), + "Failed to init regmap"); + + ret = ltc2688_regulator_supply_get(st, "vcc"); + if (ret) + return ret; + + ret = ltc2688_regulator_supply_get(st, "iovcc"); + if (ret) + return ret; + + vref_reg = devm_regulator_get_optional(&spi->dev, "vref"); + if (!IS_ERR(vref_reg)) { + ret = regulator_enable(vref_reg); + if (ret) + return dev_err_probe(&spi->dev, ret, + "Failed to enable vref regulators\n"); + + ret = devm_add_action_or_reset(&spi->dev, + ltc2688_disable_regulator, + vref_reg); + if (ret) + return ret; + + ret = regulator_get_voltage(vref_reg); + if (ret < 0) + return dev_err_probe(&spi->dev, ret, + "Failed to get vref\n"); + + st->vref = ret / 1000; + } else { + if (PTR_ERR(vref_reg) != -ENODEV) + return dev_err_probe(&spi->dev, PTR_ERR(vref_reg), + "Failed to get vref regulator"); + + vref_reg = NULL; + /* internal reference */ + st->vref = 4096; + } + + indio_dev->name = "ltc2688"; + indio_dev->info = <c2688_info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = ltc2688_channels; + indio_dev->num_channels = ARRAY_SIZE(ltc2688_channels); + + ret = ltc2688_setup(st, vref_reg); + if (ret < 0) + return ret; + + return devm_iio_device_register(&st->spi->dev, indio_dev); +} + +static const struct of_device_id ltc2688_of_id[] = { + { .compatible = "adi,ltc2688" }, + {} +}; +MODULE_DEVICE_TABLE(of, ltc2688_of_id); + +static const struct spi_device_id ltc2688_id[] = { + { "ltc2688" }, + {} +}; +MODULE_DEVICE_TABLE(spi, ltc2688_id); + +static struct spi_driver ltc2688_driver = { + .driver = { + .name = "ltc2688", + .of_match_table = ltc2688_of_id, + }, + .probe = ltc2688_probe, + .id_table = ltc2688_id, +}; +module_spi_driver(ltc2688_driver); + +MODULE_AUTHOR("Nuno Sá <nuno.sa@analog.com>"); +MODULE_DESCRIPTION("Analog Devices LTC2688 DAC"); +MODULE_LICENSE("GPL");
The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated precission reference. It is guaranteed monotonic and has built in rail-to-rail output buffers that can source or sink up to 20 mA. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/dac/ltc2688.c | 995 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 995 insertions(+) create mode 100644 drivers/iio/dac/ltc2688.c