Hartmut Knaack schrieb am 13.12.2014 um 12:18:
> Nikolaus Schulz schrieb am 12.12.2014 um 16:58:
>> 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] =ata >> 16;
>>>> + st->xfer[1] =ata >> 8;
>>>> + st->xfer[2] =ata;
>>>> +
>>>> + 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 =io_priv(indio_dev);
>>>> +
>>>> + switch (m) {
>>>> + case IIO_CHAN_INFO_RAW:
>>>> + *val =t->val[chan->address];
>>>> + return IIO_VAL_INT;
>>>> + case IIO_CHAN_INFO_SCALE:
>>>> + *val =t->vref_mv;
>>>> + *val2 =6;
>>>> + 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 =io_priv(indio_dev);
>>>> + int err;
>>>> +
>>>> + switch (mask) {
>>>> + case IIO_CHAN_INFO_RAW:
>>>> + if (val > 0xffff || val < 0)
>>>> + return -EINVAL;
>>>> +
>>>> + err =ac8554_spi_write(st, DAC8554_CMD_UPDATE_DAC_N,
>>>> + chan->address, val);
>>>> + if (err)
>>>> + return err;
>>>> +
>>>> + st->val[chan->address] =al;
>>>> +
>>>> + /* By hw design, DAC updates automatically trigger powerup. */
>>>> + st->powerdown[chan->address] =alse;
>>>> +
>>>> + 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 =io_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 =io_priv(indio_dev);
>>>> +
>>>> + st->powerdown_mode[chan->address] =ode;
>>>> +
>>>> + 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 =io_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 =io_priv(indio_dev);
>>>> + u8 powerdown_mode;
>>>> +
>>>> + ret =trtobool(buf, &powerdown);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + st->powerdown[chan->address] =owerdown;
>>>> +
>>>> + if (powerdown) {
>>>> + powerdown_mode =t->powerdown_mode[chan->address];
>>>> + ret =ac8554_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 =ac8554_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.
> Fair enough.
>>
>>>> +}
>>>> +
>>>> +static int dac8554_powerdown(struct dac8554_state *st,
>>>> + u8 powerdown_mode)
>>>> +{
>>>> + int chan, cmd, ret;
>>>> +
>>>> + for (chan =AC8554_DAC_CHANNELS-1; chan >= 0; --chan) {
>>> Please mind spaces around operators.
>>
>> Fixed.
>>
>>>> + cmd =han ? DAC8554_CMD_STORE_DAC_N
>>>> + : DAC8554_CMD_STORE_DAC_N_UPDATE_ALL;
>>>> + ret =ac8554_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 = ac8554_write_raw,
>>>> + .read_raw =ac8554_read_raw,
>>>> + .driver_module =HIS_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 = ac8554_powerdown_modes,
>>>> + .num_items =RRAY_SIZE(dac8554_powerdown_modes),
>>>> + .get =ac8554_get_powerdown_mode,
>>>> + .set =ac8554_set_powerdown_mode,
>>>> +};
>>>> +
>>>> +static const struct iio_chan_spec_ext_info dac8554_ext_info[] =
>>>> + {
>>>> + .name =powerdown",
>>>> + .read =ac8554_read_dac_powerdown,
>>>> + .write =ac8554_write_dac_powerdown,
>>>> + .shared =IO_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 =IO_VOLTAGE, \
>>>> + .indexed =, \
>>>> + .output =, \
>>>> + .channel =chan), \
>>>> + .info_mask_separate =IT(IIO_CHAN_INFO_RAW), \
>>>> + .info_mask_shared_by_type =IT(IIO_CHAN_INFO_SCALE), \
>>>> + .address =chan), \
>>>> + .ext_info =ac8554_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 =evm_iio_device_alloc(&spi->dev, sizeof(*st));
>>>> + if (!indio_dev)
>>>> + return -ENOMEM;
>>>> +
>>>> + indio_dev->dev.parent =spi->dev;
>>>> + indio_dev->name =AC8554_DRIVERNAME;
>>>> + indio_dev->info =dac8554_info;
>>>> + indio_dev->modes =NDIO_DIRECT_MODE;
>>>> + indio_dev->channels =ac8554_channels;
>>>> + indio_dev->num_channels =RRAY_SIZE(dac8554_channels);
>>>> +
>>>> + spi_set_drvdata(spi, indio_dev);
>>>> +
>>>> + st =io_priv(indio_dev);
>>>> +
>>>> + if (!spi->dev.of_node) {
>>>> + dev_err(&spi->dev, "missing OF node");
>>>> + return -ENODEV;
>>>> + }
>>>> + ret =f_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 =pi;
>>>> + st->addr =ddr;
>>>> +
>>> According to your DT bindings, the regulator from property "vref-supply" should
>>> be used. This is missing here.
>>
>> Uhm, it's right below, no?
> Looking into your DT bindings patch (which unfortunately didn't make it into our
> list), you specify "vref-supply" as a required property. So, I expected that you
> intended to make use of this property somewhere around here.
>>
>>>> + st->reg =evm_regulator_get(&spi->dev, "vref");
>>>> + if (IS_ERR(st->reg))
>>>> + return PTR_ERR(st->reg);
>>>> +
>>>> + ret =egulator_enable(st->reg);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + voltage_uv =egulator_get_voltage(st->reg);
>>>> + if (voltage_uv < 0)
>>>> + goto error_disable_reg;
>>> Missing ret =oltage_uv before goto. Or just drop voltage_uv completely and use ret instead.
>>
>> Fixed.
>>
>>>> + st->vref_mv =oltage_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.
>>
> I had people in mind, who might want to use such DAC with one of the popular
> embedded boards like Raspberry Pi - people who would just like to "plug and
> play" without the need or ability to build a kernel (or would prefer to go
> with the convenience of a distribution kernel). But since you've put a
> dependency on OF, I don't see a way to use this device with a stock distribution
> kernel anyway.
> But I still think that calculating the actual voltage during _read_raw() would
> be a good idea, as there is support for regulators providing different voltages,
> which can be changed during runtime.
One more thing: you should consider to add a dependency on regulators in Kconfig.
>>>> +
>>>> + ret =ac8554_powerdown(st, DAC8554_PWRDN_100K);
>>>> + if (ret)
>>>> + goto error_disable_reg;
>>>> +
>>>> + ret =io_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 =pi_get_drvdata(spi);
>>>> + struct dac8554_state *st =io_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 = AC8554_DRIVERNAME,
>>>> + .owner =HIS_MODULE,
>>>> + .of_match_table =ac8554_of_match,
>>>> + },
>>>> + .probe =ac8554_probe,
>>>> + .remove =ac8554_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-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
