| 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
| ||
|
Date: Fri, 24 May 2024 12:56:38 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Ramona Gradinariu <ramona.bolboaca13@...il.com>,
linux-kernel@...r.kernel.org, jic23@...nel.org, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, conor+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, robh@...nel.org, nuno.sa@...log.com
Subject: Re: [PATCH v4 10/10] drivers: iio: imu: Add support for adis1657x
family
On Fri, 2024-05-24 at 12:03 +0300, Ramona Gradinariu wrote:
> Add support for ADIS1657X family devices in already exiting ADIS16475
> driver.
>
> Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@...il.com>
> ---
> changes in v4:
> - removed AIDS16575_HAS_FIFO flag and instead used has_fifo flag from adis_data
> - removed timestamp channel for devices which support FIFO readings (adis1657x)
> - dropped the dev_attr.attr. from adis16475_fifo_attributes
> - reworked if (ret) as advised
> drivers/iio/imu/adis16475.c | 649 ++++++++++++++++++++++++++++++++----
> 1 file changed, 579 insertions(+), 70 deletions(-)
>
> diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
> index 3bbf3e181e1a..f0eed75c4fb2 100644
> --- a/drivers/iio/imu/adis16475.c
> +++ b/drivers/iio/imu/adis16475.c
> @@ -14,6 +14,7 @@
> #include <linux/iio/buffer.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/imu/adis.h>
> +#include <linux/iio/sysfs.h>
> #include <linux/iio/trigger_consumer.h>
> #include <linux/irq.h>
> #include <linux/lcm.h>
..
> @@ -1264,20 +1582,30 @@ static int adis16475_push_single_sample(struct
> iio_poll_func *pf)
> __be16 *buffer;
> u16 crc;
> bool valid;
> + u8 crc_offset = 9;
> + u16 burst_size = ADIS16475_BURST_MAX_DATA;
> + u16 start_idx = (st->info->flags & ADIS16475_HAS_TIMESTAMP32) ? 2 : 0;
> +
> /* offset until the first element after gyro and accel */
> const u8 offset = st->burst32 ? 13 : 7;
>
> + if (st->burst32) {
> + crc_offset = (st->info->flags & ADIS16475_HAS_TIMESTAMP32) ? 16 :
> 15;
> + burst_size = (st->info->flags & ADIS16475_HAS_TIMESTAMP32) ?
> + ADIS16575_BURST32_DATA_TS32 :
> ADIS16475_BURST32_MAX_DATA_NO_TS32;
can we use the info in the adis_data struct rather than the conditional?
> + }
> +
> ret = spi_sync(adis->spi, &adis->msg);
> if (ret)
> - goto check_burst32;
> + return ret;
>
..
>
> +static irqreturn_t adis16475_trigger_handler_with_fifo(int irq, void *p)
> {
> struct iio_poll_func *pf = p;
> struct iio_dev *indio_dev = pf->indio_dev;
> + struct adis16475 *st = iio_priv(indio_dev);
> + struct adis *adis = &st->adis;
> + int ret;
> + u16 fifo_cnt, i;
>
> - adis16475_push_single_sample(pf);
> + adis_dev_lock(&st->adis);
> +
> + ret = __adis_read_reg_16(adis, ADIS16575_REG_FIFO_CNT, &fifo_cnt);
> + if (ret)
> + goto unlock;
> +
> + /*
> + * If no sample is available, nothing can be read. This can happen if
> + * a the used trigger has a higher frequency than the selected sample
> rate.
> + */
> + if (!fifo_cnt)
> + goto unlock;
> +
> + /*
> + * First burst request - FIFO pop: popped data will be returned in the
> + * next burst request.
> + */
> + ret = adis16575_custom_burst_read(pf, adis->data->burst_reg_cmd);
> + if (ret)
> + goto unlock;
> +
> + for (i = 0; i < fifo_cnt - 1; i++) {
> + ret = adis16475_push_single_sample(pf);
> + if (ret)
> + goto unlock;
> + }
> +
> + /* FIFO read without popping */
> + ret = adis16575_custom_burst_read(pf, 0);
> + if (ret)
> + goto unlock;
> +
This jump is useless :). Either move the label before adis_dev_unlock() or ignore the
error code completely. It's the question of we should still do
adis16475_burst32_check() in case adis16575_custom_burst_read() fails. Likely not...
> +unlock:
> + /*
> + * We only check the burst mode at the end of the current capture since
> + * reading data from registers will impact the FIFO reading.
> + */
> + adis16475_burst32_check(st);
> + adis_dev_unlock(&st->adis);
> iio_trigger_notify_done(indio_dev->trig);
>
> return IRQ_HANDLED;
> @@ -1367,6 +1799,14 @@ static int adis16475_config_sync_mode(struct adis16475 *st)
> u32 sync_mode;
> u16 max_sample_rate = st->info->int_clk + 100;
>
> + /* if available, enable 4khz internal clock */
> + if (st->info->int_clk == 4000) {
> + ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> + ADIS16575_SYNC_4KHZ_MASK,
> (u16)ADIS16575_SYNC_4KHZ(1));
> + if (ret)
> + return ret;
> + }
> +
> /* default to internal clk */
> st->clk_freq = st->info->int_clk * 1000;
>
> @@ -1444,34 +1884,67 @@ static int adis16475_config_irq_pin(struct adis16475 *st)
> u8 polarity;
> struct spi_device *spi = st->adis.spi;
>
> - /*
> - * It is possible to configure the data ready polarity. Furthermore, we
> - * need to update the adis struct if we want data ready as active low.
> - */
> irq_type = irq_get_trigger_type(spi->irq);
> - if (irq_type == IRQ_TYPE_EDGE_RISING) {
> - polarity = 1;
> - st->adis.irq_flag = IRQF_TRIGGER_RISING;
> - } else if (irq_type == IRQ_TYPE_EDGE_FALLING) {
> - polarity = 0;
> - st->adis.irq_flag = IRQF_TRIGGER_FALLING;
> +
> + if (st->adis.data->has_fifo) {
> + /*
> + * It is possible to configure the fifo watermark pin polarity.
> + * Furthermore, we need to update the adis struct if we want the
> + * watermark pin active low.
> + */
> + if (irq_type == IRQ_TYPE_LEVEL_HIGH) {
> + polarity = 1;
> + st->adis.irq_flag = IRQF_TRIGGER_HIGH;
> + } else if (irq_type == IRQ_TYPE_LEVEL_LOW) {
> + polarity = 0;
> + st->adis.irq_flag = IRQF_TRIGGER_LOW;
> + } else {
> + dev_err(&spi->dev, "Invalid interrupt type 0x%x
> specified\n",
> + irq_type);
> + return -EINVAL;
> + }
> +
> + /* Configure the watermark pin polarity. */
> + ret = adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL,
> + ADIS16575_WM_POL_MASK,
> (u16)ADIS16575_WM_POL(polarity));
Maybe in the if() statements, do polarity = ADIS16575_WM_POL(0|1) and here use the
variable. Then, no need for the annoying cast.
> + if (ret)
> + return ret;
> +
> + /* Enable watermark interrupt pin. */
> + ret = adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL,
> + ADIS16575_WM_EN_MASK,
> (u16)ADIS16575_WM_EN(1));
> + if (ret)
> + return ret;
> +
> } else {
> - dev_err(&spi->dev, "Invalid interrupt type 0x%x specified\n",
> - irq_type);
> - return -EINVAL;
> - }
> + /*
> + * It is possible to configure the data ready polarity.
> Furthermore, we
> + * need to update the adis struct if we want data ready as active
> low.
> + */
> + if (irq_type == IRQ_TYPE_EDGE_RISING) {
> + polarity = 1;
> + st->adis.irq_flag = IRQF_TRIGGER_RISING;
> + } else if (irq_type == IRQ_TYPE_EDGE_FALLING) {
> + polarity = 0;
> + st->adis.irq_flag = IRQF_TRIGGER_FALLING;
> + } else {
> + dev_err(&spi->dev, "Invalid interrupt type 0x%x
> specified\n",
> + irq_type);
> + return -EINVAL;
> + }
>
> - val = ADIS16475_MSG_CTRL_DR_POL(polarity);
> - ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> - ADIS16475_MSG_CTRL_DR_POL_MASK, val);
> - if (ret)
> - return ret;
> - /*
> - * There is a delay writing to any bits written to the MSC_CTRL
> - * register. It should not be bigger than 200us, so 250 should be more
> - * than enough!
> - */
> - usleep_range(250, 260);
> + val = ADIS16475_MSG_CTRL_DR_POL(polarity);
> + ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> + ADIS16475_MSG_CTRL_DR_POL_MASK, val);
> + if (ret)
> + return ret;
> + /*
> + * There is a delay writing to any bits written to the MSC_CTRL
> + * register. It should not be bigger than 200us, so 250 should be
> more
> + * than enough!
> + */
> + usleep_range(250, 260);
> + }
>
> return 0;
> }
> @@ -1500,7 +1973,10 @@ static int adis16475_probe(struct spi_device *spi)
> indio_dev->name = st->info->name;
> indio_dev->channels = st->info->channels;
> indio_dev->num_channels = st->info->num_channels;
> - indio_dev->info = &adis16475_info;
> + if (st->adis.data->has_fifo)
> + indio_dev->info = &adis16575_info;
> + else
> + indio_dev->info = &adis16475_info;
> indio_dev->modes = INDIO_DIRECT_MODE;
>
> ret = __adis_initial_startup(&st->adis);
> @@ -1515,10 +1991,25 @@ static int adis16475_probe(struct spi_device *spi)
> if (ret)
> return ret;
>
> - ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev,
> - adis16475_trigger_handler);
> - if (ret)
> - return ret;
> + if (st->adis.data->has_fifo) {
> + ret = devm_adis_setup_buffer_and_trigger_with_attrs(&st->adis,
> indio_dev,
> +
> adis16475_trigger_handler_with_fifo,
> +
> &adis16475_buffer_ops,
> +
> adis16475_fifo_attributes);
> + if (ret)
> + return ret;
> +
> + /* Update overflow behavior to always overwrite the oldest sample.
> */
> + ret = adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL,
> + ADIS16575_OVERFLOW_MASK,
> (u16)ADIS16575_OVERWRITE_OLDEST);
Slight preference for local variable to avoid the cast.
- Nuno Sá
Powered by blists - more mailing lists