From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from lahtoruutu.iki.fi (lahtoruutu.iki.fi [185.185.170.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 685EE270EA1; Tue, 6 May 2025 15:37:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=pass smtp.client-ip=185.185.170.37 ARC-Seal:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1746545862; cv=pass; b=HQ0KCPLFipEyLxdbAkwOmnAcyOONTpdZr41rWbNVKUmWYiR0MSjV+UIkiB5E8DhDn7H5Cw5iCiKUG/RHEW9RW6oyUoFIpQV2Smwf1Tc1MK4ae0KIRSgSKSE16qylNf1FsJKPIOJlq0/2mLK82Js3rCqhXjWNe8d2zbir2+u31bA= ARC-Message-Signature:i=2; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1746545862; c=relaxed/simple; bh=YMiyYvi8bpnC4qQtxWL5YgUgrRRS+KfwfJZJgQuT/vI=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=nwxdN2MDao0CwhuUvVk2XEU6piATyjQhG9MMJs4jKIqwF2p8Gc/flITqyaIyCSfub1pRaMyvWPCucg+A+zl0LQkdjx2dsf0FkPDFjBIJylhlw68S0L0Vjc6xNvsvDLU35LQLH+hHLRnz16bqThxftrQiNpaxC/VkxZ0nD1sGUQE= ARC-Authentication-Results:i=2; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=iki.fi; spf=pass smtp.mailfrom=iki.fi; dkim=pass (2048-bit key) header.d=iki.fi header.i=@iki.fi header.b=hUVy0wDC; arc=pass smtp.client-ip=185.185.170.37 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=iki.fi Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=iki.fi Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=iki.fi header.i=@iki.fi header.b="hUVy0wDC" Received: from hillosipuli.retiisi.eu (2a00-1190-d1dd-0-127c-61ff-fee2-b97e.v6.cust.suomicom.net [IPv6:2a00:1190:d1dd:0:127c:61ff:fee2:b97e]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: sailus) by lahtoruutu.iki.fi (Postfix) with ESMTPSA id 4ZsMwC3zKvz49Q4h; Tue, 6 May 2025 18:37:31 +0300 (EEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=lahtoruutu; t=1746545852; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=zT6MtYij4Ex75ufnEnnsrXjVPmGK4IQtRiuDJtU5ie0=; b=hUVy0wDCIEmkY251qP3INIyeRl+25MPnLhQaAlNUoK54BPQJVIKNslYHbcf/RndXpvOv3K zNwaml/8EXxZRjVf3+GGKNZVOZr0Z2WrBbJNF5FohIGOjnnvM6eTJgVltFw86zZRN/4xmk pSBzBpA1E67PX/XjJuY67KBOx5iWFJg1EoQ1Dj4bF9mdb8jhVuM1yP1CG+AS4mv9gzZKnz QKLXDVvikZQn3ltKBlsYhogqkIv9YOgCyijnTUjsemralyCOeptLWH3kJVO+g1yypiX4xj e0QfKZUkavgiEkA3XZyGlfvVroaFRbQYGlV8IG0efcPmUOvmXry4TMFlALxJTg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=lahtoruutu; t=1746545852; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=zT6MtYij4Ex75ufnEnnsrXjVPmGK4IQtRiuDJtU5ie0=; b=CfNILcNFlzV3PV5SCUpBd4h0xRP5b0Fs9Iu2fxaGZLCAG3aAfoBshk6Ddz3X0CGWCD/1/t 77D2kvnD2+eZu27UA2MnY0IvCuzzvsTh0RbtMBZk6oLrK9URXTk33jMoxtPXZI2SnXUpzS X5XtWsPHX29yO3jyZy5b2ffJkOP+lQtjN8MW6oAV+12VWY4aa1TJqOiZBG1aDtyog9uB/8 JQBMBlAzkk5G69zgVigyE/v2ZB+Mb0aqDv9SlZuY9W60D3EjhUDYRJT6zUwYl7yK10s/y9 4m1WnzefuC2cEXdXNG4oVrSVI1qsUgsrT5VVLQjndiiUpRROScljjONghKRhvw== ARC-Seal: i=1; s=lahtoruutu; d=iki.fi; t=1746545852; a=rsa-sha256; cv=none; b=ii+0CSS5PF2B768CQf78jjyp+FbnkMVfKYj2MDed+5NCBxyD0Wt47+gSLeG734RnO20F/S 9D5tgLI5nJPZpXCbEiKrEy06s1bT1u0sJe4WX75QT8CjZeizGljmd4eoH+6yCFhbiH9LU8 ndMxscz501kgC5Qqx9/GsjqnEARgHJUFQc9tSmGlL7B3J+TSt8KE4p53vOoozlTUJoAVsi iXmdqL9v3tSCW2AiH0hL8BuSNm2yff/S1z+NXSzAQhQ4d7q1uF4wTjnrWzxxGUnuhgqQnA 8zGUCnXjEiuGFKzpsMHtKblWgs9VuH2oT/Ava05dRf1p6ZAUOOVqZtJVcYGbRw== ARC-Authentication-Results: i=1; ORIGINATING; auth=pass smtp.auth=sailus smtp.mailfrom=sakari.ailus@iki.fi Received: from valkosipuli.retiisi.eu (valkosipuli.local [192.168.4.2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by hillosipuli.retiisi.eu (Postfix) with ESMTPS id 2AC47634C93; Tue, 6 May 2025 18:37:31 +0300 (EEST) Date: Tue, 6 May 2025 15:37:30 +0000 From: Sakari Ailus To: Pratap Nirujogi Cc: hdegoede@redhat.com, W_Armin@gmx.de, ilpo.jarvinen@linux.intel.com, mario.limonciello@amd.com, platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org, benjamin.chan@amd.com, bin.du@amd.com, gjorgji.rosikopulos@amd.com, king.li@amd.com, dantony@amd.com Subject: Re: [PATCH v12] platform/x86: Add AMD ISP platform config for OV05C10 Message-ID: References: <20250505171302.4177445-1-pratap.nirujogi@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: <20250505171302.4177445-1-pratap.nirujogi@amd.com> Hi Pratap, On Mon, May 05, 2025 at 01:11:26PM -0400, Pratap Nirujogi wrote: > ISP device specific configuration is not available in ACPI. Add > swnode graph to configure the missing device properties for the > OV05C10 camera device supported on amdisp platform. > > Add support to create i2c-client dynamically when amdisp i2c > adapter is available. > > Co-developed-by: Benjamin Chan > Signed-off-by: Benjamin Chan > Reviewed-by: Mario Limonciello > Reviewed-by: Hans de Goede > Reviewed-by: Armin Wolf > Signed-off-by: Pratap Nirujogi > --- > Changes v11 -> v12: > > * Add missing space before the open parenthesis '(' > > drivers/platform/x86/amd/Kconfig | 11 ++ > drivers/platform/x86/amd/Makefile | 1 + > drivers/platform/x86/amd/amd_isp4.c | 280 ++++++++++++++++++++++++++++ > 3 files changed, 292 insertions(+) > create mode 100644 drivers/platform/x86/amd/amd_isp4.c > > diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig > index c3e086ea64fc..152a68a470e8 100644 > --- a/drivers/platform/x86/amd/Kconfig > +++ b/drivers/platform/x86/amd/Kconfig > @@ -32,3 +32,14 @@ config AMD_WBRF > > This mechanism will only be activated on platforms that advertise a > need for it. > + > +config AMD_ISP_PLATFORM > + tristate "AMD ISP4 platform driver" > + depends on I2C && X86_64 && ACPI > + help > + Platform driver for AMD platforms containing image signal processor > + gen 4. Provides camera sensor module board information to allow > + sensor and V4L drivers to work properly. > + > + This driver can also be built as a module. If so, the module > + will be called amd_isp4. > diff --git a/drivers/platform/x86/amd/Makefile b/drivers/platform/x86/amd/Makefile > index c6c40bdcbded..b0e284b5d497 100644 > --- a/drivers/platform/x86/amd/Makefile > +++ b/drivers/platform/x86/amd/Makefile > @@ -10,3 +10,4 @@ obj-$(CONFIG_AMD_PMC) += pmc/ > obj-$(CONFIG_AMD_HSMP) += hsmp/ > obj-$(CONFIG_AMD_PMF) += pmf/ > obj-$(CONFIG_AMD_WBRF) += wbrf.o > +obj-$(CONFIG_AMD_ISP_PLATFORM) += amd_isp4.o > diff --git a/drivers/platform/x86/amd/amd_isp4.c b/drivers/platform/x86/amd/amd_isp4.c > new file mode 100644 > index 000000000000..1520ebb94507 > --- /dev/null > +++ b/drivers/platform/x86/amd/amd_isp4.c > @@ -0,0 +1,280 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * AMD ISP platform driver for sensor i2-client instantiation > + * > + * Copyright 2025 Advanced Micro Devices, Inc. > + */ > + > +#include > +#include > +#include > +#include > +#include > + > +#define AMDISP_OV05C10_I2C_ADDR 0x10 > +#define AMDISP_OV05C10_PLAT_NAME "amdisp_ov05c10_platform" > +#define AMDISP_OV05C10_HID "OMNI5C10" What's the purpose of this _HID and is it present on actual firmware implementation? There's no such ACPI vendor ID as "OMNI". > +#define AMDISP_OV05C10_REMOTE_EP_NAME "ov05c10_isp_4_1_1" > +#define AMD_ISP_PLAT_DRV_NAME "amd-isp4" > + > +/* > + * AMD ISP platform definition to configure the device properties > + * missing in the ACPI table. > + */ > +struct amdisp_platform { > + struct i2c_board_info board_info; > + struct notifier_block i2c_nb; > + struct i2c_client *i2c_dev; > + struct mutex lock; /* protects i2c client creation */ > +}; > + > +/* Top-level OV05C10 camera node property table */ > +static const struct property_entry ov05c10_camera_props[] = { > + PROPERTY_ENTRY_U32("clock-frequency", 24 * HZ_PER_MHZ), > + { } > +}; > + > +/* Root AMD ISP OV05C10 camera node definition */ > +static const struct software_node camera_node = { > + .name = AMDISP_OV05C10_HID, > + .properties = ov05c10_camera_props, > +}; > + > +/* > + * AMD ISP OV05C10 Ports node definition. No properties defined for > + * ports node for OV05C10. > + */ > +static const struct software_node ports = { > + .name = "ports", > + .parent = &camera_node, > +}; > + > +/* > + * AMD ISP OV05C10 Port node definition. No properties defined for > + * port node for OV05C10. > + */ > +static const struct software_node port_node = { > + .name = "port@", > + .parent = &ports, > +}; > + > +/* > + * Remote endpoint AMD ISP node definition. No properties defined for > + * remote endpoint node for OV05C10. How will this scale? Can you use other sensors with this ISP? Although if you get little from firmware, there's not much you can do. That being said, switching to DisCo for Imaging could be an easier step in this case. > + */ > +static const struct software_node remote_ep_isp_node = { > + .name = AMDISP_OV05C10_REMOTE_EP_NAME, > +}; > + > +/* > + * Remote endpoint reference for isp node included in the > + * OV05C10 endpoint. > + */ > +static const struct software_node_ref_args ov05c10_refs[] = { > + SOFTWARE_NODE_REFERENCE(&remote_ep_isp_node), > +}; > + > +/* OV05C supports one single link frequency */ > +static const u64 ov05c10_link_freqs[] = { > + 925 * HZ_PER_MHZ, > +}; > + > +/* OV05C supports only 2-lane configuration */ > +static const u32 ov05c10_data_lanes[] = { > + 1, > + 2, > +}; > + > +/* OV05C10 endpoint node properties table */ > +static const struct property_entry ov05c10_endpoint_props[] = { > + PROPERTY_ENTRY_U32("bus-type", 4), > + PROPERTY_ENTRY_U32_ARRAY_LEN("data-lanes", ov05c10_data_lanes, > + ARRAY_SIZE(ov05c10_data_lanes)), > + PROPERTY_ENTRY_U64_ARRAY_LEN("link-frequencies", ov05c10_link_freqs, > + ARRAY_SIZE(ov05c10_link_freqs)), > + PROPERTY_ENTRY_REF_ARRAY("remote-endpoint", ov05c10_refs), > + { } > +}; > + > +/* AMD ISP endpoint node definition */ > +static const struct software_node endpoint_node = { > + .name = "endpoint", > + .parent = &port_node, > + .properties = ov05c10_endpoint_props, > +}; > + > +/* > + * AMD ISP swnode graph uses 5 nodes and also its relationship is > + * fixed to align with the structure that v4l2 expects for successful > + * endpoint fwnode parsing. > + * > + * It is only the node property_entries that will vary for each platform > + * supporting different sensor modules. > + */ > +#define NUM_SW_NODES 5 > + > +static const struct software_node *ov05c10_nodes[NUM_SW_NODES + 1] = { Defining the number of entries seems to be redundant here. > + &camera_node, > + &ports, > + &port_node, > + &endpoint_node, > + &remote_ep_isp_node, > + NULL > +}; > + > +static const struct acpi_device_id amdisp_sensor_ids[] = { > + { AMDISP_OV05C10_HID }, > + { } > +}; > +MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids); > + > +static inline bool is_isp_i2c_adapter(struct i2c_adapter *adap) > +{ > + return !strcmp(adap->owner->name, "i2c_designware_amdisp"); > +} > + > +static void instantiate_isp_i2c_client(struct amdisp_platform *ov05c10, struct i2c_adapter *adap) > +{ > + struct i2c_board_info *info = &ov05c10->board_info; > + struct i2c_client *i2c_dev; > + > + guard(mutex)(&ov05c10->lock); > + > + if (ov05c10->i2c_dev) > + return; > + > + i2c_dev = i2c_new_client_device(adap, info); > + if (IS_ERR(i2c_dev)) { > + dev_err(&adap->dev, "error %pe registering isp i2c_client\n", i2c_dev); > + return; > + } > + ov05c10->i2c_dev = i2c_dev; > +} > + > +static int isp_i2c_bus_notify(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + struct amdisp_platform *ov05c10 = container_of(nb, struct amdisp_platform, i2c_nb); > + struct device *dev = data; > + struct i2c_client *client; > + struct i2c_adapter *adap; > + > + switch (action) { > + case BUS_NOTIFY_ADD_DEVICE: > + adap = i2c_verify_adapter(dev); > + if (!adap) > + break; > + if (is_isp_i2c_adapter(adap)) > + instantiate_isp_i2c_client(ov05c10, adap); > + break; > + case BUS_NOTIFY_REMOVED_DEVICE: > + client = i2c_verify_client(dev); > + if (!client) > + break; > + > + scoped_guard(mutex, &ov05c10->lock) { > + if (ov05c10->i2c_dev == client) { > + dev_dbg(&client->adapter->dev, "amdisp i2c_client removed\n"); > + ov05c10->i2c_dev = NULL; > + } > + } > + break; > + default: > + break; > + } > + > + return NOTIFY_DONE; > +} > + > +static struct amdisp_platform *prepare_amdisp_platform(struct device *dev) > +{ > + struct amdisp_platform *isp_ov05c10; > + int ret; > + > + isp_ov05c10 = devm_kzalloc(dev, sizeof(*isp_ov05c10), GFP_KERNEL); > + if (!isp_ov05c10) > + return ERR_PTR(-ENOMEM); > + > + ret = devm_mutex_init(dev, &isp_ov05c10->lock); > + if (ret) > + return ERR_PTR(ret); > + > + isp_ov05c10->board_info.dev_name = "ov05c10"; > + strscpy(isp_ov05c10->board_info.type, "ov05c10", I2C_NAME_SIZE); > + isp_ov05c10->board_info.addr = AMDISP_OV05C10_I2C_ADDR; > + > + ret = software_node_register_node_group(ov05c10_nodes); > + if (ret) > + return ERR_PTR(ret); > + > + isp_ov05c10->board_info.swnode = ov05c10_nodes[0]; > + > + return isp_ov05c10; > +} > + > +static int try_to_instantiate_i2c_client(struct device *dev, void *data) > +{ > + struct amdisp_platform *ov05c10 = (struct amdisp_platform *)data; > + struct i2c_adapter *adap = i2c_verify_adapter(dev); > + > + if (!ov05c10 || !adap) > + return 0; > + if (!adap->owner) > + return 0; > + > + if (is_isp_i2c_adapter(adap)) > + instantiate_isp_i2c_client(ov05c10, adap); > + > + return 0; > +} > + > +static int amd_isp_probe(struct platform_device *pdev) > +{ > + struct amdisp_platform *ov05c10; > + int ret; > + > + ov05c10 = prepare_amdisp_platform(&pdev->dev); > + if (IS_ERR(ov05c10)) > + return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10), > + "failed to prepare AMD ISP platform fwnode\n"); > + > + ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify; > + ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb); > + if (ret) > + goto error_unregister_sw_node; > + > + /* check if adapter is already registered and create i2c client instance */ > + i2c_for_each_dev((void *)ov05c10, try_to_instantiate_i2c_client); > + > + platform_set_drvdata(pdev, ov05c10); > + return 0; > + > +error_unregister_sw_node: > + software_node_unregister_node_group(ov05c10_nodes); > + return ret; > +} > + > +static void amd_isp_remove(struct platform_device *pdev) > +{ > + struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev); > + > + bus_unregister_notifier(&i2c_bus_type, &ov05c10->i2c_nb); > + i2c_unregister_device(ov05c10->i2c_dev); > + software_node_unregister_node_group(ov05c10_nodes); > +} > + > +static struct platform_driver amd_isp_platform_driver = { > + .driver = { > + .name = AMD_ISP_PLAT_DRV_NAME, > + .acpi_match_table = amdisp_sensor_ids, > + }, > + .probe = amd_isp_probe, > + .remove = amd_isp_remove, > +}; > + > +module_platform_driver(amd_isp_platform_driver); > + > +MODULE_AUTHOR("Benjamin Chan "); > +MODULE_AUTHOR("Pratap Nirujogi "); > +MODULE_DESCRIPTION("AMD ISP4 Platform Driver"); > +MODULE_LICENSE("GPL"); -- Regards, Sakari Ailus