From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-1.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 25D7FC43387 for ; Tue, 1 Jan 2019 01:10:37 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E437520828 for ; Tue, 1 Jan 2019 01:10:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728187AbfAABKg (ORCPT ); Mon, 31 Dec 2018 20:10:36 -0500 Received: from kvm5.telegraphics.com.au ([98.124.60.144]:58604 "EHLO kvm5.telegraphics.com.au" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727988AbfAABKf (ORCPT ); Mon, 31 Dec 2018 20:10:35 -0500 Received: from localhost (localhost.localdomain [127.0.0.1]) by kvm5.telegraphics.com.au (Postfix) with ESMTP id 2962D29C55; Mon, 31 Dec 2018 20:10:33 -0500 (EST) Date: Tue, 1 Jan 2019 12:10:11 +1100 (AEDT) From: Finn Thain To: Arnd Bergmann cc: LEROY Christophe , Linux Fbdev development list , dri-devel , Linux Kernel Mailing List , linuxppc-dev , linux-m68k , Bartlomiej Zolnierkiewicz , Michael Ellerman , Paul Mackerras , Benjamin Herrenschmidt , Greg Kroah-Hartman Subject: Re: [PATCH v8 20/25] powerpc, fbdev: Use arch_nvram_ops methods instead of nvram_read_byte() and nvram_write_byte() In-Reply-To: Message-ID: References: <7e8eb87ea829c03941c895c968df2ebd0f80512f.1545784679.git.fthain@telegraphics.com.au> <20181229180236.Horde.idY3gOIzkSWywjIrqlXJMA1@messagerie.si.c-s.fr> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 31 Dec 2018, Arnd Bergmann wrote: > On Sun, Dec 30, 2018 at 12:43 AM Finn Thain wrote: > > > > > Is there some benefit, or is that just personal taste? > > > > Avoiding changes to call sites avoids code review, but I think 1) the > > thinkpad_acpi changes have already been reviewed and 2) the fbdev changes > > need review anyway. > > > > Your suggesion would add several new entities and one extra layer of > > indirection. > > > > I think indirection harms readability because now the reader now has to go > > and look up the meaning of the new entities. > > > > It's not the case that we need to choose between definitions of > > nvram_read_byte() at compile time, or stub them out: > > > > #ifdef CONFIG_FOO > > static inline unsigned char nvram_read_byte(int addr) > > { > > return arch_nvram_ops.read_byte(addr); > > } > > #else > > static inline unsigned char nvram_read_byte(int addr) { } > > #endif > > > > And I don't anticipate a need for a macro here either: > > > > #define nvram_read_byte(a) random_nvram_read_byte_impl(a) > > > > I think I've used the simplest solution. > > Having the indirection would help if the inline function can > encapsulate the NULL pointer check, like > > static inline unsigned char nvram_read_byte(loff_t addr) > { > char data; > > if (!IS_ENABLED(CONFIG_NVRAM)) > return 0xff; > > if (arch_nvram_ops.read_byte) > return arch_nvram_ops.read_byte(addr); > > if (arch_nvram_ops.read) > return arch_nvram_ops.read(char, 1, &addr); > > return 0xff; > } > The semantics of .read_byte and .read are subtly different. For CONFIG_X86 and CONFIG_ATARI, .read implies checksum validation and .read_byte does not. In particular, in the thinkpad_acpi case, checksum validation isn't used, but in the atari_scsi case, it is. So I like to see drivers explicitly call the method they want. I didn't want to obscure this distinction in a helper. --