Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752468AbdGETrp (ORCPT ); Wed, 5 Jul 2017 15:47:45 -0400 Received: from mail-pf0-f169.google.com ([209.85.192.169]:33383 "EHLO mail-pf0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751964AbdGETrn (ORCPT ); Wed, 5 Jul 2017 15:47:43 -0400 MIME-Version: 1.0 In-Reply-To: References: <20170705190416.51691-1-venture@google.com> From: Patrick Venture Date: Wed, 5 Jul 2017 12:47:22 -0700 Message-ID: Subject: Re: [PATCH v2] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat To: Arnd Bergmann Cc: Joel Stanley , Robert Lippert , gregkh , Rick Altherr , Linux Kernel Mailing List Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2375 Lines: 66 On Wed, Jul 5, 2017 at 12:43 PM, Arnd Bergmann wrote: > On Wed, Jul 5, 2017 at 9:04 PM, Patrick Venture wrote: >> This driver can be used on the aspeed ast2400 with minor >> modifications. >> >> Tested: ast2400 on quanta-q71l >> >> Signed-off-by: Patrick Venture >> --- >> v2: added aspeed-g5 area because ast2400 doesn't use those bits. >> also updated commit message. >> --- >> drivers/misc/aspeed-lpc-snoop.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/misc/aspeed-lpc-snoop.c b/drivers/misc/aspeed-lpc-snoop.c >> index 593905565b74..83f9a9e5a7cf 100644 >> --- a/drivers/misc/aspeed-lpc-snoop.c >> +++ b/drivers/misc/aspeed-lpc-snoop.c >> @@ -155,8 +155,9 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop, >> regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en); >> regmap_update_bits(lpc_snoop->regmap, SNPWADR, snpwadr_mask, >> lpc_port << snpwadr_shift); >> +#ifdef CONFIG_MACH_ASPEED_G5 >> regmap_update_bits(lpc_snoop->regmap, HICRB, hicrb_en, hicrb_en); >> - >> +#endif >> return rc; >> } > > Hi Patrick, > > Sorry for bringing up yet another point on a fairly trivial patch, but > in general, > I'd recommend making this a runtime check rather than compile-time. > > At the moment, your version is safe because CONFIG_MACH_ASPEED_G5 > and CONFIG_MACH_ASPEED_G4 are mutually exclusive and there is > always one of them set, but once we get support for G6, G7 etc, > the driver might silently break when it behaves differently depending > on a configuration option that may or may not be set on a particular > kernel build. > > You can use the .data field in the of_device_id to add a trigger for the > behavior change. > > Arnd No problem whatsoever. You're quite right. I forgot about future aspeed platforms. I'm going to dig around a few drivers and see what the right way is to handle this at run-time. Presumably something like: #ifdef CONFIG_MACH_ASPEED_G4 .data = G4, #else .data = G5, #endif Since we're holding that maybe the future should default to this behaviour. Is it linux standard to do something like #(error) in the #else so that it fails if they didn't update this driver instead of defaulting to the G5 setting? Thanks, Patrick