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.
Attachment:
signature.asc
Description: PGP signature
