| 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: <ZHG8e5qivS5rGe1H@smile.fi.intel.com>
Date: Sat, 27 May 2023 11:16:59 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Mike Looijmans <mike.looijmans@...ic.nl>
Cc: devicetree@...r.kernel.org, linux-iio@...r.kernel.org,
Arnd Bergmann <arnd@...db.de>,
Caleb Connolly <caleb.connolly@...aro.org>,
ChiYuan Huang <cy_huang@...htek.com>,
Cosmin Tanislav <demonsingur@...il.com>,
Haibo Chen <haibo.chen@....com>,
Hugo Villeneuve <hvilleneuve@...onoff.com>,
Ibrahim Tilki <Ibrahim.Tilki@...log.com>,
Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Leonard Göhrs <l.goehrs@...gutronix.de>,
Liam Girdwood <lgirdwood@...il.com>,
Marcus Folkesson <marcus.folkesson@...il.com>,
Mark Brown <broonie@...nel.org>,
Ramona Bolboaca <ramona.bolboaca@...log.com>,
William Breathitt Gray <william.gray@...aro.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] iio: adc: Add microchip MCP3561/2/4R devices
On Tue, May 23, 2023 at 02:43:54PM +0200, Mike Looijmans wrote:
> The MCP3564R is a 24-bit ADC with 8 multiplexed inputs. The MCP3561R is
> the same device with 2 inputs, the MCP3562R has 4 inputs. The device
> contains one ADC and a multiplexer to select the inputs to the ADC.
> To facilitate buffered reading, only channels that can be continuously
> sampled are exported to the IIO subsystem. The driver does not support
> buffered reading yet.
...
> +#include <asm/unaligned.h>
Move this after linux/* in a separate group.
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
Avoid OF-centric calls in the new IIO drivers. Or maybe you simply used the
wrong header?
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
...
> +#define MCP396XR_CONFIG2_DEFAULT \
> + (FIELD_PREP(MCP396XR_CONFIG2_BOOST, 0x2) | \
BIT()?
> + FIELD_PREP(MCP396XR_CONFIG2_GAIN, 0x1) | \
Ditto.
> + MCP396XR_CONFIG2_RESERVED1)
...
> +#define MCP396XR_CONFIG3_DEFAULT \
> + (FIELD_PREP(MCP396XR_CONFIG3_CONV_MODE, 0x1) | \
> + FIELD_PREP(MCP396XR_CONFIG3_DATA_FORMAT, 0x3))
Ditto / GENMASK()?
...
> +#define MCP396XR_INT_VREF_UV 2400000
I would go with _uV, but I don't remember what is the practice currently.
...
> +struct mcp356xr {
> + struct spi_device *spi;
> + struct mutex lock;
Locks must be commented.
> + struct regulator *vref;
> + struct clk *clki;
> + struct completion sample_available;
> + u8 dev_addr;
> + u8 n_inputs;
> + u8 config[4];
> + int scale_avail[8 * 2]; /* 8 gain settings */
Shouldn't it be double array [8][2] ?
Also I would move it before u8 members. In some cases might save a few bytes
(although seems not the case here right now).
> + /* SPI transfer buffer */
> + u8 buf[1 + MCP396XR_MAX_TRANSFER_SIZE] ____cacheline_aligned;
> +};
> +static int mcp356xr_read(struct mcp356xr *adc, u8 reg, u8 len)
> +{
> + int ret;
> + struct spi_transfer xfer = {
> + .tx_buf = adc->buf,
> + .rx_buf = adc->buf,
> + .len = len + 1,
> + };
> +
> + adc->buf[0] = FIELD_PREP(MCP396XR_CMD_MASK_DEV_ADDR, adc->dev_addr) |
> + FIELD_PREP(MCP396XR_CMD_MASK_REG_ADDR, reg) |
> + MCP396XR_CMD_TYPE_READ_SEQ;
> + memset(adc->buf + 1, 0, len);
I would rather move it at the top and take whole buffer.
u16 length = len + 1;
...
.len = length,
...
memset(...->buf, 0, length);
> + ret = spi_sync_transfer(adc->spi, &xfer, 1);
> + if (ret < 0)
> + return ret;
> +
> + return ret;
return 0;
?
But why you check for negative? May spi_sync_transfer() return positive?
Hence
return spi_sync_transfer() would suffice.
> +}
...
> +static int mcp356xr_write(struct mcp356xr *adc, u8 reg, void *val, u8 len)
> +{
Same comments as per above function.
> +}
...
> +static int mcp356xr_get_oversampling_rate(struct mcp356xr *adc)
> +{
> + return mcp356xr_oversampling_rates[FIELD_GET(MCP396XR_CONFIG1_OSR,
> + adc->config[1])];
With a temporary variable for index it will look better.
> +}
> + /* The scale is always below 1 */
> + if (val)
> + return -EINVAL;
> +
> + if (!val2)
> + return -EINVAL;
Both can be done in one "if".
...
> + /*
> + * val2 is in 'micro' units, n = val2 / 1000000
> + * the full-scale value is millivolts / n, corresponds to 2^23,
> + * hence the gain = ((val2 / 1000000) << 23) / millivolts
> + * Approximate ((val2 / 1000000) << 23) as (549755 * val2) >> 16
> + * because 2 << (23 + 16) / 1000000 = 549755
You can use definitions from units.h. They made to be readable in the code and
in the comments.
> + */
...
> +static long mcp356xr_get_amclk_freq(struct mcp356xr *adc)
> +{
> + long result;
> +
> + if (adc->clki) {
> + result = clk_get_rate(adc->clki);
> + if (result > 0) {
if (result == 0)
return 0;
> + result >>= FIELD_GET(MCP396XR_CONFIG1_AMCLK_PRE,
> + adc->config[1]);
return result >> ...;
> + }
> + } else {
> + result = MCP396XR_INTERNAL_CLOCK_FREQ;
> + }
> +
> + return result;
return MCP...;
> +}
...
> +static int mcp356xr_adc_conversion(struct mcp356xr *adc,
> + struct iio_chan_spec const *channel,
> + int *val)
> +{
> + long freq = mcp356xr_get_amclk_freq(adc);
> + int osr = mcp356xr_get_oversampling_rate(adc);
> + /* Over-estimate timeout by a factor 2 */
> + int timeout_ms = DIV_ROUND_UP((osr << 2) * 2 * 1000, freq);
MILLI ?
> + int ret;
> +
> + /* Setup input mux (address field is the mux setting) */
> + ret = mcp356xr_write_u8(adc, MCP396XR_REG_MUX, channel->address);
> + if (ret)
> + return ret;
> +
> + reinit_completion(&adc->sample_available);
> + /* Start conversion */
> + ret = mcp356xr_fast_command(adc, MCP396XR_FASTCMD_START);
> + if (ret)
> + return ret;
> +
> + if (timeout_ms < 10)
> + timeout_ms = 10;
timeout_ms = max(DIV_ROUND_UP(...), 10);
?
> + ret = wait_for_completion_interruptible_timeout(
> + &adc->sample_available, msecs_to_jiffies(timeout_ms));
Strange indentation.
> + if (ret == 0) {
> + /* Interrupt did not fire, check status and report */
> + dev_warn(&adc->spi->dev, "Timeout (%d ms)\n", timeout_ms);
> + ret = mcp356xr_read(adc, MCP396XR_REG_ADCDATA, 4);
> + if (!ret) {
> + /* Check if data-ready was asserted */
> + if ((adc->buf[0] & MCP396XR_STATUS_DR))
> + return -ETIMEDOUT;
> + }
> + }
> +
> + if (ret < 0)
> + return ret;
> +
> + /*
> + * We're using data format 0b11 (see datasheet). While the ADC output is
> + * 24-bit, it allows over-ranging it and produces a 25-bit output in
> + * this mode. Hence the "24".
> + */
> + *val = sign_extend32(get_unaligned_be32(&adc->buf[1]), 24);
> +
> + return 0;
> +}
...
> + int ret = -EINVAL;
Seems missing default.
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + *vals = mcp356xr_oversampling_rates;
> + *type = IIO_VAL_INT;
> + *length = ARRAY_SIZE(mcp356xr_oversampling_rates);
> + ret = IIO_AVAIL_LIST;
> + break;
return ...;
> + case IIO_CHAN_INFO_SCALE:
> + *type = IIO_VAL_FRACTIONAL_LOG2;
> + *vals = adc->scale_avail;
> + *length = ARRAY_SIZE(adc->scale_avail);
> + ret = IIO_AVAIL_LIST;
> + break;
Ditto.
> + }
> +
> + return ret;
default?
...
> +static int mcp356xr_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel, int *val,
> + int *val2, long mask)
> +{
> + struct mcp356xr *adc = iio_priv(indio_dev);
> + int ret = -EINVAL;
Use default.
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + break;
> +
> + ret = mcp356xr_adc_conversion(adc, channel, val);
> + if (ret >= 0)
Can we use traditional pattern?
if (ret < 0)
...handle error...
> + ret = IIO_VAL_INT;
> + iio_device_release_direct_mode(indio_dev);
> + break;
> + case IIO_CHAN_INFO_SCALE:
> + ret = FIELD_GET(MCP396XR_CONFIG2_GAIN, adc->config[2]);
> + *val = adc->scale_avail[ret * 2 + 0];
> + *val2 = adc->scale_avail[ret * 2 + 1];
> + ret = IIO_VAL_FRACTIONAL_LOG2;
> + if (channel->type == IIO_TEMP) {
> + /* To obtain temperature scale, divide by 0.0002973 */
> + *val = 100 * ((*val * 100000) / 2973);
> + }
> + break;
> + case IIO_CHAN_INFO_OFFSET:
> + if (channel->type == IIO_TEMP) {
> + ret = FIELD_GET(MCP396XR_CONFIG2_GAIN, adc->config[2]);
> + /* temperature has 80 mV offset */
> + *val = (-80 << adc->scale_avail[ret * 2 + 1]) /
> + adc->scale_avail[ret * 2 + 0];
> + ret = IIO_VAL_INT;
> + }
> + break;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *val = mcp356xr_get_samp_freq(adc);
> + ret = IIO_VAL_INT;
> + break;
> + case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> + *val = mcp356xr_get_oversampling_rate(adc);
> + ret = IIO_VAL_INT;
> + break;
> + }
> +
> + return ret;
Return directly from each case.
> +}
...
> +static int mcp356xr_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel, int val,
> + int val2, long mask)
> +{
As per previous comments.
> +}
...
> + ret = mcp356xr_read(adc, MCP396XR_REG_ADCDATA, 4);
> + if (!ret) {
Please, use traditional pattern
> + /* Check if data-ready bit is 0 (active) */
> + if (!(adc->buf[0] & MCP396XR_STATUS_DR))
> + complete(&adc->sample_available);
> + }
...
> + if (!of_property_read_u32(of_node, "device-addr", &value)) {
No of_*() in a new IIO drivers, please.
> + if (value > 3) {
> + dev_err(&adc->spi->dev,
> + "invalid device address (%u). Must be <3.\n",
> + value);
> + return -EINVAL;
> + }
Validation should be done on YAML level, no?
> + adc->dev_addr = value;
> + } else {
> + /* Default address is "1" unless you special-order them */
> + adc->dev_addr = 0x1;
Why hex?
> + }
...
Missing comment to explain the below sleep.
> + usleep_range(200, 400);
...
> + if (!of_property_read_bool(of_node, "drive-open-drain"))
No of_*().
> + regval |= MCP396XR_IRQ_PUSH_PULL;
> + ret = mcp356xr_write_u8(adc, MCP396XR_REG_IRQ, regval);
> + if (ret)
> + return ret;
> +
> + return 0;
return ncp356xr_...;
> + adc->clki = devm_clk_get(dev, NULL);
> + if (IS_ERR(adc->clki)) {
> + if (PTR_ERR(adc->clki) == -ENOENT) {
> + adc->clki = NULL;
We have _optional()
> + } else {
> + return dev_err_probe(dev, PTR_ERR(adc->clki),
> + "Failed to get adc clk\n");
> + }
> + } else {
> + ret = clk_prepare_enable(adc->clki);
We have _enabled()
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "Failed to enable adc clk\n");
> +
> + ret = devm_add_action_or_reset(dev, mcp356xr_clk_disable,
> + adc->clki);
> + if (ret)
> + return ret;
> + }
Just call devm_clk_get_optional_enabled()
...
> + ret = devm_iio_device_register(dev, indio_dev);
> + return ret;
return devm_...;
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists