Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751933AbdL1E6z (ORCPT ); Wed, 27 Dec 2017 23:58:55 -0500 Received: from mail-yw0-f174.google.com ([209.85.161.174]:46092 "EHLO mail-yw0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750987AbdL1E6x (ORCPT ); Wed, 27 Dec 2017 23:58:53 -0500 X-Google-Smtp-Source: ACJfBovProyAAkOnjTvzfnJrTx9z5XyBdW02GFnZ82LiuJlxoc059Xx+qcKgdk7f8EMWAIFV+8vnFQ== Date: Wed, 27 Dec 2017 23:58:50 -0500 From: William Breathitt Gray To: "Maciej S. Szmigiero" Cc: Linus Walleij , Andy Shevchenko , Jonathan Cameron , linux-kernel , linux-gpio@vger.kernel.org Subject: Re: [PATCH v2] gpio: winbond: add driver Message-ID: <20171228045850.GB11117@sophia> References: <309acd17-5244-da8c-a28e-dace15ada4fb@maciej.szmigiero.name> <20171224224215.GA2372@sophia> <0e8ece1e-3041-ea58-f5b6-298c163fa747@maciej.szmigiero.name> <20171227002410.GA12050@sophia> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3309 Lines: 68 On Wed, Dec 27, 2017 at 07:42:21PM +0100, Maciej S. Szmigiero wrote: >On 27.12.2017 01:24, William Breathitt Gray wrote: >> On Mon, Dec 25, 2017 at 03:48:16PM +0100, Maciej S. Szmigiero wrote: >(..) >>> All the existing ISA bus drivers seem to depend on CONFIG_ISA_BUS_API >>> instead of selecting it but IMHO this is wrong because: >>> 1) This Kconfig option doesn't really enable or disable any bus support >>> but building of a library of some common boilerplate code. >>> Libraries are normally selected by drivers needing them and only provided >>> as an user-selectable option if there is a possibility that a out-of-tree >>> module would need it, >>> >>> 2) On x86_64 this option (or rather, its parent option CONFIG_ISA_BUS) >>> cannot be enabled without CONFIG_EXPERT, >>> >>> 3) This device isn't really a ISA bus device any more than, for example, >>> a 8250 serial port or a PC-style parallel port and these don't need >>> that an user explicitly enables "ISA bus support" in his kernel >>> configuration. >> >> I can see what you mean about selecting ISA_BUS_API rather than having >> it as a dependency for drivers. Part of the reason I added the >> CONFIG_EXPERT dependency for CONFIG_ISA_BUS -- as well as having >> CONFIG_ISA_BUS_API be a dependency for the drivers themselves -- was to >> hide the ISA-style drivers which blindly poke at I/O port addresses, >> lest a niave user enable all available drivers and unintentionally brick >> their system when the drivers execute. >> >> I think there is still merit in masking dangerous drivers such as this, >> since the expected behavior nowadays is for the driver to probe for the >> device before poking at memory; since ISA-style communication lacks a >> standard method of detecting devices in hardware, these devices >> generally pose a danger when loaded by niave users. > >This driver accesses the same Super I/O chip as w83627ehf hwmon and >w83627hf_wdt watchdog drivers. >In addition to this, there are loads of other hwmon and watchdog drivers >for x86 Super I/Os in the tree, most of them using the same probing and >communication style. >There are even existing GPIO drivers for some Super I/Os like gpio-it87 >and gpio-f7188x. > >None of these drivers need CONFIG_EXPERT to be selected. > >Also, CONFIG_EXPERT is described as "Configure standard kernel features" >and that "[it] allows certain base kernel options and settings to be >disabled or tweaked" for "specialized environments". >Enabling this driver is not about changing "standard kernel feature" or >a "base kernel option [or] setting". I'm sorry, I didn't make it quite clear in my previous reply. I agree with you that CONFIG_EXPERT shouldn't be necessary for this driver -- in the end, a select ISA_BUS_API line should be all that's needed to have ISA bus driver support for your driver. My reference to the CONFIG_EXPERT option is for masking options related to other ISA-style buses not commonly found in desktop systems. Devices like the Super I/O chip wouldn't fall into this category since LPC is pretty common and the relevant I/O port addresses are usually well known. Ultimately, the Winbond GPIO driver should not need CONFIG_EXPERT to be enabled in order to select ISA_BUS_API. William Breathitt Gray >> >> William Breathitt Gray > >Maciej Szmigiero