Hi Arun, I have read your patches. They seem alright, I back comments made by Hans and Sylwester. I have one question, which follows inline. Best wishes, -- Kamil Debski Linux Kernel Developer Samsung R&D Institute Poland > -----Original Message----- > From: Arun Kumar K [mailto:arun.kk@xxxxxxxxxxx] > Sent: Monday, June 10, 2013 3:23 PM > To: linux-media@xxxxxxxxxxxxxxx > Cc: k.debski@xxxxxxxxxxx; jtp.park@xxxxxxxxxxx; s.nawrocki@xxxxxxxxxxx; > avnd.kiran@xxxxxxxxxxx; arunkk.samsung@xxxxxxxxx > Subject: [PATCH 3/6] [media] s5p-mfc: Core support for MFC v7 > > Adds variant data and core support for the MFC v7 firmware > > Signed-off-by: Arun Kumar K <arun.kk@xxxxxxxxxxx> > --- > drivers/media/platform/s5p-mfc/s5p_mfc.c | 32 > +++++++++++++++++++++++ > drivers/media/platform/s5p-mfc/s5p_mfc_common.h | 2 ++ > 2 files changed, 34 insertions(+) > > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c > b/drivers/media/platform/s5p-mfc/s5p_mfc.c > index d12faa6..d6be52f 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c > @@ -1391,6 +1391,32 @@ static struct s5p_mfc_variant mfc_drvdata_v6 = { > .fw_name = "s5p-mfc-v6.fw", > }; > > +struct s5p_mfc_buf_size_v6 mfc_buf_size_v7 = { > + .dev_ctx = MFC_CTX_BUF_SIZE_V7, > + .h264_dec_ctx = MFC_H264_DEC_CTX_BUF_SIZE_V7, > + .other_dec_ctx = MFC_OTHER_DEC_CTX_BUF_SIZE_V7, > + .h264_enc_ctx = MFC_H264_ENC_CTX_BUF_SIZE_V7, > + .other_enc_ctx = MFC_OTHER_ENC_CTX_BUF_SIZE_V7, > +}; > + > +struct s5p_mfc_buf_size buf_size_v7 = { > + .fw = MAX_FW_SIZE_V7, > + .cpb = MAX_CPB_SIZE_V7, > + .priv = &mfc_buf_size_v7, > +}; > + > +struct s5p_mfc_buf_align mfc_buf_align_v7 = { > + .base = 0, > +}; > + > +static struct s5p_mfc_variant mfc_drvdata_v7 = { > + .version = MFC_VERSION_V7, > + .port_num = MFC_NUM_PORTS_V7, > + .buf_size = &buf_size_v7, > + .buf_align = &mfc_buf_align_v7, > + .fw_name = "s5p-mfc-v7.fw", > +}; > + > static struct platform_device_id mfc_driver_ids[] = { > { > .name = "s5p-mfc", > @@ -1401,6 +1427,9 @@ static struct platform_device_id mfc_driver_ids[] > = { > }, { > .name = "s5p-mfc-v6", > .driver_data = (unsigned long)&mfc_drvdata_v6, > + }, { > + .name = "s5p-mfc-v7", > + .driver_data = (unsigned long)&mfc_drvdata_v7, > }, > {}, > }; > @@ -1413,6 +1442,9 @@ static const struct of_device_id > exynos_mfc_match[] = { > }, { > .compatible = "samsung,mfc-v6", > .data = &mfc_drvdata_v6, > + }, { > + .compatible = "samsung,mfc-v7", > + .data = &mfc_drvdata_v7, > }, > {}, > }; > diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > index ef4074c..7281de2 100644 > --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h > @@ -24,6 +24,7 @@ > #include <media/videobuf2-core.h> > #include "regs-mfc.h" > #include "regs-mfc-v6.h" > +#include "regs-mfc-v7.h" > > /* Definitions related to MFC memory */ > > @@ -684,5 +685,6 @@ void set_work_bit_irqsave(struct s5p_mfc_ctx *ctx); > (dev->variant->port_num ? 1 : 0) : 0) : 0) > #define IS_TWOPORT(dev) (dev->variant->port_num == 2 ? 1 : 0) > #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? 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) Other possibility I see is to change the name of the check. Although IS_MFCV6_OR_NEWER(dev) seems too long :) > > #endif /* S5P_MFC_COMMON_H_ */ > -- > 1.7.9.5 -- 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
