This series adds support for video capture from external camera sensor to
Tegra video driver.
Jetson TX1 has camera expansion connector and supports custom camera module
designed as per TX1 design specification.
This series also enables camera capture support for Jetson Nano which has
Raspberry PI camera header.
This series is tested with IMX219 camera sensor.
This series include,
VI I2C related fixes
- Camera sensor programming happens through VI I2C which is on host1x bus.
- These patches includes device tree and I2C driver fixes for VI I2C.
Tegra video driver updates
- TPG Vs Non-TPG based on Kconfig
- Support for external sensor video capture based on device graph from DT.
- Support for selection ioctl operations
- Tegra MIPI CSI pads calibration
- CSI T-CLK and T-HS settle time computation based on clock rates.
Host1x driver updates
- Adds API to allow creating mipi device for specific device node.
- Splits MIPI pads calibrate start and waiting for calibration to be done.
Device tree updates
- Adds camera connector 2V8, 1V8, 1V2 regulator supplies to Jetson TX1 DT.
- Enabled VI and CSI support in Jetson Nano DT.
Sowjanya Komatineni (18):
dt-bindings: i2c: tegra: Document Tegra210 VI I2C clocks and
power-domains
arm64: tegra: Add missing clocks and power-domains to Tegra210 VI I2C
i2c: tegra: Don't mark VI I2C as IRQ safe runtime PM
i2c: tegra: Fix the error path in tegra_i2c_runtime_resume
i2c: tegra: Fix runtime resume to re-init VI I2C
i2c: tegra: Avoid tegra_i2c_init_dma() for Tegra210 vi i2c
media: tegra-video: Fix channel format alignment
media: tegra-video: Enable TPG based on kernel config
media: tegra-video: Update format lookup to offset based
dt-bindings: tegra: Document VI and CSI port nodes
media: tegra-video: Add support for external sensor capture
media: tegra-video: Add support for selection ioctl ops
gpu: host1x: mipi: Add of_tegra_mipi_request() API
gpu: host1x: mipi: Split tegra_mipi_calibrate and tegra_mipi_wait
media: tegra-video: Add CSI MIPI pads calibration
media: tegra-video: Compute settle times based on the clock rate
arm64: tegra: jetson-tx1: Add camera supplies
arm64: tegra: Enable Tegra VI CSI support for Jetson Nano
.../display/tegra/nvidia,tegra20-host1x.txt | 87 +++
.../devicetree/bindings/i2c/nvidia,tegra20-i2c.txt | 19 +-
arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi | 41 ++
arch/arm64/boot/dts/nvidia/tegra210-p3450-0000.dts | 10 +
arch/arm64/boot/dts/nvidia/tegra210.dtsi | 6 +
drivers/gpu/drm/tegra/dsi.c | 7 +-
drivers/gpu/host1x/mipi.c | 33 +-
drivers/i2c/busses/i2c-tegra.c | 41 +-
drivers/staging/media/tegra-video/Kconfig | 7 +
drivers/staging/media/tegra-video/csi.c | 245 ++++++-
drivers/staging/media/tegra-video/csi.h | 9 +
drivers/staging/media/tegra-video/tegra210.c | 25 +-
drivers/staging/media/tegra-video/vi.c | 770 +++++++++++++++++++--
drivers/staging/media/tegra-video/vi.h | 23 +-
drivers/staging/media/tegra-video/video.c | 23 +-
include/linux/host1x.h | 3 +
16 files changed, 1253 insertions(+), 96 deletions(-)
--
2.7.4
This patch documents missing clocks and power-domains of Tegra210 VI I2C.
Signed-off-by: Sowjanya Komatineni <[email protected]>
---
.../devicetree/bindings/i2c/nvidia,tegra20-i2c.txt | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.txt b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.txt
index 18c0de3..3f2f990 100644
--- a/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra20-i2c.txt
@@ -35,12 +35,12 @@ Required properties:
Due to above changes, Tegra114 I2C driver makes incompatible with
previous hardware driver. Hence, tegra114 I2C controller is compatible
with "nvidia,tegra114-i2c".
- nvidia,tegra210-i2c-vi: Tegra210 has one I2C controller that is part of the
- host1x domain and typically used for camera use-cases. This VI I2C
- controller is mostly compatible with the programming model of the
- regular I2C controllers with a few exceptions. The I2C registers start
- at an offset of 0xc00 (instead of 0), registers are 16 bytes apart
- (rather than 4) and the controller does not support slave mode.
+ nvidia,tegra210-i2c-vi: Tegra210 has one I2C controller that is on host1x bus
+ and is part of VE power domain and typically used for camera use-cases.
+ This VI I2C controller is mostly compatible with the programming model
+ of the regular I2C controllers with a few exceptions. The I2C registers
+ start at an offset of 0xc00 (instead of 0), registers are 16 bytes
+ apart (rather than 4) and the controller does not support slave mode.
- reg: Should contain I2C controller registers physical address and length.
- interrupts: Should contain I2C controller interrupts.
- address-cells: Address cells for I2C device address.
@@ -53,10 +53,17 @@ Required properties:
- fast-clk
Tegra114:
- div-clk
+ Tegra210:
+ - div-clk
+ - slow (only for nvidia,tegra210-i2c-vi compatible node)
- resets: Must contain an entry for each entry in reset-names.
See ../reset/reset.txt for details.
- reset-names: Must include the following entries:
- i2c
+- power-domains: Only for nvidia,tegra210-i2c-vi compatible node and must
+ include venc powergate node as vi i2c is part of VE power domain.
+ tegra210-i2c-vi:
+ - pd_venc
- dmas: Must contain an entry for each entry in clock-names.
See ../dma/dma.txt for details.
- dma-names: Must include the following entries:
--
2.7.4
Jetson TX1 development board has a camera expansion connector which
has 2V8, 1V8 and 1V2 supplies to power up the camera sensor on the
supported camera modules.
Camera module designed as per Jetson TX1 camera expansion connector
may use these supplies for camera sensor avdd 2V8, digital core 1V8,
and digital interface 1V2 voltages.
These supplies are from fixed regulators on TX1 carrier board with
enable control signals from I2C GPIO expanders.
This patch adds these camera supplies to Jetson TX1 device tree to
allow using these when a camera module is used.
Signed-off-by: Sowjanya Komatineni <[email protected]>
---
arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi | 41 ++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi b/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
index b57d837..5e24d7a 100644
--- a/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
@@ -1323,6 +1323,14 @@
#gpio-cells = <2>;
gpio-controller;
};
+
+ exp2: gpio@77 {
+ compatible = "ti,tca9539";
+ reg = <0x77>;
+
+ #gpio-cells = <2>;
+ gpio-controller;
+ };
};
/* HDMI DDC */
@@ -1667,6 +1675,39 @@
enable-active-high;
vin-supply = <&vdd_5v0_sys>;
};
+
+ vdd_cam_1v2: regulator@12 {
+ compatible = "regulator-fixed";
+ reg = <12>;
+ regulator-name = "vdd-cam-1v2";
+ regulator-min-microvolt = <1200000>;
+ regulator-max-microvolt = <1200000>;
+ gpio = <&exp2 10 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ vin-supply = <&vdd_3v3_sys>;
+ };
+
+ vdd_cam_2v8: regulator@13 {
+ compatible = "regulator-fixed";
+ reg = <13>;
+ regulator-name = "vdd-cam-2v8";
+ regulator-min-microvolt = <2800000>;
+ regulator-max-microvolt = <2800000>;
+ gpio = <&exp1 13 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ vin-supply = <&vdd_3v3_sys>;
+ };
+
+ vdd_cam_1v8: regulator@14 {
+ compatible = "regulator-fixed";
+ reg = <14>;
+ regulator-name = "vdd-cam-1v8";
+ regulator-min-microvolt = <1800000>;
+ regulator-max-microvolt = <1800000>;
+ gpio = <&exp2 9 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ vin-supply = <&vdd_3v3_sys>;
+ };
};
gpio-keys {
--
2.7.4
CSI MIPI pads need to be enabled and calibrated for capturing from
the external sensor or transmitter.
MIPI CAL unit calibrates MIPI pads pull-up, pull-down and termination
impedances. Calibration is done by co-work of MIPI BIAS pad and MIPI
CAL control unit.
Triggering calibration start can happen any time after MIPI pads are
enabled but calibration results will be latched and applied to MIPI
pads by MIPI CAL unit only when the link is in LP11 state and then
calibration status register gets updated.
This patch enables CSI MIPI pads and calibrates them during streaming.
Tegra CSI receiver is able to catch the very first clock transition.
So, CSI receiver is always enabled prior to sensor streaming and
trigger of calibration start is done during CSI subdev streaming and
status of calibration is verified after sensor stream on.
Signed-off-by: Sowjanya Komatineni <[email protected]>
---
drivers/staging/media/tegra-video/csi.c | 44 +++++++++++++++++++++++++++++++--
drivers/staging/media/tegra-video/csi.h | 2 ++
drivers/staging/media/tegra-video/vi.c | 18 ++++++++++++++
3 files changed, 62 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/media/tegra-video/csi.c b/drivers/staging/media/tegra-video/csi.c
index 14e9050..82e340a 100644
--- a/drivers/staging/media/tegra-video/csi.c
+++ b/drivers/staging/media/tegra-video/csi.c
@@ -249,15 +249,42 @@ static int tegra_csi_s_stream(struct v4l2_subdev *subdev, int enable)
return ret;
}
+ if (csi_chan->mipi) {
+ ret = tegra_mipi_enable(csi_chan->mipi);
+ if (ret < 0) {
+ dev_err(csi->dev,
+ "failed to enable MIPI pads: %d\n",
+ ret);
+ goto rpm_put;
+ }
+
+ /*
+ * CSI MIPI pads PULLUP, PULLDN and TERM impedances
+ * need to be calibrated after power on.
+ * So, trigger the calibration start here and results
+ * will be latched and applied to the pads when link is
+ * in LP11 state during start of sensor streaming.
+ */
+ tegra_mipi_calibrate(csi_chan->mipi);
+ }
+
ret = csi->ops->csi_start_streaming(csi_chan);
if (ret < 0)
- goto rpm_put;
+ goto disable_mipi;
return 0;
}
csi->ops->csi_stop_streaming(csi_chan);
+disable_mipi:
+ if (csi_chan->mipi) {
+ ret = tegra_mipi_disable(csi_chan->mipi);
+ if (ret < 0)
+ dev_err(csi->dev,
+ "failed to disable MIPI pads: %d\n", ret);
+ }
+
rpm_put:
pm_runtime_put(csi->dev);
return ret;
@@ -291,6 +318,7 @@ static int tegra_csi_channel_alloc(struct tegra_csi *csi,
unsigned int num_pads)
{
struct tegra_csi_channel *chan;
+ int ret = 0;
chan = kzalloc(sizeof(*chan), GFP_KERNEL);
if (!chan)
@@ -309,7 +337,16 @@ static int tegra_csi_channel_alloc(struct tegra_csi *csi,
chan->pads[0].flags = MEDIA_PAD_FL_SOURCE;
}
- return 0;
+ if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
+ return 0;
+
+ chan->mipi = of_tegra_mipi_request(csi->dev, node);
+ if (IS_ERR(chan->mipi)) {
+ ret = PTR_ERR(chan->mipi);
+ dev_err(csi->dev, "failed to get mipi device: %d\n", ret);
+ }
+
+ return ret;
}
static int tegra_csi_tpg_channels_alloc(struct tegra_csi *csi)
@@ -473,6 +510,9 @@ static void tegra_csi_channels_cleanup(struct tegra_csi *csi)
struct tegra_csi_channel *chan, *tmp;
list_for_each_entry_safe(chan, tmp, &csi->csi_chans, list) {
+ if (chan->mipi)
+ tegra_mipi_free(chan->mipi);
+
subdev = &chan->subdev;
if (subdev->dev) {
if (!IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
diff --git a/drivers/staging/media/tegra-video/csi.h b/drivers/staging/media/tegra-video/csi.h
index b7b754a..0e5a537 100644
--- a/drivers/staging/media/tegra-video/csi.h
+++ b/drivers/staging/media/tegra-video/csi.h
@@ -51,6 +51,7 @@ struct tegra_csi;
* @framerate: active framerate for TPG
* @h_blank: horizontal blanking for TPG active format
* @v_blank: vertical blanking for TPG active format
+ * @mipi: mipi device for corresponding csi channel pads
*/
struct tegra_csi_channel {
struct list_head list;
@@ -66,6 +67,7 @@ struct tegra_csi_channel {
unsigned int framerate;
unsigned int h_blank;
unsigned int v_blank;
+ struct tegra_mipi_device *mipi;
};
/**
diff --git a/drivers/staging/media/tegra-video/vi.c b/drivers/staging/media/tegra-video/vi.c
index 03def26..337a19f 100644
--- a/drivers/staging/media/tegra-video/vi.c
+++ b/drivers/staging/media/tegra-video/vi.c
@@ -187,6 +187,7 @@ tegra_channel_get_remote_subdev(struct tegra_vi_channel *chan, bool sensor)
int tegra_channel_set_stream(struct tegra_vi_channel *chan, bool on)
{
struct v4l2_subdev *subdev;
+ struct tegra_csi_channel *csi_chan;
int ret;
/* stream CSI */
@@ -198,11 +199,28 @@ int tegra_channel_set_stream(struct tegra_vi_channel *chan, bool on)
if (IS_ENABLED(CONFIG_VIDEO_TEGRA_TPG))
return 0;
+ if (on)
+ csi_chan = v4l2_get_subdevdata(subdev);
+
subdev = tegra_channel_get_remote_subdev(chan, on);
ret = v4l2_subdev_call(subdev, video, s_stream, on);
if (on && ret < 0 && ret != -ENOIOCTLCMD)
return ret;
+ /*
+ * CSI subdev stream on triggers start of MIPI pads calibration.
+ * Calibration results are latched and applied to the pads when
+ * link is in LP11 state which will hapen during sensor streaming.
+ * So, wait for calibration to complete here.
+ */
+ if (on && csi_chan->mipi) {
+ ret = tegra_mipi_wait(csi_chan->mipi);
+ if (ret < 0)
+ dev_err(csi_chan->csi->dev,
+ "MIPI calibration failed: %d\n", ret);
+ return ret;
+ }
+
return 0;
}
--
2.7.4
tegra_i2c_runtime_resume does not disable prior enabled clocks
properly.
This patch fixes it.
Signed-off-by: Sowjanya Komatineni <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 3be1018..dba38a5 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -668,7 +668,7 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)
ret = clk_enable(i2c_dev->slow_clk);
if (ret < 0) {
dev_err(dev, "failed to enable slow clock: %d\n", ret);
- return ret;
+ goto disable_fast_clk;
}
}
@@ -676,11 +676,18 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)
if (ret < 0) {
dev_err(i2c_dev->dev,
"Enabling div clk failed, err %d\n", ret);
- clk_disable(i2c_dev->fast_clk);
- return ret;
+ goto disable_slow_clk;
}
return 0;
+
+disable_slow_clk:
+ if (i2c_dev->slow_clk)
+ clk_disable(i2c_dev->slow_clk);
+disable_fast_clk:
+ if (!i2c_dev->hw->has_single_clk_source)
+ clk_disable(i2c_dev->fast_clk);
+ return ret;
}
static int __maybe_unused tegra_i2c_runtime_suspend(struct device *dev)
--
2.7.4
VI I2C is on host1x bus and is part of VE power domain.
During suspend/resume VE power domain goes through power off/on.
So, controller reset followed by i2c re-initialization is required
after the domain power up.
This patch fixes it.
Signed-off-by: Sowjanya Komatineni <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index dba38a5..650240d 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -293,6 +293,8 @@ struct tegra_i2c_dev {
bool is_curr_atomic_xfer;
};
+static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev, bool clk_reinit);
+
static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
unsigned long reg)
{
@@ -679,8 +681,22 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)
goto disable_slow_clk;
}
+ /*
+ * VI I2C device is attached to VE power domain which goes through
+ * power ON/OFF during PM runtime resume/suspend. So, controller
+ * should go through reset and need to re-initialize after power
+ * domain ON.
+ */
+ if (i2c_dev->is_vi) {
+ ret = tegra_i2c_init(i2c_dev, true);
+ if (ret)
+ goto disable_div_clk;
+ }
+
return 0;
+disable_div_clk:
+ clk_disable(i2c_dev->div_clk);
disable_slow_clk:
if (i2c_dev->slow_clk)
clk_disable(i2c_dev->slow_clk);
--
2.7.4
Tegra VI I2C is part of VE power domain and typically used for
camera usecases.
VE power domain is not always on and is non-IRQ safe. So, IRQ safe
device cannot be attached to a non-IRQ safe domain as it prevents
powering off the PM domain and generic power domain driver will warn.
Current driver marks all I2C devices as IRQ safe and VI I2C device
does not require IRQ safe as it will not be used for atomic transfers.
This patch has fix to make VI I2C as non-IRQ safe.
Signed-off-by: Sowjanya Komatineni <[email protected]>
---
drivers/i2c/busses/i2c-tegra.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 1577296..3be1018 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1750,7 +1750,15 @@ static int tegra_i2c_probe(struct platform_device *pdev)
goto unprepare_slow_clk;
}
- pm_runtime_irq_safe(&pdev->dev);
+ /*
+ * VI I2C is in VE power domain which is not always on and not
+ * an IRQ safe. So, IRQ safe device can't be attached to a non-IRQ
+ * safe domain as it prevents powering off the PM domain.
+ * Also, VI I2C device don't need to use runtime IRQ safe as it will
+ * not be used for atomic transfers.
+ */
+ if (!i2c_dev->is_vi)
+ pm_runtime_irq_safe(&pdev->dev);
pm_runtime_enable(&pdev->dev);
if (!pm_runtime_enabled(&pdev->dev)) {
ret = tegra_i2c_runtime_resume(&pdev->dev);
--
2.7.4
10.06.2020 09:02, Sowjanya Komatineni пишет:
> VI I2C is on host1x bus and is part of VE power domain.
>
> During suspend/resume VE power domain goes through power off/on.
>
> So, controller reset followed by i2c re-initialization is required
> after the domain power up.
>
> This patch fixes it.
>
> Signed-off-by: Sowjanya Komatineni <[email protected]>
> ---
> drivers/i2c/busses/i2c-tegra.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index dba38a5..650240d 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -293,6 +293,8 @@ struct tegra_i2c_dev {
> bool is_curr_atomic_xfer;
> };
>
> +static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev, bool clk_reinit);
> +
> static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
> unsigned long reg)
> {
> @@ -679,8 +681,22 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)
> goto disable_slow_clk;
> }
>
> + /*
> + * VI I2C device is attached to VE power domain which goes through
> + * power ON/OFF during PM runtime resume/suspend. So, controller
> + * should go through reset and need to re-initialize after power
> + * domain ON.
> + */
> + if (i2c_dev->is_vi) {
> + ret = tegra_i2c_init(i2c_dev, true);
> + if (ret)
> + goto disable_div_clk;
> + }
> +
> return 0;
>
> +disable_div_clk:
> + clk_disable(i2c_dev->div_clk);
> disable_slow_clk:
> if (i2c_dev->slow_clk)
> clk_disable(i2c_dev->slow_clk);
>
The clk_disable() can cope with a NULL argument. Won't it be cleaner to
remove the conditions?
On 6/10/20 6:14 AM, Dmitry Osipenko wrote:
> 10.06.2020 09:02, Sowjanya Komatineni пишет:
>> VI I2C is on host1x bus and is part of VE power domain.
>>
>> During suspend/resume VE power domain goes through power off/on.
>>
>> So, controller reset followed by i2c re-initialization is required
>> after the domain power up.
>>
>> This patch fixes it.
>>
>> Signed-off-by: Sowjanya Komatineni <[email protected]>
>> ---
>> drivers/i2c/busses/i2c-tegra.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>> index dba38a5..650240d 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -293,6 +293,8 @@ struct tegra_i2c_dev {
>> bool is_curr_atomic_xfer;
>> };
>>
>> +static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev, bool clk_reinit);
>> +
>> static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
>> unsigned long reg)
>> {
>> @@ -679,8 +681,22 @@ static int __maybe_unused tegra_i2c_runtime_resume(struct device *dev)
>> goto disable_slow_clk;
>> }
>>
>> + /*
>> + * VI I2C device is attached to VE power domain which goes through
>> + * power ON/OFF during PM runtime resume/suspend. So, controller
>> + * should go through reset and need to re-initialize after power
>> + * domain ON.
>> + */
>> + if (i2c_dev->is_vi) {
>> + ret = tegra_i2c_init(i2c_dev, true);
>> + if (ret)
>> + goto disable_div_clk;
>> + }
>> +
>> return 0;
>>
>> +disable_div_clk:
>> + clk_disable(i2c_dev->div_clk);
>> disable_slow_clk:
>> if (i2c_dev->slow_clk)
>> clk_disable(i2c_dev->slow_clk);
>>
> The clk_disable() can cope with a NULL argument. Won't it be cleaner to
> remove the conditions?
will remove in v2