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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2576 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]