From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
To: Sakari Ailus <sakari.ailus@iki.fi>
Cc: Jacopo Mondi <jacopo.mondi@ideasonboard.com>,
Hans de Goede <hdegoede@redhat.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Sakari Ailus <sakari.ailus@linux.intel.com>,
Andy Shevchenko <andy@kernel.org>, Kate Hsuan <hpa@redhat.com>,
Tsuchiya Yuto <kitakar@gmail.com>,
Yury Luneff <yury.lunev@gmail.com>,
Nable <nable.maininbox@googlemail.com>,
andrey.i.trufanov@gmail.com, Fabio Aiuto <fabioaiuto83@gmail.com>,
Dan Scally <djrscally@gmail.com>,
linux-media@vger.kernel.org, linux-staging@lists.linux.dev,
Jacopo Mondi <jacopo@jmondi.org>
Subject: Re: [PATCH v2 3/5] media: atomisp: gc0310: Turn into standard v4l2 sensor driver
Date: Thu, 6 Jul 2023 09:17:54 +0200 [thread overview]
Message-ID: <gyjk2gfd4pqum57nspncglu2sgyhdr2hpal3up52lghmozftto@eikckzo4huqk> (raw)
In-Reply-To: <ZKZiqlEB92lziDYm@valkosipuli.retiisi.eu>
Hi Sakari
On Thu, Jul 06, 2023 at 06:43:54AM +0000, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Thu, Jul 06, 2023 at 08:37:24AM +0200, Jacopo Mondi wrote:
> > Hi Sakari, Hans
> >
> > On Wed, Jul 05, 2023 at 01:45:30PM +0000, Sakari Ailus wrote:
> > > Hi Hans,
> > >
> > > On Thu, May 25, 2023 at 09:00:58PM +0200, Hans de Goede wrote:
> > > > Switch the atomisp-gc0310 driver to v4l2 async device registration.
> > > >
> > > > After this change this driver no longer depends on
> > > > atomisp_gmin_platform and all atomisp-isms are gone.
> > > >
> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > ---
> > > > Changes in v2:
> > > > - Drop v4l2_get_acpi_sensor_info() call in this patch
> > > > - Wait for fwnode graph endpoint so that the bridge's ACPI
> > > > parsing gets a chance to register the GPIO mappings
> > > > before probing the sensor
> > > > - Switch to endpoint matching
> > > > ---
> > > > .../media/atomisp/i2c/atomisp-gc0310.c | 29 ++++++++++++-------
> > > > .../media/atomisp/pci/atomisp_csi2_bridge.c | 2 ++
> > > > 2 files changed, 20 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > > > index 1829ba419e3e..9a11793f34f7 100644
> > > > --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > > > +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c
> > > > @@ -29,8 +29,6 @@
> > > > #include <media/v4l2-ctrls.h>
> > > > #include <media/v4l2-device.h>
> > > >
> > > > -#include "../include/linux/atomisp_gmin_platform.h"
> > > > -
> > > > #define GC0310_NATIVE_WIDTH 656
> > > > #define GC0310_NATIVE_HEIGHT 496
> > > >
> > > > @@ -85,6 +83,7 @@ struct gc0310_device {
> > > > struct mutex input_lock;
> > > > bool is_streaming;
> > > >
> > > > + struct fwnode_handle *ep_fwnode;
> > > > struct gpio_desc *reset;
> > > > struct gpio_desc *powerdown;
> > > >
> > > > @@ -596,11 +595,11 @@ static void gc0310_remove(struct i2c_client *client)
> > > >
> > > > dev_dbg(&client->dev, "gc0310_remove...\n");
> > > >
> > > > - atomisp_unregister_subdev(sd);
> > > > - v4l2_device_unregister_subdev(sd);
> > > > + v4l2_async_unregister_subdev(sd);
> > > > media_entity_cleanup(&dev->sd.entity);
> > > > v4l2_ctrl_handler_free(&dev->ctrls.handler);
> > > > mutex_destroy(&dev->input_lock);
> > > > + fwnode_handle_put(dev->ep_fwnode);
> > > > pm_runtime_disable(&client->dev);
> > > > }
> > > >
> > > > @@ -613,19 +612,27 @@ static int gc0310_probe(struct i2c_client *client)
> > > > if (!dev)
> > > > return -ENOMEM;
> > > >
> > > > - ret = v4l2_get_acpi_sensor_info(&client->dev, NULL);
> > > > - if (ret)
> > > > - return ret;
> > > > + /*
> > > > + * Sometimes the fwnode graph is initialized by the bridge driver.
> > > > + * Bridge drivers doing this may also add GPIO mappings, wait for this.
> > > > + */
> > > > + dev->ep_fwnode = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev), NULL);
> > > > + if (!dev->ep_fwnode)
> > > > + return dev_err_probe(&client->dev, -EPROBE_DEFER, "waiting for fwnode graph endpoint\n");
> > > >
> > > > dev->reset = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
> > > > - if (IS_ERR(dev->reset))
> > > > + if (IS_ERR(dev->reset)) {
> > > > + fwnode_handle_put(dev->ep_fwnode);
> > > > return dev_err_probe(&client->dev, PTR_ERR(dev->reset),
> > > > "getting reset GPIO\n");
> > > > + }
> > > >
> > > > dev->powerdown = devm_gpiod_get(&client->dev, "powerdown", GPIOD_OUT_HIGH);
> > > > - if (IS_ERR(dev->powerdown))
> > > > + if (IS_ERR(dev->powerdown)) {
> > > > + fwnode_handle_put(dev->ep_fwnode);
> > > > return dev_err_probe(&client->dev, PTR_ERR(dev->powerdown),
> > > > "getting powerdown GPIO\n");
> > > > + }
> > > >
> > > > mutex_init(&dev->input_lock);
> > > > v4l2_i2c_subdev_init(&dev->sd, client, &gc0310_ops);
> > > > @@ -645,6 +652,7 @@ static int gc0310_probe(struct i2c_client *client)
> > > > dev->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > > dev->pad.flags = MEDIA_PAD_FL_SOURCE;
> > > > dev->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > > > + dev->sd.fwnode = dev->ep_fwnode;
> > >
> > > Same for this one: leave as-is --- the v4l2_async_register_subdev()
> > > function will set the device fwnode for this.
> > >
> > > >
> > > > ret = gc0310_init_controls(dev);
> > > > if (ret) {
> > > > @@ -658,8 +666,7 @@ static int gc0310_probe(struct i2c_client *client)
> > > > return ret;
> > > > }
> > >
> > > This driver should (as well as ov2680) check for the link frequencies. This
> > > is an old sensor so if all users eventually use this via firmware that
> > > lacks this information, there's little benefit for adding the code. But
> > > otherwise this is seen as a bug.
> > >
> > > <URL:https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#handling-clocks
> > >
> > > The raw cameras should use pixel rate and blanking controls for configuring
> > > the frame interval. This one uses s_frame_interval instead, and it may be
> > > difficult to find the information needed for the pixel rate based API.
> > >
> > > Cc Jacopo.
> >
> > Thanks
> >
> > If you intend to use these devices with libcamera be aware we mandate
> > support for the following controls
> >
> > V4L2_CID_ANALOGUE_GAIN
> > V4L2_CID_EXPOSURE
> > V4L2_CID_HBLANK
> > V4L2_CID_PIXEL_RATE
> > V4L2_CID_VBLANK
> >
> > https://git.libcamera.org/libcamera/libcamera.git/tree/Documentation/sensor_driver_requirements.rst#n20
> >
> > Do you think it would be possible to at least expose them read-only ?
> > This -should- be ok for libcamera. Your s_frame_interval() calls then
> > need to update the value of the controls.
>
> Can libcamera use s_frame_interval or does it just mean the frame rate will
> remain whatever it was when libcamera started?
Currently if those 'mandatory' controls are not available libcamera
refuses to detect the sensor at all.
s_frame_interval could be considered for YUV/RGB sensors, but as both
the gc0310 and the ov2680 are RAW sensors, frame rate control should
really go through blankings and pixel rate. Read-only values are ok to
start with, framerate will be fixed but that's ok I guess (also
because as long as you don't have an IPA and do not support
controllable durations, there is no way to change it anyway).
Does this sound reasonable for you and Hans ?
>
> --
> Regards,
>
> Sakari Ailus
next prev parent reply other threads:[~2023-07-06 7:18 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-25 19:00 [PATCH v2 0/5] media: atomisp: Add support for v4l2-async sensor registration Hans de Goede
2023-05-25 19:00 ` [PATCH v2 1/5] " Hans de Goede
2023-05-26 20:30 ` Andy Shevchenko
2023-05-27 9:25 ` Hans de Goede
2023-05-27 10:26 ` Andy Shevchenko
2023-05-25 19:00 ` [PATCH v2 2/5] media: atomisp: ov2680: Turn into standard v4l2 sensor driver Hans de Goede
2023-07-05 13:34 ` Sakari Ailus
2023-05-25 19:00 ` [PATCH v2 3/5] media: atomisp: gc0310: " Hans de Goede
2023-07-05 13:45 ` Sakari Ailus
2023-07-06 6:37 ` Jacopo Mondi
2023-07-06 6:43 ` Sakari Ailus
2023-07-06 7:17 ` Jacopo Mondi [this message]
2023-07-06 7:20 ` Sakari Ailus
2023-05-25 19:00 ` [PATCH v2 4/5] media: atomisp: Drop v4l2_get_acpi_sensor_info() function Hans de Goede
2023-05-25 19:01 ` [PATCH v2 5/5] media: Move gc0310 sensor drivers to drivers/media/i2c/ Hans de Goede
2023-05-26 21:23 ` [PATCH v2 0/5] media: atomisp: Add support for v4l2-async sensor registration Andy Shevchenko
2023-05-27 15:54 ` Hans de Goede
2024-04-30 10:32 ` Mauro Carvalho Chehab
2024-04-30 11:51 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=gyjk2gfd4pqum57nspncglu2sgyhdr2hpal3up52lghmozftto@eikckzo4huqk \
--to=jacopo.mondi@ideasonboard.com \
--cc=andrey.i.trufanov@gmail.com \
--cc=andy@kernel.org \
--cc=djrscally@gmail.com \
--cc=fabioaiuto83@gmail.com \
--cc=hdegoede@redhat.com \
--cc=hpa@redhat.com \
--cc=jacopo@jmondi.org \
--cc=kitakar@gmail.com \
--cc=linux-media@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=mchehab@kernel.org \
--cc=nable.maininbox@googlemail.com \
--cc=sakari.ailus@iki.fi \
--cc=sakari.ailus@linux.intel.com \
--cc=yury.lunev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).