| Message ID | 20221122132330.30408-1-lukas.bulwahn@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: remove code for non-existing config IMX_GPT_ICAP | expand |
Hi Lukas, On Tue, Nov 22, 2022 at 02:23:30PM +0100, Lukas Bulwahn wrote: > There never was a config IMX_GPT_ICAP in the repository. So remove the code > conditional on this config and simplify the callers that just called empty > functions. > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com> > --- > drivers/staging/media/imx/imx-media-fim.c | 54 ----------------------- > 1 file changed, 54 deletions(-) > > diff --git a/drivers/staging/media/imx/imx-media-fim.c b/drivers/staging/media/imx/imx-media-fim.c > index 3a9182933508..19a48eb445fd 100644 > --- a/drivers/staging/media/imx/imx-media-fim.c > +++ b/drivers/staging/media/imx/imx-media-fim.c > @@ -187,54 +187,6 @@ static void frame_interval_monitor(struct imx_media_fim *fim, > send_fim_event(fim, error_avg); > } > > -#ifdef CONFIG_IMX_GPT_ICAP > -/* > - * Input Capture method of measuring frame intervals. Not subject > - * to interrupt latency. > - */ > -static void fim_input_capture_handler(int channel, void *dev_id, > - ktime_t timestamp) > -{ > - struct imx_media_fim *fim = dev_id; > - unsigned long flags; > - > - spin_lock_irqsave(&fim->lock, flags); > - > - frame_interval_monitor(fim, timestamp); > - > - if (!completion_done(&fim->icap_first_event)) > - complete(&fim->icap_first_event); > - > - spin_unlock_irqrestore(&fim->lock, flags); > -} > - > -static int fim_request_input_capture(struct imx_media_fim *fim) > -{ > - init_completion(&fim->icap_first_event); > - > - return mxc_request_input_capture(fim->icap_channel, > - fim_input_capture_handler, > - fim->icap_flags, fim); > -} > - > -static void fim_free_input_capture(struct imx_media_fim *fim) > -{ > - mxc_free_input_capture(fim->icap_channel, fim); > -} > - > -#else /* CONFIG_IMX_GPT_ICAP */ > - > -static int fim_request_input_capture(struct imx_media_fim *fim) > -{ > - return 0; > -} > - > -static void fim_free_input_capture(struct imx_media_fim *fim) > -{ > -} > - > -#endif /* CONFIG_IMX_GPT_ICAP */ > - > /* > * In case we are monitoring the first frame interval after streamon > * (when fim->num_skip = 0), we need a valid fim->last_ts before we > @@ -435,14 +387,8 @@ int imx_media_fim_set_stream(struct imx_media_fim *fim, > spin_unlock_irqrestore(&fim->lock, flags); > > if (icap_enabled(fim)) { > - ret = fim_request_input_capture(fim); > - if (ret) > - goto out; > fim_acquire_first_ts(fim); > } As suggested by the checkpatch.pl: WARNING: braces {} are not necessary for single statement blocks > - } else { > - if (icap_enabled(fim)) > - fim_free_input_capture(fim); > } > > fim->stream_on = on; > -- > 2.17.1 > Apart of this, patch looks good to me. Thanks. Regards, Tommaso
On Tue, Nov 22, 2022 at 5:09 PM Tommaso Merciai <tommaso.merciai@amarulasolutions.com> wrote: > > Hi Lukas, > > On Tue, Nov 22, 2022 at 02:23:30PM +0100, Lukas Bulwahn wrote: > > There never was a config IMX_GPT_ICAP in the repository. So remove the code > > conditional on this config and simplify the callers that just called empty > > functions. > > > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com> > > --- > > drivers/staging/media/imx/imx-media-fim.c | 54 ----------------------- > > 1 file changed, 54 deletions(-) > > > > diff --git a/drivers/staging/media/imx/imx-media-fim.c b/drivers/staging/media/imx/imx-media-fim.c > > index 3a9182933508..19a48eb445fd 100644 > > --- a/drivers/staging/media/imx/imx-media-fim.c > > +++ b/drivers/staging/media/imx/imx-media-fim.c > > @@ -187,54 +187,6 @@ static void frame_interval_monitor(struct imx_media_fim *fim, > > send_fim_event(fim, error_avg); > > } > > > > -#ifdef CONFIG_IMX_GPT_ICAP > > -/* > > - * Input Capture method of measuring frame intervals. Not subject > > - * to interrupt latency. > > - */ > > -static void fim_input_capture_handler(int channel, void *dev_id, > > - ktime_t timestamp) > > -{ > > - struct imx_media_fim *fim = dev_id; > > - unsigned long flags; > > - > > - spin_lock_irqsave(&fim->lock, flags); > > - > > - frame_interval_monitor(fim, timestamp); > > - > > - if (!completion_done(&fim->icap_first_event)) > > - complete(&fim->icap_first_event); > > - > > - spin_unlock_irqrestore(&fim->lock, flags); > > -} > > - > > -static int fim_request_input_capture(struct imx_media_fim *fim) > > -{ > > - init_completion(&fim->icap_first_event); > > - > > - return mxc_request_input_capture(fim->icap_channel, > > - fim_input_capture_handler, > > - fim->icap_flags, fim); > > -} > > - > > -static void fim_free_input_capture(struct imx_media_fim *fim) > > -{ > > - mxc_free_input_capture(fim->icap_channel, fim); > > -} > > - > > -#else /* CONFIG_IMX_GPT_ICAP */ > > - > > -static int fim_request_input_capture(struct imx_media_fim *fim) > > -{ > > - return 0; > > -} > > - > > -static void fim_free_input_capture(struct imx_media_fim *fim) > > -{ > > -} > > - > > -#endif /* CONFIG_IMX_GPT_ICAP */ > > - > > /* > > * In case we are monitoring the first frame interval after streamon > > * (when fim->num_skip = 0), we need a valid fim->last_ts before we > > @@ -435,14 +387,8 @@ int imx_media_fim_set_stream(struct imx_media_fim *fim, > > spin_unlock_irqrestore(&fim->lock, flags); > > > > if (icap_enabled(fim)) { > > - ret = fim_request_input_capture(fim); > > - if (ret) > > - goto out; > > fim_acquire_first_ts(fim); > > } > > As suggested by the checkpatch.pl: > WARNING: braces {} are not necessary for single statement blocks > > > - } else { > > - if (icap_enabled(fim)) > > - fim_free_input_capture(fim); > > } > > > > fim->stream_on = on; > > -- > > 2.17.1 > > > Strangely, my checkpatch invocation on next-20221116 did not show that warning and I am unaware that I made any specific changes to checkpatch or its configuration. Anyway, checkpatch and you, Tommaso, are right and the patch can of course be improved. I sent out a patch v2: https://lore.kernel.org/all/20221123075216.28193-1-lukas.bulwahn@gmail.com/ I hope to get an Acked-by: tag from you now. Thanks. Lukas > Apart of this, patch looks good to me. > Thanks. > > Regards, > Tommaso > > -- > Tommaso Merciai > Embedded Linux Engineer > tommaso.merciai@amarulasolutions.com > __________________________________ > > Amarula Solutions SRL > Via Le Canevare 30, 31100 Treviso, Veneto, IT > T. +39 042 243 5310 > info@amarulasolutions.com > www.amarulasolutions.com
On Wed, Nov 23, 2022 at 08:56:06AM +0100, Lukas Bulwahn wrote: > > Strangely, my checkpatch invocation on next-20221116 did not show that > warning and I am unaware that I made any specific changes to > checkpatch or its configuration. > I think you need to apply the patch and retest with the -f option. regards, dan carpenter
Hi Lukas, On Wed, Nov 23, 2022 at 08:56:06AM +0100, Lukas Bulwahn wrote: > On Tue, Nov 22, 2022 at 5:09 PM Tommaso Merciai > <tommaso.merciai@amarulasolutions.com> wrote: > > > > Hi Lukas, > > > > On Tue, Nov 22, 2022 at 02:23:30PM +0100, Lukas Bulwahn wrote: > > > There never was a config IMX_GPT_ICAP in the repository. So remove the code > > > conditional on this config and simplify the callers that just called empty > > > functions. > > > > > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com> > > > --- > > > drivers/staging/media/imx/imx-media-fim.c | 54 ----------------------- > > > 1 file changed, 54 deletions(-) > > > > > > diff --git a/drivers/staging/media/imx/imx-media-fim.c b/drivers/staging/media/imx/imx-media-fim.c > > > index 3a9182933508..19a48eb445fd 100644 > > > --- a/drivers/staging/media/imx/imx-media-fim.c > > > +++ b/drivers/staging/media/imx/imx-media-fim.c > > > @@ -187,54 +187,6 @@ static void frame_interval_monitor(struct imx_media_fim *fim, > > > send_fim_event(fim, error_avg); > > > } > > > > > > -#ifdef CONFIG_IMX_GPT_ICAP > > > -/* > > > - * Input Capture method of measuring frame intervals. Not subject > > > - * to interrupt latency. > > > - */ > > > -static void fim_input_capture_handler(int channel, void *dev_id, > > > - ktime_t timestamp) > > > -{ > > > - struct imx_media_fim *fim = dev_id; > > > - unsigned long flags; > > > - > > > - spin_lock_irqsave(&fim->lock, flags); > > > - > > > - frame_interval_monitor(fim, timestamp); > > > - > > > - if (!completion_done(&fim->icap_first_event)) > > > - complete(&fim->icap_first_event); > > > - > > > - spin_unlock_irqrestore(&fim->lock, flags); > > > -} > > > - > > > -static int fim_request_input_capture(struct imx_media_fim *fim) > > > -{ > > > - init_completion(&fim->icap_first_event); > > > - > > > - return mxc_request_input_capture(fim->icap_channel, > > > - fim_input_capture_handler, > > > - fim->icap_flags, fim); > > > -} > > > - > > > -static void fim_free_input_capture(struct imx_media_fim *fim) > > > -{ > > > - mxc_free_input_capture(fim->icap_channel, fim); > > > -} > > > - > > > -#else /* CONFIG_IMX_GPT_ICAP */ > > > - > > > -static int fim_request_input_capture(struct imx_media_fim *fim) > > > -{ > > > - return 0; > > > -} > > > - > > > -static void fim_free_input_capture(struct imx_media_fim *fim) > > > -{ > > > -} > > > - > > > -#endif /* CONFIG_IMX_GPT_ICAP */ > > > - > > > /* > > > * In case we are monitoring the first frame interval after streamon > > > * (when fim->num_skip = 0), we need a valid fim->last_ts before we > > > @@ -435,14 +387,8 @@ int imx_media_fim_set_stream(struct imx_media_fim *fim, > > > spin_unlock_irqrestore(&fim->lock, flags); > > > > > > if (icap_enabled(fim)) { > > > - ret = fim_request_input_capture(fim); > > > - if (ret) > > > - goto out; > > > fim_acquire_first_ts(fim); > > > } > > > > As suggested by the checkpatch.pl: > > WARNING: braces {} are not necessary for single statement blocks > > > > > - } else { > > > - if (icap_enabled(fim)) > > > - fim_free_input_capture(fim); > > > } > > > > > > fim->stream_on = on; > > > -- > > > 2.17.1 > > > > > > > Strangely, my checkpatch invocation on next-20221116 did not show that > warning and I am unaware that I made any specific changes to > checkpatch or its configuration. > > Anyway, checkpatch and you, Tommaso, are right and the patch can of > course be improved. I sent out a patch v2: > > https://lore.kernel.org/all/20221123075216.28193-1-lukas.bulwahn@gmail.com/ > > I hope to get an Acked-by: tag from you now. Thanks. Sorry I miss the tag :) Acked-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> Apply the patch then run: scripts/checkpatch.pl -f drivers/staging/media/imx/imx-media-fim.c Regards, Tommaso > > Lukas > > > Apart of this, patch looks good to me. > > Thanks. > > > > Regards, > > Tommaso > > > > -- > > Tommaso Merciai > > Embedded Linux Engineer > > tommaso.merciai@amarulasolutions.com > > __________________________________ > > > > Amarula Solutions SRL > > Via Le Canevare 30, 31100 Treviso, Veneto, IT > > T. +39 042 243 5310 > > info@amarulasolutions.com > > www.amarulasolutions.com
On Wed, Nov 23, 2022 at 9:21 AM Tommaso Merciai <tommaso.merciai@amarulasolutions.com> wrote: > > Hi Lukas, > > On Wed, Nov 23, 2022 at 08:56:06AM +0100, Lukas Bulwahn wrote: > > On Tue, Nov 22, 2022 at 5:09 PM Tommaso Merciai > > <tommaso.merciai@amarulasolutions.com> wrote: > > > > > > Hi Lukas, > > > > > > On Tue, Nov 22, 2022 at 02:23:30PM +0100, Lukas Bulwahn wrote: > > > > There never was a config IMX_GPT_ICAP in the repository. So remove the code > > > > conditional on this config and simplify the callers that just called empty > > > > functions. > > > > > > > > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com> > > > > --- > > > > drivers/staging/media/imx/imx-media-fim.c | 54 ----------------------- > > > > 1 file changed, 54 deletions(-) > > > > > > > > diff --git a/drivers/staging/media/imx/imx-media-fim.c b/drivers/staging/media/imx/imx-media-fim.c > > > > index 3a9182933508..19a48eb445fd 100644 > > > > --- a/drivers/staging/media/imx/imx-media-fim.c > > > > +++ b/drivers/staging/media/imx/imx-media-fim.c > > > > @@ -187,54 +187,6 @@ static void frame_interval_monitor(struct imx_media_fim *fim, > > > > send_fim_event(fim, error_avg); > > > > } > > > > > > > > -#ifdef CONFIG_IMX_GPT_ICAP > > > > -/* > > > > - * Input Capture method of measuring frame intervals. Not subject > > > > - * to interrupt latency. > > > > - */ > > > > -static void fim_input_capture_handler(int channel, void *dev_id, > > > > - ktime_t timestamp) > > > > -{ > > > > - struct imx_media_fim *fim = dev_id; > > > > - unsigned long flags; > > > > - > > > > - spin_lock_irqsave(&fim->lock, flags); > > > > - > > > > - frame_interval_monitor(fim, timestamp); > > > > - > > > > - if (!completion_done(&fim->icap_first_event)) > > > > - complete(&fim->icap_first_event); > > > > - > > > > - spin_unlock_irqrestore(&fim->lock, flags); > > > > -} > > > > - > > > > -static int fim_request_input_capture(struct imx_media_fim *fim) > > > > -{ > > > > - init_completion(&fim->icap_first_event); > > > > - > > > > - return mxc_request_input_capture(fim->icap_channel, > > > > - fim_input_capture_handler, > > > > - fim->icap_flags, fim); > > > > -} > > > > - > > > > -static void fim_free_input_capture(struct imx_media_fim *fim) > > > > -{ > > > > - mxc_free_input_capture(fim->icap_channel, fim); > > > > -} > > > > - > > > > -#else /* CONFIG_IMX_GPT_ICAP */ > > > > - > > > > -static int fim_request_input_capture(struct imx_media_fim *fim) > > > > -{ > > > > - return 0; > > > > -} > > > > - > > > > -static void fim_free_input_capture(struct imx_media_fim *fim) > > > > -{ > > > > -} > > > > - > > > > -#endif /* CONFIG_IMX_GPT_ICAP */ > > > > - > > > > /* > > > > * In case we are monitoring the first frame interval after streamon > > > > * (when fim->num_skip = 0), we need a valid fim->last_ts before we > > > > @@ -435,14 +387,8 @@ int imx_media_fim_set_stream(struct imx_media_fim *fim, > > > > spin_unlock_irqrestore(&fim->lock, flags); > > > > > > > > if (icap_enabled(fim)) { > > > > - ret = fim_request_input_capture(fim); > > > > - if (ret) > > > > - goto out; > > > > fim_acquire_first_ts(fim); > > > > } > > > > > > As suggested by the checkpatch.pl: > > > WARNING: braces {} are not necessary for single statement blocks > > > > > > > - } else { > > > > - if (icap_enabled(fim)) > > > > - fim_free_input_capture(fim); > > > > } > > > > > > > > fim->stream_on = on; > > > > -- > > > > 2.17.1 > > > > > > > > > > > Strangely, my checkpatch invocation on next-20221116 did not show that > > warning and I am unaware that I made any specific changes to > > checkpatch or its configuration. > > > > Anyway, checkpatch and you, Tommaso, are right and the patch can of > > course be improved. I sent out a patch v2: > > > > https://lore.kernel.org/all/20221123075216.28193-1-lukas.bulwahn@gmail.com/ > > > > I hope to get an Acked-by: tag from you now. Thanks. > > Sorry I miss the tag :) > > Acked-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com> > > Apply the patch then run: > scripts/checkpatch.pl -f drivers/staging/media/imx/imx-media-fim.c > I see. I usually just run checkpatch.pl on the patch, but do not apply the patch and run checkpatch on the file. That explains the difference of observations. Good to know. Lukas > Regards, > Tommaso > > > > > Lukas > > > > > Apart of this, patch looks good to me. > > > Thanks. > > > > > > Regards, > > > Tommaso > > > > > > -- > > > Tommaso Merciai > > > Embedded Linux Engineer > > > tommaso.merciai@amarulasolutions.com > > > __________________________________ > > > > > > Amarula Solutions SRL > > > Via Le Canevare 30, 31100 Treviso, Veneto, IT > > > T. +39 042 243 5310 > > > info@amarulasolutions.com > > > www.amarulasolutions.com > > -- > Tommaso Merciai > Embedded Linux Engineer > tommaso.merciai@amarulasolutions.com > __________________________________ > > Amarula Solutions SRL > Via Le Canevare 30, 31100 Treviso, Veneto, IT > T. +39 042 243 5310 > info@amarulasolutions.com > www.amarulasolutions.com
On Wed, Nov 23, 2022 at 8:59 AM Dan Carpenter <error27@gmail.com> wrote: > > On Wed, Nov 23, 2022 at 08:56:06AM +0100, Lukas Bulwahn wrote: > > > > Strangely, my checkpatch invocation on next-20221116 did not show that > > warning and I am unaware that I made any specific changes to > > checkpatch or its configuration. > > > > I think you need to apply the patch and retest with the -f option. > Thanks, Dan, for the hint. Now, I know. I need to see if this can become part of my usual patch submission workflow. Lukas > regards, > dan carpenter >
On Wed, Nov 23, 2022 at 09:24:42AM +0100, Lukas Bulwahn wrote: > On Wed, Nov 23, 2022 at 9:21 AM Tommaso Merciai > <tommaso.merciai@amarulasolutions.com> wrote: > > > > I see. I usually just run checkpatch.pl on the patch, but do not apply > the patch and run checkpatch on the file. That explains the difference > of observations. Good to know. There is a section "Git post-commit hooks" on this [1] web-link that is useful to configure auto-checkpatch verification at the time of commit. Hope this is useful. [1] https://kernelnewbies.org/Outreachyfirstpatch ./drv > > Lukas
diff --git a/drivers/staging/media/imx/imx-media-fim.c b/drivers/staging/media/imx/imx-media-fim.c index 3a9182933508..19a48eb445fd 100644 --- a/drivers/staging/media/imx/imx-media-fim.c +++ b/drivers/staging/media/imx/imx-media-fim.c @@ -187,54 +187,6 @@ static void frame_interval_monitor(struct imx_media_fim *fim, send_fim_event(fim, error_avg); } -#ifdef CONFIG_IMX_GPT_ICAP -/* - * Input Capture method of measuring frame intervals. Not subject - * to interrupt latency. - */ -static void fim_input_capture_handler(int channel, void *dev_id, - ktime_t timestamp) -{ - struct imx_media_fim *fim = dev_id; - unsigned long flags; - - spin_lock_irqsave(&fim->lock, flags); - - frame_interval_monitor(fim, timestamp); - - if (!completion_done(&fim->icap_first_event)) - complete(&fim->icap_first_event); - - spin_unlock_irqrestore(&fim->lock, flags); -} - -static int fim_request_input_capture(struct imx_media_fim *fim) -{ - init_completion(&fim->icap_first_event); - - return mxc_request_input_capture(fim->icap_channel, - fim_input_capture_handler, - fim->icap_flags, fim); -} - -static void fim_free_input_capture(struct imx_media_fim *fim) -{ - mxc_free_input_capture(fim->icap_channel, fim); -} - -#else /* CONFIG_IMX_GPT_ICAP */ - -static int fim_request_input_capture(struct imx_media_fim *fim) -{ - return 0; -} - -static void fim_free_input_capture(struct imx_media_fim *fim) -{ -} - -#endif /* CONFIG_IMX_GPT_ICAP */ - /* * In case we are monitoring the first frame interval after streamon * (when fim->num_skip = 0), we need a valid fim->last_ts before we @@ -435,14 +387,8 @@ int imx_media_fim_set_stream(struct imx_media_fim *fim, spin_unlock_irqrestore(&fim->lock, flags); if (icap_enabled(fim)) { - ret = fim_request_input_capture(fim); - if (ret) - goto out; fim_acquire_first_ts(fim); } - } else { - if (icap_enabled(fim)) - fim_free_input_capture(fim); } fim->stream_on = on;
There never was a config IMX_GPT_ICAP in the repository. So remove the code conditional on this config and simplify the callers that just called empty functions. Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com> --- drivers/staging/media/imx/imx-media-fim.c | 54 ----------------------- 1 file changed, 54 deletions(-)