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.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
