From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752928AbdEQFup (ORCPT ); Wed, 17 May 2017 01:50:45 -0400 Received: from mail-lf0-f46.google.com ([209.85.215.46]:34571 "EHLO mail-lf0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752073AbdEQFun (ORCPT ); Wed, 17 May 2017 01:50:43 -0400 Subject: Re: [V2, 2/6] tty: serial: lpuart: add little endian 32 bit register support To: Dong Aisheng References: <1494834539-17523-3-git-send-email-aisheng.dong@nxp.com> <20170517033927.GC9913@b29396-OptiPlex-7040> <8f4cf45e-45ab-e6e6-d95c-f3d4243697f4@cogentembedded.com> <20170517054356.GE9913@b29396-OptiPlex-7040> Cc: Dong Aisheng , linux-serial@vger.kernel.org, fugang.duan@nxp.com, gregkh@linuxfoundation.org, yangbo.lu@nxp.com, linux-kernel@vger.kernel.org, stefan@agner.ch, Mingkai.Hu@nxp.com, jslaby@suse.com, linux-arm-kernel@lists.infradead.org From: Nikita Yushchenko X-Enigmail-Draft-Status: N1110 Message-ID: Date: Wed, 17 May 2017 08:50:39 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170517054356.GE9913@b29396-OptiPlex-7040> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >>>>> static u32 lpuart32_read(void __iomem *addr) >>>>> { >>>>> - return ioread32be(addr); >>>>> + return lpuart_is_be ? ioread32be(addr) : readl(addr); >>>>> } >>>>> >>>>> static void lpuart32_write(u32 val, void __iomem *addr) >>>>> { >>>>> - iowrite32be(val, addr); >>>>> + if (lpuart_is_be) >>>>> + iowrite32be(val, addr); >>>>> + else >>>>> + writel(val, addr); >>>>> } >>>> >>>> What if this is ever executed on big endian system? >>>> >>> >>> Sorry, not catching the point... >>> >>> What issues will meet? >> >> Isn't writel() in host endian? > > On big endian systems, it is supposed to run iowrite32be. Your code states, "force BE if lpuart_is_be, don't care otherwise". This semantics looks questionable for code reviewer. If driver handles endian, should't it be explicit in both cases? And if indeed driver means handling BE explicitly, but don't caring otherwise, maybe variable name should suggest that (i.e. "force_be")? Although driver maintainer could think differently. I won't insist on this. Nikita