Re: [PATCH v8 5/7] media: i2c: add DS90UB960 driver

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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