Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965402AbdIYXHH (ORCPT ); Mon, 25 Sep 2017 19:07:07 -0400 Received: from avon.wwwdotorg.org ([104.237.132.123]:37578 "EHLO avon.wwwdotorg.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934948AbdIYXHF (ORCPT ); Mon, 25 Sep 2017 19:07:05 -0400 X-Greylist: delayed 338 seconds by postgrey-1.27 at vger.kernel.org; Mon, 25 Sep 2017 19:07:05 EDT Subject: Re: [PATCH v1 1/2] staging: Introduce NVIDIA Tegra20 video decoder driver To: Dmitry Osipenko Cc: Thierry Reding , Jonathan Hunter , Greg Kroah-Hartman , Rob Herring , linux-tegra@vger.kernel.org, devel@driverdev.osuosl.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org References: <5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com> From: Stephen Warren Message-ID: <38daee0d-0595-a7e4-69bb-5a2ddbd832b5@wwwdotorg.org> Date: Mon, 25 Sep 2017 17:01:23 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <5c8b83775b982e6ee851c127444a8e839f422ad0.1506377430.git.digetx@gmail.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1312 Lines: 32 On 09/25/2017 04:15 PM, Dmitry Osipenko wrote: > Video decoder, found on NVIDIA Tegra20 SoC, supports a standard set of > video formats like H.264 / MPEG-4 / WMV / VC1. Currently driver supports > decoding of CAVLC H.264 only. Note: I don't know anything much about video decoding on Tegra (just NV desktop GPUs, and that was a while ago), but I had a couple small comments on the DT binding: > diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-vde.txt > +NVIDIA Tegra Video Decoder Engine > + > +Required properties: > +- compatible : "nvidia,tegra20-vde" > +- reg : Must contain 2 register ranges: registers and IRAM area. > +- reg-names : Must include the following entries: > + - regs > + - iram I think the IRAM region needs more explanation: What is the region used for and by what? Can it be moved, and if so does the move need to be co-ordinated with any other piece of SW? > +- clocks : Must contain one entry, for the module clock. > + See ../clocks/clock-bindings.txt for details. > +- resets : Must contain an entry for each entry in reset-names. > + See ../reset/reset.txt for details. > +- reset-names : Must include the following entries: > + - vde Let's require a clock-names property too.