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(?). 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) -- With warm regards, Sachin -- 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
