Re: [PATCH v4 15/19] media: i2c: add Maxim GMSL2/3 serializer and deserializer drivers

[Cosmin Tanislav <demonsingur@gmail.com>]

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.

...