| Message ID | 20220121142501.151-2-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for LTC2688 | expand |
On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated > precision reference. It is guaranteed monotonic and has built in > rail-to-rail output buffers that can source or sink up to 20 mA. ... > +#include <linux/of.h> property.h please/ ... > +static int ltc2688_spi_read(void *context, const void *reg, size_t reg_size, > + void *val, size_t val_size) > +{ > + 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; > + memcpy(st->tx_data, reg, reg_size); > + > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers)); > + if (ret) > + return ret; > + > + memcpy(val, &st->rx_data[1], val_size); > + > + return 0; > +} First of all, yuo have fixed len in transfer sizes, so what the purpose of the reg_size / val_size? Second, why do you need this specific function instead of regmap bulk ops against be24/le24? ... > +unlock: out_unlock: ? (And in similar cases) ... > + if (ret) > + return ret; > + > + return len; In some cases the return ret ?: len; is used, in some like above. Maybe a bit of consistency? ... > + if (private == LTC2688_INPUT_B_AVAIL) > + return sysfs_emit(buf, "[%u %u %u]\n", ltc2688_raw_range[0], > + ltc2688_raw_range[1], > + ltc2688_raw_range[2] / 4); Is it standard form "[A B C]" for ranges in IIO? I haven't looked into the code deeply (and datasheet at all) to understand meaning. To me range is usually out of two numbers. > + if (private == LTC2688_DITHER_OFF) > + return sysfs_emit(buf, "0\n"); > + ret = ltc2688_dac_code_read(st, chan->channel, private, &val); > + if (ret) > + return ret; > + > + return sysfs_emit(buf, "%u\n", val); These three types of output for one sysfs node? Seems it's not align with the idea behind sysfs. It maybe that I missed something. ... > + ret = kstrtou16(buf, 10, &val); In other function you have long, here u16. I would expect that the types are of the same class, e.g. if here you have u16, then there something like s32 / s64. Or here something like unsigned short. A bit of elaboration why u16 is chosen here? ... > + .info_mask_separate_available = BIT(IIO_CHAN_INFO_RAW), \ > + .ext_info = ltc2688_ext_info \ + Comma ... > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st, > + struct ltc2688_chan *chan, > + struct device_node *np, int tgp) > +{ > + unsigned long rate; > + struct clk *clk; > + int ret, f; > + > + clk = devm_get_clk_from_child(&st->spi->dev, np, NULL); > + if (IS_ERR(clk)) Make it optional for non-OF, can be done as easy as if (IS_ERR(clk)) { if (PTR_ERR(clk) == -ENOENT) clk = NULL; else return dev_err_probe(...); } > + return dev_err_probe(&st->spi->dev, PTR_ERR(clk), > + "failed to get tgp clk.\n"); > + > + ret = clk_prepare_enable(clk); > + if (ret) > + return dev_err_probe(&st->spi->dev, ret, > + "failed to enable tgp clk.\n"); > + > + ret = devm_add_action_or_reset(&st->spi->dev, ltc2688_clk_disable, clk); > + if (ret) > + return ret; > + > + if (chan->toggle_chan) > + return 0; > + > + /* calculate available dither frequencies */ > + rate = clk_get_rate(clk); > + for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++) > + chan->dither_frequency[f] = DIV_ROUND_CLOSEST(rate, ltc2688_period[f]); > + > + return 0; > +} ... > +static int ltc2688_channel_config(struct ltc2688_state *st) > +{ > + struct device *dev = &st->spi->dev; > + struct device_node *child; > + u32 reg, clk_input, val, tmp[2]; > + int ret, span; > + > + for_each_available_child_of_node(dev->of_node, child) { device_for_each_child_node() > + struct ltc2688_chan *chan; > + > + ret = of_property_read_u32(child, "reg", ®); > + if (ret) { > + of_node_put(child); > + return dev_err_probe(dev, ret, > + "Failed to get reg property\n"); > + } > + > + if (reg >= LTC2688_DAC_CHANNELS) { > + of_node_put(child); > + return dev_err_probe(dev, -EINVAL, > + "reg bigger than: %d\n", > + LTC2688_DAC_CHANNELS); > + } > + > + val = 0; > + chan = &st->channels[reg]; > + if (of_property_read_bool(child, "adi,toggle-mode")) { > + chan->toggle_chan = true; > + /* assume sw toggle ABI */ > + st->iio_chan[reg].ext_info = ltc2688_toggle_sym_ext_info; > + /* > + * Clear IIO_CHAN_INFO_RAW bit as toggle channels expose > + * out_voltage_raw{0|1} files. > + */ > + clear_bit(IIO_CHAN_INFO_RAW, > + &st->iio_chan[reg].info_mask_separate); Do you need atomic operation here? > + } > + > + ret = of_property_read_u32_array(child, "adi,output-range-microvolt", > + tmp, ARRAY_SIZE(tmp)); > + if (!ret) { > + span = ltc2688_span_lookup(st, (int)tmp[0] / 1000, > + tmp[1] / 1000); > + if (span < 0) { > + of_node_put(child); > + return dev_err_probe(dev, -EINVAL, > + "output range not valid:[%d %d]\n", > + tmp[0], tmp[1]); > + } > + > + val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span); > + } > + > + ret = of_property_read_u32(child, "adi,toggle-dither-input", > + &clk_input); > + if (!ret) { > + if (clk_input >= LTC2688_CH_TGP_MAX) { > + of_node_put(child); > + return dev_err_probe(dev, -EINVAL, > + "toggle-dither-input inv value(%d)\n", > + clk_input); > + } > + > + ret = ltc2688_tgp_clk_setup(st, chan, child, clk_input); > + if (ret) { > + of_node_put(child); > + return ret; > + } > + > + /* > + * 0 means software toggle which is the default mode. > + * Hence the +1. > + */ > + val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1); > + > + /* > + * If a TGPx is given, we automatically assume a dither > + * capable channel (unless toggle is already enabled). > + * On top of this we just set here the dither bit in the > + * channel settings. It won't have any effect until the > + * global toggle/dither bit is enabled. > + */ > + if (!chan->toggle_chan) { > + val |= FIELD_PREP(LTC2688_CH_MODE_MSK, 1); > + st->iio_chan[reg].ext_info = ltc2688_dither_ext_info; > + } else { > + /* wait, no sw toggle after all */ > + st->iio_chan[reg].ext_info = ltc2688_toggle_ext_info; > + } > + } > + > + if (of_property_read_bool(child, "adi,overrange")) { > + chan->overrange = true; > + val |= LTC2688_CH_OVERRANGE_MSK; > + } > + > + if (!val) > + continue; > + > + ret = regmap_write(st->regmap, LTC2688_CMD_CH_SETTING(reg), > + val); > + if (ret) { > + of_node_put(child); > + return dev_err_probe(dev, -EINVAL, > + "failed to set chan settings\n"); > + } > + } > + > + return 0; > +} ... > +struct regmap_bus ltc2688_regmap_bus = { > + .read = ltc2688_spi_read, > + .write = ltc2688_spi_write, > + .read_flag_mask = LTC2688_READ_OPERATION, > + .reg_format_endian_default = REGMAP_ENDIAN_BIG, > + .val_format_endian_default = REGMAP_ENDIAN_BIG + Comma. > +}; > + > +static const struct regmap_config ltc2688_regmap_config = { > + .reg_bits = 8, > + .val_bits = 16, > + .readable_reg = ltc2688_reg_readable, > + .writeable_reg = ltc2688_reg_writable, > + /* ignoring the no op command */ > + .max_register = LTC2688_CMD_UPDATE_ALL Ditto. > +}; ... > + vref_reg = devm_regulator_get_optional(dev, "vref"); > + if (!IS_ERR(vref_reg)) { Why not positive conditional check (and hence standard pattern -- error handling first)? > + ret = regulator_enable(vref_reg); > + if (ret) > + return dev_err_probe(dev, ret, > + "Failed to enable vref regulators\n"); > + > + ret = devm_add_action_or_reset(dev, ltc2688_disable_regulator, > + vref_reg); > + if (ret) > + return ret; > + > + ret = regulator_get_voltage(vref_reg); > + if (ret < 0) > + return dev_err_probe(dev, ret, "Failed to get vref\n"); > + > + st->vref = ret / 1000; > + } else { > + if (PTR_ERR(vref_reg) != -ENODEV) > + return dev_err_probe(dev, PTR_ERR(vref_reg), > + "Failed to get vref regulator"); > + > + vref_reg = NULL; > + /* internal reference */ > + st->vref = 4096; > + }
On Sat, 5 Feb 2022 19:29:39 +0200 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: A few comments from me, mostly because I couldn't resist jumping in ;) Note this is only some of the things Andy raised.... Jonathan > > > + if (private == LTC2688_INPUT_B_AVAIL) > > + return sysfs_emit(buf, "[%u %u %u]\n", ltc2688_raw_range[0], > > + ltc2688_raw_range[1], > > + ltc2688_raw_range[2] / 4); > > Is it standard form "[A B C]" for ranges in IIO? I haven't looked into the code > deeply (and datasheet at all) to understand meaning. To me range is usually out > of two numbers. https://elixir.bootlin.com/linux/latest/source/Documentation/ABI/testing/sysfs-bus-iio#L105 Yes, [Min Step Max] > > > + if (private == LTC2688_DITHER_OFF) > > + return sysfs_emit(buf, "0\n"); > > > + ret = ltc2688_dac_code_read(st, chan->channel, private, &val); > > + if (ret) > > + return ret; > > + > > + return sysfs_emit(buf, "%u\n", val); > > These three types of output for one sysfs node? Seems it's not align with the > idea behind sysfs. It maybe that I missed something. Different sysfs nodes. Though it's a fair question on whether this would be better done as a bunch of separate callbacks, rather than one with a lookup on the private parameter. > > > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st, > > + struct ltc2688_chan *chan, > > + struct device_node *np, int tgp) > > +{ > > + unsigned long rate; > > + struct clk *clk; > > + int ret, f; > > + > > + clk = devm_get_clk_from_child(&st->spi->dev, np, NULL); > > + if (IS_ERR(clk)) > > Make it optional for non-OF, can be done as easy as > > if (IS_ERR(clk)) { > if (PTR_ERR(clk) == -ENOENT) > clk = NULL; > else > return dev_err_probe(...); > } Interesting. We should probably look at where else this is appropriate. > > > + return dev_err_probe(&st->spi->dev, PTR_ERR(clk), > > + "failed to get tgp clk.\n"); > > + > > + ret = clk_prepare_enable(clk); > > + if (ret) > > + return dev_err_probe(&st->spi->dev, ret, > > + "failed to enable tgp clk.\n"); > > + > > + ret = devm_add_action_or_reset(&st->spi->dev, ltc2688_clk_disable, clk); > > + if (ret) > > + return ret; > > + > > + if (chan->toggle_chan) > > + return 0; > > + > > + /* calculate available dither frequencies */ > > + rate = clk_get_rate(clk); > > + for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++) > > + chan->dither_frequency[f] = DIV_ROUND_CLOSEST(rate, ltc2688_period[f]); > > + > > + return 0; > > +} > > ... > > > +static int ltc2688_channel_config(struct ltc2688_state *st) > > +{ > > + struct device *dev = &st->spi->dev; > > + struct device_node *child; > > + u32 reg, clk_input, val, tmp[2]; > > + int ret, span; > > + > > + for_each_available_child_of_node(dev->of_node, child) { > > device_for_each_child_node() This is the old issue with missing device_for_each_available_child_node() though can just add a check on whether it's available inside the loop. > > > + struct ltc2688_chan *chan; > > + ...
On Sat, Feb 05, 2022 at 06:44:59PM +0000, Jonathan Cameron wrote: > On Sat, 5 Feb 2022 19:29:39 +0200 > Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > A few comments from me, mostly because I couldn't resist jumping in ;) > Note this is only some of the things Andy raised.... ... > > > +static int ltc2688_channel_config(struct ltc2688_state *st) > > > +{ > > > + struct device *dev = &st->spi->dev; > > > + struct device_node *child; > > > + u32 reg, clk_input, val, tmp[2]; > > > + int ret, span; > > > + > > > + for_each_available_child_of_node(dev->of_node, child) { > > > > device_for_each_child_node() > > This is the old issue with missing > device_for_each_available_child_node() > though can just add a check on whether it's available inside the loop. Didn't we discuss this with Rob and he told that device_for_each_child_node() is already for available only? > > > + struct ltc2688_chan *chan; > > > +
On Sat, Feb 05, 2022 at 08:50:31PM +0200, Andy Shevchenko wrote: > On Sat, Feb 05, 2022 at 06:44:59PM +0000, Jonathan Cameron wrote: > > On Sat, 5 Feb 2022 19:29:39 +0200 > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > > > A few comments from me, mostly because I couldn't resist jumping in ;) > > Note this is only some of the things Andy raised.... > > ... > > > > > +static int ltc2688_channel_config(struct ltc2688_state *st) > > > > +{ > > > > + struct device *dev = &st->spi->dev; > > > > + struct device_node *child; > > > > + u32 reg, clk_input, val, tmp[2]; > > > > + int ret, span; > > > > + > > > > + for_each_available_child_of_node(dev->of_node, child) { > > > > > > device_for_each_child_node() > > > > This is the old issue with missing > > device_for_each_available_child_node() > > though can just add a check on whether it's available inside the loop. > > Didn't we discuss this with Rob and he told that device_for_each_child_node() > is already for available only? https://lore.kernel.org/lkml/20211205190101.26de4a57@jic23-huawei/T/#u So, the fwnode has a correct implementation, and we may use it here.
> From: Andy Shevchenko <andriy.shevchenko@intel.com> > Sent: Saturday, February 5, 2022 6:30 PM > To: Sa, Nuno <Nuno.Sa@analog.com> > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; Rob > Herring <robh+dt@kernel.org>; Jonathan Cameron > <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; > Hennerich, Michael <Michael.Hennerich@analog.com> > Subject: Re: [PATCH v3 1/3] iio: dac: add support for ltc2688 > > [External] > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: > > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated > > precision reference. It is guaranteed monotonic and has built in > > rail-to-rail output buffers that can source or sink up to 20 mA. > > ... > > > +#include <linux/of.h> > > property.h please/ That probably means property and of both included. See below in the clock_get comments... > ... > > > +static int ltc2688_spi_read(void *context, const void *reg, size_t > reg_size, > > + void *val, size_t val_size) > > +{ > > + 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; > > > + memcpy(st->tx_data, reg, reg_size); > > + > > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers)); > > + if (ret) > > + return ret; > > + > > + memcpy(val, &st->rx_data[1], val_size); > > + > > + return 0; > > +} > > First of all, yuo have fixed len in transfer sizes, so what the purpose of > the reg_size / val_size? Well, reg_size is 1 byte and val_size is 2 as defined in the regmap_bus struct. And that is what it must be used for the transfer to work. I could also hardcode 1 and 2 but I preferred to use the parameters. I guess you can argue (and probably this is why you are complaining about this) for me to use reg_size + val_size in the transfer length for consistency. That's fair but I do not think this is __that__ bad... Can make that change though. > Second, why do you need this specific function instead of regmap bulk > ops against be24/le24? > Not sure I'm following this one... If you mean why am I using a custom regmap_bus implementation, that was already explained in the RFC patch. And IIRC, you were the one already asking
On Sat, 5 Feb 2022 20:58:12 +0200 Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Sat, Feb 05, 2022 at 08:50:31PM +0200, Andy Shevchenko wrote: > > On Sat, Feb 05, 2022 at 06:44:59PM +0000, Jonathan Cameron wrote: > > > On Sat, 5 Feb 2022 19:29:39 +0200 > > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > > > > > A few comments from me, mostly because I couldn't resist jumping in ;) > > > Note this is only some of the things Andy raised.... > > > > ... > > > > > > > +static int ltc2688_channel_config(struct ltc2688_state *st) > > > > > +{ > > > > > + struct device *dev = &st->spi->dev; > > > > > + struct device_node *child; > > > > > + u32 reg, clk_input, val, tmp[2]; > > > > > + int ret, span; > > > > > + > > > > > + for_each_available_child_of_node(dev->of_node, child) { > > > > > > > > device_for_each_child_node() > > > > > > This is the old issue with missing > > > device_for_each_available_child_node() > > > though can just add a check on whether it's available inside the loop. > > > > Didn't we discuss this with Rob and he told that device_for_each_child_node() > > is already for available only? > > https://lore.kernel.org/lkml/20211205190101.26de4a57@jic23-huawei/T/#u > > So, the fwnode has a correct implementation, and we may use it here. > I wasn't totally sure of the conclusion of that discussion. a) Fine to just use device_for_each_child_node() for this case and not worry about it. b) Worth adding device_for_each_available_child_node() with the same implementation c) (possibly workaround / avoid the issue) Use device_for_each_child_node() but also check validity (hopefully compiler would remove the check) in order to act as documentation. I'm fine with any of the above. J
On Sun, Feb 06, 2022 at 03:16:24PM +0000, Jonathan Cameron wrote: > On Sat, 5 Feb 2022 20:58:12 +0200 > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Sat, Feb 05, 2022 at 08:50:31PM +0200, Andy Shevchenko wrote: ... > > https://lore.kernel.org/lkml/20211205190101.26de4a57@jic23-huawei/T/#u > > > > So, the fwnode has a correct implementation, and we may use it here. > > > I wasn't totally sure of the conclusion of that discussion. > a) Fine to just use device_for_each_child_node() for this case and not worry > about it. Yes. As he mentioned the device_for_each_child_node() is implemented correctly from day 1. > b) Worth adding device_for_each_available_child_node() with the same > implementation I believe it's an opposite prospective, i.e. drop of_for_each_available_child_node() and use the of_for_each_child_node() everywhere. > c) (possibly workaround / avoid the issue) Use device_for_each_child_node() > but also check validity (hopefully compiler would remove the check) > in order to act as documentation. Makes no sense because implementation does it already. > I'm fine with any of the above.
On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote: > > From: Andy Shevchenko <andriy.shevchenko@intel.com> > > Sent: Saturday, February 5, 2022 6:30 PM > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: ... > > > +#include <linux/of.h> > > > > property.h please/ > > That probably means property and of both included. See below in the > clock_get comments... Why? OF won't be used at all. ... > > > + memcpy(st->tx_data, reg, reg_size); > > > + > > > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers)); > > > + if (ret) > > > + return ret; > > > + > > > + memcpy(val, &st->rx_data[1], val_size); > > > + > > > + return 0; > > > +} > > > > First of all, yuo have fixed len in transfer sizes, so what the purpose of > > the reg_size / val_size? > > Well, reg_size is 1 byte and val_size is 2 as defined in the regmap_bus > struct. And that is what it must be used for the transfer to work. I > could also hardcode 1 and 2 but I preferred to use the parameters. I guess > you can argue (and probably this is why you are complaining about this) > for me to use reg_size + val_size in the transfer length for consistency. > That's fair but I do not think this is __that__ bad... It's not bad, but I think that division between register and value is a good thing to have. > Can make that change though. Would be nice! ... > > Second, why do you need this specific function instead of regmap bulk > > ops against be24/le24? > > Not sure I'm following this one... If you mean why am I using a custom > regmap_bus implementation, that was already explained in the RFC patch. > And IIRC, you were the one already asking
On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote: > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote: > > > From: Andy Shevchenko <andriy.shevchenko@intel.com> > > > Sent: Saturday, February 5, 2022 6:30 PM > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: > > ... > > > > > +#include <linux/of.h> > > > > > > property.h please/ > > > > That probably means property and of both included. See below in the > > clock_get comments... > > Why? OF won't be used at all. > see below on the clock function... > > ... > > > > > + memcpy(st->tx_data, reg, reg_size); > > > > + > > > > + ret = spi_sync_transfer(st->spi, xfers, > > > > ARRAY_SIZE(xfers)); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + memcpy(val, &st->rx_data[1], val_size); > > > > + > > > > + return 0; > > > > +} > > > > > > First of all, yuo have fixed len in transfer sizes, so what the > > > purpose of > > > the reg_size / val_size? > > > > Well, reg_size is 1 byte and val_size is 2 as defined in the > > regmap_bus > > struct. And that is what it must be used for the transfer to work. > > I > > could also hardcode 1 and 2 but I preferred to use the parameters. > > I guess > > you can argue (and probably this is why you are complaining about > > this) > > for me to use reg_size + val_size in the transfer length for > > consistency. > > That's fair but I do not think this is __that__ bad... > > It's not bad, but I think that division between register and value is > a good > thing to have. > > > Can make that change though. > > Would be nice! > > ... > > > > Second, why do you need this specific function instead of regmap > > > bulk > > > ops against be24/le24? > > > > Not sure I'm following this one... If you mean why am I using a > > custom > > regmap_bus implementation, that was already explained in the RFC > > patch. > > And IIRC, you were the one already asking
On Mon, 2022-02-07 at 21:19 +0100, Nuno Sá wrote: > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote: > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote: > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com> > > > > Sent: Saturday, February 5, 2022 6:30 PM > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: > > > > ... > > > > > > > +#include <linux/of.h> > > > > > > > > property.h please/ > > > > > > That probably means property and of both included. See below in > > > the > > > clock_get comments... > > > > Why? OF won't be used at all. > > > see below on the clock function... > > > > ... > > > > > > > + memcpy(st->tx_data, reg, reg_size); > > > > > + > > > > > + ret = spi_sync_transfer(st->spi, xfers, > > > > > ARRAY_SIZE(xfers)); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + memcpy(val, &st->rx_data[1], val_size); > > > > > + > > > > > + return 0; > > > > > +} > > > > > > > > First of all, yuo have fixed len in transfer sizes, so what the > > > > purpose of > > > > the reg_size / val_size? > > > > > > Well, reg_size is 1 byte and val_size is 2 as defined in the > > > regmap_bus > > > struct. And that is what it must be used for the transfer to > > > work. > > > I > > > could also hardcode 1 and 2 but I preferred to use the > > > parameters. > > > I guess > > > you can argue (and probably this is why you are complaining about > > > this) > > > for me to use reg_size + val_size in the transfer length for > > > consistency. > > > That's fair but I do not think this is __that__ bad... > > > > It's not bad, but I think that division between register and value > > is > > a good > > thing to have. > > > > > Can make that change though. > > > > Would be nice! > > > > ... > > > > > > Second, why do you need this specific function instead of > > > > regmap > > > > bulk > > > > ops against be24/le24? > > > > > > Not sure I'm following this one... If you mean why am I using a > > > custom > > > regmap_bus implementation, that was already explained in the RFC > > > patch. > > > And IIRC, you were the one already asking
On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote: > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote: > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote: > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com> > > > > Sent: Saturday, February 5, 2022 6:30 PM > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: ... > > > > Second, why do you need this specific function instead of regmap > > > > bulk > > > > ops against be24/le24? > > > > > > Not sure I'm following this one... If you mean why am I using a > > > custom > > > regmap_bus implementation, that was already explained in the RFC > > > patch. > > > And IIRC, you were the one already asking
On Mon, Feb 14, 2022 at 02:23:01PM +0100, Nuno Sá wrote: > On Mon, 2022-02-07 at 21:19 +0100, Nuno Sá wrote: > I would definetly like to have some settlement on the above (before > sending a v4). It kind of was discussed a bit already in [1] where I > felt we had to live with OF for this driver (that is why I kept OF in > v3. With the above, I cannot > really see how we can have device API with also including OF... If I > missing something please let me know :) Sorry for the delay, answered to your previous message. > [1]: https://lore.kernel.org/all/CAHp75VczFs8QpsY7tuB-h4X=H54nyjABA4qDSmpQ+FRYAHZdrA@mail.gmail.com/
On Mon, 2022-02-14 at 15:50 +0200, Andy Shevchenko wrote: > On Mon, Feb 14, 2022 at 02:23:01PM +0100, Nuno Sá wrote: > > On Mon, 2022-02-07 at 21:19 +0100, Nuno Sá wrote: > > > I would definetly like to have some settlement on the above (before > > sending a v4). It kind of was discussed a bit already in [1] where > > I > > felt we had to live with OF for this driver (that is why I kept OF > > in > > v3. With the above, I cannot > > really see how we can have device API with also including OF... If > > I > > missing something please let me know :) > > Sorry for the delay, answered to your previous message. No worries. As I already said, I'm not doing much till the end of the month but I definetly wanted the device vs OF question settled before starting v4. - Nuno Sá >
On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote: > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote: > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote: > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote: > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com> > > > > > Sent: Saturday, February 5, 2022 6:30 PM > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: > > ... > > > > > > Second, why do you need this specific function instead of > > > > > regmap > > > > > bulk > > > > > ops against be24/le24? > > > > > > > > Not sure I'm following this one... If you mean why am I using a > > > > custom > > > > regmap_bus implementation, that was already explained in the > > > > RFC > > > > patch. > > > > And IIRC, you were the one already asking
On Fri, 18 Feb 2022 14:51:28 +0100 Nuno Sá <noname.nuno@gmail.com> wrote: > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote: > > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote: > > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote: > > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote: > > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com> > > > > > > Sent: Saturday, February 5, 2022 6:30 PM > > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: > > > > ... > > > > > > > > Second, why do you need this specific function instead of > > > > > > regmap > > > > > > bulk > > > > > > ops against be24/le24? > > > > > > > > > > Not sure I'm following this one... If you mean why am I using a > > > > > custom > > > > > regmap_bus implementation, that was already explained in the > > > > > RFC > > > > > patch. > > > > > And IIRC, you were the one already asking
On Sat, 2022-02-05 at 19:29 +0200, Andy Shevchenko wrote: > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: > > The LTC2688 is a 16 channel, 16 bit, +-15V DAC with an integrated > > precision reference. It is guaranteed monotonic and has built in > > rail-to-rail output buffers that can source or sink up to 20 mA. > > ... > > > +#include <linux/of.h> > > property.h please/ > > ... > > > +static int ltc2688_spi_read(void *context, const void *reg, size_t > > reg_size, > > + void *val, size_t val_size) > > +{ > > + 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; > > > + memcpy(st->tx_data, reg, reg_size); > > + > > + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers)); > > + if (ret) > > + return ret; > > + > > + memcpy(val, &st->rx_data[1], val_size); > > + > > + return 0; > > +} > > First of all, yuo have fixed len in transfer sizes, so what the > purpose of the reg_size / val_size? > Second, why do you need this specific function instead of regmap bulk > ops against be24/le24? > > ... > > > +unlock: > > out_unlock: ? > (And in similar cases) > > ... > > > + if (ret) > > + return ret; > > + > > + return len; > > In some cases the return ret ?: len; is used, in some like above. > Maybe a bit > of consistency? > Hmm, when doing some changes for v4 I realized why I used the ternary operator here (typically I'm not a fan). The thing is that we already check the error condition after calling regmap_update_bits() which is not the last code executed. Hence, I didn't want to do again if (ret) return ret after unlocking the mutex. In the other places the error check is always on the last lines where nothing else will happen (either return error or len). Alternatively, to remove the ternary operator, I would prefer to actually remove the label and goto and after regmap_update_bits(), do: if (ret) { mutex_unlock(); return ret; } It might be not consistent with other places were goto is used but this function also has it's differencies... - Nuno Sá > ... > > > + if (private == LTC2688_INPUT_B_AVAIL) > > + return sysfs_emit(buf, "[%u %u %u]\n", > > ltc2688_raw_range[0], > > + ltc2688_raw_range[1], > > + ltc2688_raw_range[2] / 4); > > Is it standard form "[A B C]" for ranges in IIO? I haven't looked > into the code > deeply (and datasheet at all) to understand meaning. To me range is > usually out > of two numbers. > > > + if (private == LTC2688_DITHER_OFF) > > + return sysfs_emit(buf, "0\n"); > > > + ret = ltc2688_dac_code_read(st, chan->channel, private, > > &val); > > + if (ret) > > + return ret; > > + > > + return sysfs_emit(buf, "%u\n", val); > > These three types of output for one sysfs node? Seems it's not align > with the > idea behind sysfs. It maybe that I missed something. > > ... > > > + ret = kstrtou16(buf, 10, &val); > > In other function you have long, here u16. I would expect that the > types are of > the same class, e.g. if here you have u16, then there something like > s32 / s64. > Or here something like unsigned short. > > A bit of elaboration why u16 is chosen here? > > ... > > > + .info_mask_separate_available = > > BIT(IIO_CHAN_INFO_RAW), \ > > + .ext_info = > > ltc2688_ext_info \ > > + Comma > > ... > > > +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st, > > + struct ltc2688_chan *chan, > > + struct device_node *np, int tgp) > > +{ > > + unsigned long rate; > > + struct clk *clk; > > + int ret, f; > > + > > + clk = devm_get_clk_from_child(&st->spi->dev, np, NULL); > > + if (IS_ERR(clk)) > > Make it optional for non-OF, can be done as easy as > > if (IS_ERR(clk)) { > if (PTR_ERR(clk) == -ENOENT) > clk = NULL; > else > return dev_err_probe(...); > } > > > + return dev_err_probe(&st->spi->dev, PTR_ERR(clk), > > + "failed to get tgp clk.\n"); > > + > > + ret = clk_prepare_enable(clk); > > + if (ret) > > + return dev_err_probe(&st->spi->dev, ret, > > + "failed to enable tgp > > clk.\n"); > > + > > + ret = devm_add_action_or_reset(&st->spi->dev, > > ltc2688_clk_disable, clk); > > + if (ret) > > + return ret; > > + > > + if (chan->toggle_chan) > > + return 0; > > + > > + /* calculate available dither frequencies */ > > + rate = clk_get_rate(clk); > > + for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++) > > + chan->dither_frequency[f] = DIV_ROUND_CLOSEST(rate, > > ltc2688_period[f]); > > + > > + return 0; > > +} > > ... > > > +static int ltc2688_channel_config(struct ltc2688_state *st) > > +{ > > + struct device *dev = &st->spi->dev; > > + struct device_node *child; > > + u32 reg, clk_input, val, tmp[2]; > > + int ret, span; > > + > > + for_each_available_child_of_node(dev->of_node, child) { > > device_for_each_child_node() > > > + struct ltc2688_chan *chan; > > + > > + ret = of_property_read_u32(child, "reg", ®); > > + if (ret) { > > + of_node_put(child); > > + return dev_err_probe(dev, ret, > > + "Failed to get reg > > property\n"); > > + } > > + > > + if (reg >= LTC2688_DAC_CHANNELS) { > > + of_node_put(child); > > + return dev_err_probe(dev, -EINVAL, > > + "reg bigger than: > > %d\n", > > + LTC2688_DAC_CHANNELS); > > + } > > + > > + val = 0; > > + chan = &st->channels[reg]; > > + if (of_property_read_bool(child, "adi,toggle- > > mode")) { > > + chan->toggle_chan = true; > > + /* assume sw toggle ABI */ > > + st->iio_chan[reg].ext_info = > > ltc2688_toggle_sym_ext_info; > > + /* > > + * Clear IIO_CHAN_INFO_RAW bit as toggle > > channels expose > > + * out_voltage_raw{0|1} files. > > + */ > > > + clear_bit(IIO_CHAN_INFO_RAW, > > + &st- > > >iio_chan[reg].info_mask_separate); > > Do you need atomic operation here? > > > + } > > + > > + ret = of_property_read_u32_array(child, > > "adi,output-range-microvolt", > > + tmp, > > ARRAY_SIZE(tmp)); > > + if (!ret) { > > + span = ltc2688_span_lookup(st, (int)tmp[0] > > / 1000, > > + tmp[1] / 1000); > > + if (span < 0) { > > + of_node_put(child); > > + return dev_err_probe(dev, -EINVAL, > > + "output range > > not valid:[%d %d]\n", > > + tmp[0], > > tmp[1]); > > + } > > + > > + val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, > > span); > > + } > > + > > + ret = of_property_read_u32(child, "adi,toggle- > > dither-input", > > + &clk_input); > > + if (!ret) { > > + if (clk_input >= LTC2688_CH_TGP_MAX) { > > + of_node_put(child); > > + return dev_err_probe(dev, -EINVAL, > > + "toggle- > > dither-input inv value(%d)\n", > > + clk_input); > > + } > > + > > + ret = ltc2688_tgp_clk_setup(st, chan, > > child, clk_input); > > + if (ret) { > > + of_node_put(child); > > + return ret; > > + } > > + > > + /* > > + * 0 means software toggle which is the > > default mode. > > + * Hence the +1. > > + */ > > + val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, > > clk_input + 1); > > + > > + /* > > + * If a TGPx is given, we automatically > > assume a dither > > + * capable channel (unless toggle is > > already enabled). > > + * On top of this we just set here the > > dither bit in the > > + * channel settings. It won't have any > > effect until the > > + * global toggle/dither bit is enabled. > > + */ > > + if (!chan->toggle_chan) { > > + val |= > > FIELD_PREP(LTC2688_CH_MODE_MSK, 1); > > + st->iio_chan[reg].ext_info = > > ltc2688_dither_ext_info; > > + } else { > > + /* wait, no sw toggle after all */ > > + st->iio_chan[reg].ext_info = > > ltc2688_toggle_ext_info; > > + } > > + } > > + > > + if (of_property_read_bool(child, "adi,overrange")) > > { > > + chan->overrange = true; > > + val |= LTC2688_CH_OVERRANGE_MSK; > > + } > > + > > + if (!val) > > + continue; > > + > > + ret = regmap_write(st->regmap, > > LTC2688_CMD_CH_SETTING(reg), > > + val); > > + if (ret) { > > + of_node_put(child); > > + return dev_err_probe(dev, -EINVAL, > > + "failed to set chan > > settings\n"); > > + } > > + } > > + > > + return 0; > > +} > > ... > > > +struct regmap_bus ltc2688_regmap_bus = { > > + .read = ltc2688_spi_read, > > + .write = ltc2688_spi_write, > > + .read_flag_mask = LTC2688_READ_OPERATION, > > + .reg_format_endian_default = REGMAP_ENDIAN_BIG, > > + .val_format_endian_default = REGMAP_ENDIAN_BIG > > + Comma. > > > +}; > > + > > +static const struct regmap_config ltc2688_regmap_config = { > > + .reg_bits = 8, > > + .val_bits = 16, > > + .readable_reg = ltc2688_reg_readable, > > + .writeable_reg = ltc2688_reg_writable, > > + /* ignoring the no op command */ > > + .max_register = LTC2688_CMD_UPDATE_ALL > > Ditto. > > > +}; > > ... > > > + vref_reg = devm_regulator_get_optional(dev, "vref"); > > > + if (!IS_ERR(vref_reg)) { > > Why not positive conditional check (and hence standard pattern -- > error > handling first)? > > > + ret = regulator_enable(vref_reg); > > + if (ret) > > + return dev_err_probe(dev, ret, > > + "Failed to enable vref > > regulators\n"); > > + > > + ret = devm_add_action_or_reset(dev, > > ltc2688_disable_regulator, > > + vref_reg); > > + if (ret) > > + return ret; > > + > > + ret = regulator_get_voltage(vref_reg); > > + if (ret < 0) > > + return dev_err_probe(dev, ret, "Failed to > > get vref\n"); > > + > > + st->vref = ret / 1000; > > + } else { > > + if (PTR_ERR(vref_reg) != -ENODEV) > > + return dev_err_probe(dev, > > PTR_ERR(vref_reg), > > + "Failed to get vref > > regulator"); > > + > > + vref_reg = NULL; > > + /* internal reference */ > > + st->vref = 4096; > > + } >
On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote: > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote: > > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote: > > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote: > > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote: > > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com> > > > > > > Sent: Saturday, February 5, 2022 6:30 PM > > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: ... > > > > > > Second, why do you need this specific function instead of > > > > > > regmap > > > > > > bulk > > > > > > ops against be24/le24? > > > > > > > > > > Not sure I'm following this one... If you mean why am I using a > > > > > custom > > > > > regmap_bus implementation, that was already explained in the > > > > > RFC > > > > > patch. > > > > > And IIRC, you were the one already asking
On Sun, 2022-02-20 at 13:32 +0200, Andy Shevchenko wrote: > On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote: > > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote: > > > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote: > > > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote: > > > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote: > > > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com> > > > > > > > Sent: Saturday, February 5, 2022 6:30 PM > > > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: > > ... > > > > > > > > Second, why do you need this specific function instead of > > > > > > > regmap > > > > > > > bulk > > > > > > > ops against be24/le24? > > > > > > > > > > > > Not sure I'm following this one... If you mean why am I > > > > > > using a > > > > > > custom > > > > > > regmap_bus implementation, that was already explained in > > > > > > the > > > > > > RFC > > > > > > patch. > > > > > > And IIRC, you were the one already asking
On Mon, Feb 21, 2022 at 01:48:12PM +0100, Nuno Sá wrote: > On Sun, 2022-02-20 at 13:32 +0200, Andy Shevchenko wrote: > > On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote: > > > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote: > > > > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote: > > > > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote: > > > > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote: > > > > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com> > > > > > > > > Sent: Saturday, February 5, 2022 6:30 PM > > > > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: ... > > > > > > > > > + ret = kstrtou16(buf, 10, &val); > > > > > > > > > > > > > > > > In other function you have long, here u16. I would expect that > > > > > > > > the types are of the same class, e.g. if here you have u16, > > > > > > > > then there something like s32 / s64. Or here something like > > > > > > > > unsigned short. > > > > > > > > > > > > > > > > A bit of elaboration why u16 is chosen here? > > > > > > > > > > > > > > Well, I never really saw any enforcement here to be honest > > > > > > > (rather than using stdint types...). So I pretty much just use > > > > > > > these in unsigned types because I'm lazy and u16 is faster to > > > > > > > type than unsigned short... In this case, unless Jonathan really > > > > > > > asks for it, I prefer not to go all over the driver and change > > > > > > > this... > > > > > > > > > > > > This is about consistency. It may work as is, but it feels not good > > > > > > when for int (or unsigned int) one uses fixed-width types. Also > > > > > > it's non- written advice to use fixed-width variables when it's > > > > > > about programming registers or so, for the rest, use POD types. > > > > > > Ok, going a bit back in the discussion, you argued that in one place I > > > was using long while here u16. Well, in the place I'm using long, that > > > was on purpose because that value is to be compared against an array of > > > longs (which has to be long because it depends on CCF rates). I guess I > > > can als0 use s64, but there is also a reason why long was used. > > > > > > In the u16 case, we really want to have 2 bytes because I'm going to use > > > that value to write the dac code which is 2 bytes. > > > > Okay, that's what I want to hear. If it's indeed goes to be a value to the > > register, then it's fine. > > > > Perhaps a comment? > > I guess you mean to have a comment to state that here we have fixed > size type (as opposed to long, used in another place), because we > directly use the value on a register write? > > Asking it because I'm not planning to add comments in all the places > where I have fixed size types for register read/writes... Thinking more about it and now I'm convinced that using the value that goes to the register in ABI is bad idea (means that user space must not care about the size or contents of the hardware register and should be abstract representation of the HW). OTOH this seems to be "raw" value of something. So, I maybe missed the convention in IIO about this kind of values WRT the variable types used on ABI side. That said, I leave it to Jonathan since I'm not convinced that u16 is a proper choice here. > > > > > I can understand your reasoning but again this is something that I > > > > > never really saw being enforced. So, I'm more than ok to change it if > > > > > it really becomes something that we will try to "enforce" in IIO. > > > > > Otherwise it just feels as a random nitpick :). > > > > > > > > No, this is about consistency and common sense. If you define type uXX, > > > > we have an API for that exact type. It's confusing why POD type APIs > > > > are used with fixed-width types or vise versa. > > > > > > > > Moreover (which is pure theoretical, though) some architectures might > > > > have no (mutual) equivalency between these types.
On Mon, 21 Feb 2022 19:04:38 +0200 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Mon, Feb 21, 2022 at 01:48:12PM +0100, Nuno Sá wrote: > > On Sun, 2022-02-20 at 13:32 +0200, Andy Shevchenko wrote: > > > On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote: > > > > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote: > > > > > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote: > > > > > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote: > > > > > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote: > > > > > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com> > > > > > > > > > Sent: Saturday, February 5, 2022 6:30 PM > > > > > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: > > ... > > > > > > > > > > > + ret = kstrtou16(buf, 10, &val); > > > > > > > > > > > > > > > > > > In other function you have long, here u16. I would expect that > > > > > > > > > the types are of the same class, e.g. if here you have u16, > > > > > > > > > then there something like s32 / s64. Or here something like > > > > > > > > > unsigned short. > > > > > > > > > > > > > > > > > > A bit of elaboration why u16 is chosen here? > > > > > > > > > > > > > > > > Well, I never really saw any enforcement here to be honest > > > > > > > > (rather than using stdint types...). So I pretty much just use > > > > > > > > these in unsigned types because I'm lazy and u16 is faster to > > > > > > > > type than unsigned short... In this case, unless Jonathan really > > > > > > > > asks for it, I prefer not to go all over the driver and change > > > > > > > > this... > > > > > > > > > > > > > > This is about consistency. It may work as is, but it feels not good > > > > > > > when for int (or unsigned int) one uses fixed-width types. Also > > > > > > > it's non- written advice to use fixed-width variables when it's > > > > > > > about programming registers or so, for the rest, use POD types. > > > > > > > > Ok, going a bit back in the discussion, you argued that in one place I > > > > was using long while here u16. Well, in the place I'm using long, that > > > > was on purpose because that value is to be compared against an array of > > > > longs (which has to be long because it depends on CCF rates). I guess I > > > > can als0 use s64, but there is also a reason why long was used. > > > > > > > > In the u16 case, we really want to have 2 bytes because I'm going to use > > > > that value to write the dac code which is 2 bytes. > > > > > > Okay, that's what I want to hear. If it's indeed goes to be a value to the > > > register, then it's fine. > > > > > > Perhaps a comment? > > > > I guess you mean to have a comment to state that here we have fixed > > size type (as opposed to long, used in another place), because we > > directly use the value on a register write? > > > > Asking it because I'm not planning to add comments in all the places > > where I have fixed size types for register read/writes... > > Thinking more about it and now I'm convinced that using the value that goes to > the register in ABI is bad idea (means that user space must not care about the > size or contents of the hardware register and should be abstract representation > of the HW). > > OTOH this seems to be "raw" value of something. So, I maybe missed the convention > in IIO about this kind of values WRT the variable types used on ABI side. > > That said, I leave it to Jonathan since I'm not convinced that u16 is a proper > choice here. From a userspace point of view it doesn't care as it's writing a string. In this particular case the string only has valid values that from 0-(2^16-1) (i.e. 16 bits). So if it writes outside of that range it is an error. You could read it into an unsigned long and then check against the range, but there is little point given you'd still return an error if it was out of range. The fact that kstrto16() does that for you really just a shortcut though it will return -ERANGE rather than perhaps -EINVAL which might be used for a more generic "not this value". Userspace can also read the range that is acceptable from out_voltage0_raw_available [0 1 2^16-1] and hence not write an invalid value in the first place - which is obviously preferred to getting an error. Scaling etc is also expressed to userspace so it it wants to write a particular voltage it can perform the appropriate scaling. Note that moving linear scaling like this to userspace allows easy use of floating point + may be a significant performance advantage if using the chrdev interface which uses the same approach (and values) as the sysfs interface. Jonathan > > > > > > > I can understand your reasoning but again this is something that I > > > > > > never really saw being enforced. So, I'm more than ok to change it if > > > > > > it really becomes something that we will try to "enforce" in IIO. > > > > > > Otherwise it just feels as a random nitpick :). > > > > > > > > > > No, this is about consistency and common sense. If you define type uXX, > > > > > we have an API for that exact type. It's confusing why POD type APIs > > > > > are used with fixed-width types or vise versa. > > > > > > > > > > Moreover (which is pure theoretical, though) some architectures might > > > > > have no (mutual) equivalency between these types. >
On Mon, Feb 21, 2022 at 05:30:45PM +0000, Jonathan Cameron wrote: > On Mon, 21 Feb 2022 19:04:38 +0200 > Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > > On Mon, Feb 21, 2022 at 01:48:12PM +0100, Nuno Sá wrote: > > > On Sun, 2022-02-20 at 13:32 +0200, Andy Shevchenko wrote: > > > > On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote: > > > > > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote: > > > > > > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote: > > > > > > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote: > > > > > > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote: > > > > > > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com> > > > > > > > > > > Sent: Saturday, February 5, 2022 6:30 PM > > > > > > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: > > > > ... > > > > > > > > > > > > > + ret = kstrtou16(buf, 10, &val); > > > > > > > > > > > > > > > > > > > > In other function you have long, here u16. I would expect that > > > > > > > > > > the types are of the same class, e.g. if here you have u16, > > > > > > > > > > then there something like s32 / s64. Or here something like > > > > > > > > > > unsigned short. > > > > > > > > > > > > > > > > > > > > A bit of elaboration why u16 is chosen here? > > > > > > > > > > > > > > > > > > Well, I never really saw any enforcement here to be honest > > > > > > > > > (rather than using stdint types...). So I pretty much just use > > > > > > > > > these in unsigned types because I'm lazy and u16 is faster to > > > > > > > > > type than unsigned short... In this case, unless Jonathan really > > > > > > > > > asks for it, I prefer not to go all over the driver and change > > > > > > > > > this... > > > > > > > > > > > > > > > > This is about consistency. It may work as is, but it feels not good > > > > > > > > when for int (or unsigned int) one uses fixed-width types. Also > > > > > > > > it's non- written advice to use fixed-width variables when it's > > > > > > > > about programming registers or so, for the rest, use POD types. > > > > > > > > > > Ok, going a bit back in the discussion, you argued that in one place I > > > > > was using long while here u16. Well, in the place I'm using long, that > > > > > was on purpose because that value is to be compared against an array of > > > > > longs (which has to be long because it depends on CCF rates). I guess I > > > > > can als0 use s64, but there is also a reason why long was used. > > > > > > > > > > In the u16 case, we really want to have 2 bytes because I'm going to use > > > > > that value to write the dac code which is 2 bytes. > > > > > > > > Okay, that's what I want to hear. If it's indeed goes to be a value to the > > > > register, then it's fine. > > > > > > > > Perhaps a comment? > > > > > > I guess you mean to have a comment to state that here we have fixed > > > size type (as opposed to long, used in another place), because we > > > directly use the value on a register write? > > > > > > Asking it because I'm not planning to add comments in all the places > > > where I have fixed size types for register read/writes... > > > > Thinking more about it and now I'm convinced that using the value that goes to > > the register in ABI is bad idea (means that user space must not care about the > > size or contents of the hardware register and should be abstract representation > > of the HW). > > > > OTOH this seems to be "raw" value of something. So, I maybe missed the convention > > in IIO about this kind of values WRT the variable types used on ABI side. > > > > That said, I leave it to Jonathan since I'm not convinced that u16 is a proper > > choice here. > > From a userspace point of view it doesn't care as it's writing a string. > In this particular case the string only has valid values that from 0-(2^16-1) > (i.e. 16 bits). So if it writes outside of that range it is an error. > You could read it into an unsigned long and then check against the range, > but there is little point given you'd still return an error if it was out of > range. The fact that kstrto16() does that for you really just a shortcut > though it will return -ERANGE rather than perhaps -EINVAL which might be used > for a more generic "not this value". > > Userspace can also read the range that is acceptable from > out_voltage0_raw_available [0 1 2^16-1] and hence not write an invalid value > in the first place - which is obviously preferred to getting an error. > Scaling etc is also expressed to userspace so it it wants to write a particular > voltage it can perform the appropriate scaling. Note that moving linear scaling > like this to userspace allows easy use of floating point + may be a significant > performance advantage if using the chrdev interface which uses the same > approach (and values) as the sysfs interface. With the same logic it can be unsigned short, no? The point is to use u16 when it's indeed fixed-width value that goes to hardware or being used as part of a protocol. And thus mentioning of the IOCTL protocols may justify the choice. Then the question to the other values, shouldn't they be also fixed-width ones? > > > > > > > I can understand your reasoning but again this is something that I > > > > > > > never really saw being enforced. So, I'm more than ok to change it if > > > > > > > it really becomes something that we will try to "enforce" in IIO. > > > > > > > Otherwise it just feels as a random nitpick :). > > > > > > > > > > > > No, this is about consistency and common sense. If you define type uXX, > > > > > > we have an API for that exact type. It's confusing why POD type APIs > > > > > > are used with fixed-width types or vise versa. > > > > > > > > > > > > Moreover (which is pure theoretical, though) some architectures might > > > > > > have no (mutual) equivalency between these types.
On Mon, 21 Feb 2022 20:49:48 +0200 Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > On Mon, Feb 21, 2022 at 05:30:45PM +0000, Jonathan Cameron wrote: > > On Mon, 21 Feb 2022 19:04:38 +0200 > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote: > > > > > On Mon, Feb 21, 2022 at 01:48:12PM +0100, Nuno Sá wrote: > > > > On Sun, 2022-02-20 at 13:32 +0200, Andy Shevchenko wrote: > > > > > On Fri, Feb 18, 2022 at 02:51:28PM +0100, Nuno Sá wrote: > > > > > > On Mon, 2022-02-14 at 15:49 +0200, Andy Shevchenko wrote: > > > > > > > On Mon, Feb 07, 2022 at 09:19:46PM +0100, Nuno Sá wrote: > > > > > > > > On Mon, 2022-02-07 at 13:09 +0200, Andy Shevchenko wrote: > > > > > > > > > On Sun, Feb 06, 2022 at 01:19:59PM +0000, Sa, Nuno wrote: > > > > > > > > > > > From: Andy Shevchenko <andriy.shevchenko@intel.com> > > > > > > > > > > > Sent: Saturday, February 5, 2022 6:30 PM > > > > > > > > > > > On Fri, Jan 21, 2022 at 03:24:59PM +0100, Nuno Sá wrote: > > > > > > ... > > > > > > > > > > > > > > > + ret = kstrtou16(buf, 10, &val); > > > > > > > > > > > > > > > > > > > > > > In other function you have long, here u16. I would expect that > > > > > > > > > > > the types are of the same class, e.g. if here you have u16, > > > > > > > > > > > then there something like s32 / s64. Or here something like > > > > > > > > > > > unsigned short. > > > > > > > > > > > > > > > > > > > > > > A bit of elaboration why u16 is chosen here? > > > > > > > > > > > > > > > > > > > > Well, I never really saw any enforcement here to be honest > > > > > > > > > > (rather than using stdint types...). So I pretty much just use > > > > > > > > > > these in unsigned types because I'm lazy and u16 is faster to > > > > > > > > > > type than unsigned short... In this case, unless Jonathan really > > > > > > > > > > asks for it, I prefer not to go all over the driver and change > > > > > > > > > > this... > > > > > > > > > > > > > > > > > > This is about consistency. It may work as is, but it feels not good > > > > > > > > > when for int (or unsigned int) one uses fixed-width types. Also > > > > > > > > > it's non- written advice to use fixed-width variables when it's > > > > > > > > > about programming registers or so, for the rest, use POD types. > > > > > > > > > > > > Ok, going a bit back in the discussion, you argued that in one place I > > > > > > was using long while here u16. Well, in the place I'm using long, that > > > > > > was on purpose because that value is to be compared against an array of > > > > > > longs (which has to be long because it depends on CCF rates). I guess I > > > > > > can als0 use s64, but there is also a reason why long was used. > > > > > > > > > > > > In the u16 case, we really want to have 2 bytes because I'm going to use > > > > > > that value to write the dac code which is 2 bytes. > > > > > > > > > > Okay, that's what I want to hear. If it's indeed goes to be a value to the > > > > > register, then it's fine. > > > > > > > > > > Perhaps a comment? > > > > > > > > I guess you mean to have a comment to state that here we have fixed > > > > size type (as opposed to long, used in another place), because we > > > > directly use the value on a register write? > > > > > > > > Asking it because I'm not planning to add comments in all the places > > > > where I have fixed size types for register read/writes... > > > > > > Thinking more about it and now I'm convinced that using the value that goes to > > > the register in ABI is bad idea (means that user space must not care about the > > > size or contents of the hardware register and should be abstract representation > > > of the HW). > > > > > > OTOH this seems to be "raw" value of something. So, I maybe missed the convention > > > in IIO about this kind of values WRT the variable types used on ABI side. > > > > > > That said, I leave it to Jonathan since I'm not convinced that u16 is a proper > > > choice here. > > > > From a userspace point of view it doesn't care as it's writing a string. > > In this particular case the string only has valid values that from 0-(2^16-1) > > (i.e. 16 bits). So if it writes outside of that range it is an error. > > You could read it into an unsigned long and then check against the range, > > but there is little point given you'd still return an error if it was out of > > range. The fact that kstrto16() does that for you really just a shortcut > > though it will return -ERANGE rather than perhaps -EINVAL which might be used > > for a more generic "not this value". > > > > Userspace can also read the range that is acceptable from > > out_voltage0_raw_available [0 1 2^16-1] and hence not write an invalid value > > in the first place - which is obviously preferred to getting an error. > > Scaling etc is also expressed to userspace so it it wants to write a particular > > voltage it can perform the appropriate scaling. Note that moving linear scaling > > like this to userspace allows easy use of floating point + may be a significant > > performance advantage if using the chrdev interface which uses the same > > approach (and values) as the sysfs interface. > > With the same logic it can be unsigned short, no? It could be any integer as long as it is at least as large as a u16. But it it is larger than a u16 you'll need an additional check on the maximum. > > The point is to use u16 when it's indeed fixed-width value that goes to > hardware or being used as part of a protocol. And thus mentioning of the > IOCTL protocols may justify the choice. Then the question to the other > values, shouldn't they be also fixed-width ones? If we had a fixed width type that took the values 0-4 sure using such a magic type would make sense, but we don't. Note that internally kstrtou16 is just strtoull and a range check. The one other case we have here does pretty much the same thing. Jonathan > > > > > > > > > I can understand your reasoning but again this is something that I > > > > > > > > never really saw being enforced. So, I'm more than ok to change it if > > > > > > > > it really becomes something that we will try to "enforce" in IIO. > > > > > > > > Otherwise it just feels as a random nitpick :). > > > > > > > > > > > > > > No, this is about consistency and common sense. If you define type uXX, > > > > > > > we have an API for that exact type. It's confusing why POD type APIs > > > > > > > are used with fixed-width types or vise versa. > > > > > > > > > > > > > > Moreover (which is pure theoretical, though) some architectures might > > > > > > > have no (mutual) equivalency between these types. >
diff --git a/MAINTAINERS b/MAINTAINERS index 78881bbaf3c0..419b181f4749 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11190,6 +11190,13 @@ S: Maintained F: Documentation/devicetree/bindings/iio/dac/lltc,ltc1660.yaml F: drivers/iio/dac/ltc1660.c +LTC2688 IIO DAC DRIVER +M: Nuno Sá <nuno.sa@analog.com> +L: linux-iio@vger.kernel.org +S: Supported +W: http://ez.analog.com/community/linux-device-drivers +F: drivers/iio/dac/ltc2688.c + LTC2947 HARDWARE MONITOR DRIVER M: Nuno Sá <nuno.sa@analog.com> L: linux-hwmon@vger.kernel.org diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig index bfcf7568de32..c0bf0d84197f 100644 --- a/drivers/iio/dac/Kconfig +++ b/drivers/iio/dac/Kconfig @@ -131,6 +131,17 @@ config AD5624R_SPI Say yes here to build support for Analog Devices AD5624R, AD5644R and AD5664R converters (DAC). This driver uses the common SPI interface. +config LTC2688 + tristate "Analog Devices LTC2688 DAC spi driver" + depends on SPI + select REGMAP + help + Say yes here to build support for Analog Devices + LTC2688 converters (DAC). + + To compile this driver as a module, choose M here: the + module will be called ltc2688. + config AD5686 tristate diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile index 01a50131572f..ec3e42713f00 100644 --- a/drivers/iio/dac/Makefile +++ b/drivers/iio/dac/Makefile @@ -35,6 +35,7 @@ obj-$(CONFIG_DS4424) += ds4424.o obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o obj-$(CONFIG_LTC1660) += ltc1660.o obj-$(CONFIG_LTC2632) += ltc2632.o +obj-$(CONFIG_LTC2688) += ltc2688.o obj-$(CONFIG_M62332) += m62332.o obj-$(CONFIG_MAX517) += max517.o obj-$(CONFIG_MAX5821) += max5821.o diff --git a/drivers/iio/dac/ltc2688.c b/drivers/iio/dac/ltc2688.c new file mode 100644 index 000000000000..179a653ce972 --- /dev/null +++ b/drivers/iio/dac/ltc2688.c @@ -0,0 +1,1070 @@ +// 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/iio/iio.h> +#include <linux/limits.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mod_devicetable.h> +#include <linux/mutex.h> +#include <linux/of.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/spi/spi.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 0x70 +#define LTC2688_CMD_POWERDOWN 0x71 +#define LTC2688_CMD_A_B_SELECT 0x72 +#define LTC2688_CMD_SW_TOGGLE 0x73 +#define LTC2688_CMD_TOGGLE_DITHER_EN 0x74 +#define LTC2688_CMD_THERMAL_STAT 0x77 +#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_TGP_MAX 3 +#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) + +#define LTC2688_DITHER_RAW_MASK GENMASK(15, 2) +#define LTC2688_CH_CALIBBIAS_MASK GENMASK(15, 2) +#define LTC2688_DITHER_RAW_MAX_VAL (BIT(14) - 1) +#define LTC2688_CH_CALIBBIAS_MAX_VAL (BIT(14) - 1) + +/* Configuration register */ +#define LTC2688_CONFIG_RST BIT(15) +#define LTC2688_CONFIG_EXT_REF BIT(1) + +#define LTC2688_DITHER_FREQ_AVAIL_N 5 + +enum { + LTC2688_SPAN_RANGE_0V_5V, + LTC2688_SPAN_RANGE_0V_10V, + LTC2688_SPAN_RANGE_M5V_5V, + LTC2688_SPAN_RANGE_M10V_10V, + LTC2688_SPAN_RANGE_M15V_15V, + LTC2688_SPAN_RANGE_MAX +}; + +enum { + LTC2688_MODE_DEFAULT, + LTC2688_MODE_DITHER_TOGGLE, +}; + +struct ltc2688_chan { + long dither_frequency[LTC2688_DITHER_FREQ_AVAIL_N]; + bool overrange; + bool toggle_chan; + u8 mode; +}; + +struct ltc2688_state { + struct spi_device *spi; + struct regmap *regmap; + struct regulator_bulk_data regulators[2]; + struct ltc2688_chan channels[LTC2688_DAC_CHANNELS]; + struct iio_chan_spec *iio_chan; + /* lock to protect against multiple access to the device and shared data */ + struct mutex lock; + int vref; + /* + * DMA (thus cache coherency maintenance) requires the + * transfer buffers to live in their own cache lines. + */ + u8 tx_data[6] ____cacheline_aligned; + u8 rx_data[3]; +}; + +static int ltc2688_spi_read(void *context, const void *reg, size_t reg_size, + void *val, size_t val_size) +{ + 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; + + memcpy(st->tx_data, reg, reg_size); + + ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers)); + if (ret) + return ret; + + memcpy(val, &st->rx_data[1], val_size); + + return 0; +} + +static int ltc2688_spi_write(void *context, const void *data, size_t count) +{ + struct ltc2688_state *st = context; + + return spi_write(st->spi, data, count); +} + +static int ltc2688_span_get(const struct ltc2688_state *st, int c) +{ + int ret, reg, span; + + 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; + + return span; +} + +static const int ltc2688_span_helper[LTC2688_SPAN_RANGE_MAX][2] = { + {0, 5000}, {0, 10000}, {-5000, 5000}, {-10000, 10000}, {-15000, 15000}, +}; + +static int ltc2688_scale_get(const struct ltc2688_state *st, int c, int *val) +{ + const struct ltc2688_chan *chan = &st->channels[c]; + int span, fs; + + span = ltc2688_span_get(st, c); + if (span < 0) + return span; + + fs = ltc2688_span_helper[span][1] - ltc2688_span_helper[span][0]; + if (chan->overrange) + fs = mult_frac(fs, 105, 100); + + *val = DIV_ROUND_CLOSEST(fs * st->vref, 4096); + + return 0; +} + +static int ltc2688_offset_get(const struct ltc2688_state *st, int c, int *val) +{ + int span; + + span = ltc2688_span_get(st, c); + if (span < 0) + return span; + + if (ltc2688_span_helper[span][0] < 0) + *val = -32768; + else + *val = 0; + + return 0; +} + +enum { + LTC2688_INPUT_A, + LTC2688_INPUT_B, + LTC2688_INPUT_B_AVAIL, + LTC2688_DITHER_OFF, + LTC2688_DITHER_FREQ_AVAIL, +}; + +static int ltc2688_dac_code_write(struct ltc2688_state *st, u32 chan, u32 input, + u16 code) +{ + struct ltc2688_chan *c = &st->channels[chan]; + int ret, reg; + + /* 2 LSBs set to 0 if writing dither amplitude */ + if (!c->toggle_chan && input == LTC2688_INPUT_B) { + if (code > LTC2688_DITHER_RAW_MAX_VAL) + return -EINVAL; + + code = FIELD_PREP(LTC2688_DITHER_RAW_MASK, code); + } + + mutex_lock(&st->lock); + /* select the correct input register to read from */ + ret = regmap_update_bits(st->regmap, LTC2688_CMD_A_B_SELECT, BIT(chan), + input << chan); + if (ret) + goto unlock; + + /* + * If in dither/toggle mode the dac should be updated by an + * external signal (or sw toggle) and not here. + */ + if (c->mode == LTC2688_MODE_DEFAULT) + reg = LTC2688_CMD_CH_CODE_UPDATE(chan); + else + reg = LTC2688_CMD_CH_CODE(chan); + + ret = regmap_write(st->regmap, reg, code); +unlock: + mutex_unlock(&st->lock); + return ret; +} + +static int ltc2688_dac_code_read(struct ltc2688_state *st, u32 chan, u32 input, + u32 *code) +{ + struct ltc2688_chan *c = &st->channels[chan]; + int ret; + + mutex_lock(&st->lock); + ret = regmap_update_bits(st->regmap, LTC2688_CMD_A_B_SELECT, BIT(chan), + input << chan); + if (ret) + goto unlock; + + ret = regmap_read(st->regmap, LTC2688_CMD_CH_CODE(chan), code); +unlock: + mutex_unlock(&st->lock); + + if (!c->toggle_chan && input == LTC2688_INPUT_B) + *code = FIELD_GET(LTC2688_DITHER_RAW_MASK, *code); + + return ret; +} + +static const int ltc2688_raw_range[] = {0, 1, U16_MAX}; + +static int ltc2688_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int **vals, int *type, int *length, + long info) +{ + switch (info) { + case IIO_CHAN_INFO_RAW: + *vals = ltc2688_raw_range; + *type = IIO_VAL_INT; + return IIO_AVAIL_RANGE; + default: + return -EINVAL; + } +} + +static int ltc2688_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long info) +{ + struct ltc2688_state *st = iio_priv(indio_dev); + int ret; + + switch (info) { + case IIO_CHAN_INFO_RAW: + ret = ltc2688_dac_code_read(st, chan->channel, LTC2688_INPUT_A, + val); + if (ret) + return ret; + + return IIO_VAL_INT; + case IIO_CHAN_INFO_OFFSET: + ret = ltc2688_offset_get(st, chan->channel, val); + if (ret) + return ret; + + return IIO_VAL_INT; + case IIO_CHAN_INFO_SCALE: + ret = ltc2688_scale_get(st, chan->channel, val); + if (ret) + return ret; + + *val = 16; + return IIO_VAL_FRACTIONAL_LOG2; + case IIO_CHAN_INFO_CALIBBIAS: + ret = regmap_read(st->regmap, + LTC2688_CMD_CH_OFFSET(chan->channel), val); + if (ret) + return ret; + + *val = FIELD_GET(LTC2688_CH_CALIBBIAS_MASK, *val); + 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(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, + int val2, long info) +{ + struct ltc2688_state *st = iio_priv(indio_dev); + + switch (info) { + case IIO_CHAN_INFO_RAW: + if (val > U16_MAX || val < 0) + return -EINVAL; + + return ltc2688_dac_code_write(st, chan->channel, + LTC2688_INPUT_A, val); + case IIO_CHAN_INFO_CALIBBIAS: + if (val > LTC2688_CH_CALIBBIAS_MAX_VAL) + return -EINVAL; + + return regmap_write(st->regmap, + LTC2688_CMD_CH_OFFSET(chan->channel), + FIELD_PREP(LTC2688_CH_CALIBBIAS_MASK, val)); + case IIO_CHAN_INFO_CALIBSCALE: + return regmap_write(st->regmap, + LTC2688_CMD_CH_GAIN(chan->channel), val); + default: + return -EINVAL; + } +} + +static ssize_t ltc2688_dither_toggle_set(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); + struct ltc2688_chan *c = &st->channels[chan->channel]; + int ret; + bool en; + + ret = kstrtobool(buf, &en); + if (ret) + return ret; + + mutex_lock(&st->lock); + ret = regmap_update_bits(st->regmap, LTC2688_CMD_TOGGLE_DITHER_EN, + BIT(chan->channel), en << chan->channel); + if (ret) + goto unlock; + + c->mode = en ? LTC2688_MODE_DITHER_TOGGLE : LTC2688_MODE_DEFAULT; +unlock: + mutex_unlock(&st->lock); + + return ret ?: len; +} + +static ssize_t ltc2688_reg_bool_get(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + char *buf) +{ + const struct ltc2688_state *st = iio_priv(indio_dev); + int ret; + u32 val; + + ret = regmap_read(st->regmap, private, &val); + if (ret) + return ret; + + return sysfs_emit(buf, "%u\n", !!(val & BIT(chan->channel))); +} + +static ssize_t ltc2688_reg_bool_set(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + const char *buf, size_t len) +{ + const struct ltc2688_state *st = iio_priv(indio_dev); + int ret; + bool en; + + ret = kstrtobool(buf, &en); + if (ret) + return ret; + + ret = regmap_update_bits(st->regmap, private, BIT(chan->channel), + en << chan->channel); + if (ret) + return ret; + + return len; +} + +static ssize_t ltc2688_dither_freq_avail(const struct ltc2688_state *st, + const struct ltc2688_chan *chan, + char *buf) +{ + int sz = 0; + u32 f; + + for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++) + sz += sysfs_emit_at(buf, sz, "%ld ", chan->dither_frequency[f]); + + buf[sz - 1] = '\n'; + + return sz; +} + +static ssize_t ltc2688_dither_freq_get(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + char *buf) +{ + const struct ltc2688_state *st = iio_priv(indio_dev); + const struct ltc2688_chan *c = &st->channels[chan->channel]; + u32 reg, freq; + int ret; + + if (private == LTC2688_DITHER_FREQ_AVAIL) + return ltc2688_dither_freq_avail(st, c, buf); + + ret = regmap_read(st->regmap, LTC2688_CMD_CH_SETTING(chan->channel), + ®); + if (ret) + return ret; + + freq = FIELD_GET(LTC2688_CH_DIT_PER_MSK, reg); + if (freq >= ARRAY_SIZE(c->dither_frequency)) + return -EIO; + + return sysfs_emit(buf, "%ld\n", c->dither_frequency[freq]); +} + +static ssize_t ltc2688_dither_freq_set(struct iio_dev *indio_dev, + uintptr_t private, + const struct iio_chan_spec *chan, + const char *buf, size_t len) +{ + const struct ltc2688_state *st = iio_priv(indio_dev); + const struct ltc2688_chan *c = &st->channels[chan->channel]; + long val; + u32 freq; + int ret; + + if (private == LTC2688_DITHER_FREQ_AVAIL) + return -EINVAL; + + ret = kstrtol(buf, 10, &val); + if (ret) + return ret; + + for (freq = 0; freq < ARRAY_SIZE(c->dither_frequency); freq++) { + if (val == c->dither_frequency[freq]) + break; + } + + if (freq == ARRAY_SIZE(c->dither_frequency)) + return -EINVAL; + + ret = regmap_update_bits(st->regmap, + LTC2688_CMD_CH_SETTING(chan->channel), + LTC2688_CH_DIT_PER_MSK, + FIELD_PREP(LTC2688_CH_DIT_PER_MSK, freq)); + if (ret) + return ret; + + return len; +} + +static ssize_t ltc2688_dac_input_read(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; + u32 val; + + if (private == LTC2688_INPUT_B_AVAIL) + return sysfs_emit(buf, "[%u %u %u]\n", ltc2688_raw_range[0], + ltc2688_raw_range[1], + ltc2688_raw_range[2] / 4); + + if (private == LTC2688_DITHER_OFF) + return sysfs_emit(buf, "0\n"); + + ret = ltc2688_dac_code_read(st, chan->channel, private, &val); + if (ret) + return ret; + + return sysfs_emit(buf, "%u\n", val); +} + +static ssize_t ltc2688_dac_input_write(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); + int ret; + u16 val; + + if (private == LTC2688_INPUT_B_AVAIL || private == LTC2688_DITHER_OFF) + return -EINVAL; + + ret = kstrtou16(buf, 10, &val); + if (ret) + return ret; + + ret = ltc2688_dac_code_write(st, chan->channel, private, val); + if (ret) + return ret; + + return len; +} + +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_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); + + return regmap_write(st->regmap, reg, writeval); +} + +static const char * const ltc2688_dither_phase[] = { + "0", "1.5708", "3.14159", "4.71239", +}; + +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, _read, _write) { \ + .name = _name, \ + .read = (_read), \ + .write = (_write), \ + .private = (_what), \ + .shared = (_shared), \ +} + +/* + * For toggle mode we only expose the symbol attr (sw_toggle) in case a TGPx is + * not provided in dts. + */ +static const struct iio_chan_spec_ext_info ltc2688_toggle_sym_ext_info[] = { + LTC2688_CHAN_EXT_INFO("raw0", LTC2688_INPUT_A, IIO_SEPARATE, + ltc2688_dac_input_read, ltc2688_dac_input_write), + LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B, IIO_SEPARATE, + ltc2688_dac_input_read, ltc2688_dac_input_write), + LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_CMD_TOGGLE_DITHER_EN, + IIO_SEPARATE, ltc2688_reg_bool_get, + ltc2688_dither_toggle_set), + LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_CMD_POWERDOWN, IIO_SEPARATE, + ltc2688_reg_bool_get, ltc2688_reg_bool_set), + LTC2688_CHAN_EXT_INFO("symbol", LTC2688_CMD_SW_TOGGLE, IIO_SEPARATE, + ltc2688_reg_bool_get, ltc2688_reg_bool_set), + {} +}; + +static const struct iio_chan_spec_ext_info ltc2688_toggle_ext_info[] = { + LTC2688_CHAN_EXT_INFO("raw0", LTC2688_INPUT_A, IIO_SEPARATE, + ltc2688_dac_input_read, ltc2688_dac_input_write), + LTC2688_CHAN_EXT_INFO("raw1", LTC2688_INPUT_B, IIO_SEPARATE, + ltc2688_dac_input_read, ltc2688_dac_input_write), + LTC2688_CHAN_EXT_INFO("toggle_en", LTC2688_CMD_TOGGLE_DITHER_EN, + IIO_SEPARATE, ltc2688_reg_bool_get, + ltc2688_dither_toggle_set), + LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_CMD_POWERDOWN, IIO_SEPARATE, + ltc2688_reg_bool_get, ltc2688_reg_bool_set), + {} +}; + +static struct iio_chan_spec_ext_info ltc2688_dither_ext_info[] = { + LTC2688_CHAN_EXT_INFO("dither_raw", LTC2688_INPUT_B, IIO_SEPARATE, + ltc2688_dac_input_read, ltc2688_dac_input_write), + LTC2688_CHAN_EXT_INFO("dither_raw_available", LTC2688_INPUT_B_AVAIL, + IIO_SEPARATE, ltc2688_dac_input_read, + ltc2688_dac_input_write), + LTC2688_CHAN_EXT_INFO("dither_offset", LTC2688_DITHER_OFF, IIO_SEPARATE, + ltc2688_dac_input_read, ltc2688_dac_input_write), + /* + * Not IIO_ENUM because the available freq needs to be computed at + * probe. We could still use it, but it didn't felt much right. + */ + LTC2688_CHAN_EXT_INFO("dither_frequency", 0, IIO_SEPARATE, + ltc2688_dither_freq_get, ltc2688_dither_freq_set), + LTC2688_CHAN_EXT_INFO("dither_frequency_available", + LTC2688_DITHER_FREQ_AVAIL, IIO_SEPARATE, + ltc2688_dither_freq_get, ltc2688_dither_freq_set), + IIO_ENUM("dither_phase", IIO_SEPARATE, <c2688_dither_phase_enum), + IIO_ENUM_AVAILABLE("dither_phase", IIO_SEPARATE, + <c2688_dither_phase_enum), + LTC2688_CHAN_EXT_INFO("dither_en", LTC2688_CMD_TOGGLE_DITHER_EN, + IIO_SEPARATE, ltc2688_reg_bool_get, + ltc2688_dither_toggle_set), + LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_CMD_POWERDOWN, IIO_SEPARATE, + ltc2688_reg_bool_get, ltc2688_reg_bool_set), + {} +}; + +static const struct iio_chan_spec_ext_info ltc2688_ext_info[] = { + LTC2688_CHAN_EXT_INFO("powerdown", LTC2688_CMD_POWERDOWN, IIO_SEPARATE, + ltc2688_reg_bool_get, ltc2688_reg_bool_set), + {} +}; + +#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_RAW), \ + .ext_info = ltc2688_ext_info \ +} + +static const 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), +}; + +static void ltc2688_clk_disable(void *clk) +{ + clk_disable_unprepare(clk); +} + +static const int ltc2688_period[LTC2688_DITHER_FREQ_AVAIL_N] = { + 4, 8, 16, 32, 64, +}; + +static int ltc2688_tgp_clk_setup(struct ltc2688_state *st, + struct ltc2688_chan *chan, + struct device_node *np, int tgp) +{ + unsigned long rate; + struct clk *clk; + int ret, f; + + clk = devm_get_clk_from_child(&st->spi->dev, np, NULL); + if (IS_ERR(clk)) + return dev_err_probe(&st->spi->dev, PTR_ERR(clk), + "failed to get tgp clk.\n"); + + ret = clk_prepare_enable(clk); + if (ret) + return dev_err_probe(&st->spi->dev, ret, + "failed to enable tgp clk.\n"); + + ret = devm_add_action_or_reset(&st->spi->dev, ltc2688_clk_disable, clk); + if (ret) + return ret; + + if (chan->toggle_chan) + return 0; + + /* calculate available dither frequencies */ + rate = clk_get_rate(clk); + for (f = 0; f < ARRAY_SIZE(chan->dither_frequency); f++) + chan->dither_frequency[f] = DIV_ROUND_CLOSEST(rate, ltc2688_period[f]); + + return 0; +} + +static int ltc2688_span_lookup(const struct ltc2688_state *st, int min, int max) +{ + u32 span; + + for (span = 0; span < ARRAY_SIZE(ltc2688_span_helper); span++) { + if (min == ltc2688_span_helper[span][0] && + max == ltc2688_span_helper[span][1]) + return span; + } + + return -EINVAL; +} + +static int ltc2688_channel_config(struct ltc2688_state *st) +{ + struct device *dev = &st->spi->dev; + struct device_node *child; + u32 reg, clk_input, val, tmp[2]; + int ret, span; + + for_each_available_child_of_node(dev->of_node, child) { + struct ltc2688_chan *chan; + + ret = of_property_read_u32(child, "reg", ®); + if (ret) { + of_node_put(child); + return dev_err_probe(dev, ret, + "Failed to get reg property\n"); + } + + if (reg >= LTC2688_DAC_CHANNELS) { + of_node_put(child); + return dev_err_probe(dev, -EINVAL, + "reg bigger than: %d\n", + LTC2688_DAC_CHANNELS); + } + + val = 0; + chan = &st->channels[reg]; + if (of_property_read_bool(child, "adi,toggle-mode")) { + chan->toggle_chan = true; + /* assume sw toggle ABI */ + st->iio_chan[reg].ext_info = ltc2688_toggle_sym_ext_info; + /* + * Clear IIO_CHAN_INFO_RAW bit as toggle channels expose + * out_voltage_raw{0|1} files. + */ + clear_bit(IIO_CHAN_INFO_RAW, + &st->iio_chan[reg].info_mask_separate); + } + + ret = of_property_read_u32_array(child, "adi,output-range-microvolt", + tmp, ARRAY_SIZE(tmp)); + if (!ret) { + span = ltc2688_span_lookup(st, (int)tmp[0] / 1000, + tmp[1] / 1000); + if (span < 0) { + of_node_put(child); + return dev_err_probe(dev, -EINVAL, + "output range not valid:[%d %d]\n", + tmp[0], tmp[1]); + } + + val |= FIELD_PREP(LTC2688_CH_SPAN_MSK, span); + } + + ret = of_property_read_u32(child, "adi,toggle-dither-input", + &clk_input); + if (!ret) { + if (clk_input >= LTC2688_CH_TGP_MAX) { + of_node_put(child); + return dev_err_probe(dev, -EINVAL, + "toggle-dither-input inv value(%d)\n", + clk_input); + } + + ret = ltc2688_tgp_clk_setup(st, chan, child, clk_input); + if (ret) { + of_node_put(child); + return ret; + } + + /* + * 0 means software toggle which is the default mode. + * Hence the +1. + */ + val |= FIELD_PREP(LTC2688_CH_TD_SEL_MSK, clk_input + 1); + + /* + * If a TGPx is given, we automatically assume a dither + * capable channel (unless toggle is already enabled). + * On top of this we just set here the dither bit in the + * channel settings. It won't have any effect until the + * global toggle/dither bit is enabled. + */ + if (!chan->toggle_chan) { + val |= FIELD_PREP(LTC2688_CH_MODE_MSK, 1); + st->iio_chan[reg].ext_info = ltc2688_dither_ext_info; + } else { + /* wait, no sw toggle after all */ + st->iio_chan[reg].ext_info = ltc2688_toggle_ext_info; + } + } + + if (of_property_read_bool(child, "adi,overrange")) { + chan->overrange = true; + val |= LTC2688_CH_OVERRANGE_MSK; + } + + if (!val) + continue; + + ret = regmap_write(st->regmap, LTC2688_CMD_CH_SETTING(reg), + val); + if (ret) { + of_node_put(child); + return dev_err_probe(dev, -EINVAL, + "failed to set chan settings\n"); + } + } + + return 0; +} + +static int ltc2688_setup(struct ltc2688_state *st, struct regulator *vref) +{ + struct gpio_desc *gpio; + int ret; + + /* + * If we have a reset pin, use that to reset the board, If not, use + * the reset bit. + */ + gpio = devm_gpiod_get_optional(&st->spi->dev, "clr", 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, + LTC2688_CONFIG_RST, + LTC2688_CONFIG_RST); + if (ret) + return ret; + } + + usleep_range(10000, 12000); + + /* + * Duplicate the default channel configuration as it can change during + * @ltc2688_channel_config() + */ + st->iio_chan = devm_kmemdup(&st->spi->dev, ltc2688_channels, + sizeof(ltc2688_channels), GFP_KERNEL); + if (!st->iio_chan) + return -ENOMEM; + + ret = ltc2688_channel_config(st); + if (ret) + return ret; + + if (!vref) + return 0; + + return regmap_set_bits(st->regmap, LTC2688_CMD_CONFIG, + LTC2688_CONFIG_EXT_REF); +} + +static void ltc2688_disable_regulators(void *data) +{ + struct ltc2688_state *st = data; + + regulator_bulk_disable(ARRAY_SIZE(st->regulators), st->regulators); +} + +static void ltc2688_disable_regulator(void *regulator) +{ + regulator_disable(regulator); +} + +static bool ltc2688_reg_readable(struct device *dev, unsigned int reg) +{ + switch (reg) { + case LTC2688_CMD_CH_CODE(0) ... LTC2688_CMD_CH_GAIN(15): + return true; + case LTC2688_CMD_CONFIG ... LTC2688_CMD_THERMAL_STAT: + return true; + default: + return false; + } +} + +static bool ltc2688_reg_writable(struct device *dev, unsigned int reg) +{ + /* + * There's a jump from 0x76 to 0x78 in the write codes and the thermal + * status code is 0x77 (which is read only) so that we need to check + * that special condition. + */ + if (reg <= LTC2688_CMD_UPDATE_ALL && reg != LTC2688_CMD_THERMAL_STAT) + return true; + + return false; +} + +struct regmap_bus ltc2688_regmap_bus = { + .read = ltc2688_spi_read, + .write = ltc2688_spi_write, + .read_flag_mask = LTC2688_READ_OPERATION, + .reg_format_endian_default = REGMAP_ENDIAN_BIG, + .val_format_endian_default = REGMAP_ENDIAN_BIG +}; + +static const struct regmap_config ltc2688_regmap_config = { + .reg_bits = 8, + .val_bits = 16, + .readable_reg = ltc2688_reg_readable, + .writeable_reg = ltc2688_reg_writable, + /* ignoring the no op command */ + .max_register = LTC2688_CMD_UPDATE_ALL +}; + +static const struct iio_info ltc2688_info = { + .write_raw = ltc2688_write_raw, + .read_raw = ltc2688_read_raw, + .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; + struct device *dev = &spi->dev; + int ret; + + indio_dev = devm_iio_device_alloc(dev, sizeof(*st)); + if (!indio_dev) + return -ENOMEM; + + st = iio_priv(indio_dev); + st->spi = spi; + + /* Just write this once. No need to do it in every regmap read. */ + st->tx_data[3] = LTC2688_CMD_NOOP; + mutex_init(&st->lock); + + st->regmap = devm_regmap_init(dev, <c2688_regmap_bus, st, + <c2688_regmap_config); + if (IS_ERR(st->regmap)) + return dev_err_probe(dev, PTR_ERR(st->regmap), + "Failed to init regmap"); + + st->regulators[0].supply = "vcc"; + st->regulators[1].supply = "iovcc"; + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(st->regulators), + st->regulators); + if (ret) + return dev_err_probe(dev, ret, "Failed to get regulators\n"); + + ret = regulator_bulk_enable(ARRAY_SIZE(st->regulators), st->regulators); + if (ret) + return dev_err_probe(dev, ret, "Failed to enable regulators\n"); + + ret = devm_add_action_or_reset(dev, ltc2688_disable_regulators, st); + if (ret) + return ret; + + vref_reg = devm_regulator_get_optional(dev, "vref"); + if (!IS_ERR(vref_reg)) { + ret = regulator_enable(vref_reg); + if (ret) + return dev_err_probe(dev, ret, + "Failed to enable vref regulators\n"); + + ret = devm_add_action_or_reset(dev, ltc2688_disable_regulator, + vref_reg); + if (ret) + return ret; + + ret = regulator_get_voltage(vref_reg); + if (ret < 0) + return dev_err_probe(dev, ret, "Failed to get vref\n"); + + st->vref = ret / 1000; + } else { + if (PTR_ERR(vref_reg) != -ENODEV) + return dev_err_probe(dev, PTR_ERR(vref_reg), + "Failed to get vref regulator"); + + vref_reg = NULL; + /* internal reference */ + st->vref = 4096; + } + + ret = ltc2688_setup(st, vref_reg); + if (ret) + return ret; + + indio_dev->name = "ltc2688"; + indio_dev->info = <c2688_info; + indio_dev->modes = INDIO_DIRECT_MODE; + indio_dev->channels = st->iio_chan; + indio_dev->num_channels = ARRAY_SIZE(ltc2688_channels); + + return devm_iio_device_register(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 precision 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> --- MAINTAINERS | 7 + drivers/iio/dac/Kconfig | 11 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/ltc2688.c | 1070 +++++++++++++++++++++++++++++++++++++ 4 files changed, 1089 insertions(+) create mode 100644 drivers/iio/dac/ltc2688.c