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

Tomi Valkeinen
2023-01-25T11:34:50Z

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