From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751270AbbEYHLi (ORCPT ); Mon, 25 May 2015 03:11:38 -0400 Received: from hqemgate14.nvidia.com ([216.228.121.143]:2839 "EHLO hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750839AbbEYHLg (ORCPT ); Mon, 25 May 2015 03:11:36 -0400 X-PGP-Universal: processed; by hqnvupgp08.nvidia.com on Mon, 25 May 2015 00:08:26 -0700 Message-ID: <5562CB20.4060700@nvidia.com> Date: Mon, 25 May 2015 10:11:28 +0300 From: Arto Merilainen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Thierry Reding , Mikko Perttunen CC: , , , , , Subject: Re: [PATCH 3/4] drm/tegra: Add VIC support References: <1432214425-27137-1-git-send-email-amerilainen@nvidia.com> <1432214425-27137-4-git-send-email-amerilainen@nvidia.com> <555DEE5F.2060100@nvidia.com> <20150522102528.GD16507@ulmo> In-Reply-To: <20150522102528.GD16507@ulmo> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.21.26.102] X-ClientProxiedBy: UKMAIL102.nvidia.com (10.26.138.15) To UKMAIL101.nvidia.com (10.26.138.13) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/22/2015 01:25 PM, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Thu, May 21, 2015 at 05:40:31PM +0300, Mikko Perttunen wrote: >> On 05/21/2015 04:20 PM, Arto Merilainen wrote: > [...] >>> +static int vic_is_addr_reg(struct device *dev, u32 class, u32 offset, u32 val) >>> +{ >>> + struct vic *vic = dev_get_drvdata(dev); >>> + >>> + /* handle host class */ >>> + if (class == HOST1X_CLASS_HOST1X) { >>> + if (offset == 0x2b) >>> + return true; >>> + return false; >> >> "return (offset == 0x2b);" perhaps? > > I think this should really be extracted into a separate helper. If we > ever need to take into account additional offsets we would otherwise > have to extend every driver rather than just the helper. I agree, that would be better. > > Also I think the 0x2b should be replaced by some symbolic name. > According to the TRM 0x2b is the host1x class method named > NV_CLASS_HOST_INDCTRL_0. Oddly enough that doesn't seem to be an address > register. Instead the address seems to be in the INDOFF2 and INDOFF > methods (0x2c and 0x2d). I also can't tell from the TRM what exactly > these are supposed to do. > > Arto, can you clarify? This looks like an unfortunate mistake that got reproduced from gr2d and gr3d. The INDCTRL method is used for indirect register accessing and it allows Host1x to read registers of an engine - or write data directly to memory. It allow implementing context switch for the clients whose state should be not change between jobs from the same application. > >>> + if (IS_ERR(vic->rst)) { >>> + dev_err(&pdev->dev, "cannot get reset\n"); >>> + return PTR_ERR(vic->rst); >>> + } >>> + >>> + platform_set_drvdata(pdev, vic); >>> + >>> + INIT_LIST_HEAD(&vic->client.base.list); >>> + vic->client.base.ops = &vic_client_ops; >>> + vic->client.base.dev = dev; >>> + vic->client.base.class = vic_config->class_id; >>> + vic->client.base.syncpts = syncpts; >>> + vic->client.base.num_syncpts = 1; >>> + vic->dev = dev; >>> + vic->config = vic_config; >>> + >>> + INIT_LIST_HEAD(&vic->client.list); >>> + vic->client.ops = &vic_ops; >>> + >>> + err = tegra_powergate_sequence_power_up(vic->config->powergate_id, >>> + vic->clk, vic->rst); >>> + if (err) { >>> + dev_err(dev, "cannot turn on the device\n"); >>> + return err; >>> + } >>> + >>> + err = host1x_client_register(&vic->client.base); >>> + if (err < 0) { >> >> You used 'if (err) {' previously, so maybe also here. > > For consistency with other Tegra DRM code these checks should use (at > least where possible) the (err < 0) notation. > Will fix. - Arto