Messages in this thread | | | From | Andy Shevchenko <> | Date | Sat, 25 Dec 2021 19:16:13 +0200 | Subject | Re: [PATCH v3 07/10] ata: pata_platform: Merge pata_of_platform into pata_platform |
| |
On Sat, Dec 25, 2021 at 3:56 AM Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > Merge the OF pata_of_platform driver into pata_platform.
For the further improvements...
...
> +MODULE_PARM_DESC(pio_mask, "PIO modes supported, mode 0 only by default (param valid only for non DT users)");
non-DT
...
> +/** > + * struct pata_platform_priv - Private info > + * @io_res: Resource representing I/O base > + * @ctl_res: Resource representing CTL base
> + * @irq_res: Resource representing IRQ and its flags
Why do we need to keep entire resource for just one value?
> + * @ioport_shift: I/O port shift > + * @pio_mask: PIO mask > + * @use16bit: Flag to indicate 16-bit IO instead of 32-bit > + */
...
> ata_port_desc(ap, "%s cmd 0x%llx ctl 0x%llx", mmio ? "mmio" : "ioport", > - (unsigned long long)io_res->start, > - (unsigned long long)ctl_res->start); > + (unsigned long long)priv->io_res->start, > + (unsigned long long)priv->ctl_res->start);
Using castings here is not fully correct. Instead just use %pR/%pR or at least %pa.
...
> irq = platform_get_irq_optional(pdev, 0); > if (irq < 0 && irq != -ENXIO) > return irq; > +
Stray change?
> if (irq > 0) { > - memset(&irq_res, 0x0, sizeof(struct resource)); > - irq_res.start = irq; > + struct resource *irq_res; > + > + irq_res = devm_kzalloc(&pdev->dev, sizeof(*irq_res), GFP_KERNEL); > + if (!irq_res) > + return -ENOMEM; > + > + irq_res->start = irq; > + priv->irq_res = irq_res; > }
...
> + ret = of_property_read_u32(dn, "pio-mode", &pio_mode); > + if (!ret) { > + if (pio_mode > 6) {
> + dev_err(&ofdev->dev, "invalid pio-mode\n"); > + return -EINVAL;
return dev_err_probe(...); ?
> + } > + } else { > + dev_info(&ofdev->dev, "pio-mode unspecified, assuming PIO0\n"); > + }
...
> + priv->pio_mask = 1 << pio_mode; > + priv->pio_mask |= (1 << pio_mode) - 1;
So, pio_mode defines the MSB in the mask, why not to use
->pio_mask = GENMASK(pio_mode, 0);
?
...
> + if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) { > + dev_err(&pdev->dev, "invalid number of resources\n"); > + return -EINVAL;
return dev_err_probe(); ?
> + }
...
> + if (!dev_of_node(&pdev->dev)) > + ret = pata_platform_get_pdata(pdev, priv); > + else > + ret = pata_of_platform_get_pdata(pdev, priv);
What the difference between them? Can't you unify them and leave only DT related part separately?
...
> +static const struct of_device_id pata_of_platform_match[] = { > + { .compatible = "ata-generic", },
> + { },
No comma for terminator line.
> +};
-- With Best Regards, Andy Shevchenko
| |