iio: add m62332 DAC driver
PATCH 1/1 iio: add m62332 DAC driver
m62332 is a simple 2-channel DAC used on several Sharp Zaurus boards to control LCD voltage, backlight and sound. The driver use regulators to control the reference voltage and enabling/disabling the DAC.
Signed-off-by Dmitry Eremin-Solenikov <[email protected]>
--- drivers/iio/dac/Kconfig | 10 ++ drivers/iio/dac/Makefile | 1 + drivers/iio/dac/m62332.c | 236 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 247 insertions(+) create mode 100644 drivers/iio/dac/m62332.c
drivers/iio/dac/Kconfig +10 -0
@@ -142,6 +142,16 @@ config AD7303 To compile this driver as module choose M here: the module will be called ad7303. +config M62332 + tristate "Mitsubishi M62332 DAC driver" + depends on I2C + help + If you say yes here you get support for the Mitsubishi M62332 + (I2C 8-Bit DACs with rail-to-rail outputs). + + This driver can also be built as a module. If so, the module + will be called m62332. + config MAX517 tristate "Maxim MAX517/518/519 DAC driver" depends on I2C
drivers/iio/dac/Makefile +1 -0
@@ -16,6 +16,7 @@ obj-$(CONFIG_AD5764) += ad5764.o obj-$(CONFIG_AD5791) += ad5791.o obj-$(CONFIG_AD5686) += ad5686.o obj-$(CONFIG_AD7303) += ad7303.o +obj-$(CONFIG_M62332) += m62332.o obj-$(CONFIG_MAX517) += max517.o obj-$(CONFIG_MAX5821) += max5821.o obj-$(CONFIG_MCP4725) += mcp4725.o
drivers/iio/dac/m62332.c new file +236 -0
236 lines not on this page · raw message
Signature · 1 line
2.1.4
Quoted 4 lines
m62332 is a simple 2-channel DAC used on several Sharp Zaurus boards to control LCD voltage, backlight and sound. The driver use regulators to control the reference voltage and enabling/disabling the DAC.
some minor comments inline
Quoted 59 lines
Signed-off-by Dmitry Eremin-Solenikov <[email protected]>--- drivers/iio/dac/Kconfig | 10 ++ drivers/iio/dac/Makefile | 1 + drivers/iio/dac/m62332.c | 236 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 247 insertions(+) create mode 100644 drivers/iio/dac/m62332.cdrivers/iio/dac/Kconfig +10
-0@@ -142,6 +142,16 @@ config AD7303 To compile this driver as module choose M here: the module will be called ad7303. +config M62332 + tristate "Mitsubishi M62332 DAC driver" + depends on I2C + help + If you say yes here you get support for the Mitsubishi M62332 + (I2C 8-Bit DACs with rail-to-rail outputs). + + This driver can also be built as a module. If so, the module + will be called m62332. + config MAX517 tristate "Maxim MAX517/518/519 DAC driver" depends on I2Cdrivers/iio/dac/Makefile +1
-0@@ -16,6 +16,7 @@ obj-$(CONFIG_AD5764) += ad5764.o obj-$(CONFIG_AD5791) += ad5791.o obj-$(CONFIG_AD5686) += ad5686.o obj-$(CONFIG_AD7303) += ad7303.o +obj-$(CONFIG_M62332) += m62332.o obj-$(CONFIG_MAX517) += max517.o obj-$(CONFIG_MAX5821) += max5821.o obj-$(CONFIG_MCP4725) += mcp4725.odrivers/iio/dac/m62332.c new file +12
-0@@ -0,0 +1,236 @@ +/* +* m62332.c - Support for Mitsubishi m62332 DAC +* +* Copyright (c) 2014 Dmitry Eremin-Solenikov +* +* Based on max517 driver: +* Copyright (C) 2010, 2011 Roland Stigge <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version.
drivers/iio/dac/m62332.c line 22
Quoted 10 lines
drivers/iio/dac/m62332.c new file +10
-0+* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/jiffies.h>
is jiffies.h needed?
drivers/iio/dac/m62332.c line 34
Quoted 12 lines
drivers/iio/dac/m62332.c new file +12
-0+#include <linux/i2c.h> +#include <linux/err.h> + +#include <linux/iio/iio.h> +#include <linux/iio/driver.h> + +#include <linux/regulator/consumer.h> + +struct m62332_data { +struct i2c_client *client; +unsigned short vref_mv; +unsigned char raw[2 ];
could use u8, u16
Quoted 10 lines
drivers/iio/dac/m62332.c new file +10
-0+struct regulator *vcc; +#ifdef CONFIG_PM_SLEEP +unsigned char save[2 ]; +#endif +}; + +static int m62332_set_value (struct iio_dev *indio_dev, +unsigned char val,int channel) +{ +struct m62332_data *data =iio_priv (indio_dev);
drivers/iio/dac/m62332.c line 54
Quoted 10 lines
drivers/iio/dac/m62332.c new file +10
-0+struct i2c_client *client = data->client; +u8 outbuf[2 ]; +int res =0 ; + +if (val == data->raw[channel]) +return 0 ; + + outbuf[0 ] = channel; + outbuf[1 ] = val; +
probably use a mutex here to protect the enable/send/disable sequence
drivers/iio/dac/m62332.c line 55
Quoted 1 line
drivers/iio/dac/m62332.c new file +1
-0+if (val || data->raw[!channel])
!channel looks a bit strange; what is the intent here?
is data->raw[!channel] needed?
maybe there exist devices with more than 2 channels so a function like all_channels_off() would make sense
drivers/iio/dac/m62332.c line 65
Quoted 10 lines
drivers/iio/dac/m62332.c new file +10
-0+ res =regulator_enable (data->vcc); +if (res) +return res; + + res =i2c_master_send (client, outbuf,2 ); +if (res <0 ) +return res; +else if (res !=2 ) +return -EIO ; +
should the regulator be disabled if write fails?
Quoted 120 lines
drivers/iio/dac/m62332.c new file +120
-0+ data->raw[channel] = val; + +if (!data->raw[0 ] && !data->raw[1 ]) +regulator_disable (data->vcc); + +return 0 ; +} + +static int m62332_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val, +int *val2, +long m) +{ +struct m62332_data *data =iio_priv (indio_dev); + +switch (m) { +case IIO_CHAN_INFO_SCALE : +/* Corresponds to Vref / 2^(bits) */ + *val = data->vref_mv; + *val2 =8 ; +return IIO_VAL_FRACTIONAL_LOG2 ; +case IIO_CHAN_INFO_RAW : + *val = data->raw[chan->channel]; +return IIO_VAL_INT ; +default : +break ; + } +return -EINVAL ; +} + +static int m62332_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan,int val,int val2,long mask) +{ +int ret; + +switch (mask) { +case IIO_CHAN_INFO_RAW : +if (val <0 || val >255 ) +return -EINVAL ; + + ret =m62332_set_value (indio_dev, val, chan->channel); +break ; +default : + ret = -EINVAL ; +break ; + } + +return ret; +} + +#ifdef CONFIG_PM_SLEEP +static int m62332_suspend (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +struct iio_dev *indio_dev =i2c_get_clientdata (client); +struct m62332_data *data =iio_priv (indio_dev); +int ret; + + data->save[0 ] = data->raw[0 ]; + data->save[1 ] = data->raw[1 ]; + + ret =m62332_set_value (indio_dev,0 ,0 ); +if (ret <0 ) +return ret; + +return m62332_set_value (indio_dev,0 ,1 ); +} + +static int m62332_resume (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +struct iio_dev *indio_dev =i2c_get_clientdata (client); +struct m62332_data *data =iio_priv (indio_dev); +int ret; + + ret =m62332_set_value (indio_dev, data->save[0 ],0 ); +if (ret <0 ) +return ret; + +return m62332_set_value (indio_dev, data->save[1 ],1 ); +} + +static SIMPLE_DEV_PM_OPS(m62332_pm_ops, m62332_suspend, m62332_resume); +#define M62332_PM_OPS (&m62332_pm_ops) +#else +#define M62332_PM_OPS NULL +#endif + +static const struct iio_info m62332_info = { + .read_raw = m62332_read_raw, + .write_raw = m62332_write_raw, + .driver_module =THIS_MODULE , +}; + +#define M62332_CHANNEL (chan) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .output = 1, \ + .channel = (chan), \ + .datasheet_name = "CH" #chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ +} + +static const struct iio_chan_spec m62332_channels[] = { +M62332_CHANNEL (0 ), +M62332_CHANNEL (1 ) +}; + +static int m62332_probe (struct i2c_client *client, +const struct i2c_device_id *id) +{ +struct m62332_data *data; +struct iio_dev *indio_dev; + + indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*data)); +if (!indio_dev) +return -ENOMEM ; + data =iio_priv (indio_dev);
drivers/iio/dac/m62332.c line 195
Quoted 10 lines
drivers/iio/dac/m62332.c new file +10
-0+i2c_set_clientdata (client, indio_dev); + data->client = client; + + data->vcc =devm_regulator_get (&client->dev,"VCC" ); +if (IS_ERR (data->vcc)) +return PTR_ERR (data->vcc); + +/* establish that the iio_dev is a child of the i2c device */ + indio_dev->dev.parent = &client->dev; +
indio_dev->name?
drivers/iio/dac/m62332.c line 203
Quoted 8 lines
drivers/iio/dac/m62332.c new file +8
-0+ indio_dev->num_channels =2 ; + indio_dev->channels = m62332_channels; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->info = &m62332_info; + + data->vref_mv =regulator_get_voltage (data->vcc) /1000 ;/* mV */ + +iio_map_array_register (indio_dev, client->dev.platform_data);
check return code?
Quoted 34 lines
drivers/iio/dac/m62332.c new file +33
-0+ +return iio_device_register (indio_dev); +} + +static int m62332_remove (struct i2c_client *client) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (client); + +iio_device_unregister (indio_dev); +iio_map_array_unregister (indio_dev); +return 0 ; +} + +static const struct i2c_device_id m62332_id[] = { + {"m62332" , }, + { } +}; +MODULE_DEVICE_TABLE (i2c, m62332_id); + +static struct i2c_driver m62332_driver = { + .driver = { + .name ="m62332" , + .pm =M62332_PM_OPS , + }, + .probe = m62332_probe, + .remove = m62332_remove, + .id_table = m62332_id, +}; +module_i2c_driver (m62332_driver); + +MODULE_AUTHOR ("Dmitry Eremin-Solenikov" ); +MODULE_DESCRIPTION ("M62332 8-bit DAC" ); +MODULE_LICENSE ("GPL v2" );
Signature · 2 lines
Peter Meerwald +43-664-2444418 (mobile)
Jonathan Cameron
maintainerQuoted 5 lines · Dmitry Eremin-Solenikov
m62332 is a simple 2-channel DAC used on several Sharp Zaurus boards to control LCD voltage, backlight and sound. The driver use regulators to control the reference voltage and enabling/disabling the DAC.
Signed-off-by Dmitry Eremin-Solenikov <[email protected]>
Looks pretty good to me. I've commented on various trivial bits in line. There are a few types that I think should be explicit kernel types of appropriate length rather than unsigned chars.
That's just me being really fussy though. It's a nice compact and clean driver. Lots of time now anyway a I'm afraid my review is too late for the upcoming merge window...
Thanks,
Jonathan
Quoted 69 lines
--- drivers/iio/dac/Kconfig | 10 ++ drivers/iio/dac/Makefile | 1 + drivers/iio/dac/m62332.c | 236 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 247 insertions(+) create mode 100644 drivers/iio/dac/m62332.cdrivers/iio/dac/Kconfig +10
-0@@ -142,6 +142,16 @@ config AD7303 To compile this driver as module choose M here: the module will be called ad7303. +config M62332 + tristate "Mitsubishi M62332 DAC driver" + depends on I2C + help + If you say yes here you get support for the Mitsubishi M62332 + (I2C 8-Bit DACs with rail-to-rail outputs). + + This driver can also be built as a module. If so, the module + will be called m62332. + config MAX517 tristate "Maxim MAX517/518/519 DAC driver" depends on I2Cdrivers/iio/dac/Makefile +1
-0@@ -16,6 +16,7 @@ obj-$(CONFIG_AD5764) += ad5764.o obj-$(CONFIG_AD5791) += ad5791.o obj-$(CONFIG_AD5686) += ad5686.o obj-$(CONFIG_AD7303) += ad7303.o +obj-$(CONFIG_M62332) += m62332.o obj-$(CONFIG_MAX517) += max517.o obj-$(CONFIG_MAX5821) += max5821.o obj-$(CONFIG_MCP4725) += mcp4725.odrivers/iio/dac/m62332.c new file +23
-0@@ -0,0 +1,236 @@ +/* +* m62332.c - Support for Mitsubishi m62332 DAC +* +* Copyright (c) 2014 Dmitry Eremin-Solenikov +* +* Based on max517 driver: +* Copyright (C) 2010, 2011 Roland Stigge <[email protected]> +* +* This program is free software; you can redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/jiffies.h> +#include <linux/i2c.h>
drivers/iio/dac/m62332.c line 33
Quoted 10 lines
drivers/iio/dac/m62332.c new file +10
-0+#include <linux/err.h> + +#include <linux/iio/iio.h> +#include <linux/iio/driver.h> + +#include <linux/regulator/consumer.h> + +struct m62332_data { +struct i2c_client *client; +unsigned short vref_mv;
I'd make this an int to match the regulator_get_voltage type or an unsigned int perhaps as we know it isn't an error value (don't ask about negative voltage regulators ;)
drivers/iio/dac/m62332.c line 34
Quoted 1 line
drivers/iio/dac/m62332.c new file +1
-0+unsigned char raw[2 ];
u8 raw[2];
drivers/iio/dac/m62332.c line 37
Quoted 3 lines
drivers/iio/dac/m62332.c new file +3
-0+struct regulator *vcc; +#ifdef CONFIG_PM_SLEEP +unsigned char save[2 ];
u8 save[2];
(yeah, I know they are the same but as we have explicit types for 8 bit unsigned integers, yets use them).
Quoted 46 lines
drivers/iio/dac/m62332.c new file +46
-0+#endif +}; + +static int m62332_set_value (struct iio_dev *indio_dev, +unsigned char val,int channel) +{ +struct m62332_data *data =iio_priv (indio_dev); +struct i2c_client *client = data->client; +u8 outbuf[2 ]; +int res =0 ; + +if (val == data->raw[channel]) +return 0 ; + + outbuf[0 ] = channel; + outbuf[1 ] = val; + +if (val || data->raw[!channel]) + res =regulator_enable (data->vcc); +if (res) +return res; + + res =i2c_master_send (client, outbuf,2 ); +if (res <0 ) +return res; +else if (res !=2 ) +return -EIO ; + + data->raw[channel] = val; + +if (!data->raw[0 ] && !data->raw[1 ]) +regulator_disable (data->vcc); + +return 0 ; +} + +static int m62332_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val, +int *val2, +long m) +{ +struct m62332_data *data =iio_priv (indio_dev); + +switch (m) { +case IIO_CHAN_INFO_SCALE :
drivers/iio/dac/m62332.c line 93
Quoted 10 lines
drivers/iio/dac/m62332.c new file +10
-0+/* Corresponds to Vref / 2^(bits) */ + *val = data->vref_mv; + *val2 =8 ; +return IIO_VAL_FRACTIONAL_LOG2 ; +case IIO_CHAN_INFO_RAW : + *val = data->raw[chan->channel]; +return IIO_VAL_INT ; +default : +break ; + }
I'd move this into the default above and save a line.
drivers/iio/dac/m62332.c line 98
Quoted 5 lines
drivers/iio/dac/m62332.c new file +5
-0+return -EINVAL ; +} + +static int m62332_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan,int val,int val2,long mask)
There is a current fad for running checkpatch with strict which moans about the alignment of parameters, so you may well get a follow up patch from someone with too much time on their hands, realigning all these. Personally I couldn't care less either way.
Quoted 30 lines
drivers/iio/dac/m62332.c new file +30
-0+{ +int ret; + +switch (mask) { +case IIO_CHAN_INFO_RAW : +if (val <0 || val >255 ) +return -EINVAL ; + + ret =m62332_set_value (indio_dev, val, chan->channel); +break ; +default : + ret = -EINVAL ; +break ; + } + +return ret; +} + +#ifdef CONFIG_PM_SLEEP +static int m62332_suspend (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +struct iio_dev *indio_dev =i2c_get_clientdata (client); +struct m62332_data *data =iio_priv (indio_dev); +int ret; + + data->save[0 ] = data->raw[0 ]; + data->save[1 ] = data->raw[1 ]; + + ret =m62332_set_value (indio_dev,0 ,0 );
drivers/iio/dac/m62332.c line 138
Quoted 10 lines
drivers/iio/dac/m62332.c new file +10
-0+if (ret <0 ) +return ret; + +return m62332_set_value (indio_dev,0 ,1 ); +} + +static int m62332_resume (struct device *dev) +{ +struct i2c_client *client =to_i2c_client (dev); +struct iio_dev *indio_dev =i2c_get_clientdata (client);
I'd roll the two lines above into one line btu that's just a matter of personal taste so the current version is fine.
Quoted 43 lines
drivers/iio/dac/m62332.c new file +43
-0+struct m62332_data *data =iio_priv (indio_dev); +int ret; + + ret =m62332_set_value (indio_dev, data->save[0 ],0 ); +if (ret <0 ) +return ret; + +return m62332_set_value (indio_dev, data->save[1 ],1 ); +} + +static SIMPLE_DEV_PM_OPS(m62332_pm_ops, m62332_suspend, m62332_resume); +#define M62332_PM_OPS (&m62332_pm_ops) +#else +#define M62332_PM_OPS NULL +#endif + +static const struct iio_info m62332_info = { + .read_raw = m62332_read_raw, + .write_raw = m62332_write_raw, + .driver_module =THIS_MODULE , +}; + +#define M62332_CHANNEL (chan) { \ + .type = IIO_VOLTAGE, \ + .indexed = 1, \ + .output = 1, \ + .channel = (chan), \ + .datasheet_name = "CH" #chan, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ + BIT(IIO_CHAN_INFO_SCALE), \ +} + +static const struct iio_chan_spec m62332_channels[] = { +M62332_CHANNEL (0 ), +M62332_CHANNEL (1 ) +}; + +static int m62332_probe (struct i2c_client *client, +const struct i2c_device_id *id) +{ +struct m62332_data *data; +struct iio_dev *indio_dev; +
drivers/iio/dac/m62332.c line 191
Quoted 10 lines
drivers/iio/dac/m62332.c new file +10
-0+ indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*data)); +if (!indio_dev) +return -ENOMEM ; + data =iio_priv (indio_dev); +i2c_set_clientdata (client, indio_dev); + data->client = client; + + data->vcc =devm_regulator_get (&client->dev,"VCC" ); +if (IS_ERR (data->vcc)) +return PTR_ERR (data->vcc);
ah. Was expecting a regulator enable here, but I see you do it on a more fine grained basis and only ensure it is on when you need it. A nice approach.
Quoted 12 lines
drivers/iio/dac/m62332.c new file +12
-0+ +/* establish that the iio_dev is a child of the i2c device */ + indio_dev->dev.parent = &client->dev; + + indio_dev->num_channels =2 ; + indio_dev->channels = m62332_channels; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->info = &m62332_info; + + data->vref_mv =regulator_get_voltage (data->vcc) /1000 ;/* mV */ + +iio_map_array_register (indio_dev, client->dev.platform_data);
drivers/iio/dac/m62332.c line 213
Quoted 10 lines
drivers/iio/dac/m62332.c new file +10
-0+ +return iio_device_register (indio_dev); +} + +static int m62332_remove (struct i2c_client *client) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (client); + +iio_device_unregister (indio_dev); +iio_map_array_unregister (indio_dev);
perhaps a blank line here. Hmm. Perhaps we want a devm_ version of iio_map_array_register. Ah well job for another day.
Quoted 24 lines
drivers/iio/dac/m62332.c new file +23
-0+return 0 ; +} + +static const struct i2c_device_id m62332_id[] = { + {"m62332" , }, + { } +}; +MODULE_DEVICE_TABLE (i2c, m62332_id); + +static struct i2c_driver m62332_driver = { + .driver = { + .name ="m62332" , + .pm =M62332_PM_OPS , + }, + .probe = m62332_probe, + .remove = m62332_remove, + .id_table = m62332_id, +}; +module_i2c_driver (m62332_driver); + +MODULE_AUTHOR ("Dmitry Eremin-Solenikov" ); +MODULE_DESCRIPTION ("M62332 8-bit DAC" ); +MODULE_LICENSE ("GPL v2" );