Messages in this thread | | | Subject | Re: [PATCH] Respond:Patch 0004-Add-support-for-SUNIX-parallel-card | From | "Enrico Weigelt, metux IT consult" <> | Date | Mon, 1 Apr 2019 20:00:35 +0200 |
| |
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
| |