Date:   Fri, 25 Feb 2022 15:18:10 +0200
From:   Sakari Ailus <sakari.ailus@....fi>
To:     Krzysztof Hałasa <khalasa@...p.pl>
Cc:     Jacopo Mondi <jacopo@...ndi.org>,
        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>,
        Joe Perches <joe@...ches.com>
Subject: Re: [PATCH v7 2/2] Driver for ON Semi AR0521 camera sensor

Hi Krysztof,

On Fri, Feb 25, 2022 at 01:15:11PM +0100, Krzysztof Hałasa wrote:
> 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

/* This is the preferred comment style */

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

The OF folks have shunned to the use of the iterative varants as that can
often lead to complicated parsing of the endpoints. As obtaining the
endpoint based on port and endpoint IDs works well in all cases I've
suggested people to use that. But as the backend, at least currently, uses
iterative functions, they're unlikely to disappear in the future.

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

If that is the case, then you can't use the bulk API.

That also requires the board to be wired in such a way that these
regulators have no other users.

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

I think you'll need pm_runtime_idle() call after enabling runtime PM.

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

-- 
Regard,s

Sakari Ailus

Powered by blists - more mailing lists