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.
