[RFC 3/7] staging: fbtft: add lcd controller abstraction

Dan Carpenter dan.carpenter at oracle.com
Mon Mar 2 11:48:21 UTC 2015


On Mon, Mar 02, 2015 at 11:54:25AM +0100, Noralf Trønnes wrote:
> +int lcdctrl_enable(struct lcdctrl *ctrl, void *videomem)
> +{
> +	struct lcdctrl_update update = {
> +		.base = videomem,
> +		.ys = 0,
> +		.ye = lcdctrl_yres(ctrl) - 1,
> +	};
> +	int ret;
> +
> +	if (WARN_ON_ONCE(!ctrl->update))
> +		return -EINVAL;
> +
> +	if (ctrl->initialized)
> +		return 0;
> +
> +	lcdreg_lock(ctrl->lcdreg);
> +
> +	if (ctrl->power_supply) {
> +		ret = regulator_enable(ctrl->power_supply);
> +		if (ret) {
> +			dev_err(ctrl->lcdreg->dev,
> +				"failed to enable power supply: %d\n", ret);
> +			goto enable_failed;

This kind of label naming where the label name is based on the function
which failed is a common anti-pattern.

> +		}
> +	}
> +	if (ctrl->poweron) {
> +		ret = ctrl->poweron(ctrl);
> +		if (ret)
> +			goto enable_failed;

It means that the other gotos don't make any sense.  It's better to
pick the label name based on the label location like err_unlock,
out_unlock.

> +/**
> + * Caller is responsible for locking.
> + */
> +int _lcdctrl_rotate(struct lcdctrl *ctrl, u32 rotation)

Why the underscore?  I assume it's because of the locking.  Just
documentating it is sufficient, no need for the underscore.

> +{
> +	int ret;
> +
> +	if (!ctrl->rotate)
> +		return -ENOSYS;
> +
> +	switch (rotation) {
> +	case 0:
> +	case 90:
> +	case 180:
> +	case 270:
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = ctrl->rotate(ctrl, rotation);
> +	if (!ret)
> +		ctrl->rotation = rotation;
> +
> +	return ret;

Better to check "if (ret)" consistently (error handling vs success
handling).

> +}

> +/**
> + * Description of a display update
> + *
> + * @base: Base address of video memory
> + * @ys: Horizontal line to start the transfer from (zero based)
> + * @ye: Last line to transfer (inclusive)
> + */
> +struct lcdctrl_update {
> +	void *base;
> +	u32 ys;
> +	u32 ye;

"ys" and "ye" are easy to mix up when you're reading the code.  They
are not especially self documenting.  Maybe use y_start and y_end.


regards,
dan carpenter


More information about the devel mailing list