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

Noralf Trønnes noralf at tronnes.org
Tue Mar 3 11:57:29 UTC 2015


Den 02.03.2015 12:48, skrev Dan Carpenter:

[snip]

>> +	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.
Yes, I see that.
>> +/**
>> + * 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.
Ok, I thought this was a common pattern based on other functions I have 
seen.

>> +{
>> +	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).
Like this?

	ret = ctrl->rotate(ctrl, rotation);
	if (ret)
		return ret;

	ctrl->rotation = rotation;

	return 0;


>> +/**
>> + * 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.
>
Yes, they're kind on convoluted. Not suited for fast reading.


Thanks,
Noralf.



More information about the devel mailing list