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