Hi,
> From: Kukjin Kim [mailto:[email protected]]
> Kamil Debski wrote:
> >
> Would be better if you could keep the subject style...
>
Ok, I'll make the comment more consistent.
(snip)
> > diff --git a/arch/arm/mach-s5pv210/include/mach/map.h
> b/arch/arm/mach-
> > s5pv210/include/mach/map.h
> > index 0982443..8f887c4 100644
> > --- a/arch/arm/mach-s5pv210/include/mach/map.h
> > +++ b/arch/arm/mach-s5pv210/include/mach/map.h
> > @@ -107,6 +107,9 @@
> > #define S5PV210_PA_DMC0 (0xF0000000)
> > #define S5PV210_PA_DMC1 (0xF1400000)
> >
> > +/* MFC */
>
> No need useless comment.
Just above the place, where I have added MFC, there are defines
for AC97, PCM, I2S etc. and all of them have such comment before
( /* AC97 */ for AC97 codec). As such I wanted to keep the style.
I agree that it is not necessary, but removing it could make the
file less consistent. Are you sure you want me to remove it?
>
> > +#define S5PV210_PA_MFC (0xF1700000)
> > +
> > /* compatibiltiy defines. */
> > #define S3C_PA_UART S5PV210_PA_UART
> > #define S3C_PA_HSMMC0 S5PV210_PA_HSMMC(0)
> > @@ -123,6 +126,7 @@
> > #define S5P_PA_FIMC0 S5PV210_PA_FIMC0
> > #define S5P_PA_FIMC1 S5PV210_PA_FIMC1
> > #define S5P_PA_FIMC2 S5PV210_PA_FIMC2
> > +#define S5P_PA_MFC S5PV210_PA_MFC
> >
> > #define SAMSUNG_PA_ADC S5PV210_PA_ADC
> > #define SAMSUNG_PA_CFCON S5PV210_PA_CFCON
> > diff --git a/arch/arm/plat-s5p/Kconfig b/arch/arm/plat-s5p/Kconfig
> > index 9755df9..c7a048e 100644
> > --- a/arch/arm/plat-s5p/Kconfig
> > +++ b/arch/arm/plat-s5p/Kconfig
> > @@ -5,6 +5,11 @@
> > #
> > # Licensed under GPLv2
> >
> > +config S5P_DEV_MFC
> > + bool
> > + help
> > + Compile in platform device definitions for MFC
> > +
> > config PLAT_S5P
> > bool
> > depends on (ARCH_S5P64X0 || ARCH_S5P6442 || ARCH_S5PC100 ||
> > ARCH_S5PV210 || ARCH_S5PV310)
> > diff --git a/arch/arm/plat-s5p/Makefile b/arch/arm/plat-s5p/Makefile
> > index df65cb7..ed0f33d 100644
> > --- a/arch/arm/plat-s5p/Makefile
> > +++ b/arch/arm/plat-s5p/Makefile
> > @@ -23,6 +23,7 @@ obj-$(CONFIG_PM) += pm.o
> > obj-$(CONFIG_PM) += irq-pm.o
> >
> > # devices
> > +obj-$(CONFIG_S5P_DEV_MFC) += dev-mfc5.o
>
> Do we really need to distinguish mfc version in file name?
> So why did you use just CONFIG_S5P_DEV_MFC as config name?
>
> I mean don't use mixed name in there.
Right, I think removing the version number is ok. Another way would be
adding
"v5" everywhere. But I don't think it is necessary.
> +obj-$(CONFIG_S5P_DEV_MFC5) += dev-mfc5.o
>
> Or
>
> +obj-$(CONFIG_S5P_DEV_MFC) += dev-mfc.o
>
> I prefer the latter...because no need to separate the hardware version
> in
> platform data now.
Ok.
> >
> > obj-$(CONFIG_S5P_DEV_FIMC0) += dev-fimc0.o
> > obj-$(CONFIG_S5P_DEV_FIMC1) += dev-fimc1.o
> > diff --git a/arch/arm/plat-s5p/dev-mfc5.c b/arch/arm/plat-s5p/dev-
> mfc5.c
> > new file mode 100644
> > index 0000000..c06ea97
> > --- /dev/null
> > +++ b/arch/arm/plat-s5p/dev-mfc5.c
> > @@ -0,0 +1,37 @@
> > +/* Base S3C64XX mfc resource and device definitions */
> > +
> > +
>
> Where is the GPL license?
> This is very important...
The file lacks a proper comment altogether. Thanks for pointing
this out, indeed it is important.
> > +#include <linux/kernel.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/ioport.h>
> > +
> > +#include <mach/map.h>
> > +#include <plat/map-base.h>
>
> Do we _really_ need <plat/map-base.h> inclusion?
> NO.
Right.
>
> > +#include <plat/devs.h>
> > +#include <plat/irqs.h>
> > +
> > +/* MFC controller */
>
> No need above comment.
Ok.
> > +static struct resource s5p_mfc_resource[] = {
> > + [0] = {
> > + .start = S5P_PA_MFC,
> > + .end = S5P_PA_MFC + SZ_64K - 1,
> > + .flags = IORESOURCE_MEM,
> > + },
> > + [1] = {
> > + .start = IRQ_MFC,
> > + .end = IRQ_MFC,
> > + .flags = IORESOURCE_IRQ,
> > + }
> > +};
> > +
> > +struct platform_device s5p_device_mfc5 = {
> > + .name = "s5p-mfc5",
> > + .id = -1,
> > + .num_resources = ARRAY_SIZE(s5p_mfc_resource),
> > + .resource = s5p_mfc_resource,
> > +};
> > +
> > +EXPORT_SYMBOL(s5p_device_mfc5);
> > +
> > +
>
> No need useless 2 empty lines here.
Ok.
(snip)
Thank you for your comments.
Best regards,
--
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
