Hi Sachin, On Tue, Jun 18, 2013 at 10:56 AM, Sachin Kamat <sachin.kamat@xxxxxxxxxx> wrote: > On 18 June 2013 10:21, Arun Kumar K <arunkk.samsung@xxxxxxxxx> wrote: >> Hi Kamil, >> >> Thank you for the review. >> >> >>>> #define IS_MFCV6(dev) (dev->variant->version >= 0x60 ? 1 : >>> 0) >>>> +#define IS_MFCV7(dev) (dev->variant->version >= 0x70 ? 1 : >>> 0) >>> >>> According to this, MFC v7 is also detected as MFC v6. Was this intended? >> >> Yes this was intentional as most of v7 will be reusing the v6 code and >> only minor >> changes are there w.r.t firmware interface. >> >> >>> I think that it would be much better to use this in code: >>> if (IS_MFCV6(dev) || IS_MFCV7(dev)) >>> And change the define to detect only single MFC revision: >>> #define IS_MFCV6(dev) (dev->variant->version >= 0x60 && >>> dev->variant->version < 0x70) >>> >> >> I kept it like that since the macro IS_MFCV6() is used quite frequently >> in the driver. Also if MFCv8 comes which is again similar to v6 (not >> sure about this), >> then it will add another OR condition to this check. >> >>> Other possibility I see is to change the name of the check. Although >>> IS_MFCV6_OR_NEWER(dev) seems too long :) >>> >> >> How about making it IS_MFCV6_PLUS()? > > Technically > #define IS_MFCV6(dev) (dev->variant->version >= 0x60...) > means all lower versions are also higher versions. > This may not cause much of a problem (other than the macro being a > misnomer) as all current higher versions are supersets of lower > versions. > But this is not guaranteed(?). > Till now we havent encountered otherwise and we can only hope that it remains like this :) > Hence changing the definition of the macro to (dev->variant->version >>= 0x60 && dev->variant->version < 0x70) as Kamil suggested or > renaming it to > IS_MFCV6_PLUS() makes sense. > > OTOH, do we really have intermediate version numbers? For e.g. 0x61, 0x72, etc? > > If not we can make it just: > #define IS_MFCV6(dev) (dev->variant->version == 0x60 ? 1 : 0) > The v6 version we use is actually v6.5 and v7 is v7.2. In mainline we havent used any FW sub-versions yet. Regards Arun -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
