Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Nicolin Chen]

On Sat, Dec 16, 2017 at 12:31:34PM -0600, Timur Tabi wrote:
> On 12/16/17 11:49 AM, Nicolin Chen wrote:
> >I never said that I don't agree with Timur. Every change here is
> >to simplify things. As long as Timur or any reviewer feels one of
> >new comments is harder to understand, I am totally fine to rework.
> >I respect everyone's opinion, but I hope everyone can respect my
> >effort too by telling me which one needs to rework and why?
> 
> I do respect it.  I apologize if I came across as unduly harsh.

I believe everyone here has a good heart to make things better.
So I am not and won't take it personally. Let's just act more
cooperative. Thanks.

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Nicolin Chen]

On Sat, Dec 16, 2017 at 12:31:34PM -0600, Timur Tabi wrote:
> On 12/16/17 11:49 AM, Nicolin Chen wrote:
> >I never said that I don't agree with Timur. Every change here is
> >to simplify things. As long as Timur or any reviewer feels one of
> >new comments is harder to understand, I am totally fine to rework.
> >I respect everyone's opinion, but I hope everyone can respect my
> >effort too by telling me which one needs to rework and why?
> 
> I do respect it.  I apologize if I came across as unduly harsh.

I believe everyone here has a good heart to make things better.
So I am not and won't take it personally. Let's just act more
cooperative. Thanks.

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Nicolin Chen]

First of all, thanks a lot for the review. And I will send a v4
after I refine these comments.

But please please don't think in the way like "you can touch it
unless it's untrue." I never said anything or anyone is wrong
here. As the other patches that shortens variable names, this
patch just does something similar.

The point here is just to make the driver necessarily explained
while being brief. I am totally fine if you feel some of my new
comments are worse. I will refine it until you get satisfied.

And I hope you (or anyone else) can tell me more about what is
wrong with my new comments instead of asking me to drop them all.
I locally have more than 20 patches based on this one. Any change
I make to this one will give me a lot of trouble to rebase them.
So I am more willing to refine those that people really feel hard
to understand.

On Sat, Dec 16, 2017 at 11:15:43AM -0600, Timur Tabi wrote:
> On 12/13/17 5:18 PM, Nicolin Chen wrote:
> 
> >-/* Used when using fsl-ssi as sound-card. This is only used by ppc and
> >- * should be replaced with simple-sound-card. */
> > struct platform_device *pdev;
> 
> Is this comment no longer true?

It's moved to the comments of structure fsl_ssi. Since we defined
a pdev at the first place, we should have explained it along with
the definition.

> >-/**
> >- * fsl_ssi_isr: SSI interrupt handler
> >- *
> >- * Although it's possible to use the interrupt handler to send and receive
> >- * data to/from the SSI, we use the DMA instead.  Programming is more
> >- * complicated, but the performance is much better.
> >- *
> >- * This interrupt handler is used only to gather statistics.
> >- *
> >- * @irq: IRQ of the SSI device
> >- * @dev_id: pointer to the fsl_ssi structure for this SSI device
> >- */
> 
> What's wrong with this comment?

Nothing wrong. An interrupt handler is way too common sense in
a driver code. I am okay to retain it if you strongly feel it's
that necessary. But I would feel more plausible to clean it away.

> >- * Clear RX or TX FIFO to remove samples from the previous
> >- * stream session which may be still present in the FIFO and
> >- * may introduce bad samples and/or channel slipping.
> >- *
> >- * Note: The SOR is not documented in recent IMX datasheet, but
> >- * is described in IMX51 reference manual at section 56.3.3.15.
> >+/**
> >+ * Clear remaining data in the FIFO to avoid dirty data or channel slipping
> 
> I think the original is better, unless there's something untrue about it.

It's literally stating the same thing. And SOR register comments
are moved to the header file.

> >- * We are running on a SoC which does not support online SSI
> >- * reconfiguration, so we have to enable all necessary flags at once
> >- * even if we do not use them later (capture and playback configuration)
> >+ * Online configuration is not supported
> >+ * Enable or Disable all necessary bits at once
> 
> Ditto

The code also does disabling as well however it doesn't mention
at all. Just like you might have hard time to understand my new
comments, I also had a hard time to understand this one. So I'll
have to change it. My new comments are shorter but covers both
enable and disable. I could make it more descriptive if necessary.

> >-/*
> >- * Configure single direction units while the SSI unit is running
> >- * (online configuration)
> >- */
> >+/* Online configure single direction while SSI is running */
> 
> Ditto

It's literally the same but shorter. I don't think anyone would
have trouble to understand mine...

> >-/*
> >- * Be sure the Tx FIFO is filled when TE is set.
> >- * Otherwise, there are some chances to start the
> >- * playback with some void samples inserted first,
> >- * generating a channel slip.
> >- *
> >- * First, SSIEN must be set, to let the FIFO be filled.
> >- *
> >- * Notes:
> >- * - Limit this fix to the DMA case until FIQ cases can
> >- *   be tested.
> >- * - Limit the length of the busy loop to not lock the
> >- *   system too long, even if 1-2 loops are sufficient
> >- *   in general.
> >- */
> 
> What's wrong with this comment?

I have new comments covering necessary steps. But I could bring
some parts back if that makes you happy.

> >-/*
> >- * Note that these below aren't just normal registers.
> >- * They are a way to disable or enable bits in SACCST
> >- * register:
> >- * - writing a '1' bit at some position in SACCEN sets the
> >- * relevant bit in SACCST,
> >- * - writing a '1' bit at some position in SACCDIS unsets
> >- * the relevant bit in SACCST register.
> >- 

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Nicolin Chen]

First of all, thanks a lot for the review. And I will send a v4
after I refine these comments.

But please please don't think in the way like "you can touch it
unless it's untrue." I never said anything or anyone is wrong
here. As the other patches that shortens variable names, this
patch just does something similar.

The point here is just to make the driver necessarily explained
while being brief. I am totally fine if you feel some of my new
comments are worse. I will refine it until you get satisfied.

And I hope you (or anyone else) can tell me more about what is
wrong with my new comments instead of asking me to drop them all.
I locally have more than 20 patches based on this one. Any change
I make to this one will give me a lot of trouble to rebase them.
So I am more willing to refine those that people really feel hard
to understand.

On Sat, Dec 16, 2017 at 11:15:43AM -0600, Timur Tabi wrote:
> On 12/13/17 5:18 PM, Nicolin Chen wrote:
> 
> >-/* Used when using fsl-ssi as sound-card. This is only used by ppc and
> >- * should be replaced with simple-sound-card. */
> > struct platform_device *pdev;
> 
> Is this comment no longer true?

It's moved to the comments of structure fsl_ssi. Since we defined
a pdev at the first place, we should have explained it along with
the definition.

> >-/**
> >- * fsl_ssi_isr: SSI interrupt handler
> >- *
> >- * Although it's possible to use the interrupt handler to send and receive
> >- * data to/from the SSI, we use the DMA instead.  Programming is more
> >- * complicated, but the performance is much better.
> >- *
> >- * This interrupt handler is used only to gather statistics.
> >- *
> >- * @irq: IRQ of the SSI device
> >- * @dev_id: pointer to the fsl_ssi structure for this SSI device
> >- */
> 
> What's wrong with this comment?

Nothing wrong. An interrupt handler is way too common sense in
a driver code. I am okay to retain it if you strongly feel it's
that necessary. But I would feel more plausible to clean it away.

> >- * Clear RX or TX FIFO to remove samples from the previous
> >- * stream session which may be still present in the FIFO and
> >- * may introduce bad samples and/or channel slipping.
> >- *
> >- * Note: The SOR is not documented in recent IMX datasheet, but
> >- * is described in IMX51 reference manual at section 56.3.3.15.
> >+/**
> >+ * Clear remaining data in the FIFO to avoid dirty data or channel slipping
> 
> I think the original is better, unless there's something untrue about it.

It's literally stating the same thing. And SOR register comments
are moved to the header file.

> >- * We are running on a SoC which does not support online SSI
> >- * reconfiguration, so we have to enable all necessary flags at once
> >- * even if we do not use them later (capture and playback configuration)
> >+ * Online configuration is not supported
> >+ * Enable or Disable all necessary bits at once
> 
> Ditto

The code also does disabling as well however it doesn't mention
at all. Just like you might have hard time to understand my new
comments, I also had a hard time to understand this one. So I'll
have to change it. My new comments are shorter but covers both
enable and disable. I could make it more descriptive if necessary.

> >-/*
> >- * Configure single direction units while the SSI unit is running
> >- * (online configuration)
> >- */
> >+/* Online configure single direction while SSI is running */
> 
> Ditto

It's literally the same but shorter. I don't think anyone would
have trouble to understand mine...

> >-/*
> >- * Be sure the Tx FIFO is filled when TE is set.
> >- * Otherwise, there are some chances to start the
> >- * playback with some void samples inserted first,
> >- * generating a channel slip.
> >- *
> >- * First, SSIEN must be set, to let the FIFO be filled.
> >- *
> >- * Notes:
> >- * - Limit this fix to the DMA case until FIQ cases can
> >- *   be tested.
> >- * - Limit the length of the busy loop to not lock the
> >- *   system too long, even if 1-2 loops are sufficient
> >- *   in general.
> >- */
> 
> What's wrong with this comment?

I have new comments covering necessary steps. But I could bring
some parts back if that makes you happy.

> >-/*
> >- * Note that these below aren't just normal registers.
> >- * They are a way to disable or enable bits in SACCST
> >- * register:
> >- * - writing a '1' bit at some position in SACCEN sets the
> >- * relevant bit in SACCST,
> >- * - writing a '1' bit at some position in SACCDIS unsets
> >- * the relevant bit in SACCST register.
> >- 

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/16/17 11:49 AM, Nicolin Chen wrote:

I never said that I don't agree with Timur. Every change here is
to simplify things. As long as Timur or any reviewer feels one of
new comments is harder to understand, I am totally fine to rework.
I respect everyone's opinion, but I hope everyone can respect my
effort too by telling me which one needs to rework and why?


I do respect it.  I apologize if I came across as unduly harsh.

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/16/17 11:49 AM, Nicolin Chen wrote:

I never said that I don't agree with Timur. Every change here is
to simplify things. As long as Timur or any reviewer feels one of
new comments is harder to understand, I am totally fine to rework.
I respect everyone's opinion, but I hope everyone can respect my
effort too by telling me which one needs to rework and why?


I do respect it.  I apologize if I came across as unduly harsh.

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Nicolin Chen]

On Sat, Dec 16, 2017 at 09:30:14AM -0800, Caleb Crome wrote:

> Having come to work on this driver with very little knowledge about
> kernel programming, and i.MX, I have to agree with Timur. It's an
> amazingly complex driver (with support of so many variants).  By
> eliminating verbose commentary, it's also wiping away a lot of
> knowledge.  The more sparse commentary makes things harder to
> understand for newcomers, or really anybody who isn't already steeped
> in knowledge about the SSI port and linux, and it's interaction with
> DMA.

I never said that I don't agree with Timur. Every change here is
to simplify things. As long as Timur or any reviewer feels one of
new comments is harder to understand, I am totally fine to rework.
I respect everyone's opinion, but I hope everyone can respect my
effort too by telling me which one needs to rework and why?

Thanks
Nicolin

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Nicolin Chen]

On Sat, Dec 16, 2017 at 09:30:14AM -0800, Caleb Crome wrote:

> Having come to work on this driver with very little knowledge about
> kernel programming, and i.MX, I have to agree with Timur. It's an
> amazingly complex driver (with support of so many variants).  By
> eliminating verbose commentary, it's also wiping away a lot of
> knowledge.  The more sparse commentary makes things harder to
> understand for newcomers, or really anybody who isn't already steeped
> in knowledge about the SSI port and linux, and it's interaction with
> DMA.

I never said that I don't agree with Timur. Every change here is
to simplify things. As long as Timur or any reviewer feels one of
new comments is harder to understand, I am totally fine to rework.
I respect everyone's opinion, but I hope everyone can respect my
effort too by telling me which one needs to rework and why?

Thanks
Nicolin

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/16/17 11:30 AM, Caleb Crome wrote:

Having come to work on this driver with very little knowledge about
kernel programming, and i.MX, I have to agree with Timur. It's an
amazingly complex driver (with support of so many variants).  By
eliminating verbose commentary, it's also wiping away a lot of
knowledge.  The more sparse commentary makes things harder to
understand for newcomers, or really anybody who isn't already steeped
in knowledge about the SSI port and linux, and it's interaction with
DMA.


This is exactly why I wrote the comments the way I did.  As I was 
learning about the hardware and ASoC drivers, whenever I was confused 
about something and then figured it out, I would add a comment about 
that.  I figured if it wasn't obvious to me, it wouldn't be obvious to 
anyone else.  And since this was one of the first ASoC drivers, and the 
first to use device tree, it would become a reference driver for years 
to come.  That made it even more important that I document everything I 
learned.


I am pleased that other developers kept up that commenting style.  This 
patch destroys all of that.

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/16/17 11:30 AM, Caleb Crome wrote:

Having come to work on this driver with very little knowledge about
kernel programming, and i.MX, I have to agree with Timur. It's an
amazingly complex driver (with support of so many variants).  By
eliminating verbose commentary, it's also wiping away a lot of
knowledge.  The more sparse commentary makes things harder to
understand for newcomers, or really anybody who isn't already steeped
in knowledge about the SSI port and linux, and it's interaction with
DMA.


This is exactly why I wrote the comments the way I did.  As I was 
learning about the hardware and ASoC drivers, whenever I was confused 
about something and then figured it out, I would add a comment about 
that.  I figured if it wasn't obvious to me, it wouldn't be obvious to 
anyone else.  And since this was one of the first ASoC drivers, and the 
first to use device tree, it would become a reference driver for years 
to come.  That made it even more important that I document everything I 
learned.


I am pleased that other developers kept up that commenting style.  This 
patch destroys all of that.

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Nicolin Chen]

Hi,

On Sat, Dec 16, 2017 at 10:27:06AM -0600, Timur Tabi wrote:
> On 12/16/17 12:10 AM, Nicolin Chen wrote:
> >Hi,
> >
> >I am outside so can't use mutt. Sorry for that.
> >
> >This comment is going to be replaced in the 2nd set anyway because the
> >whole function will be replaced.
> 
> So you're asking me to review comment changes that will soon be deleted?

I never said "deleted".

> Can you send out a new patch without changes to comments, so that I can
> focus on the ones that matter?

Please don't make any assumption. I am trying my best to do that.
And that's the reason why I have this patch here. I have already
done as much as I can to integrate all comments here.

Thanks
Nicolin

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Nicolin Chen]

Hi,

On Sat, Dec 16, 2017 at 10:27:06AM -0600, Timur Tabi wrote:
> On 12/16/17 12:10 AM, Nicolin Chen wrote:
> >Hi,
> >
> >I am outside so can't use mutt. Sorry for that.
> >
> >This comment is going to be replaced in the 2nd set anyway because the
> >whole function will be replaced.
> 
> So you're asking me to review comment changes that will soon be deleted?

I never said "deleted".

> Can you send out a new patch without changes to comments, so that I can
> focus on the ones that matter?

Please don't make any assumption. I am trying my best to do that.
And that's the reason why I have this patch here. I have already
done as much as I can to integrate all comments here.

Thanks
Nicolin

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Caleb Crome]

On Sat, Dec 16, 2017 at 9:15 AM, Timur Tabi  wrote:
>
> On 12/13/17 5:18 PM, Nicolin Chen wrote:
>
>> -   /* Used when using fsl-ssi as sound-card. This is only used by ppc 
>> and
>> -* should be replaced with simple-sound-card. */
>> struct platform_device *pdev;
>
>
> Is this comment no longer true?
>
>> + * 1) SSI in earlier SoCS has crtical bits in control registers that
>
>
> critical
>
>> -/**
>> - * fsl_ssi_isr: SSI interrupt handler
>> - *
>> - * Although it's possible to use the interrupt handler to send and receive
>> - * data to/from the SSI, we use the DMA instead.  Programming is more
>> - * complicated, but the performance is much better.
>> - *
>> - * This interrupt handler is used only to gather statistics.
>> - *
>> - * @irq: IRQ of the SSI device
>> - * @dev_id: pointer to the fsl_ssi structure for this SSI device
>> - */
>
>
> What's wrong with this comment?
>
>> -/*
>> - * Clear RX or TX FIFO to remove samples from the previous
>> - * stream session which may be still present in the FIFO and
>> - * may introduce bad samples and/or channel slipping.
>> - *
>> - * Note: The SOR is not documented in recent IMX datasheet, but
>> - * is described in IMX51 reference manual at section 56.3.3.15.
>> +/**
>> + * Clear remaining data in the FIFO to avoid dirty data or channel slipping
>
>
> I think the original is better, unless there's something untrue about it.
>
>> -* We are running on a SoC which does not support online SSI
>> -* reconfiguration, so we have to enable all necessary flags at once
>> -* even if we do not use them later (capture and playback 
>> configuration)
>> +* Online configuration is not supported
>> +* Enable or Disable all necessary bits at once
>
>
> Ditto
>
>> -   /*
>> -* Configure single direction units while the SSI unit is running
>> -* (online configuration)
>> -*/
>> +   /* Online configure single direction while SSI is running */
>
>
> Ditto
>
>> -   /*
>> -* Disabling the necessary flags for one of rx/tx while the
>> -* other stream is active is a little bit more difficult. We
>> -* have to disable only those flags that differ between both
>> -* streams (rx XOR tx) and that are set in the stream that is
>> -* disabled now. Otherwise we could alter flags of the other
>> -* stream
>> -*/
>> -
>> -   /* These assignments are simply vals without bits set in 
>> avals*/
>> +   /* Exclude necessary bits for the opposite stream */
>
>
> Ditto
>
>> -   /*
>> -* Be sure the Tx FIFO is filled when TE is set.
>> -* Otherwise, there are some chances to start the
>> -* playback with some void samples inserted first,
>> -* generating a channel slip.
>> -*
>> -* First, SSIEN must be set, to let the FIFO be 
>> filled.
>> -*
>> -* Notes:
>> -* - Limit this fix to the DMA case until FIQ cases 
>> can
>> -*   be tested.
>> -* - Limit the length of the busy loop to not lock 
>> the
>> -*   system too long, even if 1-2 loops are 
>> sufficient
>> -*   in general.
>> -*/
>
>
> What's wrong with this comment?
>
>> -   /*
>> -* Note that these below aren't just normal registers.
>> -* They are a way to disable or enable bits in SACCST
>> -* register:
>> -* - writing a '1' bit at some position in SACCEN sets the
>> -* relevant bit in SACCST,
>> -* - writing a '1' bit at some position in SACCDIS unsets
>> -* the relevant bit in SACCST register.
>> -*
>> -* The two writes below first disable all channels slots,
>> -* then enable just slots 3 & 4 ("PCM Playback Left Channel"
>> -* and "PCM Playback Right Channel").
>> -*/
>> +   /* Disable all channel slots */
>
>
> Ditto.
>
>
>> -* Why are we setting up SACCST everytime we are starting a
>> -* playback?
>> -* Some CODECs (like VT1613 CODEC on UDOO board) like to
>> -* (sometimes) set extra bits in their SLOTREQ requests.
>> -* When a bit is set in a SLOTREQ request then SSI sets the
>> -* relevant bit in SACCST automatically (it is enough if a bit was
>> -* set in a SLOTREQ just once, bits in SACCST are 'sticky').
>> -* If an extra slot gets enabled that's a disaster for playback
>> -* because some of normal left or right channel samples are
>> -

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Caleb Crome]

On Sat, Dec 16, 2017 at 9:15 AM, Timur Tabi  wrote:
>
> On 12/13/17 5:18 PM, Nicolin Chen wrote:
>
>> -   /* Used when using fsl-ssi as sound-card. This is only used by ppc 
>> and
>> -* should be replaced with simple-sound-card. */
>> struct platform_device *pdev;
>
>
> Is this comment no longer true?
>
>> + * 1) SSI in earlier SoCS has crtical bits in control registers that
>
>
> critical
>
>> -/**
>> - * fsl_ssi_isr: SSI interrupt handler
>> - *
>> - * Although it's possible to use the interrupt handler to send and receive
>> - * data to/from the SSI, we use the DMA instead.  Programming is more
>> - * complicated, but the performance is much better.
>> - *
>> - * This interrupt handler is used only to gather statistics.
>> - *
>> - * @irq: IRQ of the SSI device
>> - * @dev_id: pointer to the fsl_ssi structure for this SSI device
>> - */
>
>
> What's wrong with this comment?
>
>> -/*
>> - * Clear RX or TX FIFO to remove samples from the previous
>> - * stream session which may be still present in the FIFO and
>> - * may introduce bad samples and/or channel slipping.
>> - *
>> - * Note: The SOR is not documented in recent IMX datasheet, but
>> - * is described in IMX51 reference manual at section 56.3.3.15.
>> +/**
>> + * Clear remaining data in the FIFO to avoid dirty data or channel slipping
>
>
> I think the original is better, unless there's something untrue about it.
>
>> -* We are running on a SoC which does not support online SSI
>> -* reconfiguration, so we have to enable all necessary flags at once
>> -* even if we do not use them later (capture and playback 
>> configuration)
>> +* Online configuration is not supported
>> +* Enable or Disable all necessary bits at once
>
>
> Ditto
>
>> -   /*
>> -* Configure single direction units while the SSI unit is running
>> -* (online configuration)
>> -*/
>> +   /* Online configure single direction while SSI is running */
>
>
> Ditto
>
>> -   /*
>> -* Disabling the necessary flags for one of rx/tx while the
>> -* other stream is active is a little bit more difficult. We
>> -* have to disable only those flags that differ between both
>> -* streams (rx XOR tx) and that are set in the stream that is
>> -* disabled now. Otherwise we could alter flags of the other
>> -* stream
>> -*/
>> -
>> -   /* These assignments are simply vals without bits set in 
>> avals*/
>> +   /* Exclude necessary bits for the opposite stream */
>
>
> Ditto
>
>> -   /*
>> -* Be sure the Tx FIFO is filled when TE is set.
>> -* Otherwise, there are some chances to start the
>> -* playback with some void samples inserted first,
>> -* generating a channel slip.
>> -*
>> -* First, SSIEN must be set, to let the FIFO be 
>> filled.
>> -*
>> -* Notes:
>> -* - Limit this fix to the DMA case until FIQ cases 
>> can
>> -*   be tested.
>> -* - Limit the length of the busy loop to not lock 
>> the
>> -*   system too long, even if 1-2 loops are 
>> sufficient
>> -*   in general.
>> -*/
>
>
> What's wrong with this comment?
>
>> -   /*
>> -* Note that these below aren't just normal registers.
>> -* They are a way to disable or enable bits in SACCST
>> -* register:
>> -* - writing a '1' bit at some position in SACCEN sets the
>> -* relevant bit in SACCST,
>> -* - writing a '1' bit at some position in SACCDIS unsets
>> -* the relevant bit in SACCST register.
>> -*
>> -* The two writes below first disable all channels slots,
>> -* then enable just slots 3 & 4 ("PCM Playback Left Channel"
>> -* and "PCM Playback Right Channel").
>> -*/
>> +   /* Disable all channel slots */
>
>
> Ditto.
>
>
>> -* Why are we setting up SACCST everytime we are starting a
>> -* playback?
>> -* Some CODECs (like VT1613 CODEC on UDOO board) like to
>> -* (sometimes) set extra bits in their SLOTREQ requests.
>> -* When a bit is set in a SLOTREQ request then SSI sets the
>> -* relevant bit in SACCST automatically (it is enough if a bit was
>> -* set in a SLOTREQ just once, bits in SACCST are 'sticky').
>> -* If an extra slot gets enabled that's a disaster for playback
>> -* because some of normal left or right channel samples are
>> -* redirected 

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/13/17 5:18 PM, Nicolin Chen wrote:


-   /* Used when using fsl-ssi as sound-card. This is only used by ppc and
-* should be replaced with simple-sound-card. */
struct platform_device *pdev;


Is this comment no longer true?


+ * 1) SSI in earlier SoCS has crtical bits in control registers that


critical


-/**
- * fsl_ssi_isr: SSI interrupt handler
- *
- * Although it's possible to use the interrupt handler to send and receive
- * data to/from the SSI, we use the DMA instead.  Programming is more
- * complicated, but the performance is much better.
- *
- * This interrupt handler is used only to gather statistics.
- *
- * @irq: IRQ of the SSI device
- * @dev_id: pointer to the fsl_ssi structure for this SSI device
- */


What's wrong with this comment?


-/*
- * Clear RX or TX FIFO to remove samples from the previous
- * stream session which may be still present in the FIFO and
- * may introduce bad samples and/or channel slipping.
- *
- * Note: The SOR is not documented in recent IMX datasheet, but
- * is described in IMX51 reference manual at section 56.3.3.15.
+/**
+ * Clear remaining data in the FIFO to avoid dirty data or channel slipping


I think the original is better, unless there's something untrue about it.


-* We are running on a SoC which does not support online SSI
-* reconfiguration, so we have to enable all necessary flags at once
-* even if we do not use them later (capture and playback configuration)
+* Online configuration is not supported
+* Enable or Disable all necessary bits at once


Ditto


-   /*
-* Configure single direction units while the SSI unit is running
-* (online configuration)
-*/
+   /* Online configure single direction while SSI is running */


Ditto


-   /*
-* Disabling the necessary flags for one of rx/tx while the
-* other stream is active is a little bit more difficult. We
-* have to disable only those flags that differ between both
-* streams (rx XOR tx) and that are set in the stream that is
-* disabled now. Otherwise we could alter flags of the other
-* stream
-*/
-
-   /* These assignments are simply vals without bits set in avals*/
+   /* Exclude necessary bits for the opposite stream */


Ditto


-   /*
-* Be sure the Tx FIFO is filled when TE is set.
-* Otherwise, there are some chances to start the
-* playback with some void samples inserted first,
-* generating a channel slip.
-*
-* First, SSIEN must be set, to let the FIFO be filled.
-*
-* Notes:
-* - Limit this fix to the DMA case until FIQ cases can
-*   be tested.
-* - Limit the length of the busy loop to not lock the
-*   system too long, even if 1-2 loops are sufficient
-*   in general.
-*/


What's wrong with this comment?


-   /*
-* Note that these below aren't just normal registers.
-* They are a way to disable or enable bits in SACCST
-* register:
-* - writing a '1' bit at some position in SACCEN sets the
-* relevant bit in SACCST,
-* - writing a '1' bit at some position in SACCDIS unsets
-* the relevant bit in SACCST register.
-*
-* The two writes below first disable all channels slots,
-* then enable just slots 3 & 4 ("PCM Playback Left Channel"
-* and "PCM Playback Right Channel").
-*/
+   /* Disable all channel slots */


Ditto.



-* Why are we setting up SACCST everytime we are starting a
-* playback?
-* Some CODECs (like VT1613 CODEC on UDOO board) like to
-* (sometimes) set extra bits in their SLOTREQ requests.
-* When a bit is set in a SLOTREQ request then SSI sets the
-* relevant bit in SACCST automatically (it is enough if a bit was
-* set in a SLOTREQ just once, bits in SACCST are 'sticky').
-* If an extra slot gets enabled that's a disaster for playback
-* because some of normal left or right channel samples are
-* redirected instead to this extra slot.
+* SACCST might be modified via AC Link by a CODEC if it sends
+* extra bits in their SLOTREQ requests, which'll accidentally
+* send valid data to slots other than normal playback slots.
 *
-* A workaround implemented in fsl-asoc-card of setting an
-* appropriate CODEC register so that slots 3 & 4 

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/13/17 5:18 PM, Nicolin Chen wrote:


-   /* Used when using fsl-ssi as sound-card. This is only used by ppc and
-* should be replaced with simple-sound-card. */
struct platform_device *pdev;


Is this comment no longer true?


+ * 1) SSI in earlier SoCS has crtical bits in control registers that


critical


-/**
- * fsl_ssi_isr: SSI interrupt handler
- *
- * Although it's possible to use the interrupt handler to send and receive
- * data to/from the SSI, we use the DMA instead.  Programming is more
- * complicated, but the performance is much better.
- *
- * This interrupt handler is used only to gather statistics.
- *
- * @irq: IRQ of the SSI device
- * @dev_id: pointer to the fsl_ssi structure for this SSI device
- */


What's wrong with this comment?


-/*
- * Clear RX or TX FIFO to remove samples from the previous
- * stream session which may be still present in the FIFO and
- * may introduce bad samples and/or channel slipping.
- *
- * Note: The SOR is not documented in recent IMX datasheet, but
- * is described in IMX51 reference manual at section 56.3.3.15.
+/**
+ * Clear remaining data in the FIFO to avoid dirty data or channel slipping


I think the original is better, unless there's something untrue about it.


-* We are running on a SoC which does not support online SSI
-* reconfiguration, so we have to enable all necessary flags at once
-* even if we do not use them later (capture and playback configuration)
+* Online configuration is not supported
+* Enable or Disable all necessary bits at once


Ditto


-   /*
-* Configure single direction units while the SSI unit is running
-* (online configuration)
-*/
+   /* Online configure single direction while SSI is running */


Ditto


-   /*
-* Disabling the necessary flags for one of rx/tx while the
-* other stream is active is a little bit more difficult. We
-* have to disable only those flags that differ between both
-* streams (rx XOR tx) and that are set in the stream that is
-* disabled now. Otherwise we could alter flags of the other
-* stream
-*/
-
-   /* These assignments are simply vals without bits set in avals*/
+   /* Exclude necessary bits for the opposite stream */


Ditto


-   /*
-* Be sure the Tx FIFO is filled when TE is set.
-* Otherwise, there are some chances to start the
-* playback with some void samples inserted first,
-* generating a channel slip.
-*
-* First, SSIEN must be set, to let the FIFO be filled.
-*
-* Notes:
-* - Limit this fix to the DMA case until FIQ cases can
-*   be tested.
-* - Limit the length of the busy loop to not lock the
-*   system too long, even if 1-2 loops are sufficient
-*   in general.
-*/


What's wrong with this comment?


-   /*
-* Note that these below aren't just normal registers.
-* They are a way to disable or enable bits in SACCST
-* register:
-* - writing a '1' bit at some position in SACCEN sets the
-* relevant bit in SACCST,
-* - writing a '1' bit at some position in SACCDIS unsets
-* the relevant bit in SACCST register.
-*
-* The two writes below first disable all channels slots,
-* then enable just slots 3 & 4 ("PCM Playback Left Channel"
-* and "PCM Playback Right Channel").
-*/
+   /* Disable all channel slots */


Ditto.



-* Why are we setting up SACCST everytime we are starting a
-* playback?
-* Some CODECs (like VT1613 CODEC on UDOO board) like to
-* (sometimes) set extra bits in their SLOTREQ requests.
-* When a bit is set in a SLOTREQ request then SSI sets the
-* relevant bit in SACCST automatically (it is enough if a bit was
-* set in a SLOTREQ just once, bits in SACCST are 'sticky').
-* If an extra slot gets enabled that's a disaster for playback
-* because some of normal left or right channel samples are
-* redirected instead to this extra slot.
+* SACCST might be modified via AC Link by a CODEC if it sends
+* extra bits in their SLOTREQ requests, which'll accidentally
+* send valid data to slots other than normal playback slots.
 *
-* A workaround implemented in fsl-asoc-card of setting an
-* appropriate CODEC register so that slots 3 & 4 

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/16/17 12:10 AM, Nicolin Chen wrote:

Hi,

I am outside so can't use mutt. Sorry for that.

This comment is going to be replaced in the 2nd set anyway because the 
whole function will be replaced.


So you're asking me to review comment changes that will soon be deleted? 
 Can you send out a new patch without changes to comments, so that I 
can focus on the ones that matter?


And please point out all comments that you think I need to rework. I am 
totally fine to do that. I don't think every single one is bad. And this 
patch has to go in as it also adds a lot of new comments.


Ok.

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/16/17 12:10 AM, Nicolin Chen wrote:

Hi,

I am outside so can't use mutt. Sorry for that.

This comment is going to be replaced in the 2nd set anyway because the 
whole function will be replaced.


So you're asking me to review comment changes that will soon be deleted? 
 Can you send out a new patch without changes to comments, so that I 
can focus on the ones that matter?


And please point out all comments that you think I need to rework. I am 
totally fine to do that. I don't think every single one is bad. And this 
patch has to go in as it also adds a lot of new comments.


Ok.

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/13/17 5:18 PM, Nicolin Chen wrote:

-* We are running on a SoC which does not support online SSI
-* reconfiguration, so we have to enable all necessary flags at once
-* even if we do not use them later (capture and playback configuration)
+* Online configuration is not supported
+* Enable or Disable all necessary bits at once


This is an example of a bad change, IMHO.  The original was written in 
elegant prose.  The new version is just two short sentences.

Re: [PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Timur Tabi]

On 12/13/17 5:18 PM, Nicolin Chen wrote:

-* We are running on a SoC which does not support online SSI
-* reconfiguration, so we have to enable all necessary flags at once
-* even if we do not use them later (capture and playback configuration)
+* Online configuration is not supported
+* Enable or Disable all necessary bits at once


This is an example of a bad change, IMHO.  The original was written in 
elegant prose.  The new version is just two short sentences.

[PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Nicolin Chen]

This patch refines the comments by:
1) Removing all out-of-date comments
2) Removing all not-so-useful comments
3) Unifying the styles of all comments
4) Simplifying over-descriptive comments
5) Adding comments to improve code readablity
6) Moving all register related comments to fsl_ssi.h
7) Adding comments to all register and field defines

Signed-off-by: Nicolin Chen 
Tested-by: Maciej S. Szmigiero 
Reviewed-by: Maciej S. Szmigiero 
---

Changelog
v2->v3
 * Revised a comment in hw_params() by taking Maciej's advice
v1->v2
 * Added some new comments at "SoC specific data" to be more precise
 * Revised one comment in fsl_ssi_config()
 * Revised the comment of fsl_ssi_setup_reg_vals()
 * Added one comment for AC97 in fsl_ssi_setup_reg_vals()
 * Revised the comment of fsl_ssi_hw_params() to be more precise
 * Added some comments in _fsl_ssi_set_dai_fmt() to help understand
   the formats
 * Added one comment in fsl_ssi_set_dai_fmt() to explain why AC97
   needs to bypass it
 * Revised comments in fsl_ssi_set_dai_tdm_slot() to be more precise
 * Revised comments around dual FIFO code in fsl_ssi_imx_probe() to
   be more precise

 sound/soc/fsl/fsl_ssi.c | 377 +++-
 sound/soc/fsl/fsl_ssi.h |  67 +++-
 sound/soc/fsl/fsl_ssi_dbg.c |  12 +-
 3 files changed, 191 insertions(+), 265 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index e903c92..fe11a23 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -187,42 +187,48 @@ struct fsl_ssi_soc_data {
 /**
  * fsl_ssi: per-SSI private data
  *
- * @reg: Pointer to the regmap registers
+ * @regs: Pointer to the regmap registers
  * @irq: IRQ of this SSI
  * @cpu_dai_drv: CPU DAI driver for this device
  *
  * @dai_fmt: DAI configuration this device is currently used with
- * @i2s_mode: i2s and network mode configuration of the device. Is used to
- * switch between normal and i2s/network mode
- * mode depending on the number of channels
+ * @i2s_mode: I2S and Network mode configuration of SCR register
  * @use_dma: DMA is used or FIQ with stream filter
- * @use_dual_fifo: DMA with support for both FIFOs used
- * @fifo_deph: Depth of the SSI FIFOs
- * @slot_width: width of each DAI slot
- * @slots: number of slots
- * @rxtx_reg_val: Specific register settings for receive/transmit configuration
+ * @use_dual_fifo: DMA with support for dual FIFO mode
+ * @has_ipg_clk_name: If "ipg" is in the clock name list of device tree
+ * @fifo_depth: Depth of the SSI FIFOs
+ * @slot_width: Width of each DAI slot
+ * @slots: Number of slots
+ * @rxtx_reg_val: Specific RX/TX register settings
  *
- * @clk: SSI clock
- * @baudclk: SSI baud clock for master mode
+ * @clk: Clock source to access register
+ * @baudclk: Clock source to generate bit and frame-sync clocks
  * @baudclk_streams: Active streams that are using baudclk
  *
+ * @regcache_sfcsr: Cache sfcsr register value during suspend and resume
+ * @regcache_sacnt: Cache sacnt register value during suspend and resume
+ *
  * @dma_params_tx: DMA transmit parameters
  * @dma_params_rx: DMA receive parameters
  * @ssi_phys: physical address of the SSI registers
  *
  * @fiq_params: FIQ stream filtering parameters
  *
- * @pdev: Pointer to pdev used for deprecated fsl-ssi sound card
+ * @pdev: Pointer to pdev when using fsl-ssi as sound card (ppc only)
+ *TODO: Should be replaced with simple-sound-card
  *
  * @dbg_stats: Debugging statistics
  *
  * @soc: SoC specific data
+ * @dev: Pointer to >dev
+ *
+ * @fifo_watermark: The FIFO watermark setting. Notifies DMA when there are
+ *  @fifo_watermark or fewer words in TX fifo or
+ *  @fifo_watermark or more empty words in RX fifo.
+ * @dma_maxburst: Max number of words to transfer in one go. So far,
+ *this is always the same as fifo_watermark.
  *
- * @fifo_watermark: the FIFO watermark setting.  Notifies DMA when
- * there are @fifo_watermark or fewer words in TX fifo or
- * @fifo_watermark or more empty words in RX fifo.
- * @dma_maxburst: max number of words to transfer in one go.  So far,
- * this is always the same as fifo_watermark.
+ * @ac97_reg_lock: Mutex lock to serialize AC97 register access operations
  */
 struct fsl_ssi {
struct regmap *regs;
@@ -243,20 +249,15 @@ struct fsl_ssi {
struct clk *baudclk;
unsigned int baudclk_streams;
 
-   /* regcache for volatile regs */
u32 regcache_sfcsr;
u32 regcache_sacnt;
 
-   /* DMA params */
struct snd_dmaengine_dai_dma_data dma_params_tx;
struct snd_dmaengine_dai_dma_data dma_params_rx;
dma_addr_t ssi_phys;
 
-   /* params for non-dma FIQ stream filtered mode */
struct imx_pcm_fiq_params fiq_params;
 
-   /* Used when using fsl-ssi as sound-card. This is only used by ppc and
-  

[PATCH v3 03/11] ASoC: fsl_ssi: Refine all comments

[Nicolin Chen]

This patch refines the comments by:
1) Removing all out-of-date comments
2) Removing all not-so-useful comments
3) Unifying the styles of all comments
4) Simplifying over-descriptive comments
5) Adding comments to improve code readablity
6) Moving all register related comments to fsl_ssi.h
7) Adding comments to all register and field defines

Signed-off-by: Nicolin Chen 
Tested-by: Maciej S. Szmigiero 
Reviewed-by: Maciej S. Szmigiero 
---

Changelog
v2->v3
 * Revised a comment in hw_params() by taking Maciej's advice
v1->v2
 * Added some new comments at "SoC specific data" to be more precise
 * Revised one comment in fsl_ssi_config()
 * Revised the comment of fsl_ssi_setup_reg_vals()
 * Added one comment for AC97 in fsl_ssi_setup_reg_vals()
 * Revised the comment of fsl_ssi_hw_params() to be more precise
 * Added some comments in _fsl_ssi_set_dai_fmt() to help understand
   the formats
 * Added one comment in fsl_ssi_set_dai_fmt() to explain why AC97
   needs to bypass it
 * Revised comments in fsl_ssi_set_dai_tdm_slot() to be more precise
 * Revised comments around dual FIFO code in fsl_ssi_imx_probe() to
   be more precise

 sound/soc/fsl/fsl_ssi.c | 377 +++-
 sound/soc/fsl/fsl_ssi.h |  67 +++-
 sound/soc/fsl/fsl_ssi_dbg.c |  12 +-
 3 files changed, 191 insertions(+), 265 deletions(-)

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index e903c92..fe11a23 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -187,42 +187,48 @@ struct fsl_ssi_soc_data {
 /**
  * fsl_ssi: per-SSI private data
  *
- * @reg: Pointer to the regmap registers
+ * @regs: Pointer to the regmap registers
  * @irq: IRQ of this SSI
  * @cpu_dai_drv: CPU DAI driver for this device
  *
  * @dai_fmt: DAI configuration this device is currently used with
- * @i2s_mode: i2s and network mode configuration of the device. Is used to
- * switch between normal and i2s/network mode
- * mode depending on the number of channels
+ * @i2s_mode: I2S and Network mode configuration of SCR register
  * @use_dma: DMA is used or FIQ with stream filter
- * @use_dual_fifo: DMA with support for both FIFOs used
- * @fifo_deph: Depth of the SSI FIFOs
- * @slot_width: width of each DAI slot
- * @slots: number of slots
- * @rxtx_reg_val: Specific register settings for receive/transmit configuration
+ * @use_dual_fifo: DMA with support for dual FIFO mode
+ * @has_ipg_clk_name: If "ipg" is in the clock name list of device tree
+ * @fifo_depth: Depth of the SSI FIFOs
+ * @slot_width: Width of each DAI slot
+ * @slots: Number of slots
+ * @rxtx_reg_val: Specific RX/TX register settings
  *
- * @clk: SSI clock
- * @baudclk: SSI baud clock for master mode
+ * @clk: Clock source to access register
+ * @baudclk: Clock source to generate bit and frame-sync clocks
  * @baudclk_streams: Active streams that are using baudclk
  *
+ * @regcache_sfcsr: Cache sfcsr register value during suspend and resume
+ * @regcache_sacnt: Cache sacnt register value during suspend and resume
+ *
  * @dma_params_tx: DMA transmit parameters
  * @dma_params_rx: DMA receive parameters
  * @ssi_phys: physical address of the SSI registers
  *
  * @fiq_params: FIQ stream filtering parameters
  *
- * @pdev: Pointer to pdev used for deprecated fsl-ssi sound card
+ * @pdev: Pointer to pdev when using fsl-ssi as sound card (ppc only)
+ *TODO: Should be replaced with simple-sound-card
  *
  * @dbg_stats: Debugging statistics
  *
  * @soc: SoC specific data
+ * @dev: Pointer to >dev
+ *
+ * @fifo_watermark: The FIFO watermark setting. Notifies DMA when there are
+ *  @fifo_watermark or fewer words in TX fifo or
+ *  @fifo_watermark or more empty words in RX fifo.
+ * @dma_maxburst: Max number of words to transfer in one go. So far,
+ *this is always the same as fifo_watermark.
  *
- * @fifo_watermark: the FIFO watermark setting.  Notifies DMA when
- * there are @fifo_watermark or fewer words in TX fifo or
- * @fifo_watermark or more empty words in RX fifo.
- * @dma_maxburst: max number of words to transfer in one go.  So far,
- * this is always the same as fifo_watermark.
+ * @ac97_reg_lock: Mutex lock to serialize AC97 register access operations
  */
 struct fsl_ssi {
struct regmap *regs;
@@ -243,20 +249,15 @@ struct fsl_ssi {
struct clk *baudclk;
unsigned int baudclk_streams;
 
-   /* regcache for volatile regs */
u32 regcache_sfcsr;
u32 regcache_sacnt;
 
-   /* DMA params */
struct snd_dmaengine_dai_dma_data dma_params_tx;
struct snd_dmaengine_dai_dma_data dma_params_rx;
dma_addr_t ssi_phys;
 
-   /* params for non-dma FIQ stream filtered mode */
struct imx_pcm_fiq_params fiq_params;
 
-   /* Used when using fsl-ssi as sound-card. This is only used by ppc and
-* should be replaced with simple-sound-card. */
struct