From: Christoph Hellwig <hch@lst.de>
To: Ondrej Zary <linux@zary.sk>
Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com>,
Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>,
Tim Waugh <tim@cyberelk.net>,
linux-block@vger.kernel.org, linux-parport@lists.infradead.org,
linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v0] pata_parport: add driver (PARIDE replacement)
Date: Fri, 11 Mar 2022 06:45:55 +0100 [thread overview]
Message-ID: <20220311054555.GA16362@lst.de> (raw)
In-Reply-To: <20220310212812.13944-1-linux@zary.sk>
On Thu, Mar 10, 2022 at 10:28:12PM +0100, Ondrej Zary wrote:
> Add pata_parport (PARIDE replacement) core libata driver.
>
> The original paride protocol modules are used for now so allow them to
> be compiled without old PARIDE core.
I agree with Damien that this needs a bit more text here. Explaining
what kind of hardware this drives, that this will allow to eventually
drop paride, how it reuesed the low-level drivers, etc.
> + If your parallel port support is in a loadable module, you must build
> + PATA_PARPORT as a module. If you built PATA_PARPORT support into your
> + kernel, you may still build the individual protocol modules
> + as loadable modules.
I'd drop the above. The dependencies are already enforced by Kconfig
and we don't really tend to mention this elsewhere.
> + Unlike the old PARIDE, there are no high-level drivers needed.
> + The IDE devices behind parallel port adapters are handled by the
> + ATA layer.
I also don't think this is needed.
> index 000000000000..3ea8d824091e
> --- /dev/null
> +++ b/drivers/ata/parport/pata_parport.c
> @@ -0,0 +1,805 @@
> +// SPDX-License-Identifier: GPL-2.0-only
Please add your copyright statement here.
> +static void pata_parport_tf_load(struct ata_port *ap, const struct ata_taskfile *tf)
Overly long line.
> + pi->proto->write_regr(pi, 0, ATA_REG_NSECT, tf->hob_nsect);
> + pi->proto->write_regr(pi, 0, ATA_REG_LBAL, tf->hob_lbal);
> + pi->proto->write_regr(pi, 0, ATA_REG_LBAM, tf->hob_lbam);
> + pi->proto->write_regr(pi, 0, ATA_REG_LBAH, tf->hob_lbah);
Same here.
> +static void pata_parport_exec_command(struct ata_port *ap, const struct ata_taskfile *tf)
.. and here.
And a bunch more.
> +static void pata_parport_bus_release(struct device *dev)
> +{
> + /* nothing to do here but required to avoid warning on device removal */
> +}
> +
> +static struct bus_type pata_parport_bus_type = {
> + .name = DRV_NAME,
> +};
> +
> +static struct device pata_parport_bus = {
> + .init_name = DRV_NAME,
> + .release = pata_parport_bus_release,
> +};
> +
> +/* temporary for old paride protocol modules */
> +static struct scsi_host_template pata_parport_sht = {
> + PATA_PARPORT_SHT("pata_parport")
> +};
Did you look into my suggestion to use struct pardevice.dev instead?
> index ddb9e589da7f..f3bd01a9c9ec 100644
> --- a/drivers/block/paride/paride.h
> +++ b/drivers/block/paride/paride.h
> @@ -1,3 +1,7 @@
> +#if IS_ENABLED(CONFIG_PATA_PARPORT)
> +#include "../../ata/parport/pata_parport.h"
> +
> +#else
Maybe add a comment here? Also this is a pretty clear indication
that pata_parport.h should be in include/linux/ at least for now.
prev parent reply other threads:[~2022-03-11 5:46 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-10 21:28 [PATCH v0] pata_parport: add driver (PARIDE replacement) Ondrej Zary
2022-03-10 23:59 ` Damien Le Moal
2022-03-11 18:55 ` Ondrej Zary
2022-03-12 8:09 ` Damien Le Moal
2022-03-12 11:21 ` Ondrej Zary
2022-03-15 8:23 ` Christoph Hellwig
2022-03-11 5:45 ` Christoph Hellwig [this message]
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=20220311054555.GA16362@lst.de \
--to=hch@lst.de \
--cc=axboe@kernel.dk \
--cc=damien.lemoal@opensource.wdc.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-ide@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-parport@lists.infradead.org \
--cc=linux@zary.sk \
--cc=tim@cyberelk.net \
/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).