Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 11 Mar 2022 06:45:55 +0100
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)
Message-ID: <20220311054555.GA16362@lst.de>
References: <20220310212812.13944-1-linux@zary.sk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220310212812.13944-1-linux@zary.sk>
User-Agent: Mutt/1.5.17 (2007-11-01)
Precedence: bulk

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.