Date:   Fri, 25 Feb 2022 13:15:11 +0100
From:   Krzysztof Hałasa <khalasa@...p.pl>
To:     Jacopo Mondi <jacopo@...ndi.org>
Cc:     Mauro Carvalho Chehab <mchehab@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Sakari Ailus <sakari.ailus@....fi>,
        Joe Perches <joe@...ches.com>
Subject: Re: [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor

Hi Jacopo,

Sorry it took that long. Unfortunately I no longer have much time to
work on this, thus the delays.

Jacopo Mondi <jacopo@...ndi.org> writes:

>> +static int ar0521_s_ctrl(struct v4l2_ctrl *ctrl)
>> +{
>> +	struct v4l2_subdev *sd = ctrl_to_sd(ctrl);
>> +	struct ar0521_dev *sensor = to_ar0521_dev(sd);
>> +	int ret;
>> +
>> +	/* v4l2_ctrl_lock() locks our own mutex */
>> +
>> +	dev_dbg(&sensor->i2c_client->dev, "%s(0x%X)\n", __func__, ctrl->id);
>> +
>> +	switch (ctrl->id) {
>> +	case V4L2_CID_HBLANK:
>> +	case V4L2_CID_VBLANK:
>> +		sensor->total_width = sensor->fmt.width +
>> +			sensor->ctrls.hblank->val;
>> +		sensor->total_height = sensor->fmt.width +
>> +			sensor->ctrls.vblank->val;
>> +		break;
>> +	default:
>> +		ret = -EINVAL;
>> +		break;
>> +	}
>> +
>> +	// access the sensor only if it's powered up
>> +	if (!pm_runtime_get_if_in_use(&sensor->i2c_client->dev))
>
> As you correctly do not access the chip's registers if it's powered
> off, you have to call __v4l2_ctrl_handler_setup() at power on time to
> make sure controls are actually set.

These registers are also written in ar0521_set_stream(), isn't it
enough?

>> +	ctrls->hblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_HBLANK,
>> +					  AR0521_WIDTH_BLANKING_MIN, 4094, 1,
>> +					  AR0521_WIDTH_BLANKING_MIN);
>> +	ctrls->vblank = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_VBLANK,
>> +					  AR0521_HEIGHT_BLANKING_MIN, 4094, 2,
>> +					  AR0521_HEIGHT_BLANKING_MIN);
>> +	v4l2_ctrl_cluster(2, &ctrls->hblank);
>
> Is it intended to have vblank and hblank as cluster, can't they change
> independently ?

They can. It appears, though, that clusters aren't about controls which
can't change independently. Both of the two are written to the hardware
at the same time, which appears to be a valid reason to combine them
into a cluster.
This is similar to the gain/balance controls, and BTW the use of
clusters there was suggested by you.

Please correct me if I'm wrong.

>> +	/* Read-only */
>> +	ctrls->pixrate = v4l2_ctrl_new_std(hdl, ops, V4L2_CID_PIXEL_RATE,
>> +					   AR0521_PIXEL_CLOCK_MIN,
>> +					   AR0521_PIXEL_CLOCK_MAX, 1,
>> +					   AR0521_PIXEL_CLOCK_RATE);
>
> so you have to mark it as read-only

Oh, I'd be happy for this control to be R/W. Unfortunately the core
media core enforces R/O. This is only a comment about what the core code
does, currently at least.

>> +	dev_dbg(dev, "%s()\n", __func__);
>
> Rather useless, please drop. Same for all other debug functions that
> just print the current function name in other parts of the driver.
> While maybe useful when developing, they're mostly noise for users
> imo.

Fine, will drop the rest of the debug. In fact, I already dropped the
most useful parts (I2C register access debugging).

>> +	endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(dev), 0, 0,
>> +						   FWNODE_GRAPH_ENDPOINT_NEXT);
>
> The drivers supports a single endpoint right ? Then
> fwnode_graph_get_next_enpoint() can be used

Well, I originally used
fwnode_graph_get_next_endpoint(of_fwnode_handle(dev->of_node), NULL).
Sakari suggested I should use the above
fwnode_graph_get_endpoint_by_id().
It could be a good idea to agree on this. Not sure about the difference.

>> +	for (cnt = 0; cnt < ARRAY_SIZE(ar0521_supply_names); cnt++) {
>> +		struct regulator *supply = devm_regulator_get(dev,
>> +						ar0521_supply_names[cnt]);
>> +
>> +		if (IS_ERR(supply)) {
>> +			dev_info(dev, "no %s regulator found: %li\n",
>> +				 ar0521_supply_names[cnt], PTR_ERR(supply));
>> +			return PTR_ERR(supply);
>> +		}
>> +		sensor->supplies[cnt] = supply;
>> +	}
>
> Using the regulator bulk api would simplify this.
> See drivers/media/i2c/ccs/ccs-core.c

The bulk API doesn't allow for delays between individual supplies, does it?
The delays are mandated by the manufacturer.

>> +
>> +	mutex_init(&sensor->lock);
>> +
>> +	ret = ar0521_init_controls(sensor);
>> +	if (ret)
>> +		goto entity_cleanup;
>> +
>> +	ar0521_adj_fmt(&sensor->fmt);
>
> Called already here above.

Right, I will remove the first one.

>> +	ret = v4l2_async_register_subdev(&sensor->sd);
>> +	if (ret)
>> +		goto free_ctrls;
>> +
>> +	/* Turn on the device and enable runtime PM */
>> +	ret = ar0521_power_on(&client->dev);
>> +	if (ret)
>> +		goto disable;
>
> Does the device stay powered all the time ?

Depends on the hw, but the power could be disabled, yes.

> Doesn't your resume callback call power_on() already ?

It does, when the PM is enabled.

Should the above code be changed?

>> +static struct i2c_driver ar0521_i2c_driver = {
>> +	.driver = {
>> +		.name  = "ar0521",
>> +		.pm = &ar0521_pm_ops,
>> +		.of_match_table	= ar0521_dt_ids,
>> +	},
>> +	.probe    = ar0521_probe,
>
> You could use probe_new and drop the "const struct i2c_device_id *id"
> argument to probe()

You are right again.

That said, I wonder if it would be better (like easier) to have this
driver added to the staging area instead.
-- 
Krzysztof "Chris" Hałasa

Sieć Badawcza Łukasiewicz
Przemysłowy Instytut Automatyki i Pomiarów PIAP
Al. Jerozolimskie 202, 02-486 Warszawa

Powered by blists - more mailing lists