Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554

From: Jonathan Cameron
Date: Fri Dec 12 2014 - 13:14:17 EST

On 12/12/14 15:58, Nikolaus Schulz wrote:
> On Sat, Dec 06, 2014 at 12:36:19PM +0100, Hartmut Knaack wrote:
>> Nikolaus Schulz schrieb am 24.11.2014 um 20:50:
>>> The TI DAC8554 is a quad-channel Digital-to-Analog Converter with an SPI
>>> interface.
>>>
>>> Changes in v2:
>>> * Use DMA-safe buffer for SPI transfer
>>> * Normalize powerdown_mode name "hi-z" to "three_state" as per
>>> ABI/testing/sysfs-bus-iio
>>> * Register device late in probe function
>>> * Avoid powerdown broadcast update, which touches all DAC on the bus
>> There are a few issues left, please see my comments inline.
>>>
>>> Signed-off-by: Nikolaus Schulz <nikolaus.schulz@xxxxxxxxxxxxxxxxx>
>>> ---
>>> drivers/iio/dac/Kconfig | 10 ++
>>> drivers/iio/dac/Makefile | 1 +
>>> drivers/iio/dac/ti-dac8554.c | 374 +++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 385 insertions(+)
>>> create mode 100644 drivers/iio/dac/ti-dac8554.c
> [...]
>>> diff --git a/drivers/iio/dac/ti-dac8554.c b/drivers/iio/dac/ti-dac8554.c
>>> new file mode 100644
>>> index 0000000..fca751f
>>> --- /dev/null
>>> +++ b/drivers/iio/dac/ti-dac8554.c
>>> @@ -0,0 +1,374 @@
>>> +/*
>>> + * TI DAC8554 Digital to Analog Converter SPI driver
>>> + *
>>> + * Copyright (C) 2014 Avionic Design GmbH
>>> + *
>>> + * Based on ad5446r_spi.c
>>> + * Copyright (C) 2010,2011 Analog Devices Inc.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <linux/spi/spi.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <linux/module.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/of.h>
>>> +
>>> +#define DAC8554_DRIVERNAME "ti-dac8554"
>>> +#define DAC8554_DAC_CHANNELS 4
>>> +
>>> +/* Load commands */
>>> +#define DAC8554_CMD_STORE_DAC_N 0x0
>>> +#define DAC8554_CMD_UPDATE_DAC_N 0x1
>>> +#define DAC8554_CMD_STORE_DAC_N_UPDATE_ALL 0x2
>>> +#define DAC8554_CMD_UPDATE_BROADCAST 0x3
>>> +
>>> +#define DAC8554_BROADCAST_USE_SRDATA 0x2
>> Add a blank line here.
>
> Fixed.
>
>>> +/* Powerdown modes (ORed PD1|PD2 bits) */
>> It took my some time to figure out ORed. I think you could drop it and just
>> mention (PD1 | PD2 bits).
>
> Agreed, and fixed.
>
>>> +#define DAC8554_PWRDN_HIZ 0x0
>>> +#define DAC8554_PWRDN_1K 0x1
>>> +#define DAC8554_PWRDN_100K 0x2
>>> +
>>> +#define DAC8554_PWRDN_TO_SR(mode) (BIT(16) | (mode) << 14)
>>> +
>>> +/**
>>> + * struct dac8554_state - driver instance specific data
>>> + * @spi: spi_device
>> SPI device?
>
> Yes, fixed.
>
>>> + * @reg: supply regulator
>>> + * @addr: two-bit chip address
>>> + * @vref_mv: reference voltage in millivolt
>>> + * @val DAC/channel data
>> DAC channel data?
>
> "channel data" will do, actually.
>
>>> + * @powerdown channel powerdown flag
>>> + * @powerdown_mode channel powerdown mode
>>> + * @xfer SPI transfer buffer
>> Half of the comments are with colon, the other half are missing it.
>
> Fixed.
>
>>> + */
>>> +struct dac8554_state {
>>> + struct spi_device *spi;
>>> + struct regulator *reg;
>>> + unsigned addr;
>>> + unsigned vref_mv;
>>> + u16 val[DAC8554_DAC_CHANNELS];
>>> + bool powerdown[DAC8554_DAC_CHANNELS];
>>> + u8 powerdown_mode[DAC8554_DAC_CHANNELS];
>>> +
>>> + /*
>>> + * DMA (thus cache coherency maintenance) requires the
>>> + * transfer buffers to live in their own cache lines.
>>> + */
>>> + u8 xfer[3] ____cacheline_aligned;
>>> +};
>>> +
>>> +static int dac8554_spi_write(struct dac8554_state *st,
>>> + unsigned cmd,
>>> + unsigned chan_addr,
>>> + unsigned val)
>>> +{
>>> + u32 data;
>>> +
>>> + /*
>>> + * The input shift register is 24 bits wide. The 8 MSB are
>>> + * control bits, followed by 16 data bits.
>>> + * The first two bits A1 and A0 address a DAC8554 chip.
>>> + * The next two are the command bits, LD1 and LD0.
>>> + * After a don't-care-bit, the next two bits select the channel.
>>> + * The final control bit PD0 is a flag signalling if the data
>>> + * bits encode a powerdown mode. We merge PD0 with the adjacent
>>> + * data bits.
>>> + */
>>> +
>>> + if (cmd > 3 || chan_addr > 3 ||
>>> + (val > 0xffff && (val & ~DAC8554_PWRDN_TO_SR(3))))
>>> + return -EINVAL;
>>> +
>>> + data = (st->addr << 22) | (cmd << 20) | (chan_addr << 17) | val;
>> Could be more readable by using definition for the magic shifts, or macros.
>
> Hmm. The magic is all concentrated at this place, and the variable names
> tell the story, so I figured that's ok. But, why not: fixed.
>
>>> + st->xfer[0] = data >> 16;
>>> + st->xfer[1] = data >> 8;
>>> + st->xfer[2] = data;
>>> +
>>> + return spi_write(st->spi, st->xfer, sizeof(st->xfer));
>>> +}
>>> +
>>> +static int dac8554_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int *val,
>>> + int *val2,
>>> + long m)
>> Commonly, m is called mask (like you do in _write_raw).
>
> Ah yes. Fixed.
>
>>> +{
>>> + struct dac8554_state *st = iio_priv(indio_dev);
>>> +
>>> + switch (m) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + *val = st->val[chan->address];
>>> + return IIO_VAL_INT;
>>> + case IIO_CHAN_INFO_SCALE:
>>> + *val = st->vref_mv;
>>> + *val2 = 16;
>>> + return IIO_VAL_FRACTIONAL_LOG2;
>>> + }
>>> + return -EINVAL;
>>> +}
>>> +
>>> +static int dac8554_write_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *chan,
>>> + int val,
>>> + int val2,
>>> + long mask)
>>> +{
>>> + struct dac8554_state *st = iio_priv(indio_dev);
>>> + int err;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + if (val > 0xffff || val < 0)
>>> + return -EINVAL;
>>> +
>>> + err = dac8554_spi_write(st, DAC8554_CMD_UPDATE_DAC_N,
>>> + chan->address, val);
>>> + if (err)
>>> + return err;
>>> +
>>> + st->val[chan->address] = val;
>>> +
>>> + /* By hw design, DAC updates automatically trigger powerup. */
>>> + st->powerdown[chan->address] = false;
>>> +
>>> + return 0;
>>> +
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +static int dac8554_get_powerdown_mode(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan)
>>> +{
>>> + struct dac8554_state *st = iio_priv(indio_dev);
>>> +
>>> + return st->powerdown_mode[chan->address];
>>> +}
>>> +
>>> +static int dac8554_set_powerdown_mode(struct iio_dev *indio_dev,
>>> + const struct iio_chan_spec *chan,
>>> + unsigned int mode)
>>> +{
>>> + struct dac8554_state *st = iio_priv(indio_dev);
>>> +
>>> + st->powerdown_mode[chan->address] = mode;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static ssize_t dac8554_read_dac_powerdown(struct iio_dev *indio_dev,
>>> + uintptr_t private,
>>> + const struct iio_chan_spec *chan,
>>> + char *buf)
>>> +{
>>> + struct dac8554_state *st = iio_priv(indio_dev);
>>> +
>>> + return sprintf(buf, "%d\n", st->powerdown[chan->address]);
>>> +}
>>> +
>>> +static ssize_t dac8554_write_dac_powerdown(struct iio_dev *indio_dev,
>>> + uintptr_t private,
>>> + const struct iio_chan_spec *chan,
>>> + const char *buf,
>>> + size_t len)
>>> +{
>>> + bool powerdown;
>>> + int ret;
>>> + struct dac8554_state *st = iio_priv(indio_dev);
>>> + u8 powerdown_mode;
>>> +
>>> + ret = strtobool(buf, &powerdown);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + st->powerdown[chan->address] = powerdown;
>>> +
>>> + if (powerdown) {
>>> + powerdown_mode = st->powerdown_mode[chan->address];
>>> + ret = dac8554_spi_write(st,
>>> + DAC8554_CMD_UPDATE_DAC_N,
>>> + chan->address,
>>> + DAC8554_PWRDN_TO_SR(powerdown_mode));
>> The parameters sill fit on line even with proper indentation.
>
> Fixed.
>
>>> + } else {
>>> + /* Load DAC with cached value. This triggers a powerup. */
>>> + ret = dac8554_spi_write(st,
>>> + DAC8554_CMD_UPDATE_DAC_N,
>>> + chan->address,
>>> + st->val[chan->address]);
>> Same here with indentation.
>
> Fixed.
>
>>> + }
>>> +
>>> + if (ret)
>>> + return ret;
>>> +
>>> + return len;
>> Just: return (ret) ? ret : len;
>
> I think avoiding the ternary operator makes the code more readable, and
> it separates the error path from normal operation. So I would prefer to
> keep it that way.
Agreed. Matter of personal taste - personally I don't really care either
way ;)
>
>>> +}
>>> +
>>> +static int dac8554_powerdown(struct dac8554_state *st,
>>> + u8 powerdown_mode)
>>> +{
>>> + int chan, cmd, ret;
>>> +
>>> + for (chan = DAC8554_DAC_CHANNELS-1; chan >= 0; --chan) {
>> Please mind spaces around operators.
>
> Fixed.
>
>>> + cmd = chan ? DAC8554_CMD_STORE_DAC_N
>>> + : DAC8554_CMD_STORE_DAC_N_UPDATE_ALL;
>>> + ret = dac8554_spi_write(st, cmd, chan,
>>> + DAC8554_PWRDN_TO_SR(powerdown_mode));
>>> + if (ret)
>>> + return ret;
>>> + }
>>> +
>>> + memset(st->powerdown_mode, powerdown_mode, sizeof(st->powerdown_mode));
>>> + memset(st->powerdown, true, sizeof(st->powerdown));
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct iio_info dac8554_info = {
>>> + .write_raw = dac8554_write_raw,
>>> + .read_raw = dac8554_read_raw,
>>> + .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +static const char * const dac8554_powerdown_modes[] = {
>>> + "three_state",
>>> + "1kohm_to_gnd",
>>> + "100kohm_to_gnd"
>>> +};
>>> +
>>> +static const struct iio_enum dac8854_powerdown_mode_enum = {
>>> + .items = dac8554_powerdown_modes,
>>> + .num_items = ARRAY_SIZE(dac8554_powerdown_modes),
>>> + .get = dac8554_get_powerdown_mode,
>>> + .set = dac8554_set_powerdown_mode,
>>> +};
>>> +
>>> +static const struct iio_chan_spec_ext_info dac8554_ext_info[] = {
>>> + {
>>> + .name = "powerdown",
>>> + .read = dac8554_read_dac_powerdown,
>>> + .write = dac8554_write_dac_powerdown,
>>> + .shared = IIO_SEPARATE,
>>> + },
>>> + IIO_ENUM("powerdown_mode", IIO_SEPARATE,
>>> + &dac8854_powerdown_mode_enum),
>>> + IIO_ENUM_AVAILABLE("powerdown_mode", &dac8854_powerdown_mode_enum),
>>> + { },
>>> +};
>>> +
>>> +#define DAC8554_CHANNEL(chan) { \
>>> + .type = IIO_VOLTAGE, \
>>> + .indexed = 1, \
>>> + .output = 1, \
>>> + .channel = (chan), \
>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
>>> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
>>> + .address = (chan), \
>>> + .ext_info = dac8554_ext_info, \
>>> +}
>>> +const struct iio_chan_spec dac8554_channels[] = {
>>> + DAC8554_CHANNEL(0),
>>> + DAC8554_CHANNEL(1),
>>> + DAC8554_CHANNEL(2),
>>> + DAC8554_CHANNEL(3),
>>> +};
>>> +#undef DAC8554_CHANNEL
>>> +
>>> +static int dac8554_probe(struct spi_device *spi)
>>> +{
>>> + struct dac8554_state *st;
>>> + struct iio_dev *indio_dev;
>>> + int ret, voltage_uv;
>>> + u32 addr;
>>> +
>>> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
>>> + if (!indio_dev)
>>> + return -ENOMEM;
>>> +
>>> + indio_dev->dev.parent = &spi->dev;
>>> + indio_dev->name = DAC8554_DRIVERNAME;
>>> + indio_dev->info = &dac8554_info;
>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>> + indio_dev->channels = dac8554_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(dac8554_channels);
>>> +
>>> + spi_set_drvdata(spi, indio_dev);
>>> +
>>> + st = iio_priv(indio_dev);
>>> +
>>> + if (!spi->dev.of_node) {
>>> + dev_err(&spi->dev, "missing OF node");
>>> + return -ENODEV;
>>> + }
>>> + ret = of_property_read_u32(spi->dev.of_node, "address", &addr);
>>> + if (ret || addr < 0 || addr > 2) {
>>> + dev_err(&spi->dev, "no or invalid chip address");
>>> + return -ENODEV;
>>> + }
>>> +
>>> + st->spi = spi;
>>> + st->addr = addr;
>>> +
>> According to your DT bindings, the regulator from property "vref-supply" should
>> be used. This is missing here.
>
> Uhm, it's right below, no?
>
>>> + st->reg = devm_regulator_get(&spi->dev, "vref");
>>> + if (IS_ERR(st->reg))
>>> + return PTR_ERR(st->reg);
>>> +
>>> + ret = regulator_enable(st->reg);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + voltage_uv = regulator_get_voltage(st->reg);
>>> + if (voltage_uv < 0)
>>> + goto error_disable_reg;
>> Missing ret = voltage_uv before goto. Or just drop voltage_uv completely and use ret instead.
>
> Fixed.
>
>>> + st->vref_mv = voltage_uv / 1000;
>> How hard do you want to depend on a voltage regulator? Doing regulator_get_voltage()
>> could even be called dynamically in _read_raw(), making a real regulator optional.
>
> Hmm. I understand that the DAC voltage input may not be provided by a
> regulator, but is that a common scenario? No other DAC driver I looked
> at handles that case, they all consider it an error if the regulator is
> absent.
Sure - if there isn't one, then a fixed regulator can be specified in the DT
to provide whatever the voltage is.
>
>>> +
>>> + ret = dac8554_powerdown(st, DAC8554_PWRDN_100K);
>>> + if (ret)
>>> + goto error_disable_reg;
>>> +
>>> + ret = iio_device_register(indio_dev);
>>> + if (ret)
>>> + goto error_disable_reg;
>>> +
>>> + return 0;
>>> +
>>> +error_disable_reg:
>>> + regulator_disable(st->reg);
>>> + return ret;
>>> +}
>>> +
>>> +static int dac8554_remove(struct spi_device *spi)
>>> +{
>>> + struct iio_dev *indio_dev = spi_get_drvdata(spi);
>>> + struct dac8554_state *st = iio_priv(indio_dev);
>>> +
>>> + iio_device_unregister(indio_dev);
>>> + regulator_disable(st->reg);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static const struct of_device_id dac8554_of_match[] = {
>>> + { .compatible = "ti,dac8554" },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, dac8554_of_match);
>>> +
>>> +static struct spi_driver dac8554_driver = {
>>> + .driver = {
>>> + .name = DAC8554_DRIVERNAME,
>>> + .owner = THIS_MODULE,
>>> + .of_match_table = dac8554_of_match,
>>> + },
>>> + .probe = dac8554_probe,
>>> + .remove = dac8554_remove,
>>> +};
>>> +module_spi_driver(dac8554_driver);
>>> +
>>> +MODULE_AUTHOR("Nikolaus Schulz <nikolaus.schulz@xxxxxxxxxxxxxxxxx>");
>>> +MODULE_DESCRIPTION("Texas Instruments DAC8554 SPI driver");
>>> +MODULE_LICENSE("GPL v2");
>
> Thanks for the review!
>
> Nikolaus
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/