Re: [PATCH v2 5/5] staging: media: atomisp: sh_css_mipi: Remove #ifdef 2041

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hans,

On Thu, May 11, 2023 at 2:34 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi Kate,
>
> On 5/8/23 08:26, Kate Hsuan wrote:
> > The actions of ISP2401 and 2400 are determined at the runtime.
> >
> > Signed-off-by: Kate Hsuan <hpa@xxxxxxxxxx>
> > ---
> >  .../staging/media/atomisp/pci/sh_css_mipi.c   | 65 ++++++-------------
> >  1 file changed, 20 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/staging/media/atomisp/pci/sh_css_mipi.c b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > index bc6e8598a776..52a1ed63e9a5 100644
> > --- a/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > +++ b/drivers/staging/media/atomisp/pci/sh_css_mipi.c
> > @@ -386,30 +381,22 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
> >               return -EINVAL;
> >       }
> >
> > -#ifdef ISP2401
> > -     err = calculate_mipi_buff_size(&pipe->stream->config,
> > -                                    &my_css.mipi_frame_size[port]);
>
> Before you changes this code always run ISP2401, now it only runs
> when (ref_count_mipi_allocation[port] != 0) is not true.
>
> So this statement should stay here in the code, just prefixed
> with a if (IS_ISP2401) condition.
>
> > -     /*
> > -      * 2401 system allows multiple streams to use same physical port. This is not
> > -      * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution.
> > -      * TODO AM: Once that is changed (removed) this code should be removed as well.
> > -      * In that case only 2400 related code should remain.
> > -      */
> > -     if (ref_count_mipi_allocation[port] != 0) {
> > -             ref_count_mipi_allocation[port]++;
> > -             ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> > -                                 "allocate_mipi_frames(%p) leave: nothing to do, already allocated for this port (port=%d).\n",
> > -                                 pipe, port);
> > -             return 0;
> > -     }
> > -#else
> >       if (ref_count_mipi_allocation[port] != 0) {
> >               ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> >                                   "allocate_mipi_frames(%p) exit: already allocated for this port (port=%d).\n",
> >                                   pipe, port);
> >               return 0;
> > +     } else {
> > +             /*
> > +              * 2401 system allows multiple streams to use same physical port. This is not
> > +              * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution.
> > +              * TODO AM: Once that is changed (removed) this code should be removed as well.
> > +              * In that case only 2400 related code should remain.
> > +              */
>
> This comment block actually belongs to the if (ref_count_mipi_allocation[port] != 0)
> check, the code executed if that check passes was actually different between
> the ISP2400 and ISP2401 (my bad, sorry). The ISP2401 case did an extra:
>
>                 ref_count_mipi_allocation[port]++;
>
> when (ref_count_mipi_allocation[port] != 0), so we need to add:
>
>                 if (IS_ISP2401)
>                         ref_count_mipi_allocation[port]++;
>
> above the return 0 above.
>
> > +             if (IS_ISP2401)
> > +                     err = calculate_mipi_buff_size(&pipe->stream->config,
> > +                                                    &my_css.mipi_frame_size[port]);
>
> I have fixed this all up while merging your series and the new
> diff for this code-block now looks like this:
>
> @@ -386,9 +381,10 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
>                 return -EINVAL;
>         }
>
> -#ifdef ISP2401
> -       err = calculate_mipi_buff_size(&pipe->stream->config,
> -                                      &my_css.mipi_frame_size[port]);
> +       if (IS_ISP2401)
> +               err = calculate_mipi_buff_size(&pipe->stream->config,
> +                                              &my_css.mipi_frame_size[port]);
> +
>         /*
>          * 2401 system allows multiple streams to use same physical port. This is not
>          * true for 2400 system. Currently 2401 uses MIPI buffers as a temporary solution.
> @@ -396,20 +392,14 @@ allocate_mipi_frames(struct ia_css_pipe *pipe,
>          * In that case only 2400 related code should remain.
>          */
>         if (ref_count_mipi_allocation[port] != 0) {
> -               ref_count_mipi_allocation[port]++;
> +               if (IS_ISP2401)
> +                       ref_count_mipi_allocation[port]++;
> +
>                 ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
>                                     "allocate_mipi_frames(%p) leave: nothing to do, already allocated for this port (port=%d).\n",
>                                     pipe, port);
>                 return 0;
>         }
> -#else
> -       if (ref_count_mipi_allocation[port] != 0) {
> -               ia_css_debug_dtrace(IA_CSS_DEBUG_TRACE_PRIVATE,
> -                                   "allocate_mipi_frames(%p) exit: already allocated for this port (port=%d).\n",
> -                                   pipe, port);
> -               return 0;
> -       }
> -#endif
>
>         ref_count_mipi_allocation[port]++;
>
>
>
> Regards,
>
> Hans
>

It's my bad when trying to simplify the if expressions.
Thank you for fixing this. :)

-- 
BR,
Kate






[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux