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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Cosmin,

On 6/24/25 18:34, Cosmin Tanislav wrote:
> On 6/24/25 11:52 AM, Sakari Ailus wrote:
>  >> +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.

Ah, I missed this wasn't specific to a device. Please ignore the comment.

...

>  >> +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`.

Ok. Up to you.

...

>  >> +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.

Ack.

> 
> ...
> 
>  >> +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.

Ack.

> 
>  >> +
>  >> +    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.

Ack, sounds good.

> 
> ...
> 
>  >> 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.

Yeah, the prefix Maxim uses for their chips is a bit unfortunate in 
naming context. I guess it's ok as it's not part of the UAPI.

> 
> 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.

I'd do that. The device requires this configuration to operate anyway. 
Also the concern of EMI may be lesser in environments serdes devices are 
used but I wouldn't say it's not there.

-- 
Regards,

Sakari Ailus