[PATCH v1 1/4] ALSA: hda/cirrus: Add error handling into CS8409 I2C functions

2021-03-13 Thread Vitaly Rodionov 
From: Stefan Binding 

Tested on DELL Inspiron-3505, DELL Inspiron-3501, DELL Inspiron-3500

Signed-off-by: Stefan Binding 
Signed-off-by: Vitaly Rodionov 

Changes in v1:
- No changes

---
 sound/pci/hda/patch_cirrus.c | 95 +---
 1 file changed, 56 insertions(+), 39 deletions(-)

diff --git a/sound/pci/hda/patch_cirrus.c b/sound/pci/hda/patch_cirrus.c
index 6a9e5c803977..ca8b522b1d6d 100644
--- a/sound/pci/hda/patch_cirrus.c
+++ b/sound/pci/hda/patch_cirrus.c
@@ -1508,7 +1508,7 @@ static void cs8409_enable_i2c_clock(struct hda_codec 
*codec, unsigned int flag)
 static int cs8409_i2c_wait_complete(struct hda_codec *codec)
 {
int repeat = 5;
-   unsigned int retval = 0;
+   unsigned int retval;
 
do {
retval = cs_vendor_coef_get(codec, CIR_I2C_STATUS);
@@ -1520,78 +1520,82 @@ static int cs8409_i2c_wait_complete(struct hda_codec 
*codec)
 
} while (repeat);
 
-   return repeat > 0 ? 0 : -1;
+   return !!repeat;
 }
 
-/* CS8409 slave i2cRead */
-static unsigned int cs8409_i2c_read(struct hda_codec *codec,
+/* CS8409 slave i2cRead.
+ * Returns negative on error, otherwise returns read value in bits 0-7.
+ */
+static int cs8409_i2c_read(struct hda_codec *codec,
unsigned int i2c_address,
unsigned int i2c_reg,
unsigned int paged)
 {
unsigned int i2c_reg_data;
-   unsigned int retval = 0;
+   unsigned int read_data;
 
cs8409_enable_i2c_clock(codec, 1);
cs_vendor_coef_set(codec, CIR_I2C_ADDR, i2c_address);
 
if (paged) {
cs_vendor_coef_set(codec, CIR_I2C_QWRITE, i2c_reg >> 8);
-   if (cs8409_i2c_wait_complete(codec) == -1) {
+   if (!cs8409_i2c_wait_complete(codec)) {
codec_err(codec,
-   "%s() Paged Transaction Failed 0x%02x : 0x%04x 
= 0x%02x\n",
-   __func__, i2c_address, i2c_reg, retval);
+   "%s() Paged Transaction Failed 0x%02x : 
0x%04x\n",
+   __func__, i2c_address, i2c_reg);
+   return -EIO;
}
}
 
i2c_reg_data = (i2c_reg << 8) & 0x0;
cs_vendor_coef_set(codec, CIR_I2C_QREAD, i2c_reg_data);
-   if (cs8409_i2c_wait_complete(codec) == -1) {
-   codec_err(codec, "%s() Transaction Failed 0x%02x : 0x%04x = 
0x%02x\n",
-   __func__, i2c_address, i2c_reg, retval);
+   if (!cs8409_i2c_wait_complete(codec)) {
+   codec_err(codec, "%s() Transaction Failed 0x%02x : 0x%04x\n",
+   __func__, i2c_address, i2c_reg);
+   return -EIO;
}
 
/* Register in bits 15-8 and the data in 7-0 */
-   retval = cs_vendor_coef_get(codec, CIR_I2C_QREAD);
-   retval &= 0x0ff;
+   read_data = cs_vendor_coef_get(codec, CIR_I2C_QREAD);
 
cs8409_enable_i2c_clock(codec, 0);
 
-   return retval;
+   return read_data & 0x0ff;
 }
 
 /* CS8409 slave i2cWrite */
-static unsigned int cs8409_i2c_write(struct hda_codec *codec,
+static int cs8409_i2c_write(struct hda_codec *codec,
unsigned int i2c_address, unsigned int i2c_reg,
unsigned int i2c_data,
unsigned int paged)
 {
-   unsigned int retval = 0;
-   unsigned int i2c_reg_data = 0;
+   unsigned int i2c_reg_data;
 
cs8409_enable_i2c_clock(codec, 1);
cs_vendor_coef_set(codec, CIR_I2C_ADDR, i2c_address);
 
if (paged) {
cs_vendor_coef_set(codec, CIR_I2C_QWRITE, i2c_reg >> 8);
-   if (cs8409_i2c_wait_complete(codec) == -1) {
+   if (!cs8409_i2c_wait_complete(codec)) {
codec_err(codec,
-   "%s() Paged Transaction Failed 0x%02x : 0x%04x 
= 0x%02x\n",
-   __func__, i2c_address, i2c_reg, retval);
+   "%s() Paged Transaction Failed 0x%02x : 
0x%04x\n",
+   __func__, i2c_address, i2c_reg);
+   return -EIO;
}
}
 
i2c_reg_data = ((i2c_reg << 8) & 0x0ff00) | (i2c_data & 0x0ff);
cs_vendor_coef_set(codec, CIR_I2C_QWRITE, i2c_reg_data);
 
-   if (cs8409_i2c_wait_complete(codec) == -1) {
-   codec_err(codec, "%s() Transaction Failed 0x%02x : 0x%04x = 
0x%02x\n",
-   __func__, i2c_address, i2c_reg, retval);
+   if (!cs8409_i2c_wait_complete(codec)) {
+   codec_err(codec, "%s() Transaction Failed 0x%02x : 0x%04x\n",
+   __func__, i2c_address, i2c_reg);
+   return -EIO;
}
 
cs8409_enable_i2c_clock(codec, 0);
 
-   return retval;
+   return 0;
 }
 
 static int cs8409_cs42l42_volume_info(struct snd_kcontrol *kcontrol,
@@ -1624,14 +1628,27 @@ static int cs8409_cs42l42_vo

Re: [PATCH v1 1/4] ALSA: hda/cirrus: Add error handling into CS8409 I2C functions

2021-03-15 Thread Vitaly Rodionov 

On 15/03/2021 7:45 am, Takashi Iwai wrote:

Hi Takashi,
Thanks a lot for your comments.


On Sat, 13 Mar 2021 12:34:07 +0100,
Vitaly Rodionov wrote:

@@ -1508,7 +1508,7 @@ static void cs8409_enable_i2c_clock(struct hda_codec 
*codec, unsigned int flag)
  static int cs8409_i2c_wait_complete(struct hda_codec *codec)
  {
int repeat = 5;
-   unsigned int retval = 0;
+   unsigned int retval;
  
  	do {

retval = cs_vendor_coef_get(codec, CIR_I2C_STATUS);
@@ -1520,78 +1520,82 @@ static int cs8409_i2c_wait_complete(struct hda_codec 
*codec)
  
  	} while (repeat);
  
-	return repeat > 0 ? 0 : -1;

+   return !!repeat;
  }

If the return value of the function has changed, it's nicer to
comment, e.g. a brief function description would be helpful.
Also now this looks rather like a bool?

Yes, agreed , we will add comments to describe parameters and return values




@@ -1881,13 +1896,15 @@ static void cs8409_jack_unsol_event(struct hda_codec 
*codec, unsigned int res)
reg_hs_status = cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1124, 1);
reg_ts_status = cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x130f, 1);
  
-	/* Clear interrupts */

+   /* Clear interrupts, by reading interrupt status registers */
cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1b7b, 1);
-   cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1308, 1);
-   cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x130f, 1);

Why those two calls are removed?
This 2 call are redundant as we already did read these 2 registers in a 
code few lines above.



mutex_unlock(&spec->cs8409_i2c_mux);
  
+	/* If status values are < 0, read error has occurred. */

+   if ((reg_cdc_status < 0) || (reg_hs_status < 0) || (reg_ts_status < 0))
+   return;

Parentheses around the comparison are superfluous, you can remove
them.

Will fix.


thanks,

Takashi

Re: [PATCH v1 1/4] ALSA: hda/cirrus: Add error handling into CS8409 I2C functions

2021-03-15 Thread Takashi Iwai 
On Mon, 15 Mar 2021 16:37:32 +0100,
Vitaly Rodionov wrote:
> 
> >> @@ -1881,13 +1896,15 @@ static void cs8409_jack_unsol_event(struct 
> >> hda_codec *codec, unsigned int res)
> >>reg_hs_status = cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1124, 1);
> >>reg_ts_status = cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x130f, 1);
> >>   -/* Clear interrupts */
> >> +  /* Clear interrupts, by reading interrupt status registers */
> >>cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1b7b, 1);
> >> -  cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1308, 1);
> >> -  cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x130f, 1);
> > Why those two calls are removed?
> This 2 call are redundant as we already did read these 2 registers in
> a code few lines above.

OK, then please either split the patch to address this or mention the
change briefly in the commit log.


thanks,

Takashi

Re: [PATCH v1 1/4] ALSA: hda/cirrus: Add error handling into CS8409 I2C functions

2021-03-15 Thread Takashi Iwai 
On Sat, 13 Mar 2021 12:34:07 +0100,
Vitaly Rodionov wrote:
> 
> @@ -1508,7 +1508,7 @@ static void cs8409_enable_i2c_clock(struct hda_codec 
> *codec, unsigned int flag)
>  static int cs8409_i2c_wait_complete(struct hda_codec *codec)
>  {
>   int repeat = 5;
> - unsigned int retval = 0;
> + unsigned int retval;
>  
>   do {
>   retval = cs_vendor_coef_get(codec, CIR_I2C_STATUS);
> @@ -1520,78 +1520,82 @@ static int cs8409_i2c_wait_complete(struct hda_codec 
> *codec)
>  
>   } while (repeat);
>  
> - return repeat > 0 ? 0 : -1;
> + return !!repeat;
>  }

If the return value of the function has changed, it's nicer to
comment, e.g. a brief function description would be helpful.
Also now this looks rather like a bool?


> @@ -1881,13 +1896,15 @@ static void cs8409_jack_unsol_event(struct hda_codec 
> *codec, unsigned int res)
>   reg_hs_status = cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1124, 1);
>   reg_ts_status = cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x130f, 1);
>  
> - /* Clear interrupts */
> + /* Clear interrupts, by reading interrupt status registers */
>   cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1b7b, 1);
> - cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x1308, 1);
> - cs8409_i2c_read(codec, CS42L42_I2C_ADDR, 0x130f, 1);

Why those two calls are removed?

>   mutex_unlock(&spec->cs8409_i2c_mux);
>  
> + /* If status values are < 0, read error has occurred. */
> + if ((reg_cdc_status < 0) || (reg_hs_status < 0) || (reg_ts_status < 0))
> + return;

Parentheses around the comparison are superfluous, you can remove
them.


thanks,

Takashi