lkml.org 
[lkml]   [2013]   [Dec]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCHv7 1/4] pwm: Add Freescale FTM PWM driver support
    On Tue, Dec 17, 2013 at 12:10:22PM +0100, Thierry Reding wrote:
    > On Fri, Dec 13, 2013 at 04:57:04PM +0800, Xiubo Li wrote:
    > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
    > > + const void __iomem *addr)
    > > +{
    > > + u32 val;
    > > +
    > > + val = __raw_readl(addr);
    > > +
    > > + if (likely(fpc->big_endian))
    >
    > The likely() probably isn't very useful in this case. But if you want to
    > keep it, it should at least be reversed, since little-endian is actually
    > the default (you have to specify the big-endian property to activate the
    > big endian mode).
    >
    > > + val = be32_to_cpu(val);
    > > + else
    > > + val = le32_to_cpu(val);

    This will also cause sparse errors, because when sparse is enabled, these
    expect __le32 or __be32 arguments, not u32.

    > > + rmb();
    >
    > I'd prefer the rmb() to follow the __raw_readl() immediately to make the
    > relationship more explicit.

    A better question to ask is: why is this barrier here? What memory
    ordering operations is it trying to serialise?

    > > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc,
    > > + u32 val, void __iomem *addr)
    > > +{
    > > + wmb();
    > > + if (likely(fpc->big_endian))
    > > + val = cpu_to_be32(val);
    > > + else
    > > + val = cpu_to_le32(val);
    > > +
    > > + __raw_writel(val, addr);
    >
    > Same here. wmb() should precede __raw_writel() immediately.

    Same comments here - what memory operations is the wmb() trying to
    serialise? Does this PWM driver somehow end up doing DMA?


    \
     
     \ /
      Last update: 2013-12-17 13:21    [W:2.632 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site