Date:   Wed, 25 Jan 2023 09:39:57 +0200
From:   Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Wolfram Sang <wsa@...nel.org>,
        Luca Ceresoli <luca.ceresoli@...tlin.com>,
        Andy Shevchenko <andriy.shevchenko@...el.com>,
        Matti Vaittinen <Matti.Vaittinen@...rohmeurope.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Peter Rosin <peda@...ntia.se>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Michael Tretter <m.tretter@...gutronix.de>,
        Shawn Tu <shawnx.tu@...el.com>,
        Hans Verkuil <hverkuil@...all.nl>,
        Mike Pagano <mpagano@...too.org>,
        Krzysztof Hałasa <khalasa@...p.pl>,
        Marek Vasut <marex@...x.de>,
        Satish Nagireddy <satish.nagireddy@...cruise.com>
Subject: Re: [PATCH v8 5/7] media: i2c: add DS90UB960 driver

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.

>>>> +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.

>>>> +}
> 
> [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.

>>>> +				     priv->pads);
>>>> +	if (ret)
>>>> +		goto err_free_ctrl;
>>>> +
>>>> +	priv->sd.state_lock = priv->sd.ctrl_handler->lock;
>>>> +
>>>> +	ret = v4l2_subdev_init_finalize(&priv->sd);
>>>> +	if (ret)
>>>> +		goto err_entity_cleanup;
>>>> +
>>>> +	ret = ub960_v4l2_notifier_register(priv);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "v4l2 subdev notifier register failed: %d\n", ret);
>>>> +		goto err_free_state;
>>>> +	}
>>>> +
>>>> +	ret = v4l2_async_register_subdev(&priv->sd);
>>>> +	if (ret) {
>>>> +		dev_err(dev, "v4l2_async_register_subdev error: %d\n", ret);
>>>> +		goto err_unreg_notif;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +
>>>> +err_unreg_notif:
>>>> +	ub960_v4l2_notifier_unregister(priv);
>>>> +err_free_state:
>>>
>>> err_subdev_cleanup:
>>
>> Yep.
>>
>>>> +	v4l2_subdev_cleanup(&priv->sd);
>>>> +err_entity_cleanup:
>>>> +	media_entity_cleanup(&priv->sd.entity);
>>>> +err_free_ctrl:
>>>> +	v4l2_ctrl_handler_free(&priv->ctrl_handler);
>>>> +
>>>> +	return ret;
>>>> +}
> 
> [snip]
> 
>>>> +static int ub960_probe(struct i2c_client *client)
>>>> +{
>>>> +	struct device *dev = &client->dev;
>>>> +	struct ub960_data *priv;
>>>> +	int ret;
>>>> +
>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	priv->client = client;
>>>> +
>>>> +	priv->hw_data = device_get_match_data(dev);
>>>> +
>>>> +	mutex_init(&priv->reg_lock);
>>>> +	mutex_init(&priv->atr_alias_table.lock);
>>>> +
>>>> +	INIT_DELAYED_WORK(&priv->poll_work, ub960_handler_work);
>>>> +
>>>> +	/*
>>>> +	 * Initialize these to invalid values so that the first reg writes will
>>>> +	 * configure the target.
>>>> +	 */
>>>> +	priv->current_indirect_target = 0xff;
>>>> +	priv->current_read_rxport = 0xff;
>>>> +	priv->current_write_rxport_mask = 0xff;
>>>> +	priv->current_read_csiport = 0xff;
>>>> +	priv->current_write_csiport_mask = 0xff;
>>>> +
>>>> +	ret = ub960_get_hw_resources(priv);
>>>> +	if (ret)
>>>> +		goto err_mutex_destroy;
>>>> +
>>>> +	ret = ub960_enable_core_hw(priv);
>>>> +	if (ret)
>>>> +		goto err_mutex_destroy;
>>>> +
>>>> +	/* release GPIO lock */
>>>> +	if (priv->hw_data->is_ub9702)
>>>> +		ub960_update_bits(priv, UB960_SR_RESET,
>>>> +				  UB960_SR_RESET_GPIO_LOCK_RELEASE,
>>>> +				  UB960_SR_RESET_GPIO_LOCK_RELEASE);
>>>
>>> Could this be moved to ub960_enable_core_hw() ?
>>
>> Yes.
>>
>>>> +
>>>> +	ret = ub960_parse_dt(priv);
>>>> +	if (ret)
>>>> +		goto err_disable_core_hw;
>>>> +
>>>> +	ret = ub960_init_tx_ports(priv);
>>>> +	if (ret)
>>>> +		goto err_free_ports;
>>>> +
>>>> +	ret = ub960_rxport_enable_vpocs(priv);
>>>> +	if (ret)
>>>> +		goto err_free_ports;
>>>> +
>>>> +	ret = ub960_init_rx_ports(priv);
>>>> +	if (ret)
>>>> +		goto err_disable_vpocs;
>>>> +
>>>> +	ub960_reset(priv, false);
>>>> +
>>>> +	ub960_rxport_wait_locks(priv, GENMASK(3, 0), NULL);
>>>> +
>>>> +	/*
>>>> +	 * Clear any errors caused by switching the RX port settings while
>>>> +	 * probing.
>>>> +	 */
>>>> +	ub960_clear_rx_errors(priv);
>>>> +
>>>> +	ret = ub960_init_atr(priv);
>>>> +	if (ret)
>>>> +		goto err_disable_vpocs;
>>>> +
>>>> +	ret = ub960_rxport_add_serializers(priv);
>>>> +	if (ret)
>>>> +		goto err_uninit_atr;
>>>> +
>>>> +	ret = ub960_create_subdev(priv);
>>>> +	if (ret)
>>>> +		goto err_free_sers;
>>>> +
>>>> +	if (client->irq)
>>>> +		dev_warn(dev, "irq support not implemented, using polling\n");
>>>
>>> That's not nice :-( Can it be fixed ? I'm OK if you do so on top.
>>
>> Fixed? You mean implemented? I don't have HW, so I'd rather leave it to
>> someone who has.
> 
> Yes, I meant implemented. The fact that we wake up the system every
> 500ms for I2C transfers isn't great, although I suppose in systems that
> use FPD-Link, that may not matter that much.

I agree, polling is annoying. But again, when there's a platform that 
uses IRQs, I think irq handling can be added (and tested) easily.

  Tomi

Powered by blists - more mailing lists