From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751256AbdBLLFG (ORCPT ); Sun, 12 Feb 2017 06:05:06 -0500 Received: from mail-qk0-f194.google.com ([209.85.220.194]:34390 "EHLO mail-qk0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751120AbdBLLFE (ORCPT ); Sun, 12 Feb 2017 06:05:04 -0500 MIME-Version: 1.0 In-Reply-To: <20170211184858.26421-2-noralf@tronnes.org> References: <20170211184858.26421-1-noralf@tronnes.org> <20170211184858.26421-2-noralf@tronnes.org> From: Andy Shevchenko Date: Sun, 12 Feb 2017 13:05:03 +0200 Message-ID: Subject: Re: [PATCH v4 1/7] drm: Add DRM support for tiny LCD displays To: =?UTF-8?Q?Noralf_Tr=C3=B8nnes?= Cc: dri-devel@lists.freedesktop.org, devicetree , robh@kernel.org, Thomas Petazzoni , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v1CB5Mh9032051 On Sat, Feb 11, 2017 at 8:48 PM, Noralf Trønnes wrote: > tinydrm provides helpers for very simple displays that can use > CMA backed framebuffers and need flushing on changes. > +/** > + * tinydrm_gem_cma_free_object - Free resources associated with a CMA GEM > + * object Keep on one line? > +const struct file_operations tinydrm_fops = { > + .owner = THIS_MODULE, Do we still need this? > +static int tinydrm_init(struct device *parent, struct tinydrm_device *tdev, > + const struct drm_framebuffer_funcs *fb_funcs, > + struct drm_driver *driver) > +{ > + struct drm_device *drm; > + > + mutex_init(&tdev->dirty_lock); > + tdev->fb_funcs = fb_funcs; > + > + /* > + * We don't embed drm_device, because that prevent us from using > + * devm_kzalloc() to allocate tinydrm_device in the driver since > + * drm_dev_unref() frees the structure. The devm_ functions provide "devm_ functions" -> "devm_kzalloc()" ? Otherwise what else do you refer to and why here? > + * for easy error handling. > + */ > +static int tinydrm_register(struct tinydrm_device *tdev) > +{ > + struct drm_device *drm = tdev->drm; > + int bpp = drm->mode_config.preferred_depth; > + struct drm_fbdev_cma *fbdev; > + int ret; > + > + ret = drm_dev_register(tdev->drm, 0); > + if (ret) > + return ret; > + > + fbdev = drm_fbdev_cma_init_with_funcs(drm, bpp ? bpp : 32, > + drm->mode_config.num_connector, > + tdev->fb_funcs); > + if (IS_ERR(fbdev)) > + DRM_ERROR("Failed to initialize fbdev: %ld\n", PTR_ERR(fbdev)); > + else > + tdev->fbdev_cma = fbdev; Apparently I missed previous round of reviews, can you just put in short words why error is not propagated here to the caller? > + > + return 0; > +} > +/** > + * tinydrm_display_pipe_init - Initialize display pipe > + * @tdev: tinydrm device > + * @funcs: Display pipe functions > + * @connector_type: Connector type > + * @formats: Array of supported formats (DRM_FORMAT\_\*) DRM_FORMAT_* ? > + * @format_count: Number of elements in @formats > + * @mode: Supported mode > + * @rotation: Initial @mode rotation in degrees Counter Clock Wise > + * > + * This function sets up a &drm_simple_display_pipe with a &drm_connector that > + * has one fixed &drm_display_mode which is rotated according to @rotation. > + * > + * Returns: > + * Zero on success, negative error code on failure. > + */ > +{ > + struct drm_device *drm = tdev->drm; > + struct drm_display_mode *mode_copy; > + struct drm_connector *connector; > + int ret; > + connector = tinydrm_connector_create(drm, mode_copy, connector_type); > + if (IS_ERR(connector)) > + return PTR_ERR(connector); > + > + ret = drm_simple_display_pipe_init(drm, &tdev->pipe, funcs, formats, > + format_count, connector); > + if (ret) > + return ret; > + > + return 0; return drm_... ? > +} > +EXPORT_SYMBOL(tinydrm_display_pipe_init); -- With Best Regards, Andy Shevchenko