| 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: <42fdeee8-a23e-4499-83c6-77d50418cff2@gmail.com>
Date: Tue, 24 Jun 2025 18:34:58 +0300
From: Cosmin Tanislav <demonsingur@...il.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
Niklas Söderlund <niklas.soderlund@...natech.se>,
Julien Massot <julien.massot@...labora.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Linus Walleij <linus.walleij@...aro.org>,
"open list:MAXIM GMSL2 SERIALIZERS AND DESERIALIZERS"
<linux-media@...r.kernel.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>, open list <linux-kernel@...r.kernel.org>,
"moderated list:ARM64 PORT (AARCH64 ARCHITECTURE)"
<linux-arm-kernel@...ts.infradead.org>,
"open list:STAGING SUBSYSTEM" <linux-staging@...ts.linux.dev>,
open "list:GPIO"
"SUBSYSTEM:Keyword:(devm_)?gpio_(request|free|direction|get|set)"
<linux-gpio@...r.kernel.org>, Cosmin Tanislav <cosmin.tanislav@...log.com>
Subject: Re: [PATCH v4 15/19] media: i2c: add Maxim GMSL2/3 serializer and
deserializer drivers
On 6/24/25 11:52 AM, Sakari Ailus wrote:
> Hi Cosmin,
>
> Thanks for the set.
>
> This patch is pretty massive. Could you split it into the serialiser and
> deserialiser frameworks and then invididual drivers using it (six-ish
> patches in total)?
>
I'll try to split it up for easier review, thanks.
I'll strip some of the context so I can answer easier.
The comments I stripped out are things I'll do without discussion.
...
>> +struct max9296a_chip_info {
>> + unsigned int max_register;
>> + unsigned int versions;
>> + unsigned int modes;
>> + unsigned int num_pipes;
>> + unsigned int pipe_hw_ids[MAX9296A_PIPES_NUM];
>> + unsigned int phy_hw_ids[MAX9296A_PHYS_NUM];
>> + unsigned int num_phys;
>> + unsigned int num_links;
>> + struct max_phys_configs phys_configs;
>> + bool use_atr;
>> + bool has_per_link_reset;
>> + bool phy0_lanes_0_1_on_second_phy;
>> + bool polarity_on_physical_lanes;
>> + bool needs_single_link_version;
>> + bool needs_unique_stream_id;
>> + bool supports_cphy;
>> + bool supports_phy_log;
>> + bool adjust_rlms;
>> + bool fix_tx_ids;
>> +
>> + enum max_gmsl_mode tpg_mode;
>> +
>> + int (*set_pipe_stream_id)(struct max_des *des, struct max_des_pipe
*pipe,
>> + unsigned int stream_id);
>> + int (*set_pipe_enable)(struct max_des *des, struct max_des_pipe *pipe,
>> + bool enable);
>> + int (*set_pipe_link)(struct max_des *des, struct max_des_pipe *pipe,
>> + struct max_des_link *link);
>> + int (*set_pipe_tunnel_phy)(struct max_des *des, struct
max_des_pipe *pipe,
>> + struct max_des_phy *phy);
>> + int (*set_pipe_tunnel_enable)(struct max_des *des, struct
max_des_pipe *pipe,
>> + bool enable);
>
> Given this many callbacks, having an operations struct for them seems
> appropriate.
>
Are you proposing adding another struct just to put these ops inside?
What I could do is use the existing struct max_des_ops and assign the
appropriate members in it, and then put whatever is left that's not
needed for the common framework in struct max9296a_chip_info, and add
a pointer to the struct max_des_ops instance it struct
max9296a_chip_info.
...
>> +static int max9296a_log_phy_status(struct max_des *des,
>> + struct max_des_phy *phy, const char *name)
>> +{
>> + struct max9296a_priv *priv = des_to_priv(des);
>> + unsigned int index = phy->index;
>> + unsigned int val;
>> + int ret;
>> +
>> + if (!priv->info->supports_phy_log)
>> + return 0;
>> +
>> + ret = regmap_read(priv->regmap, MAX9296A_MIPI_PHY18, &val);
>> + if (ret)
>> + return ret;
>> +
>> + pr_info("%s: \tcsi2_pkt_cnt: %lu\n", name,
>> + field_get(MAX9296A_MIPI_PHY18_CSI2_TX_PKT_CNT(index), val));
>> +
>> + ret = regmap_read(priv->regmap, MAX9296A_MIPI_PHY20(index), &val);
>> + if (ret)
>> + return ret;
>> +
>> + pr_info("%s: \tphy_pkt_cnt: %u\n", name, val);
>
> dev_info()?
>
I initially used pr_info() with the subdev name passed from core
framwork to be consistent with the rest of the prints which use
v4l2_info().
I'll see how using dev_info() looks like, it would be nice if it would
match the v4l2_info() format, even though v4l2-ctl --log-status doesn't
care since all it looks for is `START STATUS`.
>> +
>> + return 0;
>> +}
>> +
>> +static int max9296a_set_enable(struct max_des *des, bool enable)
>> +{
>> + struct max9296a_priv *priv = des_to_priv(des);
>> +
>> + return regmap_assign_bits(priv->regmap, MAX9296A_BACKTOP12,
>> + MAX9296A_BACKTOP12_CSI_OUT_EN, enable);
>> +}
>> +
>> +static int max9296a_init_phy(struct max_des *des, struct
max_des_phy *phy)
>> +{
>> + struct max9296a_priv *priv = des_to_priv(des);
>> + bool is_cphy = phy->bus_type == V4L2_MBUS_CSI2_CPHY;
>> + unsigned int num_data_lanes = phy->mipi.num_data_lanes;
>> + unsigned int dpll_freq = phy->link_frequency * 2;
>> + unsigned int num_hw_data_lanes;
>> + unsigned int hw_index = max9296a_phy_id(priv, phy);
>> + unsigned int index = phy->index;
>> + unsigned int used_data_lanes = 0;
>> + unsigned int val;
>
> For register values, please use a type that explicitly specifies the
number
> of bits, e.g. u32.
>
I used the type of the argument received by the regmap methods.
...
>> +static int max9296a_reset_link(struct max9296a_priv *priv, unsigned
int index)
>> +{
>> + unsigned int reg, mask;
>> +
>> + if (index == 0) {
>> + reg = MAX9296A_CTRL0;
>> + mask = MAX9296A_CTRL0_RESET_ONESHOT;
>> + } else {
>> + reg = MAX9296A_CTRL2;
>> + mask = MAX9296A_CTRL2_RESET_ONESHOT_B;
>> + }
>
> I might use an array for this.
>
> Is index guaranteed to be 0 or 1?
>
Should be, yes, but using an array for just two indices would cause a
bit of indirection when looking at the code. If it was more I would have
used one.
>> +
>> + return regmap_set_bits(priv->regmap, reg, mask);
>> +}
>> +
>> +static int max9296a_init_link_rlms(struct max9296a_priv *priv,
>> + struct max_des_link *link)
>> +{
>> + unsigned int index = link->index;
>> + int ret;
>> +
>> + /*
>> + * These settings are described as required on datasheet page 53
>> + * for MAX96714.
>> + */
>> +
>> + ret = regmap_write(priv->regmap, MAX9296A_RLMS3E(index), 0xfd);
>> + if (ret)
>> + return ret;
>
> You could also do:
>
> if (!ret)
> ret = ...;
>
> And return ret at the end. It's one line less per call. Up to you.
>
>> +
>> + ret = regmap_write(priv->regmap, MAX9296A_RLMS3F(index), 0x3d);
>
> What are these magic numbers? Could we have human-readable names for
them?
>
> The register names seem pretty opaque, too. Some explanation here would
> seem reasonable.
>
They're extracted from the datasheet of MAX96714, the values are
undocumented, but they're necessary to "optimize performance" of the
GMSL link. I'll add a comment stating the datasheet page and section.
...
>> diff --git a/drivers/media/i2c/maxim-serdes/max_des.c
b/drivers/media/i2c/maxim-serdes/max_des.c
>> new file mode 100644
>> index 000000000000..6a45f42fe033
>> --- /dev/null
>> +++ b/drivers/media/i2c/maxim-serdes/max_des.c
>> @@ -0,0 +1,3108 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Maxim GMSL2 Deserializer Driver
>
> How about naming the file maxim-deserialiser? I'd use e.g. "maxim_des"
> prefix for the functions. Just "max" is quite generic.
>
max_des seems explicit enough. The max naming comes from the common
prefix in the name of the chips, eg: MAX96724, MAX96717, not from Maxim.
If I intended it to come from maxim, I would have obviously used maxim_,
since that's what vendor-prefixes.yaml specifies.
Although they were initially made by Maxim, Analog Devices bought Maxim,
so using maxim_ would make as much sense as using adi_.
Switching to maxim_ would only lengthen the name of the functions,
types, symbols, macros, etc, and at least register/masks macros are
already extremely long.
...
>> +static int max_des_parse_src_dt_endpoint(struct max_des_priv *priv,
>> + struct max_des_phy *phy,
>> + struct fwnode_handle *fwnode)
>> +{
>> + struct max_des *des = priv->des;
>> + u32 pad = max_des_phy_to_pad(des, phy);
>> + struct v4l2_fwnode_endpoint v4l2_ep = { .bus_type =
V4L2_MBUS_UNKNOWN };
>> + struct v4l2_mbus_config_mipi_csi2 *mipi = &v4l2_ep.bus.mipi_csi2;
>> + enum v4l2_mbus_type bus_type;
>> + struct fwnode_handle *ep;
>> + u64 link_frequency;
>> + unsigned int i;
>> + int ret;
>> +
>> + ep = fwnode_graph_get_endpoint_by_id(fwnode, pad, 0, 0);
>> + if (!ep)
>> + return 0;
>> +
>> + ret = v4l2_fwnode_endpoint_alloc_parse(ep, &v4l2_ep);
>> + fwnode_handle_put(ep);
>> + if (ret) {
>> + dev_err(priv->dev, "Could not parse endpoint on port %u\n", pad);
>> + return ret;
>> + }
>> +
>> + bus_type = v4l2_ep.bus_type;
>> + if (bus_type != V4L2_MBUS_CSI2_DPHY &&
>> + bus_type != V4L2_MBUS_CSI2_CPHY) {
>> + v4l2_fwnode_endpoint_free(&v4l2_ep);
>> + dev_err(priv->dev, "Unsupported bus-type %u on port %u\n",
>> + pad, bus_type);
>> + return -EINVAL;
>> + }
>> +
>> + ret = 0;
>
> ret is already 0 here.
>
>> + if (v4l2_ep.nr_of_link_frequencies == 0)
>> + link_frequency = MAX_DES_LINK_FREQUENCY_DEFAULT;
>
> Isn't this required in DT?
>
Required from a schema standpoint or a code standpoint?
Code:
rval = fwnode_property_count_u64(fwnode, "link-frequencies");
if (rval > 0) {
}
No rval <= 0 case.
And I don't think I made it required in the schema.
...
Powered by blists - more mailing lists