| Message ID | 20221120224101.746199-18-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | 5317baa0a39ec480d6b225cca639b2fb19583515 |
| Headers | show |
| Series | media: atomisp: Misc. cleanups / fixes | expand |
On Sun, Nov 20, 2022 at 11:40:58PM +0100, Hans de Goede wrote: > atomisp_css_init() is always called after calling atomisp_power_on() > either directly or through getting a runtime-pm reference. > > Likewise atomisp_css_uninit() is always called after calling > atomisp_power_off(). > > Move the call site of these 2 functions to inside atomisp_power_on() / > atomisp_power_off() to make this more explicit. ... > dev_dbg(isp->dev, "%s\n", __func__); > - atomisp_css_uninit(isp); > + > ret = atomisp_power_off(isp->dev); > if (ret < 0) > dev_err(isp->dev, "atomisp_power_off failed, %d\n", ret); > > ret = atomisp_power_on(isp->dev); > - if (ret < 0) > + if (ret < 0) { > dev_err(isp->dev, "atomisp_power_on failed, %d\n", ret); > - > - ret = atomisp_css_init(isp); > - if (ret) > isp->isp_fatal_error = true; This was only set when css_init() failed, now it may be set even when power_on() fails. Why is it not a problem? Commit message doesn't shed a light on this change. > + }
Hi, On 11/21/22 10:14, Andy Shevchenko wrote: > On Sun, Nov 20, 2022 at 11:40:58PM +0100, Hans de Goede wrote: >> atomisp_css_init() is always called after calling atomisp_power_on() >> either directly or through getting a runtime-pm reference. >> >> Likewise atomisp_css_uninit() is always called after calling >> atomisp_power_off(). >> >> Move the call site of these 2 functions to inside atomisp_power_on() / >> atomisp_power_off() to make this more explicit. > > ... > >> dev_dbg(isp->dev, "%s\n", __func__); >> - atomisp_css_uninit(isp); >> + >> ret = atomisp_power_off(isp->dev); >> if (ret < 0) >> dev_err(isp->dev, "atomisp_power_off failed, %d\n", ret); >> >> ret = atomisp_power_on(isp->dev); >> - if (ret < 0) >> + if (ret < 0) { >> dev_err(isp->dev, "atomisp_power_on failed, %d\n", ret); >> - >> - ret = atomisp_css_init(isp); >> - if (ret) > >> isp->isp_fatal_error = true; > > This was only set when css_init() failed, now it may be set even when > power_on() fails. Why is it not a problem? Commit message doesn't shed > a light on this change. Failing to power-on the device is pretty fatal too, but this is indeed a behavior change. I have updated the commit message to reflect this. Regards, Hans
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c index dac19db55625..7821bbd1e8da 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c +++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c @@ -302,18 +302,16 @@ int atomisp_reset(struct atomisp_device *isp) int ret = 0; dev_dbg(isp->dev, "%s\n", __func__); - atomisp_css_uninit(isp); + ret = atomisp_power_off(isp->dev); if (ret < 0) dev_err(isp->dev, "atomisp_power_off failed, %d\n", ret); ret = atomisp_power_on(isp->dev); - if (ret < 0) + if (ret < 0) { dev_err(isp->dev, "atomisp_power_on failed, %d\n", ret); - - ret = atomisp_css_init(isp); - if (ret) isp->isp_fatal_error = true; + } return ret; } diff --git a/drivers/staging/media/atomisp/pci/atomisp_fops.c b/drivers/staging/media/atomisp/pci/atomisp_fops.c index 8cff26d42b82..0123429d94f4 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_fops.c +++ b/drivers/staging/media/atomisp/pci/atomisp_fops.c @@ -809,13 +809,6 @@ static int atomisp_open(struct file *file) goto error; } - /* Init ISP */ - if (atomisp_css_init(isp)) { - ret = -EINVAL; - /* Need to clean up CSS init if it fails. */ - goto css_error; - } - atomisp_dev_init_struct(isp); ret = v4l2_subdev_call(isp->flash, core, s_power, 1); @@ -840,7 +833,6 @@ static int atomisp_open(struct file *file) return 0; css_error: - atomisp_css_uninit(isp); pm_runtime_put(vdev->v4l2_dev->dev); error: mutex_unlock(&isp->mutex); @@ -909,7 +901,6 @@ static int atomisp_release(struct file *file) goto done; atomisp_destroy_pipes_stream_force(asd); - atomisp_css_uninit(isp); if (defer_fw_load) { ia_css_unload_firmware(); diff --git a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c index f670517bc141..f46046d7ef50 100644 --- a/drivers/staging/media/atomisp/pci/atomisp_v4l2.c +++ b/drivers/staging/media/atomisp/pci/atomisp_v4l2.c @@ -726,6 +726,8 @@ int atomisp_power_off(struct device *dev) dev_get_drvdata(dev); int ret; + atomisp_css_uninit(isp); + ret = atomisp_mrfld_pre_power_down(isp); if (ret) return ret; @@ -761,7 +763,8 @@ int atomisp_power_on(struct device *dev) atomisp_restore_iunit_reg(isp); atomisp_freq_scaling(isp, ATOMISP_DFS_MODE_LOW, true); - return 0; + + return atomisp_css_init(isp); } static int __maybe_unused atomisp_suspend(struct device *dev)
atomisp_css_init() is always called after calling atomisp_power_on() either directly or through getting a runtime-pm reference. Likewise atomisp_css_uninit() is always called after calling atomisp_power_off(). Move the call site of these 2 functions to inside atomisp_power_on() / atomisp_power_off() to make this more explicit. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/staging/media/atomisp/pci/atomisp_cmd.c | 8 +++----- drivers/staging/media/atomisp/pci/atomisp_fops.c | 9 --------- drivers/staging/media/atomisp/pci/atomisp_v4l2.c | 5 ++++- 3 files changed, 7 insertions(+), 15 deletions(-)