From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 4A029C4332F for ; Wed, 1 Nov 2023 15:36:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234584AbjKAPgk (ORCPT ); Wed, 1 Nov 2023 11:36:40 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:36600 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234557AbjKAPgi (ORCPT ); Wed, 1 Nov 2023 11:36:38 -0400 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 480F0DA for ; Wed, 1 Nov 2023 08:36:30 -0700 (PDT) Received: from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi [213.243.189.158]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 0398BAE; Wed, 1 Nov 2023 16:36:11 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1698852972; bh=E1984onHQ6hGbrkjprVNdqs7xHsxoS2pfFgyNq1F0cY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=m1AgiH0aQcLCc2r70Qd5zCoQYrsfevbdR/jNiEzLD0wPXUGvC7hldHNSRAqWyIcJl 6608SU0hjy/P7+VESzphjkZIAE3F8Vh60Xw5qnSGRpTXHzgsfty+9NOOF3ytCSLgbx t66UTaWKJwp/kgkzJwWUusJegbuGyhAF2xOtQnSQ= Date: Wed, 1 Nov 2023 17:36:35 +0200 From: Laurent Pinchart To: Sakari Ailus Cc: linux-media@vger.kernel.org, "Cao, Bingbu" , Hans de Goede , Daniel Scally , Mauro Carvalho Chehab , Andy Shevchenko , Kate Hsuan , hverkuil@xs4all.nl Subject: Re: [PATCH 1/1] media: Documentation: Initialisation finishes before subdev registration Message-ID: <20231101153635.GA12764@pendragon.ideasonboard.com> References: <20231101080546.1145527-1-sakari.ailus@linux.intel.com> <20231101125350.GL12764@pendragon.ideasonboard.com> <20231101134506.GP12764@pendragon.ideasonboard.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Wed, Nov 01, 2023 at 02:08:32PM +0000, Sakari Ailus wrote: > On Wed, Nov 01, 2023 at 03:45:06PM +0200, Laurent Pinchart wrote: > > On Wed, Nov 01, 2023 at 01:31:13PM +0000, Sakari Ailus wrote: > > > On Wed, Nov 01, 2023 at 02:53:50PM +0200, Laurent Pinchart wrote: > > > > On Wed, Nov 01, 2023 at 10:05:46AM +0200, Sakari Ailus wrote: > > > > > Document that sub-device initialisation needs to complete before the async > > > > > sub-device is registered as there is no further driver action needed > > > > > before the sensor becomes accessible via the UAPI. > > > > > > > > > > Signed-off-by: Sakari Ailus > > > > > --- > > > > > Documentation/driver-api/media/camera-sensor.rst | 3 ++- > > > > > Documentation/driver-api/media/v4l2-subdev.rst | 4 ++++ > > > > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/Documentation/driver-api/media/camera-sensor.rst b/Documentation/driver-api/media/camera-sensor.rst > > > > > index 6456145f96ed..c675ce77c4d6 100644 > > > > > --- a/Documentation/driver-api/media/camera-sensor.rst > > > > > +++ b/Documentation/driver-api/media/camera-sensor.rst > > > > > @@ -60,7 +60,8 @@ management over the pipeline. > > > > > Camera sensor drivers are responsible for controlling the power state of the > > > > > device they otherwise control as well. They shall use runtime PM to manage > > > > > power states. Runtime PM shall be enabled at probe time and disabled at remove > > > > > -time. Drivers should enable runtime PM autosuspend. > > > > > +time. Drivers should enable runtime PM autosuspend. Note that runtime PM has to > > > > > +be enabled before registering the sensor's async sub-device. > > > > > > > > I wouldn't single out runtime PM initialization here, and neither would > > > > I talk about registration, as that's not in scope for this file. I think > > > > it would be better to instead reference v4l2-subdev.rst at the beginning > > > > of this file. Something generic like stating that camera sensor must be > > > > implemented as subdevs, and comply with the generic rules for subdevs, > > > > as explaiend in v4l2-subdev.rst. > > > > > > I added this text here as this appears to be a very common problem in > > > sensor drivers. Tost likely the reason is that some drivers have had this > > > issue and it has gotten copied elsewhere. > > > > Yes, I think there's lots of cargo cult there. Documentation is good, > > fixing drivers so that the next person to copy code will copy good code > > is better, but the best solution is to move most of the problem to > > helper functions provided by the core. > > I wouldn't call it "cargo cult" if a sensor driver needs to deal with > multiple unrelated matters. I meant that most sensor drivers are written by copying code from other drivers without understanding what that code does exactly, and why. > Of course it's possible to have helper > functions performing multiple unrelated things but at some point it is easy > to notice some drivers need things to be done differently. Admittedly, most > of such differences are due to historical reasons, not something you want > in new drivers anymore. It's time to relegate historical code to the git history :-) > > I still prefer linking to v4l2-subdev.rst. Duplicating documentation in > > multiple places increases the chances it will get stale. > > I'm fine with dropping this part of the patch. Although what is being said > there is very unlikely to change in the foreseeable future. Would you add a link to v4l2-subdev.rst at the top of the file ? It can be done in a separate patch. > > > > > The runtime PM handlers shall handle clocks, regulators, GPIOs, and other > > > > > system resources required to power the sensor up and down. For drivers that > > > > > diff --git a/Documentation/driver-api/media/v4l2-subdev.rst b/Documentation/driver-api/media/v4l2-subdev.rst > > > > > index e56b50b3f203..b22d1b075fd6 100644 > > > > > --- a/Documentation/driver-api/media/v4l2-subdev.rst > > > > > +++ b/Documentation/driver-api/media/v4l2-subdev.rst > > > > > @@ -195,6 +195,10 @@ performed using the :c:func:`v4l2_async_unregister_subdev` call. Subdevices > > > > > registered this way are stored in a global list of subdevices, ready to be > > > > > picked up by bridge drivers. > > > > > > > > > > +Note that all sensor initialisation has to complete before registering the async > > > > > +sub-device, including enabling runtime PM. This is because the sensor becomes > > > > > +accessible via the UAPI without further action by the sensor driver. > > > > > > > > It's not just the UAPI, but also in-kernel users. > > > > > > > > The passive form is heavier and thus harder to read. Maybe something > > > > like the following ? Feel free to rework it. > > > > > > > > ---- > > > > Drivers must complete all initialization of the sensor before registering the > > > > async sub-device. This includes enabling runtime PM. This is because the sensor > > > > becomes accessible and may be used as soon as it gets registered. > > > > > > s/used/accessed/ maybe? Or remove "and maybe used"? > > > > I'm fine with that. > > > > > > ---- > > > > > > > > Also, wouldn't this be better placed in the "Registering asynchronous > > > > sub-devices" section ? This isn't specific to sensors. > > > > > > Ah, yes, I meant sub-devices there. This actually applies to all > > > of UAPI, in general, but it's the async sub-device drivers that tend to > > > have the problem as people tend to think it's "just" registering the async > > > sub-device but in fact the related sub-device may be immediately registered > > > as well. > > > > Note there are two sections, one about subdev registration, one about > > camera sensor registration. You've added the text to the latter. > > The section I added the text to does not mention camera sensors separately, > the section title is "Registering asynchronous sub-devices". My bad, I was looking at the wrong location. Replacing the reference to sensor with subdev should be good enough then. > > > > > + > > > > > Asynchronous sub-device notifiers > > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > -- Regards, Laurent Pinchart