* [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 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 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 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 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 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 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
* 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
* 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 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 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 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 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 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 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: [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 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: [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: 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 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 08/12] iio: imu: inv_icm42600: add device interrupt
From: Jonathan Cameron @ 2020-05-31 12:16 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-9-jmaneyrol@invensense.com>
On Wed, 27 May 2020 20:57:07 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
> Add INT1 interrupt support. Support interrupt edge and level,
> active high or low. Push-pull or open-drain configurations.
>
> Interrupt will be used to read data from the FIFO.
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Some nitpicks inline - all cases where a blank line would slightly
help readability.
J
> ---
> drivers/iio/imu/inv_icm42600/inv_icm42600.h | 2 +-
> .../iio/imu/inv_icm42600/inv_icm42600_core.c | 96 ++++++++++++++++++-
> .../iio/imu/inv_icm42600/inv_icm42600_i2c.c | 3 +-
> .../iio/imu/inv_icm42600/inv_icm42600_spi.c | 3 +-
> 4 files changed, 100 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index c534acae0308..43749f56426c 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -372,7 +372,7 @@ int inv_icm42600_set_temp_conf(struct inv_icm42600_state *st, bool enable,
> int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
> unsigned int writeval, unsigned int *readval);
>
> -int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> +int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
> inv_icm42600_bus_setup bus_setup);
>
> int inv_icm42600_gyro_init(struct inv_icm42600_state *st);
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index e7f7835aca9b..246c1eb52231 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -9,8 +9,11 @@
> #include <linux/slab.h>
> #include <linux/delay.h>
> #include <linux/mutex.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> #include <linux/regulator/consumer.h>
> #include <linux/pm_runtime.h>
> +#include <linux/property.h>
> #include <linux/regmap.h>
> #include <linux/iio/iio.h>
>
> @@ -409,6 +412,79 @@ static int inv_icm42600_setup(struct inv_icm42600_state *st,
> return inv_icm42600_set_conf(st, hw->conf);
> }
>
> +static irqreturn_t inv_icm42600_irq_handler(int irq, void *_data)
> +{
> + struct inv_icm42600_state *st = _data;
> + struct device *dev = regmap_get_device(st->map);
> + unsigned int status;
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + ret = regmap_read(st->map, INV_ICM42600_REG_INT_STATUS, &status);
> + if (ret)
> + goto out_unlock;
> +
> + /* FIFO full */
> + if (status & INV_ICM42600_INT_STATUS_FIFO_FULL)
> + dev_warn(dev, "FIFO full data lost!\n");
> +
> +out_unlock:
> + mutex_unlock(&st->lock);
> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * inv_icm42600_irq_init() - initialize int pin and interrupt handler
> + * @st: driver internal state
> + * @irq: irq number
> + * @irq_type: irq trigger type
> + * @open_drain: true if irq is open drain, false for push-pull
> + *
> + * Returns 0 on success, a negative error code otherwise.
> + */
> +static int inv_icm42600_irq_init(struct inv_icm42600_state *st, int irq,
> + int irq_type, bool open_drain)
> +{
> + struct device *dev = regmap_get_device(st->map);
> + unsigned int val;
> + int ret;
> +
> + /* configure INT1 interrupt: default is active low on edge */
> + switch (irq_type) {
> + case IRQF_TRIGGER_RISING:
> + case IRQF_TRIGGER_HIGH:
> + val = INV_ICM42600_INT_CONFIG_INT1_ACTIVE_HIGH;
> + break;
> + default:
> + val = INV_ICM42600_INT_CONFIG_INT1_ACTIVE_LOW;
> + break;
> + }
blank line here
> + switch (irq_type) {
> + case IRQF_TRIGGER_LOW:
> + case IRQF_TRIGGER_HIGH:
> + val |= INV_ICM42600_INT_CONFIG_INT1_LATCHED;
> + break;
> + default:
> + break;
> + }
blank line here.
> + if (!open_drain)
> + val |= INV_ICM42600_INT_CONFIG_INT1_PUSH_PULL;
blank line here
> + ret = regmap_write(st->map, INV_ICM42600_REG_INT_CONFIG, val);
> + if (ret)
> + return ret;
> +
> + /* Deassert async reset for proper INT pin operation (cf datasheet) */
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INT_CONFIG1,
> + INV_ICM42600_INT_CONFIG1_ASYNC_RESET, 0);
> + if (ret)
> + return ret;
> +
> + return devm_request_threaded_irq(dev, irq, NULL,
> + inv_icm42600_irq_handler, irq_type,
> + "inv_icm42600", st);
> +}
> +
> static int inv_icm42600_enable_regulator_vddio(struct inv_icm42600_state *st)
> {
> int ret;
> @@ -453,11 +529,14 @@ static void inv_icm42600_disable_pm(void *_data)
> pm_runtime_disable(dev);
> }
>
> -int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> +int inv_icm42600_core_probe(struct regmap *regmap, int chip, int irq,
> inv_icm42600_bus_setup bus_setup)
> {
> struct device *dev = regmap_get_device(regmap);
> struct inv_icm42600_state *st;
> + struct irq_data *irq_desc;
> + int irq_type;
> + bool open_drain;
> int ret;
>
> if (chip < 0 || chip >= INV_CHIP_NB) {
> @@ -465,6 +544,17 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> return -ENODEV;
> }
>
> + /* get irq properties, set trigger falling by default */
> + irq_desc = irq_get_irq_data(irq);
> + if (!irq_desc) {
> + dev_err(dev, "could not find IRQ %d\n", irq);
> + return -EINVAL;
> + }
nitpick: Blank line here
> + irq_type = irqd_get_trigger_type(irq_desc);
> + if (!irq_type)
> + irq_type = IRQF_TRIGGER_FALLING;
blank line here.
> + open_drain = device_property_read_bool(dev, "drive-open-drain");
> +
> st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> if (!st)
> return -ENOMEM;
> @@ -518,6 +608,10 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> if (ret)
> return ret;
>
> + ret = inv_icm42600_irq_init(st, irq, irq_type, open_drain);
> + if (ret)
> + return ret;
> +
> /* setup runtime power management */
> ret = pm_runtime_set_active(dev);
> if (ret)
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> index 4789cead23b3..85b1934cec60 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> @@ -64,7 +64,8 @@ static int inv_icm42600_probe(struct i2c_client *client)
> if (IS_ERR(regmap))
> return PTR_ERR(regmap);
>
> - return inv_icm42600_core_probe(regmap, chip, inv_icm42600_i2c_bus_setup);
> + return inv_icm42600_core_probe(regmap, chip, client->irq,
> + inv_icm42600_i2c_bus_setup);
> }
>
> static const struct of_device_id inv_icm42600_of_matches[] = {
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
> index a9c5e2fdbe2a..323789697a08 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_spi.c
> @@ -63,7 +63,8 @@ static int inv_icm42600_probe(struct spi_device *spi)
> if (IS_ERR(regmap))
> return PTR_ERR(regmap);
>
> - return inv_icm42600_core_probe(regmap, chip, inv_icm42600_spi_bus_setup);
> + return inv_icm42600_core_probe(regmap, chip, spi->irq,
> + inv_icm42600_spi_bus_setup);
> }
>
> static const struct of_device_id inv_icm42600_of_matches[] = {
^ permalink raw reply
* Re: [PATCH v2 04/12] iio: imu: inv_icm42600: add gyroscope IIO device
From: Jonathan Cameron @ 2020-05-31 11:54 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-5-jmaneyrol@invensense.com>
On Wed, 27 May 2020 20:57:03 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
> Add IIO device for gyroscope sensor with data polling interface.
> Attributes: raw, scale, sampling_frequency, calibbias.
>
> Gyroscope in low noise mode.
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Unusual to have a calibration offset specified in output units,
which contributes a lot of the complexity in here.
Normally those are strictly front end (output of some calibration DAC).
So if they have units (and often they don't) I'd expect them to be
the same as _raw.
We need to tidy up the docs on this though as it doesn't express
any sort of preference. It's hard to be specific as often the calibration
scales are defined - they are just like tweaking a POT on an analog
sensor board.
A few trivial other things inline, including a suggestion to modify
the layering of the driver a tiny bit during probe.
Thanks,
Jonathan
> ---
> drivers/iio/imu/inv_icm42600/inv_icm42600.h | 6 +
> .../iio/imu/inv_icm42600/inv_icm42600_core.c | 4 +
> .../iio/imu/inv_icm42600/inv_icm42600_gyro.c | 600 ++++++++++++++++++
> 3 files changed, 610 insertions(+)
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600.h b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> index 14c8ef152418..c1023d59b37b 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600.h
> @@ -120,6 +120,8 @@ struct inv_icm42600_suspended {
> * @orientation: sensor chip orientation relative to main hardware.
> * @conf: chip sensors configurations.
> * @suspended: suspended sensors configuration.
> + * @indio_gyro: gyroscope IIO device.
> + * @buffer: data transfer buffer aligned for DMA.
> */
> struct inv_icm42600_state {
> struct mutex lock;
> @@ -131,6 +133,8 @@ struct inv_icm42600_state {
> struct iio_mount_matrix orientation;
> struct inv_icm42600_conf conf;
> struct inv_icm42600_suspended suspended;
> + struct iio_dev *indio_gyro;
> + uint8_t buffer[2] ____cacheline_aligned;
> };
>
> /* Virtual register addresses: @bank on MSB (4 upper bits), @address on LSB */
> @@ -369,4 +373,6 @@ int inv_icm42600_debugfs_reg(struct iio_dev *indio_dev, unsigned int reg,
> int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> inv_icm42600_bus_setup bus_setup);
>
> +int inv_icm42600_gyro_init(struct inv_icm42600_state *st);
> +
> #endif
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> index 81b171d6782c..dccb7bcc782e 100644
> --- a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> @@ -510,6 +510,10 @@ int inv_icm42600_core_probe(struct regmap *regmap, int chip,
> if (ret)
> return ret;
>
> + ret = inv_icm42600_gyro_init(st);
> + if (ret)
> + return ret;
> +
> /* setup runtime power management */
> ret = pm_runtime_set_active(dev);
> if (ret)
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> new file mode 100644
> index 000000000000..9d9672989b23
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_gyro.c
> @@ -0,0 +1,600 @@
> +// 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 "inv_icm42600.h"
> +
> +#define INV_ICM42600_GYRO_CHAN(_modifier, _index, _ext_info) \
> + { \
> + .type = IIO_ANGL_VEL, \
> + .modified = 1, \
> + .channel2 = _modifier, \
> + .info_mask_separate = \
> + BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_CALIBBIAS), \
> + .info_mask_shared_by_type = \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_shared_by_type_available = \
> + BIT(IIO_CHAN_INFO_SCALE) | \
> + BIT(IIO_CHAN_INFO_CALIBBIAS), \
> + .info_mask_shared_by_all = \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .info_mask_shared_by_all_available = \
> + BIT(IIO_CHAN_INFO_SAMP_FREQ), \
> + .scan_index = _index, \
> + .scan_type = { \
> + .sign = 's', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_BE, \
> + }, \
> + .ext_info = _ext_info, \
> + }
> +
> +enum inv_icm42600_gyro_scan {
> + INV_ICM42600_GYRO_SCAN_X,
> + INV_ICM42600_GYRO_SCAN_Y,
> + INV_ICM42600_GYRO_SCAN_Z,
> +};
> +
> +static const struct iio_chan_spec_ext_info inv_icm42600_gyro_ext_infos[] = {
> + IIO_MOUNT_MATRIX(IIO_SHARED_BY_ALL, inv_icm42600_get_mount_matrix),
> + {},
> +};
> +
> +static const struct iio_chan_spec inv_icm42600_gyro_channels[] = {
> + INV_ICM42600_GYRO_CHAN(IIO_MOD_X, INV_ICM42600_GYRO_SCAN_X,
> + inv_icm42600_gyro_ext_infos),
> + INV_ICM42600_GYRO_CHAN(IIO_MOD_Y, INV_ICM42600_GYRO_SCAN_Y,
> + inv_icm42600_gyro_ext_infos),
> + INV_ICM42600_GYRO_CHAN(IIO_MOD_Z, INV_ICM42600_GYRO_SCAN_Z,
> + inv_icm42600_gyro_ext_infos),
> +};
> +
> +static int inv_icm42600_gyro_read_sensor(struct inv_icm42600_state *st,
> + struct iio_chan_spec const *chan,
> + int16_t *val)
> +{
> + struct device *dev = regmap_get_device(st->map);
> + struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> + unsigned int reg;
> + __be16 *data;
> + int ret;
> +
> + if (chan->type != IIO_ANGL_VEL)
> + return -EINVAL;
> +
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + reg = INV_ICM42600_REG_GYRO_DATA_X;
> + break;
> + case IIO_MOD_Y:
> + reg = INV_ICM42600_REG_GYRO_DATA_Y;
> + break;
> + case IIO_MOD_Z:
> + reg = INV_ICM42600_REG_GYRO_DATA_Z;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + pm_runtime_get_sync(dev);
> + mutex_lock(&st->lock);
> +
> + /* enable gyro sensor */
> + conf.mode = INV_ICM42600_SENSOR_MODE_LOW_NOISE;
> + ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);
> + if (ret)
> + goto exit;
> +
> + /* read gyro register data */
> + data = (__be16 *)&st->buffer[0];
> + ret = regmap_bulk_read(st->map, reg, data, sizeof(*data));
> + if (ret)
> + goto exit;
> +
> + *val = (int16_t)be16_to_cpup(data);
> + if (*val == INV_ICM42600_DATA_INVALID)
> + ret = -EINVAL;
> +exit:
> + mutex_unlock(&st->lock);
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> + return ret;
> +}
> +
> +/* IIO format int + nano */
> +static const int inv_icm42600_gyro_scale[] = {
> + /* +/- 2000dps => 0.001065264 rad/s */
> + [2 * INV_ICM42600_GYRO_FS_2000DPS] = 0,
> + [2 * INV_ICM42600_GYRO_FS_2000DPS + 1] = 1065264,
> + /* +/- 1000dps => 0.000532632 rad/s */
> + [2 * INV_ICM42600_GYRO_FS_1000DPS] = 0,
> + [2 * INV_ICM42600_GYRO_FS_1000DPS + 1] = 532632,
> + /* +/- 500dps => 0.000266316 rad/s */
> + [2 * INV_ICM42600_GYRO_FS_500DPS] = 0,
> + [2 * INV_ICM42600_GYRO_FS_500DPS + 1] = 266316,
> + /* +/- 250dps => 0.000133158 rad/s */
> + [2 * INV_ICM42600_GYRO_FS_250DPS] = 0,
> + [2 * INV_ICM42600_GYRO_FS_250DPS + 1] = 133158,
> + /* +/- 125dps => 0.000066579 rad/s */
> + [2 * INV_ICM42600_GYRO_FS_125DPS] = 0,
> + [2 * INV_ICM42600_GYRO_FS_125DPS + 1] = 66579,
> + /* +/- 62.5dps => 0.000033290 rad/s */
> + [2 * INV_ICM42600_GYRO_FS_62_5DPS] = 0,
> + [2 * INV_ICM42600_GYRO_FS_62_5DPS + 1] = 33290,
> + /* +/- 31.25dps => 0.000016645 rad/s */
> + [2 * INV_ICM42600_GYRO_FS_31_25DPS] = 0,
> + [2 * INV_ICM42600_GYRO_FS_31_25DPS + 1] = 16645,
> + /* +/- 15.625dps => 0.000008322 rad/s */
> + [2 * INV_ICM42600_GYRO_FS_15_625DPS] = 0,
> + [2 * INV_ICM42600_GYRO_FS_15_625DPS + 1] = 8322,
> +};
> +
> +static int inv_icm42600_gyro_read_scale(struct inv_icm42600_state *st,
> + int *val, int *val2)
> +{
> + unsigned int idx;
> +
> + idx = st->conf.gyro.fs;
> +
> + *val = inv_icm42600_gyro_scale[2 * idx];
> + *val2 = inv_icm42600_gyro_scale[2 * idx + 1];
> + return IIO_VAL_INT_PLUS_NANO;
> +}
> +
> +static int inv_icm42600_gyro_write_scale(struct inv_icm42600_state *st,
> + int val, int val2)
> +{
> + struct device *dev = regmap_get_device(st->map);
> + unsigned int idx;
> + struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> + int ret;
> +
> + for (idx = 0; idx < ARRAY_SIZE(inv_icm42600_gyro_scale); idx += 2) {
> + if (val == inv_icm42600_gyro_scale[idx] &&
> + val2 == inv_icm42600_gyro_scale[idx + 1])
> + break;
> + }
> + if (idx >= ARRAY_SIZE(inv_icm42600_gyro_scale))
> + return -EINVAL;
> +
> + conf.fs = idx / 2;
> +
> + pm_runtime_get_sync(dev);
> + mutex_lock(&st->lock);
> +
> + ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);
> +
> + mutex_unlock(&st->lock);
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +
> + return ret;
> +}
> +
> +/* IIO format int + micro */
> +static const int inv_icm42600_gyro_odr[] = {
> + /* 12.5Hz */
> + 12, 500000,
> + /* 25Hz */
> + 25, 0,
> + /* 50Hz */
> + 50, 0,
> + /* 100Hz */
> + 100, 0,
> + /* 200Hz */
> + 200, 0,
> + /* 1kHz */
> + 1000, 0,
> + /* 2kHz */
> + 2000, 0,
> + /* 4kHz */
> + 4000, 0,
> +};
> +
> +static const int inv_icm42600_gyro_odr_conv[] = {
> + INV_ICM42600_ODR_12_5HZ,
> + INV_ICM42600_ODR_25HZ,
> + INV_ICM42600_ODR_50HZ,
> + INV_ICM42600_ODR_100HZ,
> + INV_ICM42600_ODR_200HZ,
> + INV_ICM42600_ODR_1KHZ_LN,
> + INV_ICM42600_ODR_2KHZ_LN,
> + INV_ICM42600_ODR_4KHZ_LN,
> +};
> +
> +static int inv_icm42600_gyro_read_odr(struct inv_icm42600_state *st,
> + int *val, int *val2)
> +{
> + unsigned int odr;
> + unsigned int i;
> +
> + odr = st->conf.gyro.odr;
> +
> + for (i = 0; i < ARRAY_SIZE(inv_icm42600_gyro_odr_conv); ++i) {
> + if (inv_icm42600_gyro_odr_conv[i] == odr)
> + break;
> + }
> + if (i >= ARRAY_SIZE(inv_icm42600_gyro_odr_conv))
> + return -EINVAL;
> +
> + *val = inv_icm42600_gyro_odr[2 * i];
> + *val2 = inv_icm42600_gyro_odr[2 * i + 1];
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int inv_icm42600_gyro_write_odr(struct inv_icm42600_state *st,
> + int val, int val2)
> +{
> + struct device *dev = regmap_get_device(st->map);
> + unsigned int idx;
> + struct inv_icm42600_sensor_conf conf = INV_ICM42600_SENSOR_CONF_INIT;
> + int ret;
> +
> + for (idx = 0; idx < ARRAY_SIZE(inv_icm42600_gyro_odr); idx += 2) {
> + if (val == inv_icm42600_gyro_odr[idx] &&
> + val2 == inv_icm42600_gyro_odr[idx + 1])
> + break;
> + }
> + if (idx >= ARRAY_SIZE(inv_icm42600_gyro_odr))
> + return -EINVAL;
> +
> + conf.odr = inv_icm42600_gyro_odr_conv[idx / 2];
> +
> + pm_runtime_get_sync(dev);
> + mutex_lock(&st->lock);
> +
> + ret = inv_icm42600_set_gyro_conf(st, &conf, NULL);
> +
> + mutex_unlock(&st->lock);
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> +
> + return ret;
> +}
> +
> +/*
> + * Calibration bias values, IIO range format int + nano.
> + * Value is limited to +/-64dps coded on 12 bits signed. Step is 1/32 dps.
> + */
> +static int inv_icm42600_gyro_calibbias[] = {
> + -1, 117010721, /* min: -1.117010721 rad/s */
> + 0, 545415, /* step: 0.000545415 rad/s */
> + 1, 116465306, /* max: 1.116465306 rad/s */
> +};
> +
> +static int inv_icm42600_gyro_read_offset(struct inv_icm42600_state *st,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2)
> +{
> + struct device *dev = regmap_get_device(st->map);
> + int64_t val64;
> + int32_t bias;
> + unsigned int reg;
> + int16_t offset;
> + uint8_t data[2];
> + int ret;
> +
> + if (chan->type != IIO_ANGL_VEL)
> + return -EINVAL;
> +
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + reg = INV_ICM42600_REG_OFFSET_USER0;
> + break;
> + case IIO_MOD_Y:
> + reg = INV_ICM42600_REG_OFFSET_USER1;
> + break;
> + case IIO_MOD_Z:
> + reg = INV_ICM42600_REG_OFFSET_USER3;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + pm_runtime_get_sync(dev);
> + mutex_lock(&st->lock);
> +
> + ret = regmap_bulk_read(st->map, reg, st->buffer, sizeof(data));
> + memcpy(data, st->buffer, sizeof(data));
> +
> + mutex_unlock(&st->lock);
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> + if (ret)
> + return ret;
> +
> + /* 12 bits signed value */
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + offset = sign_extend32(((data[1] & 0x0F) << 8) | data[0], 11);
> + break;
> + case IIO_MOD_Y:
> + offset = sign_extend32(((data[0] & 0xF0) << 4) | data[1], 11);
> + break;
> + case IIO_MOD_Z:
> + offset = sign_extend32(((data[1] & 0x0F) << 8) | data[0], 11);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + /*
> + * convert raw offset to dps then to rad/s
> + * 12 bits signed raw max 64 to dps: 64 / 2048
> + * dps to rad: Pi / 180
> + * result in nano (1000000000)
> + * (offset * 64 * Pi * 1000000000) / (2048 * 180)
> + */
> + val64 = (int64_t)offset * 64LL * 3141592653LL;
> + /* for rounding, add + or - divisor (2048 * 180) divided by 2 */
> + if (val64 >= 0)
> + val64 += 2048 * 180 / 2;
> + else
> + val64 -= 2048 * 180 / 2;
> + bias = div_s64(val64, 2048 * 180);
> + *val = bias / 1000000000L;
> + *val2 = bias % 1000000000L;
> +
> + return IIO_VAL_INT_PLUS_NANO;
> +}
> +
> +static int inv_icm42600_gyro_write_offset(struct inv_icm42600_state *st,
> + struct iio_chan_spec const *chan,
> + int val, int val2)
> +{
> + struct device *dev = regmap_get_device(st->map);
> + int64_t val64, min, max;
> + unsigned int reg, regval;
> + int16_t offset;
> + int ret;
> +
> + if (chan->type != IIO_ANGL_VEL)
> + return -EINVAL;
> +
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + reg = INV_ICM42600_REG_OFFSET_USER0;
> + break;
> + case IIO_MOD_Y:
> + reg = INV_ICM42600_REG_OFFSET_USER1;
> + break;
> + case IIO_MOD_Z:
> + reg = INV_ICM42600_REG_OFFSET_USER3;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + /* inv_icm42600_gyro_calibbias: min - step - max in nano */
> + min = (int64_t)inv_icm42600_gyro_calibbias[0] * 1000000000LL +
> + (int64_t)inv_icm42600_gyro_calibbias[1];
> + max = (int64_t)inv_icm42600_gyro_calibbias[4] * 1000000000LL +
> + (int64_t)inv_icm42600_gyro_calibbias[5];
> + val64 = (int64_t)val * 1000000000LL + (int64_t)val2;
> + if (val64 < min || val64 > max)
> + return -EINVAL;
> +
> + /*
> + * convert rad/s to dps then to raw value
> + * rad to dps: 180 / Pi
> + * dps to raw 12 bits signed, max 64: 2048 / 64
> + * val in nano (1000000000)
> + * val * 180 * 2048 / (Pi * 1000000000 * 64)
> + */
> + val64 = val64 * 180LL * 2048LL;
> + /* for rounding, add + or - divisor (3141592653 * 64) divided by 2 */
> + if (val64 >= 0)
> + val64 += 3141592653LL * 64LL / 2LL;
> + else
> + val64 -= 3141592653LL * 64LL / 2LL;
> + offset = div64_s64(val64, 3141592653LL * 64LL);
> +
> + /* clamp value limited to 12 bits signed */
> + if (offset < -2048)
> + offset = -2048;
> + else if (offset > 2047)
> + offset = 2047;
> +
> + pm_runtime_get_sync(dev);
> + mutex_lock(&st->lock);
> +
> + switch (chan->channel2) {
> + case IIO_MOD_X:
> + /* OFFSET_USER1 register is shared */
> + ret = regmap_read(st->map, INV_ICM42600_REG_OFFSET_USER1,
> + ®val);
> + if (ret)
> + goto out_unlock;
> + st->buffer[0] = offset & 0xFF;
> + st->buffer[1] = (regval & 0xF0) | ((offset & 0xF00) >> 8);
> + break;
> + case IIO_MOD_Y:
> + /* OFFSET_USER1 register is shared */
> + ret = regmap_read(st->map, INV_ICM42600_REG_OFFSET_USER1,
> + ®val);
> + if (ret)
> + goto out_unlock;
> + st->buffer[0] = ((offset & 0xF00) >> 4) | (regval & 0x0F);
> + st->buffer[1] = offset & 0xFF;
> + break;
> + case IIO_MOD_Z:
> + /* OFFSET_USER4 register is shared */
> + ret = regmap_read(st->map, INV_ICM42600_REG_OFFSET_USER4,
> + ®val);
> + if (ret)
> + goto out_unlock;
> + st->buffer[0] = offset & 0xFF;
> + st->buffer[1] = (regval & 0xF0) | ((offset & 0xF00) >> 8);
> + break;
> + default:
> + ret = -EINVAL;
> + goto out_unlock;
> + }
> +
> + ret = regmap_bulk_write(st->map, reg, st->buffer, 2);
> +
> +out_unlock:
> + mutex_unlock(&st->lock);
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
> + return ret;
> +}
> +
> +static int inv_icm42600_gyro_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + int16_t data;
> + int ret;
> +
> + if (chan->type != IIO_ANGL_VEL)
> + return -EINVAL;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> + ret = inv_icm42600_gyro_read_sensor(st, chan, &data);
> + iio_device_release_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> + *val = data;
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + return inv_icm42600_gyro_read_scale(st, val, val2);
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return inv_icm42600_gyro_read_odr(st, val, val2);
> + case IIO_CHAN_INFO_CALIBBIAS:
> + return inv_icm42600_gyro_read_offset(st, chan, val, val2);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int inv_icm42600_gyro_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals,
> + int *type, int *length, long mask)
> +{
> + if (chan->type != IIO_ANGL_VEL)
> + return -EINVAL;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + *vals = inv_icm42600_gyro_scale;
> + *type = IIO_VAL_INT_PLUS_NANO;
> + *length = ARRAY_SIZE(inv_icm42600_gyro_scale);
> + return IIO_AVAIL_LIST;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *vals = inv_icm42600_gyro_odr;
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + *length = ARRAY_SIZE(inv_icm42600_gyro_odr);
> + return IIO_AVAIL_LIST;
> + case IIO_CHAN_INFO_CALIBBIAS:
> + *vals = inv_icm42600_gyro_calibbias;
> + *type = IIO_VAL_INT_PLUS_NANO;
> + return IIO_AVAIL_RANGE;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int inv_icm42600_gyro_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct inv_icm42600_state *st = iio_device_get_drvdata(indio_dev);
> + int ret;
> +
> + if (chan->type != IIO_ANGL_VEL)
> + return -EINVAL;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> + ret = inv_icm42600_gyro_write_scale(st, val, val2);
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return inv_icm42600_gyro_write_odr(st, val, val2);
> + case IIO_CHAN_INFO_CALIBBIAS:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> + ret = inv_icm42600_gyro_write_offset(st, chan, val, val2);
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int inv_icm42600_gyro_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + long mask)
> +{
> + if (chan->type != IIO_ANGL_VEL)
> + return -EINVAL;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_CALIBBIAS:
> + return IIO_VAL_INT_PLUS_NANO;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +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,
> +};
> +
> +int inv_icm42600_gyro_init(struct inv_icm42600_state *st)
This feels like the layering would be clearer if this
returned the struct iio_dev * and the assignment happened in the
core driver.
Then state parameter can be const and it'll be obvious it has
no side effects on the state structure.
> +{
> + struct device *dev = regmap_get_device(st->map);
> + const char *name;
> + struct iio_dev *indio_dev;
> +
> + name = devm_kasprintf(dev, GFP_KERNEL, "%s-gyro", st->name);
> + if (!name)
> + return -ENOMEM;
> +
> + indio_dev = devm_iio_device_alloc(dev, 0);
> + if (!indio_dev)
> + 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->channels = inv_icm42600_gyro_channels;
> + indio_dev->num_channels = ARRAY_SIZE(inv_icm42600_gyro_channels);
> +
> + st->indio_gyro = indio_dev;
> + return devm_iio_device_register(dev, st->indio_gyro);
> +}
^ permalink raw reply
* Re: [PATCH v2 02/12] iio: imu: inv_icm42600: add I2C driver for inv_icm42600 driver
From: Jonathan Cameron @ 2020-05-31 11:36 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-3-jmaneyrol@invensense.com>
On Wed, 27 May 2020 20:57:01 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
> Add I2C driver for InvenSense ICM-426xxx devices.
>
> Configure bus signal slew rates as indicated in the datasheet.
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Looks fine to me.
J
> ---
> .../iio/imu/inv_icm42600/inv_icm42600_i2c.c | 100 ++++++++++++++++++
> 1 file changed, 100 insertions(+)
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
>
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> new file mode 100644
> index 000000000000..4789cead23b3
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_i2c.c
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2020 InvenSense, Inc.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/property.h>
> +
> +#include "inv_icm42600.h"
> +
> +static int inv_icm42600_i2c_bus_setup(struct inv_icm42600_state *st)
> +{
> + unsigned int mask, val;
> + int ret;
> +
> + /* setup interface registers */
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG6,
> + INV_ICM42600_INTF_CONFIG6_MASK,
> + INV_ICM42600_INTF_CONFIG6_I3C_EN);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG4,
> + INV_ICM42600_INTF_CONFIG4_I3C_BUS_ONLY, 0);
> + if (ret)
> + return ret;
> +
> + /* set slew rates for I2C and SPI */
> + mask = INV_ICM42600_DRIVE_CONFIG_I2C_MASK |
> + INV_ICM42600_DRIVE_CONFIG_SPI_MASK;
> + val = INV_ICM42600_DRIVE_CONFIG_I2C(INV_ICM42600_SLEW_RATE_12_36NS) |
> + INV_ICM42600_DRIVE_CONFIG_SPI(INV_ICM42600_SLEW_RATE_12_36NS);
> + ret = regmap_update_bits(st->map, INV_ICM42600_REG_DRIVE_CONFIG,
> + mask, val);
> + if (ret)
> + return ret;
> +
> + /* disable SPI bus */
> + return regmap_update_bits(st->map, INV_ICM42600_REG_INTF_CONFIG0,
> + INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_MASK,
> + INV_ICM42600_INTF_CONFIG0_UI_SIFS_CFG_SPI_DIS);
> +}
> +
> +static int inv_icm42600_probe(struct i2c_client *client)
> +{
> + const void *match;
> + enum inv_icm42600_chip chip;
> + struct regmap *regmap;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_I2C_BLOCK))
> + return -ENOTSUPP;
> +
> + match = device_get_match_data(&client->dev);
> + if (!match)
> + return -EINVAL;
> + chip = (enum inv_icm42600_chip)match;
> +
> + regmap = devm_regmap_init_i2c(client, &inv_icm42600_regmap_config);
> + if (IS_ERR(regmap))
> + return PTR_ERR(regmap);
> +
> + return inv_icm42600_core_probe(regmap, chip, inv_icm42600_i2c_bus_setup);
> +}
> +
> +static const struct of_device_id inv_icm42600_of_matches[] = {
> + {
> + .compatible = "invensense,icm42600",
> + .data = (void *)INV_CHIP_ICM42600,
> + }, {
> + .compatible = "invensense,icm42602",
> + .data = (void *)INV_CHIP_ICM42602,
> + }, {
> + .compatible = "invensense,icm42605",
> + .data = (void *)INV_CHIP_ICM42605,
> + }, {
> + .compatible = "invensense,icm42622",
> + .data = (void *)INV_CHIP_ICM42622,
> + },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, inv_icm42600_of_matches);
> +
> +static struct i2c_driver inv_icm42600_driver = {
> + .driver = {
> + .name = "inv-icm42600-i2c",
> + .of_match_table = inv_icm42600_of_matches,
> + .pm = &inv_icm42600_pm_ops,
> + },
> + .probe_new = inv_icm42600_probe,
> +};
> +module_i2c_driver(inv_icm42600_driver);
> +
> +MODULE_AUTHOR("InvenSense, Inc.");
> +MODULE_DESCRIPTION("InvenSense ICM-426xx I2C driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply
* Re: [PATCH v2 01/12] iio: imu: inv_icm42600: add core of new inv_icm42600 driver
From: Jonathan Cameron @ 2020-05-31 11:34 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-2-jmaneyrol@invensense.com>
On Wed, 27 May 2020 20:57:00 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:
> Core component of a new driver for InvenSense ICM-426xx devices.
> It includes registers definition, main probe/setup, and device
> utility functions.
>
> ICM-426xx devices are latest generation of 6-axis IMU,
> gyroscope+accelerometer and temperature sensor. This device
> includes a 2K FIFO, supports I2C/I3C/SPI, and provides
> intelligent motion features like pedometer, tilt detection,
> and tap detection.
>
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
A few things inline.
Either I'm missing something or I'm guessing vddio is not controllable
on your test board.
> ---
> drivers/iio/imu/inv_icm42600/inv_icm42600.h | 372 ++++++++++
> .../iio/imu/inv_icm42600/inv_icm42600_core.c | 635 ++++++++++++++++++
> 2 files changed, 1007 insertions(+)
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600.h
> create mode 100644 drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
>
...
> diff --git a/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> new file mode 100644
> index 000000000000..81b171d6782c
> --- /dev/null
> +++ b/drivers/iio/imu/inv_icm42600/inv_icm42600_core.c
> +const struct iio_mount_matrix *
> +inv_icm42600_get_mount_matrix(const struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan)
> +{
> + const struct inv_icm42600_state *st =
> + iio_device_get_drvdata((struct iio_dev *)indio_dev);
If you review my patch to the core, I can get that applied and we can drop
the ugly cast from here!
Just waiting for someone to sanity check it.
> +
> + return &st->orientation;
> +}
...
> +/* Runtime suspend will turn off sensors that are enabled by iio devices. */
> +static int __maybe_unused inv_icm42600_runtime_suspend(struct device *dev)
> +{
> + struct inv_icm42600_state *st = dev_get_drvdata(dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + /* disable all sensors */
> + ret = inv_icm42600_set_pwr_mgmt0(st, INV_ICM42600_SENSOR_MODE_OFF,
> + INV_ICM42600_SENSOR_MODE_OFF, false,
> + NULL);
> + if (ret)
> + goto error_unlock;
> +
> + regulator_disable(st->vddio_supply);
Don't seem to turn this on again in runtime_resume..
Why? Definitely needs at least a comment.
> +
> +error_unlock:
> + mutex_unlock(&st->lock);
> + return ret;
> +}
> +
> +/* Sensors are enabled by iio devices, no need to turn them back on here. */
> +static int __maybe_unused inv_icm42600_runtime_resume(struct device *dev)
> +{
> + struct inv_icm42600_state *st = dev_get_drvdata(dev);
> + int ret;
> +
> + mutex_lock(&st->lock);
> +
> + ret = inv_icm42600_enable_regulator_vddio(st);
> +
> + mutex_unlock(&st->lock);
> + return ret;
> +}
> +
> +const struct dev_pm_ops inv_icm42600_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(inv_icm42600_suspend, inv_icm42600_resume)
> + SET_RUNTIME_PM_OPS(inv_icm42600_runtime_suspend,
> + inv_icm42600_runtime_resume, NULL)
> +};
> +EXPORT_SYMBOL_GPL(inv_icm42600_pm_ops);
> +
> +MODULE_AUTHOR("InvenSense, Inc.");
> +MODULE_DESCRIPTION("InvenSense ICM-426xx device driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply
* [PATCH] arm: allwinner: a20: Add Drejo DS167 initial support
From: stulluk @ 2020-05-31 11:05 UTC (permalink / raw)
To: mripard; +Cc: robh+dt, wens, devicetree, linux-kernel, linux-sunxi,
Sertac TULLUK
From: Sertac TULLUK <stulluk@gmail.com>
Drejo DS167 is an Allwinner A20 based IoT device,
which support
- Allwinner A20 Cortex-A7
- Mali-400MP2 GPU
- AXP209 PMIC
- 1GB DDR3 RAM
- 8GB eMMC
- 10/100M Ethernet
- SATA
- HDMI
- 10.1inch and 7.0inch LVDS LCD and Touch screens
- CSI: OV5640 sensor
- USB OTG
- 2x USB2.0
- built-in KNX Transceiver
- 3x Dry Contact Input
- 3x Relay output
- IR RX/TX
- UART3
- SPI1
- RTC Battery
- 8x GPIO
- Analogue + Digital Microphone
- PAM8620 speaker Amplifier
- 12V DC power supply
- 3.7V Battery Operable
Signed-off-by: Sertac TULLUK <stulluk@gmail.com>
---
arch/arm/boot/dts/Makefile | 2 +
.../boot/dts/sun7i-a20-drejo-ds167-emmc.dts | 69 +++++
arch/arm/boot/dts/sun7i-a20-drejo-ds167.dts | 288 ++++++++++++++++++
3 files changed, 359 insertions(+)
create mode 100644 arch/arm/boot/dts/sun7i-a20-drejo-ds167-emmc.dts
create mode 100644 arch/arm/boot/dts/sun7i-a20-drejo-ds167.dts
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 3823090d07e7..d81e685dee38 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -1097,6 +1097,8 @@ dtb-$(CONFIG_MACH_SUN7I) += \
sun7i-a20-bananapro.dtb \
sun7i-a20-cubieboard2.dtb \
sun7i-a20-cubietruck.dtb \
+ sun7i-a20-drejo-ds167.dtb \
+ sun7i-a20-drejo-ds167-emmc.dtb \
sun7i-a20-hummingbird.dtb \
sun7i-a20-itead-ibox.dtb \
sun7i-a20-i12-tvbox.dtb \
diff --git a/arch/arm/boot/dts/sun7i-a20-drejo-ds167-emmc.dts b/arch/arm/boot/dts/sun7i-a20-drejo-ds167-emmc.dts
new file mode 100644
index 000000000000..b6147eb013b0
--- /dev/null
+++ b/arch/arm/boot/dts/sun7i-a20-drejo-ds167-emmc.dts
@@ -0,0 +1,69 @@
+/*
+ * Copyright 2020 Sertac TULLUK
+ *
+ * Sertac TULLUK <stulluk@gmail.com>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ * a) This file is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This file is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Or, alternatively,
+ *
+ * b) Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+#include "sun7i-a20-drejo-ds167.dts"
+
+/ {
+ model = "drejo DS167-eMMC";
+ compatible = "drejo,sun7i-a20-drejo-ds167-emmc", "allwinner,sun7i-a20";
+
+ mmc2_pwrseq: pwrseq {
+ compatible = "mmc-pwrseq-emmc";
+ reset-gpios = <&pio 2 24 GPIO_ACTIVE_LOW>;
+ };
+};
+
+&mmc2 {
+ vmmc-supply = <®_vcc3v3>;
+ bus-width = <4>;
+ non-removable;
+ mmc-pwrseq = <&mmc2_pwrseq>;
+ status = "okay";
+
+ emmc: emmc@0 {
+ reg = <0>;
+ compatible = "mmc-card";
+ broken-hpi;
+ };
+};
diff --git a/arch/arm/boot/dts/sun7i-a20-drejo-ds167.dts b/arch/arm/boot/dts/sun7i-a20-drejo-ds167.dts
new file mode 100644
index 000000000000..79db92f88251
--- /dev/null
+++ b/arch/arm/boot/dts/sun7i-a20-drejo-ds167.dts
@@ -0,0 +1,288 @@
+/*
+ * Copyright 2020 Sertac TULLUK
+ *
+ * Sertac TULLUK <stulluk@gmail.com>
+ *
+ * This file is dual-licensed: you can use it either under the terms
+ * of the GPL or the X11 license, at your option. Note that this dual
+ * licensing only applies to this file, and not this project as a
+ * whole.
+ *
+ * a) This file is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This file is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * Or, alternatively,
+ *
+ * b) Permission is hereby granted, free of charge, to any person
+ * obtaining a copy of this software and associated documentation
+ * files (the "Software"), to deal in the Software without
+ * restriction, including without limitation the rights to use,
+ * copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following
+ * conditions:
+ *
+ * The above copyright notice and this permission notice shall be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
+ * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
+ * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
+ * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/dts-v1/;
+#include "sun7i-a20.dtsi"
+#include "sunxi-common-regulators.dtsi"
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+
+/ {
+ model = "drejo DS167";
+ compatible = "drejo,sun7i-a20-drejo-ds167", "allwinner,sun7i-a20";
+
+ aliases {
+ serial0 = &uart0;
+ serial1 = &uart3;
+
+ spi0 = &spi1;
+ spi1 = &spi2;
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ hdmi-connector {
+ compatible = "hdmi-connector";
+ type = "a";
+
+ port {
+ hdmi_con_in: endpoint {
+ remote-endpoint = <&hdmi_out_con>;
+ };
+ };
+ };
+
+ leds {
+ compatible = "gpio-leds";
+ pinctrl-names = "default";
+ pinctrl-0 = <&led_pins_ds167>;
+
+ red {
+ label = "ds167-status";
+ gpios = <&pio 8 9 GPIO_ACTIVE_LOW>; /* PI9 STATUS LED NEAR A20 SERTAC */
+ default-state = "on";
+ };
+ };
+};
+
+&ahci {
+ target-supply = <®_ahci_5v>;
+ status = "okay";
+};
+
+&codec {
+ status = "okay";
+};
+
+&cpu0 {
+ cpu-supply = <®_dcdc2>;
+};
+
+&de {
+ status = "okay";
+};
+
+&ehci0 {
+ status = "okay";
+};
+
+&ehci1 {
+ status = "okay";
+};
+
+&gmac {
+ pinctrl-names = "default";
+ pinctrl-0 = <&gmac_mii_pins>, <&gmac_txerr>;
+ phy-handle = <&phy1>;
+ phy-mode = "mii";
+ status = "okay";
+};
+
+&hdmi {
+ status = "okay";
+};
+
+&hdmi_out {
+ hdmi_out_con: endpoint {
+ remote-endpoint = <&hdmi_con_in>;
+ };
+};
+
+&i2c0 {
+ status = "okay";
+
+ axp209: pmic@34 {
+ reg = <0x34>;
+ interrupt-parent = <&nmi_intc>;
+ interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+ };
+};
+
+&i2c2 {
+ status = "okay";
+};
+
+&lradc {
+ vref-supply = <®_vcc3v0>;
+ status = "okay";
+};
+
+&gmac_mdio {
+ phy1: ethernet-phy@1 {
+ reg = <1>;
+ };
+};
+
+&mmc0 {
+ vmmc-supply = <®_vcc3v3>;
+ bus-width = <4>;
+ cd-gpios = <&pio 7 1 GPIO_ACTIVE_LOW>; /* PH1 */
+ status = "okay";
+};
+
+&ohci0 {
+ status = "okay";
+};
+
+&ohci1 {
+ status = "okay";
+};
+
+&otg_sram {
+ status = "okay";
+};
+
+&pio {
+ gmac_txerr: gmac-txerr-pin {
+ pins = "PA17";
+ function = "gmac";
+ };
+
+ led_pins_ds167: led-pins {
+ pins = "PI9"; /* Status led, on schematic, this is LED_EN */
+ function = "gpio_out";
+ drive-strength = <20>;
+ };
+};
+
+&pwm {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pwm0_pin>, <&pwm1_pin>;
+ status = "okay";
+};
+
+#include "axp209.dtsi"
+
+&ac_power_supply {
+ status = "okay";
+};
+
+&battery_power_supply {
+ status = "okay";
+};
+
+®_dcdc2 {
+ regulator-always-on;
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <1400000>;
+ regulator-name = "vdd-cpu";
+};
+
+®_dcdc3 {
+ regulator-always-on;
+ regulator-min-microvolt = <1000000>;
+ regulator-max-microvolt = <1400000>;
+ regulator-name = "vdd-int-dll";
+};
+
+®_ldo2 {
+ regulator-always-on;
+ regulator-min-microvolt = <3000000>;
+ regulator-max-microvolt = <3000000>;
+ regulator-name = "avcc";
+};
+
+®_ahci_5v {
+ status = "okay";
+};
+
+®_usb0_vbus {
+ status = "okay";
+};
+
+®_usb1_vbus {
+ status = "okay";
+};
+
+®_usb2_vbus {
+ status = "okay";
+};
+
+&spi1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&spi1_pi_pins>,
+ <&spi1_cs0_pi_pin>;
+ status = "okay";
+};
+
+&spi2 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&spi2_pc_pins>,
+ <&spi2_cs0_pc_pin>;
+ status = "okay";
+};
+
+&uart0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart0_pb_pins>;
+ status = "okay";
+};
+
+&uart3 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart3_pg_pins>;
+ status = "okay";
+};
+
+&usb_otg {
+ dr_mode = "otg";
+ status = "okay";
+};
+
+&usb_power_supply {
+ status = "okay";
+};
+
+&usbphy {
+ usb0_id_det-gpios = <&pio 7 4 (GPIO_ACTIVE_HIGH | GPIO_PULL_UP)>; /* PH4 */
+ usb0_vbus_det-gpios = <&pio 7 5 (GPIO_ACTIVE_HIGH | GPIO_PULL_DOWN)>; /* PH5 */
+ usb0_vbus-supply = <®_usb0_vbus>;
+ usb1_vbus-supply = <®_usb1_vbus>;
+ usb2_vbus-supply = <®_usb2_vbus>;
+ status = "okay";
+};
--
2.17.1
^ 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