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;
> + }
--
With Best Regards,
Andy Shevchenko
