| Message ID | 20230525190100.130010-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: atomisp: Add support for v4l2-async sensor registration | expand |
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. > > - ret = atomisp_register_sensor_no_gmin(&dev->sd, 1, ATOMISP_INPUT_FORMAT_RAW_8, > - atomisp_bayer_order_grbg); > + ret = v4l2_async_register_subdev_sensor(&dev->sd); > if (ret) { > gc0310_remove(client); > return ret; > diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c > index d7d9cac2c3b8..5268a0d25051 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c > +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c > @@ -89,6 +89,8 @@ static const guid_t atomisp_dsm_guid = > * power-management and with v4l2-async probing. > */ > static const struct atomisp_csi2_sensor_config supported_sensors[] = { > + /* GalaxyCore GC0310 */ > + { "INT0310", 1 }, > /* Omnivision OV2680 */ > { "OVTI2680", 1 }, > };
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. Thanks j > > > > > - ret = atomisp_register_sensor_no_gmin(&dev->sd, 1, ATOMISP_INPUT_FORMAT_RAW_8, > > - atomisp_bayer_order_grbg); > > + ret = v4l2_async_register_subdev_sensor(&dev->sd); > > if (ret) { > > gc0310_remove(client); > > return ret; > > diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c > > index d7d9cac2c3b8..5268a0d25051 100644 > > --- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c > > +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c > > @@ -89,6 +89,8 @@ static const guid_t atomisp_dsm_guid = > > * power-management and with v4l2-async probing. > > */ > > static const struct atomisp_csi2_sensor_config supported_sensors[] = { > > + /* GalaxyCore GC0310 */ > > + { "INT0310", 1 }, > > /* Omnivision OV2680 */ > > { "OVTI2680", 1 }, > > }; > > -- > Kind regards, > > Sakari Ailus
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?
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
Hi Jacopo, On Thu, Jul 06, 2023 at 09:17:54AM +0200, Jacopo Mondi wrote: > 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 I don't disagree, I was just wondering what libcamera does. :-) > 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 ? Yes.
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; ret = gc0310_init_controls(dev); if (ret) { @@ -658,8 +666,7 @@ static int gc0310_probe(struct i2c_client *client) return ret; } - ret = atomisp_register_sensor_no_gmin(&dev->sd, 1, ATOMISP_INPUT_FORMAT_RAW_8, - atomisp_bayer_order_grbg); + ret = v4l2_async_register_subdev_sensor(&dev->sd); if (ret) { gc0310_remove(client); return ret; diff --git a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c index d7d9cac2c3b8..5268a0d25051 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c +++ b/drivers/staging/media/atomisp/pci/atomisp_csi2_bridge.c @@ -89,6 +89,8 @@ static const guid_t atomisp_dsm_guid = * power-management and with v4l2-async probing. */ static const struct atomisp_csi2_sensor_config supported_sensors[] = { + /* GalaxyCore GC0310 */ + { "INT0310", 1 }, /* Omnivision OV2680 */ { "OVTI2680", 1 }, };
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(-)