| Message ID | 20221122223250.21233-15-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: imx290: Miscellaneous improvements | expand |
Hello Laurent, thanks for working on this. Am Dienstag, 22. November 2022, 23:32:49 CET schrieb Laurent Pinchart: > There's no need to configure the data lanes in the runtime PM resume > handler. Do so in imx290_start_streaming() instead. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/imx290.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > index dbed703fa199..4dfa090f918d 100644 > --- a/drivers/media/i2c/imx290.c > +++ b/drivers/media/i2c/imx290.c > @@ -753,6 +753,9 @@ static int imx290_start_streaming(struct imx290 *imx290, > return ret; > } > > + /* Set data lane count */ > + imx290_set_data_lanes(imx290); > + > /* Apply the register values related to current frame format */ > format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0); > ret = imx290_setup_format(imx290, format); > @@ -1052,9 +1055,6 @@ static int imx290_power_on(struct device *dev) > gpiod_set_value_cansleep(imx290->rst_gpio, 0); > usleep_range(30000, 31000); > > - /* Set data lane count */ > - imx290_set_data_lanes(imx290); > - > return 0; > } Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Hi Laurent On Tue, 22 Nov 2022 at 22:34, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > There's no need to configure the data lanes in the runtime PM resume > handler. Do so in imx290_start_streaming() instead. Interesting as I had Sakari advocating putting clock mode selection register control in "power on" for my recent ov9282 series. Is there any consistency? Dave https://patchwork.linuxtv.org/project/linux-media/patch/20221005152809.3785786-9-dave.stevenson@raspberrypi.com/#141118 > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/imx290.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > index dbed703fa199..4dfa090f918d 100644 > --- a/drivers/media/i2c/imx290.c > +++ b/drivers/media/i2c/imx290.c > @@ -753,6 +753,9 @@ static int imx290_start_streaming(struct imx290 *imx290, > return ret; > } > > + /* Set data lane count */ > + imx290_set_data_lanes(imx290); > + > /* Apply the register values related to current frame format */ > format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0); > ret = imx290_setup_format(imx290, format); > @@ -1052,9 +1055,6 @@ static int imx290_power_on(struct device *dev) > gpiod_set_value_cansleep(imx290->rst_gpio, 0); > usleep_range(30000, 31000); > > - /* Set data lane count */ > - imx290_set_data_lanes(imx290); > - > return 0; > } > > -- > Regards, > > Laurent Pinchart >
Hi Dave, On Wed, Nov 23, 2022 at 10:16:58AM +0000, Dave Stevenson wrote: > On Tue, 22 Nov 2022 at 22:34, Laurent Pinchart wrote: > > > > There's no need to configure the data lanes in the runtime PM resume > > handler. Do so in imx290_start_streaming() instead. > > Interesting as I had Sakari advocating putting clock mode selection > register control in "power on" for my recent ov9282 series. Is there > any consistency? No there isn't :-) There hasn't been any official rule so far, so it's no surprise different drivers exhibit different behaviours. I'm all for standardization when possible though. Overall, I think there's a general agreement that the runtime PM resume handler needs to control everything required to make the sensor accessible by software. That covers at least hard reset, regulators and clocks. For software settings, it's less clear. If the sensor requires a software reset sequence immediately after power on, it makes sense to also handle that in the runtime PM resume handler. Same thing for any other initialization required to reach a quiescent state (for instance there are many sensors that start streaming automatically right after power on for a reason I can't understand, so that needs to be disabled). This means that the runtime PM handler will thus access the sensor over I2C. We may not want to do so in probe() before having a chance to probe it (by reading an ID register for instance). The power on sequence could be split in two to handle this, with one function that powers the sensor up, and the other one that handles software initialization. Both would be called from the runtime PM resume handler, and in probe(), we could first power on the sensor, identify it, and then initialize it. I think that will be fine on DT platforms as we don't need to RPM-resume the I2C device in probe before accessing it as far as I can tell, given that the probe() function should be called with the I2C controller RPM-resumed. I'll let Sakari confirms if this works for ACPI). For other settings, I wouldn't handle them in the runtime PM resume handler. In this particular case, the number of data lanes could even vary based on the sensor mode (we don't do so at the moment), so .s_stream() time seems better to me. > https://patchwork.linuxtv.org/project/linux-media/patch/20221005152809.3785786-9-dave.stevenson@raspberrypi.com/#141118 > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/media/i2c/imx290.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > index dbed703fa199..4dfa090f918d 100644 > > --- a/drivers/media/i2c/imx290.c > > +++ b/drivers/media/i2c/imx290.c > > @@ -753,6 +753,9 @@ static int imx290_start_streaming(struct imx290 *imx290, > > return ret; > > } > > > > + /* Set data lane count */ > > + imx290_set_data_lanes(imx290); > > + > > /* Apply the register values related to current frame format */ > > format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0); > > ret = imx290_setup_format(imx290, format); > > @@ -1052,9 +1055,6 @@ static int imx290_power_on(struct device *dev) > > gpiod_set_value_cansleep(imx290->rst_gpio, 0); > > usleep_range(30000, 31000); > > > > - /* Set data lane count */ > > - imx290_set_data_lanes(imx290); > > - > > return 0; > > } > >
Hi Laurent On Wed, 23 Nov 2022 at 14:29, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Dave, > > On Wed, Nov 23, 2022 at 10:16:58AM +0000, Dave Stevenson wrote: > > On Tue, 22 Nov 2022 at 22:34, Laurent Pinchart wrote: > > > > > > There's no need to configure the data lanes in the runtime PM resume > > > handler. Do so in imx290_start_streaming() instead. > > > > Interesting as I had Sakari advocating putting clock mode selection > > register control in "power on" for my recent ov9282 series. Is there > > any consistency? > > No there isn't :-) There hasn't been any official rule so far, so it's > no surprise different drivers exhibit different behaviours. I'm all for > standardization when possible though. Likewise! Standardisation is a good thing! Sorry my comment was slightly tongue-in-cheek due to having had such a similar thread with Sakari so recently. When a long-standing member of the community then comes along with a similar patch it just reinforced that, in the absence of any documented guidelines, it is all very much ad-hoc. It then frustrates me that these sorts of issues are then raised at review, which either results in having to justify the choice, or respinning patches often with time constraints if trying to hit a merge window. > Overall, I think there's a general agreement that the runtime PM resume > handler needs to control everything required to make the sensor > accessible by software. That covers at least hard reset, regulators and > clocks. > > For software settings, it's less clear. If the sensor requires a > software reset sequence immediately after power on, it makes sense to > also handle that in the runtime PM resume handler. Same thing for any > other initialization required to reach a quiescent state (for instance > there are many sensors that start streaming automatically right after > power on for a reason I can't understand, so that needs to be disabled). > > This means that the runtime PM handler will thus access the sensor over > I2C. We may not want to do so in probe() before having a chance to probe > it (by reading an ID register for instance). The power on sequence could > be split in two to handle this, with one function that powers the sensor > up, and the other one that handles software initialization. Both would > be called from the runtime PM resume handler, and in probe(), we could > first power on the sensor, identify it, and then initialize it. I think > that will be fine on DT platforms as we don't need to RPM-resume the I2C > device in probe before accessing it as far as I can tell, given that the > probe() function should be called with the I2C controller RPM-resumed. > I'll let Sakari confirms if this works for ACPI). For ov9282 I'd also raised the issue that a fair number of sensor drivers include a software reset in their lists of registers, which will undo any settings done in resume. As above, it was more of an observation than issue with this patch. Alexander has already given an R-b, so there's limited point adding mine. I'll try and test the series out tomorrow, and I will get around to submitting my series on top. Cheers. Dave > For other settings, I wouldn't handle them in the runtime PM resume > handler. In this particular case, the number of data lanes could even > vary based on the sensor mode (we don't do so at the moment), so > .s_stream() time seems better to me. > > > https://patchwork.linuxtv.org/project/linux-media/patch/20221005152809.3785786-9-dave.stevenson@raspberrypi.com/#141118 > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > drivers/media/i2c/imx290.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > > index dbed703fa199..4dfa090f918d 100644 > > > --- a/drivers/media/i2c/imx290.c > > > +++ b/drivers/media/i2c/imx290.c > > > @@ -753,6 +753,9 @@ static int imx290_start_streaming(struct imx290 *imx290, > > > return ret; > > > } > > > > > > + /* Set data lane count */ > > > + imx290_set_data_lanes(imx290); > > > + > > > /* Apply the register values related to current frame format */ > > > format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0); > > > ret = imx290_setup_format(imx290, format); > > > @@ -1052,9 +1055,6 @@ static int imx290_power_on(struct device *dev) > > > gpiod_set_value_cansleep(imx290->rst_gpio, 0); > > > usleep_range(30000, 31000); > > > > > > - /* Set data lane count */ > > > - imx290_set_data_lanes(imx290); > > > - > > > return 0; > > > } > > > > > -- > Regards, > > Laurent Pinchart
Hi Dave, On Thu, Nov 24, 2022 at 06:02:54PM +0000, Dave Stevenson wrote: > On Wed, 23 Nov 2022 at 14:29, Laurent Pinchart wrote: > > On Wed, Nov 23, 2022 at 10:16:58AM +0000, Dave Stevenson wrote: > > > On Tue, 22 Nov 2022 at 22:34, Laurent Pinchart wrote: > > > > > > > > There's no need to configure the data lanes in the runtime PM resume > > > > handler. Do so in imx290_start_streaming() instead. > > > > > > Interesting as I had Sakari advocating putting clock mode selection > > > register control in "power on" for my recent ov9282 series. Is there > > > any consistency? > > > > No there isn't :-) There hasn't been any official rule so far, so it's > > no surprise different drivers exhibit different behaviours. I'm all for > > standardization when possible though. > > Likewise! Standardisation is a good thing! > > Sorry my comment was slightly tongue-in-cheek due to having had such a > similar thread with Sakari so recently. When a long-standing member of > the community then comes along with a similar patch it just reinforced > that, in the absence of any documented guidelines, it is all very much > ad-hoc. > It then frustrates me that these sorts of issues are then raised at > review, which either results in having to justify the choice, or > respinning patches often with time constraints if trying to hit a > merge window. I understand your frustration, and have experienced it too myself at times. We're trying to align our messages, unfortunately it's mostly ad-hoc. There are multiple reasons for that, sometimes we realize that we've being done things wrong for a long time, or we need to experiment to find the best option. The media subsystem being understaffed doesn't help, and I think it also drives contributors away as a result, which makes the problem worse. With more time, I would really like to work on standardization of camera sensor APIs in the media subsystem, both in-kernel and towards userspace. We've discussed that in Dublin, you know how painful it is today. > > Overall, I think there's a general agreement that the runtime PM resume > > handler needs to control everything required to make the sensor > > accessible by software. That covers at least hard reset, regulators and > > clocks. > > > > For software settings, it's less clear. If the sensor requires a > > software reset sequence immediately after power on, it makes sense to > > also handle that in the runtime PM resume handler. Same thing for any > > other initialization required to reach a quiescent state (for instance > > there are many sensors that start streaming automatically right after > > power on for a reason I can't understand, so that needs to be disabled). > > > > This means that the runtime PM handler will thus access the sensor over > > I2C. We may not want to do so in probe() before having a chance to probe > > it (by reading an ID register for instance). The power on sequence could > > be split in two to handle this, with one function that powers the sensor > > up, and the other one that handles software initialization. Both would > > be called from the runtime PM resume handler, and in probe(), we could > > first power on the sensor, identify it, and then initialize it. I think > > that will be fine on DT platforms as we don't need to RPM-resume the I2C > > device in probe before accessing it as far as I can tell, given that the > > probe() function should be called with the I2C controller RPM-resumed. > > I'll let Sakari confirms if this works for ACPI). > > For ov9282 I'd also raised the issue that a fair number of sensor > drivers include a software reset in their lists of registers, which > will undo any settings done in resume. > > As above, it was more of an observation than issue with this patch. > Alexander has already given an R-b, so there's limited point adding mine. > > I'll try and test the series out tomorrow, and I will get around to > submitting my series on top. Great, I'll then test your patches on my board :-) > > For other settings, I wouldn't handle them in the runtime PM resume > > handler. In this particular case, the number of data lanes could even > > vary based on the sensor mode (we don't do so at the moment), so > > .s_stream() time seems better to me. > > > > > https://patchwork.linuxtv.org/project/linux-media/patch/20221005152809.3785786-9-dave.stevenson@raspberrypi.com/#141118 > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > drivers/media/i2c/imx290.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c > > > > index dbed703fa199..4dfa090f918d 100644 > > > > --- a/drivers/media/i2c/imx290.c > > > > +++ b/drivers/media/i2c/imx290.c > > > > @@ -753,6 +753,9 @@ static int imx290_start_streaming(struct imx290 *imx290, > > > > return ret; > > > > } > > > > > > > > + /* Set data lane count */ > > > > + imx290_set_data_lanes(imx290); > > > > + > > > > /* Apply the register values related to current frame format */ > > > > format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0); > > > > ret = imx290_setup_format(imx290, format); > > > > @@ -1052,9 +1055,6 @@ static int imx290_power_on(struct device *dev) > > > > gpiod_set_value_cansleep(imx290->rst_gpio, 0); > > > > usleep_range(30000, 31000); > > > > > > > > - /* Set data lane count */ > > > > - imx290_set_data_lanes(imx290); > > > > - > > > > return 0; > > > > } > > > >
diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c index dbed703fa199..4dfa090f918d 100644 --- a/drivers/media/i2c/imx290.c +++ b/drivers/media/i2c/imx290.c @@ -753,6 +753,9 @@ static int imx290_start_streaming(struct imx290 *imx290, return ret; } + /* Set data lane count */ + imx290_set_data_lanes(imx290); + /* Apply the register values related to current frame format */ format = v4l2_subdev_get_pad_format(&imx290->sd, state, 0); ret = imx290_setup_format(imx290, format); @@ -1052,9 +1055,6 @@ static int imx290_power_on(struct device *dev) gpiod_set_value_cansleep(imx290->rst_gpio, 0); usleep_range(30000, 31000); - /* Set data lane count */ - imx290_set_data_lanes(imx290); - return 0; }
There's no need to configure the data lanes in the runtime PM resume handler. Do so in imx290_start_streaming() instead. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/i2c/imx290.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)