Message-ID: <63a2737b-384b-44e4-8eb3-f255d3011644@amd.com>
Date: Tue, 22 Apr 2025 11:59:22 -0400
From: "Nirujogi, Pratap" <pnirujog@....com>
To: Mario Limonciello <mario.limonciello@....com>,
 Pratap Nirujogi <pratap.nirujogi@....com>, andi.shyti@...nel.org
Cc: linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
 benjamin.chan@....com, bin.du@....com, gjorgji.rosikopulos@....com,
 king.li@....com, dominic.antony@....com
Subject: Re: [PATCH] i2c: amd-isp: Add ISP i2c-designware driver

Hi Mario,

Please accept my apologies for the delayed response to your review 
comments.

Thanks,
Pratap

On 2/28/2025 10:33 PM, Mario Limonciello wrote:
> On 2/28/2025 10:45, Pratap Nirujogi wrote:
>> The camera sensor is connected via ISP I2C bus in AMD SOC
>> architectures. Add new I2C designware driver to support
>> new camera sensors on AMD HW.
>>
>> Signed-off-by: Pratap Nirujogi <pratap.nirujogi@....com>
>> ---
>>   drivers/i2c/busses/Kconfig                 |  10 +
>>   drivers/i2c/busses/Makefile                |   1 +
>>   drivers/i2c/busses/i2c-designware-amdisp.c | 266 +++++++++++++++++++++
>>   drivers/i2c/busses/i2c-designware-amdisp.h |  24 ++
>>   4 files changed, 301 insertions(+)
>>   create mode 100644 drivers/i2c/busses/i2c-designware-amdisp.c
>>   create mode 100644 drivers/i2c/busses/i2c-designware-amdisp.h
>>
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index fc438f445771..79448211baae 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -592,6 +592,16 @@ config I2C_DESIGNWARE_PLATFORM
>>         This driver can also be built as a module.  If so, the module
>>         will be called i2c-designware-platform.
>> +config I2C_DESIGNWARE_AMDISP
>> +    tristate "Synopsys DesignWare Platform for AMDISP"
>> +    depends on I2C_DESIGNWARE_CORE
>> +    help
>> +      If you say yes to this option, support will be included for the
>> +      AMDISP Synopsys DesignWare I2C adapter.
>> +
>> +      This driver can also be built as a module.  If so, the module
>> +      will be called amd_isp_i2c_designware.
>> +
>>   config I2C_DESIGNWARE_AMDPSP
>>       bool "AMD PSP I2C semaphore support"
>>       depends on ACPI
>> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>> index 1c2a4510abe4..cfe53038df69 100644
>> --- a/drivers/i2c/busses/Makefile
>> +++ b/drivers/i2c/busses/Makefile
>> @@ -58,6 +58,7 @@ obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)            += 
>> i2c-designware-platform.o
>>   i2c-designware-platform-y                 := i2c-designware-platdrv.o
>>   i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_AMDPSP)    += i2c- 
>> designware-amdpsp.o
>>   i2c-designware-platform-$(CONFIG_I2C_DESIGNWARE_BAYTRAIL) += i2c- 
>> designware-baytrail.o
>> +obj-$(CONFIG_I2C_DESIGNWARE_AMDISP) += i2c-designware-amdisp.o
>>   obj-$(CONFIG_I2C_DESIGNWARE_PCI)            += i2c-designware-pci.o
>>   i2c-designware-pci-y                    := i2c-designware-pcidrv.o
>>   obj-$(CONFIG_I2C_DIGICOLOR)    += i2c-digicolor.o
>> diff --git a/drivers/i2c/busses/i2c-designware-amdisp.c b/drivers/i2c/ 
>> busses/i2c-designware-amdisp.c
>> new file mode 100644
>> index 000000000000..dc90510a440b
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-designware-amdisp.c
>> @@ -0,0 +1,266 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright 2024-2025 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be 
>> included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT 
>> SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
>> DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/dmi.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/property.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +#include <linux/sched.h>
>> +#include <linux/slab.h>
>> +#include <linux/suspend.h>
>> +#include <linux/units.h>
> 
> Can you please audit this list for accuracy?  It seems like a very large 
> number of headers.
> 
> Just off the bat for example I didn't see any DMI use, so dmi.h probably 
> isn't needed.  I suspect there are others.
> 
Thanks. Will clean-up headers files in V2.

>> +
>> +#include "i2c-designware-core.h"
>> +#include "i2c-designware-amdisp.h"
>> +
>> +#define AMD_ISP_I2C_INPUT_CLK            100 //100 Mhz
>> +
>> +#define to_amd_isp_i2c_dev(dev) \
>> +    ((struct amd_isp_i2c_dev *)container_of(dev, struct 
>> amd_isp_i2c_dev, dw_dev))
>> +
>> +struct amd_isp_i2c_dev {
>> +    struct dw_i2c_dev    dw_dev;
>> +};
>> +
>> +static void amd_isp_dw_i2c_plat_pm_cleanup(struct dw_i2c_dev *dev)
>> +{
>> +    pm_runtime_disable(dev->dev);
>> +
>> +    if (dev->shared_with_punit)
>> +        pm_runtime_put_noidle(dev->dev);
>> +}
>> +
>> +static u32 amd_isp_dw_i2c_get_clk_rate(struct dw_i2c_dev *dev)
>> +{
>> +    return AMD_ISP_I2C_INPUT_CLK * 1000;
>> +}
>> +
>> +static int amd_isp_dw_i2c_plat_probe(struct platform_device *pdev)
>> +{
>> +    struct i2c_adapter *adap;
>> +    struct amd_isp_i2c_dev *isp_i2c_dev;
>> +    struct dw_i2c_dev *dev;
>> +    int ret;
>> +
>> +    isp_i2c_dev = devm_kzalloc(&pdev->dev, sizeof(struct 
>> amd_isp_i2c_dev),
>> +                   GFP_KERNEL);
>> +    if (!isp_i2c_dev)
>> +        return -ENOMEM;
>> +
>> +    dev = &isp_i2c_dev->dw_dev;
>> +    dev->dev = &pdev->dev;
>> +
>> +    /**
>> +     * Use the polling mode to send/receive the data, because
>> +     * no IRQ connection from ISP I2C
>> +     */
>> +    dev->flags |= ACCESS_POLLING;
>> +    platform_set_drvdata(pdev, dev);
>> +
>> +    dev->base = devm_platform_ioremap_resource(pdev, 0);
>> +    if (IS_ERR(dev->base))
>> +        return PTR_ERR(dev->base);
>> +
>> +    ret = isp_power_set(true);
>> +    if (ret) {
>> +        dev_err(dev->dev, "unable to turn on the amdisp i2c power: 
>> %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    dev->get_clk_rate_khz = amd_isp_dw_i2c_get_clk_rate;
>> +    ret = i2c_dw_fw_parse_and_configure(dev);
>> +    if (ret)
>> +        goto exit;
>> +
>> +    i2c_dw_configure(dev);
>> +
>> +    adap = &dev->adapter;
>> +    adap->owner = THIS_MODULE;
>> +    ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>> +    adap->dev.of_node = pdev->dev.of_node;
>> +    /* arbitrary large number to avoid any conflicts */
>> +    adap->nr = 99;
> 
> Would it be better to allocate an IDA here?
> 
Thanks. Will remove this hardcoding and use the dynamically assigned bus 
id in V2.

>> +
>> +    if (dev->flags & ACCESS_NO_IRQ_SUSPEND) {
>> +        dev_pm_set_driver_flags(&pdev->dev,
>> +                    DPM_FLAG_SMART_PREPARE);
>> +    } else {
>> +        dev_pm_set_driver_flags(&pdev->dev,
>> +                    DPM_FLAG_SMART_PREPARE |
>> +                    DPM_FLAG_SMART_SUSPEND);
>> +    }
>> +
>> +    device_enable_async_suspend(&pdev->dev);
>> +
>> +    /* The code below assumes runtime PM to be disabled. */
>> +    WARN_ON(pm_runtime_enabled(&pdev->dev));
>> +
>> +    pm_runtime_dont_use_autosuspend(&pdev->dev);
>> +    pm_runtime_set_active(&pdev->dev);
>> +
>> +    if (dev->shared_with_punit)
>> +        pm_runtime_get_noresume(&pdev->dev);
>> +
>> +    pm_runtime_enable(&pdev->dev);
>> +
>> +    ret = i2c_dw_probe(dev);
>> +    if (ret) {
>> +        dev_err(dev->dev, "i2c_dw_probe failed %d\n", ret);
>> +        goto exit_probe;
>> +    }
>> +
>> +    isp_power_set(false);
>> +    return ret;
>> +
>> +exit_probe:
>> +    amd_isp_dw_i2c_plat_pm_cleanup(dev);
>> +    isp_power_set(false);
>> +exit:
>> +    isp_power_set(false);
> 
> I see 3 cases that isp_power_set(false) is called above:
> * success
> * failure exit probe
> * failure exit
> 
I should have removed isp_power_set() references before submitting the 
patch. Sorry its overlooked.

isp_power_set() is the exported symbol from ISP driver. Will take care 
of removing the dependency on isp_power_set() in V2.

> exit_probe falls through to exit, which means it's called twice.  That 
> seems to be a mistake to me.
> 
Thanks of identifying the bug with "exit_probe" and "exit", will fix it 
in V2.

> But since it's also called in the success flow this makes me wonder if 
> it's better to use a macro like _free() which will automatically call 
> isp_power_set(false) when the function exits.
> 
> Furthermore; are you missing a call to isp_power_set(true) in runtime 
> resume and isp_power_set(false) in runtime suspend?  It would seem 
> logical to me that when runtime suspended the tile is off and when on 
> runtime resumed it's back on.
> 
Instead of direct call to ISP driver function, will add the ISP device 
to generic power domain (gpd) to control power remotely using pm runtime 
calls.

>> +    return ret;
>> +}
>> +
>> +static void amd_isp_dw_i2c_plat_remove(struct platform_device *pdev)
>> +{
>> +    struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
>> +
>> +    pm_runtime_get_sync(&pdev->dev);
>> +
>> +    i2c_del_adapter(&dev->adapter);
>> +
>> +    i2c_dw_disable(dev);
>> +
>> +    pm_runtime_dont_use_autosuspend(&pdev->dev);
>> +    pm_runtime_put_sync(&pdev->dev);
>> +    amd_isp_dw_i2c_plat_pm_cleanup(dev);
>> +
>> +    reset_control_assert(dev->rst);
>> +}
>> +
>> +static int amd_isp_dw_i2c_plat_prepare(struct device *dev)
>> +{
>> +    /*
>> +     * If the ACPI companion device object is present for this 
>> device, it
>> +     * may be accessed during suspend and resume of other devices via 
>> I2C
>> +     * operation regions, so tell the PM core and middle layers to avoid
>> +     * skipping system suspend/resume callbacks for it in that case.
>> +     */
>> +    return !has_acpi_companion(dev);
>> +}
>> +
>> +static int amd_isp_dw_i2c_plat_runtime_suspend(struct device *dev)
>> +{
>> +    struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>> +
>> +    if (i_dev->shared_with_punit)
>> +        return 0;
>> +
>> +    i2c_dw_disable(i_dev);
>> +    i2c_dw_prepare_clk(i_dev, false);
>> +
>> +    return 0;
>> +}
>> +
>> +static int amd_isp_dw_i2c_plat_suspend(struct device *dev)
>> +{
>> +    struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>> +
>> +    i2c_mark_adapter_suspended(&i_dev->adapter);
>> +
>> +    return amd_isp_dw_i2c_plat_runtime_suspend(dev);
>> +}
>> +
>> +static int amd_isp_dw_i2c_plat_runtime_resume(struct device *dev)
>> +{
>> +    struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>> +
>> +    if (!i_dev->shared_with_punit)
>> +        i2c_dw_prepare_clk(i_dev, true);
>> +
>> +    i_dev->init(i_dev);
>> +
>> +    return 0;
>> +}
>> +
>> +static int amd_isp_dw_i2c_plat_resume(struct device *dev)
>> +{
>> +    struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
>> +
>> +    amd_isp_dw_i2c_plat_runtime_resume(dev);
>> +    i2c_mark_adapter_resumed(&i_dev->adapter);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct dev_pm_ops amd_isp_dw_i2c_dev_pm_ops = {
>> +    .prepare = pm_sleep_ptr(amd_isp_dw_i2c_plat_prepare),
>> +    LATE_SYSTEM_SLEEP_PM_OPS(amd_isp_dw_i2c_plat_suspend, 
>> amd_isp_dw_i2c_plat_resume)
>> +    RUNTIME_PM_OPS(amd_isp_dw_i2c_plat_runtime_suspend, 
>> amd_isp_dw_i2c_plat_runtime_resume, NULL)
>> +};
>> +
>> +/* Work with hotplug and coldplug */
>> +MODULE_ALIAS("platform:amd_isp_i2c_designware");
>> +
>> +static struct platform_driver amd_isp_dw_i2c_driver = {
>> +    .probe = amd_isp_dw_i2c_plat_probe,
>> +    .remove = amd_isp_dw_i2c_plat_remove,
>> +    .driver        = {
>> +        .name    = "amd_isp_i2c_designware",
>> +        .pm    = pm_ptr(&amd_isp_dw_i2c_dev_pm_ops),
>> +    },
>> +};
>> +
>> +static int __init amd_isp_dw_i2c_init_driver(void)
>> +{
>> +    return platform_driver_register(&amd_isp_dw_i2c_driver);
>> +}
>> +subsys_initcall(amd_isp_dw_i2c_init_driver);
>> +
>> +static void __exit amd_isp_dw_i2c_exit_driver(void)
>> +{
>> +    platform_driver_unregister(&amd_isp_dw_i2c_driver);
>> +}
>> +module_exit(amd_isp_dw_i2c_exit_driver);
>> +
>> +MODULE_AUTHOR("Venkata Narendra Kumar Gutta <vengutta@....com>");
>> +MODULE_AUTHOR("Pratap Nirujogi <pratap.nirujogi@....com>");
>> +MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter in AMD ISP");
>> +MODULE_LICENSE("GPL");
>> +MODULE_IMPORT_NS("I2C_DW");
>> +MODULE_IMPORT_NS("I2C_DW_COMMON");
>> +MODULE_LICENSE("GPL and additional rights");
>> diff --git a/drivers/i2c/busses/i2c-designware-amdisp.h b/drivers/i2c/ 
>> busses/i2c-designware-amdisp.h
>> new file mode 100644
>> index 000000000000..f98661fdaedf
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-designware-amdisp.h
>> @@ -0,0 +1,24 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright 2024-2025 Advanced Micro Devices, Inc.
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be 
>> included in
>> + * all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT 
>> SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
>> DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +int isp_power_set(int on);
> 
> Where does this symbol actually come from?  I didn't see it in this series.
Sorry for the confusion caused. This symbol is coming from ISP driver 
which is not part of this series. We are planning to submit ISP driver 
patches after the completion of platform and sensor driver patches review.

Powered by blists - more mailing lists