* [PATCH] Respond:Patch 0004-Add-support-for-SUNIX-parallel-card
@ 2019-03-29 11:25 Morris Ku
2019-04-01 10:17 ` Lee Jones
2019-04-01 18:00 ` Enrico Weigelt, metux IT consult
0 siblings, 2 replies; 3+ messages in thread
From: Morris Ku @ 2019-03-29 11:25 UTC (permalink / raw)
To: lee.jones; +Cc: linux-kernel, morris_ku, lkml, Morris Ku
Hi,
Thanks for review, my replies are inline:
Signed-off-by: Morris Ku <saumah@gmail.com>
---
@@ -0,0 +1,255 @@
+
+On 19.03.19 13:08, Morris Ku wrote:
+
+> diff --git a/mfd/sunix/snx_ieee1284_ops.c b/mfd/sunix/snx_ieee1284_ops.c
+> new file mode 100644
+> index 00000000..2dac03fd
+> --- /dev/null
+
+<snip>
+
+> +size_t sunix_parport_ieee1284_read_nibble(struct snx_parport *port,
+> +void *buffer, size_t len, int flags)
+> +{
+> + return 0;
+> +}
+> +
+> +size_t sunix_parport_ieee1284_read_byte(struct snx_parport *port,
+> +void *buffer, size_t len, int flags)
+> +{
+> + return 0;
+> +}
+> +
+> +size_t sunix_parport_ieee1284_ecp_write_data(struct snx_parport *port,
+> +const void *buffer, size_t len, int flags)
+> +{
+> + return 0;
+> +}
+> +
+> +size_t sunix_parport_ieee1284_ecp_read_data(struct snx_parport *port,
+> +void *buffer, size_t len, int flags)
+> +{
+> + return 0;
+> +}
+> +
+> +size_t sunix_parport_ieee1284_ecp_write_addr(struct snx_parport *port,
+> +const void *buffer, size_t len, int flags)
+> +{
+> + return 0;
+> +}
+
+Why are these all no-ops ?
+
+i will fix it.
+
+> diff --git a/mfd/sunix/snx_lp.c b/mfd/sunix/snx_lp.c
+> new file mode 100644
+> index 00000000..f2478447
+> --- /dev/null
+> +++ b/mfd/sunix/snx_lp.c
+
+<snip>
+
+> +#undef SNX_LP_STATS
+
+what is this ?
+
+i will fix it.
+
+> +static int SNX_PAL_MAJOR;
+> +
+> +#define SNX_LP_NO SNX_PAR_TOTAL_MAX
+> +#undef SNX_CONFIG_LP_CONSOLE
+
+and this ?
+
+i will fix it.
+
+> +#ifdef SNX_CONFIG_PARPORT_1284
+
+dont do #define in the middle of the code.
+
+i will fix it.
+
+> +static const struct file_operations snx_lp_fops = {
+> + .owner = THIS_MODULE,
+> + .write = snx_lp_write,
+> + .open = snx_lp_open,
+> + .release = snx_lp_release,
+> +#ifdef SNX_CONFIG_PARPORT_1284
+> + .read = snx_lp_read,
+> +#endif
+> +};
+
+dont reimplement existing standard functionality your own weird way.
+use the partport subsystem. see: Documentation/parport-lowlevel.txt
+
+i will fix it.
+
+> +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) , about struct snx_parport driver,
+i will keep current format , because add a list for store device informations.
+
+> + SNX_PAL_MAJOR = register_chrdev(0, "lx", &snx_lp_fops);
+
+dont register your own chardev - use the parport subsystem.
+
+i will fix it.
+
+> diff --git a/mfd/sunix/snx_parallel.c b/mfd/sunix/snx_parallel.c
+> new file mode 100644
+> index 00000000..461ea4cc
+> --- /dev/null
+
+<snip>
+
+> +struct snx_parport *sunix_parport_pc_probe_port(struct sunix_par_port *priv)
+> +{
+> + struct snx_parport_ops *ops = NULL;
+> + struct snx_parport *p = NULL;
+> + struct resource *base_res;
+> + struct resource *ecr_res = NULL;
+> +
+> + if (!priv)
+> + goto out1;
+> +
+> + ops = kmalloc(sizeof(struct snx_parport_ops), GFP_KERNEL);
+
+why not kzmalloc ?
+
+i will fix it.
+
+> diff --git a/mfd/sunix/snx_ppdev.c b/mfd/sunix/snx_ppdev.c
+> new file mode 100644
+> index 00000000..9482ed9f
+> --- /dev/null
+> +++ b/mfd/sunix/snx_ppdev.c
+
+<snip>
+
+> +static const struct file_operations snx_pp_fops = {
+> + .owner = THIS_MODULE,
+> + .llseek = no_llseek,
+> + .read = snx_pp_read,
+> + .write = snx_pp_write,
+> + .poll = snx_pp_poll,
+> + .unlocked_ioctl = snx_dump_par_ioctl,
+> +
+> + .open = snx_pp_open,
+> + .release = snx_pp_release,
+> +};
+
+don't reimplement existing standard functionality - use the parport
+subsystem.
+
+i will fix it.
+
+> diff --git a/mfd/sunix/snx_ppdev.h b/mfd/sunix/snx_ppdev.h
+> new file mode 100644
+> index 00000000..0dfec064
+> --- /dev/null
+> +++ b/mfd/sunix/snx_ppdev.h
+> @@ -0,0 +1,15 @@
+> +/* SPDX-License-Identifier: GPL-2.0 */
+> +#include "snx_common.h"
+> +
+> +#define SNX_PP_IOCTL 'p'
+> +
+> +#define SNX_PP_FASTWRITE (1<<2)
+> +#define SNX_PP_FASTREAD (1<<3)
+> +#define SNX_PP_W91284PIC (1<<4)
+
+use the BIT() macro
+
+i will use BIT() macro for bits definition. (DONE)
+
+> diff --git a/mfd/sunix/snx_share.c b/mfd/sunix/snx_share.c
+> new file mode 100644
+> index 00000000..ba6f86a2
+> --- /dev/null
+> +++ b/mfd/sunix/snx_share.c
+> @@ -0,0 +1,629 @@
+> +// SPDX-License-Identifier: GPL-2.0
+> +#include "snx_common.h"
+> +#define SNX_PARPORT_DEFAULT_TIMESLICE (HZ/5)
+> +
+> +unsigned long sunix_parport_default_timeslice = SNX_PARPORT_DEFAULT_TIMESLICE;
+> +int sunix_parport_default_spintime = DEFAULT_SPIN_TIME;
+> +
+> +static LIST_HEAD(snx_portlist);
+> +static DEFINE_SPINLOCK(snx_full_list_lock);
+> +
+> +static DEFINE_SPINLOCK(snx_parportlist_lock);
+> +
+> +static LIST_HEAD(snx_all_ports);
+> +static LIST_HEAD(snx_drivers);
+> +
+> +static DEFINE_SEMAPHORE(snx_registration_lock);
+> +
+> +static void sunix_dead_write_lines(
+> +struct snx_parport *p, unsigned char b)
+> +{}
+> +static unsigned char sunix_dead_read_lines(
+> +struct snx_parport *p)
+> +{ return 0; }
+> +static unsigned char sunix_dead_frob_lines(
+> +struct snx_parport *p, unsigned char b, unsigned char c)
+> +{ return 0; }
+> +static void sunix_dead_onearg(struct snx_parport *p)
+> +{}
+> +static void sunix_dead_initstate(
+> +struct snx_pardevice *d, struct snx_parport_state *s)
+> +{}
+> +static void sunix_dead_state(
+> +struct snx_parport *p, struct snx_parport_state *s)
+> +{}
+> +static size_t sunix_dead_write(
+> +struct snx_parport *p, const void *b, size_t l, int f)
+> +{ return 0; }
+> +static size_t sunix_dead_read(
+> +struct snx_parport *p, void *b, size_t l, int f)
+> +{ return 0; }
+> +
+> +
+> +static struct snx_parport_ops sunix_dead_ops = {
+> + .write_data = sunix_dead_write_lines,
+> + .read_data = sunix_dead_read_lines,
+> + .write_control = sunix_dead_write_lines,
+> + .read_control = sunix_dead_read_lines,
+> + .frob_control = sunix_dead_frob_lines,
+> + .read_status = sunix_dead_read_lines,
+> + .enable_irq = sunix_dead_onearg,
+> + .disable_irq = sunix_dead_onearg,
+> + .data_forward = sunix_dead_onearg,
+> + .data_reverse = sunix_dead_onearg,
+> + .init_state = sunix_dead_initstate,
+> + .save_state = sunix_dead_state,
+> + .restore_state = sunix_dead_state,
+> + .epp_write_data = sunix_dead_write,
+> + .epp_read_data = sunix_dead_read,
+> + .epp_write_addr = sunix_dead_write,
+> + .epp_read_addr = sunix_dead_read,
+> + .ecp_write_data = sunix_dead_write,
+> + .ecp_read_data = sunix_dead_read,
+> + .ecp_write_addr = sunix_dead_write,
+> + .compat_write_data = sunix_dead_write,
+> + .nibble_read_data = sunix_dead_read,
+> + .byte_read_data = sunix_dead_read,
+> + .owner = NULL,
+> +};
+
+
+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.
+
+--mtx
--
2.17.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Respond:Patch 0004-Add-support-for-SUNIX-parallel-card
2019-03-29 11:25 [PATCH] Respond:Patch 0004-Add-support-for-SUNIX-parallel-card Morris Ku
@ 2019-04-01 10:17 ` Lee Jones
2019-04-01 18:00 ` Enrico Weigelt, metux IT consult
1 sibling, 0 replies; 3+ messages in thread
From: Lee Jones @ 2019-04-01 10:17 UTC (permalink / raw)
To: Morris Ku; +Cc: linux-kernel, morris_ku, lkml
On Fri, 29 Mar 2019, Morris Ku wrote:
> Hi,
> Thanks for review, my replies are inline:
I literally have no idea what this is!
It's a reply to a diff of a reply to a patch AFAICS!
I think the best I can do is point you to:
Documentation/process/submitting-patches.rst
> Signed-off-by: Morris Ku <saumah@gmail.com>
> ---
> @@ -0,0 +1,255 @@
> +
> +On 19.03.19 13:08, Morris Ku wrote:
> +
> +> diff --git a/mfd/sunix/snx_ieee1284_ops.c b/mfd/sunix/snx_ieee1284_ops.c
> +> new file mode 100644
> +> index 00000000..2dac03fd
> +> --- /dev/null
> +
> +<snip>
> +
> +> +size_t sunix_parport_ieee1284_read_nibble(struct snx_parport *port,
> +> +void *buffer, size_t len, int flags)
> +> +{
> +> + return 0;
> +> +}
> +> +
> +> +size_t sunix_parport_ieee1284_read_byte(struct snx_parport *port,
> +> +void *buffer, size_t len, int flags)
> +> +{
> +> + return 0;
> +> +}
> +> +
> +> +size_t sunix_parport_ieee1284_ecp_write_data(struct snx_parport *port,
> +> +const void *buffer, size_t len, int flags)
> +> +{
> +> + return 0;
> +> +}
> +> +
> +> +size_t sunix_parport_ieee1284_ecp_read_data(struct snx_parport *port,
> +> +void *buffer, size_t len, int flags)
> +> +{
> +> + return 0;
> +> +}
> +> +
> +> +size_t sunix_parport_ieee1284_ecp_write_addr(struct snx_parport *port,
> +> +const void *buffer, size_t len, int flags)
> +> +{
> +> + return 0;
> +> +}
> +
> +Why are these all no-ops ?
> +
> +i will fix it.
> +
> +> diff --git a/mfd/sunix/snx_lp.c b/mfd/sunix/snx_lp.c
> +> new file mode 100644
> +> index 00000000..f2478447
> +> --- /dev/null
> +> +++ b/mfd/sunix/snx_lp.c
> +
> +<snip>
> +
> +> +#undef SNX_LP_STATS
> +
> +what is this ?
> +
> +i will fix it.
> +
> +> +static int SNX_PAL_MAJOR;
> +> +
> +> +#define SNX_LP_NO SNX_PAR_TOTAL_MAX
> +> +#undef SNX_CONFIG_LP_CONSOLE
> +
> +and this ?
> +
> +i will fix it.
> +
> +> +#ifdef SNX_CONFIG_PARPORT_1284
> +
> +dont do #define in the middle of the code.
> +
> +i will fix it.
> +
> +> +static const struct file_operations snx_lp_fops = {
> +> + .owner = THIS_MODULE,
> +> + .write = snx_lp_write,
> +> + .open = snx_lp_open,
> +> + .release = snx_lp_release,
> +> +#ifdef SNX_CONFIG_PARPORT_1284
> +> + .read = snx_lp_read,
> +> +#endif
> +> +};
> +
> +dont reimplement existing standard functionality your own weird way.
> +use the partport subsystem. see: Documentation/parport-lowlevel.txt
> +
> +i will fix it.
> +
> +> +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) , about struct snx_parport driver,
> +i will keep current format , because add a list for store device informations.
> +
> +> + SNX_PAL_MAJOR = register_chrdev(0, "lx", &snx_lp_fops);
> +
> +dont register your own chardev - use the parport subsystem.
> +
> +i will fix it.
> +
> +> diff --git a/mfd/sunix/snx_parallel.c b/mfd/sunix/snx_parallel.c
> +> new file mode 100644
> +> index 00000000..461ea4cc
> +> --- /dev/null
> +
> +<snip>
> +
> +> +struct snx_parport *sunix_parport_pc_probe_port(struct sunix_par_port *priv)
> +> +{
> +> + struct snx_parport_ops *ops = NULL;
> +> + struct snx_parport *p = NULL;
> +> + struct resource *base_res;
> +> + struct resource *ecr_res = NULL;
> +> +
> +> + if (!priv)
> +> + goto out1;
> +> +
> +> + ops = kmalloc(sizeof(struct snx_parport_ops), GFP_KERNEL);
> +
> +why not kzmalloc ?
> +
> +i will fix it.
> +
> +> diff --git a/mfd/sunix/snx_ppdev.c b/mfd/sunix/snx_ppdev.c
> +> new file mode 100644
> +> index 00000000..9482ed9f
> +> --- /dev/null
> +> +++ b/mfd/sunix/snx_ppdev.c
> +
> +<snip>
> +
> +> +static const struct file_operations snx_pp_fops = {
> +> + .owner = THIS_MODULE,
> +> + .llseek = no_llseek,
> +> + .read = snx_pp_read,
> +> + .write = snx_pp_write,
> +> + .poll = snx_pp_poll,
> +> + .unlocked_ioctl = snx_dump_par_ioctl,
> +> +
> +> + .open = snx_pp_open,
> +> + .release = snx_pp_release,
> +> +};
> +
> +don't reimplement existing standard functionality - use the parport
> +subsystem.
> +
> +i will fix it.
> +
> +> diff --git a/mfd/sunix/snx_ppdev.h b/mfd/sunix/snx_ppdev.h
> +> new file mode 100644
> +> index 00000000..0dfec064
> +> --- /dev/null
> +> +++ b/mfd/sunix/snx_ppdev.h
> +> @@ -0,0 +1,15 @@
> +> +/* SPDX-License-Identifier: GPL-2.0 */
> +> +#include "snx_common.h"
> +> +
> +> +#define SNX_PP_IOCTL 'p'
> +> +
> +> +#define SNX_PP_FASTWRITE (1<<2)
> +> +#define SNX_PP_FASTREAD (1<<3)
> +> +#define SNX_PP_W91284PIC (1<<4)
> +
> +use the BIT() macro
> +
> +i will use BIT() macro for bits definition. (DONE)
> +
> +> diff --git a/mfd/sunix/snx_share.c b/mfd/sunix/snx_share.c
> +> new file mode 100644
> +> index 00000000..ba6f86a2
> +> --- /dev/null
> +> +++ b/mfd/sunix/snx_share.c
> +> @@ -0,0 +1,629 @@
> +> +// SPDX-License-Identifier: GPL-2.0
> +> +#include "snx_common.h"
> +> +#define SNX_PARPORT_DEFAULT_TIMESLICE (HZ/5)
> +> +
> +> +unsigned long sunix_parport_default_timeslice = SNX_PARPORT_DEFAULT_TIMESLICE;
> +> +int sunix_parport_default_spintime = DEFAULT_SPIN_TIME;
> +> +
> +> +static LIST_HEAD(snx_portlist);
> +> +static DEFINE_SPINLOCK(snx_full_list_lock);
> +> +
> +> +static DEFINE_SPINLOCK(snx_parportlist_lock);
> +> +
> +> +static LIST_HEAD(snx_all_ports);
> +> +static LIST_HEAD(snx_drivers);
> +> +
> +> +static DEFINE_SEMAPHORE(snx_registration_lock);
> +> +
> +> +static void sunix_dead_write_lines(
> +> +struct snx_parport *p, unsigned char b)
> +> +{}
> +> +static unsigned char sunix_dead_read_lines(
> +> +struct snx_parport *p)
> +> +{ return 0; }
> +> +static unsigned char sunix_dead_frob_lines(
> +> +struct snx_parport *p, unsigned char b, unsigned char c)
> +> +{ return 0; }
> +> +static void sunix_dead_onearg(struct snx_parport *p)
> +> +{}
> +> +static void sunix_dead_initstate(
> +> +struct snx_pardevice *d, struct snx_parport_state *s)
> +> +{}
> +> +static void sunix_dead_state(
> +> +struct snx_parport *p, struct snx_parport_state *s)
> +> +{}
> +> +static size_t sunix_dead_write(
> +> +struct snx_parport *p, const void *b, size_t l, int f)
> +> +{ return 0; }
> +> +static size_t sunix_dead_read(
> +> +struct snx_parport *p, void *b, size_t l, int f)
> +> +{ return 0; }
> +> +
> +> +
> +> +static struct snx_parport_ops sunix_dead_ops = {
> +> + .write_data = sunix_dead_write_lines,
> +> + .read_data = sunix_dead_read_lines,
> +> + .write_control = sunix_dead_write_lines,
> +> + .read_control = sunix_dead_read_lines,
> +> + .frob_control = sunix_dead_frob_lines,
> +> + .read_status = sunix_dead_read_lines,
> +> + .enable_irq = sunix_dead_onearg,
> +> + .disable_irq = sunix_dead_onearg,
> +> + .data_forward = sunix_dead_onearg,
> +> + .data_reverse = sunix_dead_onearg,
> +> + .init_state = sunix_dead_initstate,
> +> + .save_state = sunix_dead_state,
> +> + .restore_state = sunix_dead_state,
> +> + .epp_write_data = sunix_dead_write,
> +> + .epp_read_data = sunix_dead_read,
> +> + .epp_write_addr = sunix_dead_write,
> +> + .epp_read_addr = sunix_dead_read,
> +> + .ecp_write_data = sunix_dead_write,
> +> + .ecp_read_data = sunix_dead_read,
> +> + .ecp_write_addr = sunix_dead_write,
> +> + .compat_write_data = sunix_dead_write,
> +> + .nibble_read_data = sunix_dead_read,
> +> + .byte_read_data = sunix_dead_read,
> +> + .owner = NULL,
> +> +};
> +
> +
> +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.
> +
> +--mtx
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Respond:Patch 0004-Add-support-for-SUNIX-parallel-card
2019-03-29 11:25 [PATCH] Respond:Patch 0004-Add-support-for-SUNIX-parallel-card Morris Ku
2019-04-01 10:17 ` Lee Jones
@ 2019-04-01 18:00 ` Enrico Weigelt, metux IT consult
1 sibling, 0 replies; 3+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2019-04-01 18:00 UTC (permalink / raw)
To: Morris Ku, lee.jones; +Cc: linux-kernel, morris_ku
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-04-01 18:00 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-29 11:25 [PATCH] Respond:Patch 0004-Add-support-for-SUNIX-parallel-card Morris Ku
2019-04-01 10:17 ` Lee Jones
2019-04-01 18:00 ` Enrico Weigelt, metux IT consult
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.