Message-ID: <5860bf02-6b55-a885-742b-cd1477c7b1aa@redhat.com>
Date:   Mon, 23 Jan 2023 13:28:02 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: [PATCH] media: atomisp: pci: hive_isp_css_common: host: vmem:
 Replace SUBWORD macros with functions
Content-Language: en-US
To:     Brent Pappas <bpappas@pappasbrent.com>
Cc:     mchehab@kernel.org, sakari.ailus@linux.intel.com,
        gregkh@linuxfoundation.org, linux-media@vger.kernel.org,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org
References: <20230120182625.23227-1-bpappas@pappasbrent.com>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230120182625.23227-1-bpappas@pappasbrent.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 1/20/23 19:26, Brent Pappas wrote:
> Replace the macros SUBWORD() and INV_SUBWORD() with functions to comply
> with Linux coding style standards.
> 
> Signed-off-by: Brent Pappas <bpappas@pappasbrent.com>


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


> ---
> I am not sure if it would better to inline SUBWORD() or turn it into
> a function.
> On the one hand, SUBWORD() is only invoked once, so it may be better to
> to inline it.
> On the other hand, the macro defined beside it, INV_SUBWORD() should be
> turned into to a function because it is invoked multiple times, so it
> may make sense to turn SUBWORD() into a function as well.
> I have opted to turn SUBWORD() into a function.
> 
>  .../pci/hive_isp_css_common/host/vmem.c       | 21 +++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c b/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c
> index 6620f091442f..316abfb72a83 100644
> --- a/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c
> +++ b/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/vmem.c
> @@ -28,10 +28,19 @@ typedef hive_uedge *hive_wide;
>  /* Copied from SDK: sim_semantics.c */
>  
>  /* subword bits move like this:         MSB[____xxxx____]LSB -> MSB[00000000xxxx]LSB */
> -#define SUBWORD(w, start, end)     (((w) & (((1ULL << ((end) - 1)) - 1) << 1 | 1)) >> (start))
> +static inline hive_uedge
> +subword(hive_uedge w, unsigned int start, unsigned int end)
> +{
> +	return (w & (((1ULL << (end - 1)) - 1) << 1 | 1)) >> start;
> +}
>  
>  /* inverse subword bits move like this: MSB[xxxx____xxxx]LSB -> MSB[xxxx0000xxxx]LSB */
> -#define INV_SUBWORD(w, start, end) ((w) & (~(((1ULL << ((end) - 1)) - 1) << 1 | 1) | ((1ULL << (start)) - 1)))
> +static inline hive_uedge
> +inv_subword(hive_uedge w, unsigned int start, unsigned int end)
> +{
> +	return w & (~(((1ULL << (end - 1)) - 1) << 1 | 1) |
> +		    ((1ULL << start) - 1));
> +}
>  
>  #define uedge_bits (8 * sizeof(hive_uedge))
>  #define move_lower_bits(target, target_bit, src, src_bit) move_subword(target, target_bit, src, 0, src_bit)
> @@ -50,18 +59,18 @@ move_subword(
>  	unsigned int start_bit  = target_bit % uedge_bits;
>  	unsigned int subword_width = src_end - src_start;
>  
> -	hive_uedge src_subword = SUBWORD(src, src_start, src_end);
> +	hive_uedge src_subword = subword(src, src_start, src_end);
>  
>  	if (subword_width + start_bit > uedge_bits) { /* overlap */
>  		hive_uedge old_val1;
> -		hive_uedge old_val0 = INV_SUBWORD(target[start_elem], start_bit, uedge_bits);
> +		hive_uedge old_val0 = inv_subword(target[start_elem], start_bit, uedge_bits);
>  
>  		target[start_elem] = old_val0 | (src_subword << start_bit);
> -		old_val1 = INV_SUBWORD(target[start_elem + 1], 0,
> +		old_val1 = inv_subword(target[start_elem + 1], 0,
>  				       subword_width + start_bit - uedge_bits);
>  		target[start_elem + 1] = old_val1 | (src_subword >> (uedge_bits - start_bit));
>  	} else {
> -		hive_uedge old_val = INV_SUBWORD(target[start_elem], start_bit,
> +		hive_uedge old_val = inv_subword(target[start_elem], start_bit,
>  						 start_bit + subword_width);
>  
>  		target[start_elem] = old_val | (src_subword << start_bit);