Hi Sachin, On Tue, Jun 18, 2013 at 11:42 AM, Sachin Kamat <sachin.kamat@xxxxxxxxxx> wrote: > 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. > > For these sub-versions, the driver interface remains mostly the same and only internal firmware implementations change (atleast that's what I have seen till date). For mainline purpose, we choose one of the versions and stick to that. 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
