[PATCH v1 1/4] ALSA: hda/cirrus: Add error handling into CS8409 I2C functions
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
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
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
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