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 ... > +/** > + * 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