Re: [PATCH v4 1/2] media: i2c: adv748x: add adv748x driver

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

[snip]

On Monday 26 Jun 2017 16:14:47 Kieran Bingham wrote:
> >> +int adv748x_txa_power(struct adv748x_state *state, bool on)
> >> +{
> >> +    int val;
> >> +
> >> +    val = txa_read(state, ADV748X_CSI_FS_AS_LS);
> >> +    if (val < 0)
> >> +        return val;
> >> +
> >> +    /*
> >> +     * This test against BIT(6) is not documented by the datasheet, but
> >> was +     * specified in the downstream driver.
> >> +     * Track with a WARN_ONCE to determine if it is ever set by HW.
> >> +     */
> >> +    WARN_ONCE((on && val & ADV748X_CSI_FS_AS_LS_UNKNOWN),
> >> +            "Enabling with unknown bit set");
> >> +
> >> +    if (on)
> >> +        return adv748x_write_regs(state, adv748x_power_up_txa_4lane);
> >> +    else
> >
> > 'else' isn't needed.
> 
> That's a shame - I think the code is more elegant (/symmetrical) this way -
> but no worries.
> Adapted. (same for the others)

For what it's worth, I would personally have kept the else here. I'm all for

	if (simple_case) {
		handle_simple_case();
		return 0;
	}

	/* Complex case */

or similar constructs with s/simple_case/uncommon_case/ or 
s/simple_case/error_case/, but here the two branches are small and symmetric, 
so an else makes sense to me to highlight that symmetry.

> >> +        return adv748x_write_regs(state, adv748x_power_down_txa_4lane);
> >> +}

-- 
Regards,

Laurent Pinchart