Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-372-g43825cb665-fm-20230411.003-g43825cb6
Mime-Version: 1.0
Message-Id: <e4d7ceaa-ec0a-4ab0-9ad5-f0ecf9d263a9@app.fastmail.com>
In-Reply-To: <20230418032031.GA4703@pendragon.ideasonboard.com>
References: <20230417223738.1811110-1-arnd@kernel.org>
 <20230418032031.GA4703@pendragon.ideasonboard.com>
Date:   Tue, 18 Apr 2023 08:13:05 +0200
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "laurent.pinchart" <laurent.pinchart@ideasonboard.com>,
        "Arnd Bergmann" <arnd@kernel.org>
Cc:     "Mauro Carvalho Chehab" <mchehab@kernel.org>,
        "Shawn Guo" <shawnguo@kernel.org>,
        "Sascha Hauer" <s.hauer@pengutronix.de>,
        "Christian Hemp" <c.hemp@phytec.de>,
        "Dong Aisheng" <aisheng.dong@nxp.com>,
        "Stefan Riedmueller" <s.riedmueller@phytec.de>,
        "Pengutronix Kernel Team" <kernel@pengutronix.de>,
        "Fabio Estevam" <festevam@gmail.com>,
        "NXP Linux Team" <linux-imx@nxp.com>,
        "Jacopo Mondi" <jacopo.mondi@ideasonboard.com>,
        linux-media@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] media: nxp: imx8-isi: fix buiding on 32-bit
Content-Type: text/plain
Precedence: bulk

On Tue, Apr 18, 2023, at 05:20, Laurent Pinchart wrote:
> On Tue, Apr 18, 2023 at 12:37:27AM +0200, Arnd Bergmann wrote:
>> drivers/media/platform/nxp/imx8-isi/imx8-isi-hw.c:55:24: error: right shift count >= width of type [-Werror=shift-count-overflow]
>> 
>> But this is best avoided by using the lower_32_bits()/upper_32_bits()
>> helpers.
>
> I've submitted a patch yesterday that uses #ifdef, but I like this one
> better. One could argue that it leads to dead code on 32-bit platforms,
> but the ISI is only present in 64-bit SoCs, so that's not an issue.

I think there are actually some people using 32-bit kernels on imx8,
but they tend to do it for the wrong reasons, and I'm not worried
about wasting a few bytes in those setups.

My first draft kept an IS_ENABLED(CONFIG_ARCH_DMA_ADDR_T_64BIT) inside
each of the three if(), but I dropped that for readability.

>>  	if (buf_id == MXC_ISI_BUF1) {
>> -		mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_Y, dma_addrs[0]);
>> -		mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_U, dma_addrs[1]);
>> -		mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_V, dma_addrs[2]);
>> -#if CONFIG_ARCH_DMA_ADDR_T_64BIT
>> +		mxc_isi_write(pipe, CHNL_OUT_BUF1_ADDR_Y,
>> +			lower_32_bits(dma_addrs[0]));
>
> Could you please align this with the ( ? I can also do so in my tree,
> but I expect Mauro to pick this patch directly from the list, so a v2
> would make it easier. You can then add my
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Ok, I sent that out now, but now I see I forgot to add your
Reviewed-by.

     Arnd