From: Hans Verkuil <hverkuil-cisco@xs4all.nl>
To: Jacopo Mondi <jacopo+renesas@jmondi.org>,
mchehab@kernel.org, sakari.ailus@linux.intel.com,
laurent.pinchart@ideasonboard.com
Cc: niklas.soderlund+renesas@ragnatech.se,
kieran.bingham@ideasonboard.com, dave.stevenson@raspberrypi.com,
hyun.kwon@xilinx.com, jmkrzyszt@gmail.com,
robert.jarzmik@free.fr, linux-media@vger.kernel.org,
linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v8 04/10] media: pxa_camera: Use the new set_mbus_config op
Date: Mon, 20 Jul 2020 10:55:43 +0200 [thread overview]
Message-ID: <aee934c4-c5d4-ba7f-8989-dfb57c8eda2a@xs4all.nl> (raw)
In-Reply-To: <20200717145324.292820-5-jacopo+renesas@jmondi.org>
On 17/07/2020 16:53, Jacopo Mondi wrote:
> Move the PXA camera driver to use the new set_mbus_config pad operation.
> For this platform the change is not only cosmetic, as the pxa driver is
> currently the only driver in mainline to make use of the g_mbus_config
> and s_mbus_config video operations.
>
> The existing driver semantic is the following:
> - Collect all supported mbus config flags from the remote end
> - Match them with the supported PXA mbus configuration flags
> - If the remote subdevice allows multiple options for for VSYNC, HSYNC
> and PCLK polarity, use platform data requested settings
>
> The semantic of the new get_mbus_config and set_mbus_config differs from
> the corresponding video ops, particularly in the fact get_mbus_config
> reports the current mbus configuration and not the set of supported
> configuration options, with set_mbus_config always reporting the actual
> mbus configuration applied to the remote subdevice.
>
> Adapt the driver to perform the following
> - Set the remote subdevice mbus configuration according to the PXA
> platform data preferences.
> - If the applied configuration differs from the requested one (i.e. the
> remote subdevice does not allow changing one setting) make sure that
> - The remote end does not claim for DATA_ACTIVE_LOW, which seems not
> supported by the platform
> - The bus mastering roles match
>
> While at there remove a few checks performed on the media bus
> configuration at get_format() time as they do not belong there.
General pxa_camera question: set_mbus_config is still called when the
pxa_camera driver is using the device tree data instead of platform data.
Is that necessary? Can't it fully rely on the DT information?
I would like to limit the use of set_mbus_config to signal all these low
level bits to the absolute minimum and today it is only two pxa board files
that use the platform data and thus need set_mbus_config.
As I said elsewhere, the only reason for set_mbus_config is really to set
lanes/channels, anything else should really be fixed since that comes from
the DT.
Note: this is just for discussion, any possible changes can be done later
once the current series is merged.
Regards,
Hans
>
> Compile-tested only.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> drivers/media/platform/pxa_camera.c | 189 ++++++++--------------------
> 1 file changed, 51 insertions(+), 138 deletions(-)
>
> diff --git a/drivers/media/platform/pxa_camera.c b/drivers/media/platform/pxa_camera.c
> index 6dce33f35041..0366c4a813ce 100644
> --- a/drivers/media/platform/pxa_camera.c
> +++ b/drivers/media/platform/pxa_camera.c
> @@ -605,42 +605,6 @@ static const struct pxa_mbus_pixelfmt *pxa_mbus_get_fmtdesc(
> return pxa_mbus_find_fmtdesc(code, mbus_fmt, ARRAY_SIZE(mbus_fmt));
> }
>
> -static unsigned int pxa_mbus_config_compatible(const struct v4l2_mbus_config *cfg,
> - unsigned int flags)
> -{
> - unsigned long common_flags;
> - bool hsync = true, vsync = true, pclk, data, mode;
> - bool mipi_lanes, mipi_clock;
> -
> - common_flags = cfg->flags & flags;
> -
> - switch (cfg->type) {
> - case V4L2_MBUS_PARALLEL:
> - hsync = common_flags & (V4L2_MBUS_HSYNC_ACTIVE_HIGH |
> - V4L2_MBUS_HSYNC_ACTIVE_LOW);
> - vsync = common_flags & (V4L2_MBUS_VSYNC_ACTIVE_HIGH |
> - V4L2_MBUS_VSYNC_ACTIVE_LOW);
> - /* fall through */
> - case V4L2_MBUS_BT656:
> - pclk = common_flags & (V4L2_MBUS_PCLK_SAMPLE_RISING |
> - V4L2_MBUS_PCLK_SAMPLE_FALLING);
> - data = common_flags & (V4L2_MBUS_DATA_ACTIVE_HIGH |
> - V4L2_MBUS_DATA_ACTIVE_LOW);
> - mode = common_flags & (V4L2_MBUS_MASTER | V4L2_MBUS_SLAVE);
> - return (!hsync || !vsync || !pclk || !data || !mode) ?
> - 0 : common_flags;
> - case V4L2_MBUS_CSI2_DPHY:
> - mipi_lanes = common_flags & V4L2_MBUS_CSI2_LANES;
> - mipi_clock = common_flags & (V4L2_MBUS_CSI2_NONCONTINUOUS_CLOCK |
> - V4L2_MBUS_CSI2_CONTINUOUS_CLOCK);
> - return (!mipi_lanes || !mipi_clock) ? 0 : common_flags;
> - default:
> - WARN_ON(1);
> - return -EINVAL;
> - }
> - return 0;
> -}
> -
> /**
> * struct pxa_camera_format_xlate - match between host and sensor formats
> * @code: code of a sensor provided format
> @@ -1231,31 +1195,6 @@ static irqreturn_t pxa_camera_irq(int irq, void *data)
> return IRQ_HANDLED;
> }
>
> -static int test_platform_param(struct pxa_camera_dev *pcdev,
> - unsigned char buswidth, unsigned long *flags)
> -{
> - /*
> - * Platform specified synchronization and pixel clock polarities are
> - * only a recommendation and are only used during probing. The PXA270
> - * quick capture interface supports both.
> - */
> - *flags = (pcdev->platform_flags & PXA_CAMERA_MASTER ?
> - V4L2_MBUS_MASTER : V4L2_MBUS_SLAVE) |
> - V4L2_MBUS_HSYNC_ACTIVE_HIGH |
> - V4L2_MBUS_HSYNC_ACTIVE_LOW |
> - V4L2_MBUS_VSYNC_ACTIVE_HIGH |
> - V4L2_MBUS_VSYNC_ACTIVE_LOW |
> - V4L2_MBUS_DATA_ACTIVE_HIGH |
> - V4L2_MBUS_PCLK_SAMPLE_RISING |
> - V4L2_MBUS_PCLK_SAMPLE_FALLING;
> -
> - /* If requested data width is supported by the platform, use it */
> - if ((1 << (buswidth - 1)) & pcdev->width_flags)
> - return 0;
> -
> - return -EINVAL;
> -}
> -
> static void pxa_camera_setup_cicr(struct pxa_camera_dev *pcdev,
> unsigned long flags, __u32 pixfmt)
> {
> @@ -1598,99 +1537,78 @@ static int pxa_camera_init_videobuf2(struct pxa_camera_dev *pcdev)
> */
> static int pxa_camera_set_bus_param(struct pxa_camera_dev *pcdev)
> {
> + unsigned int bus_width = pcdev->current_fmt->host_fmt->bits_per_sample;
> struct v4l2_mbus_config cfg = {.type = V4L2_MBUS_PARALLEL,};
> u32 pixfmt = pcdev->current_fmt->host_fmt->fourcc;
> - unsigned long bus_flags, common_flags;
> + int mbus_config;
> int ret;
>
> - ret = test_platform_param(pcdev,
> - pcdev->current_fmt->host_fmt->bits_per_sample,
> - &bus_flags);
> - if (ret < 0)
> - return ret;
> -
> - ret = sensor_call(pcdev, video, g_mbus_config, &cfg);
> - if (!ret) {
> - common_flags = pxa_mbus_config_compatible(&cfg,
> - bus_flags);
> - if (!common_flags) {
> - dev_warn(pcdev_to_dev(pcdev),
> - "Flags incompatible: camera 0x%x, host 0x%lx\n",
> - cfg.flags, bus_flags);
> - return -EINVAL;
> - }
> - } else if (ret != -ENOIOCTLCMD) {
> - return ret;
> - } else {
> - common_flags = bus_flags;
> + if (!((1 << (bus_width - 1)) & pcdev->width_flags)) {
> + dev_err(pcdev_to_dev(pcdev), "Unsupported bus width %u",
> + bus_width);
> + return -EINVAL;
> }
>
> pcdev->channels = 1;
>
> /* Make choices, based on platform preferences */
> - if ((common_flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) &&
> - (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)) {
> - if (pcdev->platform_flags & PXA_CAMERA_HSP)
> - common_flags &= ~V4L2_MBUS_HSYNC_ACTIVE_HIGH;
> - else
> - common_flags &= ~V4L2_MBUS_HSYNC_ACTIVE_LOW;
> - }
> + mbus_config = 0;
> + if (pcdev->platform_flags & PXA_CAMERA_MASTER)
> + mbus_config |= V4L2_MBUS_MASTER;
> + else
> + mbus_config |= V4L2_MBUS_SLAVE;
>
> - if ((common_flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH) &&
> - (common_flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)) {
> - if (pcdev->platform_flags & PXA_CAMERA_VSP)
> - common_flags &= ~V4L2_MBUS_VSYNC_ACTIVE_HIGH;
> - else
> - common_flags &= ~V4L2_MBUS_VSYNC_ACTIVE_LOW;
> - }
> + if (pcdev->platform_flags & PXA_CAMERA_HSP)
> + mbus_config |= V4L2_MBUS_HSYNC_ACTIVE_HIGH;
> + else
> + mbus_config |= V4L2_MBUS_HSYNC_ACTIVE_LOW;
>
> - if ((common_flags & V4L2_MBUS_PCLK_SAMPLE_RISING) &&
> - (common_flags & V4L2_MBUS_PCLK_SAMPLE_FALLING)) {
> - if (pcdev->platform_flags & PXA_CAMERA_PCP)
> - common_flags &= ~V4L2_MBUS_PCLK_SAMPLE_RISING;
> - else
> - common_flags &= ~V4L2_MBUS_PCLK_SAMPLE_FALLING;
> - }
> + if (pcdev->platform_flags & PXA_CAMERA_VSP)
> + mbus_config |= V4L2_MBUS_VSYNC_ACTIVE_HIGH;
> + else
> + mbus_config |= V4L2_MBUS_VSYNC_ACTIVE_LOW;
>
> - cfg.flags = common_flags;
> - ret = sensor_call(pcdev, video, s_mbus_config, &cfg);
> + if (pcdev->platform_flags & PXA_CAMERA_PCP)
> + mbus_config |= V4L2_MBUS_PCLK_SAMPLE_RISING;
> + else
> + mbus_config |= V4L2_MBUS_PCLK_SAMPLE_FALLING;
> + mbus_config |= V4L2_MBUS_DATA_ACTIVE_HIGH;
> +
> + cfg.flags = mbus_config;
> + ret = sensor_call(pcdev, pad, set_mbus_config, 0, &cfg);
> if (ret < 0 && ret != -ENOIOCTLCMD) {
> - dev_dbg(pcdev_to_dev(pcdev),
> - "camera s_mbus_config(0x%lx) returned %d\n",
> - common_flags, ret);
> + dev_err(pcdev_to_dev(pcdev),
> + "Failed to call set_mbus_config: %d\n", ret);
> return ret;
> }
>
> - pxa_camera_setup_cicr(pcdev, common_flags, pixfmt);
> -
> - return 0;
> -}
> -
> -static int pxa_camera_try_bus_param(struct pxa_camera_dev *pcdev,
> - unsigned char buswidth)
> -{
> - struct v4l2_mbus_config cfg = {.type = V4L2_MBUS_PARALLEL,};
> - unsigned long bus_flags, common_flags;
> - int ret = test_platform_param(pcdev, buswidth, &bus_flags);
> -
> - if (ret < 0)
> - return ret;
> + /*
> + * If the requested media bus configuration has not been fully applied
> + * make sure it is supported by the platform.
> + *
> + * PXA does not support V4L2_MBUS_DATA_ACTIVE_LOW and the bus mastering
> + * roles should match.
> + */
> + if (cfg.flags != mbus_config) {
> + unsigned int pxa_mbus_role = mbus_config & (V4L2_MBUS_MASTER |
> + V4L2_MBUS_SLAVE);
> + if (pxa_mbus_role != (cfg.flags & (V4L2_MBUS_MASTER |
> + V4L2_MBUS_SLAVE))) {
> + dev_err(pcdev_to_dev(pcdev),
> + "Unsupported mbus configuration: bus mastering\n");
> + return -EINVAL;
> + }
>
> - ret = sensor_call(pcdev, video, g_mbus_config, &cfg);
> - if (!ret) {
> - common_flags = pxa_mbus_config_compatible(&cfg,
> - bus_flags);
> - if (!common_flags) {
> - dev_warn(pcdev_to_dev(pcdev),
> - "Flags incompatible: camera 0x%x, host 0x%lx\n",
> - cfg.flags, bus_flags);
> + if (cfg.flags & V4L2_MBUS_DATA_ACTIVE_LOW) {
> + dev_err(pcdev_to_dev(pcdev),
> + "Unsupported mbus configuration: DATA_ACTIVE_LOW\n");
> return -EINVAL;
> }
> - } else if (ret == -ENOIOCTLCMD) {
> - ret = 0;
> }
>
> - return ret;
> + pxa_camera_setup_cicr(pcdev, cfg.flags, pixfmt);
> +
> + return 0;
> }
>
> static const struct pxa_mbus_pixelfmt pxa_camera_formats[] = {
> @@ -1738,11 +1656,6 @@ static int pxa_camera_get_formats(struct v4l2_device *v4l2_dev,
> return 0;
> }
>
> - /* This also checks support for the requested bits-per-sample */
> - ret = pxa_camera_try_bus_param(pcdev, fmt->bits_per_sample);
> - if (ret < 0)
> - return 0;
> -
> switch (code.code) {
> case MEDIA_BUS_FMT_UYVY8_2X8:
> formats++;
>
next prev parent reply other threads:[~2020-07-20 8:55 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-17 14:53 [PATCH v8 00/10] v4l2-subdev: Introduce [g|s]et_mbus_format pad op Jacopo Mondi
2020-07-17 14:53 ` [PATCH v8 01/10] media: v4l2-subdev: Introduce [get|set]_mbus_config pad ops Jacopo Mondi
2020-07-17 14:53 ` [PATCH v8 02/10] media: i2c: Use the new get_mbus_config pad op Jacopo Mondi
2020-07-17 14:53 ` [PATCH v8 03/10] media: i2c: ov6650: Use new [get|set]_mbus_config ops Jacopo Mondi
2020-07-21 7:53 ` [PATCH v8.1 " Jacopo Mondi
2020-07-21 15:50 ` Janusz Krzysztofik
2020-07-17 14:53 ` [PATCH v8 04/10] media: pxa_camera: Use the new set_mbus_config op Jacopo Mondi
2020-07-20 8:55 ` Hans Verkuil [this message]
2020-07-17 14:53 ` [PATCH v8 05/10] media: v4l2-subdev: Remove [s|g]_mbus_config video ops Jacopo Mondi
2020-07-17 14:53 ` [PATCH v8 06/10] media: v4l2- mediabus: Add usage note for V4L2_MBUS_* Jacopo Mondi
2020-07-17 14:53 ` [PATCH v8 07/10] staging: media: imx: Update TODO entry Jacopo Mondi
2020-07-17 14:53 ` [PATCH v8 08/10] media: i2c: adv748x: Adjust TXA data lanes number Jacopo Mondi
2020-07-17 14:53 ` [PATCH v8 09/10] media: i2c: adv748x: Implement get_mbus_config Jacopo Mondi
2020-07-17 14:53 ` [PATCH v8 10/10] media: rcar-csi2: Negotiate data lanes number Jacopo Mondi
2020-07-17 14:59 ` [PATCH v8 00/10] v4l2-subdev: Introduce [g|s]et_mbus_format pad op Hans Verkuil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aee934c4-c5d4-ba7f-8989-dfb57c8eda2a@xs4all.nl \
--to=hverkuil-cisco@xs4all.nl \
--cc=dave.stevenson@raspberrypi.com \
--cc=hyun.kwon@xilinx.com \
--cc=jacopo+renesas@jmondi.org \
--cc=jmkrzyszt@gmail.com \
--cc=kieran.bingham@ideasonboard.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=robert.jarzmik@free.fr \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).