Messages in this thread |  | | From | Andy Shevchenko <> | Date | Tue, 14 Jun 2022 13:19:13 +0200 | Subject | Re: [PATCH v5 13/15] mmc: sdhci-cadence: Add AMD Pensando Elba SoC support |
| |
On Mon, Jun 13, 2022 at 9:57 PM Brad Larson <brad@pensando.io> wrote: > > From: Brad Larson <blarson@amd.com> > > Add support for AMD Pensando Elba SoC which explicitly controls > byte-lane enables on writes. Add priv_write_l() which is
enabling ?
> used on Elba platforms for byte-lane control. > > Select MMC_SDHCI_IO_ACCESSORS for MMC_SDHCI_CADENCE which > allows Elba SoC sdhci_elba_ops to overwrite the SDHCI > IO memory accessors.
...
> + void (*priv_write_l)(struct sdhci_cdns_priv *priv, u32 val,
priv_writel
> + void __iomem *reg);
And perhaps leave it on one line.
I also would swap parameters, so address goes first followed by value.
...
> +static inline void sdhci_cdns_priv_writel(struct sdhci_cdns_priv *priv, > + u32 val, void __iomem *reg) > +{
> + if (unlikely(priv->priv_write_l))
First of all, why if (unlikely())-else instead of if (likely())-else?
> + priv->priv_write_l(priv, val, reg); > + else > + writel(val, reg); > +}
Instead of branching each time you do I/O, make sure that callback is always set and call it unconditionally. In this case you don't need to have this callback, but maybe just a wrapper on `writel()`. As a result you may split this to two patches in the first of which you simply introduce a callback and a writel() wrapper which is assigned unconditionally to all current chips. In the next you add a new chip support.
...
> + u32 m = (reg & 0x3); > + u32 msk = (0x3 << (m)); > + > + spin_lock_irqsave(&priv->wrlock, flags); > + writel(msk << 3, priv->ctl_addr); > + writew(val, host->ioaddr + reg); > + spin_unlock_irqrestore(&priv->wrlock, flags);
Too many 3:s as magic. Is it GENMASK() or something else? Perhaps it needs a definition.
...
> + u32 m = (reg & 0x3); > + u32 msk = (0x1 << (m)); > + > + spin_lock_irqsave(&priv->wrlock, flags); > + writel(msk << 3, priv->ctl_addr); > + writeb(val, host->ioaddr + reg); > + spin_unlock_irqrestore(&priv->wrlock, flags);
Ditto.
...
> + writel(0x78, priv->ctl_addr);
Magic.
...
> +static const struct sdhci_cdns_drv_data sdhci_cdns_drv_data = { > + .pltfm_data = { > + .ops = &sdhci_cdns_ops, > + }, > +}; > + > +
One blank line is enough.
...
> + { > + .compatible = "amd,pensando-elba-sd4hc", > + .data = &sdhci_elba_drv_data
Leave a comma here.
> },
-- With Best Regards, Andy Shevchenko
|  |