Re: [PATCH v2] drivers/misc: (aspeed-lpc-snoop): Add ast2400 to compat

[Patrick Venture]

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
>> modifications.
>>
>> 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);
>> -
>> +#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