All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
To: Boris Brezillon
	<boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
Cc: robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
	daniel.thompson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org,
	erin.lo-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org,
	linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
	linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
	xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org,
	computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
	dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org
Subject: Re: [RFCv2: PATCH 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND
Date: Tue, 22 Mar 2016 19:43:13 -0400	[thread overview]
Message-ID: <56F1D891.8010603@linaro.org> (raw)
In-Reply-To: <20160322175850.2722c389@bbrezillon>

On 03/22/2016 12:58 PM, Boris Brezillon wrote:
>> +typedef void (*release)(struct sdg1_ecc_if *);
>> > +typedef int (*decode)(struct sdg1_ecc_if *);
>> > +typedef void (*init) (struct sdg1_ecc_if *);
>> > +
>> > +struct sdg1_ecc_if {
>> > +	release	release;
>> > +	control control;
>> > +	config config;
>> > +	encode encode;
>> > +	decode decode;
>> > +	check check;
>> > +	init init;
>> > +};
>> > +
>> > +struct sdg1_ecc_if *of_sdg1_ecc_get(struct device_node *);
> I think you should directly return a pointer to the sdg1_ecc instance
> here.
> AFAICS, the "interface" appraoch is useless, all you need is a set of
> exported functions, which all takes the sdg1_ecc pointer as a first
> argument and a set of extra arguments depending on the function.
> See what's done in the jz4780_bch.c driver.
>

yes I am of the contrary opinion (why export a number of functions when you can
just export an interface and keep a clear encapsulation).
as a matter of fact the interface exposes the dependency with the engine while a
random number of function calls does not (with the interface approach eventually
we might be able to have a common ecc interface some day, maybe)

anyway, working on v3 now. thanks for the detailed review as always (and sorry
for the clear fuck ups like the size 0 array in the wrong place...argh!)

WARNING: multiple messages have this Message-ID (diff)
From: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
To: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: dwmw2@infradead.org, computersforpeace@gmail.com,
	matthias.bgg@gmail.com, robh@kernel.org,
	linux-mtd@lists.infradead.org, xiaolei.li@mediatek.com,
	daniel.thompson@linaro.org, erin.lo@mediatek.com,
	linux-mediatek@lists.infradead.org
Subject: Re: [RFCv2: PATCH 2/2] mtd: mediatek: driver for MTK Smart Device Gen1 NAND
Date: Tue, 22 Mar 2016 19:43:13 -0400	[thread overview]
Message-ID: <56F1D891.8010603@linaro.org> (raw)
In-Reply-To: <20160322175850.2722c389@bbrezillon>

On 03/22/2016 12:58 PM, Boris Brezillon wrote:
>> +typedef void (*release)(struct sdg1_ecc_if *);
>> > +typedef int (*decode)(struct sdg1_ecc_if *);
>> > +typedef void (*init) (struct sdg1_ecc_if *);
>> > +
>> > +struct sdg1_ecc_if {
>> > +	release	release;
>> > +	control control;
>> > +	config config;
>> > +	encode encode;
>> > +	decode decode;
>> > +	check check;
>> > +	init init;
>> > +};
>> > +
>> > +struct sdg1_ecc_if *of_sdg1_ecc_get(struct device_node *);
> I think you should directly return a pointer to the sdg1_ecc instance
> here.
> AFAICS, the "interface" appraoch is useless, all you need is a set of
> exported functions, which all takes the sdg1_ecc pointer as a first
> argument and a set of extra arguments depending on the function.
> See what's done in the jz4780_bch.c driver.
>

yes I am of the contrary opinion (why export a number of functions when you can
just export an interface and keep a clear encapsulation).
as a matter of fact the interface exposes the dependency with the engine while a
random number of function calls does not (with the interface approach eventually
we might be able to have a common ecc interface some day, maybe)

anyway, working on v3 now. thanks for the detailed review as always (and sorry
for the clear fuck ups like the size 0 array in the wrong place...argh!)

  reply	other threads:[~2016-03-22 23:43 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-22 13:32 [RFCv2 0/2] MTK Smart Device Gen1 NAND Driver Jorge Ramirez-Ortiz
2016-03-22 13:32 ` Jorge Ramirez-Ortiz
     [not found] ` <1458653560-2679-1-git-send-email-jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2016-03-22 13:32   ` [RFCv2: PATCH 1/2] mtd: mediatek: device tree docs for MTK Smart Device Gen1 NAND Jorge Ramirez-Ortiz
2016-03-22 13:32     ` Jorge Ramirez-Ortiz
     [not found]     ` <1458653560-2679-2-git-send-email-jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2016-03-22 13:52       ` Boris Brezillon
2016-03-22 13:52         ` Boris Brezillon
2016-03-26 18:38         ` Jorge Ramirez-Ortiz
2016-03-26 18:38           ` Jorge Ramirez-Ortiz
     [not found]           ` <56F6D727.8070001-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2016-03-29  7:58             ` Boris Brezillon
2016-03-29  7:58               ` Boris Brezillon
2016-03-31 12:41               ` Jorge Ramirez-Ortiz
2016-03-31 12:41                 ` Jorge Ramirez-Ortiz
2016-03-22 13:32   ` [RFCv2: PATCH 2/2] mtd: mediatek: driver " Jorge Ramirez-Ortiz
2016-03-22 13:32     ` Jorge Ramirez-Ortiz
     [not found]     ` <1458653560-2679-3-git-send-email-jorge.ramirez-ortiz-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2016-03-22 16:58       ` Boris Brezillon
2016-03-22 16:58         ` Boris Brezillon
2016-03-22 23:43         ` Jorge Ramirez-Ortiz [this message]
2016-03-22 23:43           ` Jorge Ramirez-Ortiz
     [not found]           ` <56F1D891.8010603-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2016-03-23  8:41             ` Boris Brezillon
2016-03-23  8:41               ` Boris Brezillon
2016-03-23 12:44               ` Jorge Ramirez-Ortiz
2016-03-23 12:44                 ` Jorge Ramirez-Ortiz
2016-03-23  0:29         ` Jorge Ramirez-Ortiz
2016-03-23  0:29           ` Jorge Ramirez-Ortiz
     [not found]           ` <56F1E378.3000107-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
2016-03-23  8:26             ` Boris Brezillon
2016-03-23  8:26               ` Boris Brezillon
2016-03-22 13:47   ` [RFCv2 0/2] MTK Smart Device Gen1 NAND Driver Boris Brezillon
2016-03-22 13:47     ` Boris Brezillon
2016-03-22 14:08     ` Jorge Ramirez-Ortiz
2016-03-22 14:08       ` Jorge Ramirez-Ortiz

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=56F1D891.8010603@linaro.org \
    --to=jorge.ramirez-ortiz-qsej5fyqhm4dnm+yrofe0a@public.gmane.org \
    --cc=boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org \
    --cc=computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=daniel.thompson-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
    --cc=dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org \
    --cc=erin.lo-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org \
    --cc=linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
    --cc=linux-mtd-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
    --cc=matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
    --cc=xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.