[PATCH] i2c: tegra: Add ACPI support

[PATCH] i2c: tegra: Add ACPI support

[Akhil R]

Add support for ACPI based device registration so that the driver
can be also enabled through ACPI table.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 52 +++++++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index c883044..781f747 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -6,6 +6,7 @@
  * Author: Colin Cross <ccross@android.com>
  */
 
+#include <linux/acpi.h>
 #include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
@@ -618,8 +619,13 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	 * emit a noisy warning on error, which won't stay unnoticed and
 	 * won't hose machine entirely.
 	 */
-	err = reset_control_reset(i2c_dev->rst);
-	WARN_ON_ONCE(err);
+	if (has_acpi_companion(i2c_dev->dev)) {
+		acpi_evaluate_object(ACPI_HANDLE(i2c_dev->dev), "_RST",
+				     NULL, NULL);
+	} else {
+		err = reset_control_reset(i2c_dev->rst);
+		WARN_ON_ONCE(err);
+	}
 
 	if (i2c_dev->is_dvc)
 		tegra_dvc_init(i2c_dev);
@@ -1627,12 +1633,12 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
 	bool multi_mode;
 	int err;
 
-	err = of_property_read_u32(np, "clock-frequency",
-				   &i2c_dev->bus_clk_rate);
+	err = device_property_read_u32(i2c_dev->dev, "clock-frequency",
+				       &i2c_dev->bus_clk_rate);
 	if (err)
 		i2c_dev->bus_clk_rate = I2C_MAX_STANDARD_MODE_FREQ;
 
-	multi_mode = of_property_read_bool(np, "multi-master");
+	multi_mode = device_property_read_bool(i2c_dev->dev, "multi-master");
 	i2c_dev->multimaster_mode = multi_mode;
 
 	if (of_device_is_compatible(np, "nvidia,tegra20-i2c-dvc"))
@@ -1684,6 +1690,9 @@ static int tegra_i2c_init_clocks(struct tegra_i2c_dev *i2c_dev)
 
 static void tegra_i2c_release_clocks(struct tegra_i2c_dev *i2c_dev)
 {
+	if (i2c_dev->nclocks == 0)
+		return;
+
 	if (i2c_dev->multimaster_mode)
 		clk_disable(i2c_dev->div_clk);
 
@@ -1720,7 +1729,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	init_completion(&i2c_dev->msg_complete);
 	init_completion(&i2c_dev->dma_complete);
 
-	i2c_dev->hw = of_device_get_match_data(&pdev->dev);
+	i2c_dev->hw = device_get_match_data(&pdev->dev);
 	i2c_dev->cont_id = pdev->id;
 	i2c_dev->dev = &pdev->dev;
 
@@ -1746,18 +1755,20 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
-	if (IS_ERR(i2c_dev->rst)) {
-		dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
-			      "failed to get reset control\n");
-		return PTR_ERR(i2c_dev->rst);
-	}
-
 	tegra_i2c_parse_dt(i2c_dev);
 
-	err = tegra_i2c_init_clocks(i2c_dev);
-	if (err)
-		return err;
+	if (!has_acpi_companion(&pdev->dev)) {
+		i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
+		if (IS_ERR(i2c_dev->rst)) {
+			dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
+				      "failed to get reset control\n");
+			return PTR_ERR(i2c_dev->rst);
+		}
+
+		err = tegra_i2c_init_clocks(i2c_dev);
+		if (err)
+			return err;
+	}
 
 	err = tegra_i2c_init_dma(i2c_dev);
 	if (err)
@@ -1923,12 +1934,21 @@ static const struct dev_pm_ops tegra_i2c_pm = {
 			   NULL)
 };
 
+static const struct acpi_device_id tegra_i2c_acpi_match[] = {
+	{.id = "NVDA0101", .driver_data = (kernel_ulong_t)&tegra210_i2c_hw},
+	{.id = "NVDA0201", .driver_data = (kernel_ulong_t)&tegra186_i2c_hw},
+	{.id = "NVDA0301", .driver_data = (kernel_ulong_t)&tegra194_i2c_hw},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, tegra_i2c_acpi_match);
+
 static struct platform_driver tegra_i2c_driver = {
 	.probe = tegra_i2c_probe,
 	.remove = tegra_i2c_remove,
 	.driver = {
 		.name = "tegra-i2c",
 		.of_match_table = tegra_i2c_of_match,
+		.acpi_match_table = tegra_i2c_acpi_match,
 		.pm = &tegra_i2c_pm,
 	},
 };
-- 
2.7.4

Re: [PATCH] i2c: tegra: Add ACPI support

[Andy Shevchenko]

On Fri, Nov 19, 2021 at 3:37 PM Akhil R <akhilrajeev@nvidia.com> wrote:
>
> Add support for ACPI based device registration so that the driver
> can be also enabled through ACPI table.

the ACPI

...

> +       if (has_acpi_companion(i2c_dev->dev)) {

You are checkin for the companion and using a handle, why not check
for a handle explicitly?

> +               acpi_evaluate_object(ACPI_HANDLE(i2c_dev->dev), "_RST",
> +                                    NULL, NULL);
> +       } else {
> +               err = reset_control_reset(i2c_dev->rst);
> +               WARN_ON_ONCE(err);
> +       }

...

> +       if (i2c_dev->nclocks == 0)
> +               return;

Why? Make clocks optional.

...

> -       i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
> -       if (IS_ERR(i2c_dev->rst)) {

> -               dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
> -                             "failed to get reset control\n");
> -               return PTR_ERR(i2c_dev->rst);

Besides the fact this should be as simple as

return dev_err_probe(...)

> -       }

> +       if (!has_acpi_companion(&pdev->dev)) {

...why do you do this?

> +               i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
> +               if (IS_ERR(i2c_dev->rst)) {
> +                       dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
> +                                     "failed to get reset control\n");
> +                       return PTR_ERR(i2c_dev->rst);
> +               }

...

> +static const struct acpi_device_id tegra_i2c_acpi_match[] = {
> +       {.id = "NVDA0101", .driver_data = (kernel_ulong_t)&tegra210_i2c_hw},
> +       {.id = "NVDA0201", .driver_data = (kernel_ulong_t)&tegra186_i2c_hw},
> +       {.id = "NVDA0301", .driver_data = (kernel_ulong_t)&tegra194_i2c_hw},

> +       { },

No comma for the terminator entry.

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: tegra: Add ACPI support

[Andy Shevchenko]

On Fri, Nov 19, 2021 at 3:37 PM Akhil R <akhilrajeev@nvidia.com> wrote:
>
> Add support for ACPI based device registration so that the driver
> can be also enabled through ACPI table.

the ACPI

...

> +       if (has_acpi_companion(i2c_dev->dev)) {

You are checkin for the companion and using a handle, why not check
for a handle explicitly?

> +               acpi_evaluate_object(ACPI_HANDLE(i2c_dev->dev), "_RST",
> +                                    NULL, NULL);
> +       } else {
> +               err = reset_control_reset(i2c_dev->rst);
> +               WARN_ON_ONCE(err);
> +       }

...

> +       if (i2c_dev->nclocks == 0)
> +               return;

Why? Make clocks optional.

...

> -       i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
> -       if (IS_ERR(i2c_dev->rst)) {

> -               dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
> -                             "failed to get reset control\n");
> -               return PTR_ERR(i2c_dev->rst);

Besides the fact this should be as simple as

return dev_err_probe(...)

> -       }

> +       if (!has_acpi_companion(&pdev->dev)) {

...why do you do this?

> +               i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
> +               if (IS_ERR(i2c_dev->rst)) {
> +                       dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
> +                                     "failed to get reset control\n");
> +                       return PTR_ERR(i2c_dev->rst);
> +               }

...

> +static const struct acpi_device_id tegra_i2c_acpi_match[] = {
> +       {.id = "NVDA0101", .driver_data = (kernel_ulong_t)&tegra210_i2c_hw},
> +       {.id = "NVDA0201", .driver_data = (kernel_ulong_t)&tegra186_i2c_hw},
> +       {.id = "NVDA0301", .driver_data = (kernel_ulong_t)&tegra194_i2c_hw},

> +       { },

No comma for the terminator entry.

> +};

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] i2c: tegra: Add ACPI support

[Akhil R]

> 
> 
> On Fri, Nov 19, 2021 at 3:37 PM Akhil R <akhilrajeev@nvidia.com> wrote:
> >
> > Add support for ACPI based device registration so that the driver can
> > be also enabled through ACPI table.
> 
> the ACPI
> 
> ...
> 
> > +       if (has_acpi_companion(i2c_dev->dev)) {
> 
> You are checkin for the companion and using a handle, why not check for a
> handle explicitly?
Okay.
> 
> > +               acpi_evaluate_object(ACPI_HANDLE(i2c_dev->dev), "_RST",
> > +                                    NULL, NULL);
> > +       } else {
> > +               err = reset_control_reset(i2c_dev->rst);
> > +               WARN_ON_ONCE(err);
> > +       }
> 
> ...
> 
> > +       if (i2c_dev->nclocks == 0)
> > +               return;
> 
> Why? Make clocks optional.
> 
> ...
> 
> > -       i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
> > -       if (IS_ERR(i2c_dev->rst)) {
> 
> > -               dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
> > -                             "failed to get reset control\n");
> > -               return PTR_ERR(i2c_dev->rst);
> 
> Besides the fact this should be as simple as
> 
> return dev_err_probe(...)
> 
> > -       }
> 
> > +       if (!has_acpi_companion(&pdev->dev)) {
> 
> ...why do you do this?
The thought was to call out the error when using device tree and to ignore if using ACPI table. 
We are expecting the clocks to be initialized from the bootloader and to use the _RST method 
(instead of reset_control), when an ACPI table is used.
The problem I thought when making it optional is that an error could go unnoticed when using a 
device tree as well.

Best Regards,
Akhil

RE: [PATCH] i2c: tegra: Add ACPI support

[Akhil R]

> 
> 
> On Fri, Nov 19, 2021 at 3:37 PM Akhil R <akhilrajeev@nvidia.com> wrote:
> >
> > Add support for ACPI based device registration so that the driver can
> > be also enabled through ACPI table.
> 
> the ACPI
> 
> ...
> 
> > +       if (has_acpi_companion(i2c_dev->dev)) {
> 
> You are checkin for the companion and using a handle, why not check for a
> handle explicitly?
Okay.
> 
> > +               acpi_evaluate_object(ACPI_HANDLE(i2c_dev->dev), "_RST",
> > +                                    NULL, NULL);
> > +       } else {
> > +               err = reset_control_reset(i2c_dev->rst);
> > +               WARN_ON_ONCE(err);
> > +       }
> 
> ...
> 
> > +       if (i2c_dev->nclocks == 0)
> > +               return;
> 
> Why? Make clocks optional.
> 
> ...
> 
> > -       i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
> > -       if (IS_ERR(i2c_dev->rst)) {
> 
> > -               dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
> > -                             "failed to get reset control\n");
> > -               return PTR_ERR(i2c_dev->rst);
> 
> Besides the fact this should be as simple as
> 
> return dev_err_probe(...)
> 
> > -       }
> 
> > +       if (!has_acpi_companion(&pdev->dev)) {
> 
> ...why do you do this?
The thought was to call out the error when using device tree and to ignore if using ACPI table. 
We are expecting the clocks to be initialized from the bootloader and to use the _RST method 
(instead of reset_control), when an ACPI table is used.
The problem I thought when making it optional is that an error could go unnoticed when using a 
device tree as well.

Best Regards,
Akhil

Re: [PATCH] i2c: tegra: Add ACPI support

[Dmitry Osipenko]

19.11.2021 17:48, Andy Shevchenko пишет:
>> +       if (i2c_dev->nclocks == 0)
>> +               return;
> Why? Make clocks optional.

This check shouldn't be needed because both clk_disable() and
clk_bulk_unprepare() should handle NULL/zero clocks without problems.

Re: [PATCH] i2c: tegra: Add ACPI support

[Dmitry Osipenko]

19.11.2021 17:48, Andy Shevchenko пишет:
>> +       if (i2c_dev->nclocks == 0)
>> +               return;
> Why? Make clocks optional.

This check shouldn't be needed because both clk_disable() and
clk_bulk_unprepare() should handle NULL/zero clocks without problems.

Re: [PATCH] i2c: tegra: Add ACPI support

[Dmitry Osipenko]

19.11.2021 16:32, Akhil R пишет:
> -	i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
> -	if (IS_ERR(i2c_dev->rst)) {
> -		dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
> -			      "failed to get reset control\n");
> -		return PTR_ERR(i2c_dev->rst);
> -	}
> -
>  	tegra_i2c_parse_dt(i2c_dev);
>  
> -	err = tegra_i2c_init_clocks(i2c_dev);
> -	if (err)
> -		return err;
> +	if (!has_acpi_companion(&pdev->dev)) {
> +		i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
> +		if (IS_ERR(i2c_dev->rst)) {
> +			dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
> +				      "failed to get reset control\n");
> +			return PTR_ERR(i2c_dev->rst);
> +		}
> +
> +		err = tegra_i2c_init_clocks(i2c_dev);
> +		if (err)
> +			return err;
> +	}

What about to factor out the reset initialization into a separate function and write it like this:

static int tegra_i2c_init_reset(i2c_dev)
{
	if (has_acpi_companion(i2c_dev->dev)
		return 0;

	i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
	if (IS_ERR(i2c_dev->rst))
		return dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
			      	     "failed to get reset control\n");

	return 0;
}

And then change tegra_i2c_init_clocks() to:

static int tegra_i2c_init_clocks(i2c_dev)
{
	int err;

	if (has_acpi_companion(i2c_dev->dev))
		return 0;

	...
}

This will make both reset/clocks initialization to look more consistent.

[PATCH v2] i2c: tegra: Add ACPI support

[Akhil R]

Add support for ACPI based device registration so that the driver
can be also enabled through ACPI table.

Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
---
 drivers/i2c/busses/i2c-tegra.c | 52 ++++++++++++++++++++++++++++++++----------
 1 file changed, 40 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index c883044..8e47889 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -6,6 +6,7 @@
  * Author: Colin Cross <ccross@android.com>
  */
 
+#include <linux/acpi.h>
 #include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
@@ -608,6 +609,7 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
 static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 {
 	u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
+	acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
 	int err;
 
 	/*
@@ -618,7 +620,11 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	 * emit a noisy warning on error, which won't stay unnoticed and
 	 * won't hose machine entirely.
 	 */
-	err = reset_control_reset(i2c_dev->rst);
+	if (handle && acpi_has_method(handle, "_RST"))
+		err = (acpi_evaluate_object(handle, "_RST", NULL, NULL));
+	else
+		err = reset_control_reset(i2c_dev->rst);
+
 	WARN_ON_ONCE(err);
 
 	if (i2c_dev->is_dvc)
@@ -1627,12 +1633,12 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
 	bool multi_mode;
 	int err;
 
-	err = of_property_read_u32(np, "clock-frequency",
-				   &i2c_dev->bus_clk_rate);
+	err = device_property_read_u32(i2c_dev->dev, "clock-frequency",
+				       &i2c_dev->bus_clk_rate);
 	if (err)
 		i2c_dev->bus_clk_rate = I2C_MAX_STANDARD_MODE_FREQ;
 
-	multi_mode = of_property_read_bool(np, "multi-master");
+	multi_mode = device_property_read_bool(i2c_dev->dev, "multi-master");
 	i2c_dev->multimaster_mode = multi_mode;
 
 	if (of_device_is_compatible(np, "nvidia,tegra20-i2c-dvc"))
@@ -1642,10 +1648,25 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
 		i2c_dev->is_vi = true;
 }
 
+static int tegra_i2c_init_reset(struct tegra_i2c_dev *i2c_dev)
+{
+	if (has_acpi_companion(i2c_dev->dev))
+		return 0;
+
+	i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
+	if (IS_ERR(i2c_dev->rst))
+		return PTR_ERR(i2c_dev->rst);
+
+	return 0;
+}
+
 static int tegra_i2c_init_clocks(struct tegra_i2c_dev *i2c_dev)
 {
 	int err;
 
+	if (has_acpi_companion(i2c_dev->dev))
+		return 0;
+
 	i2c_dev->clocks[i2c_dev->nclocks++].id = "div-clk";
 
 	if (i2c_dev->hw == &tegra20_i2c_hw || i2c_dev->hw == &tegra30_i2c_hw)
@@ -1720,7 +1741,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	init_completion(&i2c_dev->msg_complete);
 	init_completion(&i2c_dev->dma_complete);
 
-	i2c_dev->hw = of_device_get_match_data(&pdev->dev);
+	i2c_dev->hw = device_get_match_data(&pdev->dev);
 	i2c_dev->cont_id = pdev->id;
 	i2c_dev->dev = &pdev->dev;
 
@@ -1746,15 +1767,13 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
-	i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
-	if (IS_ERR(i2c_dev->rst)) {
-		dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
-			      "failed to get reset control\n");
-		return PTR_ERR(i2c_dev->rst);
-	}
-
 	tegra_i2c_parse_dt(i2c_dev);
 
+	err = tegra_i2c_init_reset(i2c_dev);
+	if (err)
+		return dev_err_probe(i2c_dev->dev, err,
+				      "failed to get reset control\n");
+
 	err = tegra_i2c_init_clocks(i2c_dev);
 	if (err)
 		return err;
@@ -1923,12 +1942,21 @@ static const struct dev_pm_ops tegra_i2c_pm = {
 			   NULL)
 };
 
+static const struct acpi_device_id tegra_i2c_acpi_match[] = {
+	{.id = "NVDA0101", .driver_data = (kernel_ulong_t)&tegra210_i2c_hw},
+	{.id = "NVDA0201", .driver_data = (kernel_ulong_t)&tegra186_i2c_hw},
+	{.id = "NVDA0301", .driver_data = (kernel_ulong_t)&tegra194_i2c_hw},
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, tegra_i2c_acpi_match);
+
 static struct platform_driver tegra_i2c_driver = {
 	.probe = tegra_i2c_probe,
 	.remove = tegra_i2c_remove,
 	.driver = {
 		.name = "tegra-i2c",
 		.of_match_table = tegra_i2c_of_match,
+		.acpi_match_table = tegra_i2c_acpi_match,
 		.pm = &tegra_i2c_pm,
 	},
 };
-- 
2.7.4

Re: [PATCH v2] i2c: tegra: Add ACPI support

[kernel test robot]

Hi Akhil,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tegra/for-next]
[also build test ERROR on v5.16-rc2 next-20211123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Akhil-R/i2c-tegra-Add-ACPI-support/20211123-151636
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: m68k-randconfig-r011-20211123 (https://download.01.org/0day-ci/archive/20211123/202111232153.mpoejDRV-lkp@intel.com/config.gz)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/dec174be801f41a9e42f4381c59c2357c25e40fb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Akhil-R/i2c-tegra-Add-ACPI-support/20211123-151636
        git checkout dec174be801f41a9e42f4381c59c2357c25e40fb
        # save the config file to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/i2c/busses/i2c-tegra.c: In function 'tegra_i2c_init':
>> drivers/i2c/busses/i2c-tegra.c:623:23: error: implicit declaration of function 'acpi_has_method'; did you mean 'acpi_has_watchdog'? [-Werror=implicit-function-declaration]
     623 |         if (handle && acpi_has_method(handle, "_RST"))
         |                       ^~~~~~~~~~~~~~~
         |                       acpi_has_watchdog
   cc1: some warnings being treated as errors


vim +623 drivers/i2c/busses/i2c-tegra.c

   608	
   609	static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
   610	{
   611		u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
   612		acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
   613		int err;
   614	
   615		/*
   616		 * The reset shouldn't ever fail in practice. The failure will be a
   617		 * sign of a severe problem that needs to be resolved. Still we don't
   618		 * want to fail the initialization completely because this may break
   619		 * kernel boot up since voltage regulators use I2C. Hence, we will
   620		 * emit a noisy warning on error, which won't stay unnoticed and
   621		 * won't hose machine entirely.
   622		 */
 > 623		if (handle && acpi_has_method(handle, "_RST"))
   624			err = (acpi_evaluate_object(handle, "_RST", NULL, NULL));
   625		else
   626			err = reset_control_reset(i2c_dev->rst);
   627	
   628		WARN_ON_ONCE(err);
   629	
   630		if (i2c_dev->is_dvc)
   631			tegra_dvc_init(i2c_dev);
   632	
   633		val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN |
   634		      FIELD_PREP(I2C_CNFG_DEBOUNCE_CNT, 2);
   635	
   636		if (i2c_dev->hw->has_multi_master_mode)
   637			val |= I2C_CNFG_MULTI_MASTER_MODE;
   638	
   639		i2c_writel(i2c_dev, val, I2C_CNFG);
   640		i2c_writel(i2c_dev, 0, I2C_INT_MASK);
   641	
   642		if (i2c_dev->is_vi)
   643			tegra_i2c_vi_init(i2c_dev);
   644	
   645		switch (i2c_dev->bus_clk_rate) {
   646		case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ:
   647		default:
   648			tlow = i2c_dev->hw->tlow_fast_fastplus_mode;
   649			thigh = i2c_dev->hw->thigh_fast_fastplus_mode;
   650			tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode;
   651	
   652			if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ)
   653				non_hs_mode = i2c_dev->hw->clk_divisor_fast_plus_mode;
   654			else
   655				non_hs_mode = i2c_dev->hw->clk_divisor_fast_mode;
   656			break;
   657	
   658		case 0 ... I2C_MAX_STANDARD_MODE_FREQ:
   659			tlow = i2c_dev->hw->tlow_std_mode;
   660			thigh = i2c_dev->hw->thigh_std_mode;
   661			tsu_thd = i2c_dev->hw->setup_hold_time_std_mode;
   662			non_hs_mode = i2c_dev->hw->clk_divisor_std_mode;
   663			break;
   664		}
   665	
   666		/* make sure clock divisor programmed correctly */
   667		clk_divisor = FIELD_PREP(I2C_CLK_DIVISOR_HSMODE,
   668					 i2c_dev->hw->clk_divisor_hs_mode) |
   669			      FIELD_PREP(I2C_CLK_DIVISOR_STD_FAST_MODE, non_hs_mode);
   670		i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR);
   671	
   672		if (i2c_dev->hw->has_interface_timing_reg) {
   673			val = FIELD_PREP(I2C_INTERFACE_TIMING_THIGH, thigh) |
   674			      FIELD_PREP(I2C_INTERFACE_TIMING_TLOW, tlow);
   675			i2c_writel(i2c_dev, val, I2C_INTERFACE_TIMING_0);
   676		}
   677	
   678		/*
   679		 * Configure setup and hold times only when tsu_thd is non-zero.
   680		 * Otherwise, preserve the chip default values.
   681		 */
   682		if (i2c_dev->hw->has_interface_timing_reg && tsu_thd)
   683			i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1);
   684	
   685		clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1);
   686	
   687		err = clk_set_rate(i2c_dev->div_clk,
   688				   i2c_dev->bus_clk_rate * clk_multiplier);
   689		if (err) {
   690			dev_err(i2c_dev->dev, "failed to set div-clk rate: %d\n", err);
   691			return err;
   692		}
   693	
   694		if (!i2c_dev->is_dvc && !i2c_dev->is_vi) {
   695			u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG);
   696	
   697			sl_cfg |= I2C_SL_CNFG_NACK | I2C_SL_CNFG_NEWSL;
   698			i2c_writel(i2c_dev, sl_cfg, I2C_SL_CNFG);
   699			i2c_writel(i2c_dev, 0xfc, I2C_SL_ADDR1);
   700			i2c_writel(i2c_dev, 0x00, I2C_SL_ADDR2);
   701		}
   702	
   703		err = tegra_i2c_flush_fifos(i2c_dev);
   704		if (err)
   705			return err;
   706	
   707		if (i2c_dev->multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
   708			i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
   709	
   710		err = tegra_i2c_wait_for_config_load(i2c_dev);
   711		if (err)
   712			return err;
   713	
   714		return 0;
   715	}
   716	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v2] i2c: tegra: Add ACPI support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6315 bytes --]

Hi Akhil,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tegra/for-next]
[also build test ERROR on v5.16-rc2 next-20211123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Akhil-R/i2c-tegra-Add-ACPI-support/20211123-151636
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: m68k-randconfig-r011-20211123 (https://download.01.org/0day-ci/archive/20211123/202111232153.mpoejDRV-lkp(a)intel.com/config.gz)
compiler: m68k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/dec174be801f41a9e42f4381c59c2357c25e40fb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Akhil-R/i2c-tegra-Add-ACPI-support/20211123-151636
        git checkout dec174be801f41a9e42f4381c59c2357c25e40fb
        # save the config file to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=m68k 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/i2c/busses/i2c-tegra.c: In function 'tegra_i2c_init':
>> drivers/i2c/busses/i2c-tegra.c:623:23: error: implicit declaration of function 'acpi_has_method'; did you mean 'acpi_has_watchdog'? [-Werror=implicit-function-declaration]
     623 |         if (handle && acpi_has_method(handle, "_RST"))
         |                       ^~~~~~~~~~~~~~~
         |                       acpi_has_watchdog
   cc1: some warnings being treated as errors


vim +623 drivers/i2c/busses/i2c-tegra.c

   608	
   609	static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
   610	{
   611		u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
   612		acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
   613		int err;
   614	
   615		/*
   616		 * The reset shouldn't ever fail in practice. The failure will be a
   617		 * sign of a severe problem that needs to be resolved. Still we don't
   618		 * want to fail the initialization completely because this may break
   619		 * kernel boot up since voltage regulators use I2C. Hence, we will
   620		 * emit a noisy warning on error, which won't stay unnoticed and
   621		 * won't hose machine entirely.
   622		 */
 > 623		if (handle && acpi_has_method(handle, "_RST"))
   624			err = (acpi_evaluate_object(handle, "_RST", NULL, NULL));
   625		else
   626			err = reset_control_reset(i2c_dev->rst);
   627	
   628		WARN_ON_ONCE(err);
   629	
   630		if (i2c_dev->is_dvc)
   631			tegra_dvc_init(i2c_dev);
   632	
   633		val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN |
   634		      FIELD_PREP(I2C_CNFG_DEBOUNCE_CNT, 2);
   635	
   636		if (i2c_dev->hw->has_multi_master_mode)
   637			val |= I2C_CNFG_MULTI_MASTER_MODE;
   638	
   639		i2c_writel(i2c_dev, val, I2C_CNFG);
   640		i2c_writel(i2c_dev, 0, I2C_INT_MASK);
   641	
   642		if (i2c_dev->is_vi)
   643			tegra_i2c_vi_init(i2c_dev);
   644	
   645		switch (i2c_dev->bus_clk_rate) {
   646		case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ:
   647		default:
   648			tlow = i2c_dev->hw->tlow_fast_fastplus_mode;
   649			thigh = i2c_dev->hw->thigh_fast_fastplus_mode;
   650			tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode;
   651	
   652			if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ)
   653				non_hs_mode = i2c_dev->hw->clk_divisor_fast_plus_mode;
   654			else
   655				non_hs_mode = i2c_dev->hw->clk_divisor_fast_mode;
   656			break;
   657	
   658		case 0 ... I2C_MAX_STANDARD_MODE_FREQ:
   659			tlow = i2c_dev->hw->tlow_std_mode;
   660			thigh = i2c_dev->hw->thigh_std_mode;
   661			tsu_thd = i2c_dev->hw->setup_hold_time_std_mode;
   662			non_hs_mode = i2c_dev->hw->clk_divisor_std_mode;
   663			break;
   664		}
   665	
   666		/* make sure clock divisor programmed correctly */
   667		clk_divisor = FIELD_PREP(I2C_CLK_DIVISOR_HSMODE,
   668					 i2c_dev->hw->clk_divisor_hs_mode) |
   669			      FIELD_PREP(I2C_CLK_DIVISOR_STD_FAST_MODE, non_hs_mode);
   670		i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR);
   671	
   672		if (i2c_dev->hw->has_interface_timing_reg) {
   673			val = FIELD_PREP(I2C_INTERFACE_TIMING_THIGH, thigh) |
   674			      FIELD_PREP(I2C_INTERFACE_TIMING_TLOW, tlow);
   675			i2c_writel(i2c_dev, val, I2C_INTERFACE_TIMING_0);
   676		}
   677	
   678		/*
   679		 * Configure setup and hold times only when tsu_thd is non-zero.
   680		 * Otherwise, preserve the chip default values.
   681		 */
   682		if (i2c_dev->hw->has_interface_timing_reg && tsu_thd)
   683			i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1);
   684	
   685		clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1);
   686	
   687		err = clk_set_rate(i2c_dev->div_clk,
   688				   i2c_dev->bus_clk_rate * clk_multiplier);
   689		if (err) {
   690			dev_err(i2c_dev->dev, "failed to set div-clk rate: %d\n", err);
   691			return err;
   692		}
   693	
   694		if (!i2c_dev->is_dvc && !i2c_dev->is_vi) {
   695			u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG);
   696	
   697			sl_cfg |= I2C_SL_CNFG_NACK | I2C_SL_CNFG_NEWSL;
   698			i2c_writel(i2c_dev, sl_cfg, I2C_SL_CNFG);
   699			i2c_writel(i2c_dev, 0xfc, I2C_SL_ADDR1);
   700			i2c_writel(i2c_dev, 0x00, I2C_SL_ADDR2);
   701		}
   702	
   703		err = tegra_i2c_flush_fifos(i2c_dev);
   704		if (err)
   705			return err;
   706	
   707		if (i2c_dev->multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
   708			i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
   709	
   710		err = tegra_i2c_wait_for_config_load(i2c_dev);
   711		if (err)
   712			return err;
   713	
   714		return 0;
   715	}
   716	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [PATCH v2] i2c: tegra: Add ACPI support

[Dmitry Osipenko]

23.11.2021 10:15, Akhil R пишет:
> Add support for ACPI based device registration so that the driver
> can be also enabled through ACPI table.
> 
> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> ---
>  drivers/i2c/busses/i2c-tegra.c | 52 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 40 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index c883044..8e47889 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -6,6 +6,7 @@
>   * Author: Colin Cross <ccross@android.com>
>   */
>  
> +#include <linux/acpi.h>
>  #include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> @@ -608,6 +609,7 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
>  static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  {
>  	u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
> +	acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
>  	int err;
>  
>  	/*
> @@ -618,7 +620,11 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  	 * emit a noisy warning on error, which won't stay unnoticed and
>  	 * won't hose machine entirely.
>  	 */
> -	err = reset_control_reset(i2c_dev->rst);
> +	if (handle && acpi_has_method(handle, "_RST"))

Which SoC version doesn't have "_RST" method? If neither, then please
remove this check.

> +		err = (acpi_evaluate_object(handle, "_RST", NULL, NULL));

Please remove parens around acpi_evaluate_object(). Why you added them?

> +	else
> +		err = reset_control_reset(i2c_dev->rst);
> +
>  	WARN_ON_ONCE(err);
>  
>  	if (i2c_dev->is_dvc)
> @@ -1627,12 +1633,12 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
>  	bool multi_mode;
>  	int err;
>  
> -	err = of_property_read_u32(np, "clock-frequency",
> -				   &i2c_dev->bus_clk_rate);
> +	err = device_property_read_u32(i2c_dev->dev, "clock-frequency",
> +				       &i2c_dev->bus_clk_rate);
>  	if (err)
>  		i2c_dev->bus_clk_rate = I2C_MAX_STANDARD_MODE_FREQ;
>  
> -	multi_mode = of_property_read_bool(np, "multi-master");
> +	multi_mode = device_property_read_bool(i2c_dev->dev, "multi-master");
>  	i2c_dev->multimaster_mode = multi_mode;
>  
>  	if (of_device_is_compatible(np, "nvidia,tegra20-i2c-dvc"))
> @@ -1642,10 +1648,25 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
>  		i2c_dev->is_vi = true;
>  }
How are you going to differentiate the VI I2C from a non-VI? This
doesn't look right.


>  
> +static int tegra_i2c_init_reset(struct tegra_i2c_dev *i2c_dev)
> +{
> +	if (has_acpi_companion(i2c_dev->dev))
> +		return 0;
> +
> +	i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
> +	if (IS_ERR(i2c_dev->rst))
> +		return PTR_ERR(i2c_dev->rst);
> +
> +	return 0;
> +}
> +
>  static int tegra_i2c_init_clocks(struct tegra_i2c_dev *i2c_dev)
>  {
>  	int err;
>  
> +	if (has_acpi_companion(i2c_dev->dev))
> +		return 0;
> +
>  	i2c_dev->clocks[i2c_dev->nclocks++].id = "div-clk";
>  
>  	if (i2c_dev->hw == &tegra20_i2c_hw || i2c_dev->hw == &tegra30_i2c_hw)
> @@ -1720,7 +1741,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  	init_completion(&i2c_dev->msg_complete);
>  	init_completion(&i2c_dev->dma_complete);
>  
> -	i2c_dev->hw = of_device_get_match_data(&pdev->dev);
> +	i2c_dev->hw = device_get_match_data(&pdev->dev);
>  	i2c_dev->cont_id = pdev->id;
>  	i2c_dev->dev = &pdev->dev;
>  
> @@ -1746,15 +1767,13 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> -	i2c_dev->rst = devm_reset_control_get_exclusive(i2c_dev->dev, "i2c");
> -	if (IS_ERR(i2c_dev->rst)) {
> -		dev_err_probe(i2c_dev->dev, PTR_ERR(i2c_dev->rst),
> -			      "failed to get reset control\n");
> -		return PTR_ERR(i2c_dev->rst);
> -	}
> -
>  	tegra_i2c_parse_dt(i2c_dev);
>  
> +	err = tegra_i2c_init_reset(i2c_dev);
> +	if (err)
> +		return dev_err_probe(i2c_dev->dev, err,
> +				      "failed to get reset control\n");

This is inconsistent with tegra_i2c_init_clocks() which returns err
directly and prints error message within the function. Please move the
dev_err_probe() into tegra_i2c_init_reset() to make it consistent, like
I suggested before.

Please don't reply with a new version of the patch to the old thread,
always send a new version separately. Otherwise it's more difficult to
follow patches.

Lastly, each new version of a patch must contain changelog. I don't see
changelog here, please add it to v3 after the " ---" separator of the
commit message. Thanks!

Example:

 Commit message.

 Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
 ---
  drivers/i2c/busses/i2c-tegra.c | 52
++++++++++++++++++++++++++++++++----------
  1 file changed, 40 insertions(+), 12 deletions(-)

Changelog:

v3: bla-bla-bla

v2: bla-bla-bla

diff ...

RE: [PATCH v2] i2c: tegra: Add ACPI support

[Akhil R]

> 23.11.2021 10:15, Akhil R пишет:
> > Add support for ACPI based device registration so that the driver can
> > be also enabled through ACPI table.
> >
> > Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
> > ---
> >  drivers/i2c/busses/i2c-tegra.c | 52
> > ++++++++++++++++++++++++++++++++----------
> >  1 file changed, 40 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c index c883044..8e47889 100644
> > --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -6,6 +6,7 @@
> >   * Author: Colin Cross <ccross@android.com>
> >   */
> >
> > +#include <linux/acpi.h>
> >  #include <linux/bitfield.h>
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> > @@ -608,6 +609,7 @@ static int tegra_i2c_wait_for_config_load(struct
> > tegra_i2c_dev *i2c_dev)  static int tegra_i2c_init(struct
> > tegra_i2c_dev *i2c_dev)  {
> >       u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh,
> > non_hs_mode;
> > +     acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
> >       int err;
> >
> >       /*
> > @@ -618,7 +620,11 @@ static int tegra_i2c_init(struct tegra_i2c_dev
> *i2c_dev)
> >        * emit a noisy warning on error, which won't stay unnoticed and
> >        * won't hose machine entirely.
> >        */
> > -     err = reset_control_reset(i2c_dev->rst);
> > +     if (handle && acpi_has_method(handle, "_RST"))
> 
> Which SoC version doesn't have "_RST" method? If neither, then please remove
> this check.
> 
> > +             err = (acpi_evaluate_object(handle, "_RST", NULL,
> > + NULL));
> 
> Please remove parens around acpi_evaluate_object(). Why you added them?
> 
> > +     else
> > +             err = reset_control_reset(i2c_dev->rst);
> > +
> >       WARN_ON_ONCE(err);
> >
> >       if (i2c_dev->is_dvc)
> > @@ -1627,12 +1633,12 @@ static void tegra_i2c_parse_dt(struct
> tegra_i2c_dev *i2c_dev)
> >       bool multi_mode;
> >       int err;
> >
> > -     err = of_property_read_u32(np, "clock-frequency",
> > -                                &i2c_dev->bus_clk_rate);
> > +     err = device_property_read_u32(i2c_dev->dev, "clock-frequency",
> > +                                    &i2c_dev->bus_clk_rate);
> >       if (err)
> >               i2c_dev->bus_clk_rate = I2C_MAX_STANDARD_MODE_FREQ;
> >
> > -     multi_mode = of_property_read_bool(np, "multi-master");
> > +     multi_mode = device_property_read_bool(i2c_dev->dev,
> > + "multi-master");
> >       i2c_dev->multimaster_mode = multi_mode;
> >
> >       if (of_device_is_compatible(np, "nvidia,tegra20-i2c-dvc")) @@
> > -1642,10 +1648,25 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev
> *i2c_dev)
> >               i2c_dev->is_vi = true;
> >  }
> How are you going to differentiate the VI I2C from a non-VI? This doesn't look
> right.
This patch adds the ACPI support to only non-VI I2C. The device_ids in match table
are added accordingly. I suppose, of_device_is_compatible always returns false as 
there is no device tree. 
Agree with the other comments.

Re: [PATCH v2] i2c: tegra: Add ACPI support

[Dmitry Osipenko]

24.11.2021 10:18, Akhil R пишет:
>> *i2c_dev)
>>>               i2c_dev->is_vi = true;
>>>  }
>> How are you going to differentiate the VI I2C from a non-VI? This doesn't look
>> right.
> This patch adds the ACPI support to only non-VI I2C. The device_ids in match table
> are added accordingly. I suppose, of_device_is_compatible always returns false as 
> there is no device tree. 
> Agree with the other comments.

Will the VI I2C have a different ACPI ID or how it's going to work?

RE: [PATCH v2] i2c: tegra: Add ACPI support

[Akhil R]

> 24.11.2021 10:18, Akhil R пишет:
> >> *i2c_dev)
> >>>               i2c_dev->is_vi = true;  }
> >> How are you going to differentiate the VI I2C from a non-VI? This
> >> doesn't look right.
> > This patch adds the ACPI support to only non-VI I2C. The device_ids in
> > match table are added accordingly. I suppose, of_device_is_compatible
> > always returns false as there is no device tree.
> > Agree with the other comments.
> 
> Will the VI I2C have a different ACPI ID or how it's going to work?
As there is a different compatible for VI I2C in device tree, I suppose the ACPI
would have a different ID as well. I think the logic would also need an update 
if to have VI I2C using the ACPI. But that wasn't actually considered in this patch.

Best Regards,
Akhil

Re: [PATCH v2] i2c: tegra: Add ACPI support

[Dmitry Osipenko]

24.11.2021 19:40, Akhil R пишет:
>> 24.11.2021 10:18, Akhil R пишет:
>>>> *i2c_dev)
>>>>>               i2c_dev->is_vi = true;  }
>>>> How are you going to differentiate the VI I2C from a non-VI? This
>>>> doesn't look right.
>>> This patch adds the ACPI support to only non-VI I2C. The device_ids in
>>> match table are added accordingly. I suppose, of_device_is_compatible
>>> always returns false as there is no device tree.
>>> Agree with the other comments.
>>
>> Will the VI I2C have a different ACPI ID or how it's going to work?
> As there is a different compatible for VI I2C in device tree, I suppose the ACPI
> would have a different ID as well. I think the logic would also need an update 
> if to have VI I2C using the ACPI. But that wasn't actually considered in this patch.

Thanks, you could reflected it in the commit message.

Re: [PATCH v2] i2c: tegra: Add ACPI support

[kernel test robot]

Hi Akhil,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tegra/for-next]
[also build test ERROR on v5.16-rc2 next-20211123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Akhil-R/i2c-tegra-Add-ACPI-support/20211123-151636
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: riscv-buildonly-randconfig-r005-20211123 (https://download.01.org/0day-ci/archive/20211124/202111240017.bYyZ7knz-lkp@intel.com/config.gz)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 49e3838145dff1ec91c2e67a2cb562775c8d2a08)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/dec174be801f41a9e42f4381c59c2357c25e40fb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Akhil-R/i2c-tegra-Add-ACPI-support/20211123-151636
        git checkout dec174be801f41a9e42f4381c59c2357c25e40fb
        # save the config file to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/i2c/busses/i2c-tegra.c:13:
   In file included from include/linux/dmaengine.h:12:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/i2c/busses/i2c-tegra.c:13:
   In file included from include/linux/dmaengine.h:12:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/i2c/busses/i2c-tegra.c:13:
   In file included from include/linux/dmaengine.h:12:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:1024:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
                                                     ~~~~~~~~~~ ^
>> drivers/i2c/busses/i2c-tegra.c:623:16: error: implicit declaration of function 'acpi_has_method' [-Werror,-Wimplicit-function-declaration]
           if (handle && acpi_has_method(handle, "_RST"))
                         ^
   drivers/i2c/busses/i2c-tegra.c:623:16: note: did you mean 'acpi_has_watchdog'?
   include/linux/acpi.h:1321:20: note: 'acpi_has_watchdog' declared here
   static inline bool acpi_has_watchdog(void) { return false; }
                      ^
   7 warnings and 1 error generated.


vim +/acpi_has_method +623 drivers/i2c/busses/i2c-tegra.c

   608	
   609	static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
   610	{
   611		u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
   612		acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
   613		int err;
   614	
   615		/*
   616		 * The reset shouldn't ever fail in practice. The failure will be a
   617		 * sign of a severe problem that needs to be resolved. Still we don't
   618		 * want to fail the initialization completely because this may break
   619		 * kernel boot up since voltage regulators use I2C. Hence, we will
   620		 * emit a noisy warning on error, which won't stay unnoticed and
   621		 * won't hose machine entirely.
   622		 */
 > 623		if (handle && acpi_has_method(handle, "_RST"))
   624			err = (acpi_evaluate_object(handle, "_RST", NULL, NULL));
   625		else
   626			err = reset_control_reset(i2c_dev->rst);
   627	
   628		WARN_ON_ONCE(err);
   629	
   630		if (i2c_dev->is_dvc)
   631			tegra_dvc_init(i2c_dev);
   632	
   633		val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN |
   634		      FIELD_PREP(I2C_CNFG_DEBOUNCE_CNT, 2);
   635	
   636		if (i2c_dev->hw->has_multi_master_mode)
   637			val |= I2C_CNFG_MULTI_MASTER_MODE;
   638	
   639		i2c_writel(i2c_dev, val, I2C_CNFG);
   640		i2c_writel(i2c_dev, 0, I2C_INT_MASK);
   641	
   642		if (i2c_dev->is_vi)
   643			tegra_i2c_vi_init(i2c_dev);
   644	
   645		switch (i2c_dev->bus_clk_rate) {
   646		case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ:
   647		default:
   648			tlow = i2c_dev->hw->tlow_fast_fastplus_mode;
   649			thigh = i2c_dev->hw->thigh_fast_fastplus_mode;
   650			tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode;
   651	
   652			if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ)
   653				non_hs_mode = i2c_dev->hw->clk_divisor_fast_plus_mode;
   654			else
   655				non_hs_mode = i2c_dev->hw->clk_divisor_fast_mode;
   656			break;
   657	
   658		case 0 ... I2C_MAX_STANDARD_MODE_FREQ:
   659			tlow = i2c_dev->hw->tlow_std_mode;
   660			thigh = i2c_dev->hw->thigh_std_mode;
   661			tsu_thd = i2c_dev->hw->setup_hold_time_std_mode;
   662			non_hs_mode = i2c_dev->hw->clk_divisor_std_mode;
   663			break;
   664		}
   665	
   666		/* make sure clock divisor programmed correctly */
   667		clk_divisor = FIELD_PREP(I2C_CLK_DIVISOR_HSMODE,
   668					 i2c_dev->hw->clk_divisor_hs_mode) |
   669			      FIELD_PREP(I2C_CLK_DIVISOR_STD_FAST_MODE, non_hs_mode);
   670		i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR);
   671	
   672		if (i2c_dev->hw->has_interface_timing_reg) {
   673			val = FIELD_PREP(I2C_INTERFACE_TIMING_THIGH, thigh) |
   674			      FIELD_PREP(I2C_INTERFACE_TIMING_TLOW, tlow);
   675			i2c_writel(i2c_dev, val, I2C_INTERFACE_TIMING_0);
   676		}
   677	
   678		/*
   679		 * Configure setup and hold times only when tsu_thd is non-zero.
   680		 * Otherwise, preserve the chip default values.
   681		 */
   682		if (i2c_dev->hw->has_interface_timing_reg && tsu_thd)
   683			i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1);
   684	
   685		clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1);
   686	
   687		err = clk_set_rate(i2c_dev->div_clk,
   688				   i2c_dev->bus_clk_rate * clk_multiplier);
   689		if (err) {
   690			dev_err(i2c_dev->dev, "failed to set div-clk rate: %d\n", err);
   691			return err;
   692		}
   693	
   694		if (!i2c_dev->is_dvc && !i2c_dev->is_vi) {
   695			u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG);
   696	
   697			sl_cfg |= I2C_SL_CNFG_NACK | I2C_SL_CNFG_NEWSL;
   698			i2c_writel(i2c_dev, sl_cfg, I2C_SL_CNFG);
   699			i2c_writel(i2c_dev, 0xfc, I2C_SL_ADDR1);
   700			i2c_writel(i2c_dev, 0x00, I2C_SL_ADDR2);
   701		}
   702	
   703		err = tegra_i2c_flush_fifos(i2c_dev);
   704		if (err)
   705			return err;
   706	
   707		if (i2c_dev->multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
   708			i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
   709	
   710		err = tegra_i2c_wait_for_config_load(i2c_dev);
   711		if (err)
   712			return err;
   713	
   714		return 0;
   715	}
   716	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH v2] i2c: tegra: Add ACPI support

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 9665 bytes --]

Hi Akhil,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tegra/for-next]
[also build test ERROR on v5.16-rc2 next-20211123]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Akhil-R/i2c-tegra-Add-ACPI-support/20211123-151636
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: riscv-buildonly-randconfig-r005-20211123 (https://download.01.org/0day-ci/archive/20211124/202111240017.bYyZ7knz-lkp(a)intel.com/config.gz)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 49e3838145dff1ec91c2e67a2cb562775c8d2a08)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/dec174be801f41a9e42f4381c59c2357c25e40fb
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Akhil-R/i2c-tegra-Add-ACPI-support/20211123-151636
        git checkout dec174be801f41a9e42f4381c59c2357c25e40fb
        # save the config file to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/i2c/busses/i2c-tegra.c:13:
   In file included from include/linux/dmaengine.h:12:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/i2c/busses/i2c-tegra.c:13:
   In file included from include/linux/dmaengine.h:12:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/i2c/busses/i2c-tegra.c:13:
   In file included from include/linux/dmaengine.h:12:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:1024:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
                                                     ~~~~~~~~~~ ^
>> drivers/i2c/busses/i2c-tegra.c:623:16: error: implicit declaration of function 'acpi_has_method' [-Werror,-Wimplicit-function-declaration]
           if (handle && acpi_has_method(handle, "_RST"))
                         ^
   drivers/i2c/busses/i2c-tegra.c:623:16: note: did you mean 'acpi_has_watchdog'?
   include/linux/acpi.h:1321:20: note: 'acpi_has_watchdog' declared here
   static inline bool acpi_has_watchdog(void) { return false; }
                      ^
   7 warnings and 1 error generated.


vim +/acpi_has_method +623 drivers/i2c/busses/i2c-tegra.c

   608	
   609	static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
   610	{
   611		u32 val, clk_divisor, clk_multiplier, tsu_thd, tlow, thigh, non_hs_mode;
   612		acpi_handle handle = ACPI_HANDLE(i2c_dev->dev);
   613		int err;
   614	
   615		/*
   616		 * The reset shouldn't ever fail in practice. The failure will be a
   617		 * sign of a severe problem that needs to be resolved. Still we don't
   618		 * want to fail the initialization completely because this may break
   619		 * kernel boot up since voltage regulators use I2C. Hence, we will
   620		 * emit a noisy warning on error, which won't stay unnoticed and
   621		 * won't hose machine entirely.
   622		 */
 > 623		if (handle && acpi_has_method(handle, "_RST"))
   624			err = (acpi_evaluate_object(handle, "_RST", NULL, NULL));
   625		else
   626			err = reset_control_reset(i2c_dev->rst);
   627	
   628		WARN_ON_ONCE(err);
   629	
   630		if (i2c_dev->is_dvc)
   631			tegra_dvc_init(i2c_dev);
   632	
   633		val = I2C_CNFG_NEW_MASTER_FSM | I2C_CNFG_PACKET_MODE_EN |
   634		      FIELD_PREP(I2C_CNFG_DEBOUNCE_CNT, 2);
   635	
   636		if (i2c_dev->hw->has_multi_master_mode)
   637			val |= I2C_CNFG_MULTI_MASTER_MODE;
   638	
   639		i2c_writel(i2c_dev, val, I2C_CNFG);
   640		i2c_writel(i2c_dev, 0, I2C_INT_MASK);
   641	
   642		if (i2c_dev->is_vi)
   643			tegra_i2c_vi_init(i2c_dev);
   644	
   645		switch (i2c_dev->bus_clk_rate) {
   646		case I2C_MAX_STANDARD_MODE_FREQ + 1 ... I2C_MAX_FAST_MODE_PLUS_FREQ:
   647		default:
   648			tlow = i2c_dev->hw->tlow_fast_fastplus_mode;
   649			thigh = i2c_dev->hw->thigh_fast_fastplus_mode;
   650			tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode;
   651	
   652			if (i2c_dev->bus_clk_rate > I2C_MAX_FAST_MODE_FREQ)
   653				non_hs_mode = i2c_dev->hw->clk_divisor_fast_plus_mode;
   654			else
   655				non_hs_mode = i2c_dev->hw->clk_divisor_fast_mode;
   656			break;
   657	
   658		case 0 ... I2C_MAX_STANDARD_MODE_FREQ:
   659			tlow = i2c_dev->hw->tlow_std_mode;
   660			thigh = i2c_dev->hw->thigh_std_mode;
   661			tsu_thd = i2c_dev->hw->setup_hold_time_std_mode;
   662			non_hs_mode = i2c_dev->hw->clk_divisor_std_mode;
   663			break;
   664		}
   665	
   666		/* make sure clock divisor programmed correctly */
   667		clk_divisor = FIELD_PREP(I2C_CLK_DIVISOR_HSMODE,
   668					 i2c_dev->hw->clk_divisor_hs_mode) |
   669			      FIELD_PREP(I2C_CLK_DIVISOR_STD_FAST_MODE, non_hs_mode);
   670		i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR);
   671	
   672		if (i2c_dev->hw->has_interface_timing_reg) {
   673			val = FIELD_PREP(I2C_INTERFACE_TIMING_THIGH, thigh) |
   674			      FIELD_PREP(I2C_INTERFACE_TIMING_TLOW, tlow);
   675			i2c_writel(i2c_dev, val, I2C_INTERFACE_TIMING_0);
   676		}
   677	
   678		/*
   679		 * Configure setup and hold times only when tsu_thd is non-zero.
   680		 * Otherwise, preserve the chip default values.
   681		 */
   682		if (i2c_dev->hw->has_interface_timing_reg && tsu_thd)
   683			i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1);
   684	
   685		clk_multiplier = (tlow + thigh + 2) * (non_hs_mode + 1);
   686	
   687		err = clk_set_rate(i2c_dev->div_clk,
   688				   i2c_dev->bus_clk_rate * clk_multiplier);
   689		if (err) {
   690			dev_err(i2c_dev->dev, "failed to set div-clk rate: %d\n", err);
   691			return err;
   692		}
   693	
   694		if (!i2c_dev->is_dvc && !i2c_dev->is_vi) {
   695			u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG);
   696	
   697			sl_cfg |= I2C_SL_CNFG_NACK | I2C_SL_CNFG_NEWSL;
   698			i2c_writel(i2c_dev, sl_cfg, I2C_SL_CNFG);
   699			i2c_writel(i2c_dev, 0xfc, I2C_SL_ADDR1);
   700			i2c_writel(i2c_dev, 0x00, I2C_SL_ADDR2);
   701		}
   702	
   703		err = tegra_i2c_flush_fifos(i2c_dev);
   704		if (err)
   705			return err;
   706	
   707		if (i2c_dev->multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
   708			i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
   709	
   710		err = tegra_i2c_wait_for_config_load(i2c_dev);
   711		if (err)
   712			return err;
   713	
   714		return 0;
   715	}
   716	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org