2023-01-19 14:27:47

by Brent Pappas

Subject: [PATCH] media: atomisp: pci: sh_css: Replace macro STATS_ENABLED() with function

Replace the macro STATS_ENABLED() with a static function to conform to
Linux coding style standards.

Signed-off-by: Brent Pappas <[email protected]>
---
drivers/staging/media/atomisp/pci/sh_css.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
index 726cb7aa4ecd..753b3e0fcb07 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -97,8 +97,12 @@
*/
#define JPEG_BYTES (16 * 1024 * 1024)

-#define STATS_ENABLED(stage) (stage && stage->binary && stage->binary->info && \
- (stage->binary->info->sp.enable.s3a || stage->binary->info->sp.enable.dis))
+static bool stats_enabled(struct ia_css_pipeline_stage *stage)
+{
+ return stage && stage->binary && stage->binary->info &&
+ (stage->binary->info->sp.enable.s3a ||
+ stage->binary->info->sp.enable.dis);
+}

struct sh_css my_css;

@@ -3743,7 +3747,7 @@ ia_css_pipe_enqueue_buffer(struct ia_css_pipe *pipe,
* The SP will read the params after it got
* empty 3a and dis
*/
- if (STATS_ENABLED(stage)) {
+ if (stats_enabled(stage)) {
/* there is a stage that needs it */
return_err = ia_css_bufq_enqueue_buffer(thread_id,
queue_id,
--
2.34.1

2023-01-19 14:40:21

by Andy Shevchenko

Subject: Re: [PATCH] media: atomisp: pci: sh_css: Replace macro STATS_ENABLED() with function

On Thu, Jan 19, 2023 at 3:43 PM Brent Pappas <[email protected]> wrote:
>
> Replace the macro STATS_ENABLED() with a static function to conform to
> Linux coding style standards.

I believe the best approach here is to drop that completely and move
its content to be inline in the only caller.

--
With Best Regards,
Andy Shevchenko

2023-01-19 15:43:17

by Brent Pappas

Subject: [PATCH v2] media: atomisp: pci: sh_css: Inline single invocation of macro STATS_ENABLED()

Inline the single invocation of the macro STATS_ENABLED().
The macro abstraction is not necessary because the logic behind it is only
used once.

Signed-off-by: Brent Pappas <[email protected]>
---
drivers/staging/media/atomisp/pci/sh_css.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
index 726cb7aa4ecd..93789500416f 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -97,9 +97,6 @@
*/
#define JPEG_BYTES (16 * 1024 * 1024)

-#define STATS_ENABLED(stage) (stage && stage->binary && stage->binary->info && \
- (stage->binary->info->sp.enable.s3a || stage->binary->info->sp.enable.dis))
-
struct sh_css my_css;

int __printf(1, 0) (*sh_css_printf)(const char *fmt, va_list args) = NULL;
@@ -3743,7 +3740,9 @@ ia_css_pipe_enqueue_buffer(struct ia_css_pipe *pipe,
* The SP will read the params after it got
* empty 3a and dis
*/
- if (STATS_ENABLED(stage)) {
+ if (stage && stage->binary && stage->binary->info &&
+ (stage->binary->info->sp.enable.s3a ||
+ stage->binary->info->sp.enable.dis)) {
/* there is a stage that needs it */
return_err = ia_css_bufq_enqueue_buffer(thread_id,
queue_id,
--
2.34.1

2023-01-19 16:08:39

by Dan Carpenter

Subject: Re: [PATCH v2] media: atomisp: pci: sh_css: Inline single invocation of macro STATS_ENABLED()

On Thu, Jan 19, 2023 at 09:42:03AM -0500, Brent Pappas wrote:
> Inline the single invocation of the macro STATS_ENABLED().
> The macro abstraction is not necessary because the logic behind it is only
> used once.
>
> Signed-off-by: Brent Pappas <[email protected]>
> ---

Needs a V2: note which says what changed.

> drivers/staging/media/atomisp/pci/sh_css.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
> index 726cb7aa4ecd..93789500416f 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css.c
> @@ -97,9 +97,6 @@
> */
> #define JPEG_BYTES (16 * 1024 * 1024)
>
> -#define STATS_ENABLED(stage) (stage && stage->binary && stage->binary->info && \
> - (stage->binary->info->sp.enable.s3a || stage->binary->info->sp.enable.dis))
> -
> struct sh_css my_css;
>
> int __printf(1, 0) (*sh_css_printf)(const char *fmt, va_list args) = NULL;
> @@ -3743,7 +3740,9 @@ ia_css_pipe_enqueue_buffer(struct ia_css_pipe *pipe,
> * The SP will read the params after it got
> * empty 3a and dis
> */
> - if (STATS_ENABLED(stage)) {
> + if (stage && stage->binary && stage->binary->info &&
^^^^^^^^
We know that "stage" must be non-NULL. You should wait over night
before resending v2 patches so that we think "This guy spend overnight
running tests. He must be very serious." There is no rush.

regards,
dan carpenter

2023-01-20 17:34:03

by Brent Pappas

Subject: [PATCH v3] media: atomisp: pci: sh_css: Inline single invocation of macro STATS_ENABLED()

Inline the single invocation of the macro STATS_ENABLED().
The macro abstraction is not necessary because the logic behind it is only
used once.

Signed-off-by: Brent Pappas <[email protected]>
---
Changelog:
V1 -> V2: Inline macro instead of replacing it with a function.

V2 -> V3: Remove unnecessary check that stage is non-null.

drivers/staging/media/atomisp/pci/sh_css.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
index 726cb7aa4ecd..93789500416f 100644
--- a/drivers/staging/media/atomisp/pci/sh_css.c
+++ b/drivers/staging/media/atomisp/pci/sh_css.c
@@ -97,9 +97,6 @@
*/
#define JPEG_BYTES (16 * 1024 * 1024)

-#define STATS_ENABLED(stage) (stage && stage->binary && stage->binary->info && \
- (stage->binary->info->sp.enable.s3a || stage->binary->info->sp.enable.dis))
-
struct sh_css my_css;

int __printf(1, 0) (*sh_css_printf)(const char *fmt, va_list args) = NULL;
@@ -3743,7 +3740,9 @@ ia_css_pipe_enqueue_buffer(struct ia_css_pipe *pipe,
* The SP will read the params after it got
* empty 3a and dis
*/
- if (STATS_ENABLED(stage)) {
+ if (stage->binary && stage->binary->info &&
+ (stage->binary->info->sp.enable.s3a ||
+ stage->binary->info->sp.enable.dis)) {
/* there is a stage that needs it */
return_err = ia_css_bufq_enqueue_buffer(thread_id,
queue_id,
--
2.34.1

2023-01-20 18:04:38

by Andy Shevchenko

Subject: Re: [PATCH v3] media: atomisp: pci: sh_css: Inline single invocation of macro STATS_ENABLED()

On Fri, Jan 20, 2023 at 7:14 PM Brent Pappas <[email protected]> wrote:
>
> Inline the single invocation of the macro STATS_ENABLED().
> The macro abstraction is not necessary because the logic behind it is only
> used once.

LGTM,
Reviewed-by: Andy Shevchenko <[email protected]>

> Signed-off-by: Brent Pappas <[email protected]>
> ---
> Changelog:
> V1 -> V2: Inline macro instead of replacing it with a function.
>
> V2 -> V3: Remove unnecessary check that stage is non-null.
>
> drivers/staging/media/atomisp/pci/sh_css.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
> index 726cb7aa4ecd..93789500416f 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css.c
> @@ -97,9 +97,6 @@
> */
> #define JPEG_BYTES (16 * 1024 * 1024)
>
> -#define STATS_ENABLED(stage) (stage && stage->binary && stage->binary->info && \
> - (stage->binary->info->sp.enable.s3a || stage->binary->info->sp.enable.dis))
> -
> struct sh_css my_css;
>
> int __printf(1, 0) (*sh_css_printf)(const char *fmt, va_list args) = NULL;
> @@ -3743,7 +3740,9 @@ ia_css_pipe_enqueue_buffer(struct ia_css_pipe *pipe,
> * The SP will read the params after it got
> * empty 3a and dis
> */
> - if (STATS_ENABLED(stage)) {
> + if (stage->binary && stage->binary->info &&
> + (stage->binary->info->sp.enable.s3a ||
> + stage->binary->info->sp.enable.dis)) {
> /* there is a stage that needs it */
> return_err = ia_css_bufq_enqueue_buffer(thread_id,
> queue_id,
> --
> 2.34.1
>


--
With Best Regards,
Andy Shevchenko

2023-01-23 12:04:53

by Dan Carpenter

Subject: Re: [PATCH v3] media: atomisp: pci: sh_css: Inline single invocation of macro STATS_ENABLED()

On Fri, Jan 20, 2023 at 12:14:08PM -0500, Brent Pappas wrote:
> Inline the single invocation of the macro STATS_ENABLED().
> The macro abstraction is not necessary because the logic behind it is only
> used once.
>
> Signed-off-by: Brent Pappas <[email protected]>
> ---
> Changelog:
> V1 -> V2: Inline macro instead of replacing it with a function.
>
> V2 -> V3: Remove unnecessary check that stage is non-null.
>

Thanks!

Reviewed-by: Dan Carpenter <[email protected]>

regards,
dan carpenter

2023-01-23 12:29:04

by Hans de Goede

Subject: Re: [PATCH v3] media: atomisp: pci: sh_css: Inline single invocation of macro STATS_ENABLED()

Hi,

On 1/20/23 18:14, Brent Pappas wrote:
> Inline the single invocation of the macro STATS_ENABLED().
> The macro abstraction is not necessary because the logic behind it is only
> used once.
>
> Signed-off-by: Brent Pappas <[email protected]>


Thank you.

I have added this to my personal git tree now and I will include
this in the atomisp driver pull-req which I will send to the
media-subsystem maintainer in a couple of weeks.

Regards,

Hans



> ---
> Changelog:
> V1 -> V2: Inline macro instead of replacing it with a function.
>
> V2 -> V3: Remove unnecessary check that stage is non-null.
>
> drivers/staging/media/atomisp/pci/sh_css.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/media/atomisp/pci/sh_css.c b/drivers/staging/media/atomisp/pci/sh_css.c
> index 726cb7aa4ecd..93789500416f 100644
> --- a/drivers/staging/media/atomisp/pci/sh_css.c
> +++ b/drivers/staging/media/atomisp/pci/sh_css.c
> @@ -97,9 +97,6 @@
> */
> #define JPEG_BYTES (16 * 1024 * 1024)
>
> -#define STATS_ENABLED(stage) (stage && stage->binary && stage->binary->info && \
> - (stage->binary->info->sp.enable.s3a || stage->binary->info->sp.enable.dis))
> -
> struct sh_css my_css;
>
> int __printf(1, 0) (*sh_css_printf)(const char *fmt, va_list args) = NULL;
> @@ -3743,7 +3740,9 @@ ia_css_pipe_enqueue_buffer(struct ia_css_pipe *pipe,
> * The SP will read the params after it got
> * empty 3a and dis
> */
> - if (STATS_ENABLED(stage)) {
> + if (stage->binary && stage->binary->info &&
> + (stage->binary->info->sp.enable.s3a ||
> + stage->binary->info->sp.enable.dis)) {
> /* there is a stage that needs it */
> return_err = ia_css_bufq_enqueue_buffer(thread_id,
> queue_id,