| Message ID | 20200704004028.21153-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent. On Sat, 4 Jul 2020 at 01:40, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Dave, > > This patch series reports sensor orientation through DT for the OV5647, > IMX219 and IMX477. The first 8 patches are backported from mainline, > while the last 4 patches are new. Patch 09/12 could possibly be skipped > for now, as we don't use the rotation property in the ov5647 DT overlay. > > The patches are based on top of rpi-5.4.y. libcamera patches will follow > shortly. The patches look reasonable. I'd suggest that rotation really ought to be a dtoverlay property rather than hard coded. Particularly with IMX219 there is no defined "right way" of mounting it. IMX477 with the tripod screw-hole does have a natural orientation. Are you expecting those to be merged into our kernel tree? We normally use Github pull requests rather than mailing lists. Dave > Jacopo Mondi (8): > media: dt-bindings: video-interfaces: Document 'orientation' property > media: dt-bindings: video-interface: Replace 'rotation' description > media: v4l2-ctrl: Document V4L2_CID_CAMERA_ORIENTATION > media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION > media: v4l2-ctrls: Add camera orientation and rotation > media: v4l2-fwnode: Add helper to parse device properties > media: v4l2-ctrls: Add helper to register properties > media: i2c: imx219: Parse and register properties > > Laurent Pinchart (4): > media: i2c: ov5647: Parse and register properties > media: i2c: imx477: Parse and register properties > dt/dtoverlays: imx219: Set sensor rotation > dt/dtoverlays: imx477: Set sensor rotation > > .../bindings/media/video-interfaces.txt | 370 +++++++++++++++++- > .../media/uapi/v4l/ext-ctrls-camera.rst | 151 +++++++ > arch/arm/boot/dts/overlays/imx219-overlay.dts | 2 + > arch/arm/boot/dts/overlays/imx477-overlay.dts | 2 + > drivers/media/i2c/imx219.c | 12 +- > drivers/media/i2c/imx477.c | 12 +- > drivers/media/i2c/ov5647.c | 13 +- > drivers/media/v4l2-core/v4l2-ctrls.c | 53 +++ > drivers/media/v4l2-core/v4l2-fwnode.c | 42 ++ > include/media/v4l2-ctrls.h | 26 ++ > include/media/v4l2-fwnode.h | 47 +++ > include/uapi/linux/v4l2-controls.h | 7 + > 12 files changed, 731 insertions(+), 6 deletions(-) > > -- > Regards, > > Laurent Pinchart >
Hi Dave, On Mon, Jul 06, 2020 at 06:33:41PM +0100, Dave Stevenson wrote: > On Sat, 4 Jul 2020 at 01:40, Laurent Pinchart wrote: > > > > Hi Dave, > > > > This patch series reports sensor orientation through DT for the OV5647, > > IMX219 and IMX477. The first 8 patches are backported from mainline, > > while the last 4 patches are new. Patch 09/12 could possibly be skipped > > for now, as we don't use the rotation property in the ov5647 DT overlay. > > > > The patches are based on top of rpi-5.4.y. libcamera patches will follow > > shortly. > > The patches look reasonable. I'd suggest that rotation really ought to > be a dtoverlay property rather than hard coded. Particularly with > IMX219 there is no defined "right way" of mounting it. IMX477 with the > tripod screw-hole does have a natural orientation. I agree, the rotation belongs to an overlay, as it will depend on how the camera module is integrated in the final product. This patch series moves the default rotation from the RPi IPA to the camera sensor overlays to provide the same default value as currently used, but it's really up to the user to decide on the correct value. > Are you expecting those to be merged into our kernel tree? We normally > use Github pull requests rather than mailing lists. I wanted the patches to appear on the libcamera-devel mailing list in case discussions were needed, and to provide a reference to the kernel modifications required by the corresponding libcamera patches I've posted. Should I send a pull request, or are there issues you think need to be addressed first ? > > Jacopo Mondi (8): > > media: dt-bindings: video-interfaces: Document 'orientation' property > > media: dt-bindings: video-interface: Replace 'rotation' description > > media: v4l2-ctrl: Document V4L2_CID_CAMERA_ORIENTATION > > media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION > > media: v4l2-ctrls: Add camera orientation and rotation > > media: v4l2-fwnode: Add helper to parse device properties > > media: v4l2-ctrls: Add helper to register properties > > media: i2c: imx219: Parse and register properties > > > > Laurent Pinchart (4): > > media: i2c: ov5647: Parse and register properties > > media: i2c: imx477: Parse and register properties > > dt/dtoverlays: imx219: Set sensor rotation > > dt/dtoverlays: imx477: Set sensor rotation > > > > .../bindings/media/video-interfaces.txt | 370 +++++++++++++++++- > > .../media/uapi/v4l/ext-ctrls-camera.rst | 151 +++++++ > > arch/arm/boot/dts/overlays/imx219-overlay.dts | 2 + > > arch/arm/boot/dts/overlays/imx477-overlay.dts | 2 + > > drivers/media/i2c/imx219.c | 12 +- > > drivers/media/i2c/imx477.c | 12 +- > > drivers/media/i2c/ov5647.c | 13 +- > > drivers/media/v4l2-core/v4l2-ctrls.c | 53 +++ > > drivers/media/v4l2-core/v4l2-fwnode.c | 42 ++ > > include/media/v4l2-ctrls.h | 26 ++ > > include/media/v4l2-fwnode.h | 47 +++ > > include/uapi/linux/v4l2-controls.h | 7 + > > 12 files changed, 731 insertions(+), 6 deletions(-)
On Tue, 7 Jul 2020 at 17:48, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Dave, > > On Mon, Jul 06, 2020 at 06:33:41PM +0100, Dave Stevenson wrote: > > On Sat, 4 Jul 2020 at 01:40, Laurent Pinchart wrote: > > > > > > Hi Dave, > > > > > > This patch series reports sensor orientation through DT for the OV5647, > > > IMX219 and IMX477. The first 8 patches are backported from mainline, > > > while the last 4 patches are new. Patch 09/12 could possibly be skipped > > > for now, as we don't use the rotation property in the ov5647 DT overlay. > > > > > > The patches are based on top of rpi-5.4.y. libcamera patches will follow > > > shortly. > > > > The patches look reasonable. I'd suggest that rotation really ought to > > be a dtoverlay property rather than hard coded. Particularly with > > IMX219 there is no defined "right way" of mounting it. IMX477 with the > > tripod screw-hole does have a natural orientation. > > I agree, the rotation belongs to an overlay, as it will depend on how > the camera module is integrated in the final product. This patch series > moves the default rotation from the RPi IPA to the camera sensor > overlays to provide the same default value as currently used, but it's > really up to the user to decide on the correct value. > > > Are you expecting those to be merged into our kernel tree? We normally > > use Github pull requests rather than mailing lists. > > I wanted the patches to appear on the libcamera-devel mailing list in > case discussions were needed, and to provide a reference to the kernel > modifications required by the corresponding libcamera patches I've > posted. Should I send a pull request, or are there issues you think need > to be addressed first ? It'd be nice if rotation was a dt overlay parameter before creating the PR, but otherwise it looks fine to me. Where patches are backports of patches that have been merged to mainline, we normally add a line "Commit <full hash> upstream" to the commit text, just to make Dom's job easier when he pulls stuff forward to newer kernels. Dave > > > Jacopo Mondi (8): > > > media: dt-bindings: video-interfaces: Document 'orientation' property > > > media: dt-bindings: video-interface: Replace 'rotation' description > > > media: v4l2-ctrl: Document V4L2_CID_CAMERA_ORIENTATION > > > media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION > > > media: v4l2-ctrls: Add camera orientation and rotation > > > media: v4l2-fwnode: Add helper to parse device properties > > > media: v4l2-ctrls: Add helper to register properties > > > media: i2c: imx219: Parse and register properties > > > > > > Laurent Pinchart (4): > > > media: i2c: ov5647: Parse and register properties > > > media: i2c: imx477: Parse and register properties > > > dt/dtoverlays: imx219: Set sensor rotation > > > dt/dtoverlays: imx477: Set sensor rotation > > > > > > .../bindings/media/video-interfaces.txt | 370 +++++++++++++++++- > > > .../media/uapi/v4l/ext-ctrls-camera.rst | 151 +++++++ > > > arch/arm/boot/dts/overlays/imx219-overlay.dts | 2 + > > > arch/arm/boot/dts/overlays/imx477-overlay.dts | 2 + > > > drivers/media/i2c/imx219.c | 12 +- > > > drivers/media/i2c/imx477.c | 12 +- > > > drivers/media/i2c/ov5647.c | 13 +- > > > drivers/media/v4l2-core/v4l2-ctrls.c | 53 +++ > > > drivers/media/v4l2-core/v4l2-fwnode.c | 42 ++ > > > include/media/v4l2-ctrls.h | 26 ++ > > > include/media/v4l2-fwnode.h | 47 +++ > > > include/uapi/linux/v4l2-controls.h | 7 + > > > 12 files changed, 731 insertions(+), 6 deletions(-) > > -- > Regards, > > Laurent Pinchart
Hi Dave, On Tue, Jul 07, 2020 at 10:13:04PM +0100, Dave Stevenson wrote: > On Tue, 7 Jul 2020 at 17:48, Laurent Pinchart wrote: > > On Mon, Jul 06, 2020 at 06:33:41PM +0100, Dave Stevenson wrote: > > > On Sat, 4 Jul 2020 at 01:40, Laurent Pinchart wrote: > > > > > > > > Hi Dave, > > > > > > > > This patch series reports sensor orientation through DT for the OV5647, > > > > IMX219 and IMX477. The first 8 patches are backported from mainline, > > > > while the last 4 patches are new. Patch 09/12 could possibly be skipped > > > > for now, as we don't use the rotation property in the ov5647 DT overlay. > > > > > > > > The patches are based on top of rpi-5.4.y. libcamera patches will follow > > > > shortly. > > > > > > The patches look reasonable. I'd suggest that rotation really ought to > > > be a dtoverlay property rather than hard coded. Particularly with > > > IMX219 there is no defined "right way" of mounting it. IMX477 with the > > > tripod screw-hole does have a natural orientation. > > > > I agree, the rotation belongs to an overlay, as it will depend on how > > the camera module is integrated in the final product. This patch series > > moves the default rotation from the RPi IPA to the camera sensor > > overlays to provide the same default value as currently used, but it's > > really up to the user to decide on the correct value. > > > > > Are you expecting those to be merged into our kernel tree? We normally > > > use Github pull requests rather than mailing lists. > > > > I wanted the patches to appear on the libcamera-devel mailing list in > > case discussions were needed, and to provide a reference to the kernel > > modifications required by the corresponding libcamera patches I've > > posted. Should I send a pull request, or are there issues you think need > > to be addressed first ? > > It'd be nice if rotation was a dt overlay parameter before creating > the PR, but otherwise it looks fine to me. I wasn't aware of the DT parameters mechanism. I'll adapt the patches accordingly, and will also reference upstream commit for backported patches. > Where patches are backports of patches that have been merged to > mainline, we normally add a line > "Commit <full hash> upstream" > to the commit text, just to make Dom's job easier when he pulls stuff > forward to newer kernels. > > > > > Jacopo Mondi (8): > > > > media: dt-bindings: video-interfaces: Document 'orientation' property > > > > media: dt-bindings: video-interface: Replace 'rotation' description > > > > media: v4l2-ctrl: Document V4L2_CID_CAMERA_ORIENTATION > > > > media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION > > > > media: v4l2-ctrls: Add camera orientation and rotation > > > > media: v4l2-fwnode: Add helper to parse device properties > > > > media: v4l2-ctrls: Add helper to register properties > > > > media: i2c: imx219: Parse and register properties > > > > > > > > Laurent Pinchart (4): > > > > media: i2c: ov5647: Parse and register properties > > > > media: i2c: imx477: Parse and register properties > > > > dt/dtoverlays: imx219: Set sensor rotation > > > > dt/dtoverlays: imx477: Set sensor rotation > > > > > > > > .../bindings/media/video-interfaces.txt | 370 +++++++++++++++++- > > > > .../media/uapi/v4l/ext-ctrls-camera.rst | 151 +++++++ > > > > arch/arm/boot/dts/overlays/imx219-overlay.dts | 2 + > > > > arch/arm/boot/dts/overlays/imx477-overlay.dts | 2 + > > > > drivers/media/i2c/imx219.c | 12 +- > > > > drivers/media/i2c/imx477.c | 12 +- > > > > drivers/media/i2c/ov5647.c | 13 +- > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 53 +++ > > > > drivers/media/v4l2-core/v4l2-fwnode.c | 42 ++ > > > > include/media/v4l2-ctrls.h | 26 ++ > > > > include/media/v4l2-fwnode.h | 47 +++ > > > > include/uapi/linux/v4l2-controls.h | 7 + > > > > 12 files changed, 731 insertions(+), 6 deletions(-)
On Tue, 7 Jul 2020 at 22:40, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Dave, > > On Tue, Jul 07, 2020 at 10:13:04PM +0100, Dave Stevenson wrote: > > On Tue, 7 Jul 2020 at 17:48, Laurent Pinchart wrote: > > > On Mon, Jul 06, 2020 at 06:33:41PM +0100, Dave Stevenson wrote: > > > > On Sat, 4 Jul 2020 at 01:40, Laurent Pinchart wrote: > > > > > > > > > > Hi Dave, > > > > > > > > > > This patch series reports sensor orientation through DT for the OV5647, > > > > > IMX219 and IMX477. The first 8 patches are backported from mainline, > > > > > while the last 4 patches are new. Patch 09/12 could possibly be skipped > > > > > for now, as we don't use the rotation property in the ov5647 DT overlay. > > > > > > > > > > The patches are based on top of rpi-5.4.y. libcamera patches will follow > > > > > shortly. > > > > > > > > The patches look reasonable. I'd suggest that rotation really ought to > > > > be a dtoverlay property rather than hard coded. Particularly with > > > > IMX219 there is no defined "right way" of mounting it. IMX477 with the > > > > tripod screw-hole does have a natural orientation. > > > > > > I agree, the rotation belongs to an overlay, as it will depend on how > > > the camera module is integrated in the final product. This patch series > > > moves the default rotation from the RPi IPA to the camera sensor > > > overlays to provide the same default value as currently used, but it's > > > really up to the user to decide on the correct value. > > > > > > > Are you expecting those to be merged into our kernel tree? We normally > > > > use Github pull requests rather than mailing lists. > > > > > > I wanted the patches to appear on the libcamera-devel mailing list in > > > case discussions were needed, and to provide a reference to the kernel > > > modifications required by the corresponding libcamera patches I've > > > posted. Should I send a pull request, or are there issues you think need > > > to be addressed first ? > > > > It'd be nice if rotation was a dt overlay parameter before creating > > the PR, but otherwise it looks fine to me. > > I wasn't aware of the DT parameters mechanism. I'll adapt the patches > accordingly, and will also reference upstream commit for backported > patches. Thanks. If you're stuck then create the PR and I'll either add any additional changes, or make comment as to what is needed. Phil will be wanting an update to https://github.com/raspberrypi/linux/blob/rpi-5.4.y/arch/arm/boot/dts/overlays/README as well so that the help text reflects any parameters. > > Where patches are backports of patches that have been merged to > > mainline, we normally add a line > > "Commit <full hash> upstream" > > to the commit text, just to make Dom's job easier when he pulls stuff > > forward to newer kernels. > > > > > > > Jacopo Mondi (8): > > > > > media: dt-bindings: video-interfaces: Document 'orientation' property > > > > > media: dt-bindings: video-interface: Replace 'rotation' description > > > > > media: v4l2-ctrl: Document V4L2_CID_CAMERA_ORIENTATION > > > > > media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION > > > > > media: v4l2-ctrls: Add camera orientation and rotation > > > > > media: v4l2-fwnode: Add helper to parse device properties > > > > > media: v4l2-ctrls: Add helper to register properties > > > > > media: i2c: imx219: Parse and register properties > > > > > > > > > > Laurent Pinchart (4): > > > > > media: i2c: ov5647: Parse and register properties > > > > > media: i2c: imx477: Parse and register properties > > > > > dt/dtoverlays: imx219: Set sensor rotation > > > > > dt/dtoverlays: imx477: Set sensor rotation > > > > > > > > > > .../bindings/media/video-interfaces.txt | 370 +++++++++++++++++- > > > > > .../media/uapi/v4l/ext-ctrls-camera.rst | 151 +++++++ > > > > > arch/arm/boot/dts/overlays/imx219-overlay.dts | 2 + > > > > > arch/arm/boot/dts/overlays/imx477-overlay.dts | 2 + > > > > > drivers/media/i2c/imx219.c | 12 +- > > > > > drivers/media/i2c/imx477.c | 12 +- > > > > > drivers/media/i2c/ov5647.c | 13 +- > > > > > drivers/media/v4l2-core/v4l2-ctrls.c | 53 +++ > > > > > drivers/media/v4l2-core/v4l2-fwnode.c | 42 ++ > > > > > include/media/v4l2-ctrls.h | 26 ++ > > > > > include/media/v4l2-fwnode.h | 47 +++ > > > > > include/uapi/linux/v4l2-controls.h | 7 + > > > > > 12 files changed, 731 insertions(+), 6 deletions(-) > > -- > Regards, > > Laurent Pinchart