Hi,
On 16/02/2023 17:53, Andy Shevchenko wrote:
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.
Sure, I'll add.
+ 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
Indeed, that's better.
+ static const struct v4l2_mbus_framefmt outformat = {
out_format
...
-out_unlock:
+out:
Why?
I think this was a mistake, I'll change it back.
...
+/*
+ * (Possible) TODOs
TODOs:
Ok...
+ *
+ * - 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?)
Sure, I can move it.
};
...
+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?
I'll do that.
+ *val = (v1 << 8) | v2;
+ be16_to_cpu() here.
Yep.
+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?!
I didn't debug it, but having only fwnode there will break the probing
(no match).
.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.
Well, the current code handles the case of num_entries == 0. I can
change it as you suggest, and first check if num_entries == 0 and also
start the loop from 1.
+ 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.
The number of ID registers? Yes, I can add a define.
...
+ priv->atr.aliases = devm_kcalloc(dev, table_size,
+ sizeof(struct atr_alias_table_entry),
sizeof(*priv->atr.aliases) ?
Sure.
+ GFP_KERNEL);
+ if (!priv->atr.aliases)
return -ENOMEM;
...
if (ret) {
if (ret != -EINVAL) {
- dev_err(dev,
- "rx%u: failed to read 'ti,strobe-pos': %d\n",
- nport, ret);
+ dev_err(dev, "rx%u: failed to read '%s': %d\n", nport,
+ "ti,strobe-pos", ret);
return ret;
}
} else if (strobe_pos < UB960_MIN_MANUAL_STROBE_POS ||
@@ -3512,8 +3403,8 @@ ub960_parse_dt_rxport_link_properties(struct ub960_data *priv,
ret = fwnode_property_read_u32(link_fwnode, "ti,eq-level", &eq_level);
if (ret) {
if (ret != -EINVAL) {
- dev_err(dev, "rx%u: failed to read 'ti,eq-level': %d\n",
- nport, ret);
+ dev_err(dev, "rx%u: failed to read '%s': %d\n", nport,
+ "ti,eq-level", ret);
return ret;
}
} else if (eq_level > UB960_MAX_EQ_LEVEL) {
Hmm, I noticed this one (and the one above) was missing return -EINVAL.
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
}
That's not the same. You'd also need to do:
if (!ret) {
// handle the retrieved value
}
which, I think, is not any clearer (perhaps more unclear).
What I could do is:
if (ret) {
if (ret != -EINVAL) {
dev_err(dev, "rx%u: failed to read '%s': %d\n", nport,
"ti,eq-level", ret);
return ret;
}
} else {
if (eq_level > UB960_MAX_EQ_LEVEL) {
dev_err(dev, "rx%u: illegal 'ti,eq-level' value: %d\n",
nport, eq_level);
return -EINVAL;
}
rxport->eq.manual_eq = true;
rxport->eq.manual.eq_level = eq_level;
}
Maybe the above style makes it clearer, as it clearly splits the "don't
have value" and "have value" branches.
...
+ 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",
};
?
Clang-format disagrees, but I agree with you ;).
Tomi
