[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

[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 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

[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.