[alsa-devel] [PATCH 1/3] Add SuperH FSI driver support for ALSA

[Magnus Damm]

Hi Morimoto-san,

Thanks for your work on this!

On Wed, Aug 19, 2009 at 8:25 PM, Kuninori
Morimoto<morimoto.kuninori at renesas.com> wrote:
> This driver is very simple.
> It support playback only now.
> It is tested by ms7724se board.
>
> Signed-off-by: Kuninori Morimoto <morimoto.kuninori at renesas.com>
> ---

[snip]

> --- /dev/null
> +++ b/include/sound/fsi.h

How about using sh_fsi.h or sh_mobile_fsi.h? I think
include/sound/fsi.h is polluting the name space.

> --- /dev/null
> +++ b/sound/soc/sh/fsi.c
[snip]
> +struct fsi_master {
> +       void __iomem *base;
> +       atomic_t user;
> +       int irq;
> +       struct clk *clk;
> +       struct fsi_priv fsia;
> +       struct fsi_priv fsib;
> +       struct sh_fsi_platform_info *info;
> +};
> +
> +struct fsi_master *master;

Is it really necessary to have "master" as a global variable? Maybe
this global variable is there to work around some framework issue?

If possible please try to avoid using a global variable like this.
Future processors may come with multiple FSI blocks.

Right now I'm quite happy that the CEU driver does not use global
variables. Remember the case with a single CEU in sh7722 and now we
have two CEU blocks in sh7724. It was easy to support sh7724 because I
avoided using global variables when I initially wrote the driver for
sh7722.

Cheers,

/ magnus