Hi Arun, On 18 June 2013 11:25, Arun Kumar K <arunkk.samsung@xxxxxxxxx> wrote: > 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. OK. Do they co-exist or is there a possibility for that (to have v6.5 and say v6.7 or v7.2 and v7.4, etc). Just asking. -- 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
