Re: [RFCv2 endianess 1/4] pwm: Add Freescale FTM PWM driver support

[Thierry Reding]

On Thu, Dec 12, 2013 at 02:43:14AM +0000, li.xi...@freescale.com 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

pgpOBD5gckjwQ.pgp
Description: PGP signature