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@xxxxxxxxxx>
> ---
> 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++;
>
