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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <12087293.ibczxtmbyi@avalon>
Date:   Mon, 26 Jun 2017 19:00:13 +0300
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     kieran.bingham@...asonboard.com
Cc:     Hans Verkuil <hverkuil@...all.nl>,
        Kieran Bingham <kbingham@...nel.org>,
        linux-media@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        linux-kernel@...r.kernel.org, sakari.ailus@....fi,
        niklas.soderlund@...natech.se
Subject: Re: [PATCH v4 1/2] media: i2c: adv748x: add adv748x driver

[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