Re: [PATCH v8 5/7] media: i2c: add DS90UB960 driver
From: Tomi Valkeinen
Date: Wed Jan 25 2023 - 06:35:06 EST
On 25/01/2023 12:13, Laurent Pinchart wrote:
Hi Tomi,
On Wed, Jan 25, 2023 at 09:39:57AM +0200, Tomi Valkeinen wrote:
On 24/01/2023 20:27, Laurent Pinchart wrote:
+ } else if (ret < 0) {
+ dev_err(dev, "rx%u: failed to read 'ti,cdr-mode': %d\n", nport,
If you moved the "ti,cdr-mode" to an argument, printed with %s, the same
format string would be used for the other properties below, and should
thus be de-duplicated by the compiler.
I'm not quite sure if this is a sensible optimization or not, but I did
it so that I introduce:
const char *read_err_str = "rx%u: failed to read '%s': %d\n";
static
and then use that in the function, which makes the lines much shorter
and, I think, a bit more readable.
If you use the same string literal multiple times, the compiler should
de-duplicate it automatically, so you don't have to create a variable
manually.
Yes, but I think this looked better, as it made the code look less
cluttered, and the point is more obvious. Otherwise, looking at the
code, seeing dev_dbg(dev, "Foo %s\n", "bar"); looks pretty weird.
I find
dev_dbg(dev, read_err_str, port, "ti,cdr-mode", ret);
less readable as I then have to look up the read_err_str string to
understand that line. I also wonder, in that case, if the compiler can
still warn if the format string doesn't match the argument types.
That's a good point, it doesn't.
+static void ub960_notify_unbind(struct v4l2_async_notifier *notifier,
+ struct v4l2_subdev *subdev,
+ struct v4l2_async_subdev *asd)
+{
+ struct ub960_rxport *rxport = to_ub960_asd(asd)->rxport;
+
+ rxport->source_sd = NULL;
Does this serve any purpose ? If not, I'd drop the unbind handler.
It makes sure we don't access the source subdev after it has been
unbound. I don't see much harm with this function, but can catch cleanup
errors.
Do you mean we'll crash on a NULL pointer dereference instead of
accessing freed memory if this happens ? I suppose it's marginally
better :-)
Generally speaking I think it's significantly better. Accessing freed
memory might go unnoticed for a long time, and might not cause any
errors or cause randomly some minor errors. Here we might not even be
accessing freed memory, as the source sd is probably still there, so
KASAN wouldn't catch it.
In this particular case it might not matter that much. The source_sd is
only used when starting streaming, so the chances are quite small that
we'd end up there after the unbind.
Still, I think it's a very good practice to NULL the pointers when
they're no longer valid.
Fine with me.
+}
[snip]
+static int ub960_create_subdev(struct ub960_data *priv)
+{
+ struct device *dev = &priv->client->dev;
+ unsigned int i;
+ int ret;
+
+ v4l2_i2c_subdev_init(&priv->sd, priv->client, &ub960_subdev_ops);
A blank line would be nice.
Ok.
+ v4l2_ctrl_handler_init(&priv->ctrl_handler, 1);
You create two controls.
Yep. Although I dropped TPG, so only one again.
+ priv->sd.ctrl_handler = &priv->ctrl_handler;
+
+ v4l2_ctrl_new_std_menu_items(&priv->ctrl_handler, &ub960_ctrl_ops,
+ V4L2_CID_TEST_PATTERN,
+ ARRAY_SIZE(ub960_tpg_qmenu) - 1, 0, 0,
+ ub960_tpg_qmenu);
+
+ v4l2_ctrl_new_int_menu(&priv->ctrl_handler, NULL, V4L2_CID_LINK_FREQ,
+ ARRAY_SIZE(priv->tx_link_freq) - 1, 0,
+ priv->tx_link_freq);
+
+ if (priv->ctrl_handler.error) {
+ ret = priv->ctrl_handler.error;
+ goto err_free_ctrl;
+ }
+
+ priv->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
+ V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_STREAMS;
+ priv->sd.entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
+ priv->sd.entity.ops = &ub960_entity_ops;
+
+ for (i = 0; i < priv->hw_data->num_rxports + priv->hw_data->num_txports; i++) {
+ priv->pads[i].flags = ub960_pad_is_sink(priv, i) ?
+ MEDIA_PAD_FL_SINK :
+ MEDIA_PAD_FL_SOURCE;
+ }
+
+ ret = media_entity_pads_init(&priv->sd.entity,
+ priv->hw_data->num_rxports +
+ priv->hw_data->num_txports,
:-(
I don't have strong opinion on this, but don't you find it a bit
confusing if a single argument spans multiple lines but without any indent?
With a quick look, this looks like a call with 4 arguments:
ret = media_entity_pads_init(&priv->sd.entity,
priv->hw_data->num_rxports +
priv->hw_data->num_txports,
priv->pads);
I suppose I'm used to it, so it appears more readable to me. It's also
the style used through most of the kernel. There's of course always the
option of storing the result of the computation in a local variable.
I'll be happy to indent like that if someone tells me how to configure
clang-format to do that =). I didn't figure it out.
Setting ContinuationIndentWidth to 0 "fixes" it, but I suspect it may
have other side effects.
Yes, it creates some funny indenting, like:
ret =
func(......);
This being said, running clang-format on this file gives me a diffstat
of 450 insertions(+), 365 deletions(-), so I don't think you can rely on
it blindly...
True, although I the bulk of those are with the #defines and structs.
Tomi