| Message ID | 20221014183459.181567-1-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| Headers | show |
| Series | media: i2c: ov5645 driver enhancements | expand |
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
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 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 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
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