Hi Andy,
Thank you for the review.
On Sat, Dec 25, 2021 at 5:16 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Sat, Dec 25, 2021 at 3:56 AM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> 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
>
OK.
> ...
>
> > +/**
> > + * 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?
>
Agreed can be dropped.
> > + * @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.
>
Ok will use %pa.
> ...
>
> > irq = platform_get_irq_optional(pdev, 0);
> > if (irq < 0 && irq != -ENXIO)
> > return irq;
> > +
>
> Stray change?
>
My bad.
> > 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(...); ?
>
Is it just to reduce the lines?
> > + }
> > + } 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);
>
> ?
>
patch 10/10 doesn this.
> ...
>
> > + if ((pdev->num_resources != 3) && (pdev->num_resources != 2)) {
> > + dev_err(&pdev->dev, "invalid number of resources\n");
> > + return -EINVAL;
>
> return dev_err_probe(); ?
>
This is the old code, later patch drops this chunk anyway.
> > + }
>
> ...
>
> > + 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?
>
pata_of_platform_get_pdata() basically reads OF data, and there is a
function which is already shared by both the functions.
> ...
>
> > +static const struct of_device_id pata_of_platform_match[] = {
> > + { .compatible = "ata-generic", },
>
> > + { },
>
> No comma for terminator line.
>
OK, I will drop it.
Cheers,
Prabhakar
