[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
