From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pl1-f172.google.com (mail-pl1-f172.google.com [209.85.214.172]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4B3F52868B for ; Sun, 21 Sep 2025 20:23:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.214.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1758486224; cv=none; b=ioJdT/XgamweRzsRPQJB9BpWsEZjIQqi3A54/sw+bQy6/66PQpyaNzSOH4EDcOR+1qzz/+TBoP0LchlOgpdpX9jhLaX9yKlXbduHU2U9FN0LXZPIYX1lhMbpuDhIETOmNg74r+PdE9I1ZHP4JLi/TYZ3xJBifZDIn9gVbZzagCQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1758486224; c=relaxed/simple; bh=3Qky9KnjohG7xWAKypKH4N+8pnVS98dgtgaL7zUuGc8=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=WtqBM6/ovL0jA1iNBkK7gTRgro9+JEiE4LmD/tjPDJGIX1E6kkylin5aN//I1OoShe0zjBJwwzlfj0N7PJOPrwts/I6GIL4sNBBIyS9nFx6jhiYgOSQ9fYxNFr8msJ1IX1rh0OGq4mIqOj1IESGoRf/QMsV4SY9Ff0FAO8zpVJA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=kerneltoast.com; spf=pass smtp.mailfrom=kerneltoast.com; dkim=pass (2048-bit key) header.d=kerneltoast.com header.i=@kerneltoast.com header.b=FuCV2l7/; arc=none smtp.client-ip=209.85.214.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=kerneltoast.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=kerneltoast.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kerneltoast.com header.i=@kerneltoast.com header.b="FuCV2l7/" Received: by mail-pl1-f172.google.com with SMTP id d9443c01a7336-269af38418aso37335995ad.1 for ; Sun, 21 Sep 2025 13:23:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kerneltoast.com; s=google; t=1758486221; x=1759091021; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=0JZxIzAartu9ONh15YXkN5AwTrZHGKB1VwmY0Xneo7c=; b=FuCV2l7/fhbvPuT91orPhRHTdISCVlsQ17ElyQr5Osw++PxRc86QuK7e/casedh8kT 2n8uz/Ngl8P278NY4B9McAfp+Hg3AFDYKyzYKd2Ba6D4bqr00L/tHMCUbZ6DGcQTn69R FOzlFuT2fe3SOB9XPJpu4EYn2ACx9wZCYFcgREezBOZHINj1iPIkehhWGTqqDs35j5Ga V6g4LA+4d+7SLMBEZjJpPZtceG1F7B1P78PgFYuWaI6yeI5XoaEofD9Uu3pG3zdKpu6A xIxqLA73Ndk9m4kT/f7n+ieksum0cl18cKXTuX/i3ztbDwV94OOLH6uNavOSLi6UM0Ue vSKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1758486221; x=1759091021; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=0JZxIzAartu9ONh15YXkN5AwTrZHGKB1VwmY0Xneo7c=; b=UX3Y0QtXxLN0icjAD87za3sLSCJWNCqLbY8lmRBpNm42/tiAYZGNNareufekPRWOwU 7ivsZmSObrpn+4ulx/5LjOFqdOa71o/HroXxV7srZJCFO9owTrvzU64xLLmyi9I/sWHx sXuxQ0HX9fMOQnQ7yrh9+RAUTlHxhnXNSmDKWWIaqFKYnWLcNcFVusDe1l5wv9bm2wPN F4rWqlGpYe+wmVSJgWtUts6OPnUPhJS7kkWTmwwu6obH8Xh6q/bQjLaNFio4mJ0FUMFX C20ITt9oI5X0UH95PrEgzeOb5Uz0eYmhnpdNoCF4uYbvwaGGNWz6fDVHd9Yv7GgDja4i qfNQ== X-Forwarded-Encrypted: i=1; AJvYcCXOdvtE7fA36Pv0BHyKrau1E68ySQGrVdX+PPEF9O/epFYivbyrHpCMya5WHHfS536nvr43kjWkAulw8ZA=@vger.kernel.org X-Gm-Message-State: AOJu0YzKlEJjGq2C2/4Em9HT+rmcZipNkepuiIfabMXKmQ3iCBOSethV W9y6h/CpCY+IV88x8lBIwdIK3WZLo/d0N3mGQsKBpnLtXHZTqiRcCOeyf9LEtzDqH5so X-Gm-Gg: ASbGncsg6pk7jlhki+jHHUzuyQ41BpxmW2823uQphQzh1E27hSUwHu/EBER4AXk4RE1 V2+PT1JIlbu9eCwOwbExETPncWKgmuUFlglJ1RcBg+ftZjEgCGbz4ROj1MT3VgoYwBPxZKQHdGs NYGg29onWx9dnVPAzP+oGAdGL6Im3NLepRdUXzZd/FhSMBVFmv/dEmLDt6G7YbsRZrRX9eJI9YX Al2Q6ZqOueV94l/oVrT3KVAmyald4qQ+KGfteNtxjxKcUiDx/xNciW4NGQKePDlBsGlBWLAmbtY D2Pvk5wiDgxpAL/6ydE0/7frwBrnCDLhctdlHQpSWtMAB5A6dm/uGS/q8xc+JKJzXqiMDbcI1VF 5pckOU0TcQsh89Lql9R4EZhBF X-Google-Smtp-Source: AGHT+IEJlkSkntjaKiFRoVV3hUysXntSgWZ6ecRDuofXNz2X0T9uNFSJBtgiJHnFkfEf7jQdEgPujg== X-Received: by 2002:a17:902:ebc4:b0:262:9ac8:610f with SMTP id d9443c01a7336-269ba447cd3mr153065445ad.22.1758486221598; Sun, 21 Sep 2025 13:23:41 -0700 (PDT) Received: from sultan-box ([79.127.217.57]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-269802de963sm109831135ad.77.2025.09.21.13.23.39 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 21 Sep 2025 13:23:41 -0700 (PDT) Date: Sun, 21 Sep 2025 13:23:37 -0700 From: Sultan Alsawaf To: Bin Du Cc: mchehab@kernel.org, hverkuil@xs4all.nl, laurent.pinchart+renesas@ideasonboard.com, bryan.odonoghue@linaro.org, sakari.ailus@linux.intel.com, prabhakar.mahadev-lad.rj@bp.renesas.com, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, pratap.nirujogi@amd.com, benjamin.chan@amd.com, king.li@amd.com, gjorgji.rosikopulos@amd.com, Phil.Jawich@amd.com, Dominic.Antony@amd.com, mario.limonciello@amd.com, richard.gong@amd.com, anson.tsao@amd.com, Svetoslav Stoilov , Mario Limonciello , Alexey Zagorodnikov Subject: Re: [PATCH v4 1/7] media: platform: amd: Introduce amd isp4 capture driver Message-ID: References: <20250911100847.277408-1-Bin.Du@amd.com> <20250911100847.277408-2-Bin.Du@amd.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250911100847.277408-2-Bin.Du@amd.com> Hi Bin, On Thu, Sep 11, 2025 at 06:08:41PM +0800, Bin Du wrote: > AMD isp4 capture is a v4l2 media device which implements media controller > interface. It has one sub-device (AMD ISP4 sub-device) endpoint which can > be connected to a remote CSI2 TX endpoint. It supports only one physical > interface for now. Also add ISP4 driver related entry info into the > MAINTAINERS file > > Co-developed-by: Svetoslav Stoilov > Signed-off-by: Svetoslav Stoilov > Signed-off-by: Bin Du > Reviewed-by: Mario Limonciello (AMD) > Tested-by: Alexey Zagorodnikov [snip] > +++ b/drivers/media/platform/amd/isp4/Kconfig > @@ -0,0 +1,13 @@ > +# SPDX-License-Identifier: GPL-2.0+ > + > +config AMD_ISP4 > + tristate "AMD ISP4 and camera driver" > + depends on VIDEO_DEV && VIDEO_V4L2_SUBDEV_API && DRM_AMD_ISP > + select VIDEOBUF2_CORE > + select VIDEOBUF2_V4L2 > + select VIDEOBUF2_MEMOPS VIDEO_V4L2_SUBDEV_API should be selected rather than set as a dependency, per what other drivers do. You can also sort the dependencies to look cleaner. Change to: depends on VIDEO_DEV && DRM_AMD_ISP select VIDEOBUF2_CORE select VIDEOBUF2_MEMOPS select VIDEOBUF2_V4L2 select VIDEO_V4L2_SUBDEV_API > + help > + This is support for AMD ISP4 and camera subsystem driver. > + Say Y here to enable the ISP4 and camera device for video capture. > + To compile this driver as a module, choose M here. The module will > + be called amd_capture. [snip] > +++ b/drivers/media/platform/amd/isp4/isp4.c > @@ -0,0 +1,121 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Copyright (C) 2025 Advanced Micro Devices, Inc. > + */ > + > +#include > +#include > +#include > + > +#include "isp4.h" > + > +#define VIDEO_BUF_NUM 5 > + > +#define ISP4_DRV_NAME "amd_isp_capture" > + > +/* interrupt num */ > +static const u32 isp4_ringbuf_interrupt_num[] = { > + 0, /* ISP_4_1__SRCID__ISP_RINGBUFFER_WPT9 */ > + 1, /* ISP_4_1__SRCID__ISP_RINGBUFFER_WPT10 */ > + 3, /* ISP_4_1__SRCID__ISP_RINGBUFFER_WPT11 */ > + 4, /* ISP_4_1__SRCID__ISP_RINGBUFFER_WPT12 */ > +}; > + > +#define to_isp4_device(dev) \ > + ((struct isp4_device *)container_of(dev, struct isp4_device, v4l2_dev)) The unnecessary cast on container_of() is removed later in "[PATCH v4 4/7] media: platform: amd: isp4 subdev and firmware loading handling added". Remove the cast in this patch instead. > + > +static irqreturn_t isp4_irq_handler(int irq, void *arg) > +{ > + return IRQ_HANDLED; > +} > + > +static int isp4_capture_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct isp4_device *isp_dev; > + int i, irq, ret; > + > + isp_dev = devm_kzalloc(&pdev->dev, sizeof(*isp_dev), GFP_KERNEL); s/&pdev->dev/dev/ > + if (!isp_dev) > + return -ENOMEM; > + > + isp_dev->pdev = pdev; > + dev->init_name = ISP4_DRV_NAME; > + > + for (i = 0; i < ARRAY_SIZE(isp4_ringbuf_interrupt_num); i++) { > + irq = platform_get_irq(pdev, isp4_ringbuf_interrupt_num[i]); > + if (irq < 0) > + return dev_err_probe(dev, -ENODEV, > + "fail to get irq %d\n", > + isp4_ringbuf_interrupt_num[i]); Return the error from platform_get_irq() here instead of -ENODEV. > + ret = devm_request_irq(&pdev->dev, irq, isp4_irq_handler, 0, > + "ISP_IRQ", &pdev->dev); s/&pdev->dev/dev/ > + if (ret) > + return dev_err_probe(dev, ret, "fail to req irq %d\n", > + irq); > + } > + > + /* Link the media device within the v4l2_device */ > + isp_dev->v4l2_dev.mdev = &isp_dev->mdev; > + > + /* Initialize media device */ > + strscpy(isp_dev->mdev.model, "amd_isp41_mdev", > + sizeof(isp_dev->mdev.model)); > + snprintf(isp_dev->mdev.bus_info, sizeof(isp_dev->mdev.bus_info), > + "platform:%s", ISP4_DRV_NAME); > + isp_dev->mdev.dev = &pdev->dev; s/&pdev->dev/dev/ > + media_device_init(&isp_dev->mdev); > + > + /* register v4l2 device */ > + snprintf(isp_dev->v4l2_dev.name, sizeof(isp_dev->v4l2_dev.name), > + "AMD-V4L2-ROOT"); > + ret = v4l2_device_register(&pdev->dev, &isp_dev->v4l2_dev); s/&pdev->dev/dev/ > + if (ret) > + return dev_err_probe(dev, ret, > + "fail register v4l2 device\n"); > + > + ret = media_device_register(&isp_dev->mdev); > + if (ret) { > + dev_err(dev, "fail to register media device %d\n", ret); > + goto err_unreg_v4l2; > + } > + > + platform_set_drvdata(pdev, isp_dev); > + > + pm_runtime_set_suspended(dev); > + pm_runtime_enable(dev); > + > + return 0; > + > +err_unreg_v4l2: > + v4l2_device_unregister(&isp_dev->v4l2_dev); > + > + return dev_err_probe(dev, ret, "isp probe fail\n"); > +} [snip] > +++ b/drivers/media/platform/amd/isp4/isp4.h > @@ -0,0 +1,24 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ > +/* > + * Copyright (C) 2025 Advanced Micro Devices, Inc. > + */ > + > +#ifndef _ISP4_H_ > +#define _ISP4_H_ > + > +#include Remove this linux/mutex.h include. It should be moved to isp4_subdev.h instead. > +#include > +#include > +#include This media/videobuf2-vmalloc.h include is removed in "[PATCH v4 4/7] media: platform: amd: isp4 subdev and firmware loading handling added". Remove it in this patch instead. > + > +#define ISP4_GET_ISP_REG_BASE(isp4sd) (((isp4sd))->mmio) > + > +struct isp4_device { > + struct v4l2_device v4l2_dev; > + struct media_device mdev; > + > + struct platform_device *pdev; > + struct notifier_block i2c_nb; i2c_nb is unused, remove it. > +}; > + > +#endif /* _ISP4_H_ */ > -- > 2.34.1 > Sultan