From: "Enrico Weigelt, metux IT consult" <lkml@metux.net>
To: Morris Ku <saumah@gmail.com>, lee.jones@linaro.org
Cc: linux-kernel@vger.kernel.org, morris_ku@sunix.com
Subject: Re: [PATCH 4/4] Add support for SUNIX Multi-I/O board
Date: Fri, 22 Mar 2019 17:55:16 +0100 [thread overview]
Message-ID: <bf8659eb-ecf8-a9e4-33f1-c9bbb7f5ee13@metux.net> (raw)
In-Reply-To: <20190319120835.3840-1-saumah@gmail.com>
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
next prev parent reply other threads:[~2019-03-22 16:55 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-19 12:08 [PATCH 4/4] Add support for SUNIX Multi-I/O board Morris Ku
2019-03-22 16:55 ` Enrico Weigelt, metux IT consult [this message]
2019-04-02 6:26 ` Lee Jones
[not found] ` <HK2PR03MB45295EF1E814CE40BDFB798995560@HK2PR03MB4529.apcprd03.prod.outlook.com>
2019-04-03 3:40 ` Lee Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bf8659eb-ecf8-a9e4-33f1-c9bbb7f5ee13@metux.net \
--to=lkml@metux.net \
--cc=lee.jones@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=morris_ku@sunix.com \
--cc=saumah@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).