lkml.org 
[lkml]   [2020]   [Jan]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: Re: [RFC PATCH v1 4/5] media: tegra: Add Tegra Video input driver for Tegra210
    From
    Date

    On 1/29/20 3:13 AM, Thierry Reding wrote:
    > On Tue, Jan 28, 2020 at 10:23:20AM -0800, Sowjanya Komatineni wrote:
    >> diff --git a/drivers/staging/media/tegra/csi.h b/drivers/staging/media/tegra/csi.h
    > [...]
    >> +struct tegra_csi_soc_data {
    > I'd just leave out the _data suffix since it's not useful.
    >
    >> + const struct tegra_csi_fops *csi_fops;
    >> + unsigned int cil_max_clk_hz;
    >> + unsigned int num_tpg_channels;
    >> +};
    >> +
    >> +/**
    >> + * struct tegra_csi_device - NVIDIA Tegra CSI device structure
    >> + * @dev: device struct
    >> + * @client: host1x_client struct
    >> + *
    >> + * @iomem: register base
    >> + * @csi_clk: clock for CSI
    >> + * @cilab_clk: clock for CIL AB
    >> + * @cilcd_clk: clock for CIL CD
    >> + * @cilef_clk: clock for CIL EF
    >> + * @tpg_clk: clock for internal CSI TPG logic
    >> + *
    >> + * @soc_data: pointer to SoC data structure
    >> + * @fops: csi operations
    >> + *
    >> + * @channels: list of CSI channels
    >> + */
    >> +struct tegra_csi_device {
    >> + struct device *dev;
    >> + struct host1x_client client;
    >> +
    >> + void __iomem *iomem;
    >> + struct clk *csi_clk;
    >> + struct clk *cilab_clk;
    >> + struct clk *cilcd_clk;
    >> + struct clk *cilef_clk;
    >> + struct clk *tpg_clk;
    >> + atomic_t clk_refcnt;
    >> +
    >> + const struct tegra_csi_soc_data *soc_data;
    > Same here. No need for the _data suffix, it's just an extra 5 characters
    > that you have to potentially repeat a lot but doesn't add anything.
    >
    >> + const struct tegra_csi_fops *fops;
    >> +
    >> + struct list_head csi_chans;
    >> +};
    >> +
    >> +void tegra_csi_error_status(struct v4l2_subdev *subdev);
    >> +
    >> +#endif
    >> diff --git a/drivers/staging/media/tegra/csi2_fops.c b/drivers/staging/media/tegra/csi2_fops.c
    >> new file mode 100644
    >> index 000000000000..5f2f7bd3ae50
    >> --- /dev/null
    >> +++ b/drivers/staging/media/tegra/csi2_fops.c
    >> @@ -0,0 +1,335 @@
    >> +// SPDX-License-Identifier: GPL-2.0-only
    >> +/*
    >> + * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved.
    >> + */
    >> +
    >> +#include <linux/clk.h>
    >> +#include <linux/clk/tegra.h>
    >> +#include <linux/delay.h>
    >> +
    >> +#include "vi2_registers.h"
    >> +#include "mc_common.h"
    >> +#include "csi2_fops.h"
    >> +#include "csi.h"
    >> +
    >> +/* CSI block registers offset */
    >> +#define TEGRA210_CSI_PORT_OFFSET 0x34
    >> +/* CSI CIL parition registers offset */
    >> +#define TEGRA210_CSI_CIL_OFFSET 0x0f4
    >> +/* CSI TPG registers offset */
    >> +#define TEGRA210_CSI_TPG_OFFSET 0x18c
    >> +
    >> +#define CSI_PP_OFFSET(block) ((block) * 0x800)
    >> +
    >> +static void csi_write(struct tegra_csi_channel *chan, u8 block,
    >> + unsigned int addr, u32 val)
    >> +{
    >> + void __iomem *csi_pp_base;
    >> +
    >> + csi_pp_base = chan->csi->iomem + CSI_PP_OFFSET(block);
    >> + writel(val, csi_pp_base + addr);
    >> +}
    >> +
    >> +/* Pixel parser registers accessors */
    >> +static void pp_write(struct tegra_csi_port *port, u32 addr, u32 val)
    >> +{
    >> + writel(val, port->pixel_parser + addr);
    >> +}
    >> +
    >> +static u32 pp_read(struct tegra_csi_port *port, u32 addr)
    >> +{
    >> + return readl(port->pixel_parser + addr);
    >> +}
    >> +
    >> +/* CSI CIL A/B port registers accessors */
    >> +static void cil_write(struct tegra_csi_port *port, u8 port_id,
    >> + u32 addr, u32 val)
    >> +{
    >> + if (port_id & PORT_B)
    >> + writel(val, port->cilb + addr);
    >> + else
    >> + writel(val, port->cila + addr);
    >> +}
    >> +
    >> +static u32 cil_read(struct tegra_csi_port *port, u8 port_id,
    >> + u32 addr)
    >> +{
    >> + if (port_id & PORT_B)
    >> + return readl(port->cilb + addr);
    >> + else
    >> + return readl(port->cila + addr);
    >> +}
    >> +
    >> +/* Test pattern generator registers accessor */
    >> +static void tpg_write(struct tegra_csi_port *port, unsigned int addr, u32 val)
    >> +{
    >> + writel(val, port->tpg + addr);
    >> +}
    > These register accessors all look a bit convoluted to me. For example,
    > the cil_write()/cil_read() take a port ID in order to select between
    > port->cila and port->cilb register banks. But then during ->hw_init()
    > you need to go and assign port->cila and port->cilb using a CIL base
    > and a port offset from that base.
    >
    > So it sounds like this could be done much easier by doing something
    > like:
    >
    > static u32 cil_read(struct tegra_csi_port *port, u8 port_id, u32 addr)
    > {
    > unsigned int offset = port_id * TEGRA210_CSI_PORT_OFFSET;
    >
    > return readl(port->cila + TEGRA210_CSI_CIL_OFFSET + offset);
    > }
    >
    > Obviously there'd be no need for port->cilb in that case and port->cila
    > could just be port->cil. Furthermore, since you have the prefixes here
    > it sounds like these will be different for other generations, so perhaps
    > they can be parameterized as part of the SoC-specific structure?
    >
    > Another thing that I find confusing is that we have a structure called
    > tegra_csi_port, but in order to access registers within it we also need
    > to pass a port ID. So it sounds like whatever tegra_csi_port represents
    > isn't really a port.
    >
    > Do you have any ideas on how to simplify this? It's not a terribly big
    > deal, so feel free to leave it like this for now. I can take a look at
    > simplifying later on if it keeps bugging me.

    Will update cil_write/read to use cila port and compute for cilb based
    on port id.

    in csi2_hw_init, all corresponding PP/CILA/CILB/TPG register base offset
    computation based on CSI port id can all be moved into the corresponding
    pp/cil/csi/tpg_read/write. We can also move framefmt into csi_channel as
    get rid of csi_port.

    >> +static int csi2_start_streaming(struct tegra_csi_channel *csi_chan,
    >> + u8 pg_mode, int enable)
    >> +{
    >> + struct tegra_csi_device *csi = csi_chan->csi;
    >> + unsigned int port_num = csi_chan->csi_port_num;
    >> + unsigned int block = port_num >> 1;
    >> + struct tegra_csi_port *port = csi_chan->ports;
    >> + unsigned int cil_max_freq = csi->soc_data->cil_max_clk_hz;
    >> + struct clk *cil_clk;
    >> + int ret;
    >> +
    >> + if (block == CSI_CIL_AB)
    >> + cil_clk = csi->cilab_clk;
    >> + else if (block == CSI_CIL_CD)
    >> + cil_clk = csi->cilcd_clk;
    >> + else
    >> + cil_clk = csi->cilef_clk;
    >> +
    >> + if (enable) {
    >> + ret = clk_set_rate(cil_clk, cil_max_freq);
    >> + if (ret)
    >> + dev_err(csi->dev,
    >> + "failed to set cil clk rate, err: %d\n", ret);
    > Perhaps dev_warn() since it's not a fatal error? Also, maybe spell out
    > "clock" in error messages (and perhaps s/cil/CIL/). I also personally
    > prefer the style of error messages to be:
    >
    > "failed to ...: %d\n"
    >
    > i.e. without that ", err" in there. We use that style very widely, which
    > has the advantage of making the log look very consistent.
    Will update
    >> +
    >> + /* enable CIL clock on first open */
    >> + if (atomic_add_return(1, &csi->clk_refcnt) == 1) {
    >> + ret = clk_prepare_enable(cil_clk);
    >> + if (ret) {
    >> + dev_err(csi->dev,
    >> + "failed to enable cil clk, err: %d\n",
    >> + ret);
    >> + return ret;
    >> + }
    >> + }
    >> +
    >> + /*
    >> + * On Tegra210, TPG internal logic uses PLLD out along with
    >> + * the CIL clock.
    >> + * So, enable TPG clock during TPG mode streaming.
    >> + */
    >> + if (pg_mode) {
    >> + ret = clk_set_rate(csi->tpg_clk, TEGRA210_TPG_CLOCK);
    >> + if (ret)
    >> + dev_err(csi->dev,
    >> + "failed to set tpg clk rate\n");
    >> +
    >> + ret = clk_prepare_enable(csi->tpg_clk);
    >> + if (ret) {
    >> + dev_err(csi->dev,
    >> + "failed to enable tpg clk, err: %d\n",
    >> + ret);
    >> + return ret;
    >> + }
    >> + }
    >> +
    >> + csi_write(csi_chan, block, TEGRA_CSI_CLKEN_OVERRIDE, 0);
    >> +
    >> + /* clean up status */
    >> + pp_write(port, TEGRA_CSI_PIXEL_PARSER_STATUS, 0xFFFFFFFF);
    >> + cil_write(port, port_num, TEGRA_CSI_CIL_STATUS, 0xFFFFFFFF);
    >> + cil_write(port, port_num, TEGRA_CSI_CILX_STATUS, 0xFFFFFFFF);
    >> + cil_write(port, port_num, TEGRA_CSI_CIL_INTERRUPT_MASK, 0x0);
    >> +
    >> + /* CIL PHY registers setup */
    >> + cil_write(port, port_num, TEGRA_CSI_CIL_PAD_CONFIG0, 0x0);
    >> + cil_write(port, port_num, TEGRA_CSI_CIL_PHY_CONTROL, 0xA);
    >> +
    >> + /*
    >> + * The CSI unit provides for connection of up to six cameras in
    >> + * the system and is organized as three identical instances of
    >> + * two MIPI support blocks, each with a separate 4-lane
    >> + * interface that can be configured as a single camera with 4
    >> + * lanes or as a dual camera with 2 lanes available for each
    >> + * camera.
    >> + */
    >> + if (port->lanes == 4) {
    >> + cil_write(port, port_num, TEGRA_CSI_CIL_PAD_CONFIG0,
    >> + BRICK_CLOCK_A_4X);
    >> +
    >> + cil_write(port, (port_num + 1),
    > No need for parentheses around "port_num + 1" here and below.
    >
    >> + TEGRA_CSI_CIL_PAD_CONFIG0, 0x0);
    >> +
    >> + cil_write(port, (port_num + 1),
    >> + TEGRA_CSI_CIL_INTERRUPT_MASK, 0x0);
    >> +
    >> + cil_write(port, (port_num + 1),
    >> + TEGRA_CSI_CIL_PHY_CONTROL, 0xA);
    >> +
    >> + csi_write(csi_chan, block, TEGRA_CSI_PHY_CIL_COMMAND,
    >> + CSI_A_PHY_CIL_ENABLE | CSI_B_PHY_CIL_ENABLE);
    >> + } else {
    >> + u32 val = ((port_num & 1) == PORT_A) ?
    >> + CSI_A_PHY_CIL_ENABLE | CSI_B_PHY_CIL_NOP :
    >> + CSI_B_PHY_CIL_ENABLE | CSI_A_PHY_CIL_NOP;
    >> + csi_write(csi_chan, block, TEGRA_CSI_PHY_CIL_COMMAND,
    >> + val);
    >> + }
    >> +
    >> + /* CSI pixel parser registers setup */
    >> + pp_write(port, TEGRA_CSI_PIXEL_STREAM_PP_COMMAND,
    >> + (0xF << CSI_PP_START_MARKER_FRAME_MAX_OFFSET) |
    >> + CSI_PP_SINGLE_SHOT_ENABLE | CSI_PP_RST);
    >> + pp_write(port, TEGRA_CSI_PIXEL_PARSER_INTERRUPT_MASK, 0x0);
    >> + pp_write(port, TEGRA_CSI_PIXEL_STREAM_CONTROL0,
    >> + CSI_PP_PACKET_HEADER_SENT |
    >> + CSI_PP_DATA_IDENTIFIER_ENABLE |
    >> + CSI_PP_WORD_COUNT_SELECT_HEADER |
    >> + CSI_PP_CRC_CHECK_ENABLE | CSI_PP_WC_CHECK |
    >> + CSI_PP_OUTPUT_FORMAT_STORE |
    >> + CSI_PP_HEADER_EC_DISABLE | CSI_PPA_PAD_FRAME_NOPAD |
    >> + (port_num & 1));
    >> + pp_write(port, TEGRA_CSI_PIXEL_STREAM_CONTROL1,
    >> + (0x1 << CSI_PP_TOP_FIELD_FRAME_OFFSET) |
    >> + (0x1 << CSI_PP_TOP_FIELD_FRAME_MASK_OFFSET));
    >> + pp_write(port, TEGRA_CSI_PIXEL_STREAM_GAP,
    >> + 0x14 << PP_FRAME_MIN_GAP_OFFSET);
    >> + pp_write(port, TEGRA_CSI_PIXEL_STREAM_EXPECTED_FRAME, 0x0);
    >> + pp_write(port, TEGRA_CSI_INPUT_STREAM_CONTROL,
    >> + (0x3f << CSI_SKIP_PACKET_THRESHOLD_OFFSET) |
    >> + (port->lanes - 1));
    >> +
    >> + /* TPG setup */
    >> + if (pg_mode) {
    >> + tpg_write(port, TEGRA_CSI_PATTERN_GENERATOR_CTRL,
    >> + ((pg_mode - 1) << PG_MODE_OFFSET) |
    >> + PG_ENABLE);
    >> + tpg_write(port, TEGRA_CSI_PG_PHASE, 0x0);
    >> + tpg_write(port, TEGRA_CSI_PG_RED_FREQ,
    >> + (0x10 << PG_RED_VERT_INIT_FREQ_OFFSET) |
    >> + (0x10 << PG_RED_HOR_INIT_FREQ_OFFSET));
    >> + tpg_write(port, TEGRA_CSI_PG_RED_FREQ_RATE, 0x0);
    >> + tpg_write(port, TEGRA_CSI_PG_GREEN_FREQ,
    >> + (0x10 << PG_GREEN_VERT_INIT_FREQ_OFFSET) |
    >> + (0x10 << PG_GREEN_HOR_INIT_FREQ_OFFSET));
    >> + tpg_write(port, TEGRA_CSI_PG_GREEN_FREQ_RATE, 0x0);
    >> + tpg_write(port, TEGRA_CSI_PG_BLUE_FREQ,
    >> + (0x10 << PG_BLUE_VERT_INIT_FREQ_OFFSET) |
    >> + (0x10 << PG_BLUE_HOR_INIT_FREQ_OFFSET));
    >> + tpg_write(port, TEGRA_CSI_PG_BLUE_FREQ_RATE, 0x0);
    >> + }
    >> +
    >> + pp_write(port, TEGRA_CSI_PIXEL_STREAM_PP_COMMAND,
    >> + (0xF << CSI_PP_START_MARKER_FRAME_MAX_OFFSET) |
    >> + CSI_PP_SINGLE_SHOT_ENABLE | CSI_PP_ENABLE);
    >> + } else {
    >> + u32 val = pp_read(port, TEGRA_CSI_PIXEL_PARSER_STATUS);
    >> +
    >> + dev_dbg(csi->dev, "TEGRA_CSI_PIXEL_PARSER_STATUS 0x%08x\n",
    >> + val);
    > Are these still useful? I notice that you don't have debug output for
    > the case where enable == true.

    Before stopping stream, dumping PP/CIL status register which helps to
    see PP/CIL errors that can occur during the stream.

    We don't need this during enable as we start with clearing off all
    errors and any PP/CIL errors happen during the stream gets set till we
    clear them.

    >> +
    >> + val = cil_read(port, port_num, TEGRA_CSI_CIL_STATUS);
    >> + dev_dbg(csi->dev, "TEGRA_CSI_CIL_STATUS 0x%08x\n", val);
    >> +
    >> + val = cil_read(port, port_num, TEGRA_CSI_CILX_STATUS);
    >> + dev_dbg(csi->dev, "TEGRA_CSI_CILX_STATUS 0x%08x\n", val);
    >> +
    >> + pp_write(port, TEGRA_CSI_PIXEL_STREAM_PP_COMMAND,
    >> + (0xF << CSI_PP_START_MARKER_FRAME_MAX_OFFSET) |
    >> + CSI_PP_DISABLE);
    >> +
    >> + if (pg_mode) {
    >> + tpg_write(port, TEGRA_CSI_PATTERN_GENERATOR_CTRL,
    >> + ((pg_mode - 1) << PG_MODE_OFFSET) |
    >> + PG_DISABLE);
    >> + clk_disable_unprepare(csi->tpg_clk);
    >> + }
    >> +
    >> + if (port->lanes == 4) {
    >> + csi_write(csi_chan, block, TEGRA_CSI_PHY_CIL_COMMAND,
    >> + CSI_A_PHY_CIL_DISABLE |
    >> + CSI_B_PHY_CIL_DISABLE);
    >> +
    >> + } else {
    >> + u32 val = ((port_num & 1) == PORT_A) ?
    >> + CSI_A_PHY_CIL_DISABLE | CSI_B_PHY_CIL_NOP :
    >> + CSI_B_PHY_CIL_DISABLE | CSI_A_PHY_CIL_NOP;
    >> + csi_write(csi_chan, block, TEGRA_CSI_PHY_CIL_COMMAND,
    >> + val);
    >> + }
    >> + }
    >> +
    >> + return 0;
    >> +}
    >> +
    >> +static int csi2_hw_init(struct tegra_csi_device *csi)
    >> +{
    >> + struct tegra_csi_channel *csi_chan;
    >> + struct tegra_csi_port *port;
    >> + u8 portno;
    >> + int ret;
    >> +
    >> + csi->cilab_clk = devm_clk_get(csi->dev, "cilab");
    >> + if (IS_ERR(csi->cilab_clk)) {
    >> + dev_err(csi->dev, "Failed to get cilab clock\n");
    > Maybe output the error code here? The important thing here is to make
    > error messages consistent, which can simplify analysis of the kernel log
    > later on.
    WIll fix all error messages.
    >> + return PTR_ERR(csi->cilab_clk);
    >> + }
    >> +
    >> + csi->cilcd_clk = devm_clk_get(csi->dev, "cilcd");
    >> + if (IS_ERR(csi->cilcd_clk)) {
    >> + dev_err(csi->dev, "Failed to get cilcd clock\n");
    >> + return PTR_ERR(csi->cilcd_clk);
    >> + }
    >> +
    >> + csi->cilef_clk = devm_clk_get(csi->dev, "cile");
    >> + if (IS_ERR(csi->cilef_clk)) {
    >> + dev_err(csi->dev, "Failed to get cile clock\n");
    >> + return PTR_ERR(csi->cilef_clk);
    >> + }
    >> +
    >> + csi->tpg_clk = devm_clk_get(csi->dev, "csi_tpg");
    >> + if (IS_ERR(csi->tpg_clk)) {
    >> + dev_err(csi->dev, "Failed to get csi_tpg clock\n");
    >> + return PTR_ERR(csi->tpg_clk);
    >> + }
    >> +
    >> + csi->csi_clk = devm_clk_get(csi->dev, "csi");
    >> + if (IS_ERR(csi->csi_clk)) {
    >> + dev_err(csi->dev, "Failed to get csi clock\n");
    >> + return PTR_ERR(csi->csi_clk);
    >> + }
    >> +
    >> + ret = clk_prepare_enable(csi->csi_clk);
    >> + if (ret) {
    >> + dev_err(csi->dev, "Failed to enable csi clk, err: %d\n", ret);
    >> + return ret;
    >> + }
    >> +
    >> + list_for_each_entry(csi_chan, &csi->csi_chans, list) {
    >> + void __iomem *csi_pp_base;
    >> +
    >> + port = csi_chan->ports;
    >> + portno = csi_chan->csi_port_num;
    >> + csi_pp_base = csi->iomem + CSI_PP_OFFSET(portno >> 1);
    >> + port->pixel_parser = csi_pp_base +
    >> + (portno % CSI_PORTS_PER_BRICK) *
    >> + TEGRA210_CSI_PORT_OFFSET;
    >> + port->cila = csi_pp_base + TEGRA210_CSI_CIL_OFFSET;
    >> + port->cilb = port->cila + TEGRA210_CSI_PORT_OFFSET;
    >> + port->tpg = port->pixel_parser + TEGRA210_CSI_TPG_OFFSET;
    >> + }
    >> +
    >> + return 0;
    >> +}
    >> +
    >> +const struct tegra_csi_fops csi2_fops = {
    >> + .hw_init = csi2_hw_init,
    >> + .csi_start_streaming = csi2_start_streaming,
    >> + .csi_err_status = csi2_error_status,
    >> +};
    >> +EXPORT_SYMBOL(csi2_fops);
    >> diff --git a/drivers/staging/media/tegra/csi2_fops.h b/drivers/staging/media/tegra/csi2_fops.h
    >> new file mode 100644
    >> index 000000000000..cf22a28ceb1f
    >> --- /dev/null
    >> +++ b/drivers/staging/media/tegra/csi2_fops.h
    >> @@ -0,0 +1,15 @@
    >> +// SPDX-License-Identifier: GPL-2.0-only
    >> +/*
    >> + * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved.
    >> + */
    >> +
    >> +#ifndef __CSI2_H__
    >> +#define __CSI2_H__
    >> +
    >> +#define TEGRA210_TPG_CLOCK 594000000
    >> +#define TEGRA210_CSI_CIL_CLK_MAX 102000000
    >> +#define TEGRA210_CSI_BRICKS_MAX 3
    >> +
    >> +extern const struct tegra_csi_fops csi2_fops;
    >> +
    >> +#endif
    >> diff --git a/drivers/staging/media/tegra/host1x-video.c b/drivers/staging/media/tegra/host1x-video.c
    >> new file mode 100644
    >> index 000000000000..740806121e6b
    >> --- /dev/null
    >> +++ b/drivers/staging/media/tegra/host1x-video.c
    >> @@ -0,0 +1,120 @@
    >> +// SPDX-License-Identifier: GPL-2.0-only
    >> +/*
    >> + * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved.
    >> + */
    >> +
    >> +#include <linux/host1x.h>
    >> +#include <linux/module.h>
    >> +#include <linux/platform_device.h>
    >> +
    >> +#include "host1x-video.h"
    >> +
    >> +static int host1x_video_probe(struct host1x_device *dev)
    >> +{
    >> + struct tegra_camera *cam;
    >> + int ret;
    >> +
    >> + cam = devm_kzalloc(&dev->dev, sizeof(*cam), GFP_KERNEL);
    >> + if (!cam)
    >> + return -ENOMEM;
    >> +
    >> + cam->dev = get_device(&dev->dev);
    >> + cam->media_dev.dev = cam->dev;
    >> + strscpy(cam->media_dev.model, "NVIDIA Tegra Video Input Device",
    >> + sizeof(cam->media_dev.model));
    >> + cam->media_dev.hw_revision = 3;
    > Where does this come from?
    >
    >> +
    >> + media_device_init(&cam->media_dev);
    >> + ret = media_device_register(&cam->media_dev);
    >> + if (ret < 0) {
    >> + dev_err(cam->dev, "failed to register media device: %d\n", ret);
    >> + return ret;
    >> + }
    >> +
    >> + cam->v4l2_dev.mdev = &cam->media_dev;
    >> + ret = v4l2_device_register(cam->dev, &cam->v4l2_dev);
    >> + if (ret < 0) {
    >> + dev_err(cam->dev, "V4L2 device registration failed: %d\n", ret);
    >> + goto register_error;
    >> + }
    >> +
    >> + dev_set_drvdata(&dev->dev, cam);
    >> +
    >> + ret = host1x_device_init(dev);
    >> + if (ret < 0)
    >> + goto dev_exit;
    >> +
    >> + return 0;
    >> +
    >> +dev_exit:
    >> + host1x_device_exit(dev);
    > There should be no need to call host1x_device_exit() when
    > host1x_device_init() failed because the latter already takes care of
    > undoing whatever it did already.
    >
    host1x_device_init can fail if any of its client ops init fails.

    So, calling host1x_device_exit here to undo the things done in other
    successful client init ops.

    >> + v4l2_device_unregister(&cam->v4l2_dev);
    >> +register_error:
    >> + media_device_unregister(&cam->media_dev);
    >> + media_device_cleanup(&cam->media_dev);
    >> +
    >> + return ret;
    >> +}
    >> +
    >> +static int host1x_video_remove(struct host1x_device *dev)
    >> +{
    >> + struct tegra_camera *cam = dev_get_drvdata(&dev->dev);
    >> +
    >> + host1x_device_exit(dev);
    >> + v4l2_device_unregister(&cam->v4l2_dev);
    >> + media_device_unregister(&cam->media_dev);
    >> + media_device_cleanup(&cam->media_dev);
    >> +
    >> + return 0;
    >> +}
    >> +
    >> +static const struct of_device_id host1x_video_subdevs[] = {
    >> + { .compatible = "nvidia,tegra210-csi", },
    >> + { .compatible = "nvidia,tegra210-vi", },
    >> + { }
    >> +};
    >> +
    >> +static struct host1x_driver host1x_video_driver = {
    >> + .driver = {
    >> + .name = "host1x_video",
    > We typically use - instead of _ in names. This helps making access to
    > sysfs or debugfs consistent across drivers.
    Will change
    >> + },
    >> + .probe = host1x_video_probe,
    >> + .remove = host1x_video_remove,
    >> + .subdevs = host1x_video_subdevs,
    >> +};
    >> +
    >> +static struct platform_driver * const drivers[] = {
    >> + &tegra_csi_driver,
    >> + &tegra_vi_driver,
    >> +};
    >> +
    >> +static int __init host1x_video_init(void)
    >> +{
    >> + int err;
    >> +
    >> + err = host1x_driver_register(&host1x_video_driver);
    >> + if (err < 0)
    >> + return err;
    >> +
    >> + err = platform_register_drivers(drivers, ARRAY_SIZE(drivers));
    >> + if (err < 0)
    >> + goto unregister_host1x;
    >> +
    >> + return 0;
    >> +
    >> +unregister_host1x:
    >> + host1x_driver_unregister(&host1x_video_driver);
    >> + return err;
    >> +}
    >> +module_init(host1x_video_init);
    >> +
    >> +static void __exit host1x_video_exit(void)
    >> +{
    >> + platform_unregister_drivers(drivers, ARRAY_SIZE(drivers));
    >> + host1x_driver_unregister(&host1x_video_driver);
    >> +}
    >> +module_exit(host1x_video_exit);
    >> +
    >> +MODULE_AUTHOR("Sowjanya Komatineni <skomatineni@nvidia.com>");
    >> +MODULE_DESCRIPTION("NVIDIA Tegra Host1x Video driver");
    >> +MODULE_LICENSE("GPL v2");
    >> diff --git a/drivers/staging/media/tegra/host1x-video.h b/drivers/staging/media/tegra/host1x-video.h
    >> new file mode 100644
    >> index 000000000000..84d28e6f4362
    >> --- /dev/null
    >> +++ b/drivers/staging/media/tegra/host1x-video.h
    >> @@ -0,0 +1,33 @@
    >> +// SPDX-License-Identifier: GPL-2.0-only
    >> +/*
    >> + * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved.
    >> + */
    >> +
    >> +#ifndef HOST1X_VIDEO_H
    >> +#define HOST1X_VIDEO_H 1
    >> +
    >> +#include <linux/host1x.h>
    >> +
    >> +#include <media/media-device.h>
    >> +#include <media/media-entity.h>
    >> +#include <media/v4l2-async.h>
    >> +#include <media/v4l2-ctrls.h>
    >> +#include <media/v4l2-device.h>
    >> +#include <media/v4l2-dev.h>
    >> +#include <media/videobuf2-v4l2.h>
    >> +
    >> +#include "tegra-vi.h"
    >> +#include "csi.h"
    >> +
    >> +struct tegra_camera {
    > Nit: "camera" seems like a suboptimal choice because usually VI will
    > consume data from multiple cameras. Maybe something like "tegra_video"
    > would be a better name?
    Will change
    >
    >> + struct v4l2_device v4l2_dev;
    >> + struct media_device media_dev;
    >> + struct device *dev;
    >> + struct tegra_vi *vi;
    >> + struct tegra_csi_device *csi;
    >> +};
    >> +
    >> +extern struct platform_driver tegra_vi_driver;
    >> +extern struct platform_driver tegra_csi_driver;
    >> +
    >> +#endif /* HOST1X_VIDEO_H */
    > [...]
    >> diff --git a/drivers/staging/media/tegra/tegra-channel.c b/drivers/staging/media/tegra/tegra-channel.c
    >> new file mode 100644
    >> index 000000000000..7561f6fb8748
    >> --- /dev/null
    >> +++ b/drivers/staging/media/tegra/tegra-channel.c
    >> @@ -0,0 +1,628 @@
    >> +// SPDX-License-Identifier: GPL-2.0-only
    >> +/*
    >> + * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved.
    >> + */
    >> +
    >> +#include <linux/atomic.h>
    >> +#include <linux/bitmap.h>
    >> +#include <linux/clk.h>
    >> +#include <linux/host1x.h>
    >> +#include <linux/kthread.h>
    >> +#include <linux/lcm.h>
    >> +#include <linux/list.h>
    >> +#include <linux/module.h>
    >> +#include <linux/of.h>
    >> +#include <linux/sched.h>
    >> +#include <linux/slab.h>
    >> +
    >> +#include <media/v4l2-event.h>
    >> +#include <media/v4l2-fh.h>
    >> +#include <media/v4l2-ioctl.h>
    >> +#include <media/videobuf2-dma-contig.h>
    >> +
    >> +#include <soc/tegra/pmc.h>
    >> +
    >> +#include "mc_common.h"
    >> +#include "tegra-vi.h"
    >> +#include "host1x-video.h"
    >> +
    >> +#define MAX_CID_CONTROLS 1
    >> +
    >> +static const char *const vi_pattern_strings[] = {
    >> + "Black/White Direct Mode",
    >> + "Color Patch Mode",
    >> +};
    >> +
    >> +static int vi_s_ctrl(struct v4l2_ctrl *ctrl)
    >> +{
    >> + struct tegra_channel *chan = container_of(ctrl->handler,
    >> + struct tegra_channel,
    >> + ctrl_handler);
    >> +
    >> + switch (ctrl->id) {
    >> + case V4L2_CID_TEST_PATTERN:
    >> + chan->vi->pg_mode = ctrl->val + 1;
    >> + dev_info(chan->vi->dev, "TPG mode is set to %s\n",
    >> + vi_pattern_strings[ctrl->val]);
    > dev_dbg()?
    >
    >> + break;
    >> +
    >> + default:
    >> + dev_err(chan->vi->dev, "Invalid Tegra video control\n");
    > That potentially allows an attacker to DoS by flooding the kernel log.
    > Isn't the -EINVAL below already going to provide enough information to
    > the caller? If we really want this, perhaps turn it into a dev_dbg() or
    > even better yet, a rate-limited dev_dbg().
    >
    >> + return -EINVAL;
    >> + }
    >> +
    >> + return 0;
    >> +}
    >> +
    > [...]
    >> diff --git a/drivers/staging/media/tegra/vi2_fops.c b/drivers/staging/media/tegra/vi2_fops.c
    > [...]
    >> +const struct tegra_vi_fops vi2_fops = {
    >> + .vi_stride_align = vi2_stride_align,
    >> + .vi_start_streaming = vi2_channel_start_streaming,
    >> + .vi_stop_streaming = vi2_channel_stop_streaming,
    >> +};
    >> +EXPORT_SYMBOL(vi2_fops);
    > There should be no need to export this, unless you want to build this as
    > a separate module, which I don't think is a good idea.
    Will remove export.
    >> diff --git a/drivers/staging/media/tegra/vi2_fops.h b/drivers/staging/media/tegra/vi2_fops.h
    >> new file mode 100644
    >> index 000000000000..dcbd3ad4b642
    >> --- /dev/null
    >> +++ b/drivers/staging/media/tegra/vi2_fops.h
    >> @@ -0,0 +1,15 @@
    >> +// SPDX-License-Identifier: GPL-2.0-only
    >> +/*
    >> + * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved.
    >> + */
    >> +
    >> +#ifndef __T210_VI_H__
    >> +#define __T210_VI_H__
    >> +
    >> +#define TEGRA210_CLOCK_VI_MAX 460000000
    >> +
    >> +#define TEGRA_VI_CSI_BASE(x) (0x100 + (x) * 0x100)
    >> +
    >> +extern const struct tegra_vi_fops vi2_fops;
    >> +
    >> +#endif
    > I've been wondering about this. So far we've got two pairs of Tegra210
    > specific files: vi2_fops.[ch] and csi2_fops.[ch]. Is there any reason
    > why we couldn't merge those two files into a single file, say,
    > tegra210.c?
    >
    > I don't think a header file would be really necessary in that case since
    > only the tegra210.c file would use any of the definitions in that header
    > and we coul simply put the "extern" definitions into some central
    > location to make them available to the main driver.
    >
    > These files aren't terribly huge, so merging them should result in still
    > fairly manageable chunks.

    Yes, we can use single file for all Tegra210 CSI and VI fops.

    Will change in v2.

    >> diff --git a/drivers/staging/media/tegra/vi2_formats.h b/drivers/staging/media/tegra/vi2_formats.h
    >> new file mode 100644
    >> index 000000000000..416960b1c1f2
    >> --- /dev/null
    >> +++ b/drivers/staging/media/tegra/vi2_formats.h
    >> @@ -0,0 +1,119 @@
    >> +// SPDX-License-Identifier: GPL-2.0-only
    >> +/*
    >> + * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved.
    >> + */
    >> +
    >> +#ifndef __VI2_FORMATS_H_
    >> +#define __VI2_FORMATS_H_
    >> +
    >> +#include "tegra-core.h"
    >> +
    >> +/*
    >> + * These go into the TEGRA_VI_CSI_n_IMAGE_DEF registers bits 23:16
    >> + * Output pixel memory format for the VI channel.
    >> + */
    >> +enum tegra_image_format {
    >> + TEGRA_IMAGE_FORMAT_T_L8 = 16,
    >> +
    >> + TEGRA_IMAGE_FORMAT_T_R16_I = 32,
    >> + TEGRA_IMAGE_FORMAT_T_B5G6R5,
    >> + TEGRA_IMAGE_FORMAT_T_R5G6B5,
    >> + TEGRA_IMAGE_FORMAT_T_A1B5G5R5,
    >> + TEGRA_IMAGE_FORMAT_T_A1R5G5B5,
    >> + TEGRA_IMAGE_FORMAT_T_B5G5R5A1,
    >> + TEGRA_IMAGE_FORMAT_T_R5G5B5A1,
    >> + TEGRA_IMAGE_FORMAT_T_A4B4G4R4,
    >> + TEGRA_IMAGE_FORMAT_T_A4R4G4B4,
    >> + TEGRA_IMAGE_FORMAT_T_B4G4R4A4,
    >> + TEGRA_IMAGE_FORMAT_T_R4G4B4A4,
    >> +
    >> + TEGRA_IMAGE_FORMAT_T_A8B8G8R8 = 64,
    >> + TEGRA_IMAGE_FORMAT_T_A8R8G8B8,
    >> + TEGRA_IMAGE_FORMAT_T_B8G8R8A8,
    >> + TEGRA_IMAGE_FORMAT_T_R8G8B8A8,
    >> + TEGRA_IMAGE_FORMAT_T_A2B10G10R10,
    >> + TEGRA_IMAGE_FORMAT_T_A2R10G10B10,
    >> + TEGRA_IMAGE_FORMAT_T_B10G10R10A2,
    >> + TEGRA_IMAGE_FORMAT_T_R10G10B10A2,
    >> +
    >> + TEGRA_IMAGE_FORMAT_T_A8Y8U8V8 = 193,
    >> + TEGRA_IMAGE_FORMAT_T_V8U8Y8A8,
    >> +
    >> + TEGRA_IMAGE_FORMAT_T_A2Y10U10V10 = 197,
    >> + TEGRA_IMAGE_FORMAT_T_V10U10Y10A2,
    >> + TEGRA_IMAGE_FORMAT_T_Y8_U8__Y8_V8,
    >> + TEGRA_IMAGE_FORMAT_T_Y8_V8__Y8_U8,
    >> + TEGRA_IMAGE_FORMAT_T_U8_Y8__V8_Y8,
    >> + TEGRA_IMAGE_FORMAT_T_V8_Y8__U8_Y8,
    >> +
    >> + TEGRA_IMAGE_FORMAT_T_Y8__U8__V8_N444 = 224,
    >> + TEGRA_IMAGE_FORMAT_T_Y8__U8V8_N444,
    >> + TEGRA_IMAGE_FORMAT_T_Y8__V8U8_N444,
    >> + TEGRA_IMAGE_FORMAT_T_Y8__U8__V8_N422,
    >> + TEGRA_IMAGE_FORMAT_T_Y8__U8V8_N422,
    >> + TEGRA_IMAGE_FORMAT_T_Y8__V8U8_N422,
    >> + TEGRA_IMAGE_FORMAT_T_Y8__U8__V8_N420,
    >> + TEGRA_IMAGE_FORMAT_T_Y8__U8V8_N420,
    >> + TEGRA_IMAGE_FORMAT_T_Y8__V8U8_N420,
    >> + TEGRA_IMAGE_FORMAT_T_X2LC10LB10LA10,
    >> + TEGRA_IMAGE_FORMAT_T_A2R6R6R6R6R6,
    >> +};
    >> +
    >> +static const struct tegra_video_format vi2_video_formats[] = {
    >> + /* RAW 8 */
    >> + TEGRA_VIDEO_FORMAT(RAW8, 8, SRGGB8_1X8, 1, 1, T_L8,
    >> + RAW8, SRGGB8, "RGRG.. GBGB.."),
    >> + TEGRA_VIDEO_FORMAT(RAW8, 8, SGRBG8_1X8, 1, 1, T_L8,
    >> + RAW8, SGRBG8, "GRGR.. BGBG.."),
    >> + TEGRA_VIDEO_FORMAT(RAW8, 8, SGBRG8_1X8, 1, 1, T_L8,
    >> + RAW8, SGBRG8, "GBGB.. RGRG.."),
    >> + TEGRA_VIDEO_FORMAT(RAW8, 8, SBGGR8_1X8, 1, 1, T_L8,
    >> + RAW8, SBGGR8, "BGBG.. GRGR.."),
    >> +
    >> + /* RAW 10 */
    >> + TEGRA_VIDEO_FORMAT(RAW10, 10, SRGGB10_1X10, 2, 1, T_R16_I,
    >> + RAW10, SRGGB10, "RGRG.. GBGB.."),
    >> + TEGRA_VIDEO_FORMAT(RAW10, 10, SGRBG10_1X10, 2, 1, T_R16_I,
    >> + RAW10, SGRBG10, "GRGR.. BGBG.."),
    >> + TEGRA_VIDEO_FORMAT(RAW10, 10, SGBRG10_1X10, 2, 1, T_R16_I,
    >> + RAW10, SGBRG10, "GBGB.. RGRG.."),
    >> + TEGRA_VIDEO_FORMAT(RAW10, 10, SBGGR10_1X10, 2, 1, T_R16_I,
    >> + RAW10, SBGGR10, "BGBG.. GRGR.."),
    >> +
    >> + /* RAW 12 */
    >> + TEGRA_VIDEO_FORMAT(RAW12, 12, SRGGB12_1X12, 2, 1, T_R16_I,
    >> + RAW12, SRGGB12, "RGRG.. GBGB.."),
    >> + TEGRA_VIDEO_FORMAT(RAW12, 12, SGRBG12_1X12, 2, 1, T_R16_I,
    >> + RAW12, SGRBG12, "GRGR.. BGBG.."),
    >> + TEGRA_VIDEO_FORMAT(RAW12, 12, SGBRG12_1X12, 2, 1, T_R16_I,
    >> + RAW12, SGBRG12, "GBGB.. RGRG.."),
    >> + TEGRA_VIDEO_FORMAT(RAW12, 12, SBGGR12_1X12, 2, 1, T_R16_I,
    >> + RAW12, SBGGR12, "BGBG.. GRGR.."),
    >> +
    >> + /* RGB888 */
    >> + TEGRA_VIDEO_FORMAT(RGB888, 24, RGB888_1X24, 4, 1, T_A8R8G8B8,
    >> + RGB888, ABGR32, "BGRA-8-8-8-8"),
    >> + TEGRA_VIDEO_FORMAT(RGB888, 24, RGB888_1X32_PADHI, 4, 1, T_A8B8G8R8,
    >> + RGB888, RGB32, "RGB-8-8-8-8"),
    >> +
    >> + /* YUV422 */
    >> + TEGRA_VIDEO_FORMAT(YUV422, 16, UYVY8_1X16, 2, 1, T_U8_Y8__V8_Y8,
    >> + YUV422_8, UYVY, "YUV 4:2:2"),
    >> + TEGRA_VIDEO_FORMAT(YUV422, 16, VYUY8_1X16, 2, 1, T_V8_Y8__U8_Y8,
    >> + YUV422_8, VYUY, "YUV 4:2:2"),
    >> + TEGRA_VIDEO_FORMAT(YUV422, 16, YUYV8_1X16, 2, 1, T_Y8_U8__Y8_V8,
    >> + YUV422_8, YUYV, "YUV 4:2:2"),
    >> + TEGRA_VIDEO_FORMAT(YUV422, 16, YVYU8_1X16, 2, 1, T_Y8_V8__Y8_U8,
    >> + YUV422_8, YVYU, "YUV 4:2:2"),
    >> + TEGRA_VIDEO_FORMAT(YUV422, 16, UYVY8_1X16, 1, 1, T_Y8__V8U8_N422,
    >> + YUV422_8, NV16, "NV16"),
    >> + TEGRA_VIDEO_FORMAT(YUV422, 16, UYVY8_2X8, 2, 1, T_U8_Y8__V8_Y8,
    >> + YUV422_8, UYVY, "YUV 4:2:2 UYVY"),
    >> + TEGRA_VIDEO_FORMAT(YUV422, 16, VYUY8_2X8, 2, 1, T_V8_Y8__U8_Y8,
    >> + YUV422_8, VYUY, "YUV 4:2:2 VYUY"),
    >> + TEGRA_VIDEO_FORMAT(YUV422, 16, YUYV8_2X8, 2, 1, T_Y8_U8__Y8_V8,
    >> + YUV422_8, YUYV, "YUV 4:2:2 YUYV"),
    >> + TEGRA_VIDEO_FORMAT(YUV422, 16, YVYU8_2X8, 2, 1, T_Y8_V8__Y8_U8,
    >> + YUV422_8, YVYU, "YUV 4:2:2 YVYU"),
    >> +};
    > Does this table perhaps also belong in tegra210.c?

    vi2_formats.h is for Tegra210 specific video formats.

    Yes we can merge csi fops, vi fops, Tegra210 specific video formats all
    into single file.

    Will update in v2.

    >
    >> +#endif
    >> diff --git a/drivers/staging/media/tegra/vi2_registers.h b/drivers/staging/media/tegra/vi2_registers.h
    >> new file mode 100644
    >> index 000000000000..c54a6a3aa1c4
    >> --- /dev/null
    >> +++ b/drivers/staging/media/tegra/vi2_registers.h
    >> @@ -0,0 +1,194 @@
    >> +// SPDX-License-Identifier: GPL-2.0-only
    >> +/*
    >> + * Copyright (C) 2020 NVIDIA CORPORATION. All rights reserved.
    >> + */
    >> +
    >> +#ifndef __REGISTERS_H__
    >> +#define __REGISTERS_H__
    >> +
    >> +#define TEGRA_CLOCK_VI_MAX 793600000
    >> +#define TEGRA210_SURFACE_ALIGNMENT 64
    >> +#define TEGRA210_MAX_CHANNELS 6
    >> +
    >> +/* Tegra210 VI registers */
    > If these are Tegra210-specific, are they accessed outside of Tegra210-
    > specific code? If not, they may be better located in that new tegra210.c
    > source file as well. That way it becomes a lot easier to distinguish
    > between generic, perhaps parameterized, core code and the SoC generation
    > specific code.
    >
    > Thierry

    Yes they are T210 specific and not accessed outside of T210 code. Will
    move them to new single file.

    \
     
     \ /
      Last update: 2020-01-29 18:24    [W:3.467 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site