On Wed, Jul 5, 2017 at 12:43 PM, Arnd Bergmann <[email protected]> wrote: > On Wed, Jul 5, 2017 at 9:04 PM, Patrick Venture <[email protected]> wrote: >> This driver can be used on the aspeed ast2400 with minor >> modifications. >> >> Tested: ast2400 on quanta-q71l >> >> Signed-off-by: Patrick Venture <[email protected]> >> --- >> 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
