Re: [PATCH 6/6] ASoC: fsl_ssi: adjust set DAI format in AC'97 mode

2015-08-02 Thread Markus Pargmann 
On Fri, Jul 31, 2015 at 05:13:06PM +0200, Maciej S. Szmigiero wrote:
 On 31.07.2015 07:58, Markus Pargmann wrote:
  On Thu, Jul 30, 2015 at 04:35:58PM +0200, Maciej S. Szmigiero wrote:
  Adjust set DAI format function in fsl_ssi driver so it
  doesn't fail and clears RXDIR in AC'97 mode.
 
  Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
  ---
   sound/soc/fsl/fsl_ssi.c |8 +---
   1 files changed, 5 insertions(+), 3 deletions(-)
 
  diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
  index 8e5ff5e..37aabe3 100644
  --- a/sound/soc/fsl/fsl_ssi.c
  +++ b/sound/soc/fsl/fsl_ssi.c
  @@ -900,14 +900,16 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
 scr = ~CCSR_SSI_SCR_SYS_CLK_EN;
 break;
 default:
  -  return -EINVAL;
  +  if (!fsl_ssi_is_ac97(ssi_private))
  +  return -EINVAL;
  
  I think it would be better to add another case for the other mode which
  is supported (AC97) instead of using the default case.
 
 This is a switch of DAI clock masters and AC'97 is none of them:
 while case 0: can be added this would be very similar to the current code.
 
 Alternatively, the whole switch statement could be wrapped inside
 if (!fsl_ssi_is_ac97(ssi_private)) if that would be better
 with regards to code style.

I looked at the wrong switch/case the DAIFMT_AC97 is actually used
but this patch is about the master clocks. It's fine then.

Thanks,

Markus

 
 }
   
 stcr |= strcr;
 srcr |= strcr;
   
  -  if (ssi_private-cpu_dai_drv.symmetric_rates) {
  -  /* Need to clear RXDIR when using SYNC mode */
  +  if (ssi_private-cpu_dai_drv.symmetric_rates
  +  || fsl_ssi_is_ac97(ssi_private)) {
  
  Please fix this indention. Most of the driver is written with 2 tab
  indention after a line break and the new policy seems to be to indent on
  the opening bracket.
 
 Will reindent this.
 
  
  Regards,
  
  Markus
 
 Best regards,
 Maciej Szmigiero
 
 

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 5/6] ASoC: fsl_ssi: instantiate AC'97 CODEC

2015-08-02 Thread Markus Pargmann 
On Fri, Jul 31, 2015 at 04:39:20PM +0200, Maciej S. Szmigiero wrote:
 On 31.07.2015 07:46, Markus Pargmann wrote:
  On Thu, Jul 30, 2015 at 04:35:23PM +0200, Maciej S. Szmigiero wrote:
  Instantiate AC'97 CODEC in fsl_ssi driver AC'97 mode.
 
  Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
  ---
   sound/soc/fsl/fsl_ssi.c |   21 +
   1 files changed, 21 insertions(+), 0 deletions(-)
 
  diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
  index 154bcf6..8e5ff5e 100644
  --- a/sound/soc/fsl/fsl_ssi.c
  +++ b/sound/soc/fsl/fsl_ssi.c
  @@ -1460,6 +1460,27 @@ done:
 _fsl_ssi_set_dai_fmt(pdev-dev, ssi_private,
  ssi_private-dai_fmt);
   
  +  if (fsl_ssi_is_ac97(ssi_private)) {
  +  u32 ssi_idx;
  +
  +  ret = of_property_read_u32(np, cell-index, ssi_idx);
  
  This property is not set anywhere in the imx* DTs.
 
 That's right, but it is documented as required property in sound/fsl,ssi.txt:
  Required properties:
  (..)
  - cell-index:   The SSI, 0 = SSI1, 1 = SSI2, and so on.
 
  
  +  if (ret) {
  +  dev_err(pdev-dev, cannot get SSI index property\n);
  +  goto error_sound_card;
  +  }
  +
  +  ssi_private-pdev =
  +  platform_device_register_data(NULL,
  +  ac97-codec, ssi_idx, NULL, 0);
  
  If you really want to create a device at this point you should use
  PLATFORM_DEVID_AUTO. I would prefer something where this is created in
  DT. On the other side it is a discoverable bus..
 
 In the original implementation the codec was instantiated in DT but
 the feedback was that this is wrong since devices on discoverable
 buses shouldn't be in DT.
 
 The platform device index is based on SSI index since the sound
 card driver (fsl-asoc-card) has to know CODEC device name to identify
 it in DAI links - as the only identification options seem to be either
 DT node of CODEC or its device name.
 
 That's why the (platform) device name has to be deterministic
 if there is no mechanism to pass it from controller driver
 to sound card driver.

Thanks for clarification. I am not really happy with using this
cell-index as the whole information that should be necessary is already
contained in the memory address range given. The SSI units are otherwise
identical in hardware, so cell-index feels like some arbitrary
description that is used in the Reference Manual and for software.

However I don't have a better idea how to solve this at the moment and
as it is listed as required property and not a new DT binding it can be
used.

Best regards,

Markus

 
  Regards,
  
  Markus
 
 Best regards,
 Maciej Szmigiero
 
 

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/6] ASoC: fsl_ssi: enable AC'97 asymmetric rates

2015-08-02 Thread Markus Pargmann 
Hi,

On Fri, Jul 31, 2015 at 04:38:20PM +0200, Maciej S. Szmigiero wrote:
 Hi Markus,
 
 Thanks for looking into the changes.
 
 On 31.07.2015 07:53, Markus Pargmann wrote:
  On Fri, Jul 31, 2015 at 07:27:19AM +0200, Markus Pargmann wrote:
  Hi,
 
  On Thu, Jul 30, 2015 at 04:34:19PM +0200, Maciej S. Szmigiero wrote:
  AC'97 bus can support asymmetric playback/capture rates
  so enable them in this case in fsl_ssi driver.
 
  Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
  ---
   sound/soc/fsl/fsl_ssi.c |4 +++-
   1 files changed, 3 insertions(+), 1 deletions(-)
 
  diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
  index a83b900..7f4f0b9 100644
  --- a/sound/soc/fsl/fsl_ssi.c
  +++ b/sound/soc/fsl/fsl_ssi.c
  @@ -1377,7 +1377,9 @@ static int fsl_ssi_probe(struct platform_device 
  *pdev)
   
/* Are the RX and the TX clocks locked? */
if (!of_find_property(np, fsl,ssi-asynchronous, NULL)) {
  - ssi_private-cpu_dai_drv.symmetric_rates = 1;
  + if (!fsl_ssi_is_ac97(ssi_private))
  + ssi_private-cpu_dai_drv.symmetric_rates = 1;
  +
 
  Why don't you use the DT property that is parsed here to enable
  asymmetric rates?
 
 Because in the AC'97 mode the driver supports only one channel config and one 
 sample format
 anyway the remaining settings controlled by this property don't do anything.
 
 Since it should be safe to enable asymmetric rates with any AC'97 CODEC I 
 think it is good
 to do it in driver than to add fsl,ssi-asynchronous to every AC'97 DT node.
 
 Also the description of this property in fsl,ssi.txt looks like that it only 
 makes sense in
 non-AC'97 mode.
 
  Just found the last version of this series. Please use v2 and describe
  changes for a new iteration of a series.
 
 This is just a resubmission, there are no functional differences since
 it was originally submitted a month ago. 

I see, 'RESEND' tag would be helpful then.

 
  There is also a different setup with AC97 which does not use DMA. See
  the long comment at the top of the file about how ac97 is already used.
 
 I understand that the problem with FIQ handler described in comment
 on top of fsl_ssi.c only pertains incoming data, that is capture.
 
 This patch effectively does not touch capture rates as they are already
 limited to 48kHz only since this is the only capture rate defined in
 fsl_ssi AC'97 DAI driver capture capabilities.

Ok, great.

Thanks,

Markus

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/6] ASoC: fsl_ssi: enable AC'97 asymmetric rates

2015-07-30 Thread Markus Pargmann 
Hi,

On Thu, Jul 30, 2015 at 04:34:19PM +0200, Maciej S. Szmigiero wrote:
 AC'97 bus can support asymmetric playback/capture rates
 so enable them in this case in fsl_ssi driver.
 
 Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
 ---
  sound/soc/fsl/fsl_ssi.c |4 +++-
  1 files changed, 3 insertions(+), 1 deletions(-)
 
 diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
 index a83b900..7f4f0b9 100644
 --- a/sound/soc/fsl/fsl_ssi.c
 +++ b/sound/soc/fsl/fsl_ssi.c
 @@ -1377,7 +1377,9 @@ static int fsl_ssi_probe(struct platform_device *pdev)
  
   /* Are the RX and the TX clocks locked? */
   if (!of_find_property(np, fsl,ssi-asynchronous, NULL)) {
 - ssi_private-cpu_dai_drv.symmetric_rates = 1;
 + if (!fsl_ssi_is_ac97(ssi_private))
 + ssi_private-cpu_dai_drv.symmetric_rates = 1;
 +

Why don't you use the DT property that is parsed here to enable
asymmetric rates?

Regards,

Markus

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 3/6] ASoC: fsl_ssi: enable AC'97 asymmetric rates

2015-07-30 Thread Markus Pargmann 
On Fri, Jul 31, 2015 at 07:27:19AM +0200, Markus Pargmann wrote:
 Hi,
 
 On Thu, Jul 30, 2015 at 04:34:19PM +0200, Maciej S. Szmigiero wrote:
  AC'97 bus can support asymmetric playback/capture rates
  so enable them in this case in fsl_ssi driver.
  
  Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
  ---
   sound/soc/fsl/fsl_ssi.c |4 +++-
   1 files changed, 3 insertions(+), 1 deletions(-)
  
  diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
  index a83b900..7f4f0b9 100644
  --- a/sound/soc/fsl/fsl_ssi.c
  +++ b/sound/soc/fsl/fsl_ssi.c
  @@ -1377,7 +1377,9 @@ static int fsl_ssi_probe(struct platform_device *pdev)
   
  /* Are the RX and the TX clocks locked? */
  if (!of_find_property(np, fsl,ssi-asynchronous, NULL)) {
  -   ssi_private-cpu_dai_drv.symmetric_rates = 1;
  +   if (!fsl_ssi_is_ac97(ssi_private))
  +   ssi_private-cpu_dai_drv.symmetric_rates = 1;
  +
 
 Why don't you use the DT property that is parsed here to enable
 asymmetric rates?

Just found the last version of this series. Please use v2 and describe
changes for a new iteration of a series.

There is also a different setup with AC97 which does not use DMA. See
the long comment at the top of the file about how ac97 is already used.

Regards,

Markus

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 5/6] ASoC: fsl_ssi: instantiate AC'97 CODEC

2015-07-30 Thread Markus Pargmann 
On Thu, Jul 30, 2015 at 04:35:23PM +0200, Maciej S. Szmigiero wrote:
 Instantiate AC'97 CODEC in fsl_ssi driver AC'97 mode.
 
 Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
 ---
  sound/soc/fsl/fsl_ssi.c |   21 +
  1 files changed, 21 insertions(+), 0 deletions(-)
 
 diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
 index 154bcf6..8e5ff5e 100644
 --- a/sound/soc/fsl/fsl_ssi.c
 +++ b/sound/soc/fsl/fsl_ssi.c
 @@ -1460,6 +1460,27 @@ done:
   _fsl_ssi_set_dai_fmt(pdev-dev, ssi_private,
ssi_private-dai_fmt);
  
 + if (fsl_ssi_is_ac97(ssi_private)) {
 + u32 ssi_idx;
 +
 + ret = of_property_read_u32(np, cell-index, ssi_idx);

This property is not set anywhere in the imx* DTs.

 + if (ret) {
 + dev_err(pdev-dev, cannot get SSI index property\n);
 + goto error_sound_card;
 + }
 +
 + ssi_private-pdev =
 + platform_device_register_data(NULL,
 + ac97-codec, ssi_idx, NULL, 0);

If you really want to create a device at this point you should use
PLATFORM_DEVID_AUTO. I would prefer something where this is created in
DT. On the other side it is a discoverable bus..

Regards,

Markus

 + if (IS_ERR(ssi_private-pdev)) {
 + ret = PTR_ERR(ssi_private-pdev);
 + dev_err(pdev-dev,
 + failed to register AC97 codec platform: %d\n,
 + ret);
 + goto error_sound_card;
 + }
 + }
 +
   return 0;
  
  error_sound_card:
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
 

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 6/6] ASoC: fsl_ssi: adjust set DAI format in AC'97 mode

2015-07-30 Thread Markus Pargmann 
On Thu, Jul 30, 2015 at 04:35:58PM +0200, Maciej S. Szmigiero wrote:
 Adjust set DAI format function in fsl_ssi driver so it
 doesn't fail and clears RXDIR in AC'97 mode.
 
 Signed-off-by: Maciej Szmigiero m...@maciej.szmigiero.name
 ---
  sound/soc/fsl/fsl_ssi.c |8 +---
  1 files changed, 5 insertions(+), 3 deletions(-)
 
 diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
 index 8e5ff5e..37aabe3 100644
 --- a/sound/soc/fsl/fsl_ssi.c
 +++ b/sound/soc/fsl/fsl_ssi.c
 @@ -900,14 +900,16 @@ static int _fsl_ssi_set_dai_fmt(struct device *dev,
   scr = ~CCSR_SSI_SCR_SYS_CLK_EN;
   break;
   default:
 - return -EINVAL;
 + if (!fsl_ssi_is_ac97(ssi_private))
 + return -EINVAL;

I think it would be better to add another case for the other mode which
is supported (AC97) instead of using the default case.

   }
  
   stcr |= strcr;
   srcr |= strcr;
  
 - if (ssi_private-cpu_dai_drv.symmetric_rates) {
 - /* Need to clear RXDIR when using SYNC mode */
 + if (ssi_private-cpu_dai_drv.symmetric_rates
 + || fsl_ssi_is_ac97(ssi_private)) {

Please fix this indention. Most of the driver is written with 2 tab
indention after a line break and the new policy seems to be to indent on
the opening bracket.

Regards,

Markus

 + /* Need to clear RXDIR when using SYNC or AC97 mode */
   srcr = ~CCSR_SSI_SRCR_RXDIR;
   scr |= CCSR_SSI_SCR_SYN;
   }
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-kernel in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 Please read the FAQ at  http://www.tux.org/lkml/
 

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] ASoC: fsl_ssi: free irq before irq_dispose_mapping()

2014-11-30 Thread Markus Pargmann 
Hi,

On Mon, Dec 01, 2014 at 11:50:51AM +0900, Jiada Wang wrote:
 irq_dispose_mapping() in turns calls unregister_irq_proc(),
 which will remove irq proc entry, if IRQ is not freed
 before calling of irq_dispose_mapping(), then it will cause
 kernel warning.
 
 By free IRQ before irq_dispose_mapping(), this patch fix
 the following kernel warning found when remove of fsl_ssi driver:
 
 [   31.515336] [ cut here ]
 [   31.520091] WARNING: CPU: 2 PID: 434 at fs/proc/generic.c:521 
 remove_proc_entry+0x14c/0x16c()
 [   31.528708] remove_proc_entry: removing non-empty directory 'irq/79', 
 leaking at least '202c000.ss'
 [   31.537911] Modules linked in: snd_soc_wm8962 snd_soc_imx_wm8962 
 snd_soc_fsl_ssi(-) evbug
 [   31.546249] CPU: 2 PID: 434 Comm: rmmod Not tainted 
 3.18.0-rc6-00028-g3314bf6-dirty #1
 [   31.554235] Backtrace:
 [   31.556816] [80011ea8] (dump_backtrace) from [80012044] 
 (show_stack+0x18/0x1c)
 [   31.564416]  r6:80142c88 r5: r4: r3:
 [   31.570267] [8001202c] (show_stack) from [806980ec] 
 (dump_stack+0x88/0xa4)
 [   31.577588] [80698064] (dump_stack) from [80029d78] 
 (warn_slowpath_common+0x70/0x94)
 [   31.585711]  r5:0009 r4:bb61fd90
 [   31.589423] [80029d08] (warn_slowpath_common) from [80029e40] 
 (warn_slowpath_fmt+0x38/0x40)
 [   31.598187]  r8:bb61fdfe r7:be05d76d r6:be05d9a8 r5:0002 r4:be05d700
 [   31.605054] [80029e0c] (warn_slowpath_fmt) from [80142c88] 
 (remove_proc_entry+0x14c/0x16c)
 [   31.613709]  r3:806a79c0 r2:808229a0
 [   31.617371] [80142b3c] (remove_proc_entry) from [80070380] 
 (unregister_irq_proc+0x94/0xb8)
 [   31.625989]  r10: r8:8000ede4 r7:80955f2c r6:004f r5:8118e738 
 r4:be00af00
 [   31.633952] [800702ec] (unregister_irq_proc) from [80069dac] 
 (free_desc+0x2c/0x64)
 [   31.641898]  r6:004f r5:80955f38 r4:be00af00
 [   31.646604] [80069d80] (free_desc) from [80069e68] 
 (irq_free_descs+0x4c/0x8c)
 [   31.654092]  r7:0081 r6:0001 r5:004f r4:0001
 [   31.659863] [80069e1c] (irq_free_descs) from [8006fc3c] 
 (irq_dispose_mapping+0x40/0x5c)
 [   31.668247]  r6:be17b844 r5:be17b800 r4:004f r3:802c5ec0
 [   31.673998] [8006fbfc] (irq_dispose_mapping) from [7f004ea4] 
 (fsl_ssi_remove+0x58/0x70 [snd_so)
 [   31.683948]  r4:bb5bba10 r3:0001
 [   31.687618] [7f004e4c] (fsl_ssi_remove [snd_soc_fsl_ssi]) from 
 [803720a0] (platform_drv_remove)
 [   31.697564]  r5:7f0064f8 r4:be17b810
 [   31.701195] [80372080] (platform_drv_remove) from [80370494] 
 (__device_release_driver+0x78/0xc)
 [   31.710361]  r5:7f0064f8 r4:be17b810
 [   31.713987] [8037041c] (__device_release_driver) from [80370d20] 
 (driver_detach+0xbc/0xc0)
 [   31.722631]  r5:7f0064f8 r4:be17b810
 [   31.726259] [80370c64] (driver_detach) from [80370304] 
 (bus_remove_driver+0x54/0x98)
 [   31.734382]  r6:0800 r5: r4:7f0064f8 r3:bb67f500
 [   31.740149] [803702b0] (bus_remove_driver) from [80371398] 
 (driver_unregister+0x30/0x50)
 [   31.748617]  r4:7f0064f8 r3:bd9f7080
 [   31.752245] [80371368] (driver_unregister) from [80371f3c] 
 (platform_driver_unregister+0x14/0x)
 [   31.761498]  r4:7f00655c r3:7f005a70
 [   31.765130] [80371f28] (platform_driver_unregister) from [7f005a84] 
 (fsl_ssi_driver_exit+0x14/)
 [   31.776147] [7f005a70] (fsl_ssi_driver_exit [snd_soc_fsl_ssi]) from 
 [8008ed80] (SyS_delete_mod)
 [   31.786553] [8008ec64] (SyS_delete_module) from [8000ec20] 
 (ret_fast_syscall+0x0/0x48)
 [   31.794824]  r6:00c46d18 r5:0800 r4:00c46d18
 [   31.799530] ---[ end trace 954e8a3a15379e52 ]---
 
 Moreover replace devm_request_irq() with request_irq() since there is
 no need to use it as now driver always frees IRQ manually.

devm_request_irq() is used by other drivers too, this should not be a
problem. Looking at the code it seems that irq_dispose_mapping may not
be necessary with devm_request_irq(). So I think it would be better to
remove irq_dispose_mapping() instead.

Best Regards,

Markus Pargmann

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V3] ASoC: fsl_ssi: refine ipg clock usage in this module

2014-09-15 Thread Markus Pargmann 
On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
 Check if ipg clock is in clock-names property, then we can move the
 ipg clock enable and disable operation to startup and shutdown, that
 is only enable ipg clock when ssi is working and keep clock is disabled
 when ssi is in idle.
 But when the checking is failed, remain the clock control as before.
 
 Signed-off-by: Shengjiu Wang shengjiu.w...@freescale.com
 ---
 V3 change log:
 update patch according Nicolin and markus's comments
 
 
  sound/soc/fsl/fsl_ssi.c |   53 
 ---
  1 file changed, 45 insertions(+), 8 deletions(-)
 
 diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
 index 2fc3e66..6d1dfd5 100644
 --- a/sound/soc/fsl/fsl_ssi.c
 +++ b/sound/soc/fsl/fsl_ssi.c
 @@ -169,6 +169,7 @@ struct fsl_ssi_private {
   u8 i2s_mode;
   bool use_dma;
   bool use_dual_fifo;
 + bool has_ipg_clk_name;
   unsigned int fifo_depth;
   struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
  
 @@ -530,6 +531,11 @@ static int fsl_ssi_startup(struct snd_pcm_substream 
 *substream,
   struct snd_soc_pcm_runtime *rtd = substream-private_data;
   struct fsl_ssi_private *ssi_private =
   snd_soc_dai_get_drvdata(rtd-cpu_dai);
 + int ret;
 +
 + ret = clk_prepare_enable(ssi_private-clk);
 + if (ret)
 + return ret;
  
   /* When using dual fifo mode, it is safer to ensure an even period
* size. If appearing to an odd number while DMA always starts its
 @@ -544,6 +550,21 @@ static int fsl_ssi_startup(struct snd_pcm_substream 
 *substream,
  }
  
  /**
 + * fsl_ssi_shutdown: shutdown the SSI
 + *
 + */
 +static void fsl_ssi_shutdown(struct snd_pcm_substream *substream,
 + struct snd_soc_dai *dai)
 +{
 + struct snd_soc_pcm_runtime *rtd = substream-private_data;
 + struct fsl_ssi_private *ssi_private =
 + snd_soc_dai_get_drvdata(rtd-cpu_dai);
 +
 + clk_disable_unprepare(ssi_private-clk);
 +
 +}
 +
 +/**
   * fsl_ssi_set_bclk - configure Digital Audio Interface bit clock
   *
   * Note: This function can be only called when using SSI as DAI master
 @@ -1043,6 +1064,7 @@ static int fsl_ssi_dai_probe(struct snd_soc_dai *dai)
  
  static const struct snd_soc_dai_ops fsl_ssi_dai_ops = {
   .startup= fsl_ssi_startup,
 + .shutdown   = fsl_ssi_shutdown,
   .hw_params  = fsl_ssi_hw_params,
   .hw_free= fsl_ssi_hw_free,
   .set_fmt= fsl_ssi_set_dai_fmt,
 @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct platform_device 
 *pdev,
   u32 dmas[4];
   int ret;
  
 - ssi_private-clk = devm_clk_get(pdev-dev, NULL);
 + if (ssi_private-has_ipg_clk_name)
 + ssi_private-clk = devm_clk_get(pdev-dev, ipg);
 + else
 + ssi_private-clk = devm_clk_get(pdev-dev, NULL);
   if (IS_ERR(ssi_private-clk)) {
   ret = PTR_ERR(ssi_private-clk);
   dev_err(pdev-dev, could not get clock: %d\n, ret);
   return ret;
   }
  
 - ret = clk_prepare_enable(ssi_private-clk);
 - if (ret) {
 - dev_err(pdev-dev, clk_prepare_enable failed: %d\n, ret);
 - return ret;
 + if (!ssi_private-has_ipg_clk_name) {
 + ret = clk_prepare_enable(ssi_private-clk);
 + if (ret) {
 + dev_err(pdev-dev, clk_prepare_enable failed: %d\n, 
 ret);
 + return ret;
 + }
   }
  
   /* For those SLAVE implementations, we ingore non-baudclk cases
 @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device 
 *pdev,
   return 0;
  
  error_pcm:
 - clk_disable_unprepare(ssi_private-clk);
  
 + if (!ssi_private-has_ipg_clk_name)
 + clk_disable_unprepare(ssi_private-clk);
   return ret;
  }
  
 @@ -1246,7 +1274,8 @@ static void fsl_ssi_imx_clean(struct platform_device 
 *pdev,
  {
   if (!ssi_private-use_dma)
   imx_pcm_fiq_exit(pdev);
 - clk_disable_unprepare(ssi_private-clk);
 + if (!ssi_private-has_ipg_clk_name)
 + clk_disable_unprepare(ssi_private-clk);
  }
  
  static int fsl_ssi_probe(struct platform_device *pdev)
 @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device *pdev)
   return -ENOMEM;
   }
  
 - ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
 + ret = of_property_match_string(np, clock-names, ipg);
 + if (ret  0) {
 + ssi_private-has_ipg_clk_name = false;
 + ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
   fsl_ssi_regconfig);

Sorry if I was unclear about that. My suggestion was to enable the clock
right here:
clk_prepare_enable(ssi_private-clk);

Then you can remove ssi_private-has_ipg_clk_name and all
clk_prepare_enable() and clk_disable_unprepare() from above. Also you
can move the

Re: [PATCH V3] ASoC: fsl_ssi: refine ipg clock usage in this module

2014-09-15 Thread Markus Pargmann 
On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote:
 On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
  On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
   Check if ipg clock is in clock-names property, then we can move the
   ipg clock enable and disable operation to startup and shutdown, that
   is only enable ipg clock when ssi is working and keep clock is disabled
   when ssi is in idle.
   But when the checking is failed, remain the clock control as before.
   
   Signed-off-by: Shengjiu Wang shengjiu.w...@freescale.com
   ---
   V3 change log:
   update patch according Nicolin and markus's comments
   
   
sound/soc/fsl/fsl_ssi.c |   53 
   ---
1 file changed, 45 insertions(+), 8 deletions(-)
   
   diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
   index 2fc3e66..6d1dfd5 100644
   --- a/sound/soc/fsl/fsl_ssi.c
   +++ b/sound/soc/fsl/fsl_ssi.c
   @@ -169,6 +169,7 @@ struct fsl_ssi_private {
 u8 i2s_mode;
 bool use_dma;
 bool use_dual_fifo;
   + bool has_ipg_clk_name;
 unsigned int fifo_depth;
 struct fsl_ssi_rxtx_reg_val rxtx_reg_val;

   @@ -530,6 +531,11 @@ static int fsl_ssi_startup(struct snd_pcm_substream 
   *substream,
 struct snd_soc_pcm_runtime *rtd = substream-private_data;
 struct fsl_ssi_private *ssi_private =
 snd_soc_dai_get_drvdata(rtd-cpu_dai);
   + int ret;
   +
   + ret = clk_prepare_enable(ssi_private-clk);
   + if (ret)
   + return ret;

 /* When using dual fifo mode, it is safer to ensure an even period
  * size. If appearing to an odd number while DMA always starts its
   @@ -544,6 +550,21 @@ static int fsl_ssi_startup(struct snd_pcm_substream 
   *substream,
}

/**
   + * fsl_ssi_shutdown: shutdown the SSI
   + *
   + */
   +static void fsl_ssi_shutdown(struct snd_pcm_substream *substream,
   + struct snd_soc_dai *dai)
   +{
   + struct snd_soc_pcm_runtime *rtd = substream-private_data;
   + struct fsl_ssi_private *ssi_private =
   + snd_soc_dai_get_drvdata(rtd-cpu_dai);
   +
   + clk_disable_unprepare(ssi_private-clk);
   +
   +}
   +
   +/**
 * fsl_ssi_set_bclk - configure Digital Audio Interface bit clock
 *
 * Note: This function can be only called when using SSI as DAI master
   @@ -1043,6 +1064,7 @@ static int fsl_ssi_dai_probe(struct snd_soc_dai 
   *dai)

static const struct snd_soc_dai_ops fsl_ssi_dai_ops = {
 .startup= fsl_ssi_startup,
   + .shutdown   = fsl_ssi_shutdown,
 .hw_params  = fsl_ssi_hw_params,
 .hw_free= fsl_ssi_hw_free,
 .set_fmt= fsl_ssi_set_dai_fmt,
   @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct 
   platform_device *pdev,
 u32 dmas[4];
 int ret;

   - ssi_private-clk = devm_clk_get(pdev-dev, NULL);
   + if (ssi_private-has_ipg_clk_name)
   + ssi_private-clk = devm_clk_get(pdev-dev, ipg);
   + else
   + ssi_private-clk = devm_clk_get(pdev-dev, NULL);
 if (IS_ERR(ssi_private-clk)) {
 ret = PTR_ERR(ssi_private-clk);
 dev_err(pdev-dev, could not get clock: %d\n, ret);
 return ret;
 }

   - ret = clk_prepare_enable(ssi_private-clk);
   - if (ret) {
   - dev_err(pdev-dev, clk_prepare_enable failed: %d\n, ret);
   - return ret;
   + if (!ssi_private-has_ipg_clk_name) {
   + ret = clk_prepare_enable(ssi_private-clk);
   + if (ret) {
   + dev_err(pdev-dev, clk_prepare_enable failed: %d\n, 
   ret);
   + return ret;
   + }
 }

 /* For those SLAVE implementations, we ingore non-baudclk cases
   @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct platform_device 
   *pdev,
 return 0;

error_pcm:
   - clk_disable_unprepare(ssi_private-clk);

   + if (!ssi_private-has_ipg_clk_name)
   + clk_disable_unprepare(ssi_private-clk);
 return ret;
}

   @@ -1246,7 +1274,8 @@ static void fsl_ssi_imx_clean(struct 
   platform_device *pdev,
{
 if (!ssi_private-use_dma)
 imx_pcm_fiq_exit(pdev);
   - clk_disable_unprepare(ssi_private-clk);
   + if (!ssi_private-has_ipg_clk_name)
   + clk_disable_unprepare(ssi_private-clk);
}

static int fsl_ssi_probe(struct platform_device *pdev)
   @@ -1321,8 +1350,16 @@ static int fsl_ssi_probe(struct platform_device 
   *pdev)
 return -ENOMEM;
 }

   - ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
   + ret = of_property_match_string(np, clock-names, ipg);
   + if (ret  0) {
   + ssi_private-has_ipg_clk_name = false;
   + ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
 fsl_ssi_regconfig);
  
  Sorry if I was unclear about that. My suggestion was to enable the clock
  right here:
  clk_prepare_enable(ssi_private-clk

Re: [PATCH V3] ASoC: fsl_ssi: refine ipg clock usage in this module

2014-09-15 Thread Markus Pargmann 
On Mon, Sep 15, 2014 at 06:37:15PM +0800, Shengjiu Wang wrote:
 On Mon, Sep 15, 2014 at 12:32:13PM +0200, Markus Pargmann wrote:
  On Mon, Sep 15, 2014 at 06:22:27PM +0800, Shengjiu Wang wrote:
   On Mon, Sep 15, 2014 at 12:05:41PM +0200, Markus Pargmann wrote:
On Fri, Sep 12, 2014 at 06:35:15PM +0800, Shengjiu Wang wrote:
 Check if ipg clock is in clock-names property, then we can move the
 ipg clock enable and disable operation to startup and shutdown, that
 is only enable ipg clock when ssi is working and keep clock is 
 disabled
 when ssi is in idle.
 But when the checking is failed, remain the clock control as before.
 
 Signed-off-by: Shengjiu Wang shengjiu.w...@freescale.com
 ---
 V3 change log:
 update patch according Nicolin and markus's comments
 
 
  sound/soc/fsl/fsl_ssi.c |   53 
 ---
  1 file changed, 45 insertions(+), 8 deletions(-)
 
 diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
 index 2fc3e66..6d1dfd5 100644
 --- a/sound/soc/fsl/fsl_ssi.c
 +++ b/sound/soc/fsl/fsl_ssi.c
 @@ -169,6 +169,7 @@ struct fsl_ssi_private {
   u8 i2s_mode;
   bool use_dma;
   bool use_dual_fifo;
 + bool has_ipg_clk_name;
   unsigned int fifo_depth;
   struct fsl_ssi_rxtx_reg_val rxtx_reg_val;
  
 @@ -530,6 +531,11 @@ static int fsl_ssi_startup(struct 
 snd_pcm_substream *substream,
   struct snd_soc_pcm_runtime *rtd = substream-private_data;
   struct fsl_ssi_private *ssi_private =
   snd_soc_dai_get_drvdata(rtd-cpu_dai);
 + int ret;
 +
 + ret = clk_prepare_enable(ssi_private-clk);
 + if (ret)
 + return ret;
  
   /* When using dual fifo mode, it is safer to ensure an even 
 period
* size. If appearing to an odd number while DMA always starts 
 its
 @@ -544,6 +550,21 @@ static int fsl_ssi_startup(struct 
 snd_pcm_substream *substream,
  }
  
  /**
 + * fsl_ssi_shutdown: shutdown the SSI
 + *
 + */
 +static void fsl_ssi_shutdown(struct snd_pcm_substream *substream,
 + struct snd_soc_dai *dai)
 +{
 + struct snd_soc_pcm_runtime *rtd = substream-private_data;
 + struct fsl_ssi_private *ssi_private =
 + snd_soc_dai_get_drvdata(rtd-cpu_dai);
 +
 + clk_disable_unprepare(ssi_private-clk);
 +
 +}
 +
 +/**
   * fsl_ssi_set_bclk - configure Digital Audio Interface bit clock
   *
   * Note: This function can be only called when using SSI as DAI 
 master
 @@ -1043,6 +1064,7 @@ static int fsl_ssi_dai_probe(struct snd_soc_dai 
 *dai)
  
  static const struct snd_soc_dai_ops fsl_ssi_dai_ops = {
   .startup= fsl_ssi_startup,
 + .shutdown   = fsl_ssi_shutdown,
   .hw_params  = fsl_ssi_hw_params,
   .hw_free= fsl_ssi_hw_free,
   .set_fmt= fsl_ssi_set_dai_fmt,
 @@ -1168,17 +1190,22 @@ static int fsl_ssi_imx_probe(struct 
 platform_device *pdev,
   u32 dmas[4];
   int ret;
  
 - ssi_private-clk = devm_clk_get(pdev-dev, NULL);
 + if (ssi_private-has_ipg_clk_name)
 + ssi_private-clk = devm_clk_get(pdev-dev, ipg);
 + else
 + ssi_private-clk = devm_clk_get(pdev-dev, NULL);
   if (IS_ERR(ssi_private-clk)) {
   ret = PTR_ERR(ssi_private-clk);
   dev_err(pdev-dev, could not get clock: %d\n, ret);
   return ret;
   }
  
 - ret = clk_prepare_enable(ssi_private-clk);
 - if (ret) {
 - dev_err(pdev-dev, clk_prepare_enable failed: %d\n, 
 ret);
 - return ret;
 + if (!ssi_private-has_ipg_clk_name) {
 + ret = clk_prepare_enable(ssi_private-clk);
 + if (ret) {
 + dev_err(pdev-dev, clk_prepare_enable failed: 
 %d\n, ret);
 + return ret;
 + }
   }
  
   /* For those SLAVE implementations, we ingore non-baudclk cases
 @@ -1236,8 +1263,9 @@ static int fsl_ssi_imx_probe(struct 
 platform_device *pdev,
   return 0;
  
  error_pcm:
 - clk_disable_unprepare(ssi_private-clk);
  
 + if (!ssi_private-has_ipg_clk_name)
 + clk_disable_unprepare(ssi_private-clk);
   return ret;
  }
  
 @@ -1246,7 +1274,8 @@ static void fsl_ssi_imx_clean(struct 
 platform_device *pdev,
  {
   if (!ssi_private-use_dma)
   imx_pcm_fiq_exit(pdev);
 - clk_disable_unprepare(ssi_private-clk);
 + if (!ssi_private-has_ipg_clk_name)
 + clk_disable_unprepare(ssi_private-clk

Re: [PATCH V2] ASoC: fsl_ssi: refine ipg clock usage in this module

2014-09-12 Thread Markus Pargmann 
Hi,

On Fri, Sep 12, 2014 at 10:01:12AM +0800, Shengjiu Wang wrote:
 On Thu, Sep 11, 2014 at 03:57:37PM -0700, Nicolin Chen wrote:
  On Thu, Sep 11, 2014 at 01:38:29PM +0800, Shengjiu Wang wrote:
   Move the ipg clock enable and disable operation to startup and shutdown,
   that is only enable ipg clock when ssi is working. Keep clock is disabled
   when ssi is in idle.
   otherwise, _fsl_ssi_set_dai_fmt function need to be called in probe,
   so add ipg clock control for it.
  
  It seems to be no objection so far against my last suggestion to
  use regmap's mmio_clk() for named ipg clk only. So you may still
  consider about that.
 
 I think mmio_clk() can be put to another patch. and this patch only for 
 clk_enable()
 and clk_disable() operation.

I would also prefer Nicolin's suggestion using regmap's mmio clk. I
think it may be better to not add this particular patch at all and just
go with the mmio_clk patch. It should be easy enough to just add the
clock names to the devicetrees. That way we can avoid all those clock
enable/disable function calls.

  
  Anyway, I'd like to do thing in parallel. So I just simply tested
  it on my side and its works fine, it may still need to be tested
  by others though.
  
  Nicolina
 
 Hi Markus
 
 could you please review it, and share your comments?

I think the clock enabling for AC97 is missing in your patch.

Best regards,

Markus

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V2] ASoC: fsl_ssi: refine ipg clock usage in this module

2014-09-12 Thread Markus Pargmann 
On Fri, Sep 12, 2014 at 03:14:28PM +0800, Shengjiu Wang wrote:
 On Fri, Sep 12, 2014 at 08:17:06AM +0200, Markus Pargmann wrote:
  Hi,
  
  On Fri, Sep 12, 2014 at 10:01:12AM +0800, Shengjiu Wang wrote:
   On Thu, Sep 11, 2014 at 03:57:37PM -0700, Nicolin Chen wrote:
On Thu, Sep 11, 2014 at 01:38:29PM +0800, Shengjiu Wang wrote:
 Move the ipg clock enable and disable operation to startup and 
 shutdown,
 that is only enable ipg clock when ssi is working. Keep clock is 
 disabled
 when ssi is in idle.
 otherwise, _fsl_ssi_set_dai_fmt function need to be called in probe,
 so add ipg clock control for it.

It seems to be no objection so far against my last suggestion to
use regmap's mmio_clk() for named ipg clk only. So you may still
consider about that.
   
   I think mmio_clk() can be put to another patch. and this patch only for 
   clk_enable()
   and clk_disable() operation.
  
  I would also prefer Nicolin's suggestion using regmap's mmio clk. I
  think it may be better to not add this particular patch at all and just
  go with the mmio_clk patch. It should be easy enough to just add the
  clock names to the devicetrees. That way we can avoid all those clock
  enable/disable function calls.
 
 I considered if use Nicolin's suggestion, I still need ot add those
 enable/disable function calls, because I want to remove the enable/disable
 function call in fsl_ssi_imx_probe, then I need to add enable/disable 
 function call in _fsl_ssi_set_dai_fmt(), which is called in fsl_ssi_probe().
 _fsl_ssi_set_dai_fmt() need to access the registers.

I think Nicolin's suggestion was to check for a clock named ipg. If it
exists, you can simply use devm_regmap_init_mmio_clk(). Otherwise you
could fallback to the old behaviour and get the first clock and enable
it without disabling it after the probe function.

After that, you could add the clock-names property to the devicetrees
and have the same result.

 

Anyway, I'd like to do thing in parallel. So I just simply tested
it on my side and its works fine, it may still need to be tested
by others though.

Nicolina
   
   Hi Markus
   
   could you please review it, and share your comments?
  
  I think the clock enabling for AC97 is missing in your patch.
  
 I add clock enable in fsl_ssi_startup(), I think it is enough for AC97, does 
 it?

No. We export ac97 read/write ops for the whole AC97 stuff. These may be
used outside of the usual startup/shutdown which is done for substreams.
It may work to have the ipg clock enabled in
fsl_ssi_ac97_read/fsl_ssi_ac97_write.

Best regards,

Markus

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module

2014-09-11 Thread Markus Pargmann 
On Wed, Sep 10, 2014 at 10:42:04AM -0700, Nicolin Chen wrote:
 On Wed, Sep 10, 2014 at 04:12:53PM +0800, Shengjiu Wang wrote:
   Then we can get a patch like:
   open() {
   + clk_prepare_enable();
 
   }
   
   close() {
 
   + clk_disable_unprepare()
   }
 
  what is the open() and close()? do you mean the fsl_ssi_startup()
  and fsl_ssi_shutdown()?
 
 Yea.
 
   probe() {
 clk_get();
 clk_prepare_enable();
 
 if (xxx)
   - goto err_xx;
   + return ret;
 
   + clk_disable_unprepare();
 return 0;
   -err_xx:
   - clk_disable_unprepare()
   }
 
  If this probe() is fsl_ssi_imx_probe(), I think no need to add
  clk_prepare_enable() or clk_disable_unprepare(), seems there is no 
  registers accessing in this probe.
 
 This is trying to be safe, especially for such a driver being used
 by multiple platforms. You can omit this as long as the patch can
 pass the test on old imx, PowerPC, and AC97 platforms.
 
 
 
 And another risk just came to my mind is that there would be a
 possibility that a machine driver would call set_dai_fmt() early,
 after SSI's probe() and before SSI's startup(), if the machine
 driver contains dai_fmt assignment in its probe(). Then, without
 regmap_mmio_clk(), it'll be tough for us over here because we may
 also need to add clock enable/disable for set_dai_fmt/set_sysclk(),
 even if there might be still tiny risk that we missed something.

Thanks, didn't thought about that. As there are no restrictions on when
these functions may be called, it has to be handled.

 
 Then there could be a selfish approach to circumvent it is to use
 regmap_mmio_clk() with ipg at the beginning and call regmap_mmio()
 without ipg if getting a failed return value from regmap_mmio_clk,
 and meanwhile to keep the clock always enabled for the regmap_mmio()
 case just like what the current driver is doing. This may result
 those non-ipg-clk platforms can't benefit from this refinement
 unless they update their DT bindings -- use ipg for core clock
 This might be the safest and simplest way for us, I'm not sure
 everyone would be comfortable with this idea though.

I like the selfish approach. It would save a lot of clock
enabling/disabling and error handling and at the same time it doesn't break
the DT compatibility. The platforms with an old DT would have the old
behaviour, but that could be changed by updating the devicetrees which
should be easy to do for all the imx SoCs.

Best regards,

Markus

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module

2014-09-10 Thread Markus Pargmann 
On Tue, Sep 09, 2014 at 11:38:05AM -0700, Nicolin Chen wrote:
 On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
  @@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device 
  *pdev)
  return -ENOMEM;
  }
   
  -   ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
  +   if (ssi_private-soc-imx)
  +   ssi_private-regs = devm_regmap_init_mmio_clk(pdev-dev,
  +   ipg, iomem, fsl_ssi_regconfig);
  +   else
  +   ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
 
 As Markus mentioned, the key point here is to be compatible with those
 non-clock-name platforms.
 
 I think it would be safer to keep the current code while adding an extra
 clk_disable_unprepare() at the end of probe() as a common routine. And
 meantime, make sure to have the call for imx only because it seems that
 the other platforms do not depend on the clock. //a bit guessing here :)
 
 Then we can get a patch like:
 open() {
 + clk_prepare_enable();
   
 }
 
 close() {
   
 + clk_disable_unprepare()
 }
 
 probe() {
   clk_get();
   clk_prepare_enable();
   
   if (xxx)
 - goto err_xx;
 + return ret;
   
 + clk_disable_unprepare();
   return 0;
 -err_xx:
 - clk_disable_unprepare()
 }
 
 remove() {
   
 - clk_disable_unprepare()
 }

If I remember correctly, there may be AC97 communication with the codec
before any substream is created. That's why we enable the SSI unit right
at the beginning for AC97 in fsl_ssi_setup_reg_vals(). So we need to
check for AC97 before disabling clocks.

Best regards,

Markus

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module

2014-09-10 Thread Markus Pargmann 
Hi,

On Wed, Sep 10, 2014 at 06:30:06PM +0800, Shengjiu Wang wrote:
 On Wed, Sep 10, 2014 at 08:21:18AM +0200, Markus Pargmann wrote:
  On Tue, Sep 09, 2014 at 11:38:05AM -0700, Nicolin Chen wrote:
   On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
@@ -1321,7 +1333,11 @@ static int fsl_ssi_probe(struct platform_device 
*pdev)
return -ENOMEM;
}
 
-   ssi_private-regs = devm_regmap_init_mmio(pdev-dev, iomem,
+   if (ssi_private-soc-imx)
+   ssi_private-regs = 
devm_regmap_init_mmio_clk(pdev-dev,
+   ipg, iomem, fsl_ssi_regconfig);
+   else
+   ssi_private-regs = devm_regmap_init_mmio(pdev-dev, 
iomem,
   
   As Markus mentioned, the key point here is to be compatible with those
   non-clock-name platforms.
   
   I think it would be safer to keep the current code while adding an extra
   clk_disable_unprepare() at the end of probe() as a common routine. And
   meantime, make sure to have the call for imx only because it seems that
   the other platforms do not depend on the clock. //a bit guessing here :)
   
   Then we can get a patch like:
   open() {
   + clk_prepare_enable();
 
   }
   
   close() {
 
   + clk_disable_unprepare()
   }
   
   probe() {
 clk_get();
 clk_prepare_enable();
 
 if (xxx)
   - goto err_xx;
   + return ret;
 
   + clk_disable_unprepare();
 return 0;
   -err_xx:
   - clk_disable_unprepare()
   }
   
   remove() {
 
   - clk_disable_unprepare()
   }
  
  If I remember correctly, there may be AC97 communication with the codec
  before any substream is created. That's why we enable the SSI unit right
  at the beginning for AC97 in fsl_ssi_setup_reg_vals(). So we need to
  check for AC97 before disabling clocks.
  
  Best regards,
  
  Markus
 
 hi Markus
 
 I think if clk_prepare_enable() in startup(), and clk_disable_unprepare()
 in shutdown can meet this requirement, right?

Yes that could work.

 
 done:
 if (ssi_private-dai_fmt)
 _fsl_ssi_set_dai_fmt(ssi_private, ssi_private-dai_fmt);
 
 I find that in end of probe, there is setting of dai_fmt. Can we remove this?
 because this setting need to enable ipg clock, and if ac97, ipg clock can't be
 disabled.

No you can't remove it. It is necessary for the DT property fsl,mode.
Most dts do not have this property anymore because the sound cards are
setting the dai-fmt. But there are still some powerpc dts files that
contain that property.

Best regards,

Markus

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [alsa-devel] [PATCH V1] ASoC: fsl_ssi: refine ipg clock usage in this module

2014-09-09 Thread Markus Pargmann 
Hi,

On Tue, Sep 09, 2014 at 05:18:07PM +0800, Shengjiu Wang wrote:
 Move the ipg clock enable and disable operation to startup and shutdown,
 that is only enable ipg clock when ssi is working. we don't need to enable
 ipg clock in probe.
 Another register accessing need the ipg clock, so use 
 devm_regmap_init_mmio_clk
 instead of devm_regmap_init_mmio.
 
 Signed-off-by: Shengjiu Wang shengjiu.w...@freescale.com
 ---
  sound/soc/fsl/fsl_ssi.c |   38 +++---
  1 file changed, 27 insertions(+), 11 deletions(-)
 
 diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
 index 2fc3e66..d32d0f5 100644
 --- a/sound/soc/fsl/fsl_ssi.c
 +++ b/sound/soc/fsl/fsl_ssi.c
 @@ -531,6 +531,9 @@ static int fsl_ssi_startup(struct snd_pcm_substream 
 *substream,
   struct fsl_ssi_private *ssi_private =
   snd_soc_dai_get_drvdata(rtd-cpu_dai);
  
 + if (ssi_private-soc-imx)
 + clk_prepare_enable(ssi_private-clk);
 +
   /* When using dual fifo mode, it is safer to ensure an even period
* size. If appearing to an odd number while DMA always starts its
* task from fifo0, fifo1 would be neglected at the end of each
 @@ -544,6 +547,22 @@ static int fsl_ssi_startup(struct snd_pcm_substream 
 *substream,
  }
  
  /**
 + * fsl_ssi_shutdown: shutdown the SSI
 + *
 + */
 +static void fsl_ssi_shutdown(struct snd_pcm_substream *substream,
 + struct snd_soc_dai *dai)
 +{
 + struct snd_soc_pcm_runtime *rtd = substream-private_data;
 + struct fsl_ssi_private *ssi_private =
 + snd_soc_dai_get_drvdata(rtd-cpu_dai);
 +
 + if (ssi_private-soc-imx)
 + clk_disable_unprepare(ssi_private-clk);
 +
 +}
 +
 +/**
   * fsl_ssi_set_bclk - configure Digital Audio Interface bit clock
   *
   * Note: This function can be only called when using SSI as DAI master
 @@ -1043,6 +1062,7 @@ static int fsl_ssi_dai_probe(struct snd_soc_dai *dai)
  
  static const struct snd_soc_dai_ops fsl_ssi_dai_ops = {
   .startup= fsl_ssi_startup,
 + .shutdown   = fsl_ssi_shutdown,
   .hw_params  = fsl_ssi_hw_params,
   .hw_free= fsl_ssi_hw_free,
   .set_fmt= fsl_ssi_set_dai_fmt,
 @@ -1168,16 +1188,10 @@ static int fsl_ssi_imx_probe(struct platform_device 
 *pdev,
   u32 dmas[4];
   int ret;
  
 - ssi_private-clk = devm_clk_get(pdev-dev, NULL);
 + ssi_private-clk = devm_clk_get(pdev-dev, ipg);

This does not work for most imx SoCs at the moment. imx27, imx35, imx51
etc. do not have clock-names defined in the devicetree.

Best regards,

Markus

-- 
Pengutronix e.K.   | |
Industrial Linux Solutions | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686   | Fax:   +49-5121-206917- |


signature.asc
Description: Digital signature
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev