LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Viet Nga Dao <vndao@altera.com>
To: "Rafał Miłecki" <zajec5@gmail.com>
Cc: Viet Nga Dao <vndao@altera.com>,
	Brian Norris <computersforpeace@gmail.com>,
	David Woodhouse <dwmw2@infradead.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-mtd@lists.infradead.org" <linux-mtd@lists.infradead.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] mtd:spi-nor: Add Altera EPCQ Driver
Date: Tue, 10 Mar 2015 14:11:51 +0800	[thread overview]
Message-ID: <CAN1oZWw2RUf7jSDGqk5UUrT3DDR2MHD6ORGQ1OLXQyES6+3Cjw@mail.gmail.com> (raw)
In-Reply-To: <CACna6ryC8TyxvvtztoSgZUXnE+tJdY6tefbXVkaZ60JoZ7ti7A@mail.gmail.com>

Hi Rafal,
Thanks for your review.

On Mon, Mar 9, 2015 at 2:31 PM, Rafał Miłecki <zajec5@gmail.com> wrote:
> Hi Viet,
>
> I'm not too active in mtd subsystem, so I didn't notice your patch
> earlier. However I would like to share few comments.
>
> On 11 February 2015 at 05:53, Viet Nga Dao <vndao@altera.com> wrote:
>> From: Viet Nga Dao <vndao@altera.com>
>>
>> Altera EPCQ Controller is a soft IP which enables access to Altera EPCQ and
>> EPCS flash chips. This patch adds driver for these devices.
>
> First of all, your whole patch is white-space damaged. It can't be
> applied, seems all tabs were replaced with spaces. It's what I got in
> my GMail and what was also received by patchwork, see
> https://patchwork.ozlabs.org/patch/438684/
>
> You'll need to resend using some smarter e-mail client, you may e.g.
> try git send-email.
>
>
>> +#define EPCQ_INFO(_opcode_id, _ext_id, _sector_size, _n_sectors, _page_size) \
>> +    ((kernel_ulong_t)&(struct flash_info) {                \
>> +        .altera_flash_opcode_id = (_opcode_id),            \
>> +        .ext_id = (_ext_id),                    \
>> +        .sector_size = (_sector_size),                \
>> +        .n_sectors = (_n_sectors),                \
>> +        .page_size = (_page_size),                \
>> +    })
>
> Starting with kernel 3.19 we don't have ext_id in struct anymore, see:
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=d928a259385dc6fca3956b7775c588f21c0b50fc
>
>

Noted. I will make the modification accordingly.

>>  /* NOTE: double check command sets and memory organization when you add
>>   * more nor chips.  This current list focusses on newer chips, which
>>   * have been converging on command sets which including JEDEC ID.
>> @@ -637,6 +691,17 @@ static const struct spi_device_id spi_nor_ids[] = {
>>      { "cat25c09", CAT25_INFO( 128, 8, 32, 2, SPI_NOR_NO_ERASE |
>> SPI_NOR_NO_FR) },
>>      { "cat25c17", CAT25_INFO( 256, 8, 32, 2, SPI_NOR_NO_ERASE |
>> SPI_NOR_NO_FR) },
>>      { "cat25128", CAT25_INFO(2048, 8, 64, 2, SPI_NOR_NO_ERASE |
>> SPI_NOR_NO_FR) },
>> +
>> +    /* Altera EPCQ/EPCS Flashes */
>> +    { "epcq16"  , EPCQ_INFO(2, 0x15, 0x10000, 32,   0x100) },
>> +    { "epcq32"  , EPCQ_INFO(2, 0x16, 0x10000, 64,   0x100) },
>> +    { "epcq64"  , EPCQ_INFO(2, 0x17, 0x10000, 128,  0x100) },
>> +    { "epcq128" , EPCQ_INFO(2, 0x18, 0x10000, 256,  0x100) },
>> +    { "epcq256" , EPCQ_INFO(2, 0x19, 0x10000, 512,  0x100) },
>> +    { "epcq512" , EPCQ_INFO(2, 0x20, 0x10000, 1024, 0x100) },
>> +    { "epcs16"  , EPCQ_INFO(1, 0x14, 0x10000, 32,   0x100) },
>> +    { "epcs64"  , EPCQ_INFO(1, 0x16, 0x10000, 128,  0x100) },
>> +    { "epcs128" , EPCQ_INFO(1, 0x18, 0x40000, 256,  0x100) },
>>      { },
>>  };
>>
>> @@ -666,6 +731,14 @@ static const struct spi_device_id
>> *spi_nor_read_id(struct spi_nor *nor)
>>          if (info->jedec_id == jedec) {
>>              if (info->ext_id == 0 || info->ext_id == ext_jedec)
>>                  return &spi_nor_ids[tmp];
>> +
>> +        /* altera epcq which is non jedec device
>> +         * use id[4] as opcode id to differentiate
>> +         * epcs and epcq devices
>> +         */
>> +        } else if (info->altera_flash_opcode_id == id[4] &&
>> +              info->ext_id == id[3]) {
>> +                return &spi_nor_ids[tmp];
>
> This is the part I don't like. I think it's fishy, and that this check
> may result in false positives. Looks too generic.
>
> Also the logic of your behavior there seems unclear to me. On the one
> hand you don't have JEDEC, so you provide chip name using DT. But in
> place above you stop trusting DT info and you try to (kind of)
> auto-detect used chip anyway.
>
> I guess we should finally think about some more generic way of passing
> flash info.

Actually, i just want fo follow the way current spi-nor doing as much
as possible. Like to read the device id and compare with info table.
Like double checking from both dtb and the device id. Since the
flashes i support do not have JEDEC id but only extended id. But the
problem is that some of them have the same extended id, for example
epcs64 and epcq32). That is why in my driver, i have to decode 1st
byte of ext id to differentiate epcs and ecpq.

  reply	other threads:[~2015-03-10  6:11 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-02-11  4:53 Viet Nga Dao
2015-02-17  1:33 ` Viet Nga Dao
2015-02-23  1:30   ` Viet Nga Dao
2015-02-24  3:59     ` Brian Norris
2015-03-09  1:42       ` Viet Nga Dao
2015-03-13  2:42     ` Viet Nga Dao
2015-03-09  6:31 ` Rafał Miłecki
2015-03-10  6:11   ` Viet Nga Dao [this message]
2015-03-10  6:55     ` Rafał Miłecki
     [not found]       ` <A8F913A698B3864C898FDDF9BF921BC9E8C00F@PG-ITEXCH01.altera.priv.altera.com>
2015-03-10  8:09         ` FW: " Viet Nga Dao
2015-03-11  8:41           ` Viet Nga Dao
2015-03-13  5:45             ` Rafał Miłecki
2015-03-13  6:47               ` Viet Nga Dao
2015-03-13  7:38 ` Brian Norris
2015-03-13  8:25   ` Viet Nga Dao
2015-03-16  8:46   ` Viet Nga Dao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAN1oZWw2RUf7jSDGqk5UUrT3DDR2MHD6ORGQ1OLXQyES6+3Cjw@mail.gmail.com \
    --to=vndao@altera.com \
    --cc=computersforpeace@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dwmw2@infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=zajec5@gmail.com \
    --subject='Re: [PATCH v2] mtd:spi-nor: Add Altera EPCQ Driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).