Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] Respond:Patch 0004-Add-support-for-SUNIX-parallel-card
To:     Morris Ku <saumah@gmail.com>, lee.jones@linaro.org
Cc:     linux-kernel@vger.kernel.org, morris_ku@sunix.com
References: <20190329112511.9087-1-saumah@gmail.com>
From:   "Enrico Weigelt, metux IT consult" <lkml@metux.net>
Organization: metux IT consult
Message-ID: <732b88dd-651b-6770-b1f1-956aa908d6e0@metux.net>
Date:   Mon, 1 Apr 2019 20:00:35 +0200
User-Agent: Mozilla/5.0 (X11; Linux i686 on x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <20190329112511.9087-1-saumah@gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 29.03.19 12:25, Morris Ku wrote:
> Hi,
> Thanks for review, my replies are inline:

https://www.netmeister.org/news/learn2quote.html

> +> +static struct snx_parport_driver snx_lp_driver = {
> +> +     .name = "lx",
> +> +     .attach = snx_lp_attach,
> +> +     .detach = snx_lp_detach,
> +> +};
> +
> +yet another case of duplication of some standard struct and hard-
> +typecasting ? use struct parport_driver here.
> +
> i will use standard struct(struct lp_driver) ,

you mean struct parport_driver ?

> about struct snx_parport driver,
> i will keep current format , because add a list for store device informations. 

No, your current approach breaks heavily (eg. as soon ans something in
struct parport_driver changes). If you really need to extend it, do it
by nested structs (same like struct parport_driver nests struct
device_driver), and use the proper macros (eg. container_of(), etc)
for typecasting.

Keep in mind: kernel-internal structures are NOT fixed, they can change
anytime - this is a fundamental design decision.

> +don't reimplement existing standard functionality. use the parport
> +subsystem.
> +
> +can i drop it ? it seems that it has no effect when port gone away.

If it's not needed, remove it. We don't like dead code.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287