Messages in this thread | | | Subject | Re: [PATCH 4/4] Add support for SUNIX Multi-I/O board | From | "Enrico Weigelt, metux IT consult" <> | Date | Fri, 22 Mar 2019 17:55:16 +0100 |
| |
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 ?
> 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 ?
> +static int SNX_PAL_MAJOR; > + > +#define SNX_LP_NO SNX_PAR_TOTAL_MAX > +#undef SNX_CONFIG_LP_CONSOLE
and this ?
> +#ifdef SNX_CONFIG_PARPORT_1284
dont do #define in the middle of the code.
> +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
> +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.
> + SNX_PAL_MAJOR = register_chrdev(0, "lx", &snx_lp_fops);
dont register your own chardev - use the parport subsystem.
> 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 ?
> 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.
> 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
> 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.
--mtx
-- Enrico Weigelt, metux IT consult Free software and Linux embedded engineering info@metux.net -- +49-151-27565287
| |