| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230828151631.6a60282b@jic23-huawei>
Date: Mon, 28 Aug 2023 15:16:31 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: <marius.cristea@...rochip.com>
Cc: <lars@...afoo.de>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
<linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] iio: adc: adding support for MCP3564 ADC
On Fri, 18 Aug 2023 19:57:50 +0300
<marius.cristea@...rochip.com> wrote:
> From: Marius Cristea <marius.cristea@...rochip.com>
>
> This is the iio driver for Microchip family of 153.6 ksps,
> Low-Noise 16/24-Bit Delta-Sigma ADCs with an SPI interface
> (Microchip's MCP3461, MCP3462, MCP3464, MCP3461R, MCP3462R,
> MCP3464R, MCP3561, MCP3562, MCP3564, MCP3561R,
> MCP3562R and MCP3564R analog to digital converters).
>
> Signed-off-by: Marius Cristea <marius.cristea@...rochip.com>
Hi Marius,
A few more things came up when I was reading this version.
See inline.
Thanks,
Jonathan
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index e07e4a3e6237..91f713af574f 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_MAX1363) += max1363.o
> obj-$(CONFIG_MAX9611) += max9611.o
> obj-$(CONFIG_MCP320X) += mcp320x.o
> obj-$(CONFIG_MCP3422) += mcp3422.o
> +obj-$(CONFIG_MCP3564) += mcp3564.o
> obj-$(CONFIG_MCP3911) += mcp3911.o
> obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o
> obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o
> diff --git a/drivers/iio/adc/mcp3564.c b/drivers/iio/adc/mcp3564.c
> new file mode 100644
> index 000000000000..8c5649781093
> --- /dev/null
> +++ b/drivers/iio/adc/mcp3564.c
> @@ -0,0 +1,1527 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * IIO driver for MCP356X/MCP356XR and MCP346X/MCP346XR series ADC chip family
> + *
> + * Copyright (C) 2022-2023 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Marius Cristea <marius.cristea@...rochip.com>
> + *
> + * Datasheet for MCP3561, MCP3562, MCP3564 can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP3561-2-4-Family-Data-Sheet-DS20006181C.pdf
> + * Datasheet for MCP3561R, MCP3562R, MCP3564R can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3561_2_4R-Data-Sheet-DS200006391C.pdf
> + * Datasheet for MCP3461, MCP3462, MCP3464 can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3461-2-4-Two-Four-Eight-Channel-153.6-ksps-Low-Noise-16-Bit-Delta-Sigma-ADC-Data-Sheet-20006180D.pdf
> + * Datasheet for MCP3461R, MCP3462R, MCP3464R can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/APID/ProductDocuments/DataSheets/MCP3461-2-4R-Family-Data-Sheet-DS20006404C.pdf
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/iopoll.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/units.h>
> +#include <linux/util_macros.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#define MCP3564_CONFIG2_REG 0x03
> +#define MCP3564_CONFIG2_AZ_REF_MASK BIT(1)
> +#define MCP3564_CONFIG2_AZ_REF_SET(x) FIELD_PREP(MCP3564_CONFIG2_AZ_REF_MASK, (x))
> +#define MCP3564_CONFIG2_AZ_MUX_MASK BIT(2)
> +#define MCP3564_CONFIG2_AZ_MUX_SET(x) FIELD_PREP(MCP3564_CONFIG2_AZ_MUX_MASK, (x))
> +
> +#define MCP3564_CONFIG2_HARDWARE_GAIN_MASK GENMASK(5, 3)
> +#define MCP3564_CONFIG2_HARDWARE_GAIN_SET(x) FIELD_PREP(MCP3564_CONFIG2_HARDWARE_GAIN_MASK, (x))
I don't find the GAIN_SET() macros to be more readable than FIELD_PREP used directly
as the MASK naming makes it clear what is being set.
So I would drop all these in favour of inline use of FIELD_PREP
> +#define MCP3564_DEFAULT_HARDWARE_GAIN 0x01
> +#define MCP3564_CONFIG2_BOOST_CURRENT_MASK GENMASK(7, 6)
> +#define MCP3564_CONFIG2_BOOST_CURRENT_SET(x) FIELD_PREP(MCP3564_CONFIG2_BOOST_CURRENT_MASK, (x))
> +
...
> +#define MCP3464_CONFIG3_DATA_FORMAT_MASK GENMASK(5, 4)
> +#define MCP3564_CONFIG3_DATA_FORMAT_SET(x) FIELD_PREP(MCP3464_CONFIG3_DATA_FORMAT_MASK, (x))
> +
> +/* 0b11 = Continuous Conversion mode or continuous conversion cycle in SCAN mode. */
> +#define MCP3464_CONFIG3_CONV_MODE_CONTINUOUS 3
Values defined - so need for them in the comment which is otherwise useful.
> +/*
> + * 0b10 = One-shot conversion or one-shot cycle in SCAN mode. It sets ADC_MODE[1:0] to ‘10’
> + * (standby) at the end of the conversion or at the end of the conversion cycle in SCAN mode.
> + */
> +#define MCP3464_CONFIG3_CONV_MODE_ONE_SHOT_STANDBY 2
> +/*
> + * 0b0x = One-shot conversion or one-shot cycle in SCAN mode. It sets ADC_MODE[1:0] to ‘0x’ (ADC
> + * Shutdown) at the end of the conversion or at the end of the conversion cycle in SCAN
> + * mode (default).
> + */
> +#define MCP3564_CMD_HW_ADDR_MASK GENMASK(7, 6)
> +#define MCP3564_CMD_HW_ADDR_SET(x) FIELD_PREP(MCP3564_CMD_HW_ADDR_MASK, (x))
> +#define MCP3564_CMD_ADDR_MASK GENMASK(5, 2)
> +#define MCP3564_CMD_ADDR_SET(x) FIELD_PREP(MCP3564_CMD_ADDR_MASK, (x))
More cases where I'd like to see the FIELD_PREP() used directly not hidden in a macro.
> +
> +/*
> + * Current Source/Sink Selection Bits for Sensor Bias (source on VIN+/sink on VIN-)
> + */
> +static const int mcp3564_burnout_avail[][2] = {
> + [MCP3564_CONFIG0_CS_SEL_0_0_uA] = {0, 0},
> + [MCP3564_CONFIG0_CS_SEL_0_9_uA] = {0, 900},
> + [MCP3564_CONFIG0_CS_SEL_3_7_uA] = {0, 3700},
> + [MCP3564_CONFIG0_CS_SEL_15_uA] = {0, 15000}
> +};
Trivial, but I'd like brackets after the { and before the } as makes things a tiny bit
more readable to my eye.
> +/**
> + * struct mcp3564_chip_info - chip specific data
> + * @name: device name
> + * @num_channels: number of channels
> + * @resolution: ADC resolution
> + * @has_vref: does the hardware has an internal voltage reference?
have
> + */
> +struct mcp3564_chip_info {
> + const char *name;
> + unsigned int num_channels;
> + unsigned int resolution;
> + bool has_vref;
> +};
> +
...
> +
> +static int mcp3564_write_8bits(struct mcp3564_state *adc, u8 reg, u8 val)
> +{
> + u8 tmp;
> + u8 tx_buf[2];
> +
> + tx_buf[0] = mcp3564_cmd_write(adc->dev_addr, reg);
> + tx_buf[1] = val;
> +
> + return spi_write_then_read(adc->spi, tx_buf, sizeof(tx_buf), &tmp, 0);
as below.
> +}
> +
> +static int mcp3564_write_24bits(struct mcp3564_state *adc, u8 reg, u32 val)
> +{
> + u8 tmp;
> + __be32 val_be;
> +
> + val |= (mcp3564_cmd_write(adc->dev_addr, reg) << 24);
> + val_be = cpu_to_be32(val);
> +
> + return spi_write_then_read(adc->spi, &val_be, sizeof(val_be), &tmp, 0);
as below.
> +}
> +
> +static int mcp3564_fast_cmd(struct mcp3564_state *adc, u8 fast_cmd)
> +{
> + u8 val;
> + u8 tmp;
> +
> + val = MCP3564_CMD_HW_ADDR_SET(adc->dev_addr) | MCP3564_CMD_ADDR_SET(fast_cmd);
> +
> + return spi_write_then_read(adc->spi, &val, 1, &tmp, 0);
No need for tmp. I'm fairly sure spi_write_then_read is fine with NULL buffers.
> +}
> +
> +static int mcp3564_update_8bits(struct mcp3564_state *adc, u8 reg, u32 mask, u8 val)
> +{
> + u8 tmp;
> + int ret;
> +
> + ret = mcp3564_read_8bits(adc, reg, &tmp);
> + if (ret < 0)
> + return ret;
> +
> + val &= mask;
This looks wrong - would expect this to be
val &= ~mask; // wipe out the bits in mask.
> + val |= tmp & ~mask;
and
val |= tmp & mask; //write the bitss in mask.
Is the mask the inverse ? At first glance it doesn't seem to be.
> + ret = mcp3564_write_8bits(adc, reg, val);
> +
> + return ret;
return mcp...
> +}
...
> +
> +static int mcp3564_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + long info)
> +{
> + switch (info) {
> + case IIO_CHAN_INFO_RAW:
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_CALIBBIAS:
> + case IIO_CHAN_INFO_CALIBSCALE:
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mcp3564_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel, int val,
> + int val2, long mask)
> +{
> + struct mcp3564_state *adc = iio_priv(indio_dev);
> + int tmp;
> + unsigned int hwgain;
> + enum mcp3564_burnout burnout;
> + int ret = 0;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (!channel->output)
> + return -EINVAL;
> +
> + for (burnout = 0; burnout < MCP3564_MAX_BURNOUT_IDX; burnout++) {
> + if (val == mcp3564_burnout_avail[burnout][0] &&
> + val2 == mcp3564_burnout_avail[burnout][1])
> + break;
> + }
> +
> + if (burnout == MCP3564_MAX_BURNOUT_IDX)
> + return -EINVAL;
> +
> + if (burnout == adc->burnout_mode)
> + return ret;
> +
> + mutex_lock(&adc->lock);
> + ret = mcp3564_update_8bits(adc, MCP3564_CONFIG0_REG,
> + MCP3564_CONFIG0_CS_SEL_MASK,
> + MCP3564_CONFIG0_CS_SEL_SET(burnout));
As above - I'd find this clearer with FIELD_PREP here
FIELD_PREP(MCP3564_CONFIG0_CS_SEL_MASK, burnout));
> +
> + if (ret)
> + dev_err(&indio_dev->dev, "Failed to configure burnout current\n");
> + else
> + adc->burnout_mode = burnout;
> + mutex_unlock(&adc->lock);
> + return ret;
> + case IIO_CHAN_INFO_CALIBBIAS:
> + if (val < mcp3564_calib_bias[0] && val > mcp3564_calib_bias[2])
> + return -EINVAL;
> +
> + mutex_lock(&adc->lock);
> + ret = mcp3564_write_24bits(adc, MCP3564_OFFSETCAL_REG, val);
> + if (!ret)
> + adc->calib_bias = val;
> + mutex_unlock(&adc->lock);
> + return ret;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + if (val < mcp3564_calib_scale[0] && val > mcp3564_calib_scale[2])
> + return -EINVAL;
> +
> + if (adc->calib_scale == val)
> + return ret;
> +
> + mutex_lock(&adc->lock);
> + ret = mcp3564_write_24bits(adc, MCP3564_GAINCAL_REG, val);
> + if (!ret)
> + adc->calib_scale = val;
> + mutex_unlock(&adc->lock);
> + return ret;
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + if (val < 0)
> + return -EINVAL;
> +
> + tmp = find_closest(val, mcp3564_oversampling_avail,
> + ARRAY_SIZE(mcp3564_oversampling_avail));
> +
> + if (adc->oversampling == tmp)
> + return ret;
> +
> + mutex_lock(&adc->lock);
> + ret = mcp3564_update_8bits(adc, MCP3564_CONFIG1_REG,
> + MCP3564_CONFIG1_OVERSPL_RATIO_MASK,
> + MCP3564_CONFIG1_OVERSPL_RATIO_SET(adc->oversampling));
> + if (!ret)
> + adc->oversampling = tmp;
> + mutex_unlock(&adc->lock);
> + return ret;
> + case IIO_CHAN_INFO_SCALE:
> + for (hwgain = 0; hwgain < MCP3564_MAX_PGA; hwgain++) {
> + if (val == adc->scale_tbls[hwgain][0] &&
> + val2 == adc->scale_tbls[hwgain][1])
> + break;
> + }
> +
> + if (hwgain == MCP3564_MAX_PGA)
> + return -EINVAL;
> +
> + if (hwgain == adc->hwgain)
> + return ret;
> +
> + mutex_lock(&adc->lock);
> + /* Update GAIN in CONFIG2[5:3] -> GAIN[2:0]*/
I'd drop this comment as this is clear from teh code that follows and comments
rot over time ;)
> + ret = mcp3564_update_8bits(adc, MCP3564_CONFIG2_REG,
> + MCP3564_CONFIG2_HARDWARE_GAIN_MASK,
> + MCP3564_CONFIG2_HARDWARE_GAIN_SET(hwgain));
As above.
> + if (!ret)
> + adc->hwgain = hwgain;
> +
> + mutex_unlock(&adc->lock);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
...
> +
> +static int mcp3564_parse_fw_children(struct iio_dev *indio_dev)
> +{
> + struct mcp3564_state *adc = iio_priv(indio_dev);
> + struct device *dev = &adc->spi->dev;
> + struct iio_chan_spec *channels;
> + struct fwnode_handle *child;
> + struct iio_chan_spec chanspec = mcp3564_channel_template;
> + struct iio_chan_spec temp_chanspec = mcp3564_temp_channel_template;
> + struct iio_chan_spec burnout_chanspec = mcp3564_burnout_channel_template;
> + int chan_idx = 0;
> + unsigned int num_ch;
> + u32 inputs[2];
> + const char *node_name;
> + const char *label;
> + int ret;
> +
> + num_ch = device_get_child_node_count(dev);
> + if (num_ch == 0)
> + return dev_err_probe(&indio_dev->dev, -ENODEV,
> + "FW has no channels defined\n");
> +
> + /* Reserve space for burnout and temperature channel */
> + num_ch += 2;
> +
> + if (num_ch > adc->chip_info->num_channels) {
> + return dev_err_probe(dev, -EINVAL, "Too many channels %d > %d\n",
> + num_ch, adc->chip_info->num_channels);
As below. My interpretation kernel style tells me that this doesn't need brackets.
> + }
> +
> + channels = devm_kcalloc(dev, num_ch, sizeof(*channels), GFP_KERNEL);
> + if (!channels)
> + return dev_err_probe(dev, -ENOMEM, "Can't allocate memory\n");
> +
> + device_for_each_child_node(dev, child) {
> + node_name = fwnode_get_name(child);
> +
> + if (fwnode_property_present(child, "diff-channels")) {
> + ret = fwnode_property_read_u32_array(child,
> + "diff-channels",
> + inputs,
> + ARRAY_SIZE(inputs));
> + chanspec.differential = 1;
> + } else {
> + ret = fwnode_property_read_u32(child, "reg", &inputs[0]);
> +
> + chanspec.differential = 0;
> + inputs[1] = MCP3564_AGND;
> + }
> + if (ret) {
> + fwnode_handle_put(child);
> + return ret;
> + }
> +
> + if (inputs[0] > MCP3564_INTERNAL_VCM ||
> + inputs[1] > MCP3564_INTERNAL_VCM) {
> + fwnode_handle_put(child);
> + return dev_err_probe(&indio_dev->dev, -EINVAL,
> + "Channel index > %d, for %s\n",
> + MCP3564_INTERNAL_VCM + 1,
> + node_name);
> + }
> +
> + chanspec.address = (inputs[0] << 4) | inputs[1];
> + chanspec.channel = inputs[0];
> + chanspec.channel2 = inputs[1];
> + chanspec.scan_index = chan_idx;
> +
> + if (fwnode_property_present(child, "label")) {
> + fwnode_property_read_string(child, "label", &label);
> + adc->labels[chan_idx] = label;
> + }
> +
> + channels[chan_idx] = chanspec;
> + chan_idx++;
> + }
> +
> + /* Add burnout current channel */
> + burnout_chanspec.scan_index = chan_idx;
> + channels[chan_idx] = burnout_chanspec;
> + adc->labels[chan_idx] = mcp3564_channel_labels[0];
> + chanspec.scan_index = chan_idx;
> + chan_idx++;
> +
> + /* Add temperature channel */
> + temp_chanspec.scan_index = chan_idx;
> + channels[chan_idx] = temp_chanspec;
> + adc->labels[chan_idx] = mcp3564_channel_labels[1];
> + chan_idx++;
> +
> + indio_dev->num_channels = chan_idx;
> + indio_dev->channels = channels;
> +
> + return 0;
> +}
...
> +
> +static int mcp3564_config(struct iio_dev *indio_dev)
> +{
> + struct mcp3564_state *adc = iio_priv(indio_dev);
> + struct device *dev = &adc->spi->dev;
> + const struct spi_device_id *dev_id;
> + u8 tmp_reg;
> + u16 tmp_u16;
> + enum mcp3564_ids ids;
> + int ret = 0;
> + unsigned int tmp = 0x01;
> + bool err = true;
> +
> + /*
> + * The address is set on a per-device basis by fuses in the factory,
> + * configured on request. If not requested, the fuses are set for 0x1.
> + * The device address is part of the device markings to avoid
> + * potential confusion. This address is coded on two bits, so four possible
> + * addresses are available when multiple devices are present on the same
> + * SPI bus with only one Chip Select line for all devices.
> + */
> + device_property_read_u32(dev, "microchip,hw-device-address", &tmp);
> +
> + if (tmp > 3) {
> + dev_err_probe(dev, tmp,
> + "invalid device address. Must be in range 0-3.\n");
> + return -EINVAL;
> + }
> +
> + adc->dev_addr = MCP3564_HW_ADDR_MASK & tmp;
FIELD_GET() as then we don't have to care if the mask goes to bit 0
> +
> + dev_dbg(dev, "use device address %i\n", adc->dev_addr);
> +
> + ret = mcp3564_read_8bits(adc, MCP3564_RESERVED_C_REG, &tmp_reg);
> + if (ret < 0)
> + return ret;
> +
> + switch (tmp_reg) {
> + case MCP3564_C_REG_DEFAULT:
> + adc->has_vref = false;
> + break;
> + case MCP3564R_C_REG_DEFAULT:
> + adc->has_vref = true;
> + break;
> + default:
> + dev_info(dev, "Unknown chip found: %d\n", tmp_reg);
> + err = true;
> + }
> +
> + if (!err) {
> + ret = mcp3564_read_16bits(adc, MCP3564_RESERVED_E_REG, &tmp_u16);
> + if (ret < 0)
> + return ret;
> +
> + switch (tmp_u16 & MCP3564_HW_ID_MASK) {
> + case MCP3461_HW_ID:
> + if (adc->has_vref)
> + ids = mcp3461r;
> + else
> + ids = mcp3461;
> + break;
> + case MCP3462_HW_ID:
> + if (adc->has_vref)
> + ids = mcp3462r;
> + else
> + ids = mcp3462;
> + break;
> + case MCP3464_HW_ID:
> + if (adc->has_vref)
> + ids = mcp3464r;
> + else
> + ids = mcp3464;
> + break;
> + case MCP3561_HW_ID:
> + if (adc->has_vref)
> + ids = mcp3561r;
> + else
> + ids = mcp3561;
> + break;
> + case MCP3562_HW_ID:
> + if (adc->has_vref)
> + ids = mcp3562r;
> + else
> + ids = mcp3562;
> + break;
> + case MCP3564_HW_ID:
> + if (adc->has_vref)
> + ids = mcp3564r;
> + else
> + ids = mcp3564;
> + break;
> + default:
> + dev_info(dev, "Unknown chip found: %d\n", tmp_u16);
> + err = true;
> + }
> + }
> +
> + if (err) {
> + /*
> + * If failed to identify the hardware based on internal registers,
> + * try using fallback compatible in device tree to deal with some newer part number.
> + */
> + adc->chip_info = spi_get_device_match_data(adc->spi);
> + if (!adc->chip_info) {
> + dev_id = spi_get_device_id(adc->spi);
> + adc->chip_info = (const struct mcp3564_chip_info *)dev_id->driver_data;
> + }
> +
> + adc->has_vref = adc->chip_info->has_vref;
> + } else {
> + adc->chip_info = &mcp3564_chip_infos_tbl[ids];
> + }
> +
> + dev_dbg(dev, "Found %s chip\n", adc->chip_info->name);
> +
> + adc->vref = devm_regulator_get_optional(dev, "vref");
> + if (IS_ERR(adc->vref)) {
> + if (IS_ERR(adc->vref)) {
I guess this is meant to be an ENODEV check.
I would suggest flipping the logic so the error return is out of line but the rest isn't.
if (PTR_ERR(adc->vref) != -ENODEV)
return dev_err_probe(...);
//side note that you are inconsistent on brackets around a single line statement with
// a line break. None of them need brackets.
if (!adc->has_vref)
return dev_err_probe(..);
adc->vref = NULL;
dev_dbg();
} else {
...
> + /* Check if chip has internal vref */
> + if (!adc->has_vref)
> + return dev_err_probe(dev, PTR_ERR(adc->vref),
> + "Unknown Vref\n");
> + adc->vref = NULL;
> + dev_dbg(dev, "%s: Using internal Vref\n", __func__);
> + } else {
> + return dev_err_probe(dev, PTR_ERR(adc->vref),
> + "failed to get regulator\n");
> + }
> + } else {
> + ret = regulator_enable(adc->vref);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(dev, mcp3564_disable_reg,
> + adc->vref);
> + if (ret)
> + return ret;
> +
> + dev_dbg(dev, "%s: Using External Vref\n", __func__);
> +
> + ret = regulator_get_voltage(adc->vref);
> + if (ret < 0) {
> + return dev_err_probe(dev, ret,
> + "Failed to read vref regulator\n");
Brackets not needed. Consistency matters more than whether you do or do not have
brackets as the multiline because of a line break case may not be well covered
by the kernel style docs.
> + }
> +
> + adc->vref_mv = ret / MILLI;
> + }
> +
> + ret = mcp3564_parse_fw_children(indio_dev);
> + if (ret)
> + return ret;
> +
> + /*
> + * Command sequence that ensures a recovery with the desired settings
> + * in any cases of loss-of-power scenario (Full Chip Reset):
> + * - Write LOCK register to 0xA5
> + * - Write IRQ register to 0x03
> + * - Send "Device Full Reset" fast command
> + * - Wait 1ms for "Full Reset" to complete
> + */
> + ret = mcp3564_write_8bits(adc, MCP3564_LOCK_REG, MCP3564_LOCK_WRITE_ACCESS_PASSWORD);
> + if (ret)
> + return ret;
> +
> + ret = mcp3564_write_8bits(adc, MCP3564_IRQ_REG, 0x03);
> + if (ret)
> + return ret;
> +
> + ret = mcp3564_fast_cmd(adc, MCP3564_FASTCMD_RESET);
> + if (ret)
> + return ret;
> +
> + mdelay(1);
Always good to have a doc reference for delays - sometimes they turn out to be a possible
cause of instability - so good to know if this is much larger than the spec requires
or rather close to whatever it says.
> +
> + /* set a gain of 1x for GAINCAL */
> + ret = mcp3564_write_24bits(adc, MCP3564_GAINCAL_REG, MCP3564_DEFAULT_GAINCAL);
> + if (ret)
> + return ret;
> +
> + adc->calib_scale = MCP3564_DEFAULT_GAINCAL;
> +
> + ret = mcp3564_write_24bits(adc, MCP3564_OFFSETCAL_REG, MCP3564_DEFAULT_OFFSETCAL);
> + if (ret)
> + return ret;
> +
> + ret = mcp3564_write_24bits(adc, MCP3564_TIMER_REG, MCP3564_TIMER_DEFAULT_VALUE);
> + if (ret)
> + return ret;
> +
> + ret = mcp3564_write_24bits(adc, MCP3564_SCAN_REG,
> + MCP3564_SCAN_DELAY_TIME_SET(MCP3564_NO_DELAY) |
> + MCP3564_SCAN_CH_SEL_SET(MCP3564_SCAN_DEFAULT_VALUE));
> + if (ret)
> + return ret;
> +
> + ret = mcp3564_write_8bits(adc, MCP3564_MUX_REG, MCP3564_MUX_SET(MCP3564_CH0, MCP3564_CH1));
> + if (ret)
> + return ret;
> +
> + ret = mcp3564_write_8bits(adc, MCP3564_IRQ_REG,
> + FIELD_PREP(MCP3464_EN_FASTCMD_MASK, 1) |
> + FIELD_PREP(MCP3464_EN_STP_MASK, 1));
> + if (ret)
> + return ret;
> +
> + tmp_reg = MCP3564_CONFIG3_CONV_MODE_SET(MCP3464_CONFIG3_CONV_MODE_ONE_SHOT_STANDBY);
> + tmp_reg |= MCP3564_CONFIG3_DATA_FORMAT_SET(MCP3464_CONFIG3_DATA_FMT_32B_SGN_EXT);
> + tmp_reg |= MCP3464_CONFIG3_EN_OFFCAL_MASK;
> + tmp_reg |= MCP3464_CONFIG3_EN_GAINCAL_MASK;
> +
> + ret = mcp3564_write_8bits(adc, MCP3564_CONFIG3_REG, tmp_reg);
> + if (ret)
> + return ret;
> +
> + tmp_reg = MCP3564_CONFIG2_BOOST_CURRENT_SET(MCP3564_BOOST_CURRENT_x1_00);
> + tmp_reg |= MCP3564_CONFIG2_HARDWARE_GAIN_SET(0x01);
> + tmp_reg |= MCP3564_CONFIG2_AZ_MUX_SET(1);
> +
> + ret = mcp3564_write_8bits(adc, MCP3564_CONFIG2_REG, tmp_reg);
> + if (ret)
> + return ret;
> +
> + adc->hwgain = 0x01;
> + adc->auto_zeroing_mux = true;
> + adc->auto_zeroing_ref = false;
> + adc->current_boost_mode = MCP3564_BOOST_CURRENT_x1_00;
> +
> + tmp_reg = MCP3564_CONFIG1_OVERSPL_RATIO_SET(MCP3564_OVERSAMPLING_RATIO_98304);
> +
> + ret = mcp3564_write_8bits(adc, MCP3564_CONFIG1_REG, tmp_reg);
> + if (ret)
> + return ret;
> +
> + adc->oversampling = MCP3564_OVERSAMPLING_RATIO_98304;
> +
> + tmp_reg = MCP3564_CONFIG0_ADC_MODE_MASK_SET(MCP3564_ADC_MODE_STANDBY);
> + tmp_reg |= MCP3564_CONFIG0_CS_SEL_SET(MCP3564_CONFIG0_CS_SEL_0_0_uA);
> + tmp_reg |= MCP3564_CONFIG0_CLK_SEL_SET(MCP3564_CONFIG0_USE_INT_CLK);
> + tmp_reg |= MCP3456_CONFIG0_BIT6_DEFAULT;
> +
> + if (!adc->vref) {
> + tmp_reg |= MCP3564_CONFIG0_VREF_SET(1);
> + adc->vref_mv = MCP3564R_INT_VREF_MV;
> + }
> +
> + ret = mcp3564_write_8bits(adc, MCP3564_CONFIG0_REG, tmp_reg);
> +
> + adc->burnout_mode = MCP3564_CONFIG0_CS_SEL_0_0_uA;
> +
> + return ret;
> +}
> +
> +static IIO_DEVICE_ATTR(auto_zeroing_ref_enable, 0644,
> + mcp3564_auto_zeroing_ref_show,
> + mcp3564_auto_zeroing_ref_store, 0);
> +
> +static IIO_DEVICE_ATTR(auto_zeroing_mux_enable, 0644,
> + mcp3564_auto_zeroing_mux_show,
> + mcp3564_auto_zeroing_mux_store, 0);
> +
> +//#define MCP3564_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
This wants deleting...
Powered by blists - more mailing lists