[Linux-kernel-mentees] [PATCH] media: atomisp: replace boolean comparison of values with bool variables

Lukas Bulwahn lukas.bulwahn at gmail.com
Mon Dec 14 05:41:49 UTC 2020


On Sun, Dec 13, 2020 at 6:30 PM Aditya <yashsri421 at gmail.com> wrote:
>
> On 13/12/20 10:51 pm, Aditya Srivastava wrote:
> > There are certain relational expressions in atomisp where a boolean
> > variable is compared with true/false in forms such as (foo == true)
> > or (false != bar), which does not comply with the coding style rule by
> > checkpatch.pl (CHK: BOOL_COMPARISON), according to which the boolean
> > variables should be themselves used in relational expression, rather
> > than comparing with true or false.
> >
> > E.g. In drivers/staging/media/atomisp/pci/atomisp_compat_css20.c:
> >
> > if (asd->stream_prepared == false) {
> >
> > Can be replaced with:
> > if (!asd->stream_prepared) {
> >
> > Replace such relational expressions with boolean variables appropriately.
> >
> > Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>
> > ---
> >  .../staging/media/atomisp/pci/atomisp_compat_css20.c |  2 +-
> >  .../atomisp/pci/runtime/isys/src/virtual_isys.c      | 12 ++++++------
> >  drivers/staging/media/atomisp/pci/sh_css.c           | 12 ++++++------
> >  3 files changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
> > index faa0935e536a..6a80c676f668 100644
> > --- a/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
> > +++ b/drivers/staging/media/atomisp/pci/atomisp_compat_css20.c
> > @@ -1142,7 +1142,7 @@ int atomisp_css_start(struct atomisp_sub_device *asd,
> >        * Thus the stream created in set_fmt get destroyed and need to be
> >        * recreated in the next stream on.
> >        */
> > -     if (asd->stream_prepared == false) {
> > +     if (!asd->stream_prepared) {
> >               if (__create_pipes(asd)) {
> >                       dev_err(isp->dev, "create pipe error.\n");
> >                       return -EINVAL;
> > diff --git a/drivers/staging/media/atomisp/pci/runtime/isys/src/virtual_isys.c b/drivers/staging/media/atomisp/pci/runtime/isys/src/virtual_isys.c
> > index 317ea30ede7a..82f3c19dc455 100644
> > --- a/drivers/staging/media/atomisp/pci/runtime/isys/src/virtual_isys.c
> > +++ b/drivers/staging/media/atomisp/pci/runtime/isys/src/virtual_isys.c
> > @@ -179,12 +179,12 @@ ia_css_isys_error_t ia_css_isys_stream_create(
> >       isys_stream->linked_isys_stream_id = isys_stream_descr->linked_isys_stream_id;
> >       rc = create_input_system_input_port(isys_stream_descr,
> >                                           &isys_stream->input_port);
> > -     if (rc == false)
> > +     if (!rc)
> >               return false;
> >
> >       rc = create_input_system_channel(isys_stream_descr, false,
> >                                        &isys_stream->channel);
> > -     if (rc == false) {
> > +     if (!rc) {
> >               destroy_input_system_input_port(&isys_stream->input_port);
> >               return false;
> >       }
> > @@ -204,7 +204,7 @@ ia_css_isys_error_t ia_css_isys_stream_create(
> >       if (isys_stream_descr->metadata.enable) {
> >               rc = create_input_system_channel(isys_stream_descr, true,
> >                                                &isys_stream->md_channel);
> > -             if (rc == false) {
> > +             if (!rc) {
> >                       destroy_input_system_input_port(&isys_stream->input_port);
> >                       destroy_input_system_channel(&isys_stream->channel);
> >                       return false;
> > @@ -248,7 +248,7 @@ ia_css_isys_error_t ia_css_isys_stream_calculate_cfg(
> >                 isys_stream_descr,
> >                 &isys_stream_cfg->channel_cfg,
> >                 false);
> > -     if (rc == false)
> > +     if (!rc)
> >               return false;
> >
> >       /* configure metadata channel */
> > @@ -260,7 +260,7 @@ ia_css_isys_error_t ia_css_isys_stream_calculate_cfg(
> >                         isys_stream_descr,
> >                         &isys_stream_cfg->md_channel_cfg,
> >                         true);
> > -             if (rc == false)
> > +             if (!rc)
> >                       return false;
> >       }
> >
> > @@ -269,7 +269,7 @@ ia_css_isys_error_t ia_css_isys_stream_calculate_cfg(
> >                &isys_stream->input_port,
> >                isys_stream_descr,
> >                &isys_stream_cfg->input_port_cfg);
> > -     if (rc == false)
> > +     if (!rc)
> >               return false;
> >
> >       isys_stream->valid = 1;
> > diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
> > index ddee04c8248d..a71c1bbd984b 100644
> > --- a/drivers/staging/media/atomisp/pci/sh_css.c
> > +++ b/drivers/staging/media/atomisp/pci/sh_css.c
> > @@ -1063,7 +1063,7 @@ sh_css_config_input_network(struct ia_css_stream *stream) {
> >       ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> >                           "sh_css_config_input_network() enter 0x%p:\n", stream);
> >
> > -     if (stream->config.continuous == true)
> > +     if (stream->config.continuous)
> >       {
> >               if (stream->last_pipe->config.mode == IA_CSS_PIPE_MODE_CAPTURE) {
> >                       pipe = stream->last_pipe;
> > @@ -5626,7 +5626,7 @@ static int load_video_binaries(struct ia_css_pipe *pipe)
> >               } else {
> >                       /* output from main binary is not yuv line. currently this is
> >                        * possible only when bci is enabled on vfpp output */
> > -                     assert(pipe->config.enable_vfpp_bci == true);
> > +                     assert(pipe->config.enable_vfpp_bci);
> >                       ia_css_pipe_get_yuvscaler_binarydesc(pipe, &vf_pp_descr,
> >                                                            &mycs->video_binary.vf_frame_info,
> >                                                            pipe_vf_out_info, NULL, NULL);
> > @@ -8072,7 +8072,7 @@ create_host_regular_capture_pipeline(struct ia_css_pipe *pipe) {
> >               struct ia_css_frame *tmp_out_frame = NULL;
> >
> >               for (i = 0; i < num_yuv_scaler; i++) {
> > -                     if (is_output_stage[i] == true)
> > +                     if (is_output_stage[i])
> >                               tmp_out_frame = out_frame;
> >                       else
> >                               tmp_out_frame = NULL;
> > @@ -8464,7 +8464,7 @@ sh_css_pipeline_add_acc_stage(struct ia_css_pipeline *pipeline,
> >       /* In QoS case, load_extension already called, so skipping */
> >       int     err = 0;
> >
> > -     if (fw->loaded == false)
> > +     if (!fw->loaded)
> >               err = acc_load_extension(fw);
> >
> >       ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE,
> > @@ -9701,8 +9701,8 @@ ia_css_stream_destroy(struct ia_css_stream *stream) {
> >                       assert(entry);
> >                       if (entry) {
> >                               /* get the SP thread id */
> > -                             if (ia_css_pipeline_get_sp_thread_id(
> > -                                     ia_css_pipe_get_pipe_num(entry), &sp_thread_id) != true)
> > +                             if (!ia_css_pipeline_get_sp_thread_id(
> > +                                     ia_css_pipe_get_pipe_num(entry), &sp_thread_id))
> >                                       return -EINVAL;
> >                               /* get the target input terminal */
> >                               sp_pipeline_input_terminal =
> >
>
> Hi Lukas
>
> I have tested these changes by running Makefile in the root directory,
> where I did not get any errors. Can you please review it quickly
> before I send it to maintainers?
>

Well, two things are helpful even if you do not have the hardware.

First, determine the required kernel configs so that the code you
touched is actually compiled.

Second, compile ite and compare the objdump of the created object file
before and after the change.

Other than that, the change looks good. It is probably a driver that
is on its way to removal anyway. So, do not be surprised if even if
the maintainers thinks the change generally improves the code, they
simply do not want to touch code that is soon to be deleted.

Lukas


More information about the Linux-kernel-mentees mailing list