| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <641421a6-bf42-49f4-8f94-9cb0bce8e97c@linaro.org>
Date: Thu, 15 Jan 2026 00:07:53 +0200
From: Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>
To: Wenmeng Liu <wenmeng.liu@....qualcomm.com>, Robert Foss
<rfoss@...nel.org>, Todor Tomov <todor.too@...il.com>,
Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
Mauro Carvalho Chehab <mchehab@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v8 1/3] media: qcom: camss: Add common TPG support
Hi Wenmeng.
On 1/14/26 14:18, Wenmeng Liu wrote:
>
> Hi Vladimir,
>
>
> On 1/14/2026 1:05 PM, Vladimir Zapolskiy wrote:
>> Hi Wenmeng.
>>
>> On 1/14/26 05:04, Wenmeng Liu wrote:
>>> Hi Vladimir,
>>>
>>> On 1/14/2026 12:27 AM, Vladimir Zapolskiy wrote:
>>>> Hello Wenmeng.
>>>>
>>>> On 1/13/26 11:03, Wenmeng Liu wrote:
>>>>> Introduce a new common Test Pattern Generator (TPG) implementation for
>>>>> Qualcomm CAMSS. This module provides a generic interface for pattern
>>>>> generation that can be reused by multiple platforms.
>>>>>
>>>>> Unlike CSID-integrated TPG, this TPG acts as a standalone block
>>>>> that emulates both CSIPHY and sensor behavior, enabling flexible test
>>>>> patterns without external hardware.
>>>>>
>>>>> Signed-off-by: Wenmeng Liu <wenmeng.liu@....qualcomm.com>
>>>>> ---
>>>>> drivers/media/platform/qcom/camss/Makefile | 1 +
>>>>> drivers/media/platform/qcom/camss/camss-tpg.c | 710 ++++++++++++++++
>>>>> ++++++++++
>>>>> drivers/media/platform/qcom/camss/camss-tpg.h | 127 +++++
>>>>> drivers/media/platform/qcom/camss/camss.h | 5 +
>>>>> 4 files changed, 843 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/
>>>>> media/platform/qcom/camss/Makefile
>>>>> index
>>>>> 5e349b4915130c71dbff90e73102e46dfede1520..d355e67c25700ac061b878543c32ed8defc03ad0 100644
>>>>> --- a/drivers/media/platform/qcom/camss/Makefile
>>>>> +++ b/drivers/media/platform/qcom/camss/Makefile
>>>>> @@ -27,5 +27,6 @@ qcom-camss-objs += \
>>>>> camss-vfe.o \
>>>>> camss-video.o \
>>>>> camss-format.o \
>>>>> + camss-tpg.o \
>>>>
>>>> While you're here, please sort and keep the lines in alphabetical order.
>>> ACK.
>>>
>>>>
>>>>> obj-$(CONFIG_VIDEO_QCOM_CAMSS) += qcom-camss.o
>>>>> diff --git a/drivers/media/platform/qcom/camss/camss-tpg.c b/drivers/
>>>>> media/platform/qcom/camss/camss-tpg.c
>>>>> new file mode 100644
>>>>> index
>>>>> 0000000000000000000000000000000000000000..f4c015aafa202e5b64fafa3c543128fda6440b11
>>>>> --- /dev/null
>>>>> +++ b/drivers/media/platform/qcom/camss/camss-tpg.c
>>>>> @@ -0,0 +1,710 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + *
>>>>> + * Qualcomm MSM Camera Subsystem - TPG Module
>>>>> + *
>>>>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>>>>> + */
>>>>> +#include <linux/clk.h>
>>>>> +#include <linux/delay.h>
>>>>> +#include <linux/io.h>
>>>>> +#include <linux/kernel.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <linux/pm_runtime.h>
>>>>> +#include <media/media-entity.h>
>>>>> +#include <media/v4l2-device.h>
>>>>> +#include <media/v4l2-subdev.h>
>>>>> +
>>>>> +#include "camss-tpg.h"
>>>>> +#include "camss.h"
>>>>> +
>>>>> +const char * const testgen_payload_modes[] = {
>>>>> + "Disabled",
>>>>> + "Incrementing",
>>>>> + "Alternating 0x55/0xAA",
>>>>> + "Reserved",
>>>>> + "Reserved",
>>>>> + "Pseudo-random Data",
>>>>> + "User Specified",
>>>>> + "Reserved",
>>>>> + "Reserved",
>>>>> + "Color bars",
>>>>> + "Reserved"
>>>>
>>>> It makes little sense to mention the unsupported values, and then
>>>> introduce enum tpg_testgen_mode to list the supported ones.
>>>>
>>> This is for ctrl menu, will do as follow:
>>> static const char * const testgen_payload_modes[] = {
>>> [TPG_PAYLOAD_MODE_DISABLED] = "Disabled",
>>> [TPG_PAYLOAD_MODE_INCREMENTING] = "Incrementing",
>>> [TPG_PAYLOAD_MODE_ALTERNATING_55_AA] = "Alternating
>>> 0x55/0xAA",
>>> [TPG_PAYLOAD_MODE_RANDOM] = "Pseudo-random Data",
>>> [TPG_PAYLOAD_MODE_USER_SPECIFIED] = "User Specified",
>>> [TPG_PAYLOAD_MODE_COLOR_BARS] = "Color bars",
>>> };
>>>
>>
>> This is also not perfect, still userspace is misinformed about a number
>> of possible TPG modes vs. a number of actually supported TPG modes.
>>
> 0x0: INCREMENTING
> 0x1: ALTERNATING_55_AA
> 0x4: RANDOM
> 0x5: USER_SPECIFIED
> 0x8: COLOR_BARS
>
> These values come from the register configuration, these pattern values
> are consistent with the CSID TPG.
Userspace should not be aware of such low level details as register values,
there are many abstraction layers in-between to hide this type of information.
Writing proper values to registers should be a concern on the driver level,
it sounds improper to push this simple task and responsibility to userspace.
> If want to make it continuous, need to add a mapping table. How about this?
Yes, please.
>>>>> +};
>>>>
>>>> Are these test pattern modes specific to TPG Gen1 only?
>>>>
>>>> CSID TPG uses a different 'csid_testgen_modes' list, and if the list
>>>> above is
>>>> TPG Gen1 specific, it would make sense to place it right in camss-tpg-
>>>> gen1.c
>>>>
>>>
>>> Like other CAMSS nodes, the files placed in the core are meant to
>>> maintain consistency with the others.
>>
>> Please elaborate, what does it mean "files placed in the core"?
>>
> sorry, let me clarify this,
> core file: camss-cisd.c camss-csiphy.c ...
> hw version file: camss-csid-gen2.c camss-tpg-gen1.c
>> If technically possible, all local data shall be placed closer to the code,
>> which uses it, and here the exported data is exported for nothing, but
>> some "consistency".
>>
>> You draw a line what data to place into the generic camss-tpg.c and to the
>> specific camss-tpg-gen1.c, you should explain why data specific to TPG Gen1
>> is placed into the generic code, while you've already set a different and
>> dedicated place/file for storing this type of data?
>>
> The reason I want to implement it this way is that the TPG's ctrl
> requires this structure for initialization (msm_tpg_register_entity).
This function is generic, but data is specific, so let it be placed in
its specific location.
>
> Also it`s ok to push it to gen1 file.
>
Yes, please, thank you, I belive it will simplify the code.
>>>>> +
>>>>> +static const struct tpg_format_info formats_gen1[] = {
>>>>> + {
>>>>> + MEDIA_BUS_FMT_SBGGR8_1X8,
>>>>> + DATA_TYPE_RAW_8BIT,
>>>>
>>>> Please replace it with MIPI_CSI2_DT_RAW8
>>>>
>>>
>>> ACK.>> + ENCODE_FORMAT_UNCOMPRESSED_8_BIT,
>>>>> + 8,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_SGBRG8_1X8,
>>>>> + DATA_TYPE_RAW_8BIT,
>>>>> + ENCODE_FORMAT_UNCOMPRESSED_8_BIT,
>>>>> + 8,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_SGRBG8_1X8,
>>>>> + DATA_TYPE_RAW_8BIT,
>>>>> + ENCODE_FORMAT_UNCOMPRESSED_8_BIT,
>>>>> + 8,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_SRGGB8_1X8,
>>>>> + DATA_TYPE_RAW_8BIT,
>>>>> + ENCODE_FORMAT_UNCOMPRESSED_8_BIT,
>>>>> + 8,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_SBGGR10_1X10,
>>>>> + DATA_TYPE_RAW_10BIT,
>>>>
>>>> MIPI_CSI2_DT_RAW10>
>>>
>>> ACK.>> + ENCODE_FORMAT_UNCOMPRESSED_10_BIT,
>>>>> + 10,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_SGBRG10_1X10,
>>>>> + DATA_TYPE_RAW_10BIT,
>>>>> + ENCODE_FORMAT_UNCOMPRESSED_10_BIT,
>>>>> + 10,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_SGRBG10_1X10,
>>>>> + DATA_TYPE_RAW_10BIT,
>>>>> + ENCODE_FORMAT_UNCOMPRESSED_10_BIT,
>>>>> + 10,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_SRGGB10_1X10,
>>>>> + DATA_TYPE_RAW_10BIT,
>>>>> + ENCODE_FORMAT_UNCOMPRESSED_10_BIT,
>>>>> + 10,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_SBGGR12_1X12,
>>>>> + DATA_TYPE_RAW_12BIT,
>>>>
>>>> MIPI_CSI2_DT_RAW12
>>>>
>>>
>>> ACK.>> + ENCODE_FORMAT_UNCOMPRESSED_12_BIT,
>>>>> + 12,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_SGBRG12_1X12,
>>>>> + DATA_TYPE_RAW_12BIT,
>>>>> + ENCODE_FORMAT_UNCOMPRESSED_12_BIT,
>>>>> + 12,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_SGRBG12_1X12,
>>>>> + DATA_TYPE_RAW_12BIT,
>>>>> + ENCODE_FORMAT_UNCOMPRESSED_12_BIT,
>>>>> + 12,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_SRGGB12_1X12,
>>>>> + DATA_TYPE_RAW_12BIT,
>>>>> + ENCODE_FORMAT_UNCOMPRESSED_12_BIT,
>>>>> + 12,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_Y8_1X8,
>>>>> + DATA_TYPE_RAW_8BIT,
>>>>> + ENCODE_FORMAT_UNCOMPRESSED_8_BIT,
>>>>> + 8,
>>>>> + },
>>>>> + {
>>>>> + MEDIA_BUS_FMT_Y10_1X10,
>>>>> + DATA_TYPE_RAW_10BIT,
>>>>> + ENCODE_FORMAT_UNCOMPRESSED_10_BIT,
>>>>> + 10,
>>>>> + },
>>>>> +};
>>>>> +
>>>>> +const struct tpg_formats tpg_formats_gen1 = {
>>>>> + .nformats = ARRAY_SIZE(formats_gen1),
>>>>> + .formats = formats_gen1
>>>>> +};
>>>>> +
>>>>> +const struct tpg_format_info *tpg_get_fmt_entry(struct tpg_device
>>>>> *tpg,
>>>>> + const struct tpg_format_info *formats,
>>>>> + unsigned int nformats,
>>>>> + u32 code)
>>>>> +{
>>>>> + struct device *dev = tpg->camss->dev;
>>>>> + size_t i;
>>>>
>>>> unsigned int i, size_t is very unexpected to get here.
>>>>
>>>
>>> I have received comments on this.
>>>
>>> https://lore.kernel.org/all/449ac3c3-1f6a-4e69-899d-
>>> c4e4577714a4@....qualcomm.com/
>>>
>>> https://staticthinking.wordpress.com/2022/06/01/unsigned-int-i-is-
>>> stupid/>>
>>> +
>>
>> I see, in every programming language without type inference, including
>> C, types
>> shall be as precise as possible. Here the usage of signed int or size_t
>> is stupid,
>> because for a human being it adds completely wasted efforts to
>> comprehend, what
>> does happen, when the local variable is negative, while it just can not
>> be such.
>>
> ACK, will fix it.
>
>>>>> + for (i = 0; i < nformats; i++)
>>>>> + if (code == formats[i].code)
>>>>> + return &formats[i];
>>>>> +
>>>>> + dev_warn(dev, "Unknown pixel format code=0x%08x\n", code);
>>>>
>>>> Please remove dev_warn() completely, it opens a way to flood the kernel
>>>> log.
>>>>
>>>
>>> This is an exception printout and will not appear under normal
>>> circumstances.>> +
>>
>> If it can be trivially triggered from the userspace, it is not an
>> exception.
>>
>> I haven't yet tested the changes, what does happen, when an unsupported
>> pixel
>> format is asked to be set from userspace? It's a regular operation, and it
>> shall not litter the kernel log buffer.
>>
>>>>> + return ERR_PTR(-EINVAL);
>>
>> This one is sufficient.
>>
>
> ACK.>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * tpg_set_clock_rates - set clock rates on tpg module
>>>>> + * @tpg: tpg device
>>>>> + */
>>>>> +static int tpg_set_clock_rates(struct tpg_device *tpg)
>>>>> +{
>>>>> + struct device *dev = tpg->camss->dev;
>>>>> + int ret;
>>>>> + int i;
>>>>> +
>>>>> + for (i = 0; i < tpg->nclocks; i++) {
>>>>> + struct camss_clock *clock = &tpg->clock[i];
>>>>> + long round_rate;
>>>>> +
>>>>> + if (clock->freq) {
>>>>> + round_rate = clk_round_rate(clock->clk, clock->freq[0]);
>>>>> + if (round_rate < 0) {
>>>>> + dev_err(dev, "clk round rate failed: %ld\n",
>>>>> + round_rate);
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + ret = clk_set_rate(clock->clk, round_rate);
>>>>> + if (ret < 0) {
>>>>> + dev_err(dev, "clk set rate failed: %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * tpg_set_power - Power on/off tpg module
>>>>> + * @sd: tpg V4L2 subdevice
>>>>> + * @on: Requested power state
>>>>> + *
>>>>> + * Return 0 on success or a negative error code otherwise
>>>>> + */
>>>>> +static int tpg_set_power(struct v4l2_subdev *sd, int on)
>>>>> +{
>>>>> + struct tpg_device *tpg = v4l2_get_subdevdata(sd);
>>>>> + struct device *dev = tpg->camss->dev;
>>>>> +
>>>>> + if (on) {
>>>>> + int ret;
>>>>> +
>>>>> + ret = pm_runtime_resume_and_get(dev);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>> +
>>>>> + ret = tpg_set_clock_rates(tpg);
>>>>> + if (ret < 0) {
>>>>> + pm_runtime_put_sync(dev);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + ret = camss_enable_clocks(tpg->nclocks, tpg->clock, dev);
>>>>> + if (ret < 0) {
>>>>> + pm_runtime_put_sync(dev);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + tpg->res->hw_ops->reset(tpg);
>>>>> +
>>>>> + tpg->res->hw_ops->hw_version(tpg);
>>>>> + } else {
>>>>> + camss_disable_clocks(tpg->nclocks, tpg->clock);
>>>>> +
>>>>> + pm_runtime_put_sync(dev);
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * tpg_set_stream - Enable/disable streaming on tpg module
>>>>> + * @sd: tpg V4L2 subdevice
>>>>> + * @enable: Requested streaming state
>>>>> + *
>>>>> + * Return 0 on success or a negative error code otherwise
>>>>> + */
>>>>> +static int tpg_set_stream(struct v4l2_subdev *sd, int enable)
>>>>> +{
>>>>> + struct tpg_device *tpg = v4l2_get_subdevdata(sd);
>>>>> + int ret = 0;
>>>>> +
>>>>> + if (enable) {
>>>>> + ret = v4l2_ctrl_handler_setup(&tpg->ctrls);
>>>>> + if (ret < 0) {
>>>>> + dev_err(tpg->camss->dev,
>>>>> + "could not sync v4l2 controls: %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + ret = tpg->res->hw_ops->configure_stream(tpg, enable);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * __tpg_get_format - Get pointer to format structure
>>>>> + * @tpg: tpg device
>>>>> + * @cfg: V4L2 subdev pad configuration
>>>>
>>>> There is no such function argument. There are much more errors in
>>>> the doxygen descriptions of functions, please remove all these
>>>> doxygen comments, they do not bring anything valuable here.
>>>>
>>>
>>>
>>> ACK.
>>
>> For clarity, I haven't commented all errors in the doxygen descriptions,
>> because I expect that all of them will be removed.
>>
>
> Yes, I will removed it all.
>
>>>> + * @pad: pad from which format is requested
>>>>> + * @which: TRY or ACTIVE format
>>>>> + *
>>>>> + * Return pointer to TRY or ACTIVE format structure
>>>>> + */
>>>>> +static struct v4l2_mbus_framefmt *
>>>>> +__tpg_get_format(struct tpg_device *tpg,
>>>>> + struct v4l2_subdev_state *sd_state,
>>>>> + unsigned int pad,
>>>>> + enum v4l2_subdev_format_whence which)
>>>>> +{
>>>>> + if (which == V4L2_SUBDEV_FORMAT_TRY)
>>>>> + return v4l2_subdev_state_get_format(sd_state,
>>>>> + pad);
>>>>> +
>>>>> + return &tpg->fmt[pad];
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * tpg_try_format - Handle try format by pad subdev method
>>>>> + * @tpg: tpg device
>>>>> + * @cfg: V4L2 subdev pad configuration
>>>>
>>>> No such argument.
>>>>
>>>
>>> ACK.>> + * @pad: pad on which format is requested
>>>>> + * @fmt: pointer to v4l2 format structure
>>>>> + * @which: wanted subdev format
>>>>> + */
>>>>> +static void tpg_try_format(struct tpg_device *tpg,
>>>>> + struct v4l2_subdev_state *sd_state,
>>>>> + unsigned int pad,
>>>>> + struct v4l2_mbus_framefmt *fmt,
>>>>> + enum v4l2_subdev_format_whence which)
>>>>> +{
>>>>> + unsigned int i;
>>>>> +
>>>>> + switch (pad) {
>>>>> + case MSM_TPG_PAD_SINK:
>>>>> + for (i = 0; i < tpg->res->formats->nformats; i++)
>>>>> + if (tpg->res->formats->formats[i].code == fmt->code)
>>>>> + break;
>>>>> +
>>>>> + /* If not found, use SBGGR8 as default */
>>>>> + if (i >= tpg->res->formats->nformats)
>>>>> + fmt->code = MEDIA_BUS_FMT_SBGGR8_1X8;
>>>>> +
>>>>> + fmt->width = clamp_t(u32, fmt->width, 1, 8191);
>>>>> + fmt->height = clamp_t(u32, fmt->height, 1, 8191);
>>>>> +
>>>>> + fmt->field = V4L2_FIELD_NONE;
>>>>> + fmt->colorspace = V4L2_COLORSPACE_SRGB;
>>>>> +
>>>>> + break;
>>>>> + case MSM_TPG_PAD_SRC:
>>>>> + *fmt = *__tpg_get_format(tpg, sd_state,
>>>>> + MSM_TPG_PAD_SINK,
>>>>> + which);
>>>>> +
>>>>> + break;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * tpg_enum_mbus_code - Handle format enumeration
>>>>> + * @sd: tpg V4L2 subdevice
>>>>> + * @cfg: V4L2 subdev pad configuration
>>>>
>>>> No such argument.
>>>>
>>>
>>> ACK.>> + * @code: pointer to v4l2_subdev_mbus_code_enum structure
>>>>> + * return -EINVAL or zero on success
>>>>> + */
>>>>> +static int tpg_enum_mbus_code(struct v4l2_subdev *sd,
>>>>> + struct v4l2_subdev_state *sd_state,
>>>>> + struct v4l2_subdev_mbus_code_enum *code)
>>>>> +{
>>>>> + struct tpg_device *tpg = v4l2_get_subdevdata(sd);
>>>>> + struct v4l2_mbus_framefmt *format;
>>>>> +
>>>>> + if (code->pad == MSM_TPG_PAD_SINK) {
>>>>> + if (code->index >= tpg->res->formats->nformats)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + code->code = tpg->res->formats->formats[code->index].code;
>>>>> + } else {
>>>>> + if (code->index > 0)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + format = __tpg_get_format(tpg, sd_state,
>>>>> + MSM_TPG_PAD_SINK,
>>>>> + code->which);
>>>>> +
>>>>> + code->code = format->code;
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * tpg_enum_frame_size - Handle frame size enumeration
>>>>> + * @sd: tpg V4L2 subdevice
>>>>> + * @cfg: V4L2 subdev pad configuration
>>>>> + * @fse: pointer to v4l2_subdev_frame_size_enum structure
>>>>> + * return -EINVAL or zero on success
>>>>> + */
>>>>> +static int tpg_enum_frame_size(struct v4l2_subdev *sd,
>>>>> + struct v4l2_subdev_state *sd_state,
>>>>> + struct v4l2_subdev_frame_size_enum *fse)
>>>>> +{
>>>>> + struct tpg_device *tpg = v4l2_get_subdevdata(sd);
>>>>> + struct v4l2_mbus_framefmt format;
>>>>> +
>>>>> + if (fse->index != 0)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + format.code = fse->code;
>>>>> + format.width = 1;
>>>>> + format.height = 1;
>>>>> + tpg_try_format(tpg, sd_state, fse->pad, &format, fse->which);
>>>>> + fse->min_width = format.width;
>>>>> + fse->min_height = format.height;
>>>>> +
>>>>> + if (format.code != fse->code)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + format.code = fse->code;
>>>>> + format.width = -1;
>>>>> + format.height = -1;
>>>>
>>>> format.width and format.height are rewritten in the tpg_try_format(),
>>>> so it makes no sense to assign them.
>>>>
>>>
>>> This is use for pass parameters in.
>>>
>>>> The problem is that for whatever reason you can tpg_try_format() twice
>>>> in a raw, it looks wrong, and I'm certain you can modify the functions
>>>> so that only one call would be needed.
>>>> The first call is to get the supported minimum size, and the second call
>>> is to get the maximum size.
>>>
>>> The tpg_try_format function can be used multiple times; I don't think
>>> it's necessary to write a new interface.
>>
>> I didn't ask to write a new interface, please reread my comment.
>>
>> Yon can write tpg_enum_frame_size() function, that tpg_try_format()
>> is not called at all, but for an unclear reason here it's called twice.
>>
> Sure, it will be implemented in a simpler way.
>
>>>>> + tpg_try_format(tpg, sd_state, fse->pad, &format, fse->which);
>>>>> + fse->max_width = format.width;
>>>>> + fse->max_height = format.height;
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * tpg_get_format - Handle get format by pads subdev method
>>>>> + * @sd: tpg V4L2 subdevice
>>>>> + * @cfg: V4L2 subdev pad configuration
>>>>> + * @fmt: pointer to v4l2 subdev format structure
>>>>> + *
>>>>> + * Return -EINVAL or zero on success
>>>>> + */
>>>>> +static int tpg_get_format(struct v4l2_subdev *sd,
>>>>> + struct v4l2_subdev_state *sd_state,
>>>>> + struct v4l2_subdev_format *fmt)
>>>>> +{
>>>>> + struct tpg_device *tpg = v4l2_get_subdevdata(sd);
>>>>> + struct v4l2_mbus_framefmt *format;
>>>>> +
>>>>> + format = __tpg_get_format(tpg, sd_state, fmt->pad, fmt->which);
>>>>> + if (!format)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + fmt->format = *format;
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * tpg_set_format - Handle set format by pads subdev method
>>>>> + * @sd: tpg V4L2 subdevice
>>>>> + * @cfg: V4L2 subdev pad configuration
>>>>> + * @fmt: pointer to v4l2 subdev format structure
>>>>> + *
>>>>> + * Return -EINVAL or zero on success
>>>>> + */
>>>>> +static int tpg_set_format(struct v4l2_subdev *sd,
>>>>> + struct v4l2_subdev_state *sd_state,
>>>>> + struct v4l2_subdev_format *fmt)
>>>>> +{
>>>>> + struct tpg_device *tpg = v4l2_get_subdevdata(sd);
>>>>> + struct v4l2_mbus_framefmt *format;
>>>>> +
>>>>> + format = __tpg_get_format(tpg, sd_state, fmt->pad, fmt->which);
>>>>> + if (!format)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + tpg_try_format(tpg, sd_state, fmt->pad, &fmt->format,
>>>>> + fmt->which);
>>>>> + *format = fmt->format;
>>>>> +
>>>>> + if (fmt->pad == MSM_TPG_PAD_SINK) {
>>>>> + format = __tpg_get_format(tpg, sd_state,
>>>>> + MSM_TPG_PAD_SRC,
>>>>> + fmt->which);
>>>>> +
>>>>> + *format = fmt->format;
>>>>> + tpg_try_format(tpg, sd_state, MSM_TPG_PAD_SRC,
>>>>> + format,
>>>>> + fmt->which);
>>>>> + }
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * tpg_init_formats - Initialize formats on all pads
>>>>> + * @sd: tpg V4L2 subdevice
>>>>> + * @fh: V4L2 subdev file handle
>>>>> + *
>>>>> + * Initialize all pad formats with default values.
>>>>> + *
>>>>> + * Return 0 on success or a negative error code otherwise
>>>>> + */
>>>>> +static int tpg_init_formats(struct v4l2_subdev *sd,
>>>>> + struct v4l2_subdev_fh *fh)
>>>>> +{
>>>>> + struct v4l2_subdev_format format = {
>>>>> + .pad = MSM_TPG_PAD_SINK,
>>>>> + .which = fh ? V4L2_SUBDEV_FORMAT_TRY :
>>>>> + V4L2_SUBDEV_FORMAT_ACTIVE,
>>>>> + .format = {
>>>>> + .code = MEDIA_BUS_FMT_SBGGR8_1X8,
>>>>> + .width = 1920,
>>>>> + .height = 1080
>>>>> + }
>>>>> + };
>>>>> +
>>>>> + return tpg_set_format(sd, fh ? fh->state : NULL, &format);
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * tpg_set_test_pattern - Set test generator's pattern mode
>>>>> + * @tpg: TPG device
>>>>> + * @value: desired test pattern mode
>>>>> + *
>>>>> + * Return 0 on success or a negative error code otherwise
>>>>> + */
>>>>> +static int tpg_set_test_pattern(struct tpg_device *tpg, s32 value)
>>>>> +{
>>>>> + return tpg->res->hw_ops->configure_testgen_pattern(tpg, value);
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * tpg_s_ctrl - Handle set control subdev method
>>>>> + * @ctrl: pointer to v4l2 control structure
>>>>> + *
>>>>> + * Return 0 on success or a negative error code otherwise
>>>>> + */
>>>>> +static int tpg_s_ctrl(struct v4l2_ctrl *ctrl)
>>>>> +{
>>>>> + struct tpg_device *tpg = container_of(ctrl->handler,
>>>>> + struct tpg_device, ctrls);
>>>>> + int ret = -EINVAL;
>>>>> +
>>>>> + switch (ctrl->id) {
>>>>> + case V4L2_CID_TEST_PATTERN:
>>>>> + ret = tpg_set_test_pattern(tpg, ctrl->val);
>>>>> + break;
>>>>> + }
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static const struct v4l2_ctrl_ops tpg_ctrl_ops = {
>>>>> + .s_ctrl = tpg_s_ctrl,
>>>>> +};
>>>>> +
>>>>> +/*
>>>>> + * msm_tpg_subdev_init - Initialize tpg device structure and resources
>>>>> + * @tpg: tpg device
>>>>
>>>> And no 'camss' argument. The whole function description comment is quite
>>>> useless, it can be just removed with no losses.
>>>>
>>> ACK. will remove it all.
>>
>> Good, thank you.
>>
>>>> + * @res: tpg module resources table
>>>>> + * @id: tpg module id
>>>>> + *
>>>>> + * Return 0 on success or a negative error code otherwise
>>>>> + */
>>>>> +int msm_tpg_subdev_init(struct camss *camss,
>>>>> + struct tpg_device *tpg,
>>>>> + const struct camss_subdev_resources *res, u8 id)
>>>>> +{
>>>>> + struct platform_device *pdev;
>>>>> + struct device *dev;
>>>>> + int i, j;
>>>>> +
>>>>> + dev = camss->dev;
>>>>> + pdev = to_platform_device(dev);
>>>>> +
>>>>> + tpg->camss = camss;
>>>>> + tpg->id = id;
>>>>> + tpg->res = &res->tpg;
>>>>> + tpg->res->hw_ops->subdev_init(tpg);
>>>>> +
>>>>> + tpg->base = devm_platform_ioremap_resource_byname(pdev, res-
>>>>>> reg[0]);
>>>>> + if (IS_ERR(tpg->base))
>>>>> + return PTR_ERR(tpg->base);
>>>>> +
>>>>> + tpg->nclocks = 0;
>>>>> + while (res->clock[tpg->nclocks])
>>>>> + tpg->nclocks++;
>>>>> +
>>>>> + if (tpg->nclocks) {
>>>>
>>>> if (!tpg->nclocks)
>>>> return 0;
>>>>
>>> ACK.>> + tpg->clock = devm_kcalloc(dev,
>>>>> + tpg->nclocks, sizeof(*tpg->clock),
>>>>> + GFP_KERNEL);
>>>>> + if (!tpg->clock)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + for (i = 0; i < tpg->nclocks; i++) {
>>>>> + struct camss_clock *clock = &tpg->clock[i];
>>>>> +
>>>>> + clock->clk = devm_clk_get(dev, res->clock[i]);
>>>>> + if (IS_ERR(clock->clk))
>>>>> + return PTR_ERR(clock->clk);
>>>>> +
>>>>> + clock->name = res->clock[i];
>>>>> +
>>>>> + clock->nfreqs = 0;
>>>>> + while (res->clock_rate[i][clock->nfreqs])
>>>>> + clock->nfreqs++;
>>>>> +
>>>>> + if (!clock->nfreqs) {
>>>>> + clock->freq = NULL;
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>> + clock->freq = devm_kcalloc(dev,
>>>>> + clock->nfreqs,
>>>>> + sizeof(*clock->freq),
>>>>> + GFP_KERNEL);
>>>>> + if (!clock->freq)
>>>>> + return -ENOMEM;
>>>>> +
>>>>> + for (j = 0; j < clock->nfreqs; j++)
>>>>> + clock->freq[j] = res->clock_rate[i][j];
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * tpg_link_setup - Setup tpg connections
>>>>> + * @entity: Pointer to media entity structure
>>>>> + * @local: Pointer to local pad
>>>>> + * @remote: Pointer to remote pad
>>>>> + * @flags: Link flags
>>>>> + *
>>>>> + * Return 0 on success
>>>>> + */
>>>>> +static int tpg_link_setup(struct media_entity *entity,
>>>>> + const struct media_pad *local,
>>>>> + const struct media_pad *remote, u32 flags)
>>>>> +{
>>>>> + if (flags & MEDIA_LNK_FL_ENABLED)
>>>>> + if (media_pad_remote_pad_first(local))
>>>>> + return -EBUSY;
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static const struct v4l2_subdev_core_ops tpg_core_ops = {
>>>>> + .s_power = tpg_set_power,
>>>>> +};
>>>>> +
>>>>> +static const struct v4l2_subdev_video_ops tpg_video_ops = {
>>>>> + .s_stream = tpg_set_stream,
>>>>> +};
>>>>> +
>>>>> +static const struct v4l2_subdev_pad_ops tpg_pad_ops = {
>>>>> + .enum_mbus_code = tpg_enum_mbus_code,
>>>>> + .enum_frame_size = tpg_enum_frame_size,
>>>>> + .get_fmt = tpg_get_format,
>>>>> + .set_fmt = tpg_set_format,
>>>>> +};
>>>>> +
>>>>> +static const struct v4l2_subdev_ops tpg_v4l2_ops = {
>>>>> + .core = &tpg_core_ops,
>>>>> + .video = &tpg_video_ops,
>>>>> + .pad = &tpg_pad_ops,
>>>>> +};
>>>>> +
>>>>> +static const struct v4l2_subdev_internal_ops tpg_v4l2_internal_ops = {
>>>>> + .open = tpg_init_formats,
>>>>> +};
>>>>> +
>>>>> +static const struct media_entity_operations tpg_media_ops = {
>>>>> + .link_setup = tpg_link_setup,
>>>>> + .link_validate = v4l2_subdev_link_validate,
>>>>> +};
>>>>> +
>>>>> +/*
>>>>> + * msm_tpg_register_entity - Register subdev node for tpg module
>>>>> + * @tpg: tpg device
>>>>> + * @v4l2_dev: V4L2 device
>>>>> + *
>>>>> + * Return 0 on success or a negative error code otherwise
>>>>> + */
>>>>> +int msm_tpg_register_entity(struct tpg_device *tpg,
>>>>> + struct v4l2_device *v4l2_dev)
>>>>> +{
>>>>> + struct v4l2_subdev *sd = &tpg->subdev;
>>>>> + struct media_pad *pads = tpg->pads;
>>>>> + struct device *dev = tpg->camss->dev;
>>>>> + int ret;
>>>>> +
>>>>> + v4l2_subdev_init(sd, &tpg_v4l2_ops);
>>>>> + sd->internal_ops = &tpg_v4l2_internal_ops;
>>>>> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>>>>> + V4L2_SUBDEV_FL_HAS_EVENTS;
>>>>> + snprintf(sd->name, ARRAY_SIZE(sd->name), "%s%d",
>>>>> + MSM_TPG_NAME, tpg->id);
>>>>> + sd->grp_id = TPG_GUP_ID;
>>>>> + v4l2_set_subdevdata(sd, tpg);
>>>>> +
>>>>> + ret = v4l2_ctrl_handler_init(&tpg->ctrls, 1);
>>>>> + if (ret < 0) {
>>>>> + dev_err(dev, "Failed to init ctrl handler: %d\n", ret);
>>>>> + return ret;
>>>>> + }
>>>>> +
>>>>> + tpg->testgen_mode = v4l2_ctrl_new_std_menu_items(&tpg->ctrls,
>>>>> + &tpg_ctrl_ops, V4L2_CID_TEST_PATTERN,
>>>>> + tpg->testgen.nmodes, 0, 0,
>>>>> + tpg->testgen.modes);
>>>>> +
>>>>
>>>> Please remove a blank line above.
>>>>
>>> ACK.>> + if (tpg->ctrls.error) {
>>>>> + dev_err(dev, "Failed to init ctrl: %d\n", tpg->ctrls.error);
>>>>> + ret = tpg->ctrls.error;
>>>>> + goto free_ctrl;
>>>>> + }
>>>>> +
>>>>> + tpg->subdev.ctrl_handler = &tpg->ctrls;
>>>>> +
>>>>> + ret = tpg_init_formats(sd, NULL);
>>>>> + if (ret < 0) {
>>>>> + dev_err(dev, "Failed to init format: %d\n", ret);
>>>>> + goto free_ctrl;
>>>>> + }
>>>>> +
>>>>> + pads[MSM_TPG_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>>>>> + pads[MSM_TPG_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE;
>>>>
>>>> Why do you need two pads for TPG?
>>>>
>>> will fix it next version.
>>>
>>>>> +
>>>>> + sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
>>>>
>>>> TPG is not a video pixel encoding converter device.
>>>>
>>>
>>> How about MEDIA_ENT_F_CAM_SENSOR?
>>>
>>
>> Still not perfect, but probably it's better... At least it won't implicitly
>> assume a presense of two pads on the media entity.
>>
>
> Or just deleted it, if want to keep this,
> will choose MEDIA_ENT_F_CAM_SENSOR.
Formally a TPG is not a sensor, so it could be misleading for a user/reader.
Since no selected function seems an applicable choice, please go on with it.
>
> and I will delete all sink code in this driver.
>
Yes, please, thank you!
>>>>> + sd->entity.ops = &tpg_media_ops;
>>>>> + ret = media_entity_pads_init(&sd->entity, MSM_TPG_PADS_NUM, pads);
>>>>> + if (ret < 0) {
>>>>> + dev_err(dev, "Failed to init media entity: %d\n", ret);
>>>>> + goto free_ctrl;
>>>>> + }
>>>>> +
>>>>> + ret = v4l2_device_register_subdev(v4l2_dev, sd);
>>>>> + if (ret < 0) {
>>>>> + dev_err(dev, "Failed to register subdev: %d\n", ret);
>>>>> + media_entity_cleanup(&sd->entity);
>>>>> + goto free_ctrl;
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +
>>>>> +free_ctrl:
>>>>> + v4l2_ctrl_handler_free(&tpg->ctrls);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * msm_tpg_unregister_entity - Unregister tpg module subdev node
>>>>> + * @tpg: tpg device
>>>>> + */
>>>>> +void msm_tpg_unregister_entity(struct tpg_device *tpg)
>>>>> +{
>>>>> + v4l2_device_unregister_subdev(&tpg->subdev);
>>>>> + media_entity_cleanup(&tpg->subdev.entity);
>>>>> + v4l2_ctrl_handler_free(&tpg->ctrls);
>>>>> +}
>>>>> diff --git a/drivers/media/platform/qcom/camss/camss-tpg.h b/drivers/
>>>>> media/platform/qcom/camss/camss-tpg.h
>>>>> new file mode 100644
>>>>> index
>>>>> 0000000000000000000000000000000000000000..1a16addac19418f2f11d0b8abb1c865c99888bde
>>>>> --- /dev/null
>>>>> +++ b/drivers/media/platform/qcom/camss/camss-tpg.h
>>>>> @@ -0,0 +1,127 @@
>>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>>> +/*
>>>>> + * camss-tpg.h
>>>>> + *
>>>>> + * Qualcomm MSM Camera Subsystem - TPG Module
>>>>> + *
>>>>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights
>>>>> reserved.
>>>>> + */
>>>>> +#ifndef QC_MSM_CAMSS_TPG_H
>>>>> +#define QC_MSM_CAMSS_TPG_H
>>>>> +
>>>>> +#include <linux/clk.h>
>>>>> +#include <media/media-entity.h>
>>>>> +#include <media/v4l2-ctrls.h>
>>>>> +#include <media/v4l2-device.h>
>>>>> +#include <media/v4l2-mediabus.h>
>>>>> +#include <media/v4l2-subdev.h>
>>>>> +
>>>>> +#define MSM_TPG_PAD_SINK 0
>>>>> +#define MSM_TPG_PAD_SRC 1
>>>>> +#define MSM_TPG_PADS_NUM 2
>>>>> +
>>>>> +#define DATA_TYPE_RAW_8BIT 0x2a
>>>>> +#define DATA_TYPE_RAW_10BIT 0x2b
>>>>> +#define DATA_TYPE_RAW_12BIT 0x2c
>>>>
>>>> Remove all above to get the macro values from include/media/mipi-csi2.h
>>>>
>>> ACK.>> +
>>>>> +#define ENCODE_FORMAT_UNCOMPRESSED_8_BIT 0x1
>>>>> +#define ENCODE_FORMAT_UNCOMPRESSED_10_BIT 0x2
>>>>> +#define ENCODE_FORMAT_UNCOMPRESSED_12_BIT 0x3
>>>>> +#define ENCODE_FORMAT_UNCOMPRESSED_14_BIT 0x4
>>>>> +#define ENCODE_FORMAT_UNCOMPRESSED_16_BIT 0x5
>>>>> +#define ENCODE_FORMAT_UNCOMPRESSED_20_BIT 0x6
>>>>> +#define ENCODE_FORMAT_UNCOMPRESSED_24_BIT 0x7
>>>>> +
>>>>> +#define TPG_GUP_ID 0
>>
>> In addition to the previous review comments, please change the value to
>> something other than default 0. This type of data can be placed outside
>> of TPG specific code, it has a potential for further outbreak.
>>
> ACK.
>
Please move it to camss.h, a rename would be also appreciated like CAMSS_GID_TPG.
>>>>> +#define MSM_TPG_NAME "msm_tpg"
>>>>
>>>> Remove the macro, it's used only once in the code and more usecases are
>>>> not expected happen.
>>>>
>>> ACK.
>>
>>>> +
>>>>> +enum tpg_testgen_mode {
>>>>> + TPG_PAYLOAD_MODE_DISABLED = 0,
>>>>> + TPG_PAYLOAD_MODE_INCREMENTING = 1,
>>>>> + TPG_PAYLOAD_MODE_ALTERNATING_55_AA = 2,
>>>>> + TPG_PAYLOAD_MODE_RANDOM = 5,
>>>>> + TPG_PAYLOAD_MODE_USER_SPECIFIED = 6,
>>>>> + TPG_PAYLOAD_MODE_COLOR_BARS = 9,
>>>>> + TPG_PAYLOAD_MODE_NUM_SUPPORTED_GEN1 = 9,
>>>>> +};
>>>>> +
>>>>> +struct tpg_testgen_config {
>>>>> + enum tpg_testgen_mode mode;
>>>>> + const char * const*modes;
>>>>> + u8 nmodes;
>>>>> +};
>>>>> +
>>>>> +struct tpg_format_info {
>>>>> + u32 code;
>>>>> + u8 data_type;
>>>>> + u8 encode_format;
>>>>> + u8 bpp;
>>>>> +};
>>>>> +
>>>>> +struct tpg_formats {
>>>>> + unsigned int nformats;
>>>>> + const struct tpg_format_info *formats;
>>>>> +};
>>>>> +
>>>>> +struct tpg_device;
>>>>> +
>>>>> +struct tpg_hw_ops {
>>>>> + int (*configure_stream)(struct tpg_device *tpg, u8 enable);
>>>>> +
>>>>> + int (*configure_testgen_pattern)(struct tpg_device *tpg, s32 val);
>>>>> +
>>>>> + u32 (*hw_version)(struct tpg_device *tpg);
>>>>> +
>>>>> + int (*reset)(struct tpg_device *tpg);
>>>>> +
>>>>> + void (*subdev_init)(struct tpg_device *tpg);
>>>>> +};
>>>>> +
>>>>> +struct tpg_subdev_resources {
>>>>> + u8 lane_cnt;
>>>>> + u8 vc_cnt;
>>>>> + const struct tpg_formats *formats;
>>>>> + const struct tpg_hw_ops *hw_ops;
>>>>> +};
>>>>> +
>>>>> +struct tpg_device {
>>>>> + struct camss *camss;
>>>>> + u8 id;
>>>>> + struct v4l2_subdev subdev;
>>>>> + struct media_pad pads[MSM_TPG_PADS_NUM];
>>>>> + void __iomem *base;
>>>>> + struct camss_clock *clock;
>>>>> + int nclocks;
>>>>> + struct tpg_testgen_config testgen;
>>>>> + struct v4l2_mbus_framefmt fmt[MSM_TPG_PADS_NUM];
>>>>> + struct v4l2_ctrl_handler ctrls;
>>>>> + struct v4l2_ctrl *testgen_mode;
>>>>> + const struct tpg_subdev_resources *res;
>>>>> + const struct tpg_format *formats;
>>>>> + unsigned int nformats;
>>>>> + u32 hw_version;
>>>>> +};
>>>>> +
>>>>> +struct camss_subdev_resources;
>>>>> +
>>>>> +const struct tpg_format_info *tpg_get_fmt_entry(struct tpg_device
>>>>> *tpg,
>>>>> + const struct tpg_format_info *formats,
>>>>> + unsigned int nformats,
>>>>> + u32 code);
>>>>> +
>>>>> +int msm_tpg_subdev_init(struct camss *camss,
>>>>> + struct tpg_device *tpg,
>>>>> + const struct camss_subdev_resources *res, u8 id);
>>>>> +
>>>>> +int msm_tpg_register_entity(struct tpg_device *tpg,
>>>>> + struct v4l2_device *v4l2_dev);
>>>>> +
>>>>> +void msm_tpg_unregister_entity(struct tpg_device *tpg);
>>>>> +
>>>>> +extern const char * const testgen_payload_modes[];
>>>>> +
>>>>> +extern const struct tpg_formats tpg_formats_gen1;
>>>>> +
>>>>> +extern const struct tpg_hw_ops tpg_ops_gen1;
>>>>> +
>>>>> +#endif /* QC_MSM_CAMSS_TPG_H */
>>>>> diff --git a/drivers/media/platform/qcom/camss/camss.h b/drivers/
>>>>> media/platform/qcom/camss/camss.h
>>>>> index
>>>>> 9d9a62640e25dce0e8d45af9df01bbfd64b9bb4b..a892a87bed8bde8919200d6eac2b7a5338763c0e 100644
>>>>> --- a/drivers/media/platform/qcom/camss/camss.h
>>>>> +++ b/drivers/media/platform/qcom/camss/camss.h
>>>>> @@ -21,6 +21,7 @@
>>>>> #include "camss-csid.h"
>>>>> #include "camss-csiphy.h"
>>>>> #include "camss-ispif.h"
>>>>> +#include "camss-tpg.h"
>>>>> #include "camss-vfe.h"
>>>>> #include "camss-format.h"
>>>>> @@ -52,6 +53,7 @@ struct camss_subdev_resources {
>>>>> char *interrupt[CAMSS_RES_MAX];
>>>>> union {
>>>>> struct csiphy_subdev_resources csiphy;
>>>>> + struct tpg_subdev_resources tpg;
>>>>> struct csid_subdev_resources csid;
>>>>> struct vfe_subdev_resources vfe;
>>>>> };
>>>>> @@ -104,6 +106,7 @@ struct camss_resources {
>>>>> enum camss_version version;
>>>>> const char *pd_name;
>>>>> const struct camss_subdev_resources *csiphy_res;
>>>>> + const struct camss_subdev_resources *tpg_res;
>>>>> const struct camss_subdev_resources *csid_res;
>>>>> const struct camss_subdev_resources *ispif_res;
>>>>> const struct camss_subdev_resources *vfe_res;
>>>>> @@ -111,6 +114,7 @@ struct camss_resources {
>>>>> const struct resources_icc *icc_res;
>>>>> const unsigned int icc_path_num;
>>>>> const unsigned int csiphy_num;
>>>>> + const unsigned int tpg_num;
>>>>> const unsigned int csid_num;
>>>>> const unsigned int vfe_num;
>>>>> };
>>>>> @@ -121,6 +125,7 @@ struct camss {
>>>>> struct media_device media_dev;
>>>>> struct device *dev;
>>>>> struct csiphy_device *csiphy;
>>>>> + struct tpg_device *tpg;
>>>>> struct csid_device *csid;
>>>>> struct ispif_device *ispif;
>>>>> struct vfe_device *vfe;
>>>>>
>>>>
>>
> Thanks for your review.
You are welcome! I will try my best to keep on with the reviews.
--
Best wishes,
Vladimir
Powered by blists - more mailing lists