On Mon, Nov 20, 2023 at 02:17:34PM +0000, Maciej Strozek wrote:
- if (cs43130->has_irq_line) {
ret = wait_for_completion_timeout(to_poll, msecs_to_jiffies(time));- } else {
If you just put a return here then you don't need the else and can reduce the intentation level of the rest of the function, making it more legible.
if (to_poll == &cs43130->xtal_rdy) {offset = 0;flag = CS43130_XTAL_RDY_INT;} else if (to_poll == &cs43130->pll_rdy) {offset = 0;flag = CS43130_PLL_RDY_INT;} else if (to_poll == &cs43130->hpload_evt) {offset = 3;flag = CS43130_HPLOAD_NO_DC_INT | CS43130_HPLOAD_UNPLUG_INT |CS43130_HPLOAD_OOR_INT | CS43130_HPLOAD_AC_INT |CS43130_HPLOAD_DC_INT | CS43130_HPLOAD_ON_INT |CS43130_HPLOAD_OFF_INT;} else {return 0;}
Is it a bug to call this function without to_poll set to something known? This will just silently ignore it which seems wrong and is inconsitent with the handling in the interrupt case which will wait for the the completion to be signalled and report a timeout on error.
@@ -2545,8 +2579,10 @@ static int cs43130_i2c_probe(struct i2c_client *client) IRQF_ONESHOT | IRQF_TRIGGER_LOW, "cs43130", cs43130); if (ret != 0) {
dev_err(cs43130->dev, "Failed to request IRQ: %d\n", ret);goto err;
dev_dbg(cs43130->dev, "Failed to request IRQ: %d, will poll instead\n", ret);cs43130->has_irq_line = 0;- } else {
cs43130->has_irq_line = 1;
This will treat probe deferral as the interrupt not being supplied, and will squash even real errors silently - it should probably check for both the specific error the clock API returns when no clock is provided by the firmware and probe deferral and handle both specifically.