Søren Andersen schrieb, Am 05.09.2014 13:44: > v3: > Functions moved around. > Return values is simpler. This comment should be below the ---. Yet, you lack a proper commit message. > > Signed-off-by: Soeren Andersen <san at rosetechnology.dk> > --- > drivers/iio/adc/mcp320x.c | 201 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------- > 1 file changed, 172 insertions(+), 29 deletions(-) > > diff --git a/drivers/iio/adc/mcp320x.c b/drivers/iio/adc/mcp320x.c > index 28a086e..77ee363 100644 > --- a/drivers/iio/adc/mcp320x.c > +++ b/drivers/iio/adc/mcp320x.c > @@ -1,9 +1,20 @@ > /* > * Copyright (C) 2013 Oskar Andero <oskar.andero@xxxxxxxxx> > + * Copyright (C) 2014 Allan Bendorff Jensen <abj@xxxxxxxxxxxxxxxxx> I'm a bit confused, as how Allan is related in the development of this driver? > * > - * Driver for Microchip Technology's MCP3204 and MCP3208 ADC chips. > - * Datasheet can be found here: > - * http://ww1.microchip.com/downloads/en/devicedoc/21298c.pdf Hmm, providing less information on where to get datasheets instead of adding the sources for the additional devices is not exactly what I like to see happening. > + * Driver for following ADC chips from Microchip Technology's: > + * 10 Bit converter > + * MCP3001 > + * MCP3002 > + * MCP3004 > + * MCP3008 > + * ------------ > + * 12 bit converter > + * MCP3201 > + * MCP3202 > + * MCP3204 > + * MCP3208 > + * ------------ > * > * 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 > @@ -16,10 +27,16 @@ > #include <linux/iio/iio.h> > #include <linux/regulator/consumer.h> > > -#define MCP_SINGLE_ENDED (1 << 3) > -#define MCP_START_BIT (1 << 4) > +static int mcp320x_channel_to_tx_data(int device_index, > + const unsigned int channel, bool differential); What's the point in just declaring this function in this place, instead of placing it right above mcp320x_adc_conversion()? > > enum { > + mcp3001, > + mcp3002, > + mcp3004, > + mcp3008, > + mcp3201, > + mcp3202, > mcp3204, > mcp3208, > }; > @@ -36,17 +53,48 @@ struct mcp320x { > struct mutex lock; > }; > > -static int mcp320x_adc_conversion(struct mcp320x *adc, u8 msg) > +static int mcp320x_adc_conversion(struct mcp320x *adc, u8 channel, > + bool differential, int device_index) The second line of parameters could be indented a bit more to the left to align with the first line parameters. > { > int ret; > + __be16 buf; Wouldn't it be a better idea to change the type of rx_buf in struct mcp320x to __be16 and use it for the one channel devices? > > - adc->tx_buf = msg; > - ret = spi_sync(adc->spi, &adc->msg); > - if (ret < 0) > - return ret; > + adc->rx_buf[0] = 0; > + adc->rx_buf[1] = 0; > + adc->tx_buf = mcp320x_channel_to_tx_data(device_index, > + channel, differential); In your current implementation, mcp320x_channel_to_tx_data() can return -EINVAL for mcp3x01 devices, but adc->tx_buf is of type u8. It does not really do any harm, as tx_buf is not sent to these devices, but it is ugly to say the least. Not sure if it would be better to do ret = mcp320x_channel_to_tx_data(), then check ret for errors and afterwards do adc->tx_buf = ret, or just change the type of mcp320x_channel_to_tx_data() to u8. Anyway, you need to keep mcp3x01 devices in mind. > + > + if (device_index != mcp3001 && device_index != mcp3201) { > + ret = spi_sync(adc->spi, &adc->msg); > + if (ret < 0) > + return ret; > + } else { > + ret = spi_read(adc->spi, (u8 *)&buf, 2); Preferably this way: ret = spi_read(adc->spi, &adc->rx_buf), sizeof(adc->rx_buf); > + if (ret < 0) > + return ret; > + } > > - return ((adc->rx_buf[0] & 0x3f) << 6) | > - (adc->rx_buf[1] >> 2); > + switch (device_index) { > + case mcp3001: > + return ((be16_to_cpu(buf) >> 3) & 0x03ff); > + case mcp3002: > + return (((adc->rx_buf[0] << 2) | > + ((adc->rx_buf[1] & 0xC0) >> 6)) & 0x03ff); > + case mcp3004: > + case mcp3008: > + return ((((adc->rx_buf[0] & 0x7f) << 3) | > + ((adc->rx_buf[1] & 0xe0) >> 5)) & 0x03ff); > + case mcp3201: > + return (be16_to_cpu(buf) >> 1); > + case mcp3202: > + return ((adc->rx_buf[0] << 4) | ((adc->rx_buf[1] & 0xf0) >> 4)); > + case mcp3204: > + case mcp3208: > + return (((adc->rx_buf[0] & 0x7f) << 5) | > + ((adc->rx_buf[1] & 0xf8) >> 3)); As I tried to point out in my previous review, you can make the conversion process way easier by optimizing your tx-message. For all those devices with more than 1 channel, you determine with the position of the start-bit the position of the conversion data bits. Further down I will give you an example on how to do this, so that the adc will start sending its 10 or 12 bits of data starting with the MSB of rx_buf, so you will just have to shift it to the right, without need for masking out anything. This is how the above block would turn out this way: case mcp3001: return ((be16_to_cpu(buf) >> 3) & 0x03ff); case mcp3002: case mcp3004: case mcp3008: return adc->rx_buf >> 6; case mcp3201: return (be16_to_cpu(buf) >> 1); case mcp3202: case mcp3204: case mcp3208: return adc->rx_buf >> 4; > + default: > + return -EINVAL; > + } > } > > static int mcp320x_read_raw(struct iio_dev *indio_dev, > @@ -55,18 +103,17 @@ static int mcp320x_read_raw(struct iio_dev > *indio_dev, > { > struct mcp320x *adc = iio_priv(indio_dev); > int ret = -EINVAL; > + int device_index = 0; > > mutex_lock(&adc->lock); > > + device_index = spi_get_device_id(adc->spi)->driver_data; > + > switch (mask) { > case IIO_CHAN_INFO_RAW: > - if (channel->differential) > - ret = mcp320x_adc_conversion(adc, > - MCP_START_BIT | channel->address); > - else > - ret = mcp320x_adc_conversion(adc, > - MCP_START_BIT | MCP_SINGLE_ENDED | > - channel->address); > + ret = mcp320x_adc_conversion(adc, channel->address, > + channel->differential, device_index); Indentation could be better. > + > if (ret < 0) > goto out; > > @@ -75,17 +122,23 @@ static int mcp320x_read_raw(struct iio_dev > *indio_dev, > break; > > case IIO_CHAN_INFO_SCALE: > - /* Digital output code = (4096 * Vin) / Vref */ > + /* > + * Digital output code = (resolution * Vin) / Vref > + * Get regulator output voltage in uV. > + */ I don't really see the need for these comments. It's just a matter of seconds to search in LXR for the meaning of return values. > ret = regulator_get_voltage(adc->reg); > if (ret < 0) > goto out; > > + /* convert regulator output voltage to mV */ > *val = ret / 1000; > - *val2 = 12; > - ret = IIO_VAL_FRACTIONAL_LOG2; > - break; > + if (device_index == mcp3001 || device_index == mcp3002 || > + device_index == mcp3004 || device_index == mcp3008) Your indentation here is wrong, as it should be aligned with the opening parenthesis. But the whole solution is quite ugly and could be improved by adding a resolution attribute to mcp320x_chip_info, which you could then access here - either by adding a reference to the entry in mcp3208_chip_infos[] to struct mcp320x, or by adding a resolution/bits variable to struct mcp320x; being set in the probe function, as soon as chip_info can be accessed. See for example in max1363, how it can be done. > + *val2 = 10; > + else > + *val2 = 12; > This empty line should also leave. > - default: > + ret = IIO_VAL_FRACTIONAL_LOG2; > break; > } > > @@ -117,6 +170,16 @@ out: > .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) \ > } > > +static const struct iio_chan_spec mcp3201_channels[] = { > + MCP320X_VOLTAGE_CHANNEL_DIFF(0), > +}; > + > +static const struct iio_chan_spec mcp3202_channels[] = { > + MCP320X_VOLTAGE_CHANNEL(0), > + MCP320X_VOLTAGE_CHANNEL(1), > + MCP320X_VOLTAGE_CHANNEL_DIFF(0), > +}; > + > static const struct iio_chan_spec mcp3204_channels[] = { > MCP320X_VOLTAGE_CHANNEL(0), > MCP320X_VOLTAGE_CHANNEL(1), > @@ -146,12 +209,36 @@ static const struct iio_info mcp320x_info = { > .driver_module = THIS_MODULE, > }; > > -struct mcp3208_chip_info { > +struct mcp320x_chip_info { > const struct iio_chan_spec *channels; > unsigned int num_channels; Add for example u8 resolution here. > }; > > -static const struct mcp3208_chip_info mcp3208_chip_infos[] = { > +static const struct mcp320x_chip_info mcp320x_chip_infos[] = { > + [mcp3001] = { > + .channels = mcp3201_channels, > + .num_channels = ARRAY_SIZE(mcp3201_channels) And then set .resolution = 10 here and appropriate below. > + }, > + [mcp3002] = { > + .channels = mcp3202_channels, > + .num_channels = ARRAY_SIZE(mcp3202_channels) > + }, > + [mcp3004] = { > + .channels = mcp3204_channels, > + .num_channels = ARRAY_SIZE(mcp3204_channels) > + }, > + [mcp3008] = { > + .channels = mcp3208_channels, > + .num_channels = ARRAY_SIZE(mcp3208_channels) > + }, > + [mcp3201] = { > + .channels = mcp3201_channels, > + .num_channels = ARRAY_SIZE(mcp3201_channels) > + }, > + [mcp3202] = { > + .channels = mcp3202_channels, > + .num_channels = ARRAY_SIZE(mcp3202_channels) > + }, > [mcp3204] = { > .channels = mcp3204_channels, > .num_channels = ARRAY_SIZE(mcp3204_channels) > @@ -162,11 +249,29 @@ static const struct mcp3208_chip_info > mcp3208_chip_infos[] = { > }, > }; > > +static int mcp320x_channel_to_tx_data(int device_index, > + const unsigned int channel, bool differential) As mentioned above, the only calling instance expects u8 as return value. Up to you, what solution you will choose. > +{ > + switch (device_index) { > + case mcp3002: > + case mcp3202: > + return(0x12 | (!differential << 3) | ((channel & 0x01) << 2)); Missing whitespace after return. > + case mcp3004: > + case mcp3204: > + return(0x20 | (!differential << 4) | ((channel & 0x03) << 1)); To align the adc data like I suggested above, use this: return (0x40 | (!differential << 5) | ((channel & 0x03) << 2)); > + case mcp3008: > + case mcp3208: > + return(0x20 | (!differential << 4) | ((channel & 0x07) << 1)); Same here: return (0x40 | (!differential << 5) | ((channel & 0x07) << 2)); > + default: > + return -EINVAL; Cases for mcp3x01 are missing and would be caught by default. > + } > +} > + > static int mcp320x_probe(struct spi_device *spi) > { > struct iio_dev *indio_dev; > struct mcp320x *adc; > - const struct mcp3208_chip_info *chip_info; > + const struct mcp320x_chip_info *chip_info; > int ret; > > indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc)); > @@ -181,7 +286,7 @@ static int mcp320x_probe(struct spi_device *spi) > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->info = &mcp320x_info; > > - chip_info = &mcp3208_chip_infos[spi_get_device_id(spi)->driver_data]; > + chip_info = &mcp320x_chip_infos[spi_get_device_id(spi)->driver_data]; > indio_dev->channels = chip_info->channels; > indio_dev->num_channels = chip_info->num_channels; > > @@ -226,7 +331,45 @@ static int mcp320x_remove(struct spi_device *spi) > return 0; > } > > +#if defined(CONFIG_OF) > +static const struct of_device_id mcp320x_dt_ids[] = { > + { > + .compatible = "mcp3001", > + .data = &mcp320x_chip_infos[mcp3001], > + }, { > + .compatible = "mcp3002", > + .data = &mcp320x_chip_infos[mcp3002], > + }, { > + .compatible = "mcp3004", > + .data = &mcp320x_chip_infos[mcp3004], > + }, { > + .compatible = "mcp3008", > + .data = &mcp320x_chip_infos[mcp3004], mcp3008? > + }, { > + .compatible = "mcp3201", > + .data = &mcp320x_chip_infos[mcp3201], > + }, { > + .compatible = "mcp3202", > + .data = &mcp320x_chip_infos[mcp3202], > + }, { > + .compatible = "mcp3204", > + .data = &mcp320x_chip_infos[mcp3204], > + }, { > + .compatible = "mcp3008", mcp3208? > + .data = &mcp320x_chip_infos[mcp3208], > + }, { > + } > +}; > +MODULE_DEVICE_TABLE(of, mcp320x_dt_ids); > +#endif > + > static const struct spi_device_id mcp320x_id[] = { > + { "mcp3001", mcp3001 }, > + { "mcp3002", mcp3002 }, > + { "mcp3004", mcp3004 }, > + { "mcp3008", mcp3008 }, > + { "mcp3201", mcp3201 }, > + { "mcp3202", mcp3202 }, > { "mcp3204", mcp3204 }, > { "mcp3208", mcp3208 }, > { } > @@ -245,5 +388,5 @@ static struct spi_driver mcp320x_driver = { > module_spi_driver(mcp320x_driver); > > MODULE_AUTHOR("Oskar Andero <oskar.andero@xxxxxxxxx>"); > -MODULE_DESCRIPTION("Microchip Technology MCP3204/08"); > +MODULE_DESCRIPTION("Microchip Technology MCP3x01/02/04/08"); > MODULE_LICENSE("GPL v2"); > > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-iio" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
