lkml.org 
[lkml]   [2014]   [Jan]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCHv8 RFC] pwm: Add Freescale FTM PWM driver support
Date

> > +static unsigned long fsl_pwm_calculate_period_cycles(struct fsl_pwm_chip
> *fpc,
> > + unsigned long period_ns,
> > + enum fsl_pwm_clk index)
> > +{
> > + bool bg = fpc->big_endian;
> > + int ret;
> > +
> > + fpc->counter_clk_select = FTM_SC_CLK(bg, index);
>
> Yes, this is the spirit of what I was suggesting. The code is much less
> efficient/bigger on the Vybrid with this run-time detection; but this is
> more efficient/smaller than previous versions. I think that 'bg' can be
> a compiler '#define' base on the configured SOC-systems. Ie, if the
> kernel config only has 'Vybrid' or only 'LayerScape', then 'bg' can be a
> hard coded value. The compiler will produce much better code in these
> cases.
>
> Also, maybe 'distro' people may want to make a 'hand-held' (Debian) or a
> 'router' (OpenWRT) distribution and they would only pick either 'Vybrid'
> or 'LayerScape'. However, if someone wants an 'every ARM under the
> sun', then the code still works. So, I think that the code is better
> setup for a subsequent patch set like this (or at least just a good).
>
> Especially, the stuff on the I/O swapping in the 'readl()' and
> 'writel()' is no longer needed; I think you can use the same function
> for both SOCs.
>
> > +#define __FTM_SWAP32(v) ((u32)(\
> > + (((u32)(v) & (u32)0x000000ffUL) << 24) |\
> > + (((u32)(v) & (u32)0x0000ff00UL) << 8) |\
> > + (((u32)(v) & (u32)0x00ff0000UL) >> 8) |\
> > + (((u32)(v) & (u32)0xff000000UL) >> 24)))
> > +#define FTM_SWAP32(b, v) (b ? __FTM_SWAP32(v) : v)
>
> I think that there are macros that you could use here. For instance,
> '#include <linux/swab.h>' (powerpc and arm) has some assembler macros
> that are quite fast for swapping. If the kernel config has ARCH >= 6
> for ARM, then the very fast 'rev' instruction is used. If not, then a
> generic version is used as you have coded. The PowerPC (another
> possible future ARCH for QorIQ/Layerscape SOC?) always has inline
> assembler macros.
>
> So,
>
> + #include <linux/swab.h>
> ...
> + #define FTM_SWAP32(b, v) (b ? __swab32(v) : v)
>
> might be better.
>

Yes.

I have removed the big-endian support temporarily, and will send followed
patches about this.


Thanks,

--
Best Regards,
Xiubo



\
 
 \ /
  Last update: 2014-01-09 09:41    [W:0.385 / U:0.460 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site