From mboxrd@z Thu Jan  1 00:00:00 1970
From: Artem Bityutskiy <dedekind1@gmail.com>
Subject: Re: [PATCH 2/2] mtd: msm_nand: Add initial msm nand driver support.
Date: Sat, 16 Apr 2011 11:20:29 +0300
Message-ID: <1302942029.2308.1.camel@koala>
References: <1298940450-27365-1-git-send-email-mnalajal@codeaurora.org>
	 <1298940450-27365-2-git-send-email-mnalajal@codeaurora.org>
	 <1302861097.3220.12.camel@localhost>  <4DA85B24.50807@codeaurora.org>
Reply-To: dedekind1@gmail.com
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: QUOTED-PRINTABLE
Return-path: <linux-arm-msm-owner@vger.kernel.org>
Received: from mail-ey0-f174.google.com ([209.85.215.174]:65017 "EHLO
	mail-ey0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751787Ab1DPIUk (ORCPT
	<rfc822;linux-arm-msm@vger.kernel.org>);
	Sat, 16 Apr 2011 04:20:40 -0400
In-Reply-To: <4DA85B24.50807@codeaurora.org>
Sender: linux-arm-msm-owner@vger.kernel.org
List-Id: linux-arm-msm@vger.kernel.org
To: Murali Nalajala <mnalajal@codeaurora.org>
Cc: dwmw2@infradead.org, linux-mtd@lists.infradead.org, linux-arm-msm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Dima Zavin <dima@android.com>, Brian Swetland <swetland@google.com>, Arve =?ISO-8859-1?Q?Hj=F8nnev=E5g?= <arve@android.com>

On Fri, 2011-04-15 at 20:20 +0530, Murali Nalajala wrote:
> On 4/15/2011 3:21 PM, Artem Bityutskiy wrote:
> > On Tue, 2011-03-01 at 06:17 +0530, Murali Nalajala wrote:
> >> +#define pr_fmt(fmt) "%s:" fmt, __func__
> >> +
> >> +#include<linux/kernel.h>
> >> +#include<linux/mtd/mtd.h>
> >> +#include<linux/mtd/nand.h>
> >> +#include<linux/mtd/partitions.h>
> >> +#include<linux/platform_device.h>
> >> +#include<linux/sched.h>
> >> +#include<linux/slab.h>
> >> +#include<linux/dma-mapping.h>
> >> +#include<asm/mach/flash.h>
> >> +#include<mach/dma.h>
> >> +
> >> +#include "msm_nand.h"
> >> +
> >> +unsigned long msm_nand_phys;
> >
> > No global variables like this please. Here is how you use them:
> >
> > +extern unsigned long msm_nand_phys;
>=20
> extern declaration in the 'C' file causing warning. We are in plan to=
=20
> upload the OneNAND changes soon which makes use of the same .h file.

So you say that OneNAND (an independent driver) is going to use this
variable? Are you also going to export it?

> > +#define MSM_NAND_REG(off) (msm_nand_phys + (off))
> > +
> > +#define MSM_NAND_FLASH_CMD            MSM_NAND_REG(0x0000)
> > +#define MSM_NAND_ADDR0                MSM_NAND_REG(0x0004)
> >
> > Could you please make the macros to take the "struct msm_nand_chip
> > *chip" argument instead, and store the pase address there. Do not h=
ide
> > the fact that those macros are actually functions, not constant - t=
his
> > is error prone.
> >
> > Besides, I'm do not know your HW, but if you have several controlle=
rs
> > with various base addresses - your driver won't work.
>=20
> you are correct, we have multiple controllers, which breaks this logi=
c=20
> in future.

So then make your macros to accept the base address as an argument
instead please.

--=20
Best Regards,
Artem Bityutskiy (=D0=91=D0=B8=D1=82=D1=8E=D1=86=D0=BA=D0=B8=D0=B9 =D0=90=
=D1=80=D1=82=D1=91=D0=BC)