On Thu, Feb 16, 2023 at 04:07:39PM +0200, Tomi Valkeinen wrote:

...

+	if (!c2a)

I would expect here dev_warn() to let user know about "shouldn't happened, but
have happened" situation.

+		return; /* This shouldn't happen */

...

-	static const struct v4l2_mbus_framefmt format = {
+	static const struct v4l2_mbus_framefmt informat = {

Naming a bit confusing. Is it "information" that cut or what?

in_format

+	static const struct v4l2_mbus_framefmt outformat = {

out_format

...

-out_unlock:
+out:

Why?

...

+/*
+ * (Possible) TODOs

TODOs:

+ *
+ * - PM for serializer and remote peripherals. We need to manage:
+ *   - VPOC
+ *     - Power domain? Regulator? Somehow any remote device should be able to
+ *       cause the VPOC to be turned on.
+ *   - Link between the deserializer and the serializer
+ *     - Related to VPOC management. We probably always want to turn on the VPOC
+ *       and then enable the link.
+ *   - Serializer's services: i2c, gpios, power
+ *     - The serializer needs to resume before the remote peripherals can
+ *       e.g. use the i2c.
+ *     - How to handle gpios? Reserving a gpio essentially keeps the provider
+ *       (serializer) always powered on.
+ * - Do we need a new bus for the FPD-Link? At the moment the serializers
+ *   are children of the same i2c-adapter where the deserializer resides.
+ * - i2c-atr could be made embeddable instead of allocatable.
+ */

...

 struct atr_alias_table_entry {
 	u16 alias_id;	/* Alias ID from DT */
 
-	bool reserved;
+	bool in_use;
 	u8 nport;
 	u8 slave_id;	/* i2c client's local i2c address */
 	u8 port_reg_idx;

Wouldn't be wiser to move boolean at the end so if any obscure
architecture/compiler makes it longer than a byte it won't increase the memory
footprint. (Actually wouldn't it be aligned to u16 followed by u8 as well as
they are different types?)

 };

...

+static int ub960_read16(struct ub960_data *priv, u8 reg, u16 *val)
+{
+	struct device *dev = &priv->client->dev;
+	unsigned int v1, v2;
+	int ret;
+
+	mutex_lock(&priv->reg_lock);
+
+	ret = regmap_read(priv->regmap, reg, &v1);
+	if (ret) {
+		dev_err(dev, "%s: cannot read register 0x%02x (%d)!\n",
+			__func__, reg, ret);
+		goto out_unlock;
+	}
+
+	ret = regmap_read(priv->regmap, reg + 1, &v2);
+	if (ret) {
+		dev_err(dev, "%s: cannot read register 0x%02x (%d)!\n",
+			__func__, reg + 1, ret);
+		goto out_unlock;
+	}

Wondering why bulk read can't be used against properly typed __be16 variable?

+	*val = (v1 << 8) | v2;

+ be16_to_cpu() here.

+out_unlock:
+	mutex_unlock(&priv->reg_lock);
+
+	return ret;
+}

...

+static int ub960_rxport_read16(struct ub960_data *priv, u8 nport, u8 reg,
+			       u16 *val)
 {

Ditto.

+}

...

 	struct i2c_board_info ser_info = {
-		.of_node = to_of_node(rxport->remote_fwnode),
-		.fwnode = rxport->remote_fwnode,

+		.of_node = to_of_node(rxport->ser.fwnode),
+		.fwnode = rxport->ser.fwnode,

Why do you need to have both?!

 		.platform_data = ser_pdata,
 	};

...

+	for (nport = 0; nport < priv->hw_data->num_rxports; ++nport) {

Pre-increment is non-standard in the kernel.

+		struct ub960_rxport *rxport = priv->rxports[nport];
+		struct v4l2_mbus_frame_desc desc;
+		int ret;
+		u8 cur_vc;
+
+		if (!rxport)
+			continue;
+
+		ret = v4l2_subdev_call(rxport->source.sd, pad, get_frame_desc,
+				       rxport->source.pad, &desc);
+		if (ret)
+			return ret;
+
+		if (desc.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
+			continue;

		cur_vc = desc.entry[0].bus.csi2.vc;

+		for (i = 0; i < desc.num_entries; ++i) {
+			u8 vc = desc.entry[i].bus.csi2.vc;

+			if (i == 0) {
+				cur_vc = vc;
+				continue;
+			}

This is an invariant to the loop, see above.

+			if (vc == cur_vc)
+				continue;
+
+			dev_err(&priv->client->dev,
+				"rx%u: source with multiple virtual-channels is not supported\n",
+				nport);
+			return -ENODEV;
+		}
+	}

...

+	for (i = 0; i < 6; ++i)
 		ub960_read(priv, UB960_SR_FPD3_RX_ID(i), &id[i]);
 	id[6] = 0;

Wondering if this magic can be defined.

...

+	priv->atr.aliases = devm_kcalloc(dev, table_size,
+					 sizeof(struct atr_alias_table_entry),

	sizeof(*priv->atr.aliases) ?

+					 GFP_KERNEL);
+	if (!priv->atr.aliases)
 		return -ENOMEM;

...

Seems like you may do (in both cases) similar to the above:

	var = 0;
	ret = read_u32();
	if (ret && ret != -EINVAL) {
		// error handling
	}
	if (var > limit) {
		// another error handling
	}

...

+	static const char *vpoc_names[UB960_MAX_RX_NPORTS] = { "vpoc0", "vpoc1",
+							       "vpoc2", "vpoc3" };

Wouldn't be better to format it as

	static const char *vpoc_names[UB960_MAX_RX_NPORTS] = {
		"vpoc0", "vpoc1", "vpoc2", "vpoc3",
	};

?

-- 
With Best Regards,
Andy Shevchenko