* Re: [PATCH v4 0/4] fix the clock setting for SAR ADC
From: Jonathan Cameron @ 2017-11-11 0:37 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: Mark Rutland, devicetree, linux-arm-kernel, Neil Armstrong,
linux-iio, Stephen Boyd, Michael Turquette, Yixun Lan,
linux-kernel, Rob Herring, Xingyu Chen, Kevin Hilman,
Carlo Caione, linux-amlogic, linux-clk, Jonathan Cameron,
Jerome Brunet
In-Reply-To: <CAFBinCB=2pb9dZkpSqdVAiumkSCzgaW0oejftjZNP7aYbiQJuw@mail.gmail.com>
On Tue, 7 Nov 2017 22:36:00 +0100
Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote:
> Hi Yixun,
>
> On Tue, Nov 7, 2017 at 3:09 PM, Yixun Lan <yixun.lan@amlogic.com> wrote:
> > patch [1/4]:
> > Fix wrong SARADC/SANA clock gate bit in Meson-GXBB/GXL,
> > the published datasheets[4] also has wrong description about this.
> > This patch should be explicitly merged *before* other patches.
> >
> > patch [2-4/4]:
> > Drop the "sana" clock from SAR ADC module,
> I agree with Jerome that patch 2/4 should be applied last.
Let me know when I should take this.
Thanks,
Jonathan
> when I wrote the driver I couldn't get it to work on my GXBB board
> (which unfortunately has died since then) because the clocks were
> disabled (they weren't enabled by the bootloader). people who are
> using an old .dtb would get the same problem again until the clock
> driver is merged
>
> > From the hardware perspective, the SAR ADC module doesn't
> > require "sana" clock to wrok. This should apply to all SoC,
> > including meson6,8, GXBB, GXL..
> thank you for clarifying this!
>
> > Note: the whole patchset series has been tested at GXL-P212 board,
> > we haven't got any meson6,8 board to test, so I would appreciate
> > if someone (Martin?) could help to confirm it works there.
> I can test this on a Meson8b and a Meson8m2 board on the weekend -
> I'll let you know about the results
>
>
> Regards
> Martin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v7 01/25] rcar-vin: add Gen3 devicetree bindings documentation
From: Niklas Söderlund @ 2017-11-11 0:38 UTC (permalink / raw)
To: Laurent Pinchart, Hans Verkuil, linux-media
Cc: linux-renesas-soc, tomoharu.fukawa.eb, Kieran Bingham,
Niklas Söderlund, Rob Herring, devicetree
In-Reply-To: <20171111003835.4909-1-niklas.soderlund+renesas@ragnatech.se>
Document the devicetree bindings for the CSI-2 inputs available on Gen3.
There is a need to add a custom property 'renesas,id' and to define
which CSI-2 input is described in which endpoint under the port@1 node.
This information is needed since there are a set of predefined routes
between each VIN and CSI-2 block. This routing table will be kept
inside the driver but in order for it to act on it it must know which
VIN and CSI-2 is which.
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
.../devicetree/bindings/media/rcar_vin.txt | 116 ++++++++++++++++++---
1 file changed, 104 insertions(+), 12 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/rcar_vin.txt b/Documentation/devicetree/bindings/media/rcar_vin.txt
index 6e4ef8caf759e5d3..df1abd0fb20386f8 100644
--- a/Documentation/devicetree/bindings/media/rcar_vin.txt
+++ b/Documentation/devicetree/bindings/media/rcar_vin.txt
@@ -2,8 +2,12 @@ Renesas R-Car Video Input driver (rcar_vin)
-------------------------------------------
The rcar_vin device provides video input capabilities for the Renesas R-Car
-family of devices. The current blocks are always slaves and suppot one input
-channel which can be either RGB, YUYV or BT656.
+family of devices.
+
+Each VIN instance has a single parallel input that supports RGB and YUV video,
+with both external synchronization and BT.656 synchronization for the latter.
+Depending on the instance the VIN input is connected to external SoC pins, or
+on Gen3 to a CSI-2 receiver.
- compatible: Must be one or more of the following
- "renesas,vin-r8a7795" for the R8A7795 device
@@ -28,21 +32,38 @@ channel which can be either RGB, YUYV or BT656.
Additionally, an alias named vinX will need to be created to specify
which video input device this is.
-The per-board settings:
+The per-board settings Gen2:
- port sub-node describing a single endpoint connected to the vin
as described in video-interfaces.txt[1]. Only the first one will
be considered as each vin interface has one input port.
- These settings are used to work out video input format and widths
- into the system.
+The per-board settings Gen3:
+
+Gen3 can support both a single connected parallel input source from
+external SoC pins (port0) and/or multiple parallel input sources from
+local SoC CSI-2 receivers (port1) depending on SoC.
+- renesas,id - ID number of the VIN, VINx in the documentation.
+- ports
+ - port0 - sub-node describing a single endpoint connected to the VIN
+ from external SoC pins described in video-interfaces.txt[1]. Only
+ the first one will be considered as each VIN interface has at most
+ one set of SoC external input pins.
+ - port1 - sub-nodes describing one or more endpoints connected to
+ the VIN from local SoC CSI-2 receivers. The endpoint numbers must
+ use the following schema.
-Device node example
--------------------
+ - Endpoint 0 - sub-node describing the endpoint which is CSI20
+ - Endpoint 1 - sub-node describing the endpoint which is CSI21
+ - Endpoint 2 - sub-node describing the endpoint which is CSI40
+ - Endpoint 3 - sub-node describing the endpoint which is CSI41
- aliases {
- vin0 = &vin0;
- };
+Device node example Gen2
+------------------------
+
+ aliases {
+ vin0 = &vin0;
+ };
vin0: vin@0xe6ef0000 {
compatible = "renesas,vin-r8a7790", "renesas,rcar-gen2-vin";
@@ -52,8 +73,8 @@ Device node example
status = "disabled";
};
-Board setup example (vin1 composite video input)
-------------------------------------------------
+Board setup example Gen2 (vin1 composite video input)
+-----------------------------------------------------
&i2c2 {
status = "ok";
@@ -92,6 +113,77 @@ Board setup example (vin1 composite video input)
};
};
+Device node example Gen3
+------------------------
+
+ vin0: video@e6ef0000 {
+ compatible = "renesas,vin-r8a7795";
+ reg = <0 0xe6ef0000 0 0x1000>;
+ interrupts = <GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD 811>;
+ power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+ resets = <&cpg 811>;
+ renesas,id = <0>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ reg = <1>;
+
+ vin0csi20: endpoint@0 {
+ reg = <0>;
+ remote-endpoint= <&csi20vin0>;
+ };
+ vin0csi21: endpoint@1 {
+ reg = <1>;
+ remote-endpoint= <&csi21vin0>;
+ };
+ vin0csi40: endpoint@2 {
+ reg = <2>;
+ remote-endpoint= <&csi40vin0>;
+ };
+ };
+ };
+ };
+
+ csi20: csi2@fea80000 {
+ compatible = "renesas,r8a7795-csi2", "renesas,rcar-gen3-csi2";
+ reg = <0 0xfea80000 0 0x10000>;
+ interrupts = <GIC_SPI 184 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cpg CPG_MOD 714>;
+ power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+ resets = <&cpg 714>;
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+ csi20_in: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1>;
+ remote-endpoint = <&adv7482_txb>;
+ };
+ };
+
+ port@1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ reg = <1>;
+
+ csi20vin0: endpoint@0 {
+ reg = <0>;
+ remote-endpoint = <&vin0csi20>;
+ };
+ };
+ };
+ };
[1] video-interfaces.txt common video media interface
--
2.15.0
^ permalink raw reply related
* Re: [PATCH 1/3] iio: light: add support for Intersil isl76683 sensor
From: Jonathan Cameron @ 2017-11-11 0:50 UTC (permalink / raw)
To: Christoph Fritz
Cc: Jonathan Cameron, Peter Meerwald-Stadler, Rob Herring,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1510068983-25769-2-git-send-email-chf.fritz-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
On Tue, 7 Nov 2017 16:36:21 +0100
Christoph Fritz <chf.fritz-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> This patch adds support for Intersil isl76683 light sensor.
>
> Signed-off-by: Christoph Fritz <chf.fritz-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
Sorry, bit of a rushed review as boarding about to start for a transatlantic flight...
Anyhow, a few comments inline in addition to Peter's. I'll take a proper look at V2.
Jonathan
> ---
> drivers/iio/light/Kconfig | 12 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/isl76683.c | 693 +++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 706 insertions(+)
> create mode 100644 drivers/iio/light/isl76683.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 2356ed9..4f0882c 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -194,6 +194,18 @@ config ISL29125
> To compile this driver as a module, choose M here: the module will be
> called isl29125.
>
> +config ISL76683
> + tristate "Intersil ISL76683 light sensor"
> + depends on I2C
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + Say Y here if you want to build a driver for the Intersil ISL76683
> + light sensor for I2C.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called isl76683.
> +
> config HID_SENSOR_ALS
> depends on HID_SENSOR_HUB
> select IIO_BUFFER
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index fa32fa4..886a51f 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -22,6 +22,7 @@ obj-$(CONFIG_HID_SENSOR_PROX) += hid-sensor-prox.o
> obj-$(CONFIG_SENSORS_ISL29018) += isl29018.o
> obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o
> obj-$(CONFIG_ISL29125) += isl29125.o
> +obj-$(CONFIG_ISL76683) += isl76683.o
> obj-$(CONFIG_JSA1212) += jsa1212.o
> obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
> obj-$(CONFIG_LTR501) += ltr501.o
> diff --git a/drivers/iio/light/isl76683.c b/drivers/iio/light/isl76683.c
> new file mode 100644
> index 0000000..b730276
> --- /dev/null
> +++ b/drivers/iio/light/isl76683.c
> @@ -0,0 +1,693 @@
> +/*
> + * IIO driver for the light sensor ISL76683.
> + *
> + * Copyright (c) 2017 Christoph Fritz <chf.fritz-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> + *
> + * This program 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 program 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.
> + *
> + * Datasheet:
> + * http://www.intersil.com/content/dam/Intersil/documents/isl7/isl76683.pdf
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/util_macros.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +#define ISL76683_REG_CMD 0x00
> +#define ISL76683_REG_CTRL 0x01
> +#define ISL76683_REG_THR_HI 0x02
> +#define ISL76683_REG_THR_LO 0x03
> +#define ISL76683_REG_SENSOR_L 0x04
> +#define ISL76683_REG_SENSOR_M 0x05
> +#define ISL76683_REG_CLEAR_INT 0x40
> +#define ISL76683_REGMAP_MAX 0x40
> +
> +#define ISL76683_CMD_ENABLE BIT(7)
> +#define ISL76683_CMD_PWRDWN BIT(6)
> +#define ISL76683_WIDTH_MASK 0x3
> +#define ISL76683_PHOTOD_SHFT 2
> +#define ISL76683_PHOTOD_MASK (0x3 << ISL76683_PHOTOD_SHFT)
> +#define ISL76683_INTPERS_MASK 0x3
> +#define ISL76683_LUXRANGE_SHFT 2
> +#define ISL76683_LUXRANGE_MASK (0x3 << ISL76683_LUXRANGE_SHFT)
> +#define ISL76683_LUXRANGE_STR "1000 4000 16000 64000"
> +
> +enum isl76683_dmode {
> + ISL76683_DIODE_0 = 0,
> + ISL76683_DIODE_IR,
> + ISL76683_DIODE_DIFF,
> +};
> +
> +enum isl76683_lux_range {
> + ISL76683_LUX_1000 = 0,
> + ISL76683_LUX_4000,
> + ISL76683_LUX_16000,
> + ISL76683_LUX_64000,
> +};
> +
> +static const int isl76683_lux_ranges_available[] = {
> + 1000, 4000, 16000, 64000};
> +
> +#define ISL76683_LUX_RANGE_DEFAULT ISL76683_LUX_1000
> +#define ISL76683_DIODE_MAX ISL76683_DIODE_DIFF
> +#define ISL76683_DIODE_DEFAULT ISL76683_DIODE_0
> +#define ISL76683_WIDTH_DEFAULT 0x0
> +#define ISL76683_RESOLUTION_DEFAULT 16
> +#define ISL76683_EXT_RESISTOR_DEFAULT 100
> +#define ISL76683_KOHM_MIN 1
> +#define ISL76683_KOHM_MAX 1000
> +#define ISL76683_INTPERS_DEFAULT 0x0
> +#define ISL76683_THR_DEFAULT 0x7f
> +
> +struct isl76683_chip {
> + enum isl76683_lux_range luxrange;
> + int external_resistor;
> + enum isl76683_dmode photodiode;
> + struct i2c_client *client;
> + struct regmap *rmp;
> + struct completion irq_complete;
> + struct iio_trigger *trig;
> + bool trig_enabled;
> + struct mutex lock;
> + __le16 *buffer;
> + s64 time_ns;
> + bool buffer_running;
> +};
> +
> +static bool isl76683_readable_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case ISL76683_REG_CMD:
> + case ISL76683_REG_CTRL:
> + case ISL76683_REG_THR_HI:
> + case ISL76683_REG_THR_LO:
> + case ISL76683_REG_SENSOR_L:
> + case ISL76683_REG_SENSOR_M:
> + case ISL76683_REG_CLEAR_INT:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static bool isl76683_writeable_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case ISL76683_REG_CMD:
> + case ISL76683_REG_CTRL:
> + case ISL76683_REG_THR_HI:
> + case ISL76683_REG_THR_LO:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static bool isl76683_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case ISL76683_REG_SENSOR_L:
> + case ISL76683_REG_SENSOR_M:
> + case ISL76683_REG_CLEAR_INT:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static const struct regmap_config isl76683_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = ISL76683_REGMAP_MAX,
> + .readable_reg = isl76683_readable_reg,
> + .writeable_reg = isl76683_writeable_reg,
> + .volatile_reg = isl76683_is_volatile_reg,
> + .cache_type = REGCACHE_RBTREE,
> +};
> +
> +static int isl76683_set_config(struct isl76683_chip *chip)
> +{
> + int ret;
> +
> + ret = regmap_update_bits(chip->rmp, ISL76683_REG_CTRL,
> + ISL76683_LUXRANGE_MASK | ISL76683_INTPERS_MASK,
> + (chip->luxrange << ISL76683_LUXRANGE_SHFT) |
> + ISL76683_INTPERS_DEFAULT);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_update_bits(chip->rmp, ISL76683_REG_CMD,
> + ISL76683_PHOTOD_MASK | ISL76683_WIDTH_MASK,
> + (chip->photodiode << ISL76683_PHOTOD_SHFT) |
> + ISL76683_WIDTH_DEFAULT);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_write(chip->rmp, ISL76683_REG_THR_HI,
> + ISL76683_THR_DEFAULT);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_write(chip->rmp, ISL76683_REG_THR_LO,
> + ISL76683_THR_DEFAULT);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int isl76683_power(struct isl76683_chip *chip, bool on)
> +{
> + int ret;
> +
> + ret = regmap_update_bits(chip->rmp, ISL76683_REG_CMD,
> + ISL76683_CMD_ENABLE | ISL76683_CMD_PWRDWN,
> + 0x0);
> + if (ret < 0)
> + return ret;
> +
> + ret = regmap_update_bits(chip->rmp, ISL76683_REG_CMD,
> + ISL76683_CMD_ENABLE | ISL76683_CMD_PWRDWN,
> + on ? ISL76683_CMD_ENABLE : ISL76683_CMD_PWRDWN);
> + if (ret < 0)
> + return ret;
> +
> + return on ? isl76683_set_config(chip) : 0;
> +}
> +
> +static int isl76683_reset(struct isl76683_chip *chip)
> +{
> + int ret;
> +
> + ret = isl76683_power(chip, false);
> + if (ret < 0)
> + return ret;
> +
> + return isl76683_power(chip, true);
> +}
> +
> +static int isl76683_read_lux(struct isl76683_chip *chip,
> + bool is_processed, int *val)
> +{
> + unsigned int sensor_data, range, fsr;
> + __le16 sensor_raw;
> + int ret;
> +
> + ret = regmap_bulk_read(chip->rmp, ISL76683_REG_SENSOR_L,
> + &sensor_raw, sizeof(sensor_raw));
> + if (ret)
> + return ret;
> +
> + sensor_data = le16_to_cpu(sensor_raw);
> +
> + if (!is_processed) {
> + *val = sensor_data;
> + return 0;
> + }
> +
> + /* range values taken from datasheet (table 9) */
> + if (chip->luxrange == ISL76683_LUX_1000)
> + range = 973;
> + else if (chip->luxrange == ISL76683_LUX_4000)
> + range = 3892;
> + else if (chip->luxrange == ISL76683_LUX_16000)
> + range = 15568;
> + else if (chip->luxrange == ISL76683_LUX_64000)
> + range = 62272;
> + else
> + return -EINVAL;
> +
> + /* equations from datasheet (EQ.3 and EQ.4) */
> + fsr = (100 * range) / chip->external_resistor;
> + *val = (fsr * sensor_data) / (1 << ISL76683_RESOLUTION_DEFAULT);
> +
> + return 0;
> +}
> +
> +static irqreturn_t isl76683_interrupt_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct isl76683_chip *chip = iio_priv(indio_dev);
> +
> + chip->time_ns = iio_get_time_ns(indio_dev);
> +
> + if (chip->trig_enabled)
> + iio_trigger_poll(chip->trig);
> +
> + if (!completion_done(&chip->irq_complete))
> + complete(&chip->irq_complete);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static inline int isl76683_start_measurement(struct isl76683_chip *chip)
> +{
> + int dummy;
> +
> + /* dummy read is clearing irq and triggers new measurement */
> + return regmap_read(chip->rmp, ISL76683_REG_CLEAR_INT, &dummy);
> +}
> +
> +static int isl76683_singleshot_conversion(struct isl76683_chip *chip,
> + const struct iio_chan_spec *chan,
> + bool is_processed, int *val)
> +{
> + long timeout;
> + int ret;
> +
> + if (chip->buffer_running)
Don't reinvent the wheel. (this is racy btw).
iio_claim_direct_mode etc will do this in a non racy fashion for you.
> + return -EAGAIN;
> +
> + switch (chan->channel2) {
> + case IIO_MOD_LIGHT_BOTH:
> + chip->photodiode = ISL76683_DIODE_DIFF;
> + break;
> + case IIO_MOD_LIGHT_IR:
> + chip->photodiode = ISL76683_DIODE_IR;
> + break;
> + default:
> + chip->photodiode = ISL76683_DIODE_0;
> + }
> +
> + ret = isl76683_set_config(chip);
> + if (ret)
> + return ret;
> +
> + ret = isl76683_start_measurement(chip);
> + if (ret < 0)
> + return ret;
> +
> + /* wait for measurement to complete */
> + timeout = wait_for_completion_interruptible_timeout(
> + &chip->irq_complete,
> + msecs_to_jiffies(5000));
> + if (timeout == 0) {
> + dev_err(&chip->client->dev, "measurement timed out\n");
> + return -ETIMEDOUT;
> + } else if (timeout < 0) {
> + dev_err(&chip->client->dev, "wait for measurement failed\n");
> + return -EINTR;
> + }
> +
> + ret = isl76683_read_lux(chip, is_processed, val);
> + if (ret) {
> + dev_err(&chip->client->dev, "%s: Error %d reading lux\n",
> + __func__, ret);
> + return ret;
> + }
> +
> + return IIO_VAL_INT;
> +}
> +
> +static irqreturn_t isl76683_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct isl76683_chip *chip = iio_priv(indio_dev);
> + __le16 buf;
> + int ret;
> +
> + ret = regmap_bulk_read(chip->rmp, ISL76683_REG_SENSOR_L, &buf, 2);
> + if (ret)
> + return ret;
> +
> + chip->buffer[0] = le16_to_cpu(buf);
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, chip->buffer,
> + chip->time_ns);
> +
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + ret = isl76683_start_measurement(chip);
> + if (ret < 0)
> + return ret;
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int isl76683_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct isl76683_chip *chip = iio_priv(indio_dev);
> +
> + mutex_lock(&chip->lock);
> + chip->buffer_running = true;
> + mutex_unlock(&chip->lock);
> + chip->photodiode = indio_dev->channels[0].channel2;
> + return isl76683_set_config(chip);
> +}
> +
> +static int isl76683_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct isl76683_chip *chip = iio_priv(indio_dev);
> +
> + chip->buffer_running = false;
> + return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops isl76683_buffer_setup_ops = {
> + .preenable = &isl76683_buffer_preenable,
> + .postdisable = &isl76683_buffer_postdisable,
> + .predisable = iio_triggered_buffer_predisable,
> + .postenable = iio_triggered_buffer_postenable,
> + .validate_scan_mask = &iio_validate_scan_mask_onehot,
> +};
> +
> +static int isl76683_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
> +{
> + struct isl76683_chip *chip = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + mutex_lock(&chip->lock);
> + ret = isl76683_singleshot_conversion(chip, chan, false, val);
> + mutex_unlock(&chip->lock);
> + return ret;
> + case IIO_CHAN_INFO_PROCESSED:
> + mutex_lock(&chip->lock);
> + ret = isl76683_singleshot_conversion(chip, chan, true, val);
> + mutex_unlock(&chip->lock);
> + return ret;
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + *val = isl76683_lux_ranges_available[chip->luxrange];
> + return IIO_VAL_INT;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int isl76683_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct isl76683_chip *chip = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_HARDWAREGAIN:
> + mutex_lock(&chip->lock);
> + chip->luxrange = find_closest(val,
> + isl76683_lux_ranges_available,
> + ARRAY_SIZE(isl76683_lux_ranges_available));
> + ret = isl76683_set_config(chip);
> + mutex_unlock(&chip->lock);
> + return ret;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static IIO_CONST_ATTR(in_illuminance_hardwaregain_available,
> + ISL76683_LUXRANGE_STR);
> +
> +static struct attribute *isl76683_attributes[] = {
> + &iio_const_attr_in_illuminance_hardwaregain_available.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group isl76683_attribute_group = {
> + .attrs = isl76683_attributes,
> +};
> +
> +#define ISL76683_CHANNEL_2(_ch2, _si) { \
> + .type = IIO_LIGHT, \
> + .modified = 1, \
> + .channel2 = _ch2, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_PROCESSED), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_HARDWAREGAIN), \
> + .scan_index = _si, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 16, \
> + .storagebits = 16, \
> + .endianness = IIO_CPU, \
> + }, \
> +}
> +
> +static const struct iio_chan_spec isl76683_channels[] = {
> + {
> + .type = IIO_LIGHT,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_PROCESSED),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_HARDWAREGAIN),
> + .scan_index = 0,
> + .scan_type = {
> + .sign = 'u',
> + .realbits = 16,
> + .storagebits = 16,
> + .endianness = IIO_CPU,
> + },
> + },
> + ISL76683_CHANNEL_2(IIO_MOD_LIGHT_IR, 1),
> + ISL76683_CHANNEL_2(IIO_MOD_LIGHT_BOTH, 2),
> + IIO_CHAN_SOFT_TIMESTAMP(3),
> +};
> +
> +static int isl76683_update_scan_mode(struct iio_dev *indio_dev,
> + const unsigned long *scan_mask)
> +{
> + struct isl76683_chip *chip = iio_priv(indio_dev);
> +
> + kfree(chip->buffer);
> + chip->buffer = kzalloc(indio_dev->scan_bytes, GFP_KERNEL);
> + if (chip->buffer == NULL)
> + return -ENOMEM;
I would be lazy and just make the buffer big enough for all cases.
You will want to play the __cachline_aligned game to make it
dma safe at the end of the chip structure.
> +
> + return 0;
> +}
> +
> +static const struct iio_info isl76683_info = {
> + .update_scan_mode = isl76683_update_scan_mode,
> + .read_raw = isl76683_read_raw,
> + .write_raw = isl76683_write_raw,
> + .attrs = &isl76683_attribute_group,
> + .driver_module = THIS_MODULE,
This will not work with latest IIO tree (or linux-next)
Just drop setting this field as it no longer exists.
> +};
> +
> +static int isl76683_set_trigger_state(struct iio_trigger *trig, bool enable)
> +{
> + struct isl76683_chip *chip = iio_trigger_get_drvdata(trig);
> + int ret;
> +
> + if (enable) {
> + chip->trig_enabled = true;
This is unusual enough that I'd like a comment here on how you would end
up in a state where this is necessary.
> + ret = isl76683_start_measurement(chip);
> + if (ret < 0)
> + return ret;
> + } else
> + chip->trig_enabled = false;
> +
> + return 0;
> +}
> +
> +static const struct iio_trigger_ops isl76683_trigger_ops = {
> + .owner = THIS_MODULE,
> + .set_trigger_state = isl76683_set_trigger_state,
> + .validate_device = iio_trigger_validate_own_device,
> +};
> +
> +static int isl76683_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct isl76683_chip *chip;
> + struct iio_dev *indio_dev;
> + struct device_node *np = client->dev.of_node;
> + int rs = ISL76683_EXT_RESISTOR_DEFAULT;
> + int v, ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + chip = iio_priv(indio_dev);
> +
> + i2c_set_clientdata(client, indio_dev);
> + chip->client = client;
> +
> + if (np) {
> + ret = of_property_read_u32(np, "isil,external-resistor", &v);
> + if (ret || v < ISL76683_KOHM_MIN || v > ISL76683_KOHM_MAX)
> + dev_warn(&client->dev,
> + "assuming %i kOhm resistor\n", rs);
> + else
> + rs = v;
> + }
> +
> + chip->luxrange = ISL76683_LUX_RANGE_DEFAULT;
> + chip->external_resistor = rs;
> + chip->photodiode = ISL76683_DIODE_DEFAULT;
> + chip->buffer_running = false;
> +
> + chip->rmp = devm_regmap_init_i2c(client, &isl76683_regmap_config);
> + if (IS_ERR(chip->rmp)) {
> + ret = PTR_ERR(chip->rmp);
> + dev_err(&client->dev, "%s: Error %d initializing regmap\n",
> + __func__, ret);
> + return ret;
> + }
> +
> + mutex_init(&chip->lock);
> +
> + init_completion(&chip->irq_complete);
> +
> + if (!client->irq) {
> + dev_err(&client->dev, "no interrupt configured\n");
> + return -EINVAL;
> + }
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->info = &isl76683_info;
> + indio_dev->channels = isl76683_channels;
> + indio_dev->num_channels = ARRAY_SIZE(isl76683_channels);
> + indio_dev->name = id->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + chip->trig_enabled = false;
> + chip->trig = devm_iio_trigger_alloc(&client->dev, "%s-dev%d",
> + indio_dev->name, indio_dev->id);
> + if (!chip->trig)
> + return -ENOMEM;
> +
> + chip->trig->ops = &isl76683_trigger_ops;
> + chip->trig->dev.parent = &client->dev;
> + iio_trigger_set_drvdata(chip->trig, chip);
> +
> + ret = devm_request_irq(&client->dev, client->irq,
> + isl76683_interrupt_handler,
> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> + "isl76683_event", indio_dev);
> + if (ret) {
> + dev_err(&client->dev, "irq request error\n");
> + return ret;
> + }
> +
> + ret = devm_iio_trigger_register(&client->dev, chip->trig);
> + if (ret) {
> + dev_err(&client->dev, "iio_trigger register error\n");
> + return ret;
> + }
> +
> + ret = isl76683_reset(chip);
> + if (ret) {
> + dev_err(&client->dev, "reset failed\n");
> + return ret;
> + }
> +
> + ret = isl76683_read_lux(chip, false, &v);
> + if (ret) {
> + dev_err(&client->dev, "initial dummy readout failed\n");
> + return ret;
> + }
> +
> + ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev, NULL,
> + &isl76683_trigger_handler, &isl76683_buffer_setup_ops);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_device_register(&client->dev, indio_dev);
> + if (ret) {
> + dev_err(&client->dev,
> + "%s(): iio registration failed with error %d\n",
> + __func__, ret);
> + return ret;
Drop this return ret.
> + }
> +
> + return ret;
> +}
> +
> +static int isl76683_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> + struct isl76683_chip *chip = iio_priv(indio_dev);
> +
> + pm_runtime_disable(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
> + isl76683_power(chip, false);
> + kfree(chip->buffer);
> +
> + return 0;
> +}
> +
> +#if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM)
> +static int isl76683_runtime_suspend(struct device *dev)
> +{
> + struct isl76683_chip *chip =
> + iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
> + int ret;
> +
> + mutex_lock(&chip->lock);
> + ret = isl76683_power(chip, false);
> + regcache_mark_dirty(chip->rmp);
> + mutex_unlock(&chip->lock);
> +
> + return ret;
> +}
> +
> +static int isl76683_runtime_resume(struct device *dev)
> +{
> + struct isl76683_chip *chip =
> + iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
> + int ret;
> +
> + mutex_lock(&chip->lock);
> + ret = isl76683_power(chip, true);
> + mutex_unlock(&chip->lock);
> +
> + return ret;
> +}
> +#endif
> +
> +static const struct dev_pm_ops isl76683_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
> + SET_RUNTIME_PM_OPS(isl76683_runtime_suspend,
> + isl76683_runtime_resume, NULL)
> +};
> +
> +static const struct i2c_device_id isl76683_id[] = {
> + {"isl76683", 0},
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, isl76683_id);
> +
> +static const struct of_device_id isl76683_of_match[] = {
> + { .compatible = "isil,isl76683", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, isl76683_of_match);
> +
> +static struct i2c_driver isl76683_driver = {
> + .driver = {
> + .name = "isl76683",
> + .of_match_table = isl76683_of_match,
> + .pm = &isl76683_pm_ops,
> + },
> + .probe = isl76683_probe,
> + .remove = isl76683_remove,
> + .id_table = isl76683_id,
> +};
> +
> +module_i2c_driver(isl76683_driver);
> +
> +MODULE_DESCRIPTION("ISL76683 Ambient Light Sensor driver");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Christoph Fritz <chf.fritz-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>");
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] of: Document exactly what of_find_node_by_name() puts
From: Stephen Boyd @ 2017-11-11 1:45 UTC (permalink / raw)
To: Rob Herring, Frank Rowand
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
It isn't clear if this function of_node_put()s the 'from'
argument, or the node it finds in the search. Clearly indicate
which variable is touched.
Signed-off-by: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
drivers/of/base.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 63897531cd75..602c5d65734a 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -866,7 +866,7 @@ EXPORT_SYMBOL(of_find_node_opts_by_path);
* @from: The node to start searching from or NULL, the node
* you pass will not be searched, only the next one
* will; typically, you pass what the previous call
- * returned. of_node_put() will be called on it
+ * returned. of_node_put() will be called on @from.
* @name: The name string to match against
*
* Returns a node pointer with refcount incremented, use
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH] of: Document exactly what of_find_node_by_name() puts
From: Randy Dunlap @ 2017-11-11 2:12 UTC (permalink / raw)
To: Stephen Boyd, Rob Herring, Frank Rowand
Cc: linux-kernel, devicetree, linux-doc
In-Reply-To: <20171111014536.3592-1-sboyd@codeaurora.org>
On 11/10/2017 05:45 PM, Stephen Boyd wrote:
> It isn't clear if this function of_node_put()s the 'from'
> argument, or the node it finds in the search. Clearly indicate
> which variable is touched.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
> drivers/of/base.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 63897531cd75..602c5d65734a 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -866,7 +866,7 @@ EXPORT_SYMBOL(of_find_node_opts_by_path);> * @from: The node to start searching from or NULL, the node
* @from: The node to start searching from or %NULL; the node
> * you pass will not be searched, only the next one
> * will; typically, you pass what the previous call
* will. Typically, you pass what the previous call
> - * returned. of_node_put() will be called on it
> + * returned. of_node_put() will be called on @from.
> * @name: The name string to match against
> *
> * Returns a node pointer with refcount incremented, use
>
--
~Randy
^ permalink raw reply
* Re: [PATCH v10 2/2] media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver
From: Laurent Pinchart @ 2017-11-11 6:17 UTC (permalink / raw)
To: Sakari Ailus
Cc: Niklas Söderlund, Hans Verkuil,
linux-media-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
tomoharu.fukawa.eb-zM6kxYcvzFBBDgjK7y7TUQ, Kieran Bingham,
Sakari Ailus, Geert Uytterhoeven
In-Reply-To: <20171110223227.pug7d4qi7rdi4b4b-S+BSfZ9RZZmRSg0ZkenSGLdO1Tsj/99ntUK59QYPAWc@public.gmane.org>
Hi Sakari,
On Saturday, 11 November 2017 00:32:27 EET Sakari Ailus wrote:
> On Fri, Nov 10, 2017 at 02:31:37PM +0100, Niklas Söderlund wrote:
> > A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> > supports the rcar-vin driver on R-Car Gen3 SoCs where separate CSI-2
> > hardware blocks are connected between the video sources and the video
> > grabbers (VIN).
> >
> > Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >
> > drivers/media/platform/rcar-vin/Kconfig | 12 +
> > drivers/media/platform/rcar-vin/Makefile | 1 +
> > drivers/media/platform/rcar-vin/rcar-csi2.c | 934 +++++++++++++++++++++++
> > 3 files changed, 947 insertions(+)
> > create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
[snip]
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644
> > index 0000000000000000..27d09da191a09b39
> > --- /dev/null
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
[snip]
> > +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv,
> > + struct v4l2_subdev *source,
> > + struct v4l2_mbus_framefmt *mf,
> > + u32 *phypll)
> > +{
> > + const struct phypll_hsfreqrange *hsfreq;
> > + const struct rcar_csi2_format *format;
> > + struct v4l2_ctrl *ctrl;
> > + u64 mbps;
> > +
> > + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
>
> How about LINK_FREQ instead? It'd be more straightforward to calculate
> this. Up to you.
This probably needs to be documented, but I think the official API is
V4L2_CID_PIXEL_RATE. The link frequency control is not mandatory.
> > + if (!ctrl) {
> > + dev_err(priv->dev, "no link freq control in subdev %s\n",
> > + source->name);
> > + return -EINVAL;
> > + }
> > +
> > + format = rcar_csi2_code_to_fmt(mf->code);
> > + if (!format) {
> > + dev_err(priv->dev, "Unknown format: %d\n", mf->code);
> > + return -EINVAL;
> > + }
> > +
> > + mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * format->bpp;
> > + do_div(mbps, priv->lanes * 1000000);
> > +
> > + for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> > + if (hsfreq->mbps >= mbps)
> > + break;
> > +
> > + if (!hsfreq->mbps) {
> > + dev_err(priv->dev, "Unsupported PHY speed (%llu Mbps)", mbps);
> > + return -ERANGE;
> > + }
> > +
> > + dev_dbg(priv->dev, "PHY HSFREQRANGE requested %llu got %u Mbps\n",
mbps,
> > + hsfreq->mbps);
> > +
> > + *phypll = PHYPLL_HSFREQRANGE(hsfreq->reg);
> > +
> > + return 0;
> > +}
[snip]
> > +static int rcar_csi2_s_power(struct v4l2_subdev *sd, int on)
> > +{
> > + struct rcar_csi2 *priv = sd_to_csi2(sd);
> > +
> > + if (on)
> > + pm_runtime_get_sync(priv->dev);
> > + else
> > + pm_runtime_put(priv->dev);
>
> The pipeline you have is already rather complex. Would it be an option to
> power the hardware on when streaming is started? The smiapp driver does
> this, without even implementing the s_power callback. (You'd still need to
> call it on the image source, as long as we have drivers that need it.)
We've briefly discussed this before, and I agree that pipeline power
management needs to be redesigned, but we still haven't agreed on a proper
architecture for that. Feel free to propose an RFC :-) In the meantime I
wouldn't try to enforce one specific model.
> > +
> > + return 0;
> > +}
[snip]
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v10 2/2] media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver
From: Laurent Pinchart @ 2017-11-11 6:34 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Sakari Ailus, Hans Verkuil, linux-media, Rob Herring, devicetree,
linux-renesas-soc, tomoharu.fukawa.eb, Kieran Bingham,
Sakari Ailus, Geert Uytterhoeven
In-Reply-To: <20171111001113.GB13042@bigcity.dyn.berto.se>
Hi Niklas,
On Saturday, 11 November 2017 02:11:13 EET Niklas Söderlund wrote:
> On 2017-11-11 00:32:27 +0200, Sakari Ailus wrote:
> > On Fri, Nov 10, 2017 at 02:31:37PM +0100, Niklas Söderlund wrote:
> >> A V4L2 driver for Renesas R-Car MIPI CSI-2 receiver. The driver
> >> supports the rcar-vin driver on R-Car Gen3 SoCs where separate CSI-2
> >> hardware blocks are connected between the video sources and the video
> >> grabbers (VIN).
> >>
> >> Driver is based on a prototype by Koji Matsuoka in the Renesas BSP.
> >>
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >> ---
> >>
> >> drivers/media/platform/rcar-vin/Kconfig | 12 +
> >> drivers/media/platform/rcar-vin/Makefile | 1 +
> >> drivers/media/platform/rcar-vin/rcar-csi2.c | 934 ++++++++++++++++++++++
> >> 3 files changed, 947 insertions(+)
> >> create mode 100644 drivers/media/platform/rcar-vin/rcar-csi2.c
[snip]
> >> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c
> >> b/drivers/media/platform/rcar-vin/rcar-csi2.c new file mode 100644
> >> index 0000000000000000..27d09da191a09b39
> >> --- /dev/null
> >> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
[snip]
> >> +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv,
> >> + struct v4l2_subdev *source,
> >> + struct v4l2_mbus_framefmt *mf,
> >> + u32 *phypll)
> >> +{
> >> + const struct phypll_hsfreqrange *hsfreq;
> >> + const struct rcar_csi2_format *format;
> >> + struct v4l2_ctrl *ctrl;
> >> + u64 mbps;
> >> +
> >> + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> >
> > How about LINK_FREQ instead? It'd be more straightforward to calculate
> > this. Up to you.
>
> I need to use PIXEL_RATE as my test setup uses the adv748x driver which
> only implement that control. In the short term I would like to support
> both, but I need a setup to test that before I can add support for it.
> In the long term, maybe we should deprecate one of the controls?
The LINK_FREQ control is meant for the user to select one of the available
link frequencies (when multiple values are possible), while the PIXEL_RATE
control is a read-only control meant for the connected subdev to query the
resulting pixel rate. I think both controls should be kept, and PIXEL_RATE
should be used here.
> >> + if (!ctrl) {
> >> + dev_err(priv->dev, "no link freq control in subdev %s\n",
> >> + source->name);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + format = rcar_csi2_code_to_fmt(mf->code);
> >> + if (!format) {
> >> + dev_err(priv->dev, "Unknown format: %d\n", mf->code);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * format->bpp;
> >> + do_div(mbps, priv->lanes * 1000000);
> >> +
> >> + for (hsfreq = priv->info->hsfreqrange; hsfreq->mbps != 0; hsfreq++)
> >> + if (hsfreq->mbps >= mbps)
> >> + break;
> >> +
> >> + if (!hsfreq->mbps) {
> >> + dev_err(priv->dev, "Unsupported PHY speed (%llu Mbps)", mbps);
> >> + return -ERANGE;
> >> + }
> >> +
> >> + dev_dbg(priv->dev, "PHY HSFREQRANGE requested %llu got %u Mbps\n",
> >> mbps,
> >> + hsfreq->mbps);
> >> +
> >> + *phypll = PHYPLL_HSFREQRANGE(hsfreq->reg);
> >> +
> >> + return 0;
> >> +}
[snip]
--
Regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH 4/4] arm64: dts: qcom: msm8916: add bindings for i2c1, i2c3, i2c5
From: Bjorn Andersson @ 2017-11-11 7:56 UTC (permalink / raw)
To: Damien Riegel, linux-arm-msm, linux-soc, devicetree,
linux-arm-kernel, linux-kernel, Andy Gross, David Brown,
Rob Herring, Mark Rutland, Catalin Marinas, Will Deacon, kernel
In-Reply-To: <20171109171456.6t3nhig6t4bnkc77@workotop.localdomain>
On Thu 09 Nov 09:14 PST 2017, Damien Riegel wrote:
> Hi Bjorn,
>
>
> On Thu, Nov 09, 2017 at 09:00:16AM -0800, Bjorn Andersson wrote:
> > On Wed 01 Nov 10:53 PDT 2017, Damien Riegel wrote:
> >
> > I think it's better to use the word "nodes" (add nodes...)
>
> Will reword that.
>
> >
> > > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > > ---
> > > arch/arm64/boot/dts/qcom/msm8916-pins.dtsi | 72 ++++++++++++++++++++++++++++++
> > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 45 +++++++++++++++++++
> > > 2 files changed, 117 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi
> > > index c67ad8ed8b60..1cec5b30ed6e 100644
> > > --- a/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/msm8916-pins.dtsi
> > > @@ -270,6 +270,30 @@
> > > };
> > > };
> > >
> > > + i2c1_default: i2c1_default {
> > > + pinmux {
> > > + function = "blsp_i2c1";
> > > + pins = "gpio2", "gpio3";
> > > + };
> > > + pinconf {
> > > + pins = "gpio2", "gpio3";
> > > + drive-strength = <16>;
> > > + bias-disable;
> > > + };
> >
> > pinconf is typically board specific, so please leave these out from the
> > base dtsi.
>
> I don't mind dropping that, but pinconf for i2c{2,4,6} is already in
> msm8916-pins.dtsi, so we should either drop them all, or add pinconf for
> i2c{1,3,5} for consistency.
You're right, this needs to be consistent. So I would like us to update
the existing nodes,
> And if I read the pinctrl driver correctly,
> I2C cannot be routed to other pins, the only properties that users may
> want to modify are drive-strength and bias. Let me know which option you
> prefer.
>
I discussed the matter with my colleagues and we concluded that we want
to keep the muxing in the platform.dtsi and move the specification of
the electrical properties (pinconf) to the board.dts(i).
We did discuss having some "default values" in the platform file, but
pushing these down to the board file gives an indication to others that
these values are board specific.
PS. If you're willing to provide a patch for the existing nodes I would
be happy to review this as well!
Regards,
Bjorn
^ permalink raw reply
* Re: [PATCH net-next] bindings: net: stmmac: correctify note about LPI interrupt
From: David Miller @ 2017-11-11 10:29 UTC (permalink / raw)
To: niklas.cassel-VrBV9hrLPhE
Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
Joao.Pinto-HKixBCOQz3hWk0Htik3J/w, niklass-VrBV9hrLPhE,
alexandre.torgue-qxv4g6HH51o, peppe.cavallaro-qxv4g6HH51o,
treding-DDmLM1+adcrQT0dZR+AlfA, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171109170928.15496-1-niklas.cassel-VrBV9hrLPhE@public.gmane.org>
From: Niklas Cassel <niklas.cassel-VrBV9hrLPhE@public.gmane.org>
Date: Thu, 9 Nov 2017 18:09:26 +0100
> There are two different combined signal for various interrupt events:
> In EQOS-CORE and EQOS-MTL configurations, mci_intr_o is the interrupt
> signal.
> In EQOS-DMA, EQOS-AHB and EQOS-AXI configurations, these interrupt events
> are combined with the events in the DMA on the sbd_intr_o signal.
>
> Depending on configuration, the device tree irq "macirq" will refer to
> either mci_intr_o or sbd_intr_o.
>
> The databook states:
> "The MAC generates the LPI interrupt when the Tx or Rx side enters or exits
> the LPI state. The interrupt mci_intr_o (sbd_intr_o in certain
> configurations) is asserted when the LPI interrupt status is set.
>
> When the MAC exits the Rx LPI state, then in addition to the mci_intr_o
> (sbd_intr_o in certain configurations), the sideband signal lpi_intr_o is
> asserted.
>
> If you do not want to gate-off the application clock during the Rx LPI
> state, you can leave the lpi_intr_o signal unconnected and use the
> mci_intr_o (sbd_intr_o in certain configurations) signal to detect Rx LPI
> exit."
>
> Since the "macirq" is always raised when Tx or Rx enters/exits the LPI
> state, "eth_lpi" must therefore refer to lpi_intr_o, which is only raised
> when Rx exits the LPI state. Update the DT binding description to reflect
> reality.
>
> Signed-off-by: Niklas Cassel <niklas.cassel-VrBV9hrLPhE@public.gmane.org>
Applied.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 3/5] staging: Introduce NVIDIA Tegra video decoder driver
From: Vladimir Zapolskiy @ 2017-11-11 14:06 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, Jonathan Hunter,
Greg Kroah-Hartman, Rob Herring, Mauro Carvalho Chehab,
Stephen Warren, Vladimir Zapolskiy
Cc: Dan Carpenter, linux-media, linux-tegra, devel, devicetree,
linux-kernel
In-Reply-To: <1a3798f337c0097e67d70226ae3ba665fd9156c2.1508448293.git.digetx@gmail.com>
Hi Dmitry,
I'll add just a couple of minor comments, in general the code looks
very good.
On 10/20/2017 12:34 AM, Dmitry Osipenko wrote:
> NVIDIA Tegra20/30/114/124/132 SoC's have video decoder engine that
> supports standard set of video formats like H.264 / MPEG-4 / WMV / VC1.
> Currently implemented decoding of CAVLC H.264 on Tegra20 only.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
[snip]
> +++ b/drivers/staging/tegra-vde/uapi.h
> @@ -0,0 +1,101 @@
> +/*
> + * Copyright (C) 2016-2017 Dmitry Osipenko <digetx@gmail.com>
> + * All Rights Reserved.
> + *
> + * 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 (including the next
> + * paragraph) 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
> + * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS 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.
>From the specified MODULE_LICENSE("GPL") I'd rather expect to see a reference
to GPLv2+ license in the header, and here the text resembles MIT license only.
I understand that it is a UAPI header file and it may happen that different
rules are applied to this kind of sources, hopefully Greg can give the right
directions.
In general you may avoid the headache with the custom UAPI, if you reuse
V4L2 interfaces, if I remember correctly drivers/media/platform/coda does it.
Also from my point of view the custom UAPI is the only reason why the driver
is pushed to the staging folder.
[snip]
> +struct tegra_vde {
> + void __iomem *sxe;
> + void __iomem *bsev;
> + void __iomem *mbe;
> + void __iomem *ppe;
> + void __iomem *mce;
> + void __iomem *tfe;
> + void __iomem *ppb;
> + void __iomem *vdma;
> + void __iomem *frameid;
Please find a comment in tegra_vde_probe() function regarding
devm_ioremap_resource() calls.
> + struct mutex lock;
> + struct miscdevice miscdev;
> + struct reset_control *rst;
> + struct gen_pool *iram_pool;
> + struct completion decode_completion;
> + struct clk *clk;
> + dma_addr_t iram_lists_addr;
> + u32 *iram;
> +};
[snip]
> +static int tegra_vde_wait_bsev(struct tegra_vde *vde, bool wait_dma)
> +{
> + struct device *dev = vde->miscdev.parent;
> + u32 value;
> + int err;
> +
> + err = readl_relaxed_poll_timeout(vde->bsev + INTR_STATUS, value,
> + !(value & BIT(2)), 1, 100);
> + if (err) {
> + dev_err(dev, "BSEV unknown bit timeout\n");
> + return err;
> + }
> +
> + err = readl_relaxed_poll_timeout(vde->bsev + INTR_STATUS, value,
> + (value & BSE_ICMDQUE_EMPTY), 1, 100);
> + if (err) {
> + dev_err(dev, "BSEV ICMDQUE flush timeout\n");
> + return err;
> + }
> +
> + if (!wait_dma)
> + return 0;
> +
> + err = readl_relaxed_poll_timeout(vde->bsev + INTR_STATUS, value,
> + !(value & BSE_DMA_BUSY), 1, 100);
> + if (err) {
> + dev_err(dev, "BSEV DMA timeout\n");
> + return err;
> + }
> +
> + return 0;
if (err)
dev_err(dev, "BSEV DMA timeout\n");
return err;
is two lines shorter.
> +}
> +
[snip]
> +static int tegra_vde_attach_dmabufs_to_frame(struct device *dev,
> + struct video_frame *frame,
> + struct tegra_vde_h264_frame *source,
> + enum dma_data_direction dma_dir,
> + bool baseline_profile,
> + size_t csize)
> +{
> + int err;
> +
> + err = tegra_vde_attach_dmabuf(dev, source->y_fd,
> + source->y_offset, csize * 4,
> + &frame->y_dmabuf_attachment,
> + &frame->y_addr,
> + &frame->y_sgt,
> + NULL, dma_dir);
> + if (err)
> + return err;
> +
> + err = tegra_vde_attach_dmabuf(dev, source->cb_fd,
> + source->cb_offset, csize,
> + &frame->cb_dmabuf_attachment,
> + &frame->cb_addr,
> + &frame->cb_sgt,
> + NULL, dma_dir);
> + if (err)
> + goto err_release_y;
> +
> + err = tegra_vde_attach_dmabuf(dev, source->cr_fd,
> + source->cr_offset, csize,
> + &frame->cr_dmabuf_attachment,
> + &frame->cr_addr,
> + &frame->cr_sgt,
> + NULL, dma_dir);
> + if (err)
> + goto err_release_cb;
> +
> + if (baseline_profile) {
> + frame->aux_addr = 0xF4DEAD00;
> + } else {
I would rather suggest to do
if (baseline_profile) {
frame->aux_addr = 0xF4DEAD00;
return 0;
}
and remove indentation from the code block below.
> + err = tegra_vde_attach_dmabuf(dev, source->aux_fd,
> + source->aux_offset, csize,
> + &frame->aux_dmabuf_attachment,
> + &frame->aux_addr,
> + &frame->aux_sgt,
> + NULL, dma_dir);
> + if (err)
> + goto err_release_cr;
> + }
> +
> + return 0;
if (!err)
return 0;
and then remove a check above.
> +
> +err_release_cr:
> + tegra_vde_detach_and_put_dmabuf(frame->cr_dmabuf_attachment,
> + frame->cr_sgt, dma_dir);
> +err_release_cb:
> + tegra_vde_detach_and_put_dmabuf(frame->cb_dmabuf_attachment,
> + frame->cb_sgt, dma_dir);
> +err_release_y:
> + tegra_vde_detach_and_put_dmabuf(frame->y_dmabuf_attachment,
> + frame->y_sgt, dma_dir);
> +
> + return err;
> +}
[snip]
> +static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
> + unsigned long vaddr)
> +{
> + struct device *dev = vde->miscdev.parent;
> + struct tegra_vde_h264_decoder_ctx ctx;
> + struct tegra_vde_h264_frame __user *frames_user;
> + struct tegra_vde_h264_frame *frames;
> + struct video_frame *dpb_frames;
> + struct dma_buf_attachment *bitstream_data_dmabuf_attachment;
> + struct sg_table *bitstream_sgt;
> + enum dma_data_direction dma_dir;
> + dma_addr_t bitstream_data_addr;
> + dma_addr_t bsev_ptr;
> + size_t bitstream_data_size;
> + unsigned int macroblocks_nb;
> + unsigned int read_bytes;
> + unsigned int i = 0;
> + bool timeout;
> + int ret;
> +
> + if (copy_from_user(&ctx, (void __user *)vaddr, sizeof(ctx)))
> + return -EFAULT;
> +
> + ret = tegra_vde_validate_h264_ctx(dev, &ctx);
> + if (ret)
> + return -EINVAL;
Or "return ret".
> +
> + ret = tegra_vde_attach_dmabuf(dev, ctx.bitstream_data_fd,
> + ctx.bitstream_data_offset, 0,
> + &bitstream_data_dmabuf_attachment,
> + &bitstream_data_addr,
> + &bitstream_sgt,
> + &bitstream_data_size,
> + DMA_TO_DEVICE);
> + if (ret)
> + return ret;
> +
> + dpb_frames = kcalloc(ctx.dpb_frames_nb, sizeof(*dpb_frames),
> + GFP_KERNEL);
> + if (!dpb_frames) {
> + ret = -ENOMEM;
> + goto err_release_bitstream_dmabuf;
> + }
> +
> + macroblocks_nb = ctx.pic_width_in_mbs * ctx.pic_height_in_mbs;
> + frames_user = u64_to_user_ptr(ctx.dpb_frames_ptr);
> +
> + frames = kmalloc_array(ctx.dpb_frames_nb, sizeof(*frames), GFP_KERNEL);
> + if (!frames) {
> + ret = -ENOMEM;
> + goto err_release_dpb_frames;
> + }
> +
> + if (copy_from_user(frames, frames_user,
> + ctx.dpb_frames_nb * sizeof(*frames))) {
> + ret = -EFAULT;
> + goto free_frames;
It could be
ret = -EFAULT;
kfree(frames);
goto err_release_dpb_frames;
> + }
> +
> + for (i = 0; i < ctx.dpb_frames_nb; i++) {
> + ret = tegra_vde_validate_frame(dev, &frames[i]);
> + if (ret)
> + goto free_frames;
It could be
if (ret)
break;
or
if (ret) {
kfree(frames);
goto err_release_dpb_frames;
}
> +
> + dpb_frames[i].flags = frames[i].flags;
> + dpb_frames[i].frame_num = frames[i].frame_num;
> +
> + dma_dir = (i == 0) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
> +
> + ret = tegra_vde_attach_dmabufs_to_frame(dev, &dpb_frames[i],
> + &frames[i], dma_dir,
> + ctx.baseline_profile,
> + macroblocks_nb * 64);
> + if (ret)
> + goto free_frames;
It could be
if (ret)
break;
or
if (ret) {
kfree(frames);
goto err_release_dpb_frames;
}
I don't strictly ask for the changes, but in my opinion less indentation
is better.
> + }
> +
> +free_frames:
> + kfree(frames);
> +
> + if (ret)
> + goto err_release_dpb_frames;
If you select an option with 'kfree' in all three cases above, then
it is possible to remove 'kfree' here, remove the check and remove
a goto label.
It's up to you to decide, which version is better.
[snip]
> +
> +static int tegra_vde_runtime_resume(struct device *dev)
> +{
> + struct tegra_vde *vde = dev_get_drvdata(dev);
> + int err;
> +
> + err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_VDEC,
> + vde->clk, vde->rst);
> + if (err) {
> + dev_err(dev, "Failed to power up HW : %d\n", err);
> + return err;
> + }
> +
> + return 0;
if (err)
dev_err(dev, "Failed to power up HW : %d\n", err);
return err;
saves 2 lines of code.
> +}
> +
> +static int tegra_vde_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct resource *regs;
> + struct tegra_vde *vde;
> + int irq, err;
> +
> + vde = devm_kzalloc(dev, sizeof(*vde), GFP_KERNEL);
> + if (!vde)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, vde);
> +
> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sxe");
> + if (!regs)
> + return -ENODEV;
> +
> + vde->sxe = devm_ioremap_resource(dev, regs);
> + if (IS_ERR(vde->sxe))
> + return PTR_ERR(vde->sxe);
> +
> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "bsev");
> + if (!regs)
> + return -ENODEV;
> +
> + vde->bsev = devm_ioremap_resource(dev, regs);
> + if (IS_ERR(vde->bsev))
> + return PTR_ERR(vde->bsev);
> +
> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mbe");
> + if (!regs)
> + return -ENODEV;
> +
> + vde->mbe = devm_ioremap_resource(dev, regs);
> + if (IS_ERR(vde->mbe))
> + return PTR_ERR(vde->mbe);
> +
> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ppe");
> + if (!regs)
> + return -ENODEV;
> +
> + vde->ppe = devm_ioremap_resource(dev, regs);
> + if (IS_ERR(vde->ppe))
> + return PTR_ERR(vde->ppe);
> +
> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mce");
> + if (!regs)
> + return -ENODEV;
> +
> + vde->mce = devm_ioremap_resource(dev, regs);
> + if (IS_ERR(vde->mce))
> + return PTR_ERR(vde->mce);
> +
> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tfe");
> + if (!regs)
> + return -ENODEV;
> +
> + vde->tfe = devm_ioremap_resource(dev, regs);
> + if (IS_ERR(vde->tfe))
> + return PTR_ERR(vde->tfe);
> +
> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ppb");
> + if (!regs)
> + return -ENODEV;
> +
> + vde->ppb = devm_ioremap_resource(dev, regs);
> + if (IS_ERR(vde->ppb))
> + return PTR_ERR(vde->ppb);
> +
> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "vdma");
> + if (!regs)
> + return -ENODEV;
> +
> + vde->vdma = devm_ioremap_resource(dev, regs);
> + if (IS_ERR(vde->vdma))
> + return PTR_ERR(vde->vdma);
> +
> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "frameid");
> + if (!regs)
> + return -ENODEV;
> +
> + vde->frameid = devm_ioremap_resource(dev, regs);
> + if (IS_ERR(vde->frameid))
> + return PTR_ERR(vde->frameid);
This is just my personal opinion on which I do not insist, however I'd like
to share it, a single memory region described in DTB (and the same single
region mapped once) and subregion offsets in the driver may be more preferable.
If I remember correctly that was found in the previous versions of the driver,
the new scheme might be preferred only if the offsets are different in different
Tegra VDE IPs, but even in the latter case it might be better to handle the
difference inside the driver.
> +
> + vde->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(vde->clk)) {
> + err = PTR_ERR(vde->clk);
> + dev_err(dev, "Could not get VDE clk %d\n", err);
> + return err;
> + }
> +
> + vde->rst = devm_reset_control_get(dev, NULL);
> + if (IS_ERR(vde->rst)) {
> + err = PTR_ERR(vde->rst);
> + dev_err(dev, "Could not get VDE reset %d\n", err);
> + return err;
> + }
> +
> + irq = platform_get_irq_byname(pdev, "sync-token");
> + if (irq < 0)
> + return irq;
> +
> + err = devm_request_irq(dev, irq, tegra_vde_isr, 0,
> + dev_name(dev), vde);
> + if (err) {
> + dev_err(dev, "Could not request IRQ %d\n", err);
> + return err;
> + }
> +
> + vde->iram_pool = of_gen_pool_get(dev->of_node, "iram", 0);
> + if (!vde->iram_pool) {
> + dev_err(dev, "Could not get IRAM pool\n");
> + return -EPROBE_DEFER;
> + }
> +
> + vde->iram = gen_pool_dma_alloc(vde->iram_pool, 0x3FC00,
The size of the pool should not be hardcoded, it is expected to be
obtained from the device tree:
gen_pool_size(vde->iram_pool)
> + &vde->iram_lists_addr);
> + if (!vde->iram) {
> + dev_err(dev, "Could not reserve IRAM\n");
> + return -ENOMEM;
> + }
> +
> + mutex_init(&vde->lock);
> + init_completion(&vde->decode_completion);
> +
> + vde->miscdev.minor = MISC_DYNAMIC_MINOR;
> + vde->miscdev.name = "tegra_vde";
> + vde->miscdev.fops = &tegra_vde_fops;
> + vde->miscdev.parent = dev;
> +
> + err = misc_register(&vde->miscdev);
> + if (err) {
> + dev_err(dev, "Failed to register misc device: %d\n", err);
> + goto err_gen_free;
> + }
> +
> + pm_runtime_enable(dev);
> + pm_runtime_use_autosuspend(dev);
> + pm_runtime_set_autosuspend_delay(dev, 300);
> +
> + if (!pm_runtime_enabled(dev)) {
> + err = tegra_vde_runtime_resume(dev);
> + if (err)
> + goto err_misc_unreg;
> + }
> +
> + return 0;
> +
> +err_misc_unreg:
> + misc_deregister(&vde->miscdev);
> +
> +err_gen_free:
> + gen_pool_free(vde->iram_pool,
> + (unsigned long)vde->iram, 0x3FC00);
See a comment above.
> +
> + return err;
> +}
> +
> +static int tegra_vde_remove(struct platform_device *pdev)
> +{
> + struct tegra_vde *vde = platform_get_drvdata(pdev);
> + struct device *dev = &pdev->dev;
> + int err;
> +
> + if (!pm_runtime_enabled(dev)) {
> + err = tegra_vde_runtime_suspend(dev);
> + if (err)
> + return err;
> + }
> +
> + pm_runtime_dont_use_autosuspend(dev);
> + pm_runtime_disable(dev);
> +
> + misc_deregister(&vde->miscdev);
> +
> + gen_pool_free(vde->iram_pool,
> + (unsigned long)vde->iram, 0x3FC00);
See a comment above.
> +
> + return 0;
> +}
> +
[snip]
> +
> +MODULE_DESCRIPTION("NVIDIA Tegra20 Video Decoder driver");
> +MODULE_AUTHOR("Dmitry Osipenko");
Please add your email here, then if you are not a maintainer (and by
the way please consider to add your name to the MAINTAINERS file),
you'll get a higher chance to receive updates in the future.
> +MODULE_LICENSE("GPL");
>
--
With best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH v4 1/5] ARM: tegra: Add device tree node to describe IRAM
From: Vladimir Zapolskiy @ 2017-11-11 14:18 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, Jonathan Hunter,
Greg Kroah-Hartman, Rob Herring, Mauro Carvalho Chehab,
Stephen Warren
Cc: Dan Carpenter, linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <8ce696bc2b4b1808f6c7f7a967a3dacd954d2a4e.1508448293.git.digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Hi Dmitry,
On 10/20/2017 12:34 AM, Dmitry Osipenko wrote:
> From: Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
>
> All Tegra SoCs contain 256KiB IRAM, which is used to store CPU resume code
> and by hardware engines like a video decoder.
>
> Signed-off-by: Vladimir Zapolskiy <vz-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
Please add also your own closing "Signed-off-by" tag, please reference
to "Developer's Certificate of Origin 1.1", point (c), it is found in
Documentation/process/submitting-patches.rst
> ---
> arch/arm/boot/dts/tegra114.dtsi | 8 ++++++++
> arch/arm/boot/dts/tegra124.dtsi | 8 ++++++++
> arch/arm/boot/dts/tegra20.dtsi | 8 ++++++++
> arch/arm/boot/dts/tegra30.dtsi | 8 ++++++++
My assumption is that Thierry would prefer to get 4 separate patches,
one for each platform, please split the patch.
Also thanks for your time and your efforts applied to push my occasional
change, please feel free to take your own authorship for 3 out of 4 patches.
> 4 files changed, 32 insertions(+)
>
> diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
> index 8932ea3afd5f..13f6087790c8 100644
> --- a/arch/arm/boot/dts/tegra114.dtsi
> +++ b/arch/arm/boot/dts/tegra114.dtsi
> @@ -10,6 +10,14 @@
> compatible = "nvidia,tegra114";
> interrupt-parent = <&lic>;
>
> + iram@40000000 {
> + compatible = "mmio-sram";
Unfortunately Thierry hasn't yet replied, but my assumption is that
the list of compatibles should be extended with one more SoC specific
value like
compatible = "nvidia,tegra114-sysram", "mmio-sram";
I'm not sure, if Tegra maintainers want to see a new compatible
described in Documentation/devicetree/bindings.
> + reg = <0x40000000 0x40000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0x40000000 0x40000>;
> + };
> +
> host1x@50000000 {
> compatible = "nvidia,tegra114-host1x", "simple-bus";
> reg = <0x50000000 0x00028000>;
> diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
> index 8baf00b89efb..a3585ed82646 100644
> --- a/arch/arm/boot/dts/tegra124.dtsi
> +++ b/arch/arm/boot/dts/tegra124.dtsi
The considerations from above are applicable to the rest of
the touched platforms.
--
With best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH v4 2/5] media: dt: bindings: Add binding for NVIDIA Tegra Video Decoder Engine
From: Vladimir Zapolskiy @ 2017-11-11 14:21 UTC (permalink / raw)
To: Dmitry Osipenko, Thierry Reding, Jonathan Hunter,
Greg Kroah-Hartman, Rob Herring, Mauro Carvalho Chehab,
Stephen Warren
Cc: devel, devicetree, linux-kernel, linux-tegra, Dan Carpenter,
linux-media
In-Reply-To: <bf5b91666229f9e46ed8c73d6ca2e4b65f86b5ab.1508448293.git.digetx@gmail.com>
Hi Dmitry,
On 10/20/2017 12:34 AM, Dmitry Osipenko wrote:
> Add binding documentation for the Video Decoder Engine which is found
> on NVIDIA Tegra20/30/114/124/132 SoC's.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> .../devicetree/bindings/media/nvidia,tegra-vde.txt | 55 ++++++++++++++++++++++
> 1 file changed, 55 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
>
> diff --git a/Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt b/Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
> new file mode 100644
> index 000000000000..470237ed6fe5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
> @@ -0,0 +1,55 @@
> +NVIDIA Tegra Video Decoder Engine
> +
> +Required properties:
> +- compatible : Must contain one of the following values:
> + - "nvidia,tegra20-vde"
> + - "nvidia,tegra30-vde"
> + - "nvidia,tegra114-vde"
> + - "nvidia,tegra124-vde"
> + - "nvidia,tegra132-vde"
> +- reg : Must contain an entry for each entry in reg-names.
> +- reg-names : Must include the following entries:
> + - sxe
> + - bsev
> + - mbe
> + - ppe
> + - mce
> + - tfe
> + - ppb
> + - vdma
> + - frameid
I've already mentioned it in my review of the driver code, but the
version from v3 with a single region is more preferable.
Also it implies that "reg-names" property will be removed.
> +- iram : Must contain phandle to the mmio-sram device node that represents
> + IRAM region used by VDE.
> +- interrupts : Must contain an entry for each entry in interrupt-names.
> +- interrupt-names : Must include the following entries:
> + - sync-token
> + - bsev
> + - sxe
> +- clocks : Must include the following entries:
> + - vde
> +- resets : Must include the following entries:
> + - vde
> +
> +Example:
> +
> +video-codec@6001a000 {
> + compatible = "nvidia,tegra20-vde";
> + reg = <0x6001a000 0x1000 /* Syntax Engine */
> + 0x6001b000 0x1000 /* Video Bitstream Engine */
> + 0x6001c000 0x100 /* Macroblock Engine */
> + 0x6001c200 0x100 /* Post-processing Engine */
> + 0x6001c400 0x100 /* Motion Compensation Engine */
> + 0x6001c600 0x100 /* Transform Engine */
> + 0x6001c800 0x100 /* Pixel prediction block */
> + 0x6001ca00 0x100 /* Video DMA */
> + 0x6001d800 0x300 /* Video frame controls */>;
> + reg-names = "sxe", "bsev", "mbe", "ppe", "mce",
> + "tfe", "ppb", "vdma", "frameid";
> + iram = <&vde_pool>; /* IRAM region */
> + interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>, /* Sync token interrupt */
> + <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>, /* BSE-V interrupt */
> + <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; /* SXE interrupt */
> + interrupt-names = "sync-token", "bsev", "sxe";
> + clocks = <&tegra_car TEGRA20_CLK_VDE>;
> + resets = <&tegra_car 61>;
> +};
>
--
With best wishes,
Vladimir
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx6: Remove unneeded address-cells/size-cells
From: Christopher Spinrath @ 2017-11-11 14:24 UTC (permalink / raw)
To: Fabio Estevam, Shawn Guo, Sascha Hauer
Cc: Mark Rutland, devicetree@vger.kernel.org, Rob Herring,
Russell King, linux-arm-kernel@lists.infradead.org
In-Reply-To: <1510348378-18150-1-git-send-email-festevam@gmail.com>
Hi Fabio,
(I'm sorry if I've missed some recipients or broke the email thread but
I got Fabio's email from the infradead archive.)
On 11/10/2017 10:12 PM, festevam at gmail.com (Fabio Estevam) wrote:
> From: Fabio Estevam <fabio.estevam at nxp.com>
>
> According to Documentation/devicetree/bindings/mtd/jedec,spi-nor.txt:
>
> "- #address-cells, #size-cells : Must be present if the device has
> sub-nodes representing partitions."
>
> Remove the occurrences of #address-cells/size-cells where no sub-nodes
> representing partitions are present.
This results in a regression for those devices where U-Boot is supposed
to inject the partition sub-nodes. For example, this is the case for
cm-fx6 module based devices like the Utilite Pro.
Linux fails to parse the partitions and reports:
[ 9.330881] spi0.0: error parsing ofpart partition
/soc/aips-bus@2000000/spba-bus@2000000/ecspi@2008000/m25p80@0/partition@0
(/soc/aips-bus@2000000/spba-bus@2000000/ecspi@2008000/m25p80@0)
[ 9.330887] mtd: failed to find partitions; one or more parsers
reports errors (-22)
Also, the documentation does not forbid the #address-cells/size-cells if
no partitions are present.
So, please, drop this patch (or exclude at least the cm-fx6 related change).
Thanks!
Christopher
> Signed-off-by: Fabio Estevam <fabio.estevam at nxp.com>
> ---
> arch/arm/boot/dts/imx6q-cm-fx6.dts | 2 --
> arch/arm/boot/dts/imx6q-mccmon6.dts | 2 --
> arch/arm/boot/dts/imx6qdl-aristainetos.dtsi | 2 --
> arch/arm/boot/dts/imx6qdl-aristainetos2.dtsi | 2 --
> arch/arm/boot/dts/imx6qdl-sabreauto.dtsi | 2 --
> arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 2 --
> arch/arm/boot/dts/imx6sl-evk.dts | 2 --
> arch/arm/boot/dts/imx6sx-sdb-reva.dts | 4 ----
> arch/arm/boot/dts/imx6sx-sdb.dts | 4 ----
> 9 files changed, 22 deletions(-)
>
> diff --git a/arch/arm/boot/dts/imx6q-cm-fx6.dts b/arch/arm/boot/dts/imx6q-cm-fx6.dts
> index bc7587c..5df84e2 100644
> --- a/arch/arm/boot/dts/imx6q-cm-fx6.dts
> +++ b/arch/arm/boot/dts/imx6q-cm-fx6.dts
> @@ -194,8 +194,6 @@
> status = "okay";
>
> m25p80 at 0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> compatible = "st,m25p", "jedec,spi-nor";
> spi-max-frequency = <20000000>;
> reg = <0>;
> diff --git a/arch/arm/boot/dts/imx6q-mccmon6.dts b/arch/arm/boot/dts/imx6q-mccmon6.dts
> index cab36f4..9636e15 100644
> --- a/arch/arm/boot/dts/imx6q-mccmon6.dts
> +++ b/arch/arm/boot/dts/imx6q-mccmon6.dts
> @@ -90,8 +90,6 @@
> status = "okay";
>
> s25sl032p: flash at 0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> compatible = "jedec,spi-nor";
> spi-max-frequency = <40000000>;
> reg = <0>;
> diff --git a/arch/arm/boot/dts/imx6qdl-aristainetos.dtsi b/arch/arm/boot/dts/imx6qdl-aristainetos.dtsi
> index 7d64075..d8f578c 100644
> --- a/arch/arm/boot/dts/imx6qdl-aristainetos.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-aristainetos.dtsi
> @@ -106,8 +106,6 @@
> status = "okay";
>
> flash: m25p80 at 0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> compatible = "micron,n25q128a11", "jedec,spi-nor";
> spi-max-frequency = <20000000>;
> reg = <0>;
> diff --git a/arch/arm/boot/dts/imx6qdl-aristainetos2.dtsi b/arch/arm/boot/dts/imx6qdl-aristainetos2.dtsi
> index 1b18728..5875252 100644
> --- a/arch/arm/boot/dts/imx6qdl-aristainetos2.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-aristainetos2.dtsi
> @@ -136,8 +136,6 @@
> status = "okay";
>
> flash: m25p80 at 1 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> compatible = "micron,n25q128a11", "jedec,spi-nor";
> spi-max-frequency = <20000000>;
> reg = <1>;
> diff --git a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> index 4fa2fac..828a9a5 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabreauto.dtsi
> @@ -200,8 +200,6 @@
> status = "disabled"; /* pin conflict with WEIM NOR */
>
> flash: m25p80 at 0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> compatible = "st,m25p32", "jedec,spi-nor";
> spi-max-frequency = <20000000>;
> reg = <0>;
> diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> index 0a50705..acb0292 100644
> --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
> @@ -196,8 +196,6 @@
> status = "okay";
>
> flash: m25p80 at 0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> compatible = "st,m25p32", "jedec,spi-nor";
> spi-max-frequency = <20000000>;
> reg = <0>;
> diff --git a/arch/arm/boot/dts/imx6sl-evk.dts b/arch/arm/boot/dts/imx6sl-evk.dts
> index 60600b4..03a2240 100644
> --- a/arch/arm/boot/dts/imx6sl-evk.dts
> +++ b/arch/arm/boot/dts/imx6sl-evk.dts
> @@ -123,8 +123,6 @@
> status = "okay";
>
> flash: m25p80 at 0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> compatible = "st,m25p32", "jedec,spi-nor";
> spi-max-frequency = <20000000>;
> reg = <0>;
> diff --git a/arch/arm/boot/dts/imx6sx-sdb-reva.dts b/arch/arm/boot/dts/imx6sx-sdb-reva.dts
> index e3533e7..ebbed57 100644
> --- a/arch/arm/boot/dts/imx6sx-sdb-reva.dts
> +++ b/arch/arm/boot/dts/imx6sx-sdb-reva.dts
> @@ -127,16 +127,12 @@
>
> flash0: s25fl128s at 0 {
> reg = <0>;
> - #address-cells = <1>;
> - #size-cells = <1>;
> compatible = "spansion,s25fl128s", "jedec,spi-nor";
> spi-max-frequency = <66000000>;
> };
>
> flash1: s25fl128s at 1 {
> reg = <1>;
> - #address-cells = <1>;
> - #size-cells = <1>;
> compatible = "spansion,s25fl128s", "jedec,spi-nor";
> spi-max-frequency = <66000000>;
> };
> diff --git a/arch/arm/boot/dts/imx6sx-sdb.dts b/arch/arm/boot/dts/imx6sx-sdb.dts
> index 6dd9beb..4b7481a 100644
> --- a/arch/arm/boot/dts/imx6sx-sdb.dts
> +++ b/arch/arm/boot/dts/imx6sx-sdb.dts
> @@ -113,16 +113,12 @@
> status = "okay";
>
> flash0: n25q256a at 0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> compatible = "micron,n25q256a", "jedec,spi-nor";
> spi-max-frequency = <29000000>;
> reg = <0>;
> };
>
> flash1: n25q256a at 1 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> compatible = "micron,n25q256a", "jedec,spi-nor";
> spi-max-frequency = <29000000>;
> reg = <1>;
>
^ permalink raw reply
* Re: [PATCH v10 2/2] media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver
From: Sakari Ailus @ 2017-11-11 23:32 UTC (permalink / raw)
To: Niklas Söderlund
Cc: Laurent Pinchart, Hans Verkuil, linux-media, Rob Herring,
devicetree, linux-renesas-soc, tomoharu.fukawa.eb, Kieran Bingham,
Sakari Ailus, Geert Uytterhoeven
In-Reply-To: <20171111001113.GB13042@bigcity.dyn.berto.se>
Hejssan, Niklas!
On Sat, Nov 11, 2017 at 01:11:13AM +0100, Niklas Söderlund wrote:
> Hej Sakari,
>
> On 2017-11-11 00:32:27 +0200, Sakari Ailus wrote:
> > Hej Niklas,
> >
> > Tack för uppdaterade lappar! Jag har några kommentar nedan... det ser bra
> > ut överallt.
>
> Tack för din feedback!
Var så god!
...
> > > +static int rcar_csi2_calc_phypll(struct rcar_csi2 *priv,
> > > + struct v4l2_subdev *source,
> > > + struct v4l2_mbus_framefmt *mf,
> > > + u32 *phypll)
> > > +{
> > > + const struct phypll_hsfreqrange *hsfreq;
> > > + const struct rcar_csi2_format *format;
> > > + struct v4l2_ctrl *ctrl;
> > > + u64 mbps;
> > > +
> > > + ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> >
> > How about LINK_FREQ instead? It'd be more straightforward to calculate
> > this. Up to you.
>
> I need to use PIXEL_RATE as my test setup uses the adv748x driver which
> only implement that control. In the short term I would like to support
> both, but I need a setup to test that before I can add support for it.
> In the long term, maybe we should deprecate one of the controls?
Hmm. At least we musn't give the responsibility to calculate the pixel rate
to the end user: this depends on the bus and that is not visible to the
user space.
The link frequency is usually chosen from a few alternatives (or there's
just a single choice). And the pixel rate depends on the pixel code chosen
--- so you'd have a menu with menu entries changing based on format. That'd
be odd.
Perhaps just leave it as-is, as suggested by Laurent?
...
> > > +
> > > + dev_dbg(priv->dev, "Using source %s pad: %u\n", source->name,
> > > + source_pad->index);
> > > +
> > > + fmt.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > > + fmt.pad = source_pad->index;
> > > + ret = v4l2_subdev_call(source, pad, get_fmt, NULL, &fmt);
> > > + if (ret)
> > > + return ret;
> >
> > The link format has been validated already by this point this isn't
> > supposed to fail or produce different results than in link validation.
> >
> > You could get the same format from the local pad, too.
>
> Another good catch, I will use the format stored locally to make the
> code simpler. As you state the formats are already validated so it's
> safe to do so. I still need to get the remote subdevice to be able to
> read the PIXEL_RATE control, but I can move that to
> rcar_csi2_calc_phypll() where it's actually used.
Sounds good to me.
...
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int rcar_csi2_set_pad_format(struct v4l2_subdev *sd,
> > > + struct v4l2_subdev_pad_config *cfg,
> > > + struct v4l2_subdev_format *format)
> > > +{
> > > + struct rcar_csi2 *priv = sd_to_csi2(sd);
> > > + struct v4l2_mbus_framefmt *framefmt;
> > > +
> >
> > Is everything possible? For instance, are there limits regarding width or
> > height? Or the pixel code? They should be checked here.
>
> Yes it do care about pixel code which was only validated at s_stream()
> time, will move the check here thanks!
>
> >
> > Also the format on the source pad is, presumably, dependent on the format
> > on the sink pad.
>
> Yes, but since this driver can't change the format in any way is there
> any harm in mirror whatever is set on one pad on the other(s)? This will
> change once multiple stream support is added in a later series.
If the hardware does not have functionality that may affect the size or the
pixel code, then the format on the source pad shouldn't be settable.
Yes, this will change when support for multiple streams is added. You'll
get more pads, and the sink pad no longer will have a format. But... it's a
good question how to prepare for this --- is it possible without changing
the user space API? I'm sure we'll have other such cases, such as the
smiapp driver with sensor embedded data.
...
> > > +static int rcar_csi2_parse_dt(struct rcar_csi2 *priv)
> > > +{
> > > + struct device_node *ep;
> > > + struct v4l2_fwnode_endpoint v4l2_ep;
> > > + int ret;
> > > +
> > > + ep = of_graph_get_endpoint_by_regs(priv->dev->of_node, 0, 0);
> > > + if (!ep) {
> > > + dev_err(priv->dev, "Not connected to subdevice\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = v4l2_fwnode_endpoint_parse(of_fwnode_handle(ep), &v4l2_ep);
> > > + if (ret) {
> > > + dev_err(priv->dev, "Could not parse v4l2 endpoint\n");
> > > + of_node_put(ep);
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ret = rcar_csi2_parse_v4l2(priv, &v4l2_ep);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + priv->remote.match.fwnode.fwnode =
> > > + fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > > + priv->remote.match_type = V4L2_ASYNC_MATCH_FWNODE;
> > > +
> > > + of_node_put(ep);
> > > +
> > > + priv->notifier.subdevs = devm_kzalloc(priv->dev,
> > > + sizeof(*priv->notifier.subdevs),
> > > + GFP_KERNEL);
> > > + if (priv->notifier.subdevs == NULL)
> > > + return -ENOMEM;
> > > +
> > > + priv->notifier.num_subdevs = 1;
> > > + priv->notifier.subdevs[0] = &priv->remote;
> > > + priv->notifier.ops = &rcar_csi2_notify_ops;
> > > +
> > > + dev_dbg(priv->dev, "Found '%pOF'\n",
> > > + to_of_node(priv->remote.match.fwnode.fwnode));
> > > +
> > > + return v4l2_async_subdev_notifier_register(&priv->subdev,
> > > + &priv->notifier);
> >
> > Could you use v4l2_async_notifier_parse_fwnode_endpoints_by_port() or does
> > something bad happen if you do? The intent with that was to avoid functions
> > like the above in drivers.
>
> Unfortunately not :-(
>
> I have a local patch which do this, but it won't work since this driver
> needs to register remote endpoints (fwnode_graph_get_remote_endpoint()
> above) and not the remote parent in the async framework. The reason for
> this is that that is what the adv748x driver registers with the async
> framework given that it only have one node in DT but registers multiple
> subdevices. I would love to in the future resolve this in the helper
> functions and switch to such a helper here.
Right. How does the fwnode matching work then? Don't you need to do
endpoint node matching here, i.e. the patch for the fwnode framework, too?
Could you change how the framework assigns the matched fwnode?
What I'd like to do still is to decouple registering async sub-devices and
parsing the endpoints. The two are quite entangled and I just haven't been
able to come up with a good API for that yet.
--
Trevliga hälsningar,
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply
* Re: [PATCH v10 2/2] media: rcar-csi2: add Renesas R-Car MIPI CSI-2 receiver driver
From: Sakari Ailus @ 2017-11-11 23:38 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Niklas Söderlund, Hans Verkuil, linux-media, Rob Herring,
devicetree, linux-renesas-soc, tomoharu.fukawa.eb, Kieran Bingham,
Sakari Ailus, Geert Uytterhoeven
In-Reply-To: <2456560.miCsvfXtsD@avalon>
Hi Laurent,
On Sat, Nov 11, 2017 at 08:17:59AM +0200, Laurent Pinchart wrote:
> > > +static int rcar_csi2_s_power(struct v4l2_subdev *sd, int on)
> > > +{
> > > + struct rcar_csi2 *priv = sd_to_csi2(sd);
> > > +
> > > + if (on)
> > > + pm_runtime_get_sync(priv->dev);
> > > + else
> > > + pm_runtime_put(priv->dev);
> >
> > The pipeline you have is already rather complex. Would it be an option to
> > power the hardware on when streaming is started? The smiapp driver does
> > this, without even implementing the s_power callback. (You'd still need to
> > call it on the image source, as long as we have drivers that need it.)
>
> We've briefly discussed this before, and I agree that pipeline power
> management needs to be redesigned, but we still haven't agreed on a proper
> architecture for that. Feel free to propose an RFC :-) In the meantime I
Well, perhaps we should look at the use cases and see if there are gaps.
Runtime PM is essentially used everywhere else in the kernel.
> wouldn't try to enforce one specific model.
What I just wanted to point out that by switching to runtime PM you avoid
adding a new driver which is dependent on the s_power callback which I
don't think we'll want to keep in the long run. In this case there's no
benefit from s_power in any quantifiable way that I can see.
Actually the master driver manages calling the s_power callback so there's
hardly a need to do so here. It's just that the master drivers still need
that as long as there are sub-device drivers that depend on it.
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@iki.fi
^ permalink raw reply
* Re: [PATCH v4 4/4] ARM64: dts: meson: drop "sana" clock from SAR ADC
From: Martin Blumenstingl @ 2017-11-12 1:33 UTC (permalink / raw)
To: Yixun Lan
Cc: Kevin Hilman, devicetree-u79uwXL29TY76Z2rM5mHXA, Rob Herring,
Mark Rutland, Carlo Caione, Xingyu Chen,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171107141027.30717-1-yixun.lan-LpR1jeaWuhtBDgjK7y7TUQ@public.gmane.org>
Hi Yixun,
On Tue, Nov 7, 2017 at 3:10 PM, Yixun Lan <yixun.lan-LpR1jeaWuhtBDgjK7y7TUQ@public.gmane.org> wrote:
> From: Xingyu Chen <xingyu.chen-LpR1jeaWuhtBDgjK7y7TUQ@public.gmane.org>
>
> The SAR ADC modules doesn't require The "sana" clock.
>
> Singed-off-by: Xingyu Chen <xingyu.chen-LpR1jeaWuhtBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Yixun Lan <yixun.lan-LpR1jeaWuhtBDgjK7y7TUQ@public.gmane.org>
> ---
> arch/arm/boot/dts/meson8.dtsi | 5 ++---
> arch/arm/boot/dts/meson8b.dtsi | 5 ++---
these two should go into a separate patch (with "ARM: dts: ..."
prefix) - the ARM maintainers want separate pull requests for the
32-bit and 64-bit .dts changes, so patches should also follow that
schema
with that fixed, you can add my ACK on both (32-bit and 64-bit) .dts patches:
Acked-by: Martin Blumenstingl<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 3 +--
> arch/arm64/boot/dts/amlogic/meson-gxl.dtsi | 3 +--
> 4 files changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
> index b98d44fde6b6..f93d6cf6e094 100644
> --- a/arch/arm/boot/dts/meson8.dtsi
> +++ b/arch/arm/boot/dts/meson8.dtsi
> @@ -289,9 +289,8 @@
> &saradc {
> compatible = "amlogic,meson8-saradc", "amlogic,meson-saradc";
> clocks = <&clkc CLKID_XTAL>,
> - <&clkc CLKID_SAR_ADC>,
> - <&clkc CLKID_SANA>;
> - clock-names = "clkin", "core", "sana";
> + <&clkc CLKID_SAR_ADC>;
> + clock-names = "clkin", "core";
> };
>
> &spifc {
> diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
> index bc278da7df0d..4aa444284f0c 100644
> --- a/arch/arm/boot/dts/meson8b.dtsi
> +++ b/arch/arm/boot/dts/meson8b.dtsi
> @@ -185,9 +185,8 @@
> &saradc {
> compatible = "amlogic,meson8b-saradc", "amlogic,meson-saradc";
> clocks = <&clkc CLKID_XTAL>,
> - <&clkc CLKID_SAR_ADC>,
> - <&clkc CLKID_SANA>;
> - clock-names = "clkin", "core", "sana";
> + <&clkc CLKID_SAR_ADC>;
> + clock-names = "clkin", "core";
> };
>
> &uart_AO {
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> index af834cdbba79..b77f2593cdc3 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
> @@ -686,10 +686,9 @@
> compatible = "amlogic,meson-gxbb-saradc", "amlogic,meson-saradc";
> clocks = <&xtal>,
> <&clkc CLKID_SAR_ADC>,
> - <&clkc CLKID_SANA>,
> <&clkc CLKID_SAR_ADC_CLK>,
> <&clkc CLKID_SAR_ADC_SEL>;
> - clock-names = "clkin", "core", "sana", "adc_clk", "adc_sel";
> + clock-names = "clkin", "core", "adc_clk", "adc_sel";
> };
>
> &sd_emmc_a {
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> index d8dd3298b15c..07805a3b4db0 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
> @@ -628,10 +628,9 @@
> compatible = "amlogic,meson-gxl-saradc", "amlogic,meson-saradc";
> clocks = <&xtal>,
> <&clkc CLKID_SAR_ADC>,
> - <&clkc CLKID_SANA>,
> <&clkc CLKID_SAR_ADC_CLK>,
> <&clkc CLKID_SAR_ADC_SEL>;
> - clock-names = "clkin", "core", "sana", "adc_clk", "adc_sel";
> + clock-names = "clkin", "core", "adc_clk", "adc_sel";
> };
>
> &sd_emmc_a {
> --
> 2.14.1
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 0/4] fix the clock setting for SAR ADC
From: Martin Blumenstingl @ 2017-11-12 1:35 UTC (permalink / raw)
To: Yixun Lan
Cc: Neil Armstrong, Jerome Brunet, devicetree-u79uwXL29TY76Z2rM5mHXA,
Jonathan Cameron, Rob Herring, Mark Rutland, Michael Turquette,
Stephen Boyd, Carlo Caione, Kevin Hilman, Xingyu Chen,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-clk-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <CAFBinCB=2pb9dZkpSqdVAiumkSCzgaW0oejftjZNP7aYbiQJuw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Yixun,
On Tue, Nov 7, 2017 at 10:36 PM, Martin Blumenstingl
<martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org> wrote:
> Hi Yixun,
>
> On Tue, Nov 7, 2017 at 3:09 PM, Yixun Lan <yixun.lan-LpR1jeaWuhtBDgjK7y7TUQ@public.gmane.org> wrote:
>> patch [1/4]:
>> Fix wrong SARADC/SANA clock gate bit in Meson-GXBB/GXL,
>> the published datasheets[4] also has wrong description about this.
>> This patch should be explicitly merged *before* other patches.
>>
>> patch [2-4/4]:
>> Drop the "sana" clock from SAR ADC module,
> I agree with Jerome that patch 2/4 should be applied last.
> when I wrote the driver I couldn't get it to work on my GXBB board
> (which unfortunately has died since then) because the clocks were
> disabled (they weren't enabled by the bootloader). people who are
> using an old .dtb would get the same problem again until the clock
> driver is merged
>
>> From the hardware perspective, the SAR ADC module doesn't
>> require "sana" clock to wrok. This should apply to all SoC,
>> including meson6,8, GXBB, GXL..
> thank you for clarifying this!
>
>> Note: the whole patchset series has been tested at GXL-P212 board,
>> we haven't got any meson6,8 board to test, so I would appreciate
>> if someone (Martin?) could help to confirm it works there.
> I can test this on a Meson8b and a Meson8m2 board on the weekend -
> I'll let you know about the results
the ADC still works fine with this series applied on my Meson8m2 board
as well as my Meson8b EC-100
I added my "Reviewed-by" and "Acked-by" to the corresponding patches
(with a change request on the .dts patch)
Thank you for fixing this!
Regards
Martin
^ permalink raw reply
* et8ek8: Document support for flash and lens devices
From: Pavel Machek @ 2017-11-12 11:27 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, niklas.soderlund, maxime.ripard, hverkuil,
laurent.pinchart, sre, linux-acpi, devicetree
In-Reply-To: <20171026075342.5760-30-sakari.ailus@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 872 bytes --]
Document dts support of LED/focus.
Signed-off-by: Pavel Machek <pavel@ucw.cz>
diff --git a/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt b/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
index 0b7b6a4..e80d589 100644
--- a/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
+++ b/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
@@ -20,6 +20,13 @@ Mandatory properties
is in hardware standby mode when the signal is in the low state.
+Optional properties
+-------------------
+
+- flash-leds: See ../video-interfaces.txt
+- lens-focus: See ../video-interfaces.txt
+
+
Endpoint node mandatory properties
----------------------------------
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply related
* Re: [PATCH 1/3] Input: twl4030-vibra: fix sibling-node lookup
From: Johan Hovold @ 2017-11-12 12:12 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Johan Hovold, linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, stable, Peter Ujfalusi,
Marek Belisko, Lee Jones, Rob Herring,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171111175059.lwfhw2wdhlj5yxhc@dtor-ws>
[ +CC: Lee, Rob and device-tree list ]
On Sat, Nov 11, 2017 at 09:50:59AM -0800, Dmitry Torokhov wrote:
> On Sat, Nov 11, 2017 at 04:43:37PM +0100, Johan Hovold wrote:
> > A helper purported to look up a child node based on its name was using
> > the wrong of-helper and ended up prematurely freeing the parent of-node
> > while searching the whole device tree depth-first starting at the parent
> > node.
>
> Ugh, this all is pretty ugly business. Can we teach MFD to allow
> specifying firmware node to be attached to the platform devices it
> creates in mfd_add_device() so that the leaf drivers simply call
> device_property_read_XXX() on their own device and not be bothered with
> weird OF refcount issues or what node they need to locate and parse?
Yeah, that may have helped. You can actually specify a compatible string
in struct mfd_cell today which does make mfd_add_device() associate a
matching child node.
Some best practice regarding how to deal with MFD and device tree would
be good to determine and document too. For example, when should
of_platform_populate() be used in favour of mfd_add_device()?
And how best to deal with sibling nodes, which is part of the problem
here (I think the mfd should have provided a flag rather than having
subdrivers deal with sibling nodes, for example).
That said, driver authors using the wrong of-helper could possibly have
been avoided by amending the kernel docs (I'll do that as a follow up),
but once these incorrect usages get in, only review can prevent them
from being reproduced through copy-paste coding.
Johan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx6: Remove unneeded address-cells/size-cells
From: Fabio Estevam @ 2017-11-12 12:14 UTC (permalink / raw)
To: Christopher Spinrath
Cc: Shawn Guo, Sascha Hauer, Rob Herring, Mark Rutland, Russell King,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <62f3cb01-ff85-0fe0-2afa-0fe4732a7421-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org>
Hi Christopher,
On Sat, Nov 11, 2017 at 12:24 PM, Christopher Spinrath
<christopher.spinrath-vA1bhqPz9FBZXbeN9DUtxg@public.gmane.org> wrote:
> This results in a regression for those devices where U-Boot is supposed
> to inject the partition sub-nodes. For example, this is the case for
> cm-fx6 module based devices like the Utilite Pro.
>
> Linux fails to parse the partitions and reports:
> [ 9.330881] spi0.0: error parsing ofpart partition
> /soc/aips-bus@2000000/spba-bus@2000000/ecspi@2008000/m25p80@0/partition@0
> (/soc/aips-bus@2000000/spba-bus@2000000/ecspi@2008000/m25p80@0)
> [ 9.330887] mtd: failed to find partitions; one or more parsers
> reports errors (-22)
>
> Also, the documentation does not forbid the #address-cells/size-cells if
> no partitions are present.
>
> So, please, drop this patch (or exclude at least the cm-fx6 related change).
Good point! I will resend the patch and will only change the NXP boards.
Thanks
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] ARM: dts: sun8i: add support for Orange Pi R1
From: Icenowy Zheng @ 2017-11-12 12:41 UTC (permalink / raw)
To: Maxime Ripard, Chen-Yu Tsai
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Icenowy Zheng
Orange Pi R1 is a board design based on Orange Pi Zero, with XR819 Wi-Fi
chip replaced by RTL8189ETV Wi-Fi module and the USB Type-A jack
replaced by an onboard USB RTL8152B USB-Ethernet adapter.
Add support for it.
Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
---
arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/sun8i-h2-plus-orangepi-r1.dts | 73 +++++++++++++++++++++++++
2 files changed, 74 insertions(+)
create mode 100644 arch/arm/boot/dts/sun8i-h2-plus-orangepi-r1.dts
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index d0381e9caf21..3c139c63098f 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -949,6 +949,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
sun8i-a83t-bananapi-m3.dtb \
sun8i-a83t-cubietruck-plus.dtb \
sun8i-a83t-tbs-a711.dtb \
+ sun8i-h2-plus-orangepi-r1.dtb \
sun8i-h2-plus-orangepi-zero.dtb \
sun8i-h3-bananapi-m2-plus.dtb \
sun8i-h3-beelink-x2.dtb \
diff --git a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-r1.dts b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-r1.dts
new file mode 100644
index 000000000000..32a6d312422e
--- /dev/null
+++ b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-r1.dts
@@ -0,0 +1,73 @@
+/*
+ * Copyright (C) 2017 Icenowy Zheng <icenowy-ymACFijhrKM@public.gmane.org>
+ *
+ * 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.
+ */
+
+/* Orange Pi R1 is based on Orange Pi Zero design */
+#include "sun8i-h2-plus-orangepi-zero.dts"
+
+/ {
+ model = "Xunlong Orange Pi R1";
+ compatible = "xunlong,orangepi-r1", "allwinner,sun8i-h2-plus";
+
+ /delete-node/ reg_vcc_wifi;
+
+ aliases {
+ ethernet1 = &rtl8189etv;
+ };
+};
+
+&ohci1 {
+ /*
+ * RTL8152B USB-Ethernet adapter is connected to USB1,
+ * and it's a USB 2.0 device. So the OHCI1 controller
+ * can be left disabled.
+ */
+ status = "disabled";
+};
+
+&mmc1 {
+ vmmc-supply = <®_vcc3v3>;
+ vqmmc-supply = <®_vcc3v3>;
+
+ rtl8189etv: sdio_wifi@1 {
+ reg = <1>;
+ };
+};
--
2.14.2
^ permalink raw reply related
* [PATCH v2] ARM: sun8i: h2+: add support for Banana Pi M2 Zero board
From: Icenowy Zheng @ 2017-11-12 13:27 UTC (permalink / raw)
To: Maxime Ripard, Chen-Yu Tsai
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-sunxi-/JYPxA39Uh5TLH3MbocFFw, Icenowy Zheng
Banana Pi M2 Zero board is a H2+-based board by Sinovoip, with a form
factor and GPIO holes similar to Raspberry Pi Zero.
It features:
- Allwinner H2+ SoC
- Single-chip (16-bit) 512MiB DDR3 DRAM
- Ampak AP6212 Wi-Fi/Bluetooth module
- MicroSD slot
- Two MicroUSB Type-B ports (one can only be used to power the board and
the other features OTG functionality)
- Two keys, a reset and a GPIO-connected key.
- HDMI Type-C (miniHDMI) connector connected to the HDMI part of H2+.
- CSI connector to connect the camera sensor provided by Sinovoip.
Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
---
Changes in v2:
- Use high active SD card detect on the production batch.
arch/arm/boot/dts/Makefile | 1 +
.../boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts | 157 +++++++++++++++++++++
2 files changed, 158 insertions(+)
create mode 100644 arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index d0381e9caf21..acf449e731dc 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -949,6 +949,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
sun8i-a83t-bananapi-m3.dtb \
sun8i-a83t-cubietruck-plus.dtb \
sun8i-a83t-tbs-a711.dtb \
+ sun8i-h2-plus-bananapi-m2-zero.dtb \
sun8i-h2-plus-orangepi-zero.dtb \
sun8i-h3-bananapi-m2-plus.dtb \
sun8i-h3-beelink-x2.dtb \
diff --git a/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
new file mode 100644
index 000000000000..c971e954d691
--- /dev/null
+++ b/arch/arm/boot/dts/sun8i-h2-plus-bananapi-m2-zero.dts
@@ -0,0 +1,157 @@
+/*
+ * Copyright (C) 2017 Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
+ *
+ * Based on sun8i-h3-bananapi-m2-plus.dts, which is:
+ * Copyright (C) 2016 Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org>
+ *
+ * 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 "sun8i-h3.dtsi"
+#include "sunxi-common-regulators.dtsi"
+
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
+
+/ {
+ model = "Banana Pi BPI-M2-Zero";
+ compatible = "sinovoip,bpi-m2-zero", "allwinner,sun8i-h2-plus";
+
+ aliases {
+ serial0 = &uart0;
+ serial1 = &uart1;
+ };
+
+ chosen {
+ stdout-path = "serial0:115200n8";
+ };
+
+ leds {
+ compatible = "gpio-leds";
+ pinctrl-names = "default";
+
+ pwr_led {
+ label = "bananapi-m2-zero:red:pwr";
+ gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>; /* PL10 */
+ default-state = "on";
+ };
+ };
+
+ gpio_keys {
+ compatible = "gpio-keys";
+ pinctrl-names = "default";
+
+ sw4 {
+ label = "power";
+ linux,code = <BTN_0>;
+ gpios = <&r_pio 0 3 GPIO_ACTIVE_LOW>;
+ };
+ };
+
+ wifi_pwrseq: wifi_pwrseq {
+ compatible = "mmc-pwrseq-simple";
+ pinctrl-names = "default";
+ reset-gpios = <&r_pio 0 7 GPIO_ACTIVE_LOW>; /* PL7 */
+ };
+};
+
+&ehci0 {
+ status = "okay";
+};
+
+&mmc0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&mmc0_pins_a>;
+ vmmc-supply = <®_vcc3v3>;
+ bus-width = <4>;
+ /*
+ * On the production batch of this board the card detect GPIO is
+ * high active (card inserted), although on the early samples it's
+ * low active.
+ */
+ cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
+ status = "okay";
+};
+
+&mmc1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&mmc1_pins_a>;
+ vmmc-supply = <®_vcc3v3>;
+ vqmmc-supply = <®_vcc3v3>;
+ mmc-pwrseq = <&wifi_pwrseq>;
+ bus-width = <4>;
+ non-removable;
+ status = "okay";
+
+ brcmf: wifi@1 {
+ reg = <1>;
+ compatible = "brcm,bcm4329-fmac";
+ interrupt-parent = <&pio>;
+ interrupts = <6 10 IRQ_TYPE_LEVEL_LOW>; /* PG10 / EINT10 */
+ interrupt-names = "host-wake";
+ };
+};
+
+&ohci0 {
+ status = "okay";
+};
+
+&uart0 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart0_pins_a>;
+ status = "okay";
+};
+
+&uart1 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart1_pins>, <&uart1_rts_cts_pins>;
+ status = "okay";
+};
+
+&usb_otg {
+ dr_mode = "otg";
+ status = "okay";
+};
+
+&usbphy {
+ usb0_id_det-gpios = <&r_pio 0 6 GPIO_ACTIVE_HIGH>; /* PL6 */
+ /* USB OTG VBUS is directly connected to 5V without any regulators */
+ status = "okay";
+};
--
2.14.2
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: et8ek8: Document support for flash and lens devices
From: Sebastian Reichel @ 2017-11-12 14:25 UTC (permalink / raw)
To: Pavel Machek
Cc: Sakari Ailus, linux-media-u79uwXL29TY76Z2rM5mHXA,
niklas.soderlund-1zkq55x86MTxsAP9Fp7wbw,
maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8,
hverkuil-qWit8jRvyhVmR6Xm/wNWPw,
laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20171112112729.GA21247@amd>
[-- Attachment #1: Type: text/plain, Size: 1128 bytes --]
Hi,
On Sun, Nov 12, 2017 at 12:27:29PM +0100, Pavel Machek wrote:
>
> Document dts support of LED/focus.
>
> Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
Reviewed-by: Sebastian Reichel <sebastian.reichel-ZGY8ohtN/8pPYcu2f3hruQ@public.gmane.org>
-- Sebastian
> diff --git a/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt b/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
> index 0b7b6a4..e80d589 100644
> --- a/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
> +++ b/Documentation/devicetree/bindings/media/i2c/toshiba,et8ek8.txt
> @@ -20,6 +20,13 @@ Mandatory properties
> is in hardware standby mode when the signal is in the low state.
>
>
> +Optional properties
> +-------------------
> +
> +- flash-leds: See ../video-interfaces.txt
> +- lens-focus: See ../video-interfaces.txt
> +
> +
> Endpoint node mandatory properties
> ----------------------------------
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v4 3/5] staging: Introduce NVIDIA Tegra video decoder driver
From: Dmitry Osipenko @ 2017-11-12 16:17 UTC (permalink / raw)
To: Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
Greg Kroah-Hartman, Rob Herring, Mauro Carvalho Chehab,
Stephen Warren
Cc: devel, devicetree, linux-kernel, linux-tegra, Dan Carpenter,
linux-media
In-Reply-To: <2c2910bc-40d4-b4ac-cdbe-b3c670a91f1b@mleia.com>
On 11.11.2017 17:06, Vladimir Zapolskiy wrote:
> Hi Dmitry,
>
> I'll add just a couple of minor comments, in general the code looks
> very good.
>
Thank you very much for the review!
> On 10/20/2017 12:34 AM, Dmitry Osipenko wrote:
>> NVIDIA Tegra20/30/114/124/132 SoC's have video decoder engine that
>> supports standard set of video formats like H.264 / MPEG-4 / WMV / VC1.
>> Currently implemented decoding of CAVLC H.264 on Tegra20 only.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>
> [snip]
>
>> +++ b/drivers/staging/tegra-vde/uapi.h
>> @@ -0,0 +1,101 @@
>> +/*
>> + * Copyright (C) 2016-2017 Dmitry Osipenko <digetx@gmail.com>
>> + * All Rights Reserved.
>> + *
>> + * 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 (including the next
>> + * paragraph) 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
>> + * VA LINUX SYSTEMS AND/OR ITS SUPPLIERS 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.
>
> From the specified MODULE_LICENSE("GPL") I'd rather expect to see a reference
> to GPLv2+ license in the header, and here the text resembles MIT license only.
>
> I understand that it is a UAPI header file and it may happen that different
> rules are applied to this kind of sources, hopefully Greg can give the right
> directions.
Indeed, probably I copied the license text from some other UAPI header without
putting much thought. Will change it to GPL, thanks.
>
> In general you may avoid the headache with the custom UAPI, if you reuse
> V4L2 interfaces, if I remember correctly drivers/media/platform/coda does it.
> Also from my point of view the custom UAPI is the only reason why the driver
> is pushed to the staging folder.
Thanks for the pointer. I see that coda driver does some raw bitstream parsing
in the driver, which is a bit icky, but probably is a good enough variant. I'll
take a closer look at implementing V4L interface at some point later, meanwhile
custom UAPI + VDPAU userspace serves us pretty well.
>
> [snip]
>
>> +struct tegra_vde {
>> + void __iomem *sxe;
>> + void __iomem *bsev;
>> + void __iomem *mbe;
>> + void __iomem *ppe;
>> + void __iomem *mce;
>> + void __iomem *tfe;
>> + void __iomem *ppb;
>> + void __iomem *vdma;
>> + void __iomem *frameid;
>
> Please find a comment in tegra_vde_probe() function regarding
> devm_ioremap_resource() calls.
>
>> + struct mutex lock;
>> + struct miscdevice miscdev;
>> + struct reset_control *rst;
>> + struct gen_pool *iram_pool;
>> + struct completion decode_completion;
>> + struct clk *clk;
>> + dma_addr_t iram_lists_addr;
>> + u32 *iram;
>> +};
>
> [snip]
>
>> +static int tegra_vde_wait_bsev(struct tegra_vde *vde, bool wait_dma)
>> +{
>> + struct device *dev = vde->miscdev.parent;
>> + u32 value;
>> + int err;
>> +
>> + err = readl_relaxed_poll_timeout(vde->bsev + INTR_STATUS, value,
>> + !(value & BIT(2)), 1, 100);
>> + if (err) {
>> + dev_err(dev, "BSEV unknown bit timeout\n");
>> + return err;
>> + }
>> +
>> + err = readl_relaxed_poll_timeout(vde->bsev + INTR_STATUS, value,
>> + (value & BSE_ICMDQUE_EMPTY), 1, 100);
>> + if (err) {
>> + dev_err(dev, "BSEV ICMDQUE flush timeout\n");
>> + return err;
>> + }
>> +
>> + if (!wait_dma)
>> + return 0;
>> +
>> + err = readl_relaxed_poll_timeout(vde->bsev + INTR_STATUS, value,
>> + !(value & BSE_DMA_BUSY), 1, 100);
>> + if (err) {
>> + dev_err(dev, "BSEV DMA timeout\n");
>> + return err;
>> + }
>> +
>> + return 0;
>
> if (err)
> dev_err(dev, "BSEV DMA timeout\n");
>
> return err;
>
> is two lines shorter.
>
Thanks for the suggestion. I think that consistency in the code is a bit more
preferable, so I'd prefer to stick with the original variant here.
>> +}
>> +
>
> [snip]
>
>> +static int tegra_vde_attach_dmabufs_to_frame(struct device *dev,
>> + struct video_frame *frame,
>> + struct tegra_vde_h264_frame *source,
>> + enum dma_data_direction dma_dir,
>> + bool baseline_profile,
>> + size_t csize)
>> +{
>> + int err;
>> +
>> + err = tegra_vde_attach_dmabuf(dev, source->y_fd,
>> + source->y_offset, csize * 4,
>> + &frame->y_dmabuf_attachment,
>> + &frame->y_addr,
>> + &frame->y_sgt,
>> + NULL, dma_dir);
>> + if (err)
>> + return err;
>> +
>> + err = tegra_vde_attach_dmabuf(dev, source->cb_fd,
>> + source->cb_offset, csize,
>> + &frame->cb_dmabuf_attachment,
>> + &frame->cb_addr,
>> + &frame->cb_sgt,
>> + NULL, dma_dir);
>> + if (err)
>> + goto err_release_y;
>> +
>> + err = tegra_vde_attach_dmabuf(dev, source->cr_fd,
>> + source->cr_offset, csize,
>> + &frame->cr_dmabuf_attachment,
>> + &frame->cr_addr,
>> + &frame->cr_sgt,
>> + NULL, dma_dir);
>> + if (err)
>> + goto err_release_cb;
>> +
>> + if (baseline_profile) {
>> + frame->aux_addr = 0xF4DEAD00;
>> + } else {
>
> I would rather suggest to do
>
> if (baseline_profile) {
> frame->aux_addr = 0xF4DEAD00;
> return 0;
> }
>
> and remove indentation from the code block below.
>
Good suggestion, thanks.
>> + err = tegra_vde_attach_dmabuf(dev, source->aux_fd,
>> + source->aux_offset, csize,
>> + &frame->aux_dmabuf_attachment,
>> + &frame->aux_addr,
>> + &frame->aux_sgt,
>> + NULL, dma_dir);
>> + if (err)
>> + goto err_release_cr;
>> + }
>> +
>> + return 0;
>
> if (!err)
> return 0;
>
> and then remove a check above.
>
I'd prefer to keep 'goto' for the consistency.
>> +
>> +err_release_cr:
>> + tegra_vde_detach_and_put_dmabuf(frame->cr_dmabuf_attachment,
>> + frame->cr_sgt, dma_dir);
>> +err_release_cb:
>> + tegra_vde_detach_and_put_dmabuf(frame->cb_dmabuf_attachment,
>> + frame->cb_sgt, dma_dir);
>> +err_release_y:
>> + tegra_vde_detach_and_put_dmabuf(frame->y_dmabuf_attachment,
>> + frame->y_sgt, dma_dir);
>> +
>> + return err;
>> +}
>
> [snip]
>
>> +static int tegra_vde_ioctl_decode_h264(struct tegra_vde *vde,
>> + unsigned long vaddr)
>> +{
>> + struct device *dev = vde->miscdev.parent;
>> + struct tegra_vde_h264_decoder_ctx ctx;
>> + struct tegra_vde_h264_frame __user *frames_user;
>> + struct tegra_vde_h264_frame *frames;
>> + struct video_frame *dpb_frames;
>> + struct dma_buf_attachment *bitstream_data_dmabuf_attachment;
>> + struct sg_table *bitstream_sgt;
>> + enum dma_data_direction dma_dir;
>> + dma_addr_t bitstream_data_addr;
>> + dma_addr_t bsev_ptr;
>> + size_t bitstream_data_size;
>> + unsigned int macroblocks_nb;
>> + unsigned int read_bytes;
>> + unsigned int i = 0;
>> + bool timeout;
>> + int ret;
>> +
>> + if (copy_from_user(&ctx, (void __user *)vaddr, sizeof(ctx)))
>> + return -EFAULT;
>> +
>> + ret = tegra_vde_validate_h264_ctx(dev, &ctx);
>> + if (ret)
>> + return -EINVAL;
>
> Or "return ret".
>
Good catch.
>> +
>> + ret = tegra_vde_attach_dmabuf(dev, ctx.bitstream_data_fd,
>> + ctx.bitstream_data_offset, 0,
>> + &bitstream_data_dmabuf_attachment,
>> + &bitstream_data_addr,
>> + &bitstream_sgt,
>> + &bitstream_data_size,
>> + DMA_TO_DEVICE);
>> + if (ret)
>> + return ret;
>> +
>> + dpb_frames = kcalloc(ctx.dpb_frames_nb, sizeof(*dpb_frames),
>> + GFP_KERNEL);
>> + if (!dpb_frames) {
>> + ret = -ENOMEM;
>> + goto err_release_bitstream_dmabuf;
>> + }
>> +
>> + macroblocks_nb = ctx.pic_width_in_mbs * ctx.pic_height_in_mbs;
>> + frames_user = u64_to_user_ptr(ctx.dpb_frames_ptr);
>> +
>> + frames = kmalloc_array(ctx.dpb_frames_nb, sizeof(*frames), GFP_KERNEL);
>> + if (!frames) {
>> + ret = -ENOMEM;
>> + goto err_release_dpb_frames;
>> + }
>> +
>> + if (copy_from_user(frames, frames_user,
>> + ctx.dpb_frames_nb * sizeof(*frames))) {
>> + ret = -EFAULT;
>> + goto free_frames;
>
> It could be
>
> ret = -EFAULT;
> kfree(frames);
> goto err_release_dpb_frames;
>
That indeed should be a bit cleaner, thanks. I'll also add/use a new 'goto
err_free_dpb_frames'.
>> + }
>> +
>> + for (i = 0; i < ctx.dpb_frames_nb; i++) {
>> + ret = tegra_vde_validate_frame(dev, &frames[i]);
>> + if (ret)
>> + goto free_frames;
>
> It could be
>
> if (ret)
> break;
>
> or
> if (ret) {
> kfree(frames);
> goto err_release_dpb_frames;
> }
>
>> +
>> + dpb_frames[i].flags = frames[i].flags;
>> + dpb_frames[i].frame_num = frames[i].frame_num;
>> +
>> + dma_dir = (i == 0) ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
>> +
>> + ret = tegra_vde_attach_dmabufs_to_frame(dev, &dpb_frames[i],
>> + &frames[i], dma_dir,
>> + ctx.baseline_profile,
>> + macroblocks_nb * 64);
>> + if (ret)
>> + goto free_frames;
>
> It could be
>
> if (ret)
> break;
>
> or
>
> if (ret) {
> kfree(frames);
> goto err_release_dpb_frames;
> }
>
> I don't strictly ask for the changes, but in my opinion less indentation
> is better.
>
>> + }
>> +
>> +free_frames:
>> + kfree(frames);
>> +
>> + if (ret)
>> + goto err_release_dpb_frames;
>
> If you select an option with 'kfree' in all three cases above, then
> it is possible to remove 'kfree' here, remove the check and remove
> a goto label.
>
> It's up to you to decide, which version is better.
>
The 'break' looks like a good variant, thanks. But we need to keep kfree(frames)
after the loop anyway because 'frames' is a temporary allocation and it isn't
free'd later.
> [snip]
>
>> +
>> +static int tegra_vde_runtime_resume(struct device *dev)
>> +{
>> + struct tegra_vde *vde = dev_get_drvdata(dev);
>> + int err;
>> +
>> + err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_VDEC,
>> + vde->clk, vde->rst);
>> + if (err) {
>> + dev_err(dev, "Failed to power up HW : %d\n", err);
>> + return err;
>> + }
>> +
>> + return 0;
>
> if (err)
> dev_err(dev, "Failed to power up HW : %d\n", err);
>
> return err;
>
> saves 2 lines of code.
>
Yeah, but I'd prefer to keep up consistency.
>> +}
>> +
>> +static int tegra_vde_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct resource *regs;
>> + struct tegra_vde *vde;
>> + int irq, err;
>> +
>> + vde = devm_kzalloc(dev, sizeof(*vde), GFP_KERNEL);
>> + if (!vde)
>> + return -ENOMEM;
>> +
>> + platform_set_drvdata(pdev, vde);
>> +
>> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sxe");
>> + if (!regs)
>> + return -ENODEV;
>> +
>> + vde->sxe = devm_ioremap_resource(dev, regs);
>> + if (IS_ERR(vde->sxe))
>> + return PTR_ERR(vde->sxe);
>> +
>> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "bsev");
>> + if (!regs)
>> + return -ENODEV;
>> +
>> + vde->bsev = devm_ioremap_resource(dev, regs);
>> + if (IS_ERR(vde->bsev))
>> + return PTR_ERR(vde->bsev);
>> +
>> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mbe");
>> + if (!regs)
>> + return -ENODEV;
>> +
>> + vde->mbe = devm_ioremap_resource(dev, regs);
>> + if (IS_ERR(vde->mbe))
>> + return PTR_ERR(vde->mbe);
>> +
>> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ppe");
>> + if (!regs)
>> + return -ENODEV;
>> +
>> + vde->ppe = devm_ioremap_resource(dev, regs);
>> + if (IS_ERR(vde->ppe))
>> + return PTR_ERR(vde->ppe);
>> +
>> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mce");
>> + if (!regs)
>> + return -ENODEV;
>> +
>> + vde->mce = devm_ioremap_resource(dev, regs);
>> + if (IS_ERR(vde->mce))
>> + return PTR_ERR(vde->mce);
>> +
>> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tfe");
>> + if (!regs)
>> + return -ENODEV;
>> +
>> + vde->tfe = devm_ioremap_resource(dev, regs);
>> + if (IS_ERR(vde->tfe))
>> + return PTR_ERR(vde->tfe);
>> +
>> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ppb");
>> + if (!regs)
>> + return -ENODEV;
>> +
>> + vde->ppb = devm_ioremap_resource(dev, regs);
>> + if (IS_ERR(vde->ppb))
>> + return PTR_ERR(vde->ppb);
>> +
>> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "vdma");
>> + if (!regs)
>> + return -ENODEV;
>> +
>> + vde->vdma = devm_ioremap_resource(dev, regs);
>> + if (IS_ERR(vde->vdma))
>> + return PTR_ERR(vde->vdma);
>> +
>> + regs = platform_get_resource_byname(pdev, IORESOURCE_MEM, "frameid");
>> + if (!regs)
>> + return -ENODEV;
>> +
>> + vde->frameid = devm_ioremap_resource(dev, regs);
>> + if (IS_ERR(vde->frameid))
>> + return PTR_ERR(vde->frameid);
>
> This is just my personal opinion on which I do not insist, however I'd like
> to share it, a single memory region described in DTB (and the same single
> region mapped once) and subregion offsets in the driver may be more preferable.
>
> If I remember correctly that was found in the previous versions of the driver,
> the new scheme might be preferred only if the offsets are different in different
> Tegra VDE IPs, but even in the latter case it might be better to handle the
> difference inside the driver.
>
I've very briefly mentioned that change in the cover-letter, the problem is that
BSEA (bitstream audio) and UCQ (unified commands queue) registers are placed in
between the VDE registers. In the reference manual, the VDE BSEA and UCQ are
drawn as a distinct blocks on the Block Diagram, I presume that VDE driver
shouldn't cover BSEA and UCQ. But if it will turn out that it should, I think it
wouldn't be a problem to make a unified MFD driver that covers all IP blocks.
>> +
>> + vde->clk = devm_clk_get(dev, NULL);
>> + if (IS_ERR(vde->clk)) {
>> + err = PTR_ERR(vde->clk);
>> + dev_err(dev, "Could not get VDE clk %d\n", err);
>> + return err;
>> + }
>> +
>> + vde->rst = devm_reset_control_get(dev, NULL);
>> + if (IS_ERR(vde->rst)) {
>> + err = PTR_ERR(vde->rst);
>> + dev_err(dev, "Could not get VDE reset %d\n", err);
>> + return err;
>> + }
>> +
>> + irq = platform_get_irq_byname(pdev, "sync-token");
>> + if (irq < 0)
>> + return irq;
>> +
>> + err = devm_request_irq(dev, irq, tegra_vde_isr, 0,
>> + dev_name(dev), vde);
>> + if (err) {
>> + dev_err(dev, "Could not request IRQ %d\n", err);
>> + return err;
>> + }
>> +
>> + vde->iram_pool = of_gen_pool_get(dev->of_node, "iram", 0);
>> + if (!vde->iram_pool) {
>> + dev_err(dev, "Could not get IRAM pool\n");
>> + return -EPROBE_DEFER;
>> + }
>> +
>> + vde->iram = gen_pool_dma_alloc(vde->iram_pool, 0x3FC00,
>
> The size of the pool should not be hardcoded, it is expected to be
> obtained from the device tree:
>
> gen_pool_size(vde->iram_pool)
>
Indeed, thanks.
>> + &vde->iram_lists_addr);
>> + if (!vde->iram) {
>> + dev_err(dev, "Could not reserve IRAM\n");
>> + return -ENOMEM;
>> + }
>> +
>> + mutex_init(&vde->lock);
>> + init_completion(&vde->decode_completion);
>> +
>> + vde->miscdev.minor = MISC_DYNAMIC_MINOR;
>> + vde->miscdev.name = "tegra_vde";
>> + vde->miscdev.fops = &tegra_vde_fops;
>> + vde->miscdev.parent = dev;
>> +
>> + err = misc_register(&vde->miscdev);
>> + if (err) {
>> + dev_err(dev, "Failed to register misc device: %d\n", err);
>> + goto err_gen_free;
>> + }
>> +
>> + pm_runtime_enable(dev);
>> + pm_runtime_use_autosuspend(dev);
>> + pm_runtime_set_autosuspend_delay(dev, 300);
>> +
>> + if (!pm_runtime_enabled(dev)) {
>> + err = tegra_vde_runtime_resume(dev);
>> + if (err)
>> + goto err_misc_unreg;
>> + }
>> +
>> + return 0;
>> +
>> +err_misc_unreg:
>> + misc_deregister(&vde->miscdev);
>> +
>> +err_gen_free:
>> + gen_pool_free(vde->iram_pool,
>> + (unsigned long)vde->iram, 0x3FC00);
>
> See a comment above.
>
>> +
>> + return err;
>> +}
>> +
>> +static int tegra_vde_remove(struct platform_device *pdev)
>> +{
>> + struct tegra_vde *vde = platform_get_drvdata(pdev);
>> + struct device *dev = &pdev->dev;
>> + int err;
>> +
>> + if (!pm_runtime_enabled(dev)) {
>> + err = tegra_vde_runtime_suspend(dev);
>> + if (err)
>> + return err;
>> + }
>> +
>> + pm_runtime_dont_use_autosuspend(dev);
>> + pm_runtime_disable(dev);
>> +
>> + misc_deregister(&vde->miscdev);
>> +
>> + gen_pool_free(vde->iram_pool,
>> + (unsigned long)vde->iram, 0x3FC00);
>
> See a comment above.
>
>> +
>> + return 0;
>> +}
>> +
>
> [snip]
>
>> +
>> +MODULE_DESCRIPTION("NVIDIA Tegra20 Video Decoder driver");
>> +MODULE_AUTHOR("Dmitry Osipenko");
>
> Please add your email here, then if you are not a maintainer (and by
> the way please consider to add your name to the MAINTAINERS file),
> you'll get a higher chance to receive updates in the future.
>
Thank you for the suggestion, I'll add the email.
>> +MODULE_LICENSE("GPL");
>>
^ permalink raw reply
* Re: [PATCH v4 2/5] media: dt: bindings: Add binding for NVIDIA Tegra Video Decoder Engine
From: Dmitry Osipenko @ 2017-11-12 16:19 UTC (permalink / raw)
To: Vladimir Zapolskiy, Thierry Reding, Jonathan Hunter,
Greg Kroah-Hartman, Rob Herring, Mauro Carvalho Chehab,
Stephen Warren
Cc: Dan Carpenter, linux-media-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <6492d1af-19fa-253f-2b75-2c37ccd44cbe-ChpfBGZJDbMAvxtiuMwx3w@public.gmane.org>
On 11.11.2017 17:21, Vladimir Zapolskiy wrote:
> Hi Dmitry,
>
> On 10/20/2017 12:34 AM, Dmitry Osipenko wrote:
>> Add binding documentation for the Video Decoder Engine which is found
>> on NVIDIA Tegra20/30/114/124/132 SoC's.
>>
>> Signed-off-by: Dmitry Osipenko <digetx-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> .../devicetree/bindings/media/nvidia,tegra-vde.txt | 55 ++++++++++++++++++++++
>> 1 file changed, 55 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt b/Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
>> new file mode 100644
>> index 000000000000..470237ed6fe5
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/nvidia,tegra-vde.txt
>> @@ -0,0 +1,55 @@
>> +NVIDIA Tegra Video Decoder Engine
>> +
>> +Required properties:
>> +- compatible : Must contain one of the following values:
>> + - "nvidia,tegra20-vde"
>> + - "nvidia,tegra30-vde"
>> + - "nvidia,tegra114-vde"
>> + - "nvidia,tegra124-vde"
>> + - "nvidia,tegra132-vde"
>> +- reg : Must contain an entry for each entry in reg-names.
>> +- reg-names : Must include the following entries:
>> + - sxe
>> + - bsev
>> + - mbe
>> + - ppe
>> + - mce
>> + - tfe
>> + - ppb
>> + - vdma
>> + - frameid
>
> I've already mentioned it in my review of the driver code, but the
> version from v3 with a single region is more preferable.
>
> Also it implies that "reg-names" property will be removed.
>
Please see my reply to the drivers code review.
>> +- iram : Must contain phandle to the mmio-sram device node that represents
>> + IRAM region used by VDE.
>> +- interrupts : Must contain an entry for each entry in interrupt-names.
>> +- interrupt-names : Must include the following entries:
>> + - sync-token
>> + - bsev
>> + - sxe
>> +- clocks : Must include the following entries:
>> + - vde
>> +- resets : Must include the following entries:
>> + - vde
>> +
>> +Example:
>> +
>> +video-codec@6001a000 {
>> + compatible = "nvidia,tegra20-vde";
>> + reg = <0x6001a000 0x1000 /* Syntax Engine */
>> + 0x6001b000 0x1000 /* Video Bitstream Engine */
>> + 0x6001c000 0x100 /* Macroblock Engine */
>> + 0x6001c200 0x100 /* Post-processing Engine */
>> + 0x6001c400 0x100 /* Motion Compensation Engine */
>> + 0x6001c600 0x100 /* Transform Engine */
>> + 0x6001c800 0x100 /* Pixel prediction block */
>> + 0x6001ca00 0x100 /* Video DMA */
>> + 0x6001d800 0x300 /* Video frame controls */>;
>> + reg-names = "sxe", "bsev", "mbe", "ppe", "mce",
>> + "tfe", "ppb", "vdma", "frameid";
>> + iram = <&vde_pool>; /* IRAM region */
>> + interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>, /* Sync token interrupt */
>> + <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>, /* BSE-V interrupt */
>> + <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>; /* SXE interrupt */
>> + interrupt-names = "sync-token", "bsev", "sxe";
>> + clocks = <&tegra_car TEGRA20_CLK_VDE>;
>> + resets = <&tegra_car 61>;
>> +};
>>
^ permalink raw reply
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