* Re: [PATCH v2 09/12] iio: imu: inv_icm42600: add buffer support in iio devices
From: Jonathan Cameron @ 2020-05-31 12:56 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol
Cc: robh+dt, robh, mchehab+huawei, davem, gregkh, linux-iio,
devicetree, linux-kernel
In-Reply-To: <20200527185711.21331-10-jmaneyrol@invensense.com>
On Wed, 27 May 2020 20:57:08 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
> Add all FIFO parsing and reading functions. Add accel and gyro
> kfifo buffer and FIFO data parsing. Use device interrupt for
> reading data FIFO and launching accel and gyro parsing.
>
> Support hwfifo watermark by multiplexing gyro and accel settings.
> Support hwfifo flush.
Both of these are complex given the interactions of the two sensors
types and to be honest I couldn't figure out exactly what the intent was.
Needs more docs!
Thanks,
Jonathan
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> ---
> drivers/iio/imu/inv_icm42600/Kconfig | 1 +
> drivers/iio/imu/inv_icm42600/Makefile | 1 +
> drivers/iio/imu/inv_icm42600/inv_icm42600.h | 8 +
> .../iio/imu/inv_icm42600/inv_icm42600_accel.c | 160 ++++-
> .../imu/inv_icm42600/inv_icm42600_buffer.c | 555 ++++++++++++++++++
> .../imu/inv_icm42600/inv_icm42600_buffer.h | 98 ++++
> .../iio/imu/inv_icm42600/inv_icm42600_core.c | 30 +
> .../iio/imu/inv_icm42600/inv_icm42600_gyro.c | 160 ++++-
> 8 files changed, 1011 insertions(+), 2 deletions(-)
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.h
>
> diff --git a/drivers/iio/imu/inv_icm42600/Kconfig b/drivers/iio/imu/inv_icm42600/Kconfig
> index 22390a72f0a3..50cbcfcb6cf1 100644
> --- a/drivers/iio/imu/inv_icm42600/Kconfig
> +++ b/drivers/iio/imu/inv_icm42600/Kconfig
> @@ -2,6 +2,7 @@
>
> config INV_ICM42600
> tristate
> + select IIO_BUFFER
>
> config INV_ICM42600_I2C
> tristate "InvenSense ICM-426xx I2C driver"
> diff --git a/drivers/iio/imu/inv_icm42600/Makefile b/drivers/iio/imu/inv_icm42600/Makefile
> index 48965824f00c..0f49f6df3647 100644
> --- a/drivers/iio/imu/inv_icm42600/Makefile
> +++ b/drivers/iio/imu/inv_icm42600/Makefile
> @@ -5,6 +5,7 @@ inv-icm42600-y += inv_icm42600_core.o
> inv-icm42600-y += inv_icm42600_gyro.o
> inv-icm42600-y += inv_icm42600_accel.o
> inv-icm42600-y += inv_icm42600_temp.o
> +inv-icm42600-y += inv_icm42600_buffer.o
>
> obj-$(CONFIG_INV_ICM42600_I2C) += inv-icm42600-i2c.o
> inv-icm42600-i2c-y += inv_icm42600_i2c.o
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 43749f56426c..4d5811562a61 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -14,6 +14,8 @@
> #include <linux/pm.h>
> #include <linux/iio/iio.h>
>
> +#include "inv_icm42600_buffer.h"
> +
> enum inv_icm42600_chip {
> INV_CHIP_ICM42600,
> INV_CHIP_ICM42602,
> @@ -123,6 +125,7 @@ struct inv_icm42600_suspended {
> * @indio_gyro: gyroscope IIO device.
> * @indio_accel: accelerometer IIO device.
> * @buffer: data transfer buffer aligned for DMA.
> + * @fifo: FIFO management structure.
> */
> struct inv_icm42600_state {
> struct mutex lock;
> @@ -137,6 +140,7 @@ struct inv_icm42600_state {
> struct iio_dev *indio_gyro;
> struct iio_dev *indio_accel;
> uint8_t buffer[2] ____cacheline_aligned;
> + struct inv_icm42600_fifo fifo;
> };
>
> /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
> @@ -377,6 +381,10 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
>
> int inv_icm42600_gyro_init(struct inv_icm42600_state *st);
>
> +int inv_icm42600_gyro_parse_fifo(struct iio_dev *indio_dev);
> +
> int inv_icm42600_accel_init(struct inv_icm42600_state *st);
>
> +int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev);
> +
> #endif
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index 6a615d7ffb24..c73ce204efc6 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> @@ -11,9 +11,12 @@
> #include <linux/delay.h>
> #include <linux/math64.h>
> #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
>
> #include "inv_icm42600.h"
> #include "inv_icm42600_temp.h"
> +#include "inv_icm42600_buffer.h"
>
> #define INV_ICM42600_ACCEL_CHAN(_modifier, _index, _ext_info) \
> { \
> @@ -64,6 +67,76 @@ static const struct iio_chan_spec inv_icm42600_accel_channels[] = {
> INV_ICM42600_TEMP_CHAN(INV_ICM42600_ACCEL_SCAN_TEMP),
> };
>
> +/* IIO buffer data: 8 bytes */
> +struct inv_icm42600_accel_buffer {
> + struct inv_icm42600_fifo_sensor_data accel;
> + int8_t temp;
> + uint8_t padding;
> +};
> +
> +#define INV_ICM42600_SCAN_MASK_ACCEL_3AXIS \
> + (BIT(INV_ICM42600_ACCEL_SCAN_X) | \
> + BIT(INV_ICM42600_ACCEL_SCAN_Y) | \
> + BIT(INV_ICM42600_ACCEL_SCAN_Z))
> +
> +#define INV_ICM42600_SCAN_MASK_TEMP BIT(INV_ICM42600_ACCEL_SCAN_TEMP)
> +
> +static const unsigned long inv_icm42600_accel_scan_masks[] = {
> + /* 3-axis accel + temperature */
> + INV_ICM42600_SCAN_MASK_ACCEL_3AXIS | INV_ICM42600_SCAN_MASK_TEMP,
> + 0,
> +};
> +
> +/* enable accelerometer sensor and FIFO write */
> +static int inv_icm42600_accel_update_scan_mode(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> + unsigned int fifo_en = 0;
> + unsigned int sleep_temp = 0;
> + unsigned int sleep_accel = 0;
> + unsigned int sleep;
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + if (*scan_mask & INV_ICM42600_SCAN_MASK_TEMP) {
> + /* enable temp sensor */
> + ret = inv_icm42600_set_temp_conf(st, true, &sleep_temp);
> + if (ret)
> + goto out_unlock;
> + fifo_en |= INV_ICM42600_SENSOR_TEMP;
> + }
> +
> + if (*scan_mask & INV_ICM42600_SCAN_MASK_ACCEL_3AXIS) {
> + /* enable accel sensor */
> + conf.mode = INV_ICM42600_SENSOR_MODE_LOW_NOISE;
> + ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_accel);
> + if (ret)
> + goto out_unlock;
> + fifo_en |= INV_ICM42600_SENSOR_ACCEL;
> + }
> +
> + /* update data FIFO write */
> + ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
> + if (ret)
> + goto out_unlock;
> +
> + ret = inv_icm42600_buffer_update_watermark(st);
> +
> +out_unlock:
> + mutex_unlock(&st->lock);
> + /* sleep maximum required time */
> + if (sleep_accel > sleep_temp)
> + sleep = sleep_accel;
> + else
> + sleep = sleep_temp;
> + if (sleep)
> + msleep(sleep);
> + return ret;
> +}
> +
> static int inv_icm42600_accel_read_sensor(struct inv_icm42600_state *st,
> struct iio_chan_spec const *chan,
> int16_t *val)
> @@ -248,7 +321,12 @@ static int inv_icm42600_accel_write_odr(struct inv_icm42600_state *st,
> mutex_lock(&st->lock);
>
> ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
> + if (ret)
> + goto out_unlock;
> + inv_icm42600_buffer_update_fifo_period(st);
> + inv_icm42600_buffer_update_watermark(st);
>
> +out_unlock:
> mutex_unlock(&st->lock);
> pm_runtime_mark_last_busy(dev);
> pm_runtime_put_autosuspend(dev);
> @@ -563,12 +641,51 @@ static int inv_icm42600_accel_write_raw_get_fmt(struct iio_dev *indio_dev,
> }
> }
>
> +static int inv_icm42600_accel_hwfifo_set_watermark(struct iio_dev *indio_dev,
> + unsigned int val)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + st->fifo.watermark.accel = val;
> + ret = inv_icm42600_buffer_update_watermark(st);
> +
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> +static int inv_icm42600_accel_hwfifo_flush(struct iio_dev *indio_dev,
> + unsigned int count)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + int ret;
> +
> + if (count == 0)
> + return 0;
> +
> + mutex_lock(&st->lock);
> +
> + ret = inv_icm42600_buffer_hwfifo_flush(st, count);
> + if (!ret)
> + ret = st->fifo.nb.accel;
> +
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> static const struct iio_info inv_icm42600_accel_info = {
> .read_raw = inv_icm42600_accel_read_raw,
> .read_avail = inv_icm42600_accel_read_avail,
> .write_raw = inv_icm42600_accel_write_raw,
> .write_raw_get_fmt = inv_icm42600_accel_write_raw_get_fmt,
> .debugfs_reg_access = inv_icm42600_debugfs_reg,
> + .update_scan_mode = inv_icm42600_accel_update_scan_mode,
> + .hwfifo_set_watermark = inv_icm42600_accel_hwfifo_set_watermark,
> + .hwfifo_flush_to_buffer = inv_icm42600_accel_hwfifo_flush,
> };
>
> int inv_icm42600_accel_init(struct inv_icm42600_state *st)
> @@ -576,6 +693,7 @@ int inv_icm42600_accel_init(struct inv_icm42600_state *st)
> struct device *dev = regmap_get_device(st->map);
> const char *name;
> struct iio_dev *indio_dev;
> + struct iio_buffer *buffer;
>
> name = devm_kasprintf(dev, GFP_KERNEL, "%s-accel", st->name);
> if (!name)
> @@ -585,14 +703,54 @@ int inv_icm42600_accel_init(struct inv_icm42600_state *st)
> if (!indio_dev)
> return -ENOMEM;
>
> + buffer = devm_iio_kfifo_allocate(dev);
> + if (!buffer)
> + return -ENOMEM;
> +
> iio_device_set_drvdata(indio_dev, st);
> indio_dev->dev.parent = dev;
> indio_dev->name = name;
> indio_dev->info = &inv_icm42600_accel_info;
> - indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> indio_dev->channels = inv_icm42600_accel_channels;
> indio_dev->num_channels = ARRAY_SIZE(inv_icm42600_accel_channels);
> + indio_dev->available_scan_masks = inv_icm42600_accel_scan_masks;
> + indio_dev->setup_ops = &inv_icm42600_buffer_ops;
> +
> + iio_device_attach_buffer(indio_dev, buffer);
>
> st->indio_accel = indio_dev;
> return devm_iio_device_register(dev, st->indio_accel);
> }
> +
> +int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + ssize_t i, size;
> + const void *accel, *gyro, *timestamp;
> + const int8_t *temp;
> + unsigned int odr;
> + struct inv_icm42600_accel_buffer buffer = {
> + .padding = 0,
> + };
> +
> + /* parse all fifo packets */
> + for (i = 0; i < st->fifo.count; i += size) {
> + size = inv_icm42600_fifo_decode_packet(&st->fifo.data[i],
> + &accel, &gyro, &temp, ×tamp, &odr);
> + /* quit if error or FIFO is empty */
> + if (size <= 0)
> + return size;
> +
> + /* skip packet if no accel data or data is invalid */
> + if (accel == NULL || !inv_icm42600_fifo_is_data_valid(accel))
> + continue;
> +
> + /* fill and push data buffer */
> + memcpy(&buffer.accel, accel, sizeof(buffer.accel));
> + buffer.temp = temp ? *temp : 0;
> + iio_push_to_buffers(indio_dev, &buffer);
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> new file mode 100644
> index 000000000000..c91075f62231
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> @@ -0,0 +1,555 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Invensense, Inc.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/delay.h>
> +#include <linux/math64.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +
> +#include "inv_icm42600.h"
> +#include "inv_icm42600_buffer.h"
> +
> +/* FIFO header: 1 byte */
> +#define INV_ICM42600_FIFO_HEADER_MSG BIT(7)
> +#define INV_ICM42600_FIFO_HEADER_ACCEL BIT(6)
> +#define INV_ICM42600_FIFO_HEADER_GYRO BIT(5)
> +#define INV_ICM42600_FIFO_HEADER_TMST_FSYNC GENMASK(3, 2)
> +#define INV_ICM42600_FIFO_HEADER_ODR_ACCEL BIT(1)
> +#define INV_ICM42600_FIFO_HEADER_ODR_GYRO BIT(0)
> +
> +struct inv_icm42600_fifo_1sensor_packet {
> + uint8_t header;
> + struct inv_icm42600_fifo_sensor_data data;
> + int8_t temp;
> +} __packed;
> +#define INV_ICM42600_FIFO_1SENSOR_PACKET_SIZE 8
> +
> +struct inv_icm42600_fifo_2sensors_packet {
> + uint8_t header;
> + struct inv_icm42600_fifo_sensor_data accel;
> + struct inv_icm42600_fifo_sensor_data gyro;
> + int8_t temp;
> + __be16 timestamp;
> +} __packed;
> +#define INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE 16
> +
> +ssize_t inv_icm42600_fifo_decode_packet(const void *packet, const void **accel,
> + const void **gyro, const int8_t **temp,
> + const void **timestamp, unsigned int *odr)
> +{
> + const struct inv_icm42600_fifo_1sensor_packet *pack1 = packet;
> + const struct inv_icm42600_fifo_2sensors_packet *pack2 = packet;
> + uint8_t header = *((const uint8_t *)packet);
> +
> + /* FIFO empty */
> + if (header & INV_ICM42600_FIFO_HEADER_MSG) {
> + *accel = NULL;
> + *gyro = NULL;
> + *temp = NULL;
> + *timestamp = NULL;
> + *odr = 0;
> + return 0;
> + }
> +
> + /* handle odr flags */
> + *odr = 0;
> + if (header & INV_ICM42600_FIFO_HEADER_ODR_GYRO)
> + *odr |= INV_ICM42600_SENSOR_GYRO;
> + if (header & INV_ICM42600_FIFO_HEADER_ODR_ACCEL)
> + *odr |= INV_ICM42600_SENSOR_ACCEL;
> +
> + /* accel + gyro */
> + if ((header & INV_ICM42600_FIFO_HEADER_ACCEL) &&
> + (header & INV_ICM42600_FIFO_HEADER_GYRO)) {
> + *accel = &pack2->accel;
> + *gyro = &pack2->gyro;
> + *temp = &pack2->temp;
> + *timestamp = &pack2->timestamp;
> + return INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
> + }
> +
> + /* accel only */
> + if (header & INV_ICM42600_FIFO_HEADER_ACCEL) {
> + *accel = &pack1->data;
> + *gyro = NULL;
> + *temp = &pack1->temp;
> + *timestamp = NULL;
> + return INV_ICM42600_FIFO_1SENSOR_PACKET_SIZE;
> + }
> +
> + /* gyro only */
> + if (header & INV_ICM42600_FIFO_HEADER_GYRO) {
> + *accel = NULL;
> + *gyro = &pack1->data;
> + *temp = &pack1->temp;
> + *timestamp = NULL;
> + return INV_ICM42600_FIFO_1SENSOR_PACKET_SIZE;
> + }
> +
> + /* invalid packet if here */
> + return -EINVAL;
> +}
> +
> +void inv_icm42600_buffer_update_fifo_period(struct inv_icm42600_state *st)
> +{
> + uint32_t period_gyro, period_accel, period;
> +
> + if (st->fifo.en & INV_ICM42600_SENSOR_GYRO)
> + period_gyro = inv_icm42600_odr_to_period(st->conf.gyro.odr);
> + else
> + period_gyro = U32_MAX;
> +
> + if (st->fifo.en & INV_ICM42600_SENSOR_ACCEL)
> + period_accel = inv_icm42600_odr_to_period(st->conf.accel.odr);
> + else
> + period_accel = U32_MAX;
> +
> + if (period_gyro <= period_accel)
> + period = period_gyro;
> + else
> + period = period_accel;
> +
> + st->fifo.period = period;
> +}
> +
> +int inv_icm42600_buffer_set_fifo_en(struct inv_icm42600_state *st,
> + unsigned int fifo_en)
> +{
> + unsigned int mask, val;
> + int ret;
> +
> + /* update only FIFO EN bits */
> + mask = INV_ICM42600_FIFO_CONFIG1_TMST_FSYNC_EN |
> + INV_ICM42600_FIFO_CONFIG1_TEMP_EN |
> + INV_ICM42600_FIFO_CONFIG1_GYRO_EN |
> + INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;
> +
> + val = 0;
> + if (fifo_en & INV_ICM42600_SENSOR_GYRO)
> + val |= INV_ICM42600_FIFO_CONFIG1_GYRO_EN;
> + if (fifo_en & INV_ICM42600_SENSOR_ACCEL)
> + val |= INV_ICM42600_FIFO_CONFIG1_ACCEL_EN;
> + if (fifo_en & INV_ICM42600_SENSOR_TEMP)
> + val |= INV_ICM42600_FIFO_CONFIG1_TEMP_EN;
> +
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_FIFO_CONFIG1,
> + mask, val);
> + if (ret)
> + return ret;
> +
> + st->fifo.en = fifo_en;
> + inv_icm42600_buffer_update_fifo_period(st);
> +
> + return 0;
> +}
> +
> +static size_t inv_icm42600_get_packet_size(unsigned int fifo_en)
> +{
> + size_t packet_size;
> +
> + if ((fifo_en & INV_ICM42600_SENSOR_GYRO) &&
> + (fifo_en & INV_ICM42600_SENSOR_ACCEL))
> + packet_size = INV_ICM42600_FIFO_2SENSORS_PACKET_SIZE;
> + else
> + packet_size = INV_ICM42600_FIFO_1SENSOR_PACKET_SIZE;
> +
> + return packet_size;
> +}
> +
> +static unsigned int inv_icm42600_wm_truncate(unsigned int watermark,
> + size_t packet_size)
> +{
> + size_t wm_size;
> + unsigned int wm;
> +
> + wm_size = watermark * packet_size;
> + if (wm_size > INV_ICM42600_FIFO_WATERMARK_MAX)
> + wm_size = INV_ICM42600_FIFO_WATERMARK_MAX;
> +
> + wm = wm_size / packet_size;
> +
> + return wm;
> +}
> +
I think some overview docs on how this is working would be good.
Set out the aims for the watermark selected and how it is achieved.
> +int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st)
> +{
> + size_t packet_size, wm_size;
> + unsigned int wm_gyro, wm_accel, watermark;
> + uint32_t period_gyro, period_accel, period;
> + int64_t latency_gyro, latency_accel, latency;
> + bool restore;
> + __le16 raw_wm;
> + int ret;
> +
> + packet_size = inv_icm42600_get_packet_size(st->fifo.en);
> +
> + /* get minimal latency, depending on sensor watermark and odr */
> + wm_gyro = inv_icm42600_wm_truncate(st->fifo.watermark.gyro,
> + packet_size);
> + wm_accel = inv_icm42600_wm_truncate(st->fifo.watermark.accel,
> + packet_size);
> + period_gyro = inv_icm42600_odr_to_period(st->conf.gyro.odr);
> + period_accel = inv_icm42600_odr_to_period(st->conf.accel.odr);
> + latency_gyro = (int64_t)period_gyro * (int64_t)wm_gyro;
> + latency_accel = (int64_t)period_accel * (int64_t)wm_accel;
> + if (latency_gyro == 0) {
> + latency = latency_accel;
> + watermark = wm_accel;
> + } else if (latency_accel == 0) {
> + latency = latency_gyro;
> + watermark = wm_gyro;
> + } else {
> + /* compute the smallest latency that is a multiple of both */
> + if (latency_gyro <= latency_accel) {
> + latency = latency_gyro;
> + latency -= latency_accel % latency_gyro;
> + } else {
> + latency = latency_accel;
> + latency -= latency_gyro % latency_accel;
> + }
> + /* use the shortest period */
> + if (period_gyro <= period_accel)
> + period = period_gyro;
> + else
> + period = period_accel;
> + /* all this works because periods are multiple of each others */
> + watermark = div_s64(latency, period);
> + if (watermark < 1)
> + watermark = 1;
> + }
> + wm_size = watermark * packet_size;
> +
> + /* changing FIFO watermark requires to turn off watermark interrupt */
> + ret = regmap_update_bits_check(st->map, INV_ICM42600_REG_INT_SOURCE0,
> + INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN,
> + 0, &restore);
> + if (ret)
> + return ret;
> +
> + raw_wm = INV_ICM42600_FIFO_WATERMARK_VAL(wm_size);
> + memcpy(st->buffer, &raw_wm, sizeof(raw_wm));
> + ret = regmap_bulk_write(st->map, INV_ICM42600_REG_FIFO_WATERMARK,
> + st->buffer, sizeof(raw_wm));
> + if (ret)
> + return ret;
> +
> + /* restore watermark interrupt */
> + if (restore) {
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INT_SOURCE0,
> + INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN,
> + INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int inv_icm42600_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + struct device *dev = regmap_get_device(st->map);
> +
> + pm_runtime_get_sync(dev);
> +
> + return 0;
> +}
> +
> +/*
> + * update_scan_mode callback is turning sensors on and setting data FIFO enable
> + * bits.
> + */
> +static int inv_icm42600_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + /* exit if FIFO is already on */
> + if (st->fifo.on) {
> + ret = 0;
> + goto out_on;
> + }
> +
> + /* set FIFO threshold interrupt */
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INT_SOURCE0,
> + INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN,
> + INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN);
> + if (ret)
> + goto out_unlock;
> +
> + /* flush FIFO data */
> + ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
> + INV_ICM42600_SIGNAL_PATH_RESET_FIFO_FLUSH);
> + if (ret)
> + goto out_unlock;
> +
> + /* set FIFO in streaming mode */
> + ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> + INV_ICM42600_FIFO_CONFIG_STREAM);
> + if (ret)
> + goto out_unlock;
> +
> + /* workaround: first read of FIFO count after reset is always 0 */
> + ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT, st->buffer, 2);
> + if (ret)
> + goto out_unlock;
> +
> +out_on:
> + /* increase FIFO on counter */
> + st->fifo.on++;
> +out_unlock:
> + mutex_unlock(&st->lock);
> + return ret;
> +}
> +
> +static int inv_icm42600_buffer_predisable(struct iio_dev *indio_dev)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + /* exit if there are several sensors using the FIFO */
> + if (st->fifo.on > 1) {
> + ret = 0;
> + goto out_off;
> + }
> +
> + /* set FIFO in bypass mode */
> + ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> + INV_ICM42600_FIFO_CONFIG_BYPASS);
> + if (ret)
> + goto out_unlock;
> +
> + /* flush FIFO data */
> + ret = regmap_write(st->map, INV_ICM42600_REG_SIGNAL_PATH_RESET,
> + INV_ICM42600_SIGNAL_PATH_RESET_FIFO_FLUSH);
> + if (ret)
> + goto out_unlock;
> +
> + /* disable FIFO threshold interrupt */
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INT_SOURCE0,
> + INV_ICM42600_INT_SOURCE0_FIFO_THS_INT1_EN, 0);
> + if (ret)
> + goto out_unlock;
> +
> +out_off:
> + /* decrease FIFO on counter */
> + st->fifo.on--;
> +out_unlock:
> + mutex_unlock(&st->lock);
> + return ret;
> +}
> +
> +static int inv_icm42600_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + struct device *dev = regmap_get_device(st->map);
> + unsigned int sensor;
> + unsigned int *watermark;
> + struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> + unsigned int sleep_temp = 0;
> + unsigned int sleep_sensor = 0;
> + unsigned int sleep;
> + int ret;
> +
> + if (indio_dev == st->indio_gyro) {
> + sensor = INV_ICM42600_SENSOR_GYRO;
> + watermark = &st->fifo.watermark.gyro;
> + } else if (indio_dev == st->indio_accel) {
> + sensor = INV_ICM42600_SENSOR_ACCEL;
> + watermark = &st->fifo.watermark.accel;
> + } else {
> + return -EINVAL;
> + }
> +
> + mutex_lock(&st->lock);
> +
> + ret = inv_icm42600_buffer_set_fifo_en(st, st->fifo.en & ~sensor);
> + if (ret)
> + goto out_unlock;
> +
> + *watermark = 0;
> + ret = inv_icm42600_buffer_update_watermark(st);
> + if (ret)
> + goto out_unlock;
> +
> + conf.mode = INV_ICM42600_SENSOR_MODE_OFF;
> + if (sensor == INV_ICM42600_SENSOR_GYRO)
> + ret = inv_icm42600_set_gyro_conf(st, &conf, &sleep_sensor);
> + else
> + ret = inv_icm42600_set_accel_conf(st, &conf, &sleep_sensor);
> + if (ret)
> + goto out_unlock;
> +
> + /* if FIFO is off, turn temperature off */
> + if (!st->fifo.on)
> + ret = inv_icm42600_set_temp_conf(st, false, &sleep_temp);
> +
> +out_unlock:
> + mutex_unlock(&st->lock);
> +
> + /* sleep maximum required time */
> + if (sleep_sensor > sleep_temp)
> + sleep = sleep_sensor;
> + else
> + sleep = sleep_temp;
> + if (sleep)
> + msleep(sleep);
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +
> + return ret;
> +}
> +
> +const struct iio_buffer_setup_ops inv_icm42600_buffer_ops = {
> + .preenable = inv_icm42600_buffer_preenable,
> + .postenable = inv_icm42600_buffer_postenable,
We've been slowly eroding the difference between preenable and posteenable.
Would be good to understand why you need to define both?
> + .predisable = inv_icm42600_buffer_predisable,
> + .postdisable = inv_icm42600_buffer_postdisable,
> +};
> +
> +int inv_icm42600_buffer_fifo_read(struct inv_icm42600_state *st,
> + unsigned int max)
> +{
> + size_t max_count;
> + __be16 *raw_fifo_count;
> + ssize_t i, size;
> + const void *accel, *gyro, *timestamp;
> + const int8_t *temp;
> + unsigned int odr;
> + int ret;
> +
> + /* reset all samples counters */
> + st->fifo.count = 0;
> + st->fifo.nb.gyro = 0;
> + st->fifo.nb.accel = 0;
> + st->fifo.nb.total = 0;
> +
> + /* compute maximum FIFO read size */
> + if (max == 0)
> + max_count = sizeof(st->fifo.data);
> + else
> + max_count = max * inv_icm42600_get_packet_size(st->fifo.en);
> +
> + /* read FIFO count value */
> + raw_fifo_count = (__be16 *)st->buffer;
> + ret = regmap_bulk_read(st->map, INV_ICM42600_REG_FIFO_COUNT,
> + raw_fifo_count, sizeof(*raw_fifo_count));
> + if (ret)
> + return ret;
> + st->fifo.count = be16_to_cpup(raw_fifo_count);
> +
> + /* check and clamp FIFO count value */
> + if (st->fifo.count == 0)
> + return 0;
> + if (st->fifo.count > max_count)
> + st->fifo.count = max_count;
> +
> + /* read all FIFO data in internal buffer */
> + ret = regmap_noinc_read(st->map, INV_ICM42600_REG_FIFO_DATA,
> + st->fifo.data, st->fifo.count);
> + if (ret)
> + return ret;
> +
> + /* compute number of samples for each sensor */
> + for (i = 0; i < st->fifo.count; i += size) {
> + size = inv_icm42600_fifo_decode_packet(&st->fifo.data[i],
> + &accel, &gyro, &temp, ×tamp, &odr);
> + if (size <= 0)
> + break;
> + if (gyro != NULL && inv_icm42600_fifo_is_data_valid(gyro))
> + st->fifo.nb.gyro++;
> + if (accel != NULL && inv_icm42600_fifo_is_data_valid(accel))
> + st->fifo.nb.accel++;
> + st->fifo.nb.total++;
> + }
> +
> + return 0;
> +}
> +
> +int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)
> +{
> + int ret;
> +
> + if (st->fifo.nb.total == 0)
> + return 0;
> +
> + if (st->fifo.nb.gyro > 0) {
> + ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
> + if (ret)
> + return ret;
> + }
> +
> + if (st->fifo.nb.accel > 0) {
> + ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
> + unsigned int count)
> +{
> + int ret;
> +
> + ret = inv_icm42600_buffer_fifo_read(st, count);
> + if (ret)
> + return ret;
Definitely searching my memory for how this works in the core, so
I may have it wrong.
This is a bit unusual (I think). The intent of the flush
is to read up to 'n' bytes because someone just did a read on the buffer
or select, and there was data in the hwfifo capable of satisfying the read
even though we haven't yet reached the watermark.
Given both sensor types are coming from one buffer, do we have a potential
issue here or under serving even though data is available?
The case I worry may be served late is when an poll / select
is waiting for sufficient data.
So what should we be doing? We want to guarantee to provide data
for each sensor type if it's in the hwfifo. As such we could keep reading
until we have enough, but that could cause some issues if the two data rates
are very different (overflow on the other kfifo)
Maybe what you have here is the best we can do.
I'm assuming the watermark level has a similar problem. One value represents
the sum of the two types of data.
> +
> + if (st->fifo.nb.total == 0)
> + return 0;
> +
> + if (st->fifo.nb.gyro > 0) {
> + ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
> + if (ret)
> + return ret;
> + }
> +
> + if (st->fifo.nb.accel > 0) {
> + ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +int inv_icm42600_buffer_init(struct inv_icm42600_state *st)
> +{
> + unsigned int val;
> + int ret;
> +
> + /*
> + * Default FIFO configuration (bits 7 to 5)
> + * - use invalid value
> + * - FIFO count in bytes
> + * - FIFO count in big endian
> + */
> + val = INV_ICM42600_INTF_CONFIG0_FIFO_COUNT_ENDIAN;
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG0,
> + GENMASK(7, 5), val);
> + if (ret)
> + return ret;
> +
> + /*
> + * Enable FIFO partial read and continuous watermark interrupt.
> + * Disable all FIFO EN bits.
> + */
> + val = INV_ICM42600_FIFO_CONFIG1_RESUME_PARTIAL_RD |
> + INV_ICM42600_FIFO_CONFIG1_WM_GT_TH;
> + return regmap_update_bits(st->map, INV_ICM42600_REG_FIFO_CONFIG1,
> + GENMASK(6, 5) | GENMASK(3, 0), val);
> +}
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.h b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.h
> new file mode 100644
> index 000000000000..de2a3949dcc7
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.h
> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020 Invensense, Inc.
> + */
> +
> +#ifndef INV_ICM42600_BUFFER_H_
> +#define INV_ICM42600_BUFFER_H_
> +
> +#include <linux/kernel.h>
> +#include <linux/bits.h>
> +
> +struct inv_icm42600_state;
> +
> +#define INV_ICM42600_SENSOR_GYRO BIT(0)
> +#define INV_ICM42600_SENSOR_ACCEL BIT(1)
> +#define INV_ICM42600_SENSOR_TEMP BIT(2)
> +
> +/**
> + * struct inv_icm42600_fifo - FIFO state variables
> + * @on: reference counter for FIFO on.
> + * @en: bits field of INV_ICM42600_SENSOR_* for FIFO EN bits.
> + * @period: FIFO internal period.
> + * @watermark: watermark configuration values for accel and gyro.
> + * @count: number of bytes in the FIFO data buffer.
> + * @nb: gyro, accel and total samples in the FIFO data buffer.
> + * @data: FIFO data buffer aligned for DMA (2kB + 32 bytes of read cache).
> + */
> +struct inv_icm42600_fifo {
> + unsigned int on;
> + unsigned int en;
> + uint32_t period;
> + struct {
> + unsigned int gyro;
> + unsigned int accel;
> + } watermark;
> + size_t count;
> + struct {
> + size_t gyro;
> + size_t accel;
> + size_t total;
> + } nb;
> + uint8_t data[2080] ____cacheline_aligned;
> +};
> +
> +/* FIFO data packet */
> +struct inv_icm42600_fifo_sensor_data {
> + __be16 x;
> + __be16 y;
> + __be16 z;
> +} __packed;
Why packed? Should be anyway I think.
> +#define INV_ICM42600_FIFO_DATA_INVALID -32768
> +
> +static inline int16_t inv_icm42600_fifo_get_sensor_data(__be16 d)
> +{
> + return be16_to_cpu(d);
> +}
> +
> +static inline bool
> +inv_icm42600_fifo_is_data_valid(const struct inv_icm42600_fifo_sensor_data *s)
> +{
> + int16_t x, y, z;
> +
> + x = inv_icm42600_fifo_get_sensor_data(s->x);
> + y = inv_icm42600_fifo_get_sensor_data(s->y);
> + z = inv_icm42600_fifo_get_sensor_data(s->z);
> +
> + if (x == INV_ICM42600_FIFO_DATA_INVALID &&
> + y == INV_ICM42600_FIFO_DATA_INVALID &&
> + z == INV_ICM42600_FIFO_DATA_INVALID)
> + return false;
> +
> + return true;
> +}
> +
> +ssize_t inv_icm42600_fifo_decode_packet(const void *packet, const void **accel,
> + const void **gyro, const int8_t **temp,
> + const void **timestamp, unsigned int *odr);
> +
> +extern const struct iio_buffer_setup_ops inv_icm42600_buffer_ops;
> +
> +int inv_icm42600_buffer_init(struct inv_icm42600_state *st);
> +
> +void inv_icm42600_buffer_update_fifo_period(struct inv_icm42600_state *st);
> +
> +int inv_icm42600_buffer_set_fifo_en(struct inv_icm42600_state *st,
> + unsigned int fifo_en);
> +
> +int inv_icm42600_buffer_update_watermark(struct inv_icm42600_state *st);
> +
> +int inv_icm42600_buffer_fifo_read(struct inv_icm42600_state *st,
> + unsigned int max);
> +
> +int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st);
> +
> +int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
> + unsigned int count);
> +
> +#endif
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 246c1eb52231..6f1c1eb83953 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -18,6 +18,7 @@
> #include <linux/iio/iio.h>
>
> #include "inv_icm42600.h"
> +#include "inv_icm42600_buffer.h"
>
> static const struct regmap_range_cfg inv_icm42600_regmap_ranges[] = {
> {
> @@ -429,6 +430,18 @@ static irqreturn_t inv_icm42600_irq_handler(int irq, void *_data)
> if (status & INV_ICM42600_INT_STATUS_FIFO_FULL)
> dev_warn(dev, "FIFO full data lost!\n");
>
> + /* FIFO threshold reached */
> + if (status & INV_ICM42600_INT_STATUS_FIFO_THS) {
> + ret = inv_icm42600_buffer_fifo_read(st, 0);
> + if (ret) {
> + dev_err(dev, "FIFO read error %d\n", ret);
> + goto out_unlock;
> + }
> + ret = inv_icm42600_buffer_fifo_parse(st);
> + if (ret)
> + dev_err(dev, "FIFO parsing error %d\n", ret);
> + }
> +
> out_unlock:
> mutex_unlock(&st->lock);
> return IRQ_HANDLED;
> @@ -600,6 +613,10 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
> if (ret)
> return ret;
>
> + ret = inv_icm42600_buffer_init(st);
> + if (ret)
> + return ret;
> +
> ret = inv_icm42600_gyro_init(st);
> if (ret)
> return ret;
> @@ -645,6 +662,14 @@ static int __maybe_unused inv_icm42600_suspend(struct device *dev)
> goto out_unlock;
> }
>
> + /* disable FIFO data streaming */
> + if (st->fifo.on) {
> + ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> + INV_ICM42600_FIFO_CONFIG_BYPASS);
> + if (ret)
> + goto out_unlock;
> + }
> +
> ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> INV_ICM42600_SENSOR_MODE_OFF, false,
> NULL);
> @@ -684,6 +709,11 @@ static int __maybe_unused inv_icm42600_resume(struct device *dev)
> if (ret)
> goto out_unlock;
>
> + /* restore FIFO data streaming */
> + if (st->fifo.on)
> + ret = regmap_write(st->map, INV_ICM42600_REG_FIFO_CONFIG,
> + INV_ICM42600_FIFO_CONFIG_STREAM);
> +
> out_unlock:
> mutex_unlock(&st->lock);
> return ret;
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> index 38654e0d217b..b05c33876b8d 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> @@ -11,9 +11,12 @@
> #include <linux/delay.h>
> #include <linux/math64.h>
> #include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
>
> #include "inv_icm42600.h"
> #include "inv_icm42600_temp.h"
> +#include "inv_icm42600_buffer.h"
>
> #define INV_ICM42600_GYRO_CHAN(_modifier, _index, _ext_info) \
> { \
> @@ -64,6 +67,76 @@ static const struct iio_chan_spec inv_icm42600_gyro_channels[] = {
> INV_ICM42600_TEMP_CHAN(INV_ICM42600_GYRO_SCAN_TEMP),
> };
>
> +/* IIO buffer data: 8 bytes */
> +struct inv_icm42600_gyro_buffer {
> + struct inv_icm42600_fifo_sensor_data gyro;
> + int8_t temp;
> + uint8_t padding;
> +};
> +
> +#define INV_ICM42600_SCAN_MASK_GYRO_3AXIS \
> + (BIT(INV_ICM42600_GYRO_SCAN_X) | \
> + BIT(INV_ICM42600_GYRO_SCAN_Y) | \
> + BIT(INV_ICM42600_GYRO_SCAN_Z))
> +
> +#define INV_ICM42600_SCAN_MASK_TEMP BIT(INV_ICM42600_GYRO_SCAN_TEMP)
> +
> +static const unsigned long inv_icm42600_gyro_scan_masks[] = {
> + /* 3-axis gyro + temperature */
> + INV_ICM42600_SCAN_MASK_GYRO_3AXIS | INV_ICM42600_SCAN_MASK_TEMP,
> + 0,
> +};
> +
> +/* enable gyroscope sensor and FIFO write */
> +static int inv_icm42600_gyro_update_scan_mode(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> + unsigned int fifo_en = 0;
> + unsigned int sleep_gyro = 0;
> + unsigned int sleep_temp = 0;
> + unsigned int sleep;
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + if (*scan_mask & INV_ICM42600_SCAN_MASK_TEMP) {
> + /* enable temp sensor */
> + ret = inv_icm42600_set_temp_conf(st, true, &sleep_temp);
> + if (ret)
> + goto out_unlock;
> + fifo_en |= INV_ICM42600_SENSOR_TEMP;
> + }
> +
> + if (*scan_mask & INV_ICM42600_SCAN_MASK_GYRO_3AXIS) {
> + /* enable gyro sensor */
> + conf.mode = INV_ICM42600_SENSOR_MODE_LOW_NOISE;
> + ret = inv_icm42600_set_gyro_conf(st, &conf, &sleep_gyro);
> + if (ret)
> + goto out_unlock;
> + fifo_en |= INV_ICM42600_SENSOR_GYRO;
> + }
> +
> + /* update data FIFO write */
> + ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
> + if (ret)
> + goto out_unlock;
> +
> + ret = inv_icm42600_buffer_update_watermark(st);
> +
> +out_unlock:
> + mutex_unlock(&st->lock);
> + /* sleep maximum required time */
> + if (sleep_gyro > sleep_temp)
> + sleep = sleep_gyro;
> + else
> + sleep = sleep_temp;
> + if (sleep)
> + msleep(sleep);
> + return ret;
> +}
> +
> static int inv_icm42600_gyro_read_sensor(struct inv_icm42600_state *st,
> struct iio_chan_spec const *chan,
> int16_t *val)
> @@ -260,7 +333,12 @@ static int inv_icm42600_gyro_write_odr(struct inv_icm42600_state *st,
> mutex_lock(&st->lock);
>
> ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);
> + if (ret)
> + goto out_unlock;
> + inv_icm42600_buffer_update_fifo_period(st);
> + inv_icm42600_buffer_update_watermark(st);
>
> +out_unlock:
> mutex_unlock(&st->lock);
> pm_runtime_mark_last_busy(dev);
> pm_runtime_put_autosuspend(dev);
> @@ -574,12 +652,51 @@ static int inv_icm42600_gyro_write_raw_get_fmt(struct iio_dev *indio_dev,
> }
> }
>
> +static int inv_icm42600_gyro_hwfifo_set_watermark(struct iio_dev *indio_dev,
> + unsigned int val)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + st->fifo.watermark.gyro = val;
> + ret = inv_icm42600_buffer_update_watermark(st);
> +
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> +static int inv_icm42600_gyro_hwfifo_flush(struct iio_dev *indio_dev,
> + unsigned int count)
> +{
Nothing to do with this patch, but I realised reading this that we have
some 'unusual' use of the word flush here. It's a straight forward
read function so not sure why we called it flush.
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + int ret;
> +
> + if (count == 0)
> + return 0;
> +
> + mutex_lock(&st->lock);
> +
> + ret = inv_icm42600_buffer_hwfifo_flush(st, count);
> + if (!ret)
> + ret = st->fifo.nb.gyro;
> +
> + mutex_unlock(&st->lock);
> +
> + return ret;
> +}
> +
> static const struct iio_info inv_icm42600_gyro_info = {
> .read_raw = inv_icm42600_gyro_read_raw,
> .read_avail = inv_icm42600_gyro_read_avail,
> .write_raw = inv_icm42600_gyro_write_raw,
> .write_raw_get_fmt = inv_icm42600_gyro_write_raw_get_fmt,
> .debugfs_reg_access = inv_icm42600_debugfs_reg,
> + .update_scan_mode = inv_icm42600_gyro_update_scan_mode,
> + .hwfifo_set_watermark = inv_icm42600_gyro_hwfifo_set_watermark,
> + .hwfifo_flush_to_buffer = inv_icm42600_gyro_hwfifo_flush,
> };
>
> int inv_icm42600_gyro_init(struct inv_icm42600_state *st)
> @@ -587,6 +704,7 @@ int inv_icm42600_gyro_init(struct inv_icm42600_state *st)
> struct device *dev = regmap_get_device(st->map);
> const char *name;
> struct iio_dev *indio_dev;
> + struct iio_buffer *buffer;
>
> name = devm_kasprintf(dev, GFP_KERNEL, "%s-gyro", st->name);
> if (!name)
> @@ -596,14 +714,54 @@ int inv_icm42600_gyro_init(struct inv_icm42600_state *st)
> if (!indio_dev)
> return -ENOMEM;
>
> + buffer = devm_iio_kfifo_allocate(dev);
> + if (!buffer)
> + return -ENOMEM;
> +
> iio_device_set_drvdata(indio_dev, st);
> indio_dev->dev.parent = dev;
> indio_dev->name = name;
> indio_dev->info = &inv_icm42600_gyro_info;
> - indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_SOFTWARE;
> indio_dev->channels = inv_icm42600_gyro_channels;
> indio_dev->num_channels = ARRAY_SIZE(inv_icm42600_gyro_channels);
> + indio_dev->available_scan_masks = inv_icm42600_gyro_scan_masks;
> + indio_dev->setup_ops = &inv_icm42600_buffer_ops;
> +
> + iio_device_attach_buffer(indio_dev, buffer);
>
> st->indio_gyro = indio_dev;
> return devm_iio_device_register(dev, st->indio_gyro);
> }
> +
> +int inv_icm42600_gyro_parse_fifo(struct iio_dev *indio_dev)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + ssize_t i, size;
> + const void *accel, *gyro, *timestamp;
> + const int8_t *temp;
> + unsigned int odr;
> + struct inv_icm42600_gyro_buffer buffer = {
> + .padding = 0,
Might be worth a comment here or where the structure is defined
on why we make padding explicit.
> + };
> +
> + /* parse all fifo packets */
> + for (i = 0; i < st->fifo.count; i += size) {
> + size = inv_icm42600_fifo_decode_packet(&st->fifo.data[i],
> + &accel, &gyro, &temp, ×tamp, &odr);
> + /* quit if error or FIFO is empty */
> + if (size <= 0)
> + return size;
> +
> + /* skip packet if no gyro data or data is invalid */
> + if (gyro == NULL || !inv_icm42600_fifo_is_data_valid(gyro))
> + continue;
> +
> + /* fill and push data buffer */
> + memcpy(&buffer.gyro, gyro, sizeof(buffer.gyro));
> + buffer.temp = temp ? *temp : 0;
> + iio_push_to_buffers(indio_dev, &buffer);
> + }
> +
> + return 0;
> +}
^ permalink raw reply
* Re: [PATCH v2 2/3] media: rockchip: Introduce driver for Rockhip's camera interface
From: kbuild test robot @ 2020-05-31 12:16 UTC (permalink / raw)
To: Maxime Chevallier, Mauro Carvalho Chehab, Robin Murphy,
Rob Herring, Mark Rutland, Heiko Stuebner, Hans Verkuil
Cc: kbuild-all, clang-built-linux, linux-media, Maxime Chevallier,
devicetree, linux-arm-kernel
In-Reply-To: <20200529130405.929429-3-maxime.chevallier@bootlin.com>
[-- Attachment #1: Type: text/plain, Size: 3247 bytes --]
Hi Maxime,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on rockchip/for-next robh/for-next v5.7-rc7 next-20200529]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Maxime-Chevallier/media-rockchip-Introduce-driver-for-the-camera-interface-on-PX30/20200531-081952
base: git://linuxtv.org/media_tree.git master
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 2388a096e7865c043e83ece4e26654bd3d1a20d5)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
In file included from drivers/media/platform/rockchip/cif/dev.c:22:
>> drivers/media/platform/rockchip/cif/regs.h:113:9: warning: 'FRAME_END' macro redefined [-Wmacro-redefined]
#define FRAME_END (0x01 << 0)
^
arch/x86/include/asm/frame.h:89:9: note: previous definition is here
#define FRAME_END
^
>> drivers/media/platform/rockchip/cif/dev.c:253:13: warning: variable 'v4l2_dev' is uninitialized when used here [-Wuninitialized]
v4l2_info(v4l2_dev, "No reserved memory region assign to CIFn");
^~~~~~~~
include/media/v4l2-common.h:67:25: note: expanded from macro 'v4l2_info'
v4l2_printk(KERN_INFO, dev, fmt , ## arg)
^~~
include/media/v4l2-common.h:58:28: note: expanded from macro 'v4l2_printk'
printk(level "%s: " fmt, (dev)->name , ## arg)
^~~
drivers/media/platform/rockchip/cif/dev.c:216:30: note: initialize the variable 'v4l2_dev' to silence this warning
struct v4l2_device *v4l2_dev;
^
= NULL
2 warnings generated.
--
In file included from drivers/media/platform/rockchip/cif/capture.c:22:
>> drivers/media/platform/rockchip/cif/regs.h:113:9: warning: 'FRAME_END' macro redefined [-Wmacro-redefined]
#define FRAME_END (0x01 << 0)
^
arch/x86/include/asm/frame.h:89:9: note: previous definition is here
#define FRAME_END
^
1 warning generated.
vim +/FRAME_END +113 drivers/media/platform/rockchip/cif/regs.h
110
111 /* CIF INTSTAT */
112 #define INTSTAT_CLS (0x3FF)
> 113 #define FRAME_END (0x01 << 0)
114 #define LINE_ERR (0x01 << 2)
115 #define PST_INF_FRAME_END (0x01 << 9)
116 #define FRAME_END_CLR (0x01 << 0)
117 #define LINE_ERR_CLR (0x01 << 2)
118 #define PST_INF_FRAME_END_CLR (0x01 << 9)
119 #define INTSTAT_ERR (0xFC)
120
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 73692 bytes --]
^ permalink raw reply
* Re: [PATCH v2 10/12] iio: imu: inv_icm42600: add accurate timestamping
From: Jonathan Cameron @ 2020-05-31 13:04 UTC (permalink / raw)
To: Jean-Baptiste Maneyrol
Cc: robh+dt, robh, mchehab+huawei, davem, gregkh, linux-iio,
devicetree, linux-kernel
In-Reply-To: <20200527185711.21331-11-jmaneyrol@invensense.com>
On Wed, 27 May 2020 20:57:09 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
> Add a timestamping mechanism for buffer that provides accurate
> event timestamps when using watermark. This mechanism estimates
> device internal clock by comparing FIFO interrupts delta time and
> device elapsed time computed by parsing FIFO data.
>
> Take interrupt timestamp in hard irq handler and add IIO device
> specific timestamp structures in device private allocation.
I haven't checked the maths or anything, but basic principle seems
sound. I'm wondering if we want to document somewhere what the
various ways we do this sort of timestamp generation are. They
give me a headache normally and it would be good to point people
to a reference. Still that's a job for another day.
I commented on the (lack) of need for force 8 byte alignment inline.
Jonathan
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
> ---
> drivers/iio/imu/inv_icm42600/Makefile | 1 +
> drivers/iio/imu/inv_icm42600/inv_icm42600.h | 5 +
> .../iio/imu/inv_icm42600/inv_icm42600_accel.c | 40 +++-
> .../imu/inv_icm42600/inv_icm42600_buffer.c | 28 +++
> .../iio/imu/inv_icm42600/inv_icm42600_core.c | 17 +-
> .../iio/imu/inv_icm42600/inv_icm42600_gyro.c | 40 +++-
> .../imu/inv_icm42600/inv_icm42600_timestamp.c | 195 ++++++++++++++++++
> .../imu/inv_icm42600/inv_icm42600_timestamp.h | 85 ++++++++
> 8 files changed, 398 insertions(+), 13 deletions(-)
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.c
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.h
>
> diff --git a/drivers/iio/imu/inv_icm42600/Makefile b/drivers/iio/imu/inv_icm42600/Makefile
> index 0f49f6df3647..291714d9aa54 100644
> --- a/drivers/iio/imu/inv_icm42600/Makefile
> +++ b/drivers/iio/imu/inv_icm42600/Makefile
> @@ -6,6 +6,7 @@ inv-icm42600-y += inv_icm42600_gyro.o
> inv-icm42600-y += inv_icm42600_accel.o
> inv-icm42600-y += inv_icm42600_temp.o
> inv-icm42600-y += inv_icm42600_buffer.o
> +inv-icm42600-y += inv_icm42600_timestamp.o
>
> obj-$(CONFIG_INV_ICM42600_I2C) += inv-icm42600-i2c.o
> inv-icm42600-i2c-y += inv_icm42600_i2c.o
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 4d5811562a61..2de0dd7675fb 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -126,6 +126,7 @@ struct inv_icm42600_suspended {
> * @indio_accel: accelerometer IIO device.
> * @buffer: data transfer buffer aligned for DMA.
> * @fifo: FIFO management structure.
> + * @timestamp: interrupt timestamps.
> */
> struct inv_icm42600_state {
> struct mutex lock;
> @@ -141,6 +142,10 @@ struct inv_icm42600_state {
> struct iio_dev *indio_accel;
> uint8_t buffer[2] ____cacheline_aligned;
> struct inv_icm42600_fifo fifo;
> + struct {
> + int64_t gyro;
> + int64_t accel;
> + } timestamp;
> };
>
> /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> index c73ce204efc6..ec1d124c1471 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_accel.c
> @@ -17,6 +17,7 @@
> #include "inv_icm42600.h"
> #include "inv_icm42600_temp.h"
> #include "inv_icm42600_buffer.h"
> +#include "inv_icm42600_timestamp.h"
>
> #define INV_ICM42600_ACCEL_CHAN(_modifier, _index, _ext_info) \
> { \
> @@ -50,6 +51,7 @@ enum inv_icm42600_accel_scan {
> INV_ICM42600_ACCEL_SCAN_Y,
> INV_ICM42600_ACCEL_SCAN_Z,
> INV_ICM42600_ACCEL_SCAN_TEMP,
> + INV_ICM42600_ACCEL_SCAN_TIMESTAMP,
> };
>
> static const struct iio_chan_spec_ext_info inv_icm42600_accel_ext_infos[] = {
> @@ -65,13 +67,15 @@ static const struct iio_chan_spec inv_icm42600_accel_channels[] = {
> INV_ICM42600_ACCEL_CHAN(IIO_MOD_Z, INV_ICM42600_ACCEL_SCAN_Z,
> inv_icm42600_accel_ext_infos),
> INV_ICM42600_TEMP_CHAN(INV_ICM42600_ACCEL_SCAN_TEMP),
> + IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42600_ACCEL_SCAN_TIMESTAMP),
> };
>
> -/* IIO buffer data: 8 bytes */
> +/* IIO buffer data: 16 bytes */
> struct inv_icm42600_accel_buffer {
> struct inv_icm42600_fifo_sensor_data accel;
> int8_t temp;
> uint8_t padding;
> + int64_t timestamp;
So this falls into the open question I have in the cleanup set on timestamp
alignment. What you have here guarantees that we have the correct spacing, but
it allows the alignment of the whole structure to be 4 bytes on x86_32
I don't 'think' that's a problem because the relevant unaligned 8 bytes write
and read should be fine at 4 byte alignment. Most other archs will
do 8 byte alignment anyway.
Though I'd explain that here to avoid confusion against the fix set where
I will probably be more paranoid and mark all of them __aligned(8) as it
makes it harder to get it wrong.
> };
>
> #define INV_ICM42600_SCAN_MASK_ACCEL_3AXIS \
> @@ -92,6 +96,7 @@ static int inv_icm42600_accel_update_scan_mode(struct iio_dev *indio_dev,
> const unsigned long *scan_mask)
> {
> struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + struct inv_icm42600_timestamp *ts = iio_priv(indio_dev);
> struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> unsigned int fifo_en = 0;
> unsigned int sleep_temp = 0;
> @@ -119,6 +124,7 @@ static int inv_icm42600_accel_update_scan_mode(struct iio_dev *indio_dev,
> }
>
> /* update data FIFO write */
> + inv_icm42600_timestamp_apply_odr(ts, 0, 0, 0);
> ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
> if (ret)
> goto out_unlock;
> @@ -299,9 +305,11 @@ static int inv_icm42600_accel_read_odr(struct inv_icm42600_state *st,
> return IIO_VAL_INT_PLUS_MICRO;
> }
>
> -static int inv_icm42600_accel_write_odr(struct inv_icm42600_state *st,
> +static int inv_icm42600_accel_write_odr(struct iio_dev *indio_dev,
> int val, int val2)
> {
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + struct inv_icm42600_timestamp *ts = iio_priv(indio_dev);
> struct device *dev = regmap_get_device(st->map);
> unsigned int idx;
> struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> @@ -320,6 +328,11 @@ static int inv_icm42600_accel_write_odr(struct inv_icm42600_state *st,
> pm_runtime_get_sync(dev);
> mutex_lock(&st->lock);
>
> + ret = inv_icm42600_timestamp_update_odr(ts, inv_icm42600_odr_to_period(conf.odr),
> + iio_buffer_enabled(indio_dev));
> + if (ret)
> + goto out_unlock;
> +
> ret = inv_icm42600_set_accel_conf(st, &conf, NULL);
> if (ret)
> goto out_unlock;
> @@ -609,7 +622,7 @@ static int inv_icm42600_accel_write_raw(struct iio_dev *indio_dev,
> iio_device_release_direct_mode(indio_dev);
> return ret;
> case IIO_CHAN_INFO_SAMP_FREQ:
> - return inv_icm42600_accel_write_odr(st, val, val2);
> + return inv_icm42600_accel_write_odr(indio_dev, val, val2);
> case IIO_CHAN_INFO_CALIBBIAS:
> ret = iio_device_claim_direct_mode(indio_dev);
> if (ret)
> @@ -692,6 +705,8 @@ int inv_icm42600_accel_init(struct inv_icm42600_state *st)
> {
> struct device *dev = regmap_get_device(st->map);
> const char *name;
> + struct inv_icm42600_timestamp *ts;
> + uint32_t period;
> struct iio_dev *indio_dev;
> struct iio_buffer *buffer;
>
> @@ -699,7 +714,7 @@ int inv_icm42600_accel_init(struct inv_icm42600_state *st)
> if (!name)
> return -ENOMEM;
>
> - indio_dev = devm_iio_device_alloc(dev, 0);
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*ts));
> if (!indio_dev)
> return -ENOMEM;
>
> @@ -707,6 +722,10 @@ int inv_icm42600_accel_init(struct inv_icm42600_state *st)
> if (!buffer)
> return -ENOMEM;
>
> + ts = iio_priv(indio_dev);
> + period = inv_icm42600_odr_to_period(st->conf.accel.odr);
> + inv_icm42600_timestamp_init(ts, period);
> +
> iio_device_set_drvdata(indio_dev, st);
> indio_dev->dev.parent = dev;
> indio_dev->name = name;
> @@ -726,16 +745,19 @@ int inv_icm42600_accel_init(struct inv_icm42600_state *st)
> int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev)
> {
> struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + struct inv_icm42600_timestamp *ts = iio_priv(indio_dev);
> ssize_t i, size;
> + unsigned int no;
> const void *accel, *gyro, *timestamp;
> const int8_t *temp;
> unsigned int odr;
> + int64_t ts_val;
> struct inv_icm42600_accel_buffer buffer = {
> .padding = 0,
> };
>
> /* parse all fifo packets */
> - for (i = 0; i < st->fifo.count; i += size) {
> + for (i = 0, no = 0; i < st->fifo.count; i += size, ++no) {
> size = inv_icm42600_fifo_decode_packet(&st->fifo.data[i],
> &accel, &gyro, &temp, ×tamp, &odr);
> /* quit if error or FIFO is empty */
> @@ -746,10 +768,16 @@ int inv_icm42600_accel_parse_fifo(struct iio_dev *indio_dev)
> if (accel == NULL || !inv_icm42600_fifo_is_data_valid(accel))
> continue;
>
> + /* update odr */
> + if (odr & INV_ICM42600_SENSOR_ACCEL)
> + inv_icm42600_timestamp_apply_odr(ts, st->fifo.period,
> + st->fifo.nb.total, no);
> +
> /* fill and push data buffer */
> memcpy(&buffer.accel, accel, sizeof(buffer.accel));
> buffer.temp = temp ? *temp : 0;
> - iio_push_to_buffers(indio_dev, &buffer);
> + ts_val = inv_icm42600_timestamp_pop(ts);
> + iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts_val);
> }
>
> return 0;
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> index c91075f62231..3c8b1b19de15 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_buffer.c
> @@ -14,6 +14,7 @@
> #include <linux/iio/buffer.h>
>
> #include "inv_icm42600.h"
> +#include "inv_icm42600_timestamp.h"
> #include "inv_icm42600_buffer.h"
>
> /* FIFO header: 1 byte */
> @@ -356,6 +357,7 @@ static int inv_icm42600_buffer_postdisable(struct iio_dev *indio_dev)
> struct device *dev = regmap_get_device(st->map);
> unsigned int sensor;
> unsigned int *watermark;
> + struct inv_icm42600_timestamp *ts;
> struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> unsigned int sleep_temp = 0;
> unsigned int sleep_sensor = 0;
> @@ -365,9 +367,11 @@ static int inv_icm42600_buffer_postdisable(struct iio_dev *indio_dev)
> if (indio_dev == st->indio_gyro) {
> sensor = INV_ICM42600_SENSOR_GYRO;
> watermark = &st->fifo.watermark.gyro;
> + ts = iio_priv(st->indio_gyro);
> } else if (indio_dev == st->indio_accel) {
> sensor = INV_ICM42600_SENSOR_ACCEL;
> watermark = &st->fifo.watermark.accel;
> + ts = iio_priv(st->indio_accel);
> } else {
> return -EINVAL;
> }
> @@ -395,6 +399,8 @@ static int inv_icm42600_buffer_postdisable(struct iio_dev *indio_dev)
> if (!st->fifo.on)
> ret = inv_icm42600_set_temp_conf(st, false, &sleep_temp);
>
> + inv_icm42600_timestamp_reset(ts);
> +
> out_unlock:
> mutex_unlock(&st->lock);
>
> @@ -480,17 +486,26 @@ int inv_icm42600_buffer_fifo_read(struct inv_icm42600_state *st,
>
> int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)
> {
> + struct inv_icm42600_timestamp *ts;
> int ret;
>
> if (st->fifo.nb.total == 0)
> return 0;
>
> + /* handle gyroscope timestamp and FIFO data parsing */
> + ts = iio_priv(st->indio_gyro);
> + inv_icm42600_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
> + st->fifo.nb.gyro, st->timestamp.gyro);
> if (st->fifo.nb.gyro > 0) {
> ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
> if (ret)
> return ret;
> }
>
> + /* handle accelerometer timestamp and FIFO data parsing */
> + ts = iio_priv(st->indio_accel);
> + inv_icm42600_timestamp_interrupt(ts, st->fifo.period, st->fifo.nb.total,
> + st->fifo.nb.accel, st->timestamp.accel);
> if (st->fifo.nb.accel > 0) {
> ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
> if (ret)
> @@ -503,8 +518,13 @@ int inv_icm42600_buffer_fifo_parse(struct inv_icm42600_state *st)
> int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
> unsigned int count)
> {
> + struct inv_icm42600_timestamp *ts;
> + int64_t gyro_ts, accel_ts;
> int ret;
>
> + gyro_ts = iio_get_time_ns(st->indio_gyro);
> + accel_ts = iio_get_time_ns(st->indio_accel);
> +
> ret = inv_icm42600_buffer_fifo_read(st, count);
> if (ret)
> return ret;
> @@ -513,12 +533,20 @@ int inv_icm42600_buffer_hwfifo_flush(struct inv_icm42600_state *st,
> return 0;
>
> if (st->fifo.nb.gyro > 0) {
> + ts = iio_priv(st->indio_gyro);
> + inv_icm42600_timestamp_interrupt(ts, st->fifo.period,
> + st->fifo.nb.total, st->fifo.nb.gyro,
> + gyro_ts);
> ret = inv_icm42600_gyro_parse_fifo(st->indio_gyro);
> if (ret)
> return ret;
> }
>
> if (st->fifo.nb.accel > 0) {
> + ts = iio_priv(st->indio_accel);
> + inv_icm42600_timestamp_interrupt(ts, st->fifo.period,
> + st->fifo.nb.total, st->fifo.nb.accel,
> + accel_ts);
> ret = inv_icm42600_accel_parse_fifo(st->indio_accel);
> if (ret)
> return ret;
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 6f1c1eb83953..c0d676219fc7 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -19,6 +19,7 @@
>
> #include "inv_icm42600.h"
> #include "inv_icm42600_buffer.h"
> +#include "inv_icm42600_timestamp.h"
>
> static const struct regmap_range_cfg inv_icm42600_regmap_ranges[] = {
> {
> @@ -413,6 +414,16 @@ static int inv_icm42600_setup(struct inv_icm42600_state *st,
> return inv_icm42600_set_conf(st, hw->conf);
> }
>
> +static irqreturn_t inv_icm42600_irq_timestamp(int irq, void *_data)
> +{
> + struct inv_icm42600_state *st = _data;
> +
> + st->timestamp.gyro = iio_get_time_ns(st->indio_gyro);
> + st->timestamp.accel = iio_get_time_ns(st->indio_accel);
> +
> + return IRQ_WAKE_THREAD;
> +}
> +
> static irqreturn_t inv_icm42600_irq_handler(int irq, void *_data)
> {
> struct inv_icm42600_state *st = _data;
> @@ -493,7 +504,7 @@ static int inv_icm42600_irq_init(struct inv_icm42600_state *st, int irq,
> if (ret)
> return ret;
>
> - return devm_request_threaded_irq(dev, irq, NULL,
> + return devm_request_threaded_irq(dev, irq, inv_icm42600_irq_timestamp,
> inv_icm42600_irq_handler, irq_type,
> "inv_icm42600", st);
> }
> @@ -613,6 +624,10 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
> if (ret)
> return ret;
>
> + ret = inv_icm42600_timestamp_setup(st);
> + if (ret)
> + return ret;
> +
> ret = inv_icm42600_buffer_init(st);
> if (ret)
> return ret;
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> index b05c33876b8d..76d92a550278 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> @@ -17,6 +17,7 @@
> #include "inv_icm42600.h"
> #include "inv_icm42600_temp.h"
> #include "inv_icm42600_buffer.h"
> +#include "inv_icm42600_timestamp.h"
>
> #define INV_ICM42600_GYRO_CHAN(_modifier, _index, _ext_info) \
> { \
> @@ -50,6 +51,7 @@ enum inv_icm42600_gyro_scan {
> INV_ICM42600_GYRO_SCAN_Y,
> INV_ICM42600_GYRO_SCAN_Z,
> INV_ICM42600_GYRO_SCAN_TEMP,
> + INV_ICM42600_GYRO_SCAN_TIMESTAMP,
> };
>
> static const struct iio_chan_spec_ext_info inv_icm42600_gyro_ext_infos[] = {
> @@ -65,13 +67,15 @@ static const struct iio_chan_spec inv_icm42600_gyro_channels[] = {
> INV_ICM42600_GYRO_CHAN(IIO_MOD_Z, INV_ICM42600_GYRO_SCAN_Z,
> inv_icm42600_gyro_ext_infos),
> INV_ICM42600_TEMP_CHAN(INV_ICM42600_GYRO_SCAN_TEMP),
> + IIO_CHAN_SOFT_TIMESTAMP(INV_ICM42600_GYRO_SCAN_TIMESTAMP),
> };
>
> -/* IIO buffer data: 8 bytes */
> +/* IIO buffer data: 16 bytes */
> struct inv_icm42600_gyro_buffer {
> struct inv_icm42600_fifo_sensor_data gyro;
> int8_t temp;
> uint8_t padding;
> + int64_t timestamp;
> };
>
> #define INV_ICM42600_SCAN_MASK_GYRO_3AXIS \
> @@ -92,6 +96,7 @@ static int inv_icm42600_gyro_update_scan_mode(struct iio_dev *indio_dev,
> const unsigned long *scan_mask)
> {
> struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + struct inv_icm42600_timestamp *ts = iio_priv(indio_dev);
> struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> unsigned int fifo_en = 0;
> unsigned int sleep_gyro = 0;
> @@ -119,6 +124,7 @@ static int inv_icm42600_gyro_update_scan_mode(struct iio_dev *indio_dev,
> }
>
> /* update data FIFO write */
> + inv_icm42600_timestamp_apply_odr(ts, 0, 0, 0);
> ret = inv_icm42600_buffer_set_fifo_en(st, fifo_en | st->fifo.en);
> if (ret)
> goto out_unlock;
> @@ -311,9 +317,11 @@ static int inv_icm42600_gyro_read_odr(struct inv_icm42600_state *st,
> return IIO_VAL_INT_PLUS_MICRO;
> }
>
> -static int inv_icm42600_gyro_write_odr(struct inv_icm42600_state *st,
> +static int inv_icm42600_gyro_write_odr(struct iio_dev *indio_dev,
> int val, int val2)
> {
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + struct inv_icm42600_timestamp *ts = iio_priv(indio_dev);
> struct device *dev = regmap_get_device(st->map);
> unsigned int idx;
> struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> @@ -332,6 +340,11 @@ static int inv_icm42600_gyro_write_odr(struct inv_icm42600_state *st,
> pm_runtime_get_sync(dev);
> mutex_lock(&st->lock);
>
> + ret = inv_icm42600_timestamp_update_odr(ts, inv_icm42600_odr_to_period(conf.odr),
> + iio_buffer_enabled(indio_dev));
> + if (ret)
> + goto out_unlock;
> +
> ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);
> if (ret)
> goto out_unlock;
> @@ -620,7 +633,7 @@ static int inv_icm42600_gyro_write_raw(struct iio_dev *indio_dev,
> iio_device_release_direct_mode(indio_dev);
> return ret;
> case IIO_CHAN_INFO_SAMP_FREQ:
> - return inv_icm42600_gyro_write_odr(st, val, val2);
> + return inv_icm42600_gyro_write_odr(indio_dev, val, val2);
> case IIO_CHAN_INFO_CALIBBIAS:
> ret = iio_device_claim_direct_mode(indio_dev);
> if (ret)
> @@ -703,6 +716,8 @@ int inv_icm42600_gyro_init(struct inv_icm42600_state *st)
> {
> struct device *dev = regmap_get_device(st->map);
> const char *name;
> + struct inv_icm42600_timestamp *ts;
> + uint32_t period;
> struct iio_dev *indio_dev;
> struct iio_buffer *buffer;
>
> @@ -710,7 +725,7 @@ int inv_icm42600_gyro_init(struct inv_icm42600_state *st)
> if (!name)
> return -ENOMEM;
>
> - indio_dev = devm_iio_device_alloc(dev, 0);
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*ts));
> if (!indio_dev)
> return -ENOMEM;
>
> @@ -718,6 +733,10 @@ int inv_icm42600_gyro_init(struct inv_icm42600_state *st)
> if (!buffer)
> return -ENOMEM;
>
> + ts = iio_priv(indio_dev);
> + period = inv_icm42600_odr_to_period(st->conf.gyro.odr);
> + inv_icm42600_timestamp_init(ts, period);
> +
> iio_device_set_drvdata(indio_dev, st);
> indio_dev->dev.parent = dev;
> indio_dev->name = name;
> @@ -737,16 +756,19 @@ int inv_icm42600_gyro_init(struct inv_icm42600_state *st)
> int inv_icm42600_gyro_parse_fifo(struct iio_dev *indio_dev)
> {
> struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + struct inv_icm42600_timestamp *ts = iio_priv(indio_dev);
> ssize_t i, size;
> + unsigned int no;
> const void *accel, *gyro, *timestamp;
> const int8_t *temp;
> unsigned int odr;
> + int64_t ts_val;
> struct inv_icm42600_gyro_buffer buffer = {
> .padding = 0,
> };
>
> /* parse all fifo packets */
> - for (i = 0; i < st->fifo.count; i += size) {
> + for (i = 0, no = 0; i < st->fifo.count; i += size, ++no) {
> size = inv_icm42600_fifo_decode_packet(&st->fifo.data[i],
> &accel, &gyro, &temp, ×tamp, &odr);
> /* quit if error or FIFO is empty */
> @@ -757,10 +779,16 @@ int inv_icm42600_gyro_parse_fifo(struct iio_dev *indio_dev)
> if (gyro == NULL || !inv_icm42600_fifo_is_data_valid(gyro))
> continue;
>
> + /* update odr */
> + if (odr & INV_ICM42600_SENSOR_GYRO)
> + inv_icm42600_timestamp_apply_odr(ts, st->fifo.period,
> + st->fifo.nb.total, no);
> +
> /* fill and push data buffer */
> memcpy(&buffer.gyro, gyro, sizeof(buffer.gyro));
> buffer.temp = temp ? *temp : 0;
> - iio_push_to_buffers(indio_dev, &buffer);
> + ts_val = inv_icm42600_timestamp_pop(ts);
> + iio_push_to_buffers_with_timestamp(indio_dev, &buffer, ts_val);
> }
>
> return 0;
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.c
> new file mode 100644
> index 000000000000..7f2dc41f807b
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.c
> @@ -0,0 +1,195 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 Invensense, Inc.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/regmap.h>
> +#include <linux/math64.h>
> +
> +#include "inv_icm42600.h"
> +#include "inv_icm42600_timestamp.h"
> +
> +/* internal chip period is 32kHz, 31250ns */
> +#define INV_ICM42600_TIMESTAMP_PERIOD 31250
> +/* allow a jitter of +/- 2% */
> +#define INV_ICM42600_TIMESTAMP_JITTER 2
> +/* compute min and max periods accepted */
> +#define INV_ICM42600_TIMESTAMP_MIN_PERIOD(_p) \
> + (((_p) * (100 - INV_ICM42600_TIMESTAMP_JITTER)) / 100)
> +#define INV_ICM42600_TIMESTAMP_MAX_PERIOD(_p) \
> + (((_p) * (100 + INV_ICM42600_TIMESTAMP_JITTER)) / 100)
> +
> +/* Add a new value inside an accumulator and update the estimate value */
> +static void inv_update_acc(struct inv_icm42600_timestamp_acc *acc, uint32_t val)
> +{
> + uint64_t sum = 0;
> + size_t i;
> +
> + acc->values[acc->idx++] = val;
> + if (acc->idx >= ARRAY_SIZE(acc->values))
> + acc->idx = 0;
> +
> + /* compute the mean of all stored values, use 0 as empty slot */
> + for (i = 0; i < ARRAY_SIZE(acc->values); ++i) {
> + if (acc->values[i] == 0)
> + break;
> + sum += acc->values[i];
> + }
> +
> + acc->val = div_u64(sum, i);
> +}
> +
> +void inv_icm42600_timestamp_init(struct inv_icm42600_timestamp *ts,
> + uint32_t period)
> +{
> + /* initial odr for sensor after reset is 1kHz */
> + const uint32_t default_period = 1000000;
> +
> + /* current multiplier and period values after reset */
> + ts->mult = default_period / INV_ICM42600_TIMESTAMP_PERIOD;
> + ts->period = default_period;
> + /* new set multiplier is the one from chip initialization */
> + ts->new_mult = period / INV_ICM42600_TIMESTAMP_PERIOD;
> +
> + /* use theoretical value for chip period */
> + inv_update_acc(&ts->chip_period, INV_ICM42600_TIMESTAMP_PERIOD);
> +}
> +
> +int inv_icm42600_timestamp_setup(struct inv_icm42600_state *st)
> +{
> + unsigned int val;
> +
> + /* enable timestamp register */
> + val = INV_ICM42600_TMST_CONFIG_TMST_TO_REGS_EN |
> + INV_ICM42600_TMST_CONFIG_TMST_EN;
> + return regmap_update_bits(st->map, INV_ICM42600_REG_TMST_CONFIG,
> + INV_ICM42600_TMST_CONFIG_MASK, val);
> +}
> +
> +int inv_icm42600_timestamp_update_odr(struct inv_icm42600_timestamp *ts,
> + uint32_t period, bool fifo)
> +{
> + /* when FIFO is on, prevent odr change if one is already pending */
> + if (fifo && ts->new_mult != 0)
> + return -EAGAIN;
> +
> + ts->new_mult = period / INV_ICM42600_TIMESTAMP_PERIOD;
> +
> + return 0;
> +}
> +
> +static bool inv_validate_period(uint32_t period, uint32_t mult)
> +{
> + const uint32_t chip_period = INV_ICM42600_TIMESTAMP_PERIOD;
> + uint32_t period_min, period_max;
> +
> + /* check that period is acceptable */
> + period_min = INV_ICM42600_TIMESTAMP_MIN_PERIOD(chip_period) * mult;
> + period_max = INV_ICM42600_TIMESTAMP_MAX_PERIOD(chip_period) * mult;
> + if (period > period_min && period < period_max)
> + return true;
> + else
> + return false;
> +}
> +
> +static bool inv_compute_chip_period(struct inv_icm42600_timestamp *ts,
> + uint32_t mult, uint32_t period)
> +{
> + uint32_t new_chip_period;
> +
> + if (!inv_validate_period(period, mult))
> + return false;
> +
> + /* update chip internal period estimation */
> + new_chip_period = period / mult;
> + inv_update_acc(&ts->chip_period, new_chip_period);
> +
> + return true;
> +}
> +
> +void inv_icm42600_timestamp_interrupt(struct inv_icm42600_timestamp *ts,
> + uint32_t fifo_period, size_t fifo_nb,
> + size_t sensor_nb, int64_t timestamp)
> +{
> + struct inv_icm42600_timestamp_interval *it;
> + int64_t delta, interval;
> + const uint32_t fifo_mult = fifo_period / INV_ICM42600_TIMESTAMP_PERIOD;
> + uint32_t period = ts->period;
> + int32_t m;
> + bool valid = false;
> +
> + if (fifo_nb == 0)
> + return;
> +
> + /* update interrupt timestamp and compute chip and sensor periods */
> + it = &ts->it;
> + it->lo = it->up;
> + it->up = timestamp;
> + delta = it->up - it->lo;
> + if (it->lo != 0) {
> + /* compute period: delta time divided by number of samples */
> + period = div_s64(delta, fifo_nb);
> + valid = inv_compute_chip_period(ts, fifo_mult, period);
> + /* update sensor period if chip internal period is updated */
> + if (valid)
> + ts->period = ts->mult * ts->chip_period.val;
> + }
> +
> + /* no previous data, compute theoritical value from interrupt */
> + if (ts->timestamp == 0) {
> + /* elapsed time: sensor period * sensor samples number */
> + interval = (int64_t)ts->period * (int64_t)sensor_nb;
> + ts->timestamp = it->up - interval;
> + return;
> + }
> +
> + /* if interrupt interval is valid, sync with interrupt timestamp */
> + if (valid) {
> + /* compute measured fifo_period */
> + fifo_period = fifo_mult * ts->chip_period.val;
> + /* delta time between last sample and last interrupt */
> + delta = it->lo - ts->timestamp;
> + /* if there are multiple samples, go back to first one */
> + while (delta >= (fifo_period * 3 / 2))
> + delta -= fifo_period;
> + /* compute maximal adjustment value */
> + m = INV_ICM42600_TIMESTAMP_MAX_PERIOD(ts->period) - ts->period;
> + if (delta > m)
> + delta = m;
> + else if (delta < -m)
> + delta = -m;
> + ts->timestamp += delta;
> + }
> +}
> +
> +void inv_icm42600_timestamp_apply_odr(struct inv_icm42600_timestamp *ts,
> + uint32_t fifo_period, size_t fifo_nb,
> + unsigned int fifo_no)
> +{
> + int64_t interval;
> + uint32_t fifo_mult;
> +
> + if (ts->new_mult == 0)
> + return;
> +
> + /* update to new multiplier and update period */
> + ts->mult = ts->new_mult;
> + ts->new_mult = 0;
> + ts->period = ts->mult * ts->chip_period.val;
> +
> + /*
> + * After ODR change the time interval with the previous sample is
> + * undertermined (depends when the change occures). So we compute the
> + * timestamp from the current interrupt using the new FIFO period, the
> + * total number of samples and the current sample numero.
> + */
> + if (ts->timestamp != 0) {
> + /* compute measured fifo period */
> + fifo_mult = fifo_period / INV_ICM42600_TIMESTAMP_PERIOD;
> + fifo_period = fifo_mult * ts->chip_period.val;
> + /* computes time interval between interrupt and this sample */
> + interval = (int64_t)(fifo_nb - fifo_no) * (int64_t)fifo_period;
> + ts->timestamp = ts->it.up - interval;
> + }
> +}
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.h b/drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.h
> new file mode 100644
> index 000000000000..4e4f331d4fe4
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_timestamp.h
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2020 Invensense, Inc.
> + */
> +
> +#ifndef INV_ICM42600_TIMESTAMP_H_
> +#define INV_ICM42600_TIMESTAMP_H_
> +
> +#include <linux/kernel.h>
> +
> +struct inv_icm42600_state;
> +
> +/**
> + * struct inv_icm42600_timestamp_interval - timestamps interval
> + * @lo: interval lower bound
> + * @up: interval upper bound
> + */
> +struct inv_icm42600_timestamp_interval {
> + int64_t lo;
> + int64_t up;
> +};
> +
> +/**
> + * struct inv_icm42600_timestamp_acc - accumulator for computing an estimation
> + * @val: current estimation of the value, the mean of all values
> + * @idx: current index of the next free place in values table
> + * @values: table of all measured values, use for computing the mean
> + */
> +struct inv_icm42600_timestamp_acc {
> + uint32_t val;
> + size_t idx;
> + uint32_t values[32];
> +};
> +
> +/**
> + * struct inv_icm42600_timestamp - timestamp management states
> + * @it: interrupts interval timestamps
> + * @timestamp: store last timestamp for computing next data timestamp
> + * @mult: current internal period multiplier
> + * @new_mult: new set internal period multiplier (not yet effective)
> + * @period: measured current period of the sensor
> + * @chip_period: accumulator for computing internal chip period
> + */
> +struct inv_icm42600_timestamp {
> + struct inv_icm42600_timestamp_interval it;
> + int64_t timestamp;
> + uint32_t mult;
> + uint32_t new_mult;
> + uint32_t period;
> + struct inv_icm42600_timestamp_acc chip_period;
> +};
> +
> +void inv_icm42600_timestamp_init(struct inv_icm42600_timestamp *ts,
> + uint32_t period);
> +
> +int inv_icm42600_timestamp_setup(struct inv_icm42600_state *st);
> +
> +int inv_icm42600_timestamp_update_odr(struct inv_icm42600_timestamp *ts,
> + uint32_t period, bool fifo);
> +
> +void inv_icm42600_timestamp_interrupt(struct inv_icm42600_timestamp *ts,
> + uint32_t fifo_period, size_t fifo_nb,
> + size_t sensor_nb, int64_t timestamp);
> +
> +static inline int64_t
> +inv_icm42600_timestamp_pop(struct inv_icm42600_timestamp *ts)
> +{
> + ts->timestamp += ts->period;
> + return ts->timestamp;
> +}
> +
> +void inv_icm42600_timestamp_apply_odr(struct inv_icm42600_timestamp *ts,
> + uint32_t fifo_period, size_t fifo_nb,
> + unsigned int fifo_no);
> +
> +static inline void
> +inv_icm42600_timestamp_reset(struct inv_icm42600_timestamp *ts)
> +{
> + const struct inv_icm42600_timestamp_interval interval_init = {0LL, 0LL};
> +
> + ts->it = interval_init;
> + ts->timestamp = 0;
> +}
> +
> +#endif
^ permalink raw reply
* Re: [PATCH v2 2/3] media: rockchip: Introduce driver for Rockhip's camera interface
From: Ezequiel Garcia @ 2020-05-31 13:40 UTC (permalink / raw)
To: Maxime Chevallier
Cc: Mauro Carvalho Chehab, Robin Murphy, Rob Herring, Mark Rutland,
Heiko Stuebner, Hans Verkuil, devicetree,
Linux Kernel Mailing List, Paul Kocialkowski,
open list:ARM/Rockchip SoC..., Thomas Petazzoni, Miquel Raynal,
linux-arm-kernel, linux-media
In-Reply-To: <20200529130405.929429-3-maxime.chevallier@bootlin.com>
Hi Maxime,
Thanks for posting this patch. I think you can still improve it,
but it's a neat first try! :-)
On Fri, 29 May 2020 at 10:05, Maxime Chevallier
<maxime.chevallier@bootlin.com> wrote:
>
> Introduce a driver for the camera interface on some Rockchip platforms.
>
> This controller supports CSI2, Parallel and BT656 interfaces, but for
> now only the parallel interface could be tested, hence it's the only one
> that's supported in the first version of this driver.
>
I'm confused, you mention parallel as the only tested interface,
but the cover letters mentions PAL. Doesn't PAL mean BT.656
or am I completely lost?
(I am not super familiar with parallel sensors).
> This controller can be fond on PX30, RK1808, RK3128, RK3288 and RK3288,
> but for now it's only be tested on PX30.
>
My RK3288 and RK3326 (i.e. PX30) refer to this IP block as "Video
Input interface".
I am wondering if it won't be clearer for developers / users if we
rename the driver
to rockchip-vip (and of course s/cif/vip and s/CIF/VIP).
> Most of this driver was written follwing the BSP driver from rockchip,
> removing the parts that either didn't fit correctly the guidelines, or
> that couldn't be tested.
>
> This basic version doesn't support cropping nor scaling, and is only
> designed with one sensor being attached to it a any time.
>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>
> Changes since V1 :
>
> - Convert to the bulk APIs for clocks and resets
Note that the bulk API clock conversion was not
properly done.
> - remove useless references to priv data
> - Move around some init functions at probe time
> - Upate some helpers to more suitable ones
>
> Here is the output from v4l2-compliance. There are no fails in the final
> summary, but there is one in the output that I didn't catch previously.
>
> Still, here's the V2 in the meantime, if you have any further reviews
> ompliance SHA: not available, 64 bits
>
> Compliance test for rkcif device /dev/video0:
>
> Driver Info:
> Driver name : rkcif
> Card type : rkcif
> Bus info : platform:ff490000.cif
> Driver version : 5.7.0
> Capabilities : 0x84201000
> Video Capture Multiplanar
> Streaming
> Extended Pix Format
> Device Capabilities
> Device Caps : 0x04201000
> Video Capture Multiplanar
> Streaming
> Extended Pix Format
> Media Driver Info:
> Driver name : rkcif
> Model : rkcif
> Serial :
> Bus info :
> Media version : 5.7.0
> Hardware revision: 0x00000000 (0)
> Driver version : 5.7.0
> Interface Info:
> ID : 0x03000002
> Type : V4L Video
> Entity Info:
> ID : 0x00000001 (1)
> Name : video_rkcif
> Function : V4L2 I/O
> Pad 0x01000004 : 0: Sink
> Link 0x02000007: from remote pad 0x1000006 of entity 'tw9900 2-0044': Data, Enabled
>
> Required ioctls:
> test MC information (see 'Media Driver Info' above): OK
> test VIDIOC_QUERYCAP: OK
>
> Allow for multiple opens:
> test second /dev/video0 open: OK
> test VIDIOC_QUERYCAP: OK
> test VIDIOC_G/S_PRIORITY: OK
> test for unlimited opens: OK
>
> Debug ioctls:
> test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
> test VIDIOC_LOG_STATUS: OK (Not Supported)
>
> Input ioctls:
> test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> test VIDIOC_ENUMAUDIO: OK (Not Supported)
> test VIDIOC_G/S/ENUMINPUT: OK
> test VIDIOC_G/S_AUDIO: OK (Not Supported)
> Inputs: 1 Audio Inputs: 0 Tuners: 0
>
> Output ioctls:
> test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> Outputs: 0 Audio Outputs: 0 Modulators: 0
>
> Input/Output configuration ioctls:
> test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
> test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> test VIDIOC_G/S_EDID: OK (Not Supported)
>
> Control ioctls (Input 0):
> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
> test VIDIOC_QUERYCTRL: OK
> test VIDIOC_G/S_CTRL: OK
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK
> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> Standard Controls: 0 Private Controls: 0
>
> Format ioctls (Input 0):
> test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> test VIDIOC_G/S_PARM: OK (Not Supported)
> test VIDIOC_G_FBUF: OK (Not Supported)
> test VIDIOC_G_FMT: OK
> test VIDIOC_TRY_FMT: OK
> test VIDIOC_S_FMT: OK
> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> test Cropping: OK (Not Supported)
> test Composing: OK (Not Supported)
> fail: v4l2-test-formats.cpp(1772): node->can_scale && node->frmsizes_count[v4l_format_g_pixelformat(&cur)]
> test Scaling: OK
>
> Codec ioctls (Input 0):
> test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
> test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
>
> Buffer ioctls (Input 0):
> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> test VIDIOC_EXPBUF: OK
> test Requests: OK (Not Supported)
>
> Total for rkcif device /dev/video0: 45, Succeeded: 45, Failed: 0, Warnings: 0
>
> drivers/media/platform/Kconfig | 13 +
> drivers/media/platform/Makefile | 1 +
> drivers/media/platform/rockchip/cif/Makefile | 3 +
> drivers/media/platform/rockchip/cif/capture.c | 1170 +++++++++++++++++
> drivers/media/platform/rockchip/cif/dev.c | 358 +++++
> drivers/media/platform/rockchip/cif/dev.h | 213 +++
> drivers/media/platform/rockchip/cif/regs.h | 256 ++++
> 7 files changed, 2014 insertions(+)
> create mode 100644 drivers/media/platform/rockchip/cif/Makefile
> create mode 100644 drivers/media/platform/rockchip/cif/capture.c
> create mode 100644 drivers/media/platform/rockchip/cif/dev.c
> create mode 100644 drivers/media/platform/rockchip/cif/dev.h
> create mode 100644 drivers/media/platform/rockchip/cif/regs.h
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index e01bbb9dd1c1..d4ec5e36bca7 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -460,6 +460,19 @@ config VIDEO_ROCKCHIP_RGA
>
> To compile this driver as a module choose m here.
>
> +config VIDEO_ROCKCHIP_CIF
> + tristate "Rockchip Camera Interface"
> + depends on VIDEO_DEV && VIDEO_V4L2
> + depends on ARCH_ROCKCHIP || COMPILE_TEST
> + select VIDEOBUF2_DMA_SG
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_FWNODE
> + select V4L2_MEM2MEM_DEV
> + help
> + This is a v4l2 driver for Rockchip SOC Camera interface.
> +
> + To compile this driver as a module choose m here.
> +
> config VIDEO_TI_VPE
> tristate "TI VPE (Video Processing Engine) driver"
> depends on VIDEO_DEV && VIDEO_V4L2
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index d13db96e3015..67e7ac034be1 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -68,6 +68,7 @@ obj-$(CONFIG_VIDEO_RENESAS_JPU) += rcar_jpu.o
> obj-$(CONFIG_VIDEO_RENESAS_VSP1) += vsp1/
>
> obj-$(CONFIG_VIDEO_ROCKCHIP_RGA) += rockchip/rga/
> +obj-$(CONFIG_VIDEO_ROCKCHIP_CIF) += rockchip/cif/
>
> obj-y += omap/
>
> diff --git a/drivers/media/platform/rockchip/cif/Makefile b/drivers/media/platform/rockchip/cif/Makefile
> new file mode 100644
> index 000000000000..727990824316
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_VIDEO_ROCKCHIP_CIF) += video_rkcif.o
> +video_rkcif-objs += dev.o capture.o
> diff --git a/drivers/media/platform/rockchip/cif/capture.c b/drivers/media/platform/rockchip/cif/capture.c
> new file mode 100644
> index 000000000000..adab6704129f
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/capture.c
> @@ -0,0 +1,1170 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip CIF Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + * Copyright (C) 2020 Maxime Chevallier <maxime.chevallier@bootlin.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/reset.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-fh.h>
> +#include <media/v4l2-fwnode.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-mc.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/videobuf2-dma-contig.h>
> +
> +#include "dev.h"
> +#include "regs.h"
> +
> +#define CIF_REQ_BUFS_MIN 1
> +#define CIF_MIN_WIDTH 64
> +#define CIF_MIN_HEIGHT 64
> +#define CIF_MAX_WIDTH 8192
> +#define CIF_MAX_HEIGHT 8192
> +
> +#define RKCIF_PLANE_Y 0
> +#define RKCIF_PLANE_CBCR 1
> +
> +#define CIF_FETCH_Y_LAST_LINE(VAL) ((VAL) & 0x1fff)
> +/* Check if swap y and c in bt1120 mode */
> +#define CIF_FETCH_IS_Y_FIRST(VAL) ((VAL) & 0xf)
> +
> +/* Get xsubs and ysubs for fourcc formats
> + *
> + * @xsubs: horizontal color samples in a 4*4 matrix, for yuv
> + * @ysubs: vertical color samples in a 4*4 matrix, for yuv
> + */
> +static int fcc_xysubs(u32 fcc, u32 *xsubs, u32 *ysubs)
You should avoid doing this math in the driver,
and instead use the nice core helpers
v4l2_fill_pixfmt_mp and v4l2_fill_pixfmt.
> +{
> + switch (fcc) {
> + case V4L2_PIX_FMT_NV16:
> + case V4L2_PIX_FMT_NV61:
> + *xsubs = 2;
> + *ysubs = 1;
> + break;
> + case V4L2_PIX_FMT_NV21:
> + case V4L2_PIX_FMT_NV12:
> + *xsubs = 2;
> + *ysubs = 2;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static const struct cif_output_fmt out_fmts[] = {
> + {
> + .fourcc = V4L2_PIX_FMT_NV16,
> + .cplanes = 2,
> + .mplanes = 1,
Only mplanes = 1 formats are supported, please
drop it here, and drop any handling due to mplanes > 1.
> + .fmt_val = YUV_OUTPUT_422 | UV_STORAGE_ORDER_UVUV,
> + .bpp = { 8, 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_NV61,
> + .fmt_val = YUV_OUTPUT_422 | UV_STORAGE_ORDER_VUVU,
> + .cplanes = 2,
> + .mplanes = 1,
> + .bpp = { 8, 16 },
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV12,
> + .fmt_val = YUV_OUTPUT_420 | UV_STORAGE_ORDER_UVUV,
> + .cplanes = 2,
> + .mplanes = 1,
> + .bpp = { 8, 16 },
> + .mbus = MEDIA_BUS_FMT_UYVY8_2X8,
> + },
> + {
> + .fourcc = V4L2_PIX_FMT_NV21,
> + .fmt_val = YUV_OUTPUT_420 | UV_STORAGE_ORDER_VUVU,
> + .cplanes = 2,
> + .mplanes = 1,
> + .bpp = { 8, 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_RGB24,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 24 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_RGB565,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_BGR666,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 18 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SRGGB8,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 8 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGRBG8,
That is odd: how does the driver distinguish
V4L2_PIX_FMT_SGRBG8 from V4L2_PIX_FMT_SRGGB8?
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 8 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGBRG8,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 8 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SBGGR8,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 8 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SRGGB10,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGRBG10,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGBRG10,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SBGGR10,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SRGGB12,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGRBG12,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SGBRG12,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SBGGR12,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_SBGGR16,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }, {
> + .fourcc = V4L2_PIX_FMT_Y16,
> + .cplanes = 1,
> + .mplanes = 1,
> + .bpp = { 16 },
> + }
> +};
> +
> +static const struct cif_input_fmt in_fmts[] = {
> + {
> + .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_YUYV,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_YUYV8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_YUYV,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_INTERLACED,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_YVYU,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_YVYU8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_YVYU,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_INTERLACED,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_UYVY,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_UYVY8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_UYVY,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_INTERLACED,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_VYUY,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_VYUY8_2X8,
> + .dvp_fmt_val = YUV_INPUT_422 | YUV_INPUT_ORDER_VYUY,
> + .csi_fmt_val = CSI_WRDDR_TYPE_YUV422,
> + .fmt_type = CIF_FMT_TYPE_YUV,
> + .field = V4L2_FIELD_INTERLACED,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SBGGR8_1X8,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGBRG8_1X8,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGRBG8_1X8,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SRGGB8_1X8,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SBGGR12_1X12,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGBRG12_1X12,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SGRBG12_1X12,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_SRGGB12_1X12,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_RGB888_1X24,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RGB888,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_Y8_1X8,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_8,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW8,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_Y10_1X10,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_10,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW10,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }, {
> + .mbus_code = MEDIA_BUS_FMT_Y12_1X12,
> + .dvp_fmt_val = INPUT_MODE_RAW | RAW_DATA_WIDTH_12,
> + .csi_fmt_val = CSI_WRDDR_TYPE_RAW12,
> + .fmt_type = CIF_FMT_TYPE_RAW,
> + .field = V4L2_FIELD_NONE,
> + }
> +};
> +
> +static const struct
> +cif_input_fmt *get_input_fmt(struct v4l2_subdev *sd)
> +{
> + struct v4l2_subdev_format fmt;
> + int ret;
> + u32 i;
> +
> + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> + fmt.pad = 0;
> + ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
> + if (ret < 0) {
> + v4l2_warn(sd->v4l2_dev,
> + "sensor fmt invalid, set to default size\n");
> + goto set_default;
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(in_fmts); i++)
> + if (fmt.format.code == in_fmts[i].mbus_code &&
> + fmt.format.field == in_fmts[i].field)
> + return &in_fmts[i];
> +
> + v4l2_err(sd->v4l2_dev, "remote sensor mbus code not supported\n");
> +
> +set_default:
> + return NULL;
> +}
> +
> + static const struct
Tab is off. Note that Hans already mentioned this on v1.
> +cif_output_fmt *find_output_fmt(struct rkcif_stream *stream, u32 pixelfmt)
> +{
> + const struct cif_output_fmt *fmt;
> + u32 i;
> +
> + for (i = 0; i < ARRAY_SIZE(out_fmts); i++) {
> + fmt = &out_fmts[i];
> + if (fmt->fourcc == pixelfmt)
> + return fmt;
> + }
> +
> + return NULL;
> +}
> +
> +/***************************** stream operations ******************************/
> +static void rkcif_assign_new_buffer_oneframe(struct rkcif_stream *stream)
> +{
> + struct rkcif_dummy_buffer *dummy_buf = &stream->dummy_buf;
> + struct rkcif_device *dev = stream->cifdev;
> + void __iomem *base = dev->base_addr;
> +
> + /* Set up an empty buffer for the next frame */
> + spin_lock(&stream->vbq_lock);
> + if (!list_empty(&stream->buf_head)) {
> + stream->curr_buf = list_first_entry(&stream->buf_head,
> + struct rkcif_buffer, queue);
> + list_del(&stream->curr_buf->queue);
> + } else {
> + stream->curr_buf = NULL;
> + }
> + spin_unlock(&stream->vbq_lock);
> +
> + if (stream->curr_buf) {
> + write_cif_reg(base, CIF_FRM0_ADDR_Y,
> + stream->curr_buf->buff_addr[RKCIF_PLANE_Y]);
> + write_cif_reg(base, CIF_FRM0_ADDR_UV,
> + stream->curr_buf->buff_addr[RKCIF_PLANE_CBCR]);
> + write_cif_reg(base, CIF_FRM1_ADDR_Y,
> + stream->curr_buf->buff_addr[RKCIF_PLANE_Y]);
> + write_cif_reg(base, CIF_FRM1_ADDR_UV,
> + stream->curr_buf->buff_addr[RKCIF_PLANE_CBCR]);
> + } else {
> + write_cif_reg(base, CIF_FRM0_ADDR_Y, dummy_buf->dma_addr);
> + write_cif_reg(base, CIF_FRM0_ADDR_UV, dummy_buf->dma_addr);
> + write_cif_reg(base, CIF_FRM1_ADDR_Y, dummy_buf->dma_addr);
> + write_cif_reg(base, CIF_FRM1_ADDR_UV, dummy_buf->dma_addr);
Frame 0 and 1 would seem to indicate you can do better
by implementing some double buffering. Have you tried that?
> + }
> +}
> +
> +static void rkcif_stream_stop(struct rkcif_stream *stream)
> +{
> + struct rkcif_device *cif_dev = stream->cifdev;
> + void __iomem *base = cif_dev->base_addr;
> + u32 val;
> +
> + val = read_cif_reg(base, CIF_CTRL);
> + write_cif_reg(base, CIF_CTRL, val & (~ENABLE_CAPTURE));
> + write_cif_reg(base, CIF_INTEN, 0x0);
> + write_cif_reg(base, CIF_INTSTAT, 0x3ff);
> + write_cif_reg(base, CIF_FRAME_STATUS, 0x0);
> +
> + stream->state = RKCIF_STATE_READY;
> +}
> +
> +static int rkcif_queue_setup(struct vb2_queue *queue,
> + unsigned int *num_buffers,
> + unsigned int *num_planes,
> + unsigned int sizes[],
> + struct device *alloc_devs[])
> +{
> + struct rkcif_stream *stream = queue->drv_priv;
> + const struct v4l2_pix_format_mplane *pixm;
> + const struct cif_output_fmt *cif_fmt;
> + u32 i;
> +
Better to use u32 for variables that really need a fixed width,
and so use e.g. unsigned int here.
> + pixm = &stream->pixm;
> + cif_fmt = stream->cif_fmt_out;
> +
> + if (*num_planes) {
> + if (*num_planes != cif_fmt->mplanes)
> + return -EINVAL;
> +
> + for (i = 0; i < cif_fmt->mplanes; i++)
> + if (sizes[i] < pixm->plane_fmt[i].sizeimage)
> + return -EINVAL;
> + return 0;
> + }
> +
> + *num_planes = cif_fmt->mplanes;
> +
> + for (i = 0; i < cif_fmt->mplanes; i++) {
> + const struct v4l2_plane_pix_format *plane_fmt;
> +
> + plane_fmt = &pixm->plane_fmt[i];
> + sizes[i] = plane_fmt->sizeimage;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * The vb2_buffer are stored in rkcif_buffer, in order to unify
> + * mplane buffer and none-mplane buffer.
> + */
> +static void rkcif_buf_queue(struct vb2_buffer *vb)
> +{
> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> + struct rkcif_buffer *cifbuf = to_rkcif_buffer(vbuf);
> + struct vb2_queue *queue = vb->vb2_queue;
> + struct rkcif_stream *stream = queue->drv_priv;
> + struct v4l2_pix_format_mplane *pixm = &stream->pixm;
> + const struct cif_output_fmt *fmt = stream->cif_fmt_out;
> + unsigned long lock_flags = 0;
> + int i;
> +
> + memset(cifbuf->buff_addr, 0, sizeof(cifbuf->buff_addr));
> + /* If mplanes > 1, every c-plane has its own m-plane,
> + * otherwise, multiple c-planes are in the same m-plane
> + */
> + for (i = 0; i < fmt->mplanes; i++)
> + cifbuf->buff_addr[i] = vb2_dma_contig_plane_dma_addr(vb, i);
> +
> + if (fmt->mplanes == 1) {
> + for (i = 0; i < fmt->cplanes - 1; i++)
> + cifbuf->buff_addr[i + 1] = cifbuf->buff_addr[i] +
> + pixm->plane_fmt[i].bytesperline * pixm->height;
> + }
> +
> + spin_lock_irqsave(&stream->vbq_lock, lock_flags);
> + list_add_tail(&cifbuf->queue, &stream->buf_head);
> + spin_unlock_irqrestore(&stream->vbq_lock, lock_flags);
> +}
> +
> +static int rkcif_create_dummy_buf(struct rkcif_stream *stream)
> +{
As Hans pointed out, dummy buffer needs some explanation
and details.
> + struct rkcif_dummy_buffer *dummy_buf = &stream->dummy_buf;
> + struct rkcif_device *dev = stream->cifdev;
> +
> + /* get a maximum plane size */
> + dummy_buf->size = max3(stream->pixm.plane_fmt[0].bytesperline *
> + stream->pixm.height,
> + stream->pixm.plane_fmt[1].sizeimage,
> + stream->pixm.plane_fmt[2].sizeimage);
> +
> + dummy_buf->vaddr = dma_alloc_coherent(dev->dev, dummy_buf->size,
> + &dummy_buf->dma_addr,
> + GFP_KERNEL);
> + if (!dummy_buf->vaddr) {
> + v4l2_err(&dev->v4l2_dev,
> + "Failed to allocate the memory for dummy buffer\n");
> + return -ENOMEM;
> + }
> +
> + v4l2_info(&dev->v4l2_dev, "Allocate dummy buffer, size: 0x%08x\n",
> + dummy_buf->size);
> +
Drop this v4l2_info.
> + return 0;
> +}
> +
> +static void rkcif_destroy_dummy_buf(struct rkcif_stream *stream)
> +{
> + struct rkcif_dummy_buffer *dummy_buf = &stream->dummy_buf;
> + struct rkcif_device *dev = stream->cifdev;
> +
> + dma_free_coherent(dev->dev, dummy_buf->size,
> + dummy_buf->vaddr, dummy_buf->dma_addr);
> +}
> +
> +static void rkcif_stop_streaming(struct vb2_queue *queue)
> +{
> + struct rkcif_stream *stream = queue->drv_priv;
> + struct rkcif_device *dev = stream->cifdev;
> + struct rkcif_buffer *buf;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + stream->stopping = true;
> + ret = wait_event_timeout(stream->wq_stopped,
> + stream->state != RKCIF_STATE_STREAMING,
> + msecs_to_jiffies(1000));
> + if (!ret) {
> + rkcif_stream_stop(stream);
> + stream->stopping = false;
> + }
> + pm_runtime_put(dev->dev);
> +
> + /* stop the sub device*/
> + sd = dev->sensor.sd;
> + v4l2_subdev_call(sd, video, s_stream, 0);
> + v4l2_subdev_call(sd, core, s_power, 0);
> +
> + /* release buffers */
> + if (stream->curr_buf) {
> + list_add_tail(&stream->curr_buf->queue, &stream->buf_head);
> + stream->curr_buf = NULL;
> + }
> + if (stream->next_buf) {
Next buffer would seem to indicate some attempt at double-buffering,
but it's not really used.
> + list_add_tail(&stream->next_buf->queue, &stream->buf_head);
> + stream->next_buf = NULL;
> + }
> +
> + while (!list_empty(&stream->buf_head)) {
> + buf = list_first_entry(&stream->buf_head,
> + struct rkcif_buffer, queue);
> + list_del(&buf->queue);
> + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> + }
> +
> + rkcif_destroy_dummy_buf(stream);
> +}
> +
> +static u32 rkcif_determine_input_mode(struct rkcif_device *dev)
> +{
> + struct rkcif_sensor_info *sensor_info = &dev->sensor;
> + struct rkcif_stream *stream = &dev->stream;
> + v4l2_std_id std;
> + u32 mode = INPUT_MODE_YUV;
> + int ret;
> +
> + ret = v4l2_subdev_call(sensor_info->sd, video, querystd, &std);
> + if (ret == 0) {
> + /* retrieve std from sensor if exist */
> + switch (std) {
> + case V4L2_STD_NTSC:
> + mode = INPUT_MODE_NTSC;
> + break;
> + case V4L2_STD_PAL:
> + mode = INPUT_MODE_PAL;
> + break;
> + case V4L2_STD_ATSC:
> + mode = INPUT_MODE_BT1120;
> + break;
> + default:
> + v4l2_err(&dev->v4l2_dev,
> + "std: %lld is not supported", std);
> + }
> + } else {
> + /* determine input mode by mbus_code (fmt_type) */
> + switch (stream->cif_fmt_in->fmt_type) {
> + case CIF_FMT_TYPE_YUV:
> + mode = INPUT_MODE_YUV;
> + break;
> + case CIF_FMT_TYPE_RAW:
> + mode = INPUT_MODE_RAW;
> + break;
> + }
> + }
> +
> + return mode;
> +}
> +
> +static inline u32 rkcif_scl_ctl(struct rkcif_stream *stream)
> +{
> + u32 fmt_type = stream->cif_fmt_in->fmt_type;
> +
> + return (fmt_type == CIF_FMT_TYPE_YUV) ?
> + ENABLE_YUV_16BIT_BYPASS : ENABLE_RAW_16BIT_BYPASS;
> +}
> +
> +static int rkcif_stream_start(struct rkcif_stream *stream)
> +{
> + u32 val, mbus_flags, href_pol, vsync_pol,
> + xfer_mode = 0, yc_swap = 0, skip_top = 0;
Tab is off here.
> + struct rkcif_device *dev = stream->cifdev;
> + struct rkcif_sensor_info *sensor_info;
> + void __iomem *base = dev->base_addr;
> +
> + sensor_info = &dev->sensor;
> + stream->frame_idx = 0;
> +
> + mbus_flags = sensor_info->mbus.flags;
> + href_pol = (mbus_flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) ?
> + HSY_HIGH_ACTIVE : HSY_LOW_ACTIVE;
> + vsync_pol = (mbus_flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) ?
> + VSY_HIGH_ACTIVE : VSY_LOW_ACTIVE;
> +
> + if (rkcif_determine_input_mode(dev) == INPUT_MODE_BT1120) {
> + if (stream->cif_fmt_in->field == V4L2_FIELD_NONE)
> + xfer_mode = BT1120_TRANSMIT_PROGRESS;
> + else
> + xfer_mode = BT1120_TRANSMIT_INTERFACE;
> + if (!CIF_FETCH_IS_Y_FIRST(stream->cif_fmt_in->dvp_fmt_val))
> + yc_swap = BT1120_YC_SWAP;
> + }
> +
> + val = vsync_pol | href_pol | rkcif_determine_input_mode(dev) |
> + stream->cif_fmt_out->fmt_val | stream->cif_fmt_in->dvp_fmt_val |
> + xfer_mode | yc_swap;
> + write_cif_reg(base, CIF_FOR, val);
> + val = stream->pixm.width;
> + if (stream->cif_fmt_in->fmt_type == CIF_FMT_TYPE_RAW)
> + val = stream->pixm.width * 2;
> + write_cif_reg(base, CIF_VIR_LINE_WIDTH, val);
> + write_cif_reg(base, CIF_SET_SIZE,
> + stream->pixm.width | (stream->pixm.height << 16));
> +
> + v4l2_subdev_call(sensor_info->sd, sensor, g_skip_top_lines, &skip_top);
> +
> + write_cif_reg(base, CIF_CROP, skip_top << CIF_CROP_Y_SHIFT);
> + write_cif_reg(base, CIF_FRAME_STATUS, FRAME_STAT_CLS);
> + write_cif_reg(base, CIF_INTSTAT, INTSTAT_CLS);
> + write_cif_reg(base, CIF_SCL_CTRL, rkcif_scl_ctl(stream));
> +
> + rkcif_assign_new_buffer_oneframe(stream);
> +
> + write_cif_reg(base, CIF_INTEN, FRAME_END_EN | LINE_ERR_EN |
> + PST_INF_FRAME_END);
> +
> + if (dev->data->chip_id == CHIP_RK1808_CIF &&
> + rkcif_determine_input_mode(dev) == INPUT_MODE_BT1120)
> + write_cif_reg(base, CIF_CTRL,
> + AXI_BURST_16 | MODE_PINGPONG | ENABLE_CAPTURE);
> + else
> + write_cif_reg(base, CIF_CTRL,
> + AXI_BURST_16 | MODE_ONEFRAME | ENABLE_CAPTURE);
> +
> + stream->state = RKCIF_STATE_STREAMING;
> +
> + return 0;
> +}
> +
> +static int rkcif_start_streaming(struct vb2_queue *queue, unsigned int count)
> +{
> + struct rkcif_stream *stream = queue->drv_priv;
> + struct rkcif_device *dev = stream->cifdev;
> + struct v4l2_device *v4l2_dev = &dev->v4l2_dev;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + if (WARN_ON(stream->state != RKCIF_STATE_READY)) {
This check should not be needed.
> + ret = -EBUSY;
> + v4l2_err(v4l2_dev, "stream in busy state\n");
> + goto destroy_buf;
> + }
> +
> + stream->cif_fmt_in = get_input_fmt(dev->sensor.sd);
> +
> + ret = rkcif_create_dummy_buf(stream);
> + if (ret < 0) {
> + v4l2_err(v4l2_dev, "Failed to create dummy_buf, %d\n", ret);
> + goto destroy_buf;
> + }
> +
> + ret = pm_runtime_get_sync(dev->dev);
> + if (ret < 0) {
> + v4l2_err(v4l2_dev, "Failed to get runtime pm, %d\n", ret);
> + goto destroy_dummy_buf;
> + }
> +
> + /* start sub-devices */
> + sd = dev->sensor.sd;
> + ret = v4l2_subdev_call(sd, core, s_power, 1);
> + if (ret < 0 && ret != -ENOIOCTLCMD)
> + goto runtime_put;
> + ret = v4l2_subdev_call(sd, video, s_stream, 1);
> + if (ret < 0)
> + goto subdev_poweroff;
> +
> + ret = rkcif_stream_start(stream);
> + if (ret < 0)
> + goto stop_stream;
> +
> + return 0;
> +
> +stop_stream:
> + rkcif_stream_stop(stream);
> +subdev_poweroff:
> + v4l2_subdev_call(sd, core, s_power, 0);
> +runtime_put:
> + pm_runtime_put(dev->dev);
> +destroy_dummy_buf:
> + rkcif_destroy_dummy_buf(stream);
> +destroy_buf:
> + while (!list_empty(&stream->buf_head)) {
Move this to a helper?
> + struct rkcif_buffer *buf;
> +
> + buf = list_first_entry(&stream->buf_head,
> + struct rkcif_buffer, queue);
> + list_del(&buf->queue);
> + vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_QUEUED);
> + }
> +
> + return ret;
> +}
> +
> +static struct vb2_ops rkcif_vb2_ops = {
> + .queue_setup = rkcif_queue_setup,
> + .buf_queue = rkcif_buf_queue,
> + .wait_prepare = vb2_ops_wait_prepare,
> + .wait_finish = vb2_ops_wait_finish,
> + .stop_streaming = rkcif_stop_streaming,
> + .start_streaming = rkcif_start_streaming,
> +};
> +
> +static int rkcif_init_vb2_queue(struct vb2_queue *q,
> + struct rkcif_stream *stream,
> + enum v4l2_buf_type buf_type)
> +{
> + q->type = buf_type;
> + q->io_modes = VB2_MMAP | VB2_DMABUF;
> + q->drv_priv = stream;
> + q->ops = &rkcif_vb2_ops;
> + q->mem_ops = &vb2_dma_contig_memops;
I can't find any CPU mapping, so maybe you'll want to
have q->dma_attrs = DMA_ATTR_NO_KERNEL_MAPPING.
> + q->buf_struct_size = sizeof(struct rkcif_buffer);
> + q->min_buffers_needed = CIF_REQ_BUFS_MIN;
> + q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
> + q->lock = &stream->vlock;
> + q->dev = stream->cifdev->dev;
> +
> + return vb2_queue_init(q);
> +}
> +
> +static void rkcif_set_fmt(struct rkcif_stream *stream,
> + struct v4l2_pix_format_mplane *pixm,
> + bool try)
> +{
> + const struct cif_output_fmt *fmt;
> + struct v4l2_rect input_rect;
> + unsigned int imagesize = 0, planes;
> + u32 xsubs = 1, ysubs = 1, i;
> +
> + fmt = find_output_fmt(stream, pixm->pixelformat);
> + if (!fmt)
> + fmt = &out_fmts[0];
> +
> + input_rect.width = CIF_MAX_WIDTH;
> + input_rect.height = CIF_MAX_HEIGHT;
> +
> + pixm->width = clamp_t(u32, pixm->width,
> + CIF_MIN_WIDTH, input_rect.width);
> + pixm->height = clamp_t(u32, pixm->height,
> + CIF_MIN_HEIGHT, input_rect.height);
> +
> + pixm->num_planes = fmt->mplanes;
> + pixm->quantization = V4L2_QUANTIZATION_DEFAULT;
> + pixm->colorspace = V4L2_COLORSPACE_SRGB;
> +
See Hans' comment on v1.
> + pixm->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(pixm->colorspace);
> + pixm->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(pixm->colorspace);
> +
> + pixm->pixelformat = fmt->fourcc;
> + pixm->field = V4L2_FIELD_NONE;
> +
> + /* calculate plane size and image size */
> + fcc_xysubs(fmt->fourcc, &xsubs, &ysubs);
> +
> + planes = fmt->cplanes ? fmt->cplanes : fmt->mplanes;
> +
> + for (i = 0; i < planes; i++) {
> + struct v4l2_plane_pix_format *plane_fmt;
> + int width, height, bpl, size;
> +
> + if (i == 0) {
> + width = pixm->width;
> + height = pixm->height;
> + } else {
> + width = pixm->width / xsubs;
> + height = pixm->height / ysubs;
> + }
> +
> + bpl = width * fmt->bpp[i] / 8;
> + size = bpl * height;
> + imagesize += size;
> +
> + if (fmt->mplanes > i) {
> + /* Set bpl and size for each mplane */
> + plane_fmt = pixm->plane_fmt + i;
> + plane_fmt->bytesperline = bpl;
> + plane_fmt->sizeimage = size;
> + }
> + }
> +
> + /* convert to non-MPLANE format.
> + * It's important since we want to unify non-MPLANE
I suggest to only support MPLANE API and make your life simpler.
> + * and MPLANE.
> + */
> + if (fmt->mplanes == 1)
> + pixm->plane_fmt[0].sizeimage = imagesize;
> +
> + if (!try) {
> + stream->cif_fmt_out = fmt;
> + stream->pixm = *pixm;
> + }
> +}
> +
> +void rkcif_stream_init(struct rkcif_device *dev)
> +{
> + struct rkcif_stream *stream = &dev->stream;
> + struct v4l2_pix_format_mplane pixm;
> +
> + memset(stream, 0, sizeof(*stream));
> + memset(&pixm, 0, sizeof(pixm));
> + stream->cifdev = dev;
> +
> + INIT_LIST_HEAD(&stream->buf_head);
> + spin_lock_init(&stream->vbq_lock);
> + stream->state = RKCIF_STATE_READY;
> + init_waitqueue_head(&stream->wq_stopped);
> +
> + /* Set default format */
> + pixm.pixelformat = V4L2_PIX_FMT_NV12;
> + pixm.width = RKCIF_DEFAULT_WIDTH;
> + pixm.height = RKCIF_DEFAULT_HEIGHT;
> + rkcif_set_fmt(stream, &pixm, false);
> +
> + stream->crop.left = 0;
> + stream->crop.top = 0;
> + stream->crop.width = 10;
> + stream->crop.height = 10;
As already mentioned by Hans on the v1, please drop.
> +}
> +
> +static int rkcif_fh_open(struct file *filp)
> +{
> + struct video_device *vdev = video_devdata(filp);
> + struct rkcif_stream *stream = to_rkcif_stream(vdev);
> + struct rkcif_device *cifdev = stream->cifdev;
> +
> + rkcif_soft_reset(cifdev);
> +
As already mentioned by Hans, looks problematic.
> + return v4l2_fh_open(filp);
> +}
> +
> +static const struct v4l2_file_operations rkcif_fops = {
> + .open = rkcif_fh_open,
> + .release = vb2_fop_release,
> + .unlocked_ioctl = video_ioctl2,
> + .poll = vb2_fop_poll,
> + .mmap = vb2_fop_mmap,
> +};
> +
> +static int rkcif_enum_input(struct file *file, void *priv,
> + struct v4l2_input *input)
Why supporting the Input ioctls?
> +{
> + if (input->index > 0)
> + return -EINVAL;
> +
> + input->type = V4L2_INPUT_TYPE_CAMERA;
> + strlcpy(input->name, "Camera", sizeof(input->name));
> +
> + return 0;
> +}
> +
> +static int rkcif_try_fmt_vid_cap_mplane(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct rkcif_stream *stream = video_drvdata(file);
> +
> + rkcif_set_fmt(stream, &f->fmt.pix_mp, true);
> +
> + return 0;
> +}
> +
> +static int rkcif_enum_fmt_vid_cap(struct file *file, void *priv,
> + struct v4l2_fmtdesc *f)
> +{
> + const struct cif_output_fmt *fmt = NULL;
> +
> + if (f->index >= ARRAY_SIZE(out_fmts))
> + return -EINVAL;
> +
> + fmt = &out_fmts[f->index];
> + f->pixelformat = fmt->fourcc;
> +
> + return 0;
> +}
> +
> +static int rkcif_s_fmt_vid_cap_mplane(struct file *file,
> + void *priv, struct v4l2_format *f)
> +{
> + struct rkcif_stream *stream = video_drvdata(file);
> +
> + rkcif_set_fmt(stream, &f->fmt.pix_mp, false);
> +
> + return 0;
> +}
> +
> +static int rkcif_g_fmt_vid_cap_mplane(struct file *file, void *fh,
> + struct v4l2_format *f)
> +{
> + struct rkcif_stream *stream = video_drvdata(file);
> +
> + f->fmt.pix_mp = stream->pixm;
> +
> + return 0;
> +}
> +
> +static int rkcif_querycap(struct file *file, void *priv,
> + struct v4l2_capability *cap)
> +{
> + struct rkcif_stream *stream = video_drvdata(file);
> + struct device *dev = stream->cifdev->dev;
> +
> + strlcpy(cap->driver, dev->driver->name, sizeof(cap->driver));
> + strlcpy(cap->card, dev->driver->name, sizeof(cap->card));
> + snprintf(cap->bus_info, sizeof(cap->bus_info),
> + "platform:%s", dev_name(dev));
> +
> + return 0;
> +}
> +
> +static int rkcif_enum_framesizes(struct file *file, void *fh,
> + struct v4l2_frmsizeenum *fsize)
> +{
> + struct rkcif_stream *stream = video_drvdata(file);
> + struct rkcif_device *dev = stream->cifdev;
> + struct v4l2_subdev_frame_size_enum fse = {
> + .index = fsize->index,
> + .which = V4L2_SUBDEV_FORMAT_ACTIVE,
> + };
> + const struct cif_output_fmt *fmt;
> + int ret;
> +
> + if (!dev->sensor.sd)
> + return -EINVAL;
> +
> + fmt = find_output_fmt(stream, fsize->pixel_format);
> + if (!fmt)
> + return -EINVAL;
> +
> + fse.code = fmt->mbus;
> +
> + ret = v4l2_subdev_call(dev->sensor.sd, pad, enum_frame_size,
> + NULL, &fse);
> + if (ret)
> + return ret;
> +
> + fsize->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> + fsize->discrete.width = fse.max_width;
> + fsize->discrete.height = fse.max_height;
> +
> + return 0;
> +}
> +
> +static int rkcif_g_input(struct file *file, void *fh, unsigned int *i)
> +{
> + *i = 0;
> + return 0;
> +}
> +
> +static int rkcif_s_input(struct file *file, void *fh, unsigned int i)
> +{
> + if (i)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_ioctl_ops rkcif_v4l2_ioctl_ops = {
> + .vidioc_reqbufs = vb2_ioctl_reqbufs,
> + .vidioc_querybuf = vb2_ioctl_querybuf,
> + .vidioc_create_bufs = vb2_ioctl_create_bufs,
> + .vidioc_qbuf = vb2_ioctl_qbuf,
> + .vidioc_expbuf = vb2_ioctl_expbuf,
> + .vidioc_dqbuf = vb2_ioctl_dqbuf,
> + .vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> + .vidioc_streamon = vb2_ioctl_streamon,
> + .vidioc_streamoff = vb2_ioctl_streamoff,
> +
> + .vidioc_enum_fmt_vid_cap = rkcif_enum_fmt_vid_cap,
> + .vidioc_try_fmt_vid_cap_mplane = rkcif_try_fmt_vid_cap_mplane,
> + .vidioc_s_fmt_vid_cap_mplane = rkcif_s_fmt_vid_cap_mplane,
> + .vidioc_g_fmt_vid_cap_mplane = rkcif_g_fmt_vid_cap_mplane,
> + .vidioc_querycap = rkcif_querycap,
> + .vidioc_enum_framesizes = rkcif_enum_framesizes,
> +
> + .vidioc_enum_input = rkcif_enum_input,
> + .vidioc_g_input = rkcif_g_input,
> + .vidioc_s_input = rkcif_s_input,
> +};
> +
> +void rkcif_unregister_stream_vdev(struct rkcif_device *dev)
> +{
> + struct rkcif_stream *stream = &dev->stream;
> +
> + media_entity_cleanup(&stream->vdev.entity);
> + video_unregister_device(&stream->vdev);
> +}
> +
> +int rkcif_register_stream_vdev(struct rkcif_device *dev)
> +{
> + struct rkcif_stream *stream = &dev->stream;
> + struct v4l2_device *v4l2_dev = &dev->v4l2_dev;
> + struct video_device *vdev = &stream->vdev;
> + int ret;
> +
> + strlcpy(vdev->name, CIF_VIDEODEVICE_NAME, sizeof(vdev->name));
> + mutex_init(&stream->vlock);
> +
> + vdev->ioctl_ops = &rkcif_v4l2_ioctl_ops;
> + vdev->release = video_device_release_empty;
> + vdev->fops = &rkcif_fops;
> + vdev->minor = -1;
> + vdev->v4l2_dev = v4l2_dev;
> + vdev->lock = &stream->vlock;
> + vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE_MPLANE |
> + V4L2_CAP_STREAMING;
> + video_set_drvdata(vdev, stream);
> + vdev->vfl_dir = VFL_DIR_RX;
> + stream->pad.flags = MEDIA_PAD_FL_SINK;
> +
> + rkcif_init_vb2_queue(&stream->buf_queue, stream,
> + V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE |
> + V4L2_BUF_TYPE_VIDEO_CAPTURE);
As suggested, drop V4L2_BUF_TYPE_VIDEO_CAPTURE.
> + vdev->queue = &stream->buf_queue;
> + strscpy(vdev->name, KBUILD_MODNAME, sizeof(vdev->name));
> +
> + ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> + if (ret < 0) {
> + v4l2_err(v4l2_dev,
> + "video_register_device failed with error %d\n", ret);
> + return ret;
> + }
> +
> + ret = media_entity_pads_init(&vdev->entity, 1, &stream->pad);
> + if (ret < 0)
> + goto unreg;
> +
> + return 0;
> +unreg:
> + video_unregister_device(vdev);
> + return ret;
> +}
> +
> +static void rkcif_vb_done_oneframe(struct rkcif_stream *stream,
Why this "oneframe" naming? Is there some other mode planned?
> + struct vb2_v4l2_buffer *vb_done)
> +{
> + const struct cif_output_fmt *fmt = stream->cif_fmt_out;
> + u32 i;
> +
> + /* Dequeue a filled buffer */
> + for (i = 0; i < fmt->mplanes; i++) {
> + vb2_set_plane_payload(&vb_done->vb2_buf, i,
> + stream->pixm.plane_fmt[i].sizeimage);
> + }
> + vb_done->vb2_buf.timestamp = ktime_get_ns();
vb_done->vb2_buf.sequence needs to be set.
Also, you can use it to mark drop buffers. See below.
> + vb2_buffer_done(&vb_done->vb2_buf, VB2_BUF_STATE_DONE);
> +}
> +
> +void rkcif_irq_oneframe(struct rkcif_device *cif_dev)
> +{
> + struct rkcif_stream *stream = &cif_dev->stream;
> + u32 lastline, lastpix, ctl, cif_frmst, intstat;
> + void __iomem *base = cif_dev->base_addr;
> +
> + intstat = read_cif_reg(base, CIF_INTSTAT);
> + cif_frmst = read_cif_reg(base, CIF_FRAME_STATUS);
> + lastline = CIF_FETCH_Y_LAST_LINE(read_cif_reg(base, CIF_LAST_LINE));
> + lastpix = read_cif_reg(base, CIF_LAST_PIX);
> + ctl = read_cif_reg(base, CIF_CTRL);
> +
> + /* There are two irqs enabled:
> + * - PST_INF_FRAME_END: cif FIFO is ready, this is prior to FRAME_END
> + * - FRAME_END: cif has saved frame to memory, a frame ready
> + */
> +
> + if ((intstat & PST_INF_FRAME_END)) {
> + write_cif_reg(base, CIF_INTSTAT, PST_INF_FRAME_END_CLR);
> +
> + if (stream->stopping)
> + /* To stop CIF ASAP, before FRAME_END irq */
> + write_cif_reg(base, CIF_CTRL, ctl & (~ENABLE_CAPTURE));
> + }
> +
> + if ((intstat & LINE_ERR)) {
> + write_cif_reg(base, CIF_INTSTAT, LINE_ERR_CLR);
> +
> + if (stream->stopping) {
> + rkcif_stream_stop(stream);
> + stream->stopping = false;
> + wake_up(&stream->wq_stopped);
> + return;
> + }
> +
> + v4l2_err(&cif_dev->v4l2_dev,
> + "Bad frame, irq:0x%x frmst:0x%x size:%dx%d\n",
> + intstat, cif_frmst, lastline, lastpix);
> + /* Clear status to receive into the same buffer */
> + write_cif_reg(base, CIF_FRAME_STATUS, FRM0_STAT_CLS);
> + return;
> + }
> +
> +
> + if ((intstat & FRAME_END)) {
> + struct vb2_v4l2_buffer *vb_done = NULL;
> +
> + write_cif_reg(base, CIF_INTSTAT, FRAME_END_CLR);
> +
> + if (stream->stopping) {
> + rkcif_stream_stop(stream);
> + stream->stopping = false;
> + wake_up(&stream->wq_stopped);
> + return;
> + }
> +
> + if (lastline != stream->pixm.height ||
> + !(cif_frmst & CIF_F0_READY)) {
> + v4l2_err(&cif_dev->v4l2_dev,
> + "Bad frame, irq:0x%x frmst:0x%x size:%dx%d\n",
> + intstat, cif_frmst, lastline, lastpix);
> + /* Clear status to receive into the same buffer */
> + write_cif_reg(base, CIF_FRAME_STATUS, FRM0_STAT_CLS);
> + return;
> + }
> +
> + if (stream->curr_buf)
> + vb_done = &stream->curr_buf->vb;
> + rkcif_assign_new_buffer_oneframe(stream);
> +
> + /* In one-frame mode, must clear status manually to enable
> + * the next frame end irq
> + */
> + write_cif_reg(base, CIF_FRAME_STATUS, FRM0_STAT_CLS);
> +
> + if (vb_done)
> + rkcif_vb_done_oneframe(stream, vb_done);
> +
> + stream->frame_idx++;
Although it's currently unused, frame_idx will be useful to set the sequence.
Now, it would be interesting to keep track of captures to the dummy buffer
(i.e. dropped buffers) vs. captures to real buffers (captured frames).
Skipping dropped buffers from the sequence would allow userspace
applications to detect that something is going wrong.
You could also expose this captured/dropped counters in debugfs.
> + }
> +}
> diff --git a/drivers/media/platform/rockchip/cif/dev.c b/drivers/media/platform/rockchip/cif/dev.c
> new file mode 100644
> index 000000000000..dbd5fdbd1cef
> --- /dev/null
> +++ b/drivers/media/platform/rockchip/cif/dev.c
> @@ -0,0 +1,358 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Rockchip CIF Driver
> + *
> + * Copyright (C) 2018 Rockchip Electronics Co., Ltd.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_reserved_mem.h>
> +#include <linux/reset.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <media/v4l2-fwnode.h>
> +
> +#include "dev.h"
> +#include "regs.h"
> +
> +#define RKCIF_VERNO_LEN 10
> +
> +static int rkcif_create_links(struct rkcif_device *dev)
> +{
> + struct v4l2_subdev *sd = dev->sensor.sd;
> + int ret;
> +
> + ret = media_entity_get_fwnode_pad(&sd->entity, sd->fwnode,
> + MEDIA_PAD_FL_SOURCE);
> + if (ret)
> + return ret;
> +
> + ret = media_create_pad_link(&sd->entity, 0,
> + &dev->stream.vdev.entity, 0,
> + MEDIA_LNK_FL_ENABLED);
> + if (ret) {
> + dev_err(dev->dev, "failed to create link");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int subdev_notifier_complete(struct v4l2_async_notifier *notifier)
> +{
> + struct rkcif_device *dev;
> + int ret;
> +
> + dev = container_of(notifier, struct rkcif_device, notifier);
> +
> + mutex_lock(&dev->media_dev.graph_mutex);
> +
> + ret = rkcif_create_links(dev);
> + if (ret < 0)
> + goto unlock;
> +
> + ret = v4l2_device_register_subdev_nodes(&dev->v4l2_dev);
> + if (ret < 0)
> + goto unlock;
> +
> +unlock:
> + mutex_unlock(&dev->media_dev.graph_mutex);
> + return ret;
> +}
> +
> +static int subdev_notifier_bound(struct v4l2_async_notifier *notifier,
> + struct v4l2_subdev *subdev,
> + struct v4l2_async_subdev *asd)
> +{
> + struct rkcif_device *cif_dev = container_of(notifier,
> + struct rkcif_device, notifier);
> +
> + int pad;
> +
> + cif_dev->sensor.sd = subdev;
> + pad = media_entity_get_fwnode_pad(&subdev->entity, subdev->fwnode,
> + MEDIA_PAD_FL_SOURCE);
> + if (pad < 0)
> + return pad;
> +
> + cif_dev->sensor.pad = pad;
> +
> + return 0;
> +}
> +
> +static const struct v4l2_async_notifier_operations subdev_notifier_ops = {
> + .bound = subdev_notifier_bound,
> + .complete = subdev_notifier_complete,
> +};
> +
> +static int cif_subdev_notifier(struct rkcif_device *cif_dev)
> +{
> + struct v4l2_async_notifier *ntf = &cif_dev->notifier;
> + struct device *dev = cif_dev->dev;
> + struct v4l2_fwnode_endpoint vep = {
> + .bus_type = V4L2_MBUS_PARALLEL,
> + };
> + struct fwnode_handle *ep;
> + int ret;
> +
> + v4l2_async_notifier_init(ntf);
> +
> + ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
> + FWNODE_GRAPH_ENDPOINT_NEXT);
> + if (!ep)
> + return -EINVAL;
> +
> + ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> + if (ret)
> + return ret;
> +
> + ret = v4l2_async_notifier_add_fwnode_remote_subdev(ntf, ep,
> + &cif_dev->asd);
> + if (ret)
> + return ret;
> +
> + ntf->ops = &subdev_notifier_ops;
> +
> + fwnode_handle_put(ep);
> +
> + ret = v4l2_async_notifier_register(&cif_dev->v4l2_dev, ntf);
> + return ret;
> +}
> +
> +static int rkcif_register_platform_subdevs(struct rkcif_device *cif_dev)
> +{
> + int ret;
> +
> + ret = rkcif_register_stream_vdev(cif_dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = cif_subdev_notifier(cif_dev);
> + if (ret < 0) {
> + v4l2_err(&cif_dev->v4l2_dev,
> + "Failed to register subdev notifier(%d)\n", ret);
> + rkcif_unregister_stream_vdev(cif_dev);
> + }
> +
> + return 0;
> +}
> +
> +static struct clk_bulk_data px30_cif_clks[] = {
> + { .id = "aclk" },
> + { .id = "hclk" },
> + { .id = "pclkin" },
> +};
> +
> +static const struct cif_match_data px30_cif_match_data = {
> + .chip_id = CHIP_PX30_CIF,
> + .clks = px30_cif_clks,
> + .clks_num = ARRAY_SIZE(px30_cif_clks),
> +};
> +
> +static const struct of_device_id rkcif_plat_of_match[] = {
> + {
> + .compatible = "rockchip,px30-cif",
> + .data = &px30_cif_match_data,
> + },
> + {},
> +};
> +
> +static irqreturn_t rkcif_irq_handler(int irq, void *ctx)
> +{
> + struct device *dev = ctx;
> + struct rkcif_device *cif_dev = dev_get_drvdata(dev);
> +
> + rkcif_irq_oneframe(cif_dev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void rkcif_disable_sys_clk(struct rkcif_device *cif_dev)
> +{
> + int i;
> +
> + for (i = cif_dev->data->clks_num - 1; i >= 0; i--)
> + clk_disable_unprepare(cif_dev->clks[i]);
clk_bulk_disable_unprepare
> +}
> +
> +static int rkcif_enable_sys_clk(struct rkcif_device *cif_dev)
> +{
> + int i, ret = -EINVAL;
> +
> + for (i = 0; i < cif_dev->data->clks_num; i++) {
> + ret = clk_prepare_enable(cif_dev->clks[i]);
> +
clk_bulk_enable_prepare
> + if (ret < 0)
> + goto err;
> + }
> +
> + return 0;
> +
> +err:
> + for (--i; i >= 0; --i)
> + clk_disable_unprepare(cif_dev->clks[i]);
> +
> + return ret;
> +}
> +
> +void rkcif_soft_reset(struct rkcif_device *cif_dev)
> +{
> + reset_control_assert(cif_dev->cif_rst);
> +
> + udelay(5);
> +
> + reset_control_deassert(cif_dev->cif_rst);
> +}
> +
> +static int rkcif_plat_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct v4l2_device *v4l2_dev;
> + struct rkcif_device *cif_dev;
> + const struct cif_match_data *data;
> + int ret, irq;
> +
> + cif_dev = devm_kzalloc(dev, sizeof(*cif_dev), GFP_KERNEL);
> + if (!cif_dev)
> + return -ENOMEM;
> +
> + dev_set_drvdata(dev, cif_dev);
> + cif_dev->dev = dev;
> +
> + data = of_device_get_match_data(&pdev->dev);
> + if (!data)
> + return -EINVAL;
> +
> + cif_dev->data = data;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + ret = devm_request_irq(dev, irq, rkcif_irq_handler, IRQF_SHARED,
I guess it's shared with the IOMMU, or is this a mistake?
Note that you seem to always handle the interrupt.
> + dev_driver_string(dev), dev);
> + if (ret < 0) {
> + dev_err(dev, "request irq failed: %d\n", ret);
> + return ret;
> + }
> +
> + cif_dev->irq = irq;
> +
> + cif_dev->base_addr = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(cif_dev->base_addr))
> + return PTR_ERR(cif_dev->base_addr);
> +
> + ret = of_reserved_mem_device_init(dev);
Can't find any mention to this reserved memory in the bindings documentation.
Perhaps you can add an example and/or add some comments for this?
> + if (ret)
> + v4l2_info(v4l2_dev, "No reserved memory region assign to CIF\n");
> +
> + ret = devm_clk_bulk_get(dev, data->clks_num, data->clks);
> + if (ret)
> + return ret;
> +
> + cif_dev->cif_rst = devm_reset_control_array_get(dev, false, false);
> + if (IS_ERR(cif_dev->cif_rst))
> + return PTR_ERR(cif_dev->cif_rst);
> +
> + /* Initialize the stream */
> + rkcif_stream_init(cif_dev);
> +
> + strlcpy(cif_dev->media_dev.model, "rkcif",
> + sizeof(cif_dev->media_dev.model));
> + cif_dev->media_dev.dev = &pdev->dev;
> + v4l2_dev = &cif_dev->v4l2_dev;
> + v4l2_dev->mdev = &cif_dev->media_dev;
> + strlcpy(v4l2_dev->name, "rkcif", sizeof(v4l2_dev->name));
> + v4l2_ctrl_handler_init(&cif_dev->ctrl_handler, 8);
> + v4l2_dev->ctrl_handler = &cif_dev->ctrl_handler;
> +
> + ret = v4l2_device_register(cif_dev->dev, &cif_dev->v4l2_dev);
> + if (ret < 0)
> + return ret;
> +
> + media_device_init(&cif_dev->media_dev);
> +
> + ret = media_device_register(&cif_dev->media_dev);
> + if (ret < 0) {
> + v4l2_err(v4l2_dev, "Failed to register media device: %d\n",
> + ret);
> + goto err_unreg_v4l2_dev;
> + }
> +
> + /* create & register platefom subdev (from of_node) */
> + ret = rkcif_register_platform_subdevs(cif_dev);
> + if (ret < 0)
> + goto err_unreg_media_dev;
> +
> + pm_runtime_enable(&pdev->dev);
> +
Off the top of my head, I believe enabling PM runtime
after registering the device to the world could be problematic.
> + return 0;
> +
> +err_unreg_media_dev:
> + media_device_unregister(&cif_dev->media_dev);
> +err_unreg_v4l2_dev:
> + v4l2_device_unregister(&cif_dev->v4l2_dev);
> + return ret;
> +}
> +
> +static int rkcif_plat_remove(struct platform_device *pdev)
> +{
> + struct rkcif_device *cif_dev = platform_get_drvdata(pdev);
> +
> + pm_runtime_disable(&pdev->dev);
> +
> + media_device_unregister(&cif_dev->media_dev);
> + v4l2_device_unregister(&cif_dev->v4l2_dev);
> + rkcif_unregister_stream_vdev(cif_dev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused rkcif_runtime_suspend(struct device *dev)
> +{
> + struct rkcif_device *cif_dev = dev_get_drvdata(dev);
> +
> + rkcif_disable_sys_clk(cif_dev);
> +
> + return pinctrl_pm_select_sleep_state(dev);
> +}
> +
> +static int __maybe_unused rkcif_runtime_resume(struct device *dev)
> +{
> + struct rkcif_device *cif_dev = dev_get_drvdata(dev);
> + int ret;
> +
> + ret = pinctrl_pm_select_default_state(dev);
> + if (ret < 0)
> + return ret;
> + rkcif_enable_sys_clk(cif_dev);
> +
Just move the clock handling here and avoid defining rkcif_enable_sys_clk ?
> + return 0;
> +}
> +
> +static const struct dev_pm_ops rkcif_plat_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
> + SET_RUNTIME_PM_OPS(rkcif_runtime_suspend, rkcif_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver rkcif_plat_drv = {
> + .driver = {
> + .name = CIF_DRIVER_NAME,
> + .of_match_table = of_match_ptr(rkcif_plat_of_match),
> + .pm = &rkcif_plat_pm_ops,
> + },
> + .probe = rkcif_plat_probe,
> + .remove = rkcif_plat_remove,
> +};
> +
> +module_platform_driver(rkcif_plat_drv);
> +MODULE_AUTHOR("Rockchip Camera/ISP team");
MODULE_AUTHOR goes to the module's metadata,
e.g. for users to query driver's authors.
I've had to upstream drivers originally written by other
people, and I've wondered what's the value of
putting a team as author.
> +MODULE_DESCRIPTION("Rockchip CIF platform driver");
> +MODULE_LICENSE("GPL");
Thanks for the hard work,
Ezequiel
^ permalink raw reply
* Re: [GIT PULL] Immutable branch between MFD, IIO and Power due for the v5.8 merge window
From: Jonathan Cameron @ 2020-05-31 13:58 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Lee Jones, Saravanan Sekar, andy.shevchenko, robh+dt, knaack.h,
lars, pmeerw, devicetree, linux-kernel, linux-iio, linux-pm
In-Reply-To: <20200528070939.7btxjwuqn7bhf4xe@earth.universe>
On Thu, 28 May 2020 09:09:39 +0200
Sebastian Reichel <sebastian.reichel@collabora.com> wrote:
> Hi,
>
> Thanks for taking care of it Lee, merged!
Thanks indeed.
Given timing I'll probably only get this into IIO via a rebase
after rc1.
Thanks,
Jonathan
>
> -- Sebastian
>
> On Tue, May 26, 2020 at 10:47:02AM +0100, Lee Jones wrote:
> > Enjoy!
> >
> > The following changes since commit 8f3d9f354286745c751374f5f1fcafee6b3f3136:
> >
> > Linux 5.7-rc1 (2020-04-12 12:35:55 -0700)
> >
> > are available in the Git repository at:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git tags/ib-mfd-iio-power-v5.8
> >
> > for you to fetch changes up to 904ac71f4b0c1c26ec47ff597cb3d3c7d36e618d:
> >
> > MAINTAINERS: Add entry for mp2629 Battery Charger driver (2020-05-26 10:42:02 +0100)
> >
> > ----------------------------------------------------------------
> > Immutable branch between MFD, IIO and Power due for the v5.8 merge window
> >
> > ----------------------------------------------------------------
> > Saravanan Sekar (6):
> > dt-bindings: mfd: Add document bindings for mp2629
> > mfd: mp2629: Add support for mps battery charger
> > iio: adc: mp2629: Add support for mp2629 ADC driver
> > power: supply: Add support for mps mp2629 battery charger
> > power: supply: mp2629: Add impedance compensation config
> > MAINTAINERS: Add entry for mp2629 Battery Charger driver
> >
> > Documentation/ABI/testing/sysfs-class-power-mp2629 | 8 +
> > .../devicetree/bindings/mfd/mps,mp2629.yaml | 62 ++
> > MAINTAINERS | 5 +
> > drivers/iio/adc/Kconfig | 10 +
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/mp2629_adc.c | 208 +++++++
> > drivers/mfd/Kconfig | 9 +
> > drivers/mfd/Makefile | 2 +
> > drivers/mfd/mp2629.c | 79 +++
> > drivers/power/supply/Kconfig | 10 +
> > drivers/power/supply/Makefile | 1 +
> > drivers/power/supply/mp2629_charger.c | 669 +++++++++++++++++++++
> > include/linux/mfd/mp2629.h | 26 +
> > 13 files changed, 1090 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-class-power-mp2629
> > create mode 100644 Documentation/devicetree/bindings/mfd/mps,mp2629.yaml
> > create mode 100644 drivers/iio/adc/mp2629_adc.c
> > create mode 100644 drivers/mfd/mp2629.c
> > create mode 100644 drivers/power/supply/mp2629_charger.c
> > create mode 100644 include/linux/mfd/mp2629.h
> >
> > --
> > Lee Jones [李琼斯]
> > Linaro Services Technical Lead
> > Linaro.org │ Open source software for ARM SoCs
> > Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 1/5] dt-bindings: iio: imu: bmi160: convert format to yaml, add maintainer
From: Jonathan Cameron @ 2020-05-31 14:02 UTC (permalink / raw)
To: Rob Herring
Cc: Jonathan Albrieux, Rob Herring, Hartmut Knaack, devicetree,
linux-iio, daniel.baluta, Peter Meerwald-Stadler,
~postmarketos/upstreaming, linux-kernel, Lars-Peter Clausen
In-Reply-To: <20200529170834.GA2578530@bogus>
On Fri, 29 May 2020 11:08:34 -0600
Rob Herring <robh@kernel.org> wrote:
> On Mon, 25 May 2020 18:46:00 +0200, Jonathan Albrieux wrote:
> > Converts documentation from txt format to yaml.
> >
> > Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > ---
> > .../devicetree/bindings/iio/imu/bmi160.txt | 37 ---------
> > .../bindings/iio/imu/bosch,bmi160.yaml | 75 +++++++++++++++++++
> > 2 files changed, 75 insertions(+), 37 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/iio/imu/bmi160.txt
> > create mode 100644 Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml
> >
>
> Reviewed-by: Rob Herring <robh@kernel.org>
Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to poke at.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH v4 2/5] dt-bindings: iio: imu: bmi160: add regulators and mount-matrix
From: Jonathan Cameron @ 2020-05-31 14:05 UTC (permalink / raw)
To: Rob Herring
Cc: Jonathan Albrieux, linux-kernel, ~postmarketos/upstreaming,
daniel.baluta,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Hartmut Knaack, Lars-Peter Clausen,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler
In-Reply-To: <20200529170943.GA2578764@bogus>
On Fri, 29 May 2020 11:09:43 -0600
Rob Herring <robh@kernel.org> wrote:
> On Mon, May 25, 2020 at 06:46:01PM +0200, Jonathan Albrieux wrote:
> > Add vdd-supply and vddio-supply support.
> > Add mount-matrix support.
> >
> > Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
> > ---
> > .../bindings/iio/imu/bosch,bmi160.yaml | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml b/Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml
> > index 0d0ef84e22b9..cfe40dbcd723 100644
> > --- a/Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml
> > +++ b/Documentation/devicetree/bindings/iio/imu/bosch,bmi160.yaml
> > @@ -37,6 +37,17 @@ properties:
> > set if the specified interrupt pin should be configured as
> > open drain. If not set, defaults to push-pull.
> >
> > + vdd-supply:
> > + maxItems: 1
>
> Supplies are always a single item, so don't need this.
Given this (and case below) were it outstanding for this patch I
fixed them up whilst applying rather than getting Jonathan to
go around again.
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.
Thanks,
Jonathan
>
> > + description: provide VDD power to the sensor.
> > +
> > + vddio-supply:
> > + maxItems: 1
> > + description: provide VDD IO power to the sensor.
> > +
> > + mount-matrix:
> > + description: an optional 3x3 mounting rotation matrix
> > +
> > required:
> > - compatible
> > - reg
> > @@ -52,9 +63,14 @@ examples:
> > bmi160@68 {
> > compatible = "bosch,bmi160";
> > reg = <0x68>;
> > + vdd-supply = <&pm8916_l17>;
> > + vddio-supply = <&pm8916_l6>;
> > interrupt-parent = <&gpio4>;
> > interrupts = <12 IRQ_TYPE_EDGE_RISING>;
> > interrupt-names = "INT1";
> > + mount-matrix = "0", "1", "0",
> > + "-1", "0", "0",
> > + "0", "0", "1";
> > };
> > };
> > - |
> > --
> > 2.17.1
> >
^ permalink raw reply
* Re: [PATCH v4 3/5] iio: imu: bmi160: fix typo
From: Jonathan Cameron @ 2020-05-31 14:27 UTC (permalink / raw)
To: Jonathan Albrieux
Cc: linux-kernel, ~postmarketos/upstreaming, daniel.baluta,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Hartmut Knaack, Lars-Peter Clausen,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler
In-Reply-To: <20200525164615.14962-4-jonathan.albrieux@gmail.com>
On Mon, 25 May 2020 18:46:02 +0200
Jonathan Albrieux <jonathan.albrieux@gmail.com> wrote:
> Fix a typo in MODULE_AUTHOR() argument.
>
> Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
applied
Thanks,
J
> ---
> drivers/iio/imu/bmi160/bmi160_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index 6af65d6f1d28..77b05bd4a2b2 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -853,6 +853,6 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> }
> EXPORT_SYMBOL_GPL(bmi160_core_probe);
>
> -MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com");
> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
> MODULE_DESCRIPTION("Bosch BMI160 driver");
> MODULE_LICENSE("GPL v2");
^ permalink raw reply
* Re: [PATCH v4 4/5] iio: imu: bmi160: added regulator support
From: Jonathan Cameron @ 2020-05-31 14:28 UTC (permalink / raw)
To: Jonathan Albrieux
Cc: linux-kernel, ~postmarketos/upstreaming, daniel.baluta,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Hartmut Knaack, Lars-Peter Clausen,
open list:IIO SUBSYSTEM AND DRIVERS, Peter Meerwald-Stadler
In-Reply-To: <20200525164615.14962-5-jonathan.albrieux@gmail.com>
On Mon, 25 May 2020 18:46:03 +0200
Jonathan Albrieux <jonathan.albrieux@gmail.com> wrote:
> Add vdd-supply and vddio-supply support.
>
> While working on an msm8916 device and having explicit declarations for
> regulators, without setting these regulators to regulators-always-on it
> happened those lines weren't ready because they could have been controlled
> by other components, causing failure in module's probe.
>
> This patch aim is to solve this situation by adding regulators control
> during bmi160_chip_init() and bmi160_chip_uninit(), assuring power to
> this component.
>
> Signed-off-by: Jonathan Albrieux <jonathan.albrieux@gmail.com>
Applied,
Thanks,
Jonathan
> ---
> drivers/iio/imu/bmi160/bmi160.h | 2 ++
> drivers/iio/imu/bmi160/bmi160_core.c | 24 ++++++++++++++++++++++++
> 2 files changed, 26 insertions(+)
>
> diff --git a/drivers/iio/imu/bmi160/bmi160.h b/drivers/iio/imu/bmi160/bmi160.h
> index 621f5309d735..923c3b274fde 100644
> --- a/drivers/iio/imu/bmi160/bmi160.h
> +++ b/drivers/iio/imu/bmi160/bmi160.h
> @@ -3,10 +3,12 @@
> #define BMI160_H_
>
> #include <linux/iio/iio.h>
> +#include <linux/regulator/consumer.h>
>
> struct bmi160_data {
> struct regmap *regmap;
> struct iio_trigger *trig;
> + struct regulator_bulk_data supplies[2];
> };
>
> extern const struct regmap_config bmi160_regmap_config;
> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c
> index 77b05bd4a2b2..d3316ca02fbd 100644
> --- a/drivers/iio/imu/bmi160/bmi160_core.c
> +++ b/drivers/iio/imu/bmi160/bmi160_core.c
> @@ -15,6 +15,7 @@
> #include <linux/delay.h>
> #include <linux/irq.h>
> #include <linux/of_irq.h>
> +#include <linux/regulator/consumer.h>
>
> #include <linux/iio/iio.h>
> #include <linux/iio/triggered_buffer.h>
> @@ -709,6 +710,12 @@ static int bmi160_chip_init(struct bmi160_data *data, bool use_spi)
> unsigned int val;
> struct device *dev = regmap_get_device(data->regmap);
>
> + ret = regulator_bulk_enable(ARRAY_SIZE(data->supplies), data->supplies);
> + if (ret) {
> + dev_err(dev, "Failed to enable regulators: %d\n", ret);
> + return ret;
> + }
> +
> ret = regmap_write(data->regmap, BMI160_REG_CMD, BMI160_CMD_SOFTRESET);
> if (ret)
> return ret;
> @@ -793,9 +800,16 @@ int bmi160_probe_trigger(struct iio_dev *indio_dev, int irq, u32 irq_type)
> static void bmi160_chip_uninit(void *data)
> {
> struct bmi160_data *bmi_data = data;
> + struct device *dev = regmap_get_device(bmi_data->regmap);
> + int ret;
>
> bmi160_set_mode(bmi_data, BMI160_GYRO, false);
> bmi160_set_mode(bmi_data, BMI160_ACCEL, false);
> +
> + ret = regulator_bulk_disable(ARRAY_SIZE(bmi_data->supplies),
> + bmi_data->supplies);
> + if (ret)
> + dev_err(dev, "Failed to disable regulators: %d\n", ret);
> }
>
> int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> @@ -815,6 +829,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap,
> dev_set_drvdata(dev, indio_dev);
> data->regmap = regmap;
>
> + data->supplies[0].supply = "vdd";
> + data->supplies[1].supply = "vddio";
> + ret = devm_regulator_bulk_get(dev,
> + ARRAY_SIZE(data->supplies),
> + data->supplies);
> + if (ret) {
> + dev_err(dev, "Failed to get regulators: %d\n", ret);
> + return ret;
> + }
> +
> ret = bmi160_chip_init(data, use_spi);
> if (ret)
> return ret;
^ permalink raw reply
* Re: [PATCH 1/2] arm64: dts: Add a device tree for the Librem5 phone
From: Guido Günther @ 2020-05-31 15:36 UTC (permalink / raw)
To: Pavel Machek
Cc: Martin Kepplinger, robh, kernel, shawnguo, s.hauer, kernel,
festevam, linux-imx, mchehab, Anson.Huang, angus, linux-kernel,
devicetree, linux-arm-kernel
In-Reply-To: <20200529162850.GC3709@amd>
Hi,
On Fri, May 29, 2020 at 06:28:50PM +0200, Pavel Machek wrote:
> Hi!
>
> > From: "Angus Ainslie (Purism)" <angus@akkea.ca>
> >
> > Add a devicetree description for the Librem 5 phone. The early batches
> > that have been sold are supported as well as the mass-produced device
> > available later this year, see https://puri.sm/products/librem-5/
> >
> > This boots to a working console with working WWAN modem, wifi usdhc,
> > IMU sensor device, proximity sensor, haptic motor, gpio keys, GNSS and LEDs.
> >
> > Signed-off-by: Martin Kepplinger <martin.kepplinger@puri.sm>
> > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca>
> > Signed-off-by: Guido Günther <agx@sigxcpu.org>
>
>
> > + blue {
> > + label = "phone:blue:front";
> > + label = "phone:green:front";
> > + label = "phone:red:front";
>
> Droid 4 uses "status-led:{red,green,blue}". Could this use same
> naming?
Looking at leds-class.rst we don't have a useful devicename so
should that just be omitted and s.th. like
label = "blue:status";
label = "green:status";
label = "red:status";
be used instead. If we want to map to current usage
label = "blue:status";
label = "green:boot";
label = "red:charging";
would be even closer but since that is bound to change just going with
"status" would make sense.
Cheers,
-- Guido
>
> > + label = "lm3560:flash";
> > + label = "lm3560:torch";
>
> This is one LED, right? I'm pretty sure we don't want lm3560 in the
> name... "main-camera:flash" would be better. Even better would be
> something that's already in use.
>
> > + label = "white:backlight_cluster";
>
> Make this ":backlight", please. Again, we want something that's
> already used.
>
> Best regards,
> Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply
* Re: [PATCH v2 4/4] dt-bindings: iio: scd30: add device binding file
From: Tomasz Duszynski @ 2020-05-31 15:44 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Tomasz Duszynski, linux-iio, linux-kernel, devicetree, robh+dt,
andy.shevchenko, pmeerw
In-Reply-To: <20200531111914.56dbff8b@archlinux>
On Sun, May 31, 2020 at 11:19:14AM +0100, Jonathan Cameron wrote:
> On Sat, 30 May 2020 23:36:30 +0200
> Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
>
> > Add SCD30 sensor binding file.
> >
> > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > ---
> > .../iio/chemical/sensirion,scd30.yaml | 68 +++++++++++++++++++
> > MAINTAINERS | 1 +
> > 2 files changed, 69 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml b/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
> > new file mode 100644
> > index 000000000000..34cc3925d64d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
> > @@ -0,0 +1,68 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/chemical/sensirion,scd30.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Sensirion SCD30 carbon dioxide sensor
> > +
> > +maintainers:
> > + - Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > +
> > +description: |
> > + Air quality sensor capable of measuring co2 concentration, temperature
> > + and relative humidity.
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - sensirion,scd30
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + interrupts:
> > + maxItems: 1
> > +
> > + vdd-supply: true
> > +
> > + sensirion,sel-gpios:
> > + description: GPIO connected to the SEL line
> > + maxItems: 1
> > +
> > + sensirion,pwm-gpios:
> > + description: GPIO connected to the PWM line
> > + maxItems: 1
> > +
> > +required:
> > + - compatible
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + # include <dt-bindings/interrupt-controller/irq.h>
> > + i2c {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + scd30@61 {
>
> Nodes should have generic names. Not sure we have an appropriate
> one in the spec, but as main focus of people using this will be
> c02 herpas
>
> c02@61?
>
There are already a few recommended node names ending with '-sensor' on the list
like air-pollution-sensor or temperature-sensor. I'd say co2-sensor would work
here.
> Rob may well have a better suggestion!
>
> > + compatible = "sensirion,scd30";
> > + reg = <0x61>;
> > + vdd-supply = <&vdd>;
> > + interrupt-parent = <&gpio0>;
> > + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > + };
> > + };
> > + - |
> > + # include <dt-bindings/interrupt-controller/irq.h>
> > + serial {
> > + scd30 {
> > + compatible = "sensirion,scd30";
> > + vdd-supply = <&vdd>;
> > + interrupt-parent = <&gpio0>;
> > + interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
> > + };
> > + };
> > +
> > +...
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 5db4b446c8ba..0ab9cf39e051 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15140,6 +15140,7 @@ F: include/uapi/linux/phantom.h
> > SENSIRION SCD30 CARBON DIOXIDE SENSOR DRIVER
> > M: Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > S: Maintained
> > +F: Documentation/devicetree/bindings/iio/chemical/sensirion,scd30.yaml
> > F: drivers/iio/chemical/scd30.h
> > F: drivers/iio/chemical/scd30_core.c
> > F: drivers/iio/chemical/scd30_i2c.c
>
^ permalink raw reply
* Re: [PATCH v2 3/4] iio: chemical: scd30: add serial interface driver
From: Tomasz Duszynski @ 2020-05-31 15:50 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Tomasz Duszynski, linux-iio, linux-kernel, devicetree, robh+dt,
andy.shevchenko, pmeerw
In-Reply-To: <20200531111518.2340197a@archlinux>
On Sun, May 31, 2020 at 11:15:18AM +0100, Jonathan Cameron wrote:
> On Sat, 30 May 2020 23:36:29 +0200
> Tomasz Duszynski <tomasz.duszynski@octakon.com> wrote:
>
> > Add serial interface driver for the SCD30 sensor.
> >
> > Signed-off-by: Tomasz Duszynski <tomasz.duszynski@octakon.com>
>
> Ah Now I see why you had those extra elements in the iio_priv
> structure.
>
Indeed. That the whole point of having them. I'll add a short
explanation what those elements are for.
> Hmm. serdev_device callbacks using the top level device drvdata
> is a bit annoying. Really feels to me like they should have
> their own priv data for those callbacks given the device
> drvdata gets used for so many other things.
>
> Oh well. Guess this is the best we can do!
>
> Jonathan
>
> > ---
> > MAINTAINERS | 1 +
> > drivers/iio/chemical/Kconfig | 11 ++
> > drivers/iio/chemical/Makefile | 1 +
> > drivers/iio/chemical/scd30_serial.c | 266 ++++++++++++++++++++++++++++
> > 4 files changed, 279 insertions(+)
> > create mode 100644 drivers/iio/chemical/scd30_serial.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 13aed3473b7e..5db4b446c8ba 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -15143,6 +15143,7 @@ S: Maintained
> > F: drivers/iio/chemical/scd30.h
> > F: drivers/iio/chemical/scd30_core.c
> > F: drivers/iio/chemical/scd30_i2c.c
> > +F: drivers/iio/chemical/scd30_serial.c
> >
> > SENSIRION SPS30 AIR POLLUTION SENSOR DRIVER
> > M: Tomasz Duszynski <tduszyns@gmail.com>
> > diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
> > index 970d34888c2e..10bb431bc3ce 100644
> > --- a/drivers/iio/chemical/Kconfig
> > +++ b/drivers/iio/chemical/Kconfig
> > @@ -107,6 +107,17 @@ config SCD30_I2C
> > To compile this driver as a module, choose M here: the module will
> > be called scd30_i2c.
> >
> > +config SCD30_SERIAL
> > + tristate "SCD30 carbon dioxide sensor serial driver"
> > + depends on SCD30_CORE && SERIAL_DEV_BUS
> > + select CRC16
> > + help
> > + Say Y here to build support for the Sensirion SCD30 serial interface
> > + driver.
> > +
> > + To compile this driver as a module, choose M here: the module will
> > + be called scd30_serial.
> > +
> > config SENSIRION_SGP30
> > tristate "Sensirion SGPxx gas sensors"
> > depends on I2C
> > diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
> > index 0966ca34e34b..fef63dd5bf92 100644
> > --- a/drivers/iio/chemical/Makefile
> > +++ b/drivers/iio/chemical/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_IAQCORE) += ams-iaq-core.o
> > obj-$(CONFIG_PMS7003) += pms7003.o
> > obj-$(CONFIG_SCD30_CORE) += scd30_core.o
> > obj-$(CONFIG_SCD30_I2C) += scd30_i2c.o
> > +obj-$(CONFIG_SCD30_SERIAL) += scd30_serial.o
> > obj-$(CONFIG_SENSIRION_SGP30) += sgp30.o
> > obj-$(CONFIG_SPS30) += sps30.o
> > obj-$(CONFIG_VZ89X) += vz89x.o
> > diff --git a/drivers/iio/chemical/scd30_serial.c b/drivers/iio/chemical/scd30_serial.c
> > new file mode 100644
> > index 000000000000..07d7d3110fe0
> > --- /dev/null
> > +++ b/drivers/iio/chemical/scd30_serial.c
> > @@ -0,0 +1,266 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Sensirion SCD30 carbon dioxide sensor serial driver
> > + *
> > + * Copyright (c) 2020 Tomasz Duszynski <tomasz.duszynski@octakon.com>
> > + */
> > +#include <linux/crc16.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/property.h>
> > +#include <linux/serdev.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <asm/unaligned.h>
> > +
> > +#include "scd30.h"
> > +
> > +#define SCD30_SERDEV_ADDR 0x61
> > +#define SCD30_SERDEV_WRITE 0x06
> > +#define SCD30_SERDEV_READ 0x03
> > +#define SCD30_SERDEV_MAX_BUF_SIZE 17
> > +#define SCD30_SERDEV_RX_HEADER_SIZE 3
> > +#define SCD30_SERDEV_CRC_SIZE 2
> > +#define SCD30_SERDEV_TIMEOUT msecs_to_jiffies(200)
> > +
> > +struct scd30_serdev_priv {
> > + struct completion meas_ready;
> > + char *buf;
> > + int num_expected;
> > + int num;
> > +};
> > +
> > +static u16 scd30_serdev_cmd_lookup_tbl[] = {
> > + [CMD_START_MEAS] = 0x0036,
> > + [CMD_STOP_MEAS] = 0x0037,
> > + [CMD_MEAS_INTERVAL] = 0x0025,
> > + [CMD_MEAS_READY] = 0x0027,
> > + [CMD_READ_MEAS] = 0x0028,
> > + [CMD_ASC] = 0x003a,
> > + [CMD_FRC] = 0x0039,
> > + [CMD_TEMP_OFFSET] = 0x003b,
> > + [CMD_FW_VERSION] = 0x0020,
> > + [CMD_RESET] = 0x0034,
> > +};
> > +
> > +static u16 scd30_serdev_calc_crc(const char *buf, int size)
> > +{
> > + return crc16(0xffff, buf, size);
> > +}
> > +
> > +static int scd30_serdev_xfer(struct scd30_state *state, char *txbuf, int txsize,
> > + char *rxbuf, int rxsize)
> > +{
> > + struct serdev_device *serdev = to_serdev_device(state->dev);
> > + struct scd30_serdev_priv *priv = state->priv;
> > + int ret;
> > +
> > + priv->buf = rxbuf;
> > + priv->num_expected = rxsize;
> > + priv->num = 0;
> > +
> > + ret = serdev_device_write(serdev, txbuf, txsize, SCD30_SERDEV_TIMEOUT);
> > + if (ret < txsize)
> > + return ret < 0 ? ret : -EIO;
> > +
> > + ret = wait_for_completion_interruptible_timeout(&priv->meas_ready,
> > + SCD30_SERDEV_TIMEOUT);
> > + if (ret > 0)
> > + ret = 0;
> > + else if (!ret)
> > + ret = -ETIMEDOUT;
> > +
> > + return ret;
> > +}
> > +
> > +static int scd30_serdev_command(struct scd30_state *state, enum scd30_cmd cmd,
> > + u16 arg, void *response, int size)
> > +{
> > + /*
> > + * Communication over serial line is based on modbus protocol (or rather
> > + * its variation called modbus over serial to be precise). Upon
> > + * receiving a request device should reply with response.
> > + *
> > + * Frame below represents a request message. Each field takes
> > + * exactly one byte.
> > + *
> > + * +------+------+-----+-----+-------+-------+-----+-----+
> > + * | dev | op | reg | reg | byte1 | byte0 | crc | crc |
> > + * | addr | code | msb | lsb | | | lsb | msb |
> > + * +------+------+-----+-----+-------+-------+-----+-----+
> > + *
> > + * The message device replies with depends on the 'op code' field from
> > + * the request. In case it was set to SCD30_SERDEV_WRITE sensor should
> > + * reply with unchanged request. Otherwise 'op code' was set to
> > + * SCD30_SERDEV_READ and response looks like the one below. As with
> > + * request, each field takes one byte.
> > + *
> > + * +------+------+--------+-------+-----+-------+-----+-----+
> > + * | dev | op | num of | byte0 | ... | byteN | crc | crc |
> > + * | addr | code | bytes | | | | lsb | msb |
> > + * +------+------+--------+-------+-----+-------+-----+-----+
> > + */
> > + char txbuf[SCD30_SERDEV_MAX_BUF_SIZE] = { SCD30_SERDEV_ADDR },
> > + rxbuf[SCD30_SERDEV_MAX_BUF_SIZE], *rsp = response;
> > + int ret, rxsize, txsize = 2;
> > + u16 crc;
> > +
> > + put_unaligned_be16(scd30_serdev_cmd_lookup_tbl[cmd], txbuf + txsize);
> > + txsize += 2;
> > +
> > + if (rsp) {
> > + txbuf[1] = SCD30_SERDEV_READ;
> > + if (cmd == CMD_READ_MEAS)
> > + /* number of u16 words to read */
> > + put_unaligned_be16(size / 2, txbuf + txsize);
> > + else
> > + put_unaligned_be16(0x0001, txbuf + txsize);
> > + txsize += 2;
> > + crc = scd30_serdev_calc_crc(txbuf, txsize);
> > + put_unaligned_le16(crc, txbuf + txsize);
> > + txsize += 2;
> > + rxsize = SCD30_SERDEV_RX_HEADER_SIZE + size +
> > + SCD30_SERDEV_CRC_SIZE;
> > + } else {
> > + if ((cmd == CMD_STOP_MEAS) || (cmd == CMD_RESET))
> > + arg = 0x0001;
> > +
> > + txbuf[1] = SCD30_SERDEV_WRITE;
> > + put_unaligned_be16(arg, txbuf + txsize);
> > + txsize += 2;
> > + crc = scd30_serdev_calc_crc(txbuf, txsize);
> > + put_unaligned_le16(crc, txbuf + txsize);
> > + txsize += 2;
> > + rxsize = txsize;
> > + }
> > +
> > + ret = scd30_serdev_xfer(state, txbuf, txsize, rxbuf, rxsize);
> > + if (ret)
> > + return ret;
> > +
> > + switch (txbuf[1]) {
> > + case SCD30_SERDEV_WRITE:
> > + if (memcmp(txbuf, txbuf, txsize)) {
> > + dev_err(state->dev, "wrong message received\n");
> > + return -EIO;
> > + }
> > + break;
> > + case SCD30_SERDEV_READ:
> > + if (rxbuf[2] != (rxsize -
> > + SCD30_SERDEV_RX_HEADER_SIZE -
> > + SCD30_SERDEV_CRC_SIZE)) {
> > + dev_err(state->dev,
> > + "received data size does not match header\n");
> > + return -EIO;
> > + }
> > +
> > + rxsize -= SCD30_SERDEV_CRC_SIZE;
> > + crc = get_unaligned_le16(rxbuf + rxsize);
> > + if (crc != scd30_serdev_calc_crc(rxbuf, rxsize)) {
> > + dev_err(state->dev, "data integrity check failed\n");
> > + return -EIO;
> > + }
> > +
> > + rxsize -= SCD30_SERDEV_RX_HEADER_SIZE;
> > + memcpy(rsp, rxbuf + SCD30_SERDEV_RX_HEADER_SIZE, rxsize);
> > + break;
> > + default:
> > + dev_err(state->dev, "received unknown op code\n");
> > + return -EIO;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int scd30_serdev_receive_buf(struct serdev_device *serdev,
> > + const unsigned char *buf, size_t size)
> > +{
> > + struct iio_dev *indio_dev = dev_get_drvdata(&serdev->dev);
> > + struct scd30_serdev_priv *priv;
> > + struct scd30_state *state;
> > + int num;
> > +
> > + if (!indio_dev)
> > + return 0;
> > +
> > + state = iio_priv(indio_dev);
> > + priv = state->priv;
> > +
> > + /* just in case sensor puts some unexpected bytes on the bus */
> > + if (!priv->buf)
> > + return 0;
> > +
> > + if (priv->num + size >= priv->num_expected)
> > + num = priv->num_expected - priv->num;
> > + else
> > + num = size;
> > +
> > + memcpy(priv->buf + priv->num, buf, num);
> > + priv->num += num;
> > +
> > + if (priv->num == priv->num_expected) {
> > + priv->buf = NULL;
> > + complete(&priv->meas_ready);
> > + }
> > +
> > + return num;
> > +}
> > +
> > +static const struct serdev_device_ops scd30_serdev_ops = {
> > + .receive_buf = scd30_serdev_receive_buf,
> > + .write_wakeup = serdev_device_write_wakeup,
> > +};
> > +
> > +static int scd30_serdev_probe(struct serdev_device *serdev)
> > +{
> > + struct device *dev = &serdev->dev;
> > + struct scd30_serdev_priv *priv;
> > + int irq, ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + init_completion(&priv->meas_ready);
> > + serdev_device_set_client_ops(serdev, &scd30_serdev_ops);
> > +
> > + ret = devm_serdev_device_open(dev, serdev);
> > + if (ret)
> > + return ret;
> > +
> > + serdev_device_set_baudrate(serdev, 19200);
> > + serdev_device_set_flow_control(serdev, false);
> > +
> > + ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
> > + if (ret)
> > + return ret;
> > +
> > + irq = fwnode_irq_get(dev_fwnode(dev), 0);
> > +
> > + return scd30_probe(dev, irq, KBUILD_MODNAME, priv,
> > + scd30_serdev_command);
> > +}
> > +
> > +static const struct of_device_id scd30_serdev_of_match[] = {
> > + { .compatible = "sensirion,scd30" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, scd30_serdev_of_match);
> > +
> > +static struct serdev_device_driver scd30_serdev_driver = {
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .of_match_table = scd30_serdev_of_match,
> > + .pm = &scd30_pm_ops,
> > + },
> > + .probe = scd30_serdev_probe,
> > +};
> > +module_serdev_device_driver(scd30_serdev_driver);
> > +
> > +MODULE_AUTHOR("Tomasz Duszynski <tomasz.duszynski@octakon.com>");
> > +MODULE_DESCRIPTION("Sensirion SCD30 carbon dioxide sensor serial driver");
> > +MODULE_LICENSE("GPL v2");
>
^ permalink raw reply
* Re: [PATCH v6 08/11] i2c: designware: Convert driver to using regmap API
From: Serge Semin @ 2020-05-31 17:12 UTC (permalink / raw)
To: Wolfram Sang
Cc: Serge Semin, Jarkko Nikula, Andy Shevchenko, Mika Westerberg,
Alexey Malahov, Thomas Bogendoerfer, Rob Herring, devicetree,
linux-mips, linux-i2c, linux-kernel
In-Reply-To: <20200530210554.GA15682@ninjato>
On Sat, May 30, 2020 at 11:05:54PM +0200, Wolfram Sang wrote:
> On Sat, May 30, 2020 at 01:09:30PM +0200, Wolfram Sang wrote:
> > On Thu, May 28, 2020 at 12:33:18PM +0300, Serge Semin wrote:
> > > Seeing the DW I2C driver is using flags-based accessors with two
> > > conditional clauses it would be better to replace them with the regmap
> > > API IO methods and to initialize the regmap object with read/write
> > > callbacks specific to the controller registers map implementation. This
> > > will be also handy for the drivers with non-standard registers mapping
> > > (like an embedded into the Baikal-T1 System Controller DW I2C block, which
> > > glue-driver is a part of this series).
> > >
> > > As before the driver tries to detect the mapping setup at probe stage and
> > > creates a regmap object accordingly, which will be used by the rest of the
> > > code to correctly access the controller registers. In two places it was
> > > appropriate to convert the hand-written read-modify-write and
> > > read-poll-loop design patterns to the corresponding regmap API
> > > ready-to-use methods.
> > >
> > > Note the regmap IO methods return value is checked only at the probe
> > > stage. The rest of the code won't do this because basically we have
> > > MMIO-based regmap so non of the read/write methods can fail (this also
> > > won't be needed for the Baikal-T1-specific I2C controller).
> > >
> > > Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > Tested-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> > > Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: devicetree@vger.kernel.org
> > > Cc: linux-mips@vger.kernel.org
> >
> > My codecheckers found these, rightfully I'd say:
> >
> > SPARSE
> > drivers/i2c/busses/i2c-designware-master.c:427:53: warning: incorrect type in argument 3 (different signedness)
> > drivers/i2c/busses/i2c-designware-master.c:427:53: expected unsigned int *val
> > drivers/i2c/busses/i2c-designware-master.c:427:53: got int *
> >
> > CC
> > drivers/i2c/busses/i2c-designware-common.c:127: warning: Excess function parameter 'base' description in 'i2c_dw_init_regmap'
> >
> > ^ means there is an argument documented which does not exist in the
> > function declaration
>
> Well, I fixed these two minor things for your now...
>
Great! Sorry for me not doing it on time. I was away from my laptop and internet
last day to do that. I saw your message about the sparse detecting the problems
while I was on my way to outskirts and was going to fix them today either in a
new version of the patchset or sending follow-up patches. But you already fixed
the problems. Thank you very much for doing that for me.
-Sergey
^ permalink raw reply
* [PATCH 00/14] msm8992 DTS updates, peripheral enablement
From: Konrad Dybcio @ 2020-05-31 17:27 UTC (permalink / raw)
Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
linux-arm-msm, devicetree, linux-kernel
This patch series adds support for PMU, PSCI, SPMI
and SCM, as well as all six cores on msm8992.
Please note though, that due to the lack of a core
cluster interconnect driver, it is only recommended
to use one cluster (as in set NR_CPUS to 4) for now.
The other two technically work, but in practice slow
down the platform by a LOT.
Hopefully with a proper MMCC driver this platform can be
brought up to speed with other Qualcomm SoCs.
Konrad Dybcio (14):
arm64: dts: Add a proper CPU map for MSM8992
arm64: dts: msm8992: Place clock nodes in clocks{}
arm64: dts: msm8992: Add a SCM node
firmware: qcom_scm: Add msm8992 compatible
Documentation: Document msm8992 qcom_scm compatible
arm64: dts: msm8992: Add a spmi_bus node
arm64: dts: msm8992: Add BLSP2 UART2 node
arm64: dts: msm8992: Add a BLSP I2C2 node
arm64: dts: msm8992: Add a BLSP I2C6 node
arm64: dts: msm8992: Add PMU node
arm64: dts: msm8992: Add PSCI support.
arm64: dts: qcom: Update msm8992 pin configuration
arm64: dts: qcom: Fix msm8992 SDHCI
arm64: dts: qcom: Add a label to the msm8992 rpm-requests node
.../devicetree/bindings/firmware/qcom,scm.txt | 1 +
arch/arm64/boot/dts/qcom/msm8992-pins.dtsi | 149 ++++++++++++----
arch/arm64/boot/dts/qcom/msm8992.dtsi | 167 ++++++++++++++++--
drivers/firmware/qcom_scm.c | 1 +
4 files changed, 275 insertions(+), 43 deletions(-)
--
2.26.2
^ permalink raw reply
* [PATCH 01/14] arm64: dts: Add a proper CPU map for MSM8992
From: Konrad Dybcio @ 2020-05-31 17:27 UTC (permalink / raw)
Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20200531172804.256335-1-konradybcio@gmail.com>
This commit adds cpu nodes for all 6 cores
present on this SoC and the cpu-map.
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
---
arch/arm64/boot/dts/qcom/msm8992.dtsi | 68 +++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi
index 2021795c99ad..900c9445e0ba 100644
--- a/arch/arm64/boot/dts/qcom/msm8992.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi
@@ -20,11 +20,34 @@ / {
cpus {
#address-cells = <2>;
#size-cells = <0>;
+
cpu-map {
cluster0 {
core0 {
cpu = <&CPU0>;
};
+
+ core1 {
+ cpu = <&CPU1>;
+ };
+
+ core2 {
+ cpu = <&CPU2>;
+ };
+
+ core3 {
+ cpu = <&CPU3>;
+ };
+ };
+
+ cluster1 {
+ core0 {
+ cpu = <&CPU4>;
+ };
+
+ core1 {
+ cpu = <&CPU5>;
+ };
};
};
@@ -33,11 +56,56 @@ CPU0: cpu@0 {
compatible = "arm,cortex-a53";
reg = <0x0 0x0>;
next-level-cache = <&L2_0>;
+ enable-method = "psci";
L2_0: l2-cache {
compatible = "cache";
cache-level = <2>;
};
};
+
+ CPU1: cpu@1 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x0 0x1>;
+ next-level-cache = <&L2_0>;
+ enable-method = "psci";
+ };
+
+ CPU2: cpu@2 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x0 0x2>;
+ next-level-cache = <&L2_0>;
+ enable-method = "psci";
+ };
+
+ CPU3: cpu@3 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a53";
+ reg = <0x0 0x3>;
+ next-level-cache = <&L2_0>;
+ enable-method = "psci";
+ };
+
+ CPU4: cpu@100 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a57";
+ reg = <0x0 0x100>;
+ next-level-cache = <&L2_1>;
+ enable-method = "psci";
+ L2_1: l2-cache {
+ compatible = "cache";
+ cache-level = <2>;
+ };
+ };
+
+ CPU5: cpu@101 {
+ device_type = "cpu";
+ compatible = "arm,cortex-a57";
+ reg = <0x0 0x101>;
+ next-level-cache = <&L2_1>;
+ enable-method = "psci";
+ };
};
timer {
--
2.26.2
^ permalink raw reply related
* [PATCH 04/14] firmware: qcom_scm: Add msm8992 compatible
From: Konrad Dybcio @ 2020-05-31 17:27 UTC (permalink / raw)
Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20200531172804.256335-1-konradybcio@gmail.com>
This change adds a compatible for msm8992,
which requires no additional clocks for
scm to probe correctly.
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
---
drivers/firmware/qcom_scm.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 059bb0fbae9e..4ee94c2ff5c0 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -1154,6 +1154,7 @@ static const struct of_device_id qcom_scm_dt_match[] = {
SCM_HAS_IFACE_CLK |
SCM_HAS_BUS_CLK)
},
+ { .compatible = "qcom,scm-msm8992" },
{ .compatible = "qcom,scm-msm8996" },
{ .compatible = "qcom,scm" },
{}
--
2.26.2
^ permalink raw reply related
* [PATCH 07/14] arm64: dts: msm8992: Add BLSP2 UART2 node
From: Konrad Dybcio @ 2020-05-31 17:27 UTC (permalink / raw)
Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20200531172804.256335-1-konradybcio@gmail.com>
This commit enables the usage of a second
UART interface.
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
---
arch/arm64/boot/dts/qcom/msm8992.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi
index 8f7cdf2b9a1f..ff745905525c 100644
--- a/arch/arm64/boot/dts/qcom/msm8992.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi
@@ -279,6 +279,16 @@ blsp1_uart2: serial@f991e000 {
<&clock_gcc GCC_BLSP1_AHB_CLK>;
};
+ blsp2_uart2: serial@f995e000 {
+ compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
+ reg = <0xf995e000 0x1000>;
+ interrupt = <GIC_SPI 146 IRQ_TYPE_LEVEL_LOW>;
+ status = "disabled";
+ clock-names = "core", "iface";
+ clocks = <&gcc GCC_BLSP2_UART2_APPS_CLK>,
+ <&gcc GCC_BLSP2_AHB_CLK>;
+ };
+
clock_gcc: clock-controller@fc400000 {
compatible = "qcom,gcc-msm8994";
#clock-cells = <1>;
--
2.26.2
^ permalink raw reply related
* [PATCH 08/14] arm64: dts: msm8992: Add a BLSP I2C2 node
From: Konrad Dybcio @ 2020-05-31 17:27 UTC (permalink / raw)
Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20200531172804.256335-1-konradybcio@gmail.com>
This commit adds support for the second I2C interface
on msm8992 which seems to be used mostly for touchscreen
devices.
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
---
arch/arm64/boot/dts/qcom/msm8992.dtsi | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi
index ff745905525c..395f4c325c2f 100644
--- a/arch/arm64/boot/dts/qcom/msm8992.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi
@@ -333,6 +333,21 @@ sfpb_mutex_regs: syscon@fd484000 {
compatible = "syscon";
reg = <0xfd484000 0x400>;
};
+
+ blsp_i2c2: i2c@f9924000 {
+ compatible = "qcom,i2c-qup-v2.2.1";
+ reg = <0xf9924000 0x500>;
+ interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&gcc GCC_BLSP1_AHB_CLK>,
+ <&gcc GCC_BLSP1_QUP2_I2C_APPS_CLK>;
+ clock-names = "iface", "core";
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&i2c2_default>;
+ pinctrl-1 = <&i2c2_sleep>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
};
memory {
--
2.26.2
^ permalink raw reply related
* [PATCH 13/14] arm64: dts: qcom: Fix msm8992 SDHCI
From: Konrad Dybcio @ 2020-05-31 17:28 UTC (permalink / raw)
Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20200531172804.256335-1-konradybcio@gmail.com>
This commit adds correct IRQ settings and adds
the non-removable property to the msm8992 sdhci
node.
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
---
arch/arm64/boot/dts/qcom/msm8992.dtsi | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi
index e7354826d701..b5bd73205a55 100644
--- a/arch/arm64/boot/dts/qcom/msm8992.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi
@@ -302,8 +302,8 @@ sdhci1: mmc@f9824900 {
reg = <0xf9824900 0x1a0>, <0xf9824000 0x800>;
reg-names = "hc_mem", "core_mem";
- interrupts = <GIC_SPI 123 IRQ_TYPE_NONE>,
- <GIC_SPI 138 IRQ_TYPE_NONE>;
+ interrupts = <GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 138 IRQ_TYPE_LEVEL_HIGH>;
interrupt-names = "hc_irq", "pwr_irq";
clocks = <&clock_gcc GCC_SDCC1_APPS_CLK>,
@@ -319,6 +319,7 @@ sdhci1: mmc@f9824900 {
regulator-always-on;
bus-width = <8>;
mmc-hs400-1_8v;
+ non-removable;
status = "okay";
};
--
2.26.2
^ permalink raw reply related
* [PATCH 14/14] arm64: dts: qcom: Add a label to the msm8992 rpm-requests node
From: Konrad Dybcio @ 2020-05-31 17:28 UTC (permalink / raw)
Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20200531172804.256335-1-konradybcio@gmail.com>
This enables the node to be referenced in other device trees.
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
---
arch/arm64/boot/dts/qcom/msm8992.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi
index b5bd73205a55..81fed16fcee6 100644
--- a/arch/arm64/boot/dts/qcom/msm8992.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi
@@ -401,7 +401,7 @@ rpm {
qcom,local-pid = <0>;
qcom,remote-pid = <6>;
- rpm-requests {
+ rpm_requests: rpm-requests {
compatible = "qcom,rpm-msm8994";
qcom,smd-channels = "rpm_requests";
--
2.26.2
^ permalink raw reply related
* [PATCH 11/14] arm64: dts: msm8992: Add PSCI support.
From: Konrad Dybcio @ 2020-05-31 17:28 UTC (permalink / raw)
Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20200531172804.256335-1-konradybcio@gmail.com>
This SoC's firmware does not fully support the PSCI
spec, but it's good enough to bring the cores up.
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
---
arch/arm64/boot/dts/qcom/msm8992.dtsi | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi
index b86fcfb6f463..e7354826d701 100644
--- a/arch/arm64/boot/dts/qcom/msm8992.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi
@@ -386,6 +386,11 @@ pmu {
interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4)| IRQ_TYPE_LEVEL_HIGH)>;
};
+ psci {
+ compatible = "arm,psci-0.2";
+ method = "hvc";
+ };
+
smd_rpm: smd {
compatible = "qcom,smd";
rpm {
--
2.26.2
^ permalink raw reply related
* [PATCH 10/14] arm64: dts: msm8992: Add PMU node
From: Konrad Dybcio @ 2020-05-31 17:28 UTC (permalink / raw)
Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20200531172804.256335-1-konradybcio@gmail.com>
Add the PMU so we can get proper perf event support on this SoC.
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
---
arch/arm64/boot/dts/qcom/msm8992.dtsi | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi
index ef95f5ee83db..b86fcfb6f463 100644
--- a/arch/arm64/boot/dts/qcom/msm8992.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi
@@ -381,6 +381,11 @@ smem_region: smem@6a00000 {
};
};
+ pmu {
+ compatible = "arm,cortex-a53-pmu";
+ interrupts = <GIC_PPI 7 (GIC_CPU_MASK_SIMPLE(4)| IRQ_TYPE_LEVEL_HIGH)>;
+ };
+
smd_rpm: smd {
compatible = "qcom,smd";
rpm {
--
2.26.2
^ permalink raw reply related
* [PATCH 12/14] arm64: dts: qcom: Update msm8992 pin configuration
From: Konrad Dybcio @ 2020-05-31 17:28 UTC (permalink / raw)
Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20200531172804.256335-1-konradybcio@gmail.com>
Add support for i2c2 and i2c6 interfaces, wrap up
SDC pins into pmx_sdc groups (following msm8916).
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
---
arch/arm64/boot/dts/qcom/msm8992-pins.dtsi | 149 ++++++++++++++++-----
1 file changed, 118 insertions(+), 31 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/msm8992-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8992-pins.dtsi
index c543c718c22d..17c9e048e414 100644
--- a/arch/arm64/boot/dts/qcom/msm8992-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8992-pins.dtsi
@@ -32,59 +32,146 @@ pinconf {
/* Order of pins */
/* SDC1: CLK -> 0, CMD -> 1, DATA -> 2, RCLK -> 3 */
/* SDC2: CLK -> 4, CMD -> 5, DATA -> 6 */
- sdc1_clk_on: clk-on {
- pinconf {
- pins = "sdc1_clk";
- bias-disable = <0>; /* No pull */
- drive-strength = <16>; /* 16mA */
+
+ pmx_sdc1_clk {
+ sdc1_clk_on: clk-on {
+ pinmux {
+ pins = "sdc1_clk";
+ };
+ pinconf {
+ pins = "sdc1_clk";
+ bias-disable; /* No pull */
+ drive-strength = <6>;
+ };
+ };
+
+ sdc1_clk_off: clk-off {
+ pinmux {
+ pins = "sdc1_clk";
+ };
+ pinconf {
+ pins = "sdc1_clk";
+ bias-disable; /* No pull */
+ drive-strength = <2>;
+ };
};
};
- sdc1_clk_off: clk-off {
- pinconf {
- pins = "sdc1_clk";
- bias-disable = <0>; /* No pull */
- drive-strength = <2>; /* 2mA */
+ pmx_sdc1_cmd {
+ sdc1_cmd_on: cmd-on {
+ pinmux {
+ pins = "sdc1_cmd";
+ };
+ pinconf {
+ pins = "sdc1_cmd";
+ bias-pull-up;
+ drive-strength = <6>;
+ };
+ };
+
+ sdc1_cmd_off: cmd-off {
+ pinmux {
+ pins = "sdc1_cmd";
+ };
+ pinconf {
+ pins = "sdc1_cmd";
+ bias-pull-up;
+ drive-strength = <2>;
+ };
};
};
- sdc1_cmd_on: cmd-on {
- pinconf {
- pins = "sdc1_cmd";
- bias-pull-up;
- drive-strength = <8>;
+ pmx_sdc1_data {
+ sdc1_data_on: data-on {
+ pinmux {
+ pins = "sdc1_data";
+ };
+ pinconf {
+ pins = "sdc1_data";
+ bias-pull-up;
+ drive-strength = <6>;
+ };
+ };
+
+ sdc1_data_off: data-off {
+ pinmux {
+ pins = "sdc1_data";
+ };
+ pinconf {
+ pins = "sdc1_data";
+ bias-pull-up;
+ drive-strength = <2>;
+ };
};
};
- sdc1_cmd_off: cmd-off {
- pinconf {
- pins = "sdc1_cmd";
- bias-pull-up = <0x3>; /* same as 3.10 ?? */
- drive-strength = <2>; /* 2mA */
+ pmx_sdc1_rclk {
+ sdc1_rclk_on: rclk-on {
+ pinmux {
+ pins = "sdc1_rclk";
+ };
+ pinconf {
+ pins = "sdc1_rclk";
+ bias-pull-down; /* pull down */
+ };
+ };
+
+ sdc1_rclk_off: rclk-off {
+ pinmux {
+ pins = "sdc1_rclk";
+ };
+ pinconf {
+ pins = "sdc1_rclk";
+ bias-pull-down; /* pull down */
+ };
};
};
- sdc1_data_on: data-on {
+ i2c2_default: i2c2_default {
+ pinmux {
+ function = "blsp_i2c2";
+ pins = "gpio6", "gpio7";
+ };
pinconf {
- pins = "sdc1_data";
- bias-pull-up;
- drive-strength = <8>; /* 8mA */
+ pins = "gpio6", "gpio7";
+ drive-strength = <2>;
+ bias-disable;
};
};
- sdc1_data_off: data-off {
+ i2c2_sleep: i2c2_sleep {
+ pinmux {
+ function = "gpio";
+ pins = "gpio6", "gpio7";
+ };
pinconf {
- pins = "sdc1_data";
- bias-pull-up;
+ pins = "gpio6", "gpio7";
drive-strength = <2>;
+ bias-disable;
};
};
- sdc1_rclk_on: rclk-on {
- bias-pull-down; /* pull down */
+ i2c6_default: i2c6_default {
+ pinmux {
+ function = "blsp_i2c6";
+ pins = "gpio28", "gpio27";
+ };
+ pinconf {
+ pins = "gpio28", "gpio27";
+ drive-strength = <2>;
+ bias-disable;
+ };
};
- sdc1_rclk_off: rclk-off {
- bias-pull-down; /* pull down */
+ i2c6_sleep: i2c6_sleep {
+ pinmux {
+ function = "gpio";
+ pins = "gpio28", "gpio27";
+ };
+ pinconf {
+ pins = "gpio28", "gpio27";
+ drive-strength = <2>;
+ bias-disable;
+ };
};
};
--
2.26.2
^ permalink raw reply related
* [PATCH 09/14] arm64: dts: msm8992: Add a BLSP I2C6 node
From: Konrad Dybcio @ 2020-05-31 17:27 UTC (permalink / raw)
Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20200531172804.256335-1-konradybcio@gmail.com>
This commit adds support for the sixth I2C interface
on msm8992.
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
---
arch/arm64/boot/dts/qcom/msm8992.dtsi | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/msm8992.dtsi b/arch/arm64/boot/dts/qcom/msm8992.dtsi
index 395f4c325c2f..ef95f5ee83db 100644
--- a/arch/arm64/boot/dts/qcom/msm8992.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8992.dtsi
@@ -348,6 +348,21 @@ blsp_i2c2: i2c@f9924000 {
#size-cells = <0>;
status = "disabled";
};
+
+ blsp_i2c6: i2c@f9928000 {
+ compatible = "qcom,i2c-qup-v2.2.1";
+ reg = <0xf9928000 0x500>;
+ interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&gcc GCC_BLSP1_AHB_CLK>,
+ <&gcc GCC_BLSP1_QUP6_I2C_APPS_CLK>;
+ clock-names = "iface", "core";
+ pinctrl-names = "default", "sleep";
+ pinctrl-0 = <&i2c6_default>;
+ pinctrl-1 = <&i2c6_sleep>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "ok";
+ };
};
memory {
--
2.26.2
^ permalink raw reply related
* [PATCH 05/14] Documentation: Document msm8992 qcom_scm compatible
From: Konrad Dybcio @ 2020-05-31 17:27 UTC (permalink / raw)
Cc: Konrad Dybcio, Andy Gross, Bjorn Andersson, Rob Herring,
linux-arm-msm, devicetree, linux-kernel
In-Reply-To: <20200531172804.256335-1-konradybcio@gmail.com>
Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
---
Documentation/devicetree/bindings/firmware/qcom,scm.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/firmware/qcom,scm.txt b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
index 354b448fc0c3..fd7e1aff9cdc 100644
--- a/Documentation/devicetree/bindings/firmware/qcom,scm.txt
+++ b/Documentation/devicetree/bindings/firmware/qcom,scm.txt
@@ -15,6 +15,7 @@ Required properties:
* "qcom,scm-msm8916"
* "qcom,scm-msm8960"
* "qcom,scm-msm8974"
+ * "qcom,scm-msm8992"
* "qcom,scm-msm8996"
* "qcom,scm-msm8998"
* "qcom,scm-sc7180"
--
2.26.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox