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

Magnus Damm magnus.damm at gmail.com
Wed Aug 19 15:59:56 CEST 2009

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>
> ---


> --- /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
> +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


/ magnus

More information about the Alsa-devel mailing list