| Message ID | 1362038541-22406-3-git-send-email-shc_work@mail.ru |
|---|---|
| State | New, archived |
| Headers | show |
Hi Alexander, I'm not an expert on IO accessors, but since you asked... On 02/28/2013 12:02 AM, Alexander Shiyan wrote: > This patch provide using readsb/readsw/writesb/writesw functions > if it possible for particular platform. The commit description does not give enough motivation. Should this improve performance? Or is this just a "cleanup"? (If the latter, then it fails, as the resulting code is much less clean.) > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- > drivers/mtd/nand/nand_base.c | 29 ++++++++++++++++++++++++----- > 1 files changed, 24 insertions(+), 5 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 382b857..4efde01 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -214,11 +214,16 @@ static void nand_select_chip(struct mtd_info *mtd, int chipnr) > */ > static void nand_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) > { > - int i; > struct nand_chip *chip = mtd->priv; > > +#ifndef writesb No one guarantees that 'writesb' will be a macro and not a static inline function, do they? > + int i; > + > for (i = 0; i < len; i++) > writeb(buf[i], chip->IO_ADDR_W); > +#else > + writesb(chip->IO_ADDR_W, buf, len); > +#endif > } These #ifndef's are very ugly and are likely the wrong way(TM). The first question I ask when I see this is: why isn't this interface available on all platforms? In fact, it seems like it has been dropped from the asm-generic headers and specifically recommended against. See: commit b2656a138ab7bc4e7abd3b1cbd6d1f105c7a7186 Author: Will Deacon <will.deacon@arm.com> Date: Wed Oct 17 16:45:01 2012 +0100 asm-generic: io: remove {read,write} string functions Quote: "We should encourage driver writers to use the io{read,write}{8,16,32}_rep functions instead." So, an alternative patch might use iowrite8_rep() here unconditionally. > /** <snipped the rest; same comments apply> Brian
> Hi Alexander, > > I'm not an expert on IO accessors, but since you asked... > > On 02/28/2013 12:02 AM, Alexander Shiyan wrote: > > This patch provide using readsb/readsw/writesb/writesw functions > > if it possible for particular platform. > > The commit description does not give enough motivation. Should this > improve performance? Or is this just a "cleanup"? (If the latter, then > it fails, as the resulting code is much less clean.) Basic idea is a performance, because architecture may have a improved functions for multiple read/write. > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > > --- > > drivers/mtd/nand/nand_base.c | 29 ++++++++++++++++++++++++----- > > 1 files changed, 24 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > > index 382b857..4efde01 100644 > > --- a/drivers/mtd/nand/nand_base.c > > +++ b/drivers/mtd/nand/nand_base.c > > @@ -214,11 +214,16 @@ static void nand_select_chip(struct mtd_info *mtd, int chipnr) > > */ > > static void nand_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) > > { > > - int i; > > struct nand_chip *chip = mtd->priv; > > > > +#ifndef writesb > > No one guarantees that 'writesb' will be a macro and not a static inline > function, do they? > > > + int i; > > + > > for (i = 0; i < len; i++) > > writeb(buf[i], chip->IO_ADDR_W); > > +#else > > + writesb(chip->IO_ADDR_W, buf, len); > > +#endif > > } > > These #ifndef's are very ugly and are likely the wrong way(TM). The > first question I ask when I see this is: why isn't this interface > available on all platforms? In fact, it seems like it has been dropped > from the asm-generic headers and specifically recommended against. See: > > commit b2656a138ab7bc4e7abd3b1cbd6d1f105c7a7186 > Author: Will Deacon <will.deacon@arm.com> > Date: Wed Oct 17 16:45:01 2012 +0100 > > asm-generic: io: remove {read,write} string functions > > Quote: "We should encourage driver writers to use the > io{read,write}{8,16,32}_rep functions instead." > > So, an alternative patch might use iowrite8_rep() here unconditionally. Indeed. So I can remade patch to use ioread/iowrite and remove unnecessary #ifdefs if you have not additional comments. ---
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 382b857..4efde01 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -214,11 +214,16 @@ static void nand_select_chip(struct mtd_info *mtd, int chipnr) */ static void nand_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) { - int i; struct nand_chip *chip = mtd->priv; +#ifndef writesb + int i; + for (i = 0; i < len; i++) writeb(buf[i], chip->IO_ADDR_W); +#else + writesb(chip->IO_ADDR_W, buf, len); +#endif } /** @@ -231,11 +236,16 @@ static void nand_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len) */ static void nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) { - int i; struct nand_chip *chip = mtd->priv; +#ifndef readsb + int i; + for (i = 0; i < len; i++) buf[i] = readb(chip->IO_ADDR_R); +#else + readsb(chip->IO_ADDR_R, buf, len); +#endif } /** @@ -248,14 +258,18 @@ static void nand_read_buf(struct mtd_info *mtd, uint8_t *buf, int len) */ static void nand_write_buf16(struct mtd_info *mtd, const uint8_t *buf, int len) { - int i; struct nand_chip *chip = mtd->priv; u16 *p = (u16 *) buf; len >>= 1; +#ifndef writesw + int i; + for (i = 0; i < len; i++) writew(p[i], chip->IO_ADDR_W); - +#else + writesw(chip->IO_ADDR_W, p, len); +#endif } /** @@ -268,13 +282,18 @@ static void nand_write_buf16(struct mtd_info *mtd, const uint8_t *buf, int len) */ static void nand_read_buf16(struct mtd_info *mtd, uint8_t *buf, int len) { - int i; struct nand_chip *chip = mtd->priv; u16 *p = (u16 *) buf; len >>= 1; +#ifndef readsw + int i; + for (i = 0; i < len; i++) p[i] = readw(chip->IO_ADDR_R); +#else + readsw(chip->IO_ADDR_R, p, len); +#endif } /**
This patch provide using readsb/readsw/writesb/writesw functions if it possible for particular platform. Signed-off-by: Alexander Shiyan <shc_work@mail.ru> --- drivers/mtd/nand/nand_base.c | 29 ++++++++++++++++++++++++----- 1 files changed, 24 insertions(+), 5 deletions(-)