| Message ID | 20221014183459.181567-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | media: i2c: ov5645 driver enhancements | expand |
Hi Sakari, On Fri, Oct 14, 2022 at 7:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Hi All, > > The main aim of this series is to add PM support to the sensor driver. > > I had two more patches [0] and [1] which were for ov5645, so instead > sending them separately I have clubbed them as series. > > v1-> v2 > - patch #1 is infact a v3 [1] no changes > - patch #2 fixed review comments pointed by Sakari > - patch #3 [0] no changes > - patches #4 and #5 are new > > [0] https://patchwork.linuxtv.org/project/linux-media/patch/20220927202005.750621-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ > [1] https://patchwork.linuxtv.org/project/linux-media/patch/20220919153540.178732-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ > > Cheers, > Prabhakar > > Lad Prabhakar (5): > media: dt-bindings: ov5645: Convert OV5645 binding to a schema > media: i2c: ov5645: Use runtime PM > media: i2c: ov5645: Drop empty comment > media: i2c: ov5645: Return zero for s_stream(0) > media: i2c: ov5645: Call ov5645_entity_init_cfg() before registering > the subdev > After sending this series I realized I had an additional patch [0] for ov5645 which I should have tagged along with the series. Can you please pick [0] while reviewing this series. [0] https://patchwork.kernel.org/project/linux-media/patch/20220919143350.176746-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ Cheers, Prabhakar
On Fri, 14 Oct 2022 19:34:55 +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Convert the simple OV5645 Device Tree binding to json-schema. > > The previous binding marked the below properties as required which was a > driver requirement and not the device requirement so just drop them from > the required list during the conversion. > - clock-frequency > - enable-gpios > - reset-gpios > > Also drop the "clock-names" property as we have a single clock source for > the sensor and the driver has been updated to drop the clk referencing by > name. > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Resend v3: > * No change > > v2 -> v3 > * Dropped clock-names property > * Marked power supplies as mandatory > * Dropped the comment for voltage power supplies > * Included RB tag from Laurent > * Driver change to drop clock-names [0] > > [0] https://lore.kernel.org/linux-media/Yyh%2F3uzOJOu3drEB@pendragon.ideasonboard.com/T/#t > > v1 -> v2 > * Dropped ref to video-interface-devices.yaml# > * Dropped driver specific required items from the list > * Updated commit message > * Dropped clock-lanes and bus-type from the port and example node > * Marked data-lanes as required in port node > --- > .../devicetree/bindings/media/i2c/ov5645.txt | 54 --------- > .../bindings/media/i2c/ovti,ov5645.yaml | 104 ++++++++++++++++++ > 2 files changed, 104 insertions(+), 54 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt > create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml > Running 'make dtbs_check' with the schema in this patch gives the following warnings. Consider if they are expected or the schema is incorrect. These may not be new warnings. Note that it is not yet a requirement to have 0 warnings for dtbs_check. This will change in the future. Full log is available here: https://patchwork.ozlabs.org/patch/ camera@3c: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+' arch/arm/boot/dts/imx6dl-pico-dwarf.dtb arch/arm/boot/dts/imx6dl-pico-hobbit.dtb arch/arm/boot/dts/imx6dl-pico-nymph.dtb arch/arm/boot/dts/imx6dl-pico-pi.dtb arch/arm/boot/dts/imx6dl-wandboard.dtb arch/arm/boot/dts/imx6dl-wandboard-revb1.dtb arch/arm/boot/dts/imx6dl-wandboard-revd1.dtb arch/arm/boot/dts/imx6q-pico-dwarf.dtb arch/arm/boot/dts/imx6q-pico-hobbit.dtb arch/arm/boot/dts/imx6q-pico-nymph.dtb arch/arm/boot/dts/imx6q-pico-pi.dtb arch/arm/boot/dts/imx6qp-wandboard-revd1.dtb arch/arm/boot/dts/imx6q-wandboard.dtb arch/arm/boot/dts/imx6q-wandboard-revb1.dtb arch/arm/boot/dts/imx6q-wandboard-revd1.dtb ov5645@3c: 'clock-names' does not match any of the regexes: 'pinctrl-[0-9]+' arch/arm64/boot/dts/renesas/r8a774a1-hihope-rzg2m-ex-mipi-2.1.dtb arch/arm64/boot/dts/renesas/r8a774b1-hihope-rzg2n-ex-mipi-2.1.dtb arch/arm64/boot/dts/renesas/r8a774c0-ek874-mipi-2.1.dtb arch/arm64/boot/dts/renesas/r8a774e1-hihope-rzg2h-ex-mipi-2.1.dtb
On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Convert the simple OV5645 Device Tree binding to json-schema. > > The previous binding marked the below properties as required which was a > driver requirement and not the device requirement so just drop them from > the required list during the conversion. > - clock-frequency > - enable-gpios > - reset-gpios > > Also drop the "clock-names" property as we have a single clock source for > the sensor and the driver has been updated to drop the clk referencing by > name. Driver requirements are the ABI! This breaks a kernel without the driver change and a DTB that has dropped the properties. Also, with 'clock-names' dropped, you've just introduced a bunch of warnings on other people's platforms. Are you going to 'fix' all of them? Rob
Hi Rob, Thank you for the review. On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Convert the simple OV5645 Device Tree binding to json-schema. > > > > The previous binding marked the below properties as required which was a > > driver requirement and not the device requirement so just drop them from > > the required list during the conversion. > > - clock-frequency > > - enable-gpios > > - reset-gpios > > > > Also drop the "clock-names" property as we have a single clock source for > > the sensor and the driver has been updated to drop the clk referencing by > > name. > > Driver requirements are the ABI! > > This breaks a kernel without the driver change and a DTB that has > dropped the properties. > I already have a patch for the driver [0] which I missed to include along with the series. > Also, with 'clock-names' dropped, you've just introduced a bunch of > warnings on other people's platforms. Are you going to 'fix' all of > them? > Yes I will fix them, once the patch driver patch [0] is merged in. [0] https://patchwork.kernel.org/project/linux-media/patch/20220919143350.176746-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ Cheers, Prabhakar
On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote: > Hi Rob, > > Thank you for the review. > > On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > Convert the simple OV5645 Device Tree binding to json-schema. > > > > > > The previous binding marked the below properties as required which was a > > > driver requirement and not the device requirement so just drop them from > > > the required list during the conversion. > > > - clock-frequency > > > - enable-gpios > > > - reset-gpios > > > > > > Also drop the "clock-names" property as we have a single clock source for > > > the sensor and the driver has been updated to drop the clk referencing by > > > name. > > > > Driver requirements are the ABI! > > > > This breaks a kernel without the driver change and a DTB that has > > dropped the properties. > > > I already have a patch for the driver [0] which I missed to include > along with the series. You completely miss the point. Read the first sentence again. Changing driver requirements changes the ABI. This breaks the ABI. The driver patch does not help that. > > Also, with 'clock-names' dropped, you've just introduced a bunch of > > warnings on other people's platforms. Are you going to 'fix' all of > > them? > > > Yes I will fix them, once the patch driver patch [0] is merged in. Why? You are just making extra work. We have enough warnings as-is to fix. Rob
Hi Rob, On Fri, Oct 14, 2022 at 04:40:29PM -0500, Rob Herring wrote: > On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote: > > On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote: > > > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > Convert the simple OV5645 Device Tree binding to json-schema. > > > > > > > > The previous binding marked the below properties as required which was a > > > > driver requirement and not the device requirement so just drop them from > > > > the required list during the conversion. > > > > - clock-frequency > > > > - enable-gpios > > > > - reset-gpios > > > > > > > > Also drop the "clock-names" property as we have a single clock source for > > > > the sensor and the driver has been updated to drop the clk referencing by > > > > name. > > > > > > Driver requirements are the ABI! > > > > > > This breaks a kernel without the driver change and a DTB that has > > > dropped the properties. > > > > > I already have a patch for the driver [0] which I missed to include > > along with the series. > > You completely miss the point. Read the first sentence again. Changing > driver requirements changes the ABI. > > This breaks the ABI. The driver patch does not help that. I'm not following you here. If the DT binding makes a mandatory property optional, it doesn't break any existing platform. The only thing that would not work is a new DT that doesn't contain the now optional property combined with an older driver that makes it required. That's not a regression, as it would be a *new* DT. > > > Also, with 'clock-names' dropped, you've just introduced a bunch of > > > warnings on other people's platforms. Are you going to 'fix' all of > > > them? > > > > > Yes I will fix them, once the patch driver patch [0] is merged in. > > Why? You are just making extra work. We have enough warnings as-is to > fix. I agree that a DT binding change should patch all in-tree DTS to avoid introducing new warnings.
Hi Prabhakar, Thank you for the patch. On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Always return zero while stopping the stream as the caller will ignore the > return value. > > This patch drops checking the return value of ov5645_write_reg() and > continues further in the code path while stopping stream. The user anyway > gets an error message in case ov5645_write_reg() fails. Continuing all the way to pm_runtime_put() is fine, but I don't think the function should return 0. It's not up to the driver to decide if a failure would be useful to signal to the caller or not. > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > v1->v2 > * New patch > --- > drivers/media/i2c/ov5645.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index a0b9d0c43b78..b3825294aaf1 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -995,14 +995,11 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable) > if (ret < 0) > goto err_rpm_put; > } else { > - ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40); > - if (ret < 0) > - return ret; > + ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40); > + > + ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0, > + OV5645_SYSTEM_CTRL0_STOP); > > - ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0, > - OV5645_SYSTEM_CTRL0_STOP); > - if (ret < 0) > - return ret; > pm_runtime_put(ov5645->dev); > } >
Hi Prabhakar, Thank you for the patch. On Fri, Oct 14, 2022 at 07:34:59PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Make sure we call ov5645_entity_init_cfg() before registering the subdev > to make sure default formats are set up. > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> If you have a few spare cycles, it would be even better to convert the driver to the subdev active state API :-) You could then drop this call entirely. > --- > v1->v2 > * New patch > --- > drivers/media/i2c/ov5645.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > index b3825294aaf1..14bcc6e42dd2 100644 > --- a/drivers/media/i2c/ov5645.c > +++ b/drivers/media/i2c/ov5645.c > @@ -1212,6 +1212,8 @@ static int ov5645_probe(struct i2c_client *client) > goto err_pm_runtime; > } > > + ov5645_entity_init_cfg(&ov5645->sd, NULL); > + > ret = v4l2_async_register_subdev(&ov5645->sd); > if (ret < 0) { > dev_err(dev, "could not register v4l2 device\n"); > @@ -1224,8 +1226,6 @@ static int ov5645_probe(struct i2c_client *client) > pm_runtime_use_autosuspend(dev); > pm_runtime_put_autosuspend(dev); > > - ov5645_entity_init_cfg(&ov5645->sd, NULL); > - > return 0; > > err_pm_runtime:
On 15/10/2022 01:54, Laurent Pinchart wrote: > Hi Rob, > > On Fri, Oct 14, 2022 at 04:40:29PM -0500, Rob Herring wrote: >> On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote: >>> On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote: >>>> On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: >>>>> >>>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> >>>>> >>>>> Convert the simple OV5645 Device Tree binding to json-schema. >>>>> >>>>> The previous binding marked the below properties as required which was a >>>>> driver requirement and not the device requirement so just drop them from >>>>> the required list during the conversion. >>>>> - clock-frequency >>>>> - enable-gpios >>>>> - reset-gpios >>>>> >>>>> Also drop the "clock-names" property as we have a single clock source for >>>>> the sensor and the driver has been updated to drop the clk referencing by >>>>> name. >>>> >>>> Driver requirements are the ABI! >>>> >>>> This breaks a kernel without the driver change and a DTB that has >>>> dropped the properties. >>>> >>> I already have a patch for the driver [0] which I missed to include >>> along with the series. >> >> You completely miss the point. Read the first sentence again. Changing >> driver requirements changes the ABI. >> >> This breaks the ABI. The driver patch does not help that. > > I'm not following you here. If the DT binding makes a mandatory property > optional, it doesn't break any existing platform. The only thing that > would not work is a new DT that doesn't contain the now optional > property combined with an older driver that makes it required. That's > not a regression, as it would be a *new* DT. You're right although in-tree DTS are now not compatible with older kernels. So it is not only about new DTS, it is about our kernel DTS which requires new kernel to work. DTS are exported and used by other systems, thus if someone blindly takes this new DTS without clock-names, his kernel/OS/bootloader might stop working. That is however a more relaxed requirement than kernel ABI against old DTS. > >>>> Also, with 'clock-names' dropped, you've just introduced a bunch of >>>> warnings on other people's platforms. Are you going to 'fix' all of >>>> them? >>>> >>> Yes I will fix them, once the patch driver patch [0] is merged in. >> >> Why? You are just making extra work. We have enough warnings as-is to >> fix. > > I agree that a DT binding change should patch all in-tree DTS to avoid > introducing new warnings. Yes. Best regards, Krzysztof
Hi Laurent, On Sat, Oct 15, 2022 at 09:25:37AM +0300, Laurent Pinchart wrote: > Hi Prabhakar, > > Thank you for the patch. > > On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Always return zero while stopping the stream as the caller will ignore the > > return value. > > > > This patch drops checking the return value of ov5645_write_reg() and > > continues further in the code path while stopping stream. The user anyway > > gets an error message in case ov5645_write_reg() fails. > > Continuing all the way to pm_runtime_put() is fine, but I don't think > the function should return 0. It's not up to the driver to decide if a > failure would be useful to signal to the caller or not. If the function returns an error when disabling streaming, what is the expected power state of the device after this? The contract between the caller and the callee is that the state is not changed if there is an error. This is a special case as very few callers check the return value for streamoff operation and those that do generally just print something. I've never seen a caller trying to prevent streaming off in this case, for instance. Of course we could document that streaming off always counts as succeeded (e.g. decreasing device's runtime PM usage_count) while it could return an informational error code. But I wonder if anyone would ever benefit from that somehow. :-) > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > v1->v2 > > * New patch > > --- > > drivers/media/i2c/ov5645.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > index a0b9d0c43b78..b3825294aaf1 100644 > > --- a/drivers/media/i2c/ov5645.c > > +++ b/drivers/media/i2c/ov5645.c > > @@ -995,14 +995,11 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable) > > if (ret < 0) > > goto err_rpm_put; > > } else { > > - ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40); > > - if (ret < 0) > > - return ret; > > + ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40); > > + > > + ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0, > > + OV5645_SYSTEM_CTRL0_STOP); > > > > - ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0, > > - OV5645_SYSTEM_CTRL0_STOP); > > - if (ret < 0) > > - return ret; > > pm_runtime_put(ov5645->dev); > > } > > >
Hi Sakari, On Sat, Oct 15, 2022 at 09:35:12PM +0000, Sakari Ailus wrote: > On Sat, Oct 15, 2022 at 09:25:37AM +0300, Laurent Pinchart wrote: > > On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote: > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > Always return zero while stopping the stream as the caller will ignore the > > > return value. > > > > > > This patch drops checking the return value of ov5645_write_reg() and > > > continues further in the code path while stopping stream. The user anyway > > > gets an error message in case ov5645_write_reg() fails. > > > > Continuing all the way to pm_runtime_put() is fine, but I don't think > > the function should return 0. It's not up to the driver to decide if a > > failure would be useful to signal to the caller or not. > > If the function returns an error when disabling streaming, what is the > expected power state of the device after this? That's up to us to decide :-) > The contract between the caller and the callee is that the state is not > changed if there is an error. For most APIs, but that's not universal. > This is a special case as very few callers > check the return value for streamoff operation and those that do generally > just print something. I've never seen a caller trying to prevent streaming > off in this case, for instance. I think the stream off call should proceed and try to power off the device even if an error occurs along the way, i.e. it shouldn't return upon the first detected error. > Of course we could document that streaming off always counts as succeeded > (e.g. decreasing device's runtime PM usage_count) while it could return an > informational error code. But I wonder if anyone would ever benefit from > that somehow. :-) I think it could be useful to propagate errors up to inform the user that something wrong happened. That would involve fixing lots of drivers along the call chain though, so there's no urgency for the ov5645 to do so, but isn't it better to propagate the error code instead of hiding the issue ? > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > --- > > > v1->v2 > > > * New patch > > > --- > > > drivers/media/i2c/ov5645.c | 11 ++++------- > > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c > > > index a0b9d0c43b78..b3825294aaf1 100644 > > > --- a/drivers/media/i2c/ov5645.c > > > +++ b/drivers/media/i2c/ov5645.c > > > @@ -995,14 +995,11 @@ static int ov5645_s_stream(struct v4l2_subdev *subdev, int enable) > > > if (ret < 0) > > > goto err_rpm_put; > > > } else { > > > - ret = ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40); > > > - if (ret < 0) > > > - return ret; > > > + ov5645_write_reg(ov5645, OV5645_IO_MIPI_CTRL00, 0x40); > > > + > > > + ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0, > > > + OV5645_SYSTEM_CTRL0_STOP); > > > > > > - ret = ov5645_write_reg(ov5645, OV5645_SYSTEM_CTRL0, > > > - OV5645_SYSTEM_CTRL0_STOP); > > > - if (ret < 0) > > > - return ret; > > > pm_runtime_put(ov5645->dev); > > > } > > >
Hi Laurent, On Sun, Oct 16, 2022 at 02:23:13AM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Sat, Oct 15, 2022 at 09:35:12PM +0000, Sakari Ailus wrote: > > On Sat, Oct 15, 2022 at 09:25:37AM +0300, Laurent Pinchart wrote: > > > On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote: > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > Always return zero while stopping the stream as the caller will ignore the > > > > return value. > > > > > > > > This patch drops checking the return value of ov5645_write_reg() and > > > > continues further in the code path while stopping stream. The user anyway > > > > gets an error message in case ov5645_write_reg() fails. > > > > > > Continuing all the way to pm_runtime_put() is fine, but I don't think > > > the function should return 0. It's not up to the driver to decide if a > > > failure would be useful to signal to the caller or not. > > > > If the function returns an error when disabling streaming, what is the > > expected power state of the device after this? > > That's up to us to decide :-) > > > The contract between the caller and the callee is that the state is not > > changed if there is an error. > > For most APIs, but that's not universal. > > > This is a special case as very few callers > > check the return value for streamoff operation and those that do generally > > just print something. I've never seen a caller trying to prevent streaming > > off in this case, for instance. > > I think the stream off call should proceed and try to power off the > device even if an error occurs along the way, i.e. it shouldn't return > upon the first detected error. > > > Of course we could document that streaming off always counts as succeeded > > (e.g. decreasing device's runtime PM usage_count) while it could return an > > informational error code. But I wonder if anyone would ever benefit from > > that somehow. :-) > > I think it could be useful to propagate errors up to inform the user > that something wrong happened. That would involve fixing lots of drivers > along the call chain though, so there's no urgency for the ov5645 to do > so, but isn't it better to propagate the error code instead of hiding > the issue ? I also don't think hiding the issue would be the best thing to do, but that wouldn't likely be a big problem either. How about printing a warning in the wrapper while returning zero to the original caller? This would keep the API intact while still leaving a trace on something failing. Of course the driver is also free to print whatever messages it likes.
Hi Sakari, On Sun, Oct 16, 2022 at 08:19:40PM +0000, Sakari Ailus wrote: > On Sun, Oct 16, 2022 at 02:23:13AM +0300, Laurent Pinchart wrote: > > On Sat, Oct 15, 2022 at 09:35:12PM +0000, Sakari Ailus wrote: > > > On Sat, Oct 15, 2022 at 09:25:37AM +0300, Laurent Pinchart wrote: > > > > On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote: > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > Always return zero while stopping the stream as the caller will ignore the > > > > > return value. > > > > > > > > > > This patch drops checking the return value of ov5645_write_reg() and > > > > > continues further in the code path while stopping stream. The user anyway > > > > > gets an error message in case ov5645_write_reg() fails. > > > > > > > > Continuing all the way to pm_runtime_put() is fine, but I don't think > > > > the function should return 0. It's not up to the driver to decide if a > > > > failure would be useful to signal to the caller or not. > > > > > > If the function returns an error when disabling streaming, what is the > > > expected power state of the device after this? > > > > That's up to us to decide :-) > > > > > The contract between the caller and the callee is that the state is not > > > changed if there is an error. > > > > For most APIs, but that's not universal. > > > > > This is a special case as very few callers > > > check the return value for streamoff operation and those that do generally > > > just print something. I've never seen a caller trying to prevent streaming > > > off in this case, for instance. > > > > I think the stream off call should proceed and try to power off the > > device even if an error occurs along the way, i.e. it shouldn't return > > upon the first detected error. > > > > > Of course we could document that streaming off always counts as succeeded > > > (e.g. decreasing device's runtime PM usage_count) while it could return an > > > informational error code. But I wonder if anyone would ever benefit from > > > that somehow. :-) > > > > I think it could be useful to propagate errors up to inform the user > > that something wrong happened. That would involve fixing lots of drivers > > along the call chain though, so there's no urgency for the ov5645 to do > > so, but isn't it better to propagate the error code instead of hiding > > the issue ? > > I also don't think hiding the issue would be the best thing to do, but that > wouldn't likely be a big problem either. > > How about printing a warning in the wrapper while returning zero to the > original caller? This would keep the API intact while still leaving a trace > on something failing. Of course the driver is also free to print whatever > messages it likes. While I think error propagation could be more useful in the long run, printing a message in the wrapper is a good idea. I like centralized error handling, it has a tendency to go wrong when left to individual drivers.
Hi Sakari, On Mon, Oct 17, 2022 at 8:12 AM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > On Mon, Oct 17, 2022 at 12:03:17AM +0300, Laurent Pinchart wrote: > > Hi Sakari, > > > > On Sun, Oct 16, 2022 at 08:19:40PM +0000, Sakari Ailus wrote: > > > On Sun, Oct 16, 2022 at 02:23:13AM +0300, Laurent Pinchart wrote: > > > > On Sat, Oct 15, 2022 at 09:35:12PM +0000, Sakari Ailus wrote: > > > > > On Sat, Oct 15, 2022 at 09:25:37AM +0300, Laurent Pinchart wrote: > > > > > > On Fri, Oct 14, 2022 at 07:34:58PM +0100, Prabhakar wrote: > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > > > > > Always return zero while stopping the stream as the caller will ignore the > > > > > > > return value. > > > > > > > > > > > > > > This patch drops checking the return value of ov5645_write_reg() and > > > > > > > continues further in the code path while stopping stream. The user anyway > > > > > > > gets an error message in case ov5645_write_reg() fails. > > > > > > > > > > > > Continuing all the way to pm_runtime_put() is fine, but I don't think > > > > > > the function should return 0. It's not up to the driver to decide if a > > > > > > failure would be useful to signal to the caller or not. > > > > > > > > > > If the function returns an error when disabling streaming, what is the > > > > > expected power state of the device after this? > > > > > > > > That's up to us to decide :-) > > > > > > > > > The contract between the caller and the callee is that the state is not > > > > > changed if there is an error. > > > > > > > > For most APIs, but that's not universal. > > > > > > > > > This is a special case as very few callers > > > > > check the return value for streamoff operation and those that do generally > > > > > just print something. I've never seen a caller trying to prevent streaming > > > > > off in this case, for instance. > > > > > > > > I think the stream off call should proceed and try to power off the > > > > device even if an error occurs along the way, i.e. it shouldn't return > > > > upon the first detected error. > > > > > > > > > Of course we could document that streaming off always counts as succeeded > > > > > (e.g. decreasing device's runtime PM usage_count) while it could return an > > > > > informational error code. But I wonder if anyone would ever benefit from > > > > > that somehow. :-) > > > > > > > > I think it could be useful to propagate errors up to inform the user > > > > that something wrong happened. That would involve fixing lots of drivers > > > > along the call chain though, so there's no urgency for the ov5645 to do > > > > so, but isn't it better to propagate the error code instead of hiding > > > > the issue ? > > > > > > I also don't think hiding the issue would be the best thing to do, but that > > > wouldn't likely be a big problem either. > > > > > > How about printing a warning in the wrapper while returning zero to the > > > original caller? This would keep the API intact while still leaving a trace > > > on something failing. Of course the driver is also free to print whatever > > > messages it likes. > > > > While I think error propagation could be more useful in the long run, > > printing a message in the wrapper is a good idea. I like centralized > > error handling, it has a tendency to go wrong when left to individual > > drivers. > > I can send a patch... > To conclude, for v3 I'll continue down the code path upon error and then report back the error? Cheers, Prabhakar
Hi Laurent, Thank you for the review. On Sat, Oct 15, 2022 at 7:26 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Fri, Oct 14, 2022 at 07:34:59PM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Make sure we call ov5645_entity_init_cfg() before registering the subdev > > to make sure default formats are set up. > > > > Suggested-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > If you have a few spare cycles, it would be even better to convert the > driver to the subdev active state API :-) You could then drop this call > entirely. > For v3 I did think of it, but it looks like I'll need to spend more time on the subdev state for this driver (as this driver does cropping which makes use of TRY/ACTIVE). So for v3 I'll keep this patch as and will work on the subdev state switch in parallel and post when complete. (Its just I dont want to miss the v6.2 window for RZ/G2L CRU driver ;-)) Cheers, Prabhakar
On Sat, Oct 15, 2022 at 12:54 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Rob, > > On Fri, Oct 14, 2022 at 04:40:29PM -0500, Rob Herring wrote: > > On Fri, Oct 14, 2022 at 10:27:53PM +0100, Lad, Prabhakar wrote: > > > On Fri, Oct 14, 2022 at 10:05 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Fri, Oct 14, 2022 at 1:35 PM Prabhakar <prabhakar.csengg@gmail.com> wrote: > > > > > > > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > Convert the simple OV5645 Device Tree binding to json-schema. > > > > > > > > > > The previous binding marked the below properties as required which was a > > > > > driver requirement and not the device requirement so just drop them from > > > > > the required list during the conversion. > > > > > - clock-frequency > > > > > - enable-gpios > > > > > - reset-gpios > > > > > > > > > > Also drop the "clock-names" property as we have a single clock source for > > > > > the sensor and the driver has been updated to drop the clk referencing by > > > > > name. > > > > > > > > Driver requirements are the ABI! > > > > > > > > This breaks a kernel without the driver change and a DTB that has > > > > dropped the properties. > > > > > > > I already have a patch for the driver [0] which I missed to include > > > along with the series. > > > > You completely miss the point. Read the first sentence again. Changing > > driver requirements changes the ABI. > > > > This breaks the ABI. The driver patch does not help that. > > I'm not following you here. If the DT binding makes a mandatory property > optional, it doesn't break any existing platform. The only thing that > would not work is a new DT that doesn't contain the now optional > property combined with an older driver that makes it required. That's > not a regression, as it would be a *new* DT. > > > > > Also, with 'clock-names' dropped, you've just introduced a bunch of > > > > warnings on other people's platforms. Are you going to 'fix' all of > > > > them? > > > > > > > Yes I will fix them, once the patch driver patch [0] is merged in. > > > > Why? You are just making extra work. We have enough warnings as-is to > > fix. > > I agree that a DT binding change should patch all in-tree DTS to avoid > introducing new warnings. That is not what I was saying. Why not just keep 'clock-names' and go spend the DTS fixing time fixing some other warnings that we already have. Also, there is no requirement that converting bindings also fix DTS files. The only wish is that any warnings we do see are ones deemed needing to be fixed in the DTS file. Anyways, there's patches now for the new warnings, so nevermind on this one. Rob
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Hi All, The main aim of this series is to add PM support to the sensor driver. I had two more patches [0] and [1] which were for ov5645, so instead sending them separately I have clubbed them as series. v1-> v2 - patch #1 is infact a v3 [1] no changes - patch #2 fixed review comments pointed by Sakari - patch #3 [0] no changes - patches #4 and #5 are new [0] https://patchwork.linuxtv.org/project/linux-media/patch/20220927202005.750621-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ [1] https://patchwork.linuxtv.org/project/linux-media/patch/20220919153540.178732-1-prabhakar.mahadev-lad.rj@bp.renesas.com/ Cheers, Prabhakar Lad Prabhakar (5): media: dt-bindings: ov5645: Convert OV5645 binding to a schema media: i2c: ov5645: Use runtime PM media: i2c: ov5645: Drop empty comment media: i2c: ov5645: Return zero for s_stream(0) media: i2c: ov5645: Call ov5645_entity_init_cfg() before registering the subdev .../devicetree/bindings/media/i2c/ov5645.txt | 54 ------- .../bindings/media/i2c/ovti,ov5645.yaml | 104 ++++++++++++ drivers/media/i2c/Kconfig | 2 +- drivers/media/i2c/ov5645.c | 151 +++++++++--------- 4 files changed, 178 insertions(+), 133 deletions(-) delete mode 100644 Documentation/devicetree/bindings/media/i2c/ov5645.txt create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov5645.yaml