Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:1 as permitted sender) client-ip=2620:137:e000::3:1;
Date:   Wed, 11 Oct 2023 11:12:14 +0000
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc:     Sakari Ailus <sakari.ailus@iki.fi>, linux-media@vger.kernel.org,
        devicetree@vger.kernel.org,
        "Paul J. Murphy" <paul.j.murphy@intel.com>,
        Daniele Alessandrelli <daniele.alessandrelli@intel.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        open list <linux-kernel@vger.kernel.org>,
        laurent.pinchart@ideasonboard.com, hverkuil@xs4all.nl
Subject: Re: [PATCH 3/5] media: i2c: imx335: Implement get selection API
Message-ID: <ZSaDDqrpX4LeoLqX@kekkonen.localdomain>
References: <20231010005126.3425444-1-kieran.bingham@ideasonboard.com>
 <20231010005126.3425444-4-kieran.bingham@ideasonboard.com>
 <ZSTrse7OeKIA+k2t@valkosipuli.retiisi.eu>
 <169701831889.277971.6656559808677876108@ping.linuxembedded.co.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <169701831889.277971.6656559808677876108@ping.linuxembedded.co.uk>
Precedence: bulk

Hi Kieran,

On Wed, Oct 11, 2023 at 10:58:38AM +0100, Kieran Bingham wrote:
> Quoting Sakari Ailus (2023-10-10 07:14:09)
> > Hi Kieran,
> > 
> > On Tue, Oct 10, 2023 at 01:51:24AM +0100, Kieran Bingham wrote:
> > > Support reporting of the Sensor Native and Active pixel array areas
> > > through the Selection API.
> > > 
> > > The implementation reports a single target crop only for the mode that
> > > is presently exposed by the driver.
> > > 
> > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > Shouldn't you use the same callback for .set_selection? I guess this is
> > somewhat grey area but doing so would be in line with how V4L2 API works in
> > general.
> 
> Hrm ... I didn't think it was needed as it's not possible to /set/
> anything.

Similarly, VIDIOC_SUBDEV_S_FMT is available even if you can't change the
format.

> 
> I expect to change this once I add support for setting crops later
> though. It was going to be something I'd add when it is used.
> 
> Only the 'get_selection' call is necessary to make this camera operate
> on both i.MX8MP and RPi5 platforms with libcamera, so that's what I've
> done so far. My goal of this series was to bring the existing driver up
> to a point that it can be used, before I start making new feature
> additions.

I don't have concerns with that, just that we implement the IOCTLs
consitently. This has been discussed before but AFAIR without any firm
conclusions.

Additionally, some targets are settable while some won't be, and it may
well depend on the driver.

v4l2-compliance appears to be happy with G_SELECTION without S_SELECTION
though.

Also cc Hans.

-- 
Regards,

Sakari Ailus