Hi,
Thanks for review, my replies are inline:
Signed-off-by: Morris Ku <[email protected]>
---
@@ -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
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 <[email protected]>
> ---
> @@ -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
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
[email protected] -- +49-151-27565287