Re: [PATCH 2/3 v2] ASoC: dwc: Add I2S HDMI audio support

2016-03-31 Thread Mark Brown 
On Thu, Mar 31, 2016 at 10:37:20AM +0100, Jose Abreu wrote:
> On 29-03-2016 19:22, Mark Brown wrote:

> > If you want to add a new platform driver you need to add a new platform
> > driver, not shove the code into an existing driver for a seperate IP.

> I can separate the platform driver into a new file but they will have to be
> compiled into the same module as the new additions to the i2s driver depend on
> functions of the platform driver (see i2s_irq_handler()). Or should I divide

No, that's not at all acceptable.  The Designware IP is not specific to
your system, you can't make it depend on your platform driver.  The
kernel needs to work on other people's systems too.  You need to work
through and/or extend the abstractions the framework provides to
separate the drivers for different IPs.

> this into two modules and add a Kconfig option to the platform driver? Besides
> this I first wanted the driver to be compiled into the same module so that it 
> is
> compatible with kernel 3.18 where simple audio card requires that platform
> driver == cpu driver.

That's not OK upstream, we're working on the current kernel not on
random old kernels.  We don't carry compatibility code to enable current
kernel code to be run on years old kernels.


signature.asc
Description: PGP signature

Re: [PATCH 2/3 v2] ASoC: dwc: Add I2S HDMI audio support

2016-03-31 Thread Mark Brown 
On Thu, Mar 31, 2016 at 10:37:20AM +0100, Jose Abreu wrote:
> On 29-03-2016 19:22, Mark Brown wrote:

> > If you want to add a new platform driver you need to add a new platform
> > driver, not shove the code into an existing driver for a seperate IP.

> I can separate the platform driver into a new file but they will have to be
> compiled into the same module as the new additions to the i2s driver depend on
> functions of the platform driver (see i2s_irq_handler()). Or should I divide

No, that's not at all acceptable.  The Designware IP is not specific to
your system, you can't make it depend on your platform driver.  The
kernel needs to work on other people's systems too.  You need to work
through and/or extend the abstractions the framework provides to
separate the drivers for different IPs.

> this into two modules and add a Kconfig option to the platform driver? Besides
> this I first wanted the driver to be compiled into the same module so that it 
> is
> compatible with kernel 3.18 where simple audio card requires that platform
> driver == cpu driver.

That's not OK upstream, we're working on the current kernel not on
random old kernels.  We don't carry compatibility code to enable current
kernel code to be run on years old kernels.


signature.asc
Description: PGP signature

Re: [PATCH 2/3 v2] ASoC: dwc: Add I2S HDMI audio support

2016-03-31 Thread Jose Abreu 
Hi Mark,

On 29-03-2016 19:22, Mark Brown wrote:
> On Tue, Mar 29, 2016 at 07:03:01PM +0100, Jose Abreu wrote:
>
>> The major part of this patch is the adding of an ALSA platform driver so that
>> audio comes out of the box in AXS boards but we also added functionalities to
>> the i2s driver and performed one bug fix related with the mask/unmask of
>> interrupts. I will split the patches but they will depend on each other.
> If you want to add a new platform driver you need to add a new platform
> driver, not shove the code into an existing driver for a seperate IP.
>

I can separate the platform driver into a new file but they will have to be
compiled into the same module as the new additions to the i2s driver depend on
functions of the platform driver (see i2s_irq_handler()). Or should I divide
this into two modules and add a Kconfig option to the platform driver? Besides
this I first wanted the driver to be compiled into the same module so that it is
compatible with kernel 3.18 where simple audio card requires that platform
driver == cpu driver.

Best regards,
Jose Miguel Abreu

Re: [PATCH 2/3 v2] ASoC: dwc: Add I2S HDMI audio support

2016-03-31 Thread Jose Abreu 
Hi Mark,

On 29-03-2016 19:22, Mark Brown wrote:
> On Tue, Mar 29, 2016 at 07:03:01PM +0100, Jose Abreu wrote:
>
>> The major part of this patch is the adding of an ALSA platform driver so that
>> audio comes out of the box in AXS boards but we also added functionalities to
>> the i2s driver and performed one bug fix related with the mask/unmask of
>> interrupts. I will split the patches but they will depend on each other.
> If you want to add a new platform driver you need to add a new platform
> driver, not shove the code into an existing driver for a seperate IP.
>

I can separate the platform driver into a new file but they will have to be
compiled into the same module as the new additions to the i2s driver depend on
functions of the platform driver (see i2s_irq_handler()). Or should I divide
this into two modules and add a Kconfig option to the platform driver? Besides
this I first wanted the driver to be compiled into the same module so that it is
compatible with kernel 3.18 where simple audio card requires that platform
driver == cpu driver.

Best regards,
Jose Miguel Abreu

Re: [PATCH 2/3 v2] ASoC: dwc: Add I2S HDMI audio support

2016-03-29 Thread Mark Brown 
On Tue, Mar 29, 2016 at 07:03:01PM +0100, Jose Abreu wrote:

> The major part of this patch is the adding of an ALSA platform driver so that
> audio comes out of the box in AXS boards but we also added functionalities to
> the i2s driver and performed one bug fix related with the mask/unmask of
> interrupts. I will split the patches but they will depend on each other.

If you want to add a new platform driver you need to add a new platform
driver, not shove the code into an existing driver for a seperate IP.


signature.asc
Description: PGP signature

Re: [PATCH 2/3 v2] ASoC: dwc: Add I2S HDMI audio support

2016-03-29 Thread Mark Brown 
On Tue, Mar 29, 2016 at 07:03:01PM +0100, Jose Abreu wrote:

> The major part of this patch is the adding of an ALSA platform driver so that
> audio comes out of the box in AXS boards but we also added functionalities to
> the i2s driver and performed one bug fix related with the mask/unmask of
> interrupts. I will split the patches but they will depend on each other.

If you want to add a new platform driver you need to add a new platform
driver, not shove the code into an existing driver for a seperate IP.


signature.asc
Description: PGP signature

Re: [PATCH 2/3 v2] ASoC: dwc: Add I2S HDMI audio support

2016-03-29 Thread Mark Brown 
On Mon, Mar 28, 2016 at 03:36:10PM +0100, Jose Abreu wrote:
> HDMI audio support was added to the AXS board using an
> I2S cpu driver and a custom platform driver.
> 
> The platform driver supports two channels @ 16 bits with
> rates 32k, 44.1k and 48k. ALSA Simple audio card is used to
> glue the cpu, platform and codec driver (adv7511).

>  sound/soc/dwc/Kconfig  |   1 +
>  sound/soc/dwc/designware_i2s.c | 385 
> +++--

Your changelog appears to describe the writing of a machine driver but
this is a large patch adding code to an I2S controller driver.  This
means I can't review your patch since I can't tell what it is supposed
to do.  If you've added functionality to this driver you need to send
one or more patches each of which adds a single feature to the driver
together with a changelog which describes what that feature is.

Glancing at the patch I'm not 100% sure that the features you're adding
are part of the Synopsis device but I'm not entirely sure.

>  2 files changed, 373 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/soc/dwc/Kconfig b/sound/soc/dwc/Kconfig
> index d50e085..bc3fae7 100644
> --- a/sound/soc/dwc/Kconfig
> +++ b/sound/soc/dwc/Kconfig
> @@ -2,6 +2,7 @@ config SND_DESIGNWARE_I2S
>   tristate "Synopsys I2S Device Driver"
>   depends on CLKDEV_LOOKUP
>   select SND_SOC_GENERIC_DMAENGINE_PCM
> + select SND_SIMPLE_CARD

No, this doesn't make sense - the fact that someone has used a Synopsis
I2S controller doesn't mean that they have a system which uses
simple-card.  If the user wants to use simple-card they need to enable
it separately, this is the same pattern we follow for all CPU controller
drivers.


signature.asc
Description: PGP signature

Re: [PATCH 2/3 v2] ASoC: dwc: Add I2S HDMI audio support

2016-03-29 Thread Mark Brown 
On Mon, Mar 28, 2016 at 03:36:10PM +0100, Jose Abreu wrote:
> HDMI audio support was added to the AXS board using an
> I2S cpu driver and a custom platform driver.
> 
> The platform driver supports two channels @ 16 bits with
> rates 32k, 44.1k and 48k. ALSA Simple audio card is used to
> glue the cpu, platform and codec driver (adv7511).

>  sound/soc/dwc/Kconfig  |   1 +
>  sound/soc/dwc/designware_i2s.c | 385 
> +++--

Your changelog appears to describe the writing of a machine driver but
this is a large patch adding code to an I2S controller driver.  This
means I can't review your patch since I can't tell what it is supposed
to do.  If you've added functionality to this driver you need to send
one or more patches each of which adds a single feature to the driver
together with a changelog which describes what that feature is.

Glancing at the patch I'm not 100% sure that the features you're adding
are part of the Synopsis device but I'm not entirely sure.

>  2 files changed, 373 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/soc/dwc/Kconfig b/sound/soc/dwc/Kconfig
> index d50e085..bc3fae7 100644
> --- a/sound/soc/dwc/Kconfig
> +++ b/sound/soc/dwc/Kconfig
> @@ -2,6 +2,7 @@ config SND_DESIGNWARE_I2S
>   tristate "Synopsys I2S Device Driver"
>   depends on CLKDEV_LOOKUP
>   select SND_SOC_GENERIC_DMAENGINE_PCM
> + select SND_SIMPLE_CARD

No, this doesn't make sense - the fact that someone has used a Synopsis
I2S controller doesn't mean that they have a system which uses
simple-card.  If the user wants to use simple-card they need to enable
it separately, this is the same pattern we follow for all CPU controller
drivers.


signature.asc
Description: PGP signature

Re: [PATCH 2/3 v2] ASoC: dwc: Add I2S HDMI audio support

2016-03-28 Thread Jose Abreu 
Hi Alexey,

On 28-03-2016 16:35, Alexey Brodkin wrote:
> Hi Jose,
>
> On Mon, 2016-03-28 at 15:36 +0100, Jose Abreu wrote:
>> HDMI audio support was added to the AXS board using an
>> I2S cpu driver and a custom platform driver.
>>
>> The platform driver supports two channels @ 16 bits with
>> rates 32k, 44.1k and 48k. ALSA Simple audio card is used to
>> glue the cpu, platform and codec driver (adv7511).
>>
>> Signed-off-by: Jose Abreu 
>> ---
>>
>> No changes v1 -> v2.
>>
>>  sound/soc/dwc/Kconfig  |   1 +
>>  sound/soc/dwc/designware_i2s.c | 385 
>> +++--
>>  2 files changed, 373 insertions(+), 13 deletions(-)
>>
>> diff --git a/sound/soc/dwc/Kconfig b/sound/soc/dwc/Kconfig
>> index d50e085..bc3fae7 100644
>> --- a/sound/soc/dwc/Kconfig
>> +++ b/sound/soc/dwc/Kconfig
>> @@ -2,6 +2,7 @@ config SND_DESIGNWARE_I2S
>>  tristate "Synopsys I2S Device Driver"
>>  depends on CLKDEV_LOOKUP
>>  select SND_SOC_GENERIC_DMAENGINE_PCM
>> +select SND_SIMPLE_CARD
>>  help
>>   Say Y or M if you want to add support for I2S driver for
>>   Synopsys desigwnware I2S device. The device supports upto
>> diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
>> index bff258d..0f2f588 100644
>> --- a/sound/soc/dwc/designware_i2s.c
>> +++ b/sound/soc/dwc/designware_i2s.c
>> @@ -84,11 +84,37 @@
>>  #define MAX_CHANNEL_NUM 8
>>  #define MIN_CHANNEL_NUM 2
>>  
>> +/* FPGA Version Info */
>> +#define FPGA_VER_INFO   0xE0011230
>> +#define FPGA_VER_27M0x000FBED9
>> +
>> +/* PLL registers addresses */
>> +#define PLL_IDIV_ADDR   0xE00100A0
>> +#define PLL_FBDIV_ADDR  0xE00100A4
>> +#define PLL_ODIV0_ADDR  0xE00100A8
>> +#define PLL_ODIV1_ADDR  0xE00100AC
> Well I think all is not acceptable.
> See all these FPGA_VER_xxx as well as PLL_xxx
> are strictly ARC SDP specific things and have nothing to do with generic 
> driver.
>
> That's so pity we don't have a driver for all clocks/PLLs on ARC SDP yet.
> So as of now I may only propose to use hard-coded fixed clocks as I did with
> ARC PGU, see "pguclk" here:
> http://lists.infradead.org/pipermail/linux-snps-arc/2016-March/000790.html
>
> Again I'll try to implement missing clock driver sometime soon because
> more and more stuff requires it but for now let's use a work-around.
Yes, this is a workaround that we are using so that the driver works in ARC SDP
platforms. The driver still has the functionality to operate using a clock
driver (it must be declared in device tree) but if the clock handle is not
declared the driver will assume that must use the internal PLL config options.
This is currently the only option to make it work in ARC SDP.

I will send a v3 soon without this workaround and when the missing clock drivers
are implemented I will re-test this.
>> +struct dw_i2s_pll {
>> +unsigned int rate;
>> +unsigned int data_width;
>> +unsigned int idiv;
>> +unsigned int fbdiv;
>> +unsigned int odiv0;
>> +unsigned int odiv1;
>> +};
>> +
>> +static const struct dw_i2s_pll dw_i2s_pll_cfg_27m[] = {
>> +/* 27Mhz */
>> +{ 32000, 16, 0x104, 0x451, 0x10E38, 0x2000 },
>> +{ 44100, 16, 0x104, 0x596, 0x10D35, 0x2000 },
>> +{ 48000, 16, 0x208, 0xA28, 0x10B2C, 0x2000 },
>> +{ 0, 0, 0, 0, 0, 0 },
>>  };
>>  
>> +static const struct dw_i2s_pll dw_i2s_pll_cfg_28m[] = {
>> +/* 28.224Mhz */
>> +{ 32000, 16, 0x82, 0x105, 0x107DF, 0x2000 },
>> +{ 44100, 16, 0x28A, 0x1, 0x10001, 0x2000 },
>> +{ 48000, 16, 0xA28, 0x187, 0x10042, 0x2000 },
>> +{ 0, 0, 0, 0, 0, 0 },
>> +};
> These 2 hunks as well should go in ARC SDP clocks.
>
>> +static int i2s_pll_cfg(struct i2s_clk_config_data *config)
>> +{
>> +const struct dw_i2s_pll *pll_cfg;
>> +u32 rate = config->sample_rate;
>> +u32 data_width = config->data_width;
>> +int i;
>> +
>> +if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M)
>> +pll_cfg = dw_i2s_pll_cfg_27m;
>> +else
>> +pll_cfg = dw_i2s_pll_cfg_28m;
>> +
>> +for (i = 0; pll_cfg[i].rate != 0; i++) {
>> +if ((pll_cfg[i].rate == rate) &&
>> +(pll_cfg[i].data_width == data_width)) {
>> +writel(pll_cfg[i].idiv, (void *)PLL_IDIV_ADDR);
>> +writel(pll_cfg[i].fbdiv, (void *)PLL_FBDIV_ADDR);
>> +writel(pll_cfg[i].odiv0, (void *)PLL_ODIV0_ADDR);
>> +writel(pll_cfg[i].odiv1, (void *)PLL_ODIV1_ADDR);
>> +return 0;
>> +}
>> +}
>> +
>> +return -EINVAL;
>> +}
> Ditto.
>
> -Alexey

Best regards,
Jose Miguel Abreu

Re: [PATCH 2/3 v2] ASoC: dwc: Add I2S HDMI audio support

2016-03-28 Thread Jose Abreu 
Hi Alexey,

On 28-03-2016 16:35, Alexey Brodkin wrote:
> Hi Jose,
>
> On Mon, 2016-03-28 at 15:36 +0100, Jose Abreu wrote:
>> HDMI audio support was added to the AXS board using an
>> I2S cpu driver and a custom platform driver.
>>
>> The platform driver supports two channels @ 16 bits with
>> rates 32k, 44.1k and 48k. ALSA Simple audio card is used to
>> glue the cpu, platform and codec driver (adv7511).
>>
>> Signed-off-by: Jose Abreu 
>> ---
>>
>> No changes v1 -> v2.
>>
>>  sound/soc/dwc/Kconfig  |   1 +
>>  sound/soc/dwc/designware_i2s.c | 385 
>> +++--
>>  2 files changed, 373 insertions(+), 13 deletions(-)
>>
>> diff --git a/sound/soc/dwc/Kconfig b/sound/soc/dwc/Kconfig
>> index d50e085..bc3fae7 100644
>> --- a/sound/soc/dwc/Kconfig
>> +++ b/sound/soc/dwc/Kconfig
>> @@ -2,6 +2,7 @@ config SND_DESIGNWARE_I2S
>>  tristate "Synopsys I2S Device Driver"
>>  depends on CLKDEV_LOOKUP
>>  select SND_SOC_GENERIC_DMAENGINE_PCM
>> +select SND_SIMPLE_CARD
>>  help
>>   Say Y or M if you want to add support for I2S driver for
>>   Synopsys desigwnware I2S device. The device supports upto
>> diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
>> index bff258d..0f2f588 100644
>> --- a/sound/soc/dwc/designware_i2s.c
>> +++ b/sound/soc/dwc/designware_i2s.c
>> @@ -84,11 +84,37 @@
>>  #define MAX_CHANNEL_NUM 8
>>  #define MIN_CHANNEL_NUM 2
>>  
>> +/* FPGA Version Info */
>> +#define FPGA_VER_INFO   0xE0011230
>> +#define FPGA_VER_27M0x000FBED9
>> +
>> +/* PLL registers addresses */
>> +#define PLL_IDIV_ADDR   0xE00100A0
>> +#define PLL_FBDIV_ADDR  0xE00100A4
>> +#define PLL_ODIV0_ADDR  0xE00100A8
>> +#define PLL_ODIV1_ADDR  0xE00100AC
> Well I think all is not acceptable.
> See all these FPGA_VER_xxx as well as PLL_xxx
> are strictly ARC SDP specific things and have nothing to do with generic 
> driver.
>
> That's so pity we don't have a driver for all clocks/PLLs on ARC SDP yet.
> So as of now I may only propose to use hard-coded fixed clocks as I did with
> ARC PGU, see "pguclk" here:
> http://lists.infradead.org/pipermail/linux-snps-arc/2016-March/000790.html
>
> Again I'll try to implement missing clock driver sometime soon because
> more and more stuff requires it but for now let's use a work-around.
Yes, this is a workaround that we are using so that the driver works in ARC SDP
platforms. The driver still has the functionality to operate using a clock
driver (it must be declared in device tree) but if the clock handle is not
declared the driver will assume that must use the internal PLL config options.
This is currently the only option to make it work in ARC SDP.

I will send a v3 soon without this workaround and when the missing clock drivers
are implemented I will re-test this.
>> +struct dw_i2s_pll {
>> +unsigned int rate;
>> +unsigned int data_width;
>> +unsigned int idiv;
>> +unsigned int fbdiv;
>> +unsigned int odiv0;
>> +unsigned int odiv1;
>> +};
>> +
>> +static const struct dw_i2s_pll dw_i2s_pll_cfg_27m[] = {
>> +/* 27Mhz */
>> +{ 32000, 16, 0x104, 0x451, 0x10E38, 0x2000 },
>> +{ 44100, 16, 0x104, 0x596, 0x10D35, 0x2000 },
>> +{ 48000, 16, 0x208, 0xA28, 0x10B2C, 0x2000 },
>> +{ 0, 0, 0, 0, 0, 0 },
>>  };
>>  
>> +static const struct dw_i2s_pll dw_i2s_pll_cfg_28m[] = {
>> +/* 28.224Mhz */
>> +{ 32000, 16, 0x82, 0x105, 0x107DF, 0x2000 },
>> +{ 44100, 16, 0x28A, 0x1, 0x10001, 0x2000 },
>> +{ 48000, 16, 0xA28, 0x187, 0x10042, 0x2000 },
>> +{ 0, 0, 0, 0, 0, 0 },
>> +};
> These 2 hunks as well should go in ARC SDP clocks.
>
>> +static int i2s_pll_cfg(struct i2s_clk_config_data *config)
>> +{
>> +const struct dw_i2s_pll *pll_cfg;
>> +u32 rate = config->sample_rate;
>> +u32 data_width = config->data_width;
>> +int i;
>> +
>> +if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M)
>> +pll_cfg = dw_i2s_pll_cfg_27m;
>> +else
>> +pll_cfg = dw_i2s_pll_cfg_28m;
>> +
>> +for (i = 0; pll_cfg[i].rate != 0; i++) {
>> +if ((pll_cfg[i].rate == rate) &&
>> +(pll_cfg[i].data_width == data_width)) {
>> +writel(pll_cfg[i].idiv, (void *)PLL_IDIV_ADDR);
>> +writel(pll_cfg[i].fbdiv, (void *)PLL_FBDIV_ADDR);
>> +writel(pll_cfg[i].odiv0, (void *)PLL_ODIV0_ADDR);
>> +writel(pll_cfg[i].odiv1, (void *)PLL_ODIV1_ADDR);
>> +return 0;
>> +}
>> +}
>> +
>> +return -EINVAL;
>> +}
> Ditto.
>
> -Alexey

Best regards,
Jose Miguel Abreu

Re: [PATCH 2/3 v2] ASoC: dwc: Add I2S HDMI audio support

2016-03-28 Thread Alexey Brodkin 
Hi Jose,

On Mon, 2016-03-28 at 15:36 +0100, Jose Abreu wrote:
> HDMI audio support was added to the AXS board using an
> I2S cpu driver and a custom platform driver.
> 
> The platform driver supports two channels @ 16 bits with
> rates 32k, 44.1k and 48k. ALSA Simple audio card is used to
> glue the cpu, platform and codec driver (adv7511).
> 
> Signed-off-by: Jose Abreu 
> ---
> 
> No changes v1 -> v2.
> 
>  sound/soc/dwc/Kconfig  |   1 +
>  sound/soc/dwc/designware_i2s.c | 385 
> +++--
>  2 files changed, 373 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/soc/dwc/Kconfig b/sound/soc/dwc/Kconfig
> index d50e085..bc3fae7 100644
> --- a/sound/soc/dwc/Kconfig
> +++ b/sound/soc/dwc/Kconfig
> @@ -2,6 +2,7 @@ config SND_DESIGNWARE_I2S
>   tristate "Synopsys I2S Device Driver"
>   depends on CLKDEV_LOOKUP
>   select SND_SOC_GENERIC_DMAENGINE_PCM
> + select SND_SIMPLE_CARD
>   help
>    Say Y or M if you want to add support for I2S driver for
>    Synopsys desigwnware I2S device. The device supports upto
> diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
> index bff258d..0f2f588 100644
> --- a/sound/soc/dwc/designware_i2s.c
> +++ b/sound/soc/dwc/designware_i2s.c
> @@ -84,11 +84,37 @@
>  #define MAX_CHANNEL_NUM  8
>  #define MIN_CHANNEL_NUM  2
>  
> +/* FPGA Version Info */
> +#define FPGA_VER_INFO0xE0011230
> +#define FPGA_VER_27M 0x000FBED9
> +
> +/* PLL registers addresses */
> +#define PLL_IDIV_ADDR0xE00100A0
> +#define PLL_FBDIV_ADDR   0xE00100A4
> +#define PLL_ODIV0_ADDR   0xE00100A8
> +#define PLL_ODIV1_ADDR   0xE00100AC

Well I think all is not acceptable.
See all these FPGA_VER_xxx as well as PLL_xxx
are strictly ARC SDP specific things and have nothing to do with generic driver.

That's so pity we don't have a driver for all clocks/PLLs on ARC SDP yet.
So as of now I may only propose to use hard-coded fixed clocks as I did with
ARC PGU, see "pguclk" here:
http://lists.infradead.org/pipermail/linux-snps-arc/2016-March/000790.html

Again I'll try to implement missing clock driver sometime soon because
more and more stuff requires it but for now let's use a work-around.

> +struct dw_i2s_pll {
> + unsigned int rate;
> + unsigned int data_width;
> + unsigned int idiv;
> + unsigned int fbdiv;
> + unsigned int odiv0;
> + unsigned int odiv1;
> +};
> +
> +static const struct dw_i2s_pll dw_i2s_pll_cfg_27m[] = {
> + /* 27Mhz */
> + { 32000, 16, 0x104, 0x451, 0x10E38, 0x2000 },
> + { 44100, 16, 0x104, 0x596, 0x10D35, 0x2000 },
> + { 48000, 16, 0x208, 0xA28, 0x10B2C, 0x2000 },
> + { 0, 0, 0, 0, 0, 0 },
>  };
>  
> +static const struct dw_i2s_pll dw_i2s_pll_cfg_28m[] = {
> + /* 28.224Mhz */
> + { 32000, 16, 0x82, 0x105, 0x107DF, 0x2000 },
> + { 44100, 16, 0x28A, 0x1, 0x10001, 0x2000 },
> + { 48000, 16, 0xA28, 0x187, 0x10042, 0x2000 },
> + { 0, 0, 0, 0, 0, 0 },
> +};

These 2 hunks as well should go in ARC SDP clocks.

> +static int i2s_pll_cfg(struct i2s_clk_config_data *config)
> +{
> + const struct dw_i2s_pll *pll_cfg;
> + u32 rate = config->sample_rate;
> + u32 data_width = config->data_width;
> + int i;
> +
> + if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M)
> + pll_cfg = dw_i2s_pll_cfg_27m;
> + else
> + pll_cfg = dw_i2s_pll_cfg_28m;
> +
> + for (i = 0; pll_cfg[i].rate != 0; i++) {
> + if ((pll_cfg[i].rate == rate) &&
> + (pll_cfg[i].data_width == data_width)) {
> + writel(pll_cfg[i].idiv, (void *)PLL_IDIV_ADDR);
> + writel(pll_cfg[i].fbdiv, (void *)PLL_FBDIV_ADDR);
> + writel(pll_cfg[i].odiv0, (void *)PLL_ODIV0_ADDR);
> + writel(pll_cfg[i].odiv1, (void *)PLL_ODIV1_ADDR);
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}

Ditto.

-Alexey

Re: [PATCH 2/3 v2] ASoC: dwc: Add I2S HDMI audio support

2016-03-28 Thread Alexey Brodkin 
Hi Jose,

On Mon, 2016-03-28 at 15:36 +0100, Jose Abreu wrote:
> HDMI audio support was added to the AXS board using an
> I2S cpu driver and a custom platform driver.
> 
> The platform driver supports two channels @ 16 bits with
> rates 32k, 44.1k and 48k. ALSA Simple audio card is used to
> glue the cpu, platform and codec driver (adv7511).
> 
> Signed-off-by: Jose Abreu 
> ---
> 
> No changes v1 -> v2.
> 
>  sound/soc/dwc/Kconfig  |   1 +
>  sound/soc/dwc/designware_i2s.c | 385 
> +++--
>  2 files changed, 373 insertions(+), 13 deletions(-)
> 
> diff --git a/sound/soc/dwc/Kconfig b/sound/soc/dwc/Kconfig
> index d50e085..bc3fae7 100644
> --- a/sound/soc/dwc/Kconfig
> +++ b/sound/soc/dwc/Kconfig
> @@ -2,6 +2,7 @@ config SND_DESIGNWARE_I2S
>   tristate "Synopsys I2S Device Driver"
>   depends on CLKDEV_LOOKUP
>   select SND_SOC_GENERIC_DMAENGINE_PCM
> + select SND_SIMPLE_CARD
>   help
>    Say Y or M if you want to add support for I2S driver for
>    Synopsys desigwnware I2S device. The device supports upto
> diff --git a/sound/soc/dwc/designware_i2s.c b/sound/soc/dwc/designware_i2s.c
> index bff258d..0f2f588 100644
> --- a/sound/soc/dwc/designware_i2s.c
> +++ b/sound/soc/dwc/designware_i2s.c
> @@ -84,11 +84,37 @@
>  #define MAX_CHANNEL_NUM  8
>  #define MIN_CHANNEL_NUM  2
>  
> +/* FPGA Version Info */
> +#define FPGA_VER_INFO0xE0011230
> +#define FPGA_VER_27M 0x000FBED9
> +
> +/* PLL registers addresses */
> +#define PLL_IDIV_ADDR0xE00100A0
> +#define PLL_FBDIV_ADDR   0xE00100A4
> +#define PLL_ODIV0_ADDR   0xE00100A8
> +#define PLL_ODIV1_ADDR   0xE00100AC

Well I think all is not acceptable.
See all these FPGA_VER_xxx as well as PLL_xxx
are strictly ARC SDP specific things and have nothing to do with generic driver.

That's so pity we don't have a driver for all clocks/PLLs on ARC SDP yet.
So as of now I may only propose to use hard-coded fixed clocks as I did with
ARC PGU, see "pguclk" here:
http://lists.infradead.org/pipermail/linux-snps-arc/2016-March/000790.html

Again I'll try to implement missing clock driver sometime soon because
more and more stuff requires it but for now let's use a work-around.

> +struct dw_i2s_pll {
> + unsigned int rate;
> + unsigned int data_width;
> + unsigned int idiv;
> + unsigned int fbdiv;
> + unsigned int odiv0;
> + unsigned int odiv1;
> +};
> +
> +static const struct dw_i2s_pll dw_i2s_pll_cfg_27m[] = {
> + /* 27Mhz */
> + { 32000, 16, 0x104, 0x451, 0x10E38, 0x2000 },
> + { 44100, 16, 0x104, 0x596, 0x10D35, 0x2000 },
> + { 48000, 16, 0x208, 0xA28, 0x10B2C, 0x2000 },
> + { 0, 0, 0, 0, 0, 0 },
>  };
>  
> +static const struct dw_i2s_pll dw_i2s_pll_cfg_28m[] = {
> + /* 28.224Mhz */
> + { 32000, 16, 0x82, 0x105, 0x107DF, 0x2000 },
> + { 44100, 16, 0x28A, 0x1, 0x10001, 0x2000 },
> + { 48000, 16, 0xA28, 0x187, 0x10042, 0x2000 },
> + { 0, 0, 0, 0, 0, 0 },
> +};

These 2 hunks as well should go in ARC SDP clocks.

> +static int i2s_pll_cfg(struct i2s_clk_config_data *config)
> +{
> + const struct dw_i2s_pll *pll_cfg;
> + u32 rate = config->sample_rate;
> + u32 data_width = config->data_width;
> + int i;
> +
> + if (readl((void *)FPGA_VER_INFO) <= FPGA_VER_27M)
> + pll_cfg = dw_i2s_pll_cfg_27m;
> + else
> + pll_cfg = dw_i2s_pll_cfg_28m;
> +
> + for (i = 0; pll_cfg[i].rate != 0; i++) {
> + if ((pll_cfg[i].rate == rate) &&
> + (pll_cfg[i].data_width == data_width)) {
> + writel(pll_cfg[i].idiv, (void *)PLL_IDIV_ADDR);
> + writel(pll_cfg[i].fbdiv, (void *)PLL_FBDIV_ADDR);
> + writel(pll_cfg[i].odiv0, (void *)PLL_ODIV0_ADDR);
> + writel(pll_cfg[i].odiv1, (void *)PLL_ODIV1_ADDR);
> + return 0;
> + }
> + }
> +
> + return -EINVAL;
> +}

Ditto.

-Alexey