Re: [PATCH v2] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat
From: Patrick Venture
Date: Wed Jul 05 2017 - 15:47:48 EST
On Wed, Jul 5, 2017 at 12:43 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Wed, Jul 5, 2017 at 9:04 PM, Patrick Venture <venture@xxxxxxxxxx> wrote:
>> This driver can be used on the aspeed ast2400 with minor
>> Tested: ast2400 on quanta-q71l
>> Signed-off-by: Patrick Venture <venture@xxxxxxxxxx>
>> 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);
>> 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.
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
.data = G4,
.data = G5,
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?