Re: [PATCH v5] platform/x86: Add AMD ISP platform config for OV05C10

From: Ilpo Järvinen
Date: Tue Apr 15 2025 - 13:49:58 EST

Next message: David Laight: "Re: [PATCH 02/17] scsi: bfa: Rename 'timer_mod' to 'timer_module'"
Previous message: Aditya Dutt: "Test if it's still reproducible"
In reply to: Nirujogi, Pratap: "Re: [PATCH v5] platform/x86: Add AMD ISP platform config for OV05C10"
Next in thread: Nirujogi, Pratap: "Re: [PATCH v5] platform/x86: Add AMD ISP platform config for OV05C10"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 15 Apr 2025, Nirujogi, Pratap wrote:

> Hi Ilpo,
>
> Thanks for the review feedback.
>
> Thanks,
> Pratap
>
> On 4/15/2025 11:42 AM, Ilpo Järvinen wrote:
> > Caution: This message originated from an External Source. Use proper caution
> > when opening attachments, clicking links, or responding.
> >
> >
> > On Tue, 8 Apr 2025, 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 <benjamin.chan@xxxxxxx>
> > > Signed-off-by: Benjamin Chan <benjamin.chan@xxxxxxx>
> > > Signed-off-by: Pratap Nirujogi <pratap.nirujogi@xxxxxxx>
> > > ---
> > > Changes v4 -> v5:
> > >
> > > * Avoid global variables and make driver reentrant following the
> > > state container design pattern.
> > >
> > > * Fix coding error referencing i2c_dev variable of amdisp_platform
> > > in instantiate_isp_i2c_client().
> > >
> > > * Remove i2c_put_adapter(). Not required as i2c_get_adapter() is not
> > > called.
> > >
> > > * Use i2c_board_info->swnode instead of fwnode to fix refcount imbalance
> > > warnings when module is removed.
> > >
> > > * Address review comments.
> > >
> > > drivers/platform/x86/amd/Kconfig | 11 ++
> > > drivers/platform/x86/amd/Makefile | 1 +
> > > drivers/platform/x86/amd/amd_isp4.c | 295 ++++++++++++++++++++++++++++
> > > 3 files changed, 307 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..ec755b5fc93c 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 && AMD_ISP4
> > > + 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..ad181ab96423
> > > --- /dev/null
> > > +++ b/drivers/platform/x86/amd/amd_isp4.c
> > > @@ -0,0 +1,295 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * AMD ISP platform driver for sensor i2-client instantiation
> > > + *
> > > + * Copyright 2025 Advanced Micro Devices, Inc.
> > > + */
> > > +
> > > +#include <linux/acpi.h>
> > > +#include <linux/device/bus.h>
> > > +#include <linux/dmi.h>
> > > +#include <linux/gpio/machine.h>
> > > +#include <linux/init.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/property.h>
> > > +#include <linux/units.h>
> > > +
> > > +#define AMDISP_OV05C10_I2C_ADDR 0x10
> > > +#define AMDISP_OV05C10_PLAT_NAME "amdisp_ov05c10_platform"
> > > +#define AMDISP_OV05C10_HID "OMNI5C10"
> > > +#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 {
> > > + const char *name;
> > > + u8 i2c_addr;
> > > + u8 max_num_swnodes;
> > > + struct i2c_board_info board_info;
> > > + struct notifier_block i2c_nb;
> > > + struct i2c_client *i2c_dev;
> > > + struct software_node **swnodes;
> >
> > Can't this too be const struct so you can avoid a few casts below?
> >
> Thanks. I will make it a const and remove the unnecessary casts in V6.
>
> > > +};
> > > +
> > > +/* 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.
> > > + */
> > > +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] = {
> > > + &camera_node,
> > > + &ports,
> > > + &port_node,
> > > + &endpoint_node,
> > > + &remote_ep_isp_node,
> > > + NULL
> > > +};
> > > +
> > > +/* OV05C10 specific AMD ISP platform configuration */
> > > +static const struct amdisp_platform amdisp_ov05c10_platform_config = {
> > > + .name = AMDISP_OV05C10_PLAT_NAME,
> > > + .board_info = {
> > > + .dev_name = "ov05c10",
> > > + I2C_BOARD_INFO("ov05c10", AMDISP_OV05C10_I2C_ADDR),
> > > + },
> > > + .i2c_addr = AMDISP_OV05C10_I2C_ADDR,
> > > + .max_num_swnodes = NUM_SW_NODES,
> > > + .swnodes = (struct software_node **)ov05c10_nodes,
> > > +};
> > > +
> > > +static const struct acpi_device_id amdisp_sensor_ids[] = {
> > > + { AMDISP_OV05C10_HID },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(acpi, amdisp_sensor_ids);
> > > +
> > > +static 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;
> > > +
> > > + if (ov05c10->i2c_dev)
> > > + return;
> > > +
> > > + if (!info->addr) {
> > > + dev_err(&adap->dev, "invalid i2c_addr 0x%x detected\n",
> > > + ov05c10->i2c_addr);
> >
> > Put on a single line, you've longer lines already than what this is.
> >
> sure, will fix this in V6.
>
> > > + 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);
> >
> > Ditto.
> >
> sure, will fix this in V6.
>
> > > + 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;
> > > + 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(const struct
> > > amdisp_platform *src)
> > > +{
> > > + struct amdisp_platform *isp_ov05c10;
> > > + const struct software_node **sw_nodes;
> > > + const struct software_node *sw_node;
> > > + struct i2c_board_info *info;
> > > + int ret;
> > > +
> > > + isp_ov05c10 = kmemdup(src, sizeof(*isp_ov05c10), GFP_KERNEL);
> > > + if (!isp_ov05c10)
> > > + return ERR_PTR(-ENOMEM);
> > > +
> > > + info = &isp_ov05c10->board_info;
> > > +
> > > + sw_nodes = (const struct software_node **)src->swnodes;
> > > + ret = software_node_register_node_group(sw_nodes);
> > > + if (ret)
> > > + goto error_unregister_sw_node;
> > > +
> > > + sw_node = (const struct software_node *)src->swnodes[0];
> > > + info->swnode = sw_node;
> > > +
> > > + return isp_ov05c10;
> > > +
> > > +error_unregister_sw_node:
> > > + software_node_unregister_node_group(sw_nodes);
> >
> > If register failed, why you are calling unregister for it??
> >
> This is not intended, thanks for catching this, will fix it in V6.
>
>
> > > + kfree(isp_ov05c10);
> > > + return ERR_PTR(ret);
> > > +}
> > > +
> > > +static int amd_isp_probe(struct platform_device *pdev)
> > > +{
> > > + struct amdisp_platform *ov05c10;
> > > + int ret;
> > > +
> > > + ov05c10 = prepare_amdisp_platform(&amdisp_ov05c10_platform_config);
> > > + if (IS_ERR(ov05c10))
> > > + return dev_err_probe(&pdev->dev, PTR_ERR(ov05c10),
> > > + "failed to prepare amdisp platform fw
> > > node\n");
> >
> > fwnode
> >
> > Please also capitalize properly as this is user visible message, AMD ISP ?
> >
> sure, I will capitalize the words and fix the spacing as suggested in V6.
>
> > > +
> > > + ov05c10->i2c_nb.notifier_call = isp_i2c_bus_notify;
> > > +
> > > + ret = bus_register_notifier(&i2c_bus_type, &ov05c10->i2c_nb);
> > > + if (ret)
> > > + goto error_free_platform;
> > > +
> > > + platform_set_drvdata(pdev, ov05c10);
> > > + return ret;
> >
> > return 0
> >
> > > +
> > > +error_free_platform:
> > > + kfree(ov05c10);
> > > + return ret;
> > > +}
> > > +
> > > +static void amd_isp_remove(struct platform_device *pdev)
> > > +{
> > > + struct amdisp_platform *ov05c10 = platform_get_drvdata(pdev);
> > > +
> > > + if (IS_ERR_OR_NULL(ov05c10))
> >
> > How can this happen??
> >
> I agree this can never happen in the current driver. The only possibility that
> it can happen is when platform_set_drvdata() is missed in the probe().

Just remove the unnecessary check then.

--
i.

Next message: David Laight: "Re: [PATCH 02/17] scsi: bfa: Rename 'timer_mod' to 'timer_module'"
Previous message: Aditya Dutt: "Test if it's still reproducible"
In reply to: Nirujogi, Pratap: "Re: [PATCH v5] platform/x86: Add AMD ISP platform config for OV05C10"
Next in thread: Nirujogi, Pratap: "Re: [PATCH v5] platform/x86: Add AMD ISP platform config for OV05C10"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]