From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jungo Lin Subject: Re: [RFC PATCH V0 7/7] [media] platform: mtk-isp: Add Mediatek ISP Pass 1 driver Date: Fri, 22 Mar 2019 08:00:45 +0800 Message-ID: <1553212845.7066.3.camel@mtksdccf07> References: <1549348966-14451-1-git-send-email-frederic.chen@mediatek.com> <1549348966-14451-8-git-send-email-frederic.chen@mediatek.com> <1550372163.11724.59.camel@mtksdccf07> <1550647867.11724.80.camel@mtksdccf07> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Tomasz Figa Cc: Ryan Yu =?UTF-8?Q?=28=E4=BD=99=E5=AD=9F=E4=BF=AE=29?= , Frankie Chiu =?UTF-8?Q?=28=E9=82=B1=E6=96=87=E5=87=B1=29?= , ryan_yu@mediatek.com, Laurent Pinchart , Rynn Wu =?UTF-8?Q?=28=E5=90=B3=E8=82=B2=E6=81=A9=29?= , Jerry-ch Chen , Hans Verkuil , frankie_chiu@mediatek.com, Frederic Chen , Seraph Huang =?UTF-8?Q?=28=E9=BB=83=E5=9C=8B=E9=9B=84=29?= , Linux Media Mailing List , Holmes Chiou =?UTF-8?Q?=28=E9=82=B1=E6=8C=BA=29?= , seraph_huang@mediatek.com, Sj Huang , yuzhao@chromium.org, linux-mediatek@lists.infradead.org, Matthias Brugger , Mauro List-Id: linux-mediatek@lists.infradead.org On Thu, 2019-03-21 at 12:33 +0900, Tomasz Figa wrote: > On Wed, Feb 20, 2019 at 4:31 PM Jungo Lin wrote: > > > > On Tue, 2019-02-19 at 17:51 +0900, Tomasz Figa wrote: > > > Hi Jungo, > > > > > > On Sun, Feb 17, 2019 at 11:56 AM Jungo Lin wrote: > > > > > > > > On Wed, 2019-02-13 at 18:50 +0900, Tomasz Figa wrote: > > > > > (() . ( strHi Frederic, Jungo, > > > > > > > > > > On Tue, Feb 5, 2019 at 3:43 PM Frederic Chen wrote: > > > > > > > > > > > > From: Jungo Lin > > > > > > > > > > > > This patch adds the driver for Pass unit in Mediatek's camera > > > > > > ISP system. Pass 1 unit is embedded in Mediatek SOCs. It > > > > > > provides RAW processing which includes optical black correction, > > > > > > defect pixel correction, W/IR imbalance correction and lens > > > > > > shading correction. > > > > > > > > > > > > The mtk-isp directory will contain drivers for multiple IP > > > > > > blocks found in Mediatek ISP system. It will include ISP Pass 1 > > > > > > driver, sensor interface driver, DIP driver and face detection > > > > > > driver. > > > > > > > > > > Thanks for the patches! Please see my comments inline. > > > > > > > > > > > > > Dear Thomas: > > > > > > > > Thanks for your comments. > > > > > > > > We will revise the source codes based on your comments. > > > > Since there are many comments to fix/revise, we will categorize & > > > > prioritize these with below list: > > > > > > > > 1. Coding style issues. > > > > 2. Coding defects, including unused codes. > > > > 3. Driver architecture refactoring, such as removing mtk_cam_ctx, > > > > unnecessary abstraction layer, etc. > > > > > > > > > > Thanks for replying to the comments! > > > > > > Just to clarify, there is no need to hurry with resending a next patch > > > set with only a subset of the changes. Please take your time to > > > address all the comments before sending the next revision. This > > > prevents forgetting about some remaining comments and also lets other > > > reviewers come with new comments or some alternative ideas for already > > > existing comments. Second part of my review is coming too. > > > > > > P.S. Please avoid top-posting on mailing lists. If replying to a > > > message, please reply below the related part of that message. (I've > > > moved your reply to the place in the email where it should be.) > > > > > > [snip] > > > > Hi, Tomasz, > > > > Thanks for your advice. > > We will prepare the next patch set after all comments are revised. > > > > > > > > > > + phys_addr_t paddr; > > > > > > > > > > We shouldn't need physical address either. I suppose this is for the > > > > > SCP, but then it should be a DMA address obtained from dma_map_*() > > > > > with struct device pointer of the SCP. > > > > > > > > > > > > > Yes, this physical address is designed for SCP. > > > > For tuning buffer, it will be touched by SCP and > > > > SCP can't get the physical address by itself. So we need to get > > > > this physical address in the kernel space via mtk_cam_smem_iova_to_phys > > > > function call and pass it to the SCP. At the same time, DMA address > > > > (daddr) is used for ISP HW. > > > > > > > > > > Right, but my point is that in the kernel phys_addr_t is the physical > > > address from the CPU point of view. Any address from device point of > > > view is dma_addr_t and should be obtained from the DMA mapping API > > > (even if it's numerically the same as physical address). > > > > > > > OK. > > In order to clarify the address usage, is it ok to rename "dma_addr_t > > scp_addr"? > > Sounds good to me. > > > Moreover, below function will be also renamed. > > dma_addr_t mtk_cam_smem_iova_to_scp_phys(struct device *dev, > > dma_addr_t iova) > > Perhaps mtk_cam_smem_iova_to_scp_addr() for consistency with the > struct field above? Ok, we will align the function name to struct field. This fix will be included in v1 version. Thanks for your comments. Best regards, Jungo