| 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: <edd6a420-9958-43a2-a761-bed5ec547591@gmail.com>
Date: Mon, 26 Jan 2026 13:10:23 +0200
From: Cosmin Tanislav <demonsingur@...il.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Ceclan Dumitru <mitrutzceclan@...il.com>
Cc: Sakari Ailus <sakari.ailus@...ux.intel.com>, dumitru.ceclan@...log.com,
Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Julien Massot <julien.massot@...labora.com>, Rob Herring <robh@...nel.org>,
Niklas Söderlund <niklas.soderlund@...natech.se>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-gpio@...r.kernel.org,
linux-staging@...ts.linux.dev
Subject: Re: [PATCH RESEND v8 17/21] media: i2c: maxim-serdes: add MAX9296A
driver
Hi Laurent, Sakari. As this is code written by me I can provide some
clarifications.
On 1/26/26 12:01 PM, Laurent Pinchart wrote:
> On Mon, Jan 26, 2026 at 11:55:47AM +0200, Ceclan Dumitru wrote:
>>
>>
>> On 1/20/26 3:34 PM, Sakari Ailus wrote:
>>> Hi Dumitru,
>>>
>>> On Mon, Dec 08, 2025 at 04:13:09PM +0200, Dumitru Ceclan via B4 Relay wrote:
>>>> + *ops = max9296a_common_ops;
>>>> +
>>>> + ops->versions = priv->info->ops->versions;
>>>> + ops->modes = priv->info->ops->modes;
>>>> + ops->needs_single_link_version = priv->info->ops->needs_single_link_version;
>>>> + ops->needs_unique_stream_id = priv->info->ops->needs_unique_stream_id;
>>>> + ops->fix_tx_ids = priv->info->ops->fix_tx_ids;
>>>> + ops->num_phys = priv->info->ops->num_phys;
>>>> + ops->num_pipes = priv->info->ops->num_pipes;
>>>> + ops->num_links = priv->info->ops->num_links;
>>>> + ops->phys_configs = priv->info->ops->phys_configs;
>>>> + ops->set_pipe_enable = priv->info->ops->set_pipe_enable;
>>>> + ops->set_pipe_stream_id = priv->info->ops->set_pipe_stream_id;
>>>> + ops->set_pipe_tunnel_phy = priv->info->ops->set_pipe_tunnel_phy;
>>>> + ops->set_pipe_tunnel_enable = priv->info->ops->set_pipe_tunnel_enable;
>>>> + ops->use_atr = priv->info->ops->use_atr;
>>>> + ops->tpg_mode = priv->info->ops->tpg_mode;
>>>
>>> What's the reason for doing these assignments and a copy of the memory? Why
>>> not to just keep a pointer to the struct memory instead? I think there's
>>> another case of the same.
Copy of the memory is to assign the common parts of the ops, while the
assignments add the ops that need to be chip-specific on top.
>>>
>> Would this be alright:
>> #define MAX9296A_COMMON_OPS \
>>
>> .num_remaps_per_pipe = 16, \
>>
>> .tpg_entries = { ... }, \
>>
>> .init = max9296a_init, \
>>
>> .set_enable = max9296a_set_enable, \
>>
>>
>> static const struct max_des_ops max9296a_ops = {
>>
>> MAX9296A_COMMON_OPS,
>>
>> .versions = BIT(MAX_SERDES_GMSL_2_3GBPS) |
>>
>> BIT(MAX_SERDES_GMSL_2_6GBPS),
>> .modes = BIT(MAX_SERDES_GMSL_PIXEL_MODE),
>> /* ... */
>>
>> };
>>
>>
>>
>> static int max9296a_probe(struct i2c_client *client)
>>
>> {
>>
>> /* ... */
>>
>> priv->des.ops = priv->info->ops;
>>
>> /* ... */
>>
>> }
>
> That's still a copy. Why is a copy needed, why can't you write
>
> priv->des.ops = &priv->info->ops;
>
priv->des.ops is a pointer, so is priv->info->ops, in the current code,
so this would not be a copy of the ops, just a pointer assignment.
See the struct definitions below.
struct max_des {
struct max_des_priv *priv;
const struct max_des_ops *ops;
...
};
struct max9296a_chip_info {
const struct max_des_ops *ops;
...
};
struct max9296a_priv {
struct max_des des;
const struct max9296a_chip_info *info;
...
};
> or event replace priv->des.ops with priv->info->ops through the code ?
max9296a_chip_info is the chip-specific struct for this driver, it's
retrieved using device_get_match_data().
max9296a_priv is the private driver data.
max_des is the data available for both the core framework and the
drivers implementing it.
max_des_priv is the private data of the core framework, containing
implementation details that are not really related to the chip itself
but more about how to expose the chip capabilities to the user,
eg. V4L2.
Because of this, priv->info->ops cannot be used through out the code
as priv is private to the driver, and so is info, while ops are used
in the core framework.
> Is there anything in the ops structure that needs to be modified at
> runtime ?
No, there is not. I think it would be fine for a MAX9296A_COMMON_OPS
macro be added which contains all the common ops, and then place it
as the first member of each instance of struct max_des_ops.
I'm sending some code inline as a proof-of-concept, maybe it would be
clearer how the current code will have to be changed. A similar thing
would have to be done for the other chip drivers.
Thank you for your feedback.
diff --git a/drivers/media/i2c/maxim-serdes/max9296a.c b/drivers/media/i2c/maxim-serdes/max9296a.c
index a67692b2f5ee0..e9d6322c302f0 100644
--- a/drivers/media/i2c/maxim-serdes/max9296a.c
+++ b/drivers/media/i2c/maxim-serdes/max9296a.c
@@ -1038,49 +1038,48 @@ static const struct max_serdes_tpg_entry max9296a_tpg_entries[] = {
MAX_TPG_ENTRY_1920X1080P60_RGB888,
};
-static const struct max_des_ops max9296a_common_ops = {
- .num_remaps_per_pipe = 16,
- .tpg_entries = {
- .num_entries = ARRAY_SIZE(max9296a_tpg_entries),
- .entries = max9296a_tpg_entries,
- },
- .tpg_patterns = BIT(MAX_SERDES_TPG_PATTERN_CHECKERBOARD) |
- BIT(MAX_SERDES_TPG_PATTERN_GRADIENT),
#ifdef CONFIG_VIDEO_ADV_DEBUG
- .reg_read = max9296a_reg_read,
+#define MAX9296A_COMMON_OPS_DEBUG \
+ .reg_read = max9296a_reg_read, \
.reg_write = max9296a_reg_write,
+#else
+#define MAX9296A_COMMON_OPS_DEBUG
#endif
- .log_pipe_status = max9626a_log_pipe_status,
- .log_phy_status = max9296a_log_phy_status,
- .set_enable = max9296a_set_enable,
- .init = max9296a_init,
- .init_phy = max9296a_init_phy,
- .set_phy_mode = max9296a_set_phy_mode,
- .set_phy_enable = max9296a_set_phy_enable,
- .set_pipe_remap = max9296a_set_pipe_remap,
- .set_pipe_remaps_enable = max9296a_set_pipe_remaps_enable,
- .set_pipe_mode = max9296a_set_pipe_mode,
- .set_tpg = max9296a_set_tpg,
- .select_links = max9296a_select_links,
- .set_link_version = max9296a_set_link_version,
-};
+
+#define MAX9296A_COMMON_OPS \
+ MAX9296A_COMMON_OPS_DEBUG \
+ .num_remaps_per_pipe = 16, \
+ .tpg_entries = { \
+ .num_entries = ARRAY_SIZE(max9296a_tpg_entries),\
+ .entries = max9296a_tpg_entries, \
+ }, \
+ .tpg_patterns = BIT(MAX_SERDES_TPG_PATTERN_CHECKERBOARD) |\
+ BIT(MAX_SERDES_TPG_PATTERN_GRADIENT), \
+ .log_pipe_status = max9626a_log_pipe_status, \
+ .log_phy_status = max9296a_log_phy_status, \
+ .set_enable = max9296a_set_enable, \
+ .init = max9296a_init, \
+ .init_phy = max9296a_init_phy, \
+ .set_phy_mode = max9296a_set_phy_mode, \
+ .set_phy_enable = max9296a_set_phy_enable, \
+ .set_pipe_remap = max9296a_set_pipe_remap, \
+ .set_pipe_remaps_enable = max9296a_set_pipe_remaps_enable,\
+ .set_pipe_mode = max9296a_set_pipe_mode, \
+ .set_tpg = max9296a_set_tpg, \
+ .select_links = max9296a_select_links, \
+ .set_link_version = max9296a_set_link_version
static int max9296a_probe(struct i2c_client *client)
{
struct regmap_config i2c_regmap = max9296a_i2c_regmap;
struct device *dev = &client->dev;
struct max9296a_priv *priv;
- struct max_des_ops *ops;
int ret;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
return -ENOMEM;
- ops = devm_kzalloc(dev, sizeof(*ops), GFP_KERNEL);
- if (!ops)
- return -ENOMEM;
-
priv->info = device_get_match_data(dev);
if (!priv->info) {
dev_err(dev, "Failed to get match data\n");
@@ -1110,24 +1109,7 @@ static int max9296a_probe(struct i2c_client *client)
usleep_range(4000, 5000);
}
- *ops = max9296a_common_ops;
-
- ops->versions = priv->info->ops->versions;
- ops->modes = priv->info->ops->modes;
- ops->needs_single_link_version = priv->info->ops->needs_single_link_version;
- ops->needs_unique_stream_id = priv->info->ops->needs_unique_stream_id;
- ops->fix_tx_ids = priv->info->ops->fix_tx_ids;
- ops->num_phys = priv->info->ops->num_phys;
- ops->num_pipes = priv->info->ops->num_pipes;
- ops->num_links = priv->info->ops->num_links;
- ops->phys_configs = priv->info->ops->phys_configs;
- ops->set_pipe_enable = priv->info->ops->set_pipe_enable;
- ops->set_pipe_stream_id = priv->info->ops->set_pipe_stream_id;
- ops->set_pipe_tunnel_phy = priv->info->ops->set_pipe_tunnel_phy;
- ops->set_pipe_tunnel_enable = priv->info->ops->set_pipe_tunnel_enable;
- ops->use_atr = priv->info->ops->use_atr;
- ops->tpg_mode = priv->info->ops->tpg_mode;
- priv->des.ops = ops;
+ priv->des.ops = priv->info->ops;
ret = max9296a_reset(priv);
if (ret)
@@ -1154,6 +1136,7 @@ static const struct max_serdes_phys_config max96714_phys_configs[] = {
};
static const struct max_des_ops max9296a_ops = {
+ MAX9296A_COMMON_OPS,
.versions = BIT(MAX_SERDES_GMSL_2_3GBPS) |
BIT(MAX_SERDES_GMSL_2_6GBPS),
.modes = BIT(MAX_SERDES_GMSL_PIXEL_MODE),
@@ -1181,6 +1164,7 @@ static const struct max9296a_chip_info max9296a_info = {
};
static const struct max_des_ops max96714_ops = {
+ MAX9296A_COMMON_OPS,
.versions = BIT(MAX_SERDES_GMSL_2_3GBPS) |
BIT(MAX_SERDES_GMSL_2_6GBPS),
.modes = BIT(MAX_SERDES_GMSL_PIXEL_MODE) |
@@ -1226,6 +1210,7 @@ static const struct max9296a_chip_info max96714_info = {
};
static const struct max_des_ops max96714f_ops = {
+ MAX9296A_COMMON_OPS,
.versions = BIT(MAX_SERDES_GMSL_2_3GBPS),
.modes = BIT(MAX_SERDES_GMSL_PIXEL_MODE) |
BIT(MAX_SERDES_GMSL_TUNNEL_MODE),
@@ -1254,6 +1239,7 @@ static const struct max9296a_chip_info max96714f_info = {
};
static const struct max_des_ops max96716a_ops = {
+ MAX9296A_COMMON_OPS,
.versions = BIT(MAX_SERDES_GMSL_2_3GBPS) |
BIT(MAX_SERDES_GMSL_2_6GBPS),
.modes = BIT(MAX_SERDES_GMSL_PIXEL_MODE) |
@@ -1286,6 +1272,7 @@ static const struct max9296a_chip_info max96716a_info = {
};
static const struct max_des_ops max96792a_ops = {
+ MAX9296A_COMMON_OPS,
.versions = BIT(MAX_SERDES_GMSL_2_3GBPS) |
BIT(MAX_SERDES_GMSL_2_6GBPS) |
BIT(MAX_SERDES_GMSL_3_12GBPS),
Powered by blists - more mailing lists