On Thu, Dec 12, 2013 at 02:43:14AM +0000, Li.Xiubo@xxxxxxxxxxxxx wrote:
> Hi Mark,
>
>
> > > > > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
> > > > > + const void __iomem *addr)
> > > > > +{
> > > > > + if (likely(fpc->big_endian))
> > > > > + return ioread32be(addr);
> > > > > + else
> > > > > + return readl(addr);
> > > > > +}
> > >
> > > It looks a little odd to to have two different accessors here.
> > >
> > > Could these not be unified somehow?
> > >
> >
> > How about the following :
> >
> > +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
> > + const void __iomem *addr)
> > +{
> > + u32 val;
> > +
> > + if (likely(fpc->big_endian))
> > + val = be32_to_cpu(__raw_readl(addr));
> > + else
> > + val = le32_to_cpu(__raw_readl(addr));
> > +
> > + rmb();
> > +
> > + return val;
> > +}
> > +
> > +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc,
> > + u32 val, void __iomem *addr)
> > +{
> > + wmb();
> > +
> > + if (likely(fpc->big_endian))
> > + __raw_writel(cpu_to_be32(val), addr);
> > + else
> > + __raw_writel(cpu_to_le32(val), addr);
> > +}
> > +
> >
> >
>
> Or, will these be much better ?
> +++++++++++
> +static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
> + const void __iomem *addr)
> +{
> + u32 val;
> +
> + if (likely(fpc->big_endian))
> + val = be32_to_cpu((__force __be32)__raw_readl(addr));
> + else
> + val = le32_to_cpu((__force __le32)__raw_readl(addr));
> +
> + rmb();
> +
> + return val;
> +}
> +
> +static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc,
> + u32 val, void __iomem *addr)
> +{
> + wmb();
> +
> + if (likely(fpc->big_endian))
> + __raw_writel((__force u32)cpu_to_be32(val), addr);
> + else
> + __raw_writel((__force u32)cpu_to_le32(val), addr); }
> +
> -----------
I think perhaps what Mark may have meant was something like this:
static inline u32 fsl_pwm_readl(struct fsl_pwm_chip *fpc,
const void __iomem *addr)
{
u32 value = readl(addr);
if (likely(fpc->big_endian))
value = be32_to_cpu(value);
else
value = le32_to_cpu(value);
return value;
}
static inline void fsl_pwm_writel(struct fsl_pwm_chip *fpc, u32 value,
const void __iomem *addr)
{
if (likely(fpc->big_endian))
value = cpu_to_be32(value);
else
value = cpu_to_le32(value);
writel(value, addr);
}
That way you call the accessors only once, and do the conversion after
or before that.
Thierry
Attachment:
pgptte9cbpJxr.pgp
Description: PGP signature
