* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-06 11:36 ` Hartmut Knaack
0 siblings, 0 replies; 27+ messages in thread
From: Hartmut Knaack @ 2014-12-06 11:36 UTC (permalink / raw)
To: Nikolaus Schulz, Jonathan Cameron, Lars-Peter Clausen,
Peter Meerwald, Grant Likely, Rob Herring, Michael Welling,
Philippe Reynes, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Alban Bedel
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-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> ---
> 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/Kconfig b/drivers/iio/dac/Kconfig
> index 2236ea2..1689260 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -181,4 +181,14 @@ config MCP4922
> To compile this driver as a module, choose M here: the module
> will be called mcp4922.
>
> +config TI_DAC8554
> + tristate "TI DAC8554 driver"
> + depends on SPI
> + depends on OF
> + help
> + Say yes here to build the driver for the TI DAC8554.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called ti-dac8554.
> +
> endmenu
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 52be7e1..b98d79a 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -20,3 +20,4 @@ obj-$(CONFIG_MAX517) += max517.o
> obj-$(CONFIG_MAX5821) += max5821.o
> obj-$(CONFIG_MCP4725) += mcp4725.o
> obj-$(CONFIG_MCP4922) += mcp4922.o
> +obj-$(CONFIG_TI_DAC8554) += ti-dac8554.o
> 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.
> +/* 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).
> +#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?
> + * @reg: supply regulator
> + * @addr: two-bit chip address
> + * @vref_mv: reference voltage in millivolt
> + * @val DAC/channel data
DAC channel data?
> + * @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.
> + */
> +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.
> + 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).
> +{
> + 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.
> + } 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.
> + }
> +
> + if (ret)
> + return ret;
> +
> + return len;
Just: return (ret) ? ret : len;
> +}
> +
> +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.
> + 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.
> + 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.
> + 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.
> +
> + 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-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>");
> +MODULE_DESCRIPTION("Texas Instruments DAC8554 SPI driver");
> +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-12 11:57 ` Jonathan Cameron
0 siblings, 0 replies; 27+ messages in thread
From: Jonathan Cameron @ 2014-12-12 11:57 UTC (permalink / raw)
To: Hartmut Knaack, Nikolaus Schulz, Lars-Peter Clausen,
Peter Meerwald, Grant Likely, Rob Herring, Michael Welling,
Philippe Reynes, linux-kernel, linux-iio, devicetree
Cc: Alban Bedel
On 06/12/14 11:36, 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.
1 little one from me...
>>
>> Signed-off-by: Nikolaus Schulz <nikolaus.schulz@avionic-design.de>
>> ---
>> 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/Kconfig b/drivers/iio/dac/Kconfig
>> index 2236ea2..1689260 100644
>> --- a/drivers/iio/dac/Kconfig
>> +++ b/drivers/iio/dac/Kconfig
>> @@ -181,4 +181,14 @@ config MCP4922
>> To compile this driver as a module, choose M here: the module
>> will be called mcp4922.
>>
>> +config TI_DAC8554
>> + tristate "TI DAC8554 driver"
>> + depends on SPI
>> + depends on OF
>> + help
>> + Say yes here to build the driver for the TI DAC8554.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called ti-dac8554.
>> +
>> endmenu
>> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
>> index 52be7e1..b98d79a 100644
>> --- a/drivers/iio/dac/Makefile
>> +++ b/drivers/iio/dac/Makefile
>> @@ -20,3 +20,4 @@ obj-$(CONFIG_MAX517) += max517.o
>> obj-$(CONFIG_MAX5821) += max5821.o
>> obj-$(CONFIG_MCP4725) += mcp4725.o
>> obj-$(CONFIG_MCP4922) += mcp4922.o
>> +obj-$(CONFIG_TI_DAC8554) += ti-dac8554.o
>> 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.
>> +/* 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).
>> +#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?
>> + * @reg: supply regulator
>> + * @addr: two-bit chip address
>> + * @vref_mv: reference voltage in millivolt
>> + * @val DAC/channel data
> DAC channel data?
>> + * @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.
>> + */
>> +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.
>> + 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).
>> +{
>> + 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.
>> + } 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.
>> + }
>> +
>> + if (ret)
>> + return ret;
>> +
>> + return len;
> Just: return (ret) ? ret : len;
>> +}
>> +
>> +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.
>> + 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));
Whilst this probably works, I think for the boolean case it is less
than entirely obvious, so probably better to have a simple loop.
>> +
>> + 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.
>> + 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.
>> + 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.
>> +
>> + 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@avionic-design.de>");
>> +MODULE_DESCRIPTION("Texas Instruments DAC8554 SPI driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-12 11:57 ` Jonathan Cameron
0 siblings, 0 replies; 27+ messages in thread
From: Jonathan Cameron @ 2014-12-12 11:57 UTC (permalink / raw)
To: Hartmut Knaack, Nikolaus Schulz, Lars-Peter Clausen,
Peter Meerwald, Grant Likely, Rob Herring, Michael Welling,
Philippe Reynes, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Alban Bedel
On 06/12/14 11:36, 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.
1 little one from me...
>>
>> Signed-off-by: Nikolaus Schulz <nikolaus.schulz-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>> ---
>> 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/Kconfig b/drivers/iio/dac/Kconfig
>> index 2236ea2..1689260 100644
>> --- a/drivers/iio/dac/Kconfig
>> +++ b/drivers/iio/dac/Kconfig
>> @@ -181,4 +181,14 @@ config MCP4922
>> To compile this driver as a module, choose M here: the module
>> will be called mcp4922.
>>
>> +config TI_DAC8554
>> + tristate "TI DAC8554 driver"
>> + depends on SPI
>> + depends on OF
>> + help
>> + Say yes here to build the driver for the TI DAC8554.
>> +
>> + To compile this driver as a module, choose M here: the module
>> + will be called ti-dac8554.
>> +
>> endmenu
>> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
>> index 52be7e1..b98d79a 100644
>> --- a/drivers/iio/dac/Makefile
>> +++ b/drivers/iio/dac/Makefile
>> @@ -20,3 +20,4 @@ obj-$(CONFIG_MAX517) += max517.o
>> obj-$(CONFIG_MAX5821) += max5821.o
>> obj-$(CONFIG_MCP4725) += mcp4725.o
>> obj-$(CONFIG_MCP4922) += mcp4922.o
>> +obj-$(CONFIG_TI_DAC8554) += ti-dac8554.o
>> 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.
>> +/* 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).
>> +#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?
>> + * @reg: supply regulator
>> + * @addr: two-bit chip address
>> + * @vref_mv: reference voltage in millivolt
>> + * @val DAC/channel data
> DAC channel data?
>> + * @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.
>> + */
>> +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.
>> + 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).
>> +{
>> + 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.
>> + } 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.
>> + }
>> +
>> + if (ret)
>> + return ret;
>> +
>> + return len;
> Just: return (ret) ? ret : len;
>> +}
>> +
>> +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.
>> + 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));
Whilst this probably works, I think for the boolean case it is less
than entirely obvious, so probably better to have a simple loop.
>> +
>> + 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.
>> + 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.
>> + 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.
>> +
>> + 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-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>");
>> +MODULE_DESCRIPTION("Texas Instruments DAC8554 SPI driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-12 16:15 ` Nikolaus Schulz
0 siblings, 0 replies; 27+ messages in thread
From: Nikolaus Schulz @ 2014-12-12 16:15 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Grant Likely,
Rob Herring, Michael Welling, Philippe Reynes, linux-kernel,
linux-iio, devicetree, Alban Bedel
On Fri, Dec 12, 2014 at 11:57:08AM +0000, Jonathan Cameron wrote:
> On 06/12/14 11:36, 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.
> 1 little one from me...
> >>
> >> Signed-off-by: Nikolaus Schulz <nikolaus.schulz@avionic-design.de>
> >> ---
> >> 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
[...]
> >> +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.
> >> + 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));
> Whilst this probably works, I think for the boolean case it is less
> than entirely obvious, so probably better to have a simple loop.
Yeah, I agree it's better to keep it as simple and obvious as possible.
Fixed in v3.
> >> +
> >> + return 0;
> >> +}
--
Avionic Design GmbH
Nikolaus Schulz
Wragekamp 10
D-22397 Hamburg
Germany
Tel.: +49 40 88187-163
Fax: +49 40 88187-150
Email: nikolaus.schulz@avionic-design.de
Avionic Design GmbH
Amtsgericht Hamburg HRB 82598
Geschäftsführung: Cornelis Broers
Ust.-Ident-Nr.: DE813378254
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-12 16:15 ` Nikolaus Schulz
0 siblings, 0 replies; 27+ messages in thread
From: Nikolaus Schulz @ 2014-12-12 16:15 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald, Grant Likely,
Rob Herring, Michael Welling, Philippe Reynes,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Alban Bedel
On Fri, Dec 12, 2014 at 11:57:08AM +0000, Jonathan Cameron wrote:
> On 06/12/14 11:36, 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.
> 1 little one from me...
> >>
> >> Signed-off-by: Nikolaus Schulz <nikolaus.schulz-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
> >> ---
> >> 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
[...]
> >> +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.
> >> + 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));
> Whilst this probably works, I think for the boolean case it is less
> than entirely obvious, so probably better to have a simple loop.
Yeah, I agree it's better to keep it as simple and obvious as possible.
Fixed in v3.
> >> +
> >> + return 0;
> >> +}
--
Avionic Design GmbH
Nikolaus Schulz
Wragekamp 10
D-22397 Hamburg
Germany
Tel.: +49 40 88187-163
Fax: +49 40 88187-150
Email: nikolaus.schulz-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org
Avionic Design GmbH
Amtsgericht Hamburg HRB 82598
Geschäftsführung: Cornelis Broers
Ust.-Ident-Nr.: DE813378254
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
2014-12-06 11:36 ` Hartmut Knaack
(?)
(?)
@ 2014-12-12 15:58 ` Nikolaus Schulz
2014-12-12 18:14 ` Jonathan Cameron
2014-12-13 11:18 ` Hartmut Knaack
-1 siblings, 2 replies; 27+ messages in thread
From: Nikolaus Schulz @ 2014-12-12 15:58 UTC (permalink / raw)
To: Hartmut Knaack
Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald,
Grant Likely, Rob Herring, Michael Welling, Philippe Reynes,
linux-kernel, linux-iio, devicetree, Alban Bedel
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@avionic-design.de>
> > ---
> > 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.
> > +}
> > +
> > +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.
> > +
> > + 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@avionic-design.de>");
> > +MODULE_DESCRIPTION("Texas Instruments DAC8554 SPI driver");
> > +MODULE_LICENSE("GPL v2");
Thanks for the review!
Nikolaus
--
Avionic Design GmbH
Nikolaus Schulz
Wragekamp 10
D-22397 Hamburg
Germany
Tel.: +49 40 88187-163
Fax: +49 40 88187-150
Email: nikolaus.schulz@avionic-design.de
Avionic Design GmbH
Amtsgericht Hamburg HRB 82598
Geschäftsführung: Cornelis Broers
Ust.-Ident-Nr.: DE813378254
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-12 18:14 ` Jonathan Cameron
0 siblings, 0 replies; 27+ messages in thread
From: Jonathan Cameron @ 2014-12-12 18:14 UTC (permalink / raw)
To: Nikolaus Schulz, Hartmut Knaack
Cc: Lars-Peter Clausen, Peter Meerwald, Grant Likely, Rob Herring,
Michael Welling, Philippe Reynes, linux-kernel, linux-iio,
devicetree, Alban Bedel
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@avionic-design.de>
>>> ---
>>> 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@avionic-design.de>");
>>> +MODULE_DESCRIPTION("Texas Instruments DAC8554 SPI driver");
>>> +MODULE_LICENSE("GPL v2");
>
> Thanks for the review!
>
> Nikolaus
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-12 18:14 ` Jonathan Cameron
0 siblings, 0 replies; 27+ messages in thread
From: Jonathan Cameron @ 2014-12-12 18:14 UTC (permalink / raw)
To: Nikolaus Schulz, Hartmut Knaack
Cc: Lars-Peter Clausen, Peter Meerwald, Grant Likely, Rob Herring,
Michael Welling, Philippe Reynes,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Alban Bedel
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-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>>> ---
>>> 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-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>");
>>> +MODULE_DESCRIPTION("Texas Instruments DAC8554 SPI driver");
>>> +MODULE_LICENSE("GPL v2");
>
> Thanks for the review!
>
> Nikolaus
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-13 11:18 ` Hartmut Knaack
0 siblings, 0 replies; 27+ messages in thread
From: Hartmut Knaack @ 2014-12-13 11:18 UTC (permalink / raw)
To: Nikolaus Schulz
Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald,
Grant Likely, Rob Herring, Michael Welling, Philippe Reynes,
linux-kernel, linux-iio, devicetree, Alban Bedel
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@avionic-design.de>
>>> ---
>>> 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.
>>> +
>>> + 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@avionic-design.de>");
>>> +MODULE_DESCRIPTION("Texas Instruments DAC8554 SPI driver");
>>> +MODULE_LICENSE("GPL v2");
>
> Thanks for the review!
>
> Nikolaus
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-13 11:18 ` Hartmut Knaack
0 siblings, 0 replies; 27+ messages in thread
From: Hartmut Knaack @ 2014-12-13 11:18 UTC (permalink / raw)
To: Nikolaus Schulz
Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald,
Grant Likely, Rob Herring, Michael Welling, Philippe Reynes,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Alban Bedel
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-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>>> ---
>>> 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.
>>> +
>>> + 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-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>");
>>> +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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-13 11:24 ` Hartmut Knaack
0 siblings, 0 replies; 27+ messages in thread
From: Hartmut Knaack @ 2014-12-13 11:24 UTC (permalink / raw)
To: Nikolaus Schulz
Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald,
Grant Likely, Rob Herring, Michael Welling, Philippe Reynes,
linux-kernel, linux-iio, devicetree, Alban Bedel
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@avionic-design.de>
>>>> ---
>>>> 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@avionic-design.de>");
>>>> +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@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-13 11:24 ` Hartmut Knaack
0 siblings, 0 replies; 27+ messages in thread
From: Hartmut Knaack @ 2014-12-13 11:24 UTC (permalink / raw)
To: Nikolaus Schulz
Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald,
Grant Likely, Rob Herring, Michael Welling, Philippe Reynes,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Alban Bedel
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-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
>>>> ---
>>>> 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-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>");
>>>> +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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-13 11:25 ` Lars-Peter Clausen
0 siblings, 0 replies; 27+ messages in thread
From: Lars-Peter Clausen @ 2014-12-13 11:25 UTC (permalink / raw)
To: Hartmut Knaack, Nikolaus Schulz
Cc: Jonathan Cameron, Peter Meerwald, Grant Likely, Rob Herring,
Michael Welling, Philippe Reynes, linux-kernel, linux-iio,
devicetree, Alban Bedel
On 12/13/2014 12:18 PM, Hartmut Knaack wrote:
[...]
>>> 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.
The regulator framework takes care of this. If you request a regulator with
$name the framework will look for a property with the name $name-supply.
>>
>>>> + st->reg =evm_regulator_get(&spi->dev, "vref");
>>>> + if (IS_ERR(st->reg))
>>>> + return PTR_ERR(st->reg);
[...]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-13 11:25 ` Lars-Peter Clausen
0 siblings, 0 replies; 27+ messages in thread
From: Lars-Peter Clausen @ 2014-12-13 11:25 UTC (permalink / raw)
To: Hartmut Knaack, Nikolaus Schulz
Cc: Jonathan Cameron, Peter Meerwald, Grant Likely, Rob Herring,
Michael Welling, Philippe Reynes,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Alban Bedel
On 12/13/2014 12:18 PM, Hartmut Knaack wrote:
[...]
>>> 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.
The regulator framework takes care of this. If you request a regulator with
$name the framework will look for a property with the name $name-supply.
>>
>>>> + st->reg =evm_regulator_get(&spi->dev, "vref");
>>>> + if (IS_ERR(st->reg))
>>>> + return PTR_ERR(st->reg);
[...]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-15 10:02 ` Nikolaus Schulz
0 siblings, 0 replies; 27+ messages in thread
From: Nikolaus Schulz @ 2014-12-15 10:02 UTC (permalink / raw)
To: Hartmut Knaack
Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald,
Grant Likely, Rob Herring, Michael Welling, Philippe Reynes,
linux-kernel, linux-iio, devicetree, Alban Bedel
On Sat, Dec 13, 2014 at 12:18:50PM +0100, Hartmut Knaack wrote:
> 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 a=
n SPI
> >>> interface.
[dac8554_probe:]
> >>> + st->vref_mv =3Doltage_uv / 1000;
> >> How hard do you want to depend on a voltage regulator? Doing regulat=
or_get_voltage()
> >> could even be called dynamically in _read_raw(), making a real regul=
ator optional.
> >=20
> > 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 look=
ed
> > at handles that case, they all consider it an error if the regulator =
is
> > absent.
> >=20
> I had people in mind, who might want to use such DAC with one of the po=
pular
> 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 dis=
tribution
> kernel anyway.
As long as the kernel supports devicetree, compiling the module and a
new DT blob should be enough to enable support for the DAC8554.
And, as Jonathan has pointed out, if there is no real regulator
providing the input current, a fixed regulator can be added to the DT.
> 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.
Agreed.
--=20
Avionic Design GmbH
Nikolaus Schulz
Wragekamp 10
D-22397 Hamburg
Germany
Tel.: +49 40 88187-163
Fax: +49 40 88187-150
Email: nikolaus.schulz@avionic-design.de
Avionic Design GmbH
Amtsgericht Hamburg HRB 82598
Gesch=E4ftsf=FChrung: Cornelis Broers
Ust.-Ident-Nr.: DE813378254
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-15 10:02 ` Nikolaus Schulz
0 siblings, 0 replies; 27+ messages in thread
From: Nikolaus Schulz @ 2014-12-15 10:02 UTC (permalink / raw)
To: Hartmut Knaack
Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald,
Grant Likely, Rob Herring, Michael Welling, Philippe Reynes,
linux-kernel, linux-iio, devicetree, Alban Bedel
On Sat, Dec 13, 2014 at 12:18:50PM +0100, Hartmut Knaack wrote:
> 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.
[dac8554_probe:]
> >>> + 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.
As long as the kernel supports devicetree, compiling the module and a
new DT blob should be enough to enable support for the DAC8554.
And, as Jonathan has pointed out, if there is no real regulator
providing the input current, a fixed regulator can be added to the DT.
> 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.
Agreed.
--
Avionic Design GmbH
Nikolaus Schulz
Wragekamp 10
D-22397 Hamburg
Germany
Tel.: +49 40 88187-163
Fax: +49 40 88187-150
Email: nikolaus.schulz@avionic-design.de
Avionic Design GmbH
Amtsgericht Hamburg HRB 82598
Geschäftsführung: Cornelis Broers
Ust.-Ident-Nr.: DE813378254
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-15 10:02 ` Nikolaus Schulz
0 siblings, 0 replies; 27+ messages in thread
From: Nikolaus Schulz @ 2014-12-15 10:02 UTC (permalink / raw)
To: Hartmut Knaack
Cc: Jonathan Cameron, Lars-Peter Clausen, Peter Meerwald,
Grant Likely, Rob Herring, Michael Welling, Philippe Reynes,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Alban Bedel
On Sat, Dec 13, 2014 at 12:18:50PM +0100, Hartmut Knaack wrote:
> 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.
[dac8554_probe:]
> >>> + 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.
As long as the kernel supports devicetree, compiling the module and a
new DT blob should be enough to enable support for the DAC8554.
And, as Jonathan has pointed out, if there is no real regulator
providing the input current, a fixed regulator can be added to the DT.
> 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.
Agreed.
--
Avionic Design GmbH
Nikolaus Schulz
Wragekamp 10
D-22397 Hamburg
Germany
Tel.: +49 40 88187-163
Fax: +49 40 88187-150
Email: nikolaus.schulz-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org
Avionic Design GmbH
Amtsgericht Hamburg HRB 82598
Geschäftsführung: Cornelis Broers
Ust.-Ident-Nr.: DE813378254
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-13 11:32 ` Lars-Peter Clausen
0 siblings, 0 replies; 27+ messages in thread
From: Lars-Peter Clausen @ 2014-12-13 11:32 UTC (permalink / raw)
To: Hartmut Knaack, Nikolaus Schulz, Jonathan Cameron, Peter Meerwald,
Grant Likely, Rob Herring, Michael Welling, Philippe Reynes,
linux-kernel, linux-iio, devicetree
Cc: Alban Bedel
On 12/06/2014 12:36 PM, Hartmut Knaack wrote:
[...]
>> +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).
We should call it info in new driver. It used to be a mask, but that was a
long long time ago.
[...]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2 2/2] iio: add driver for the TI DAC8554
@ 2014-12-13 11:32 ` Lars-Peter Clausen
0 siblings, 0 replies; 27+ messages in thread
From: Lars-Peter Clausen @ 2014-12-13 11:32 UTC (permalink / raw)
To: Hartmut Knaack, Nikolaus Schulz, Jonathan Cameron, Peter Meerwald,
Grant Likely, Rob Herring, Michael Welling, Philippe Reynes,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: Alban Bedel
On 12/06/2014 12:36 PM, Hartmut Knaack wrote:
[...]
>> +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).
We should call it info in new driver. It used to be a mask, but that was a
long long time ago.
[...]
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 27+ messages in thread