On 04/22/2013 08:50 PM, Stephen Rothwell wrote:
> Hi Nathan,
>
> On Mon, 22 Apr 2013 13:38:47 -0500 Nathan Fontenot <nf...@linux.vnet.ibm.com>
> wrote:
>>
>> -/* Option vector 5: PAPR/OF options supported */
>> -#define OV5_LPAR 0x80 /* logical partitioning supported */
>> -#define OV5_SPLPAR 0x40 /* shared-processor LPAR supported */
>> +/* Option vector 5: PAPR/OF options supported
>> + * Thses bits are also used for the platform_has_feature() call so
> ^^^^^
> typo
will fix.
>
>> + * we encode the vector index in the define and use the OV5_FEAT()
>> + * and OV5_INDX() macros to extract the desired information.
>> + */
>> +#define OV5_FEAT(x) ((x) & 0xff)
>> +#define OV5_INDX(x) ((x) >> 8)
>> +#define OV5_LPAR 0x0280 /* logical partitioning supported */
>> +#define OV5_SPLPAR 0x0240 /* shared-processor LPAR supported */
>
> Wouldn't it be clearer to say
>
> #define OV5_LPAR (OV5_INDX(0x2) | OV5_FEAT(0x80))
The defines won't work the way you used them, they were designed to take the
combined value, i.e. 0x0280, and parse out the index and the feature.
I do think having macros to create the actual values as your example does is
easier
to read. We could do something like...
#define OV5_FEAT(x) ((x) & 0xff)
#define OV5_SETINDX(x) ((x) << 8)
#define OV5_GETINDX(x) ((x) >> 8)
#define OV5_LPAR (OV5_SETINDX(0x2) | OV5_FEAT(0x80))
Thoughts?
>
> etc?
>
>> @@ -145,6 +141,7 @@
>> * followed by # option vectors - 1, followed by the option vectors.
>> */
>> extern unsigned char ibm_architecture_vec[];
>> +bool platform_has_feature(unsigned int);
>
> "extern", please (if nothing else, for consistency).
>
That shouldn't really be there, its an artifact from a previous patch. I'll
remove it.
>> +static __initdata struct vec5_fw_feature
>> +vec5_fw_features_table[FIRMWARE_MAX_FEATURES] = {
>
> Why make this array FIRMWARE_MAX_FEATURES (63) long? You could just
> restrict the for loop below to ARRAY_SIZE(vec5_fw_features_table).
>
>> + {FW_FEATURE_TYPE1_AFFINITY, OV5_TYPE1_AFFINITY},
>> +};
>> +
>> +void __init fw_vec5_feature_init(const char *vec5, unsigned long len)
>> +{
>> + unsigned int index, feat;
>> + int i;
>> +
>> + pr_debug(" -> fw_vec5_feature_init()\n");
>> +
>> + for (i = 0; i < FIRMWARE_MAX_FEATURES; i++) {
>> + if (!vec5_fw_features_table[i].feature)
>> + continue;
>
> And this test could go away.
>
> I realise that you have just copied the existing code, but you should not
> do that blindly. Maybe you could even add an (earlier) patch that fixes
> the existing code.
I think that could be done easily enough.
Thanks for looking,
-Nathan
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
