* [PATCH] i2c: tegra: Add ACPI support
@ 2021-11-19 13:32 Akhil R
2021-11-19 14:48 ` Andy Shevchenko
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Akhil R @ 2021-11-19 13:32 UTC (permalink / raw)
To: ldewangan, digetx, thierry.reding, jonathanh, p.zabel,
sumit.semwal, christian.koenig, linux-i2c, linux-tegra,
linux-kernel, linux-media, dri-devel, linaro-mm-sig,
andy.shevchenko
Cc: 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] i2c: tegra: Add ACPI support
2021-11-19 13:32 [PATCH] i2c: tegra: Add ACPI support Akhil R
@ 2021-11-19 14:48 ` Andy Shevchenko
2021-11-20 7:36 ` Akhil R
2021-11-22 10:33 ` Dmitry Osipenko
2021-11-22 10:40 ` Dmitry Osipenko
2021-11-23 7:15 ` [PATCH v2] " Akhil R
2 siblings, 2 replies; 13+ messages in thread
From: Andy Shevchenko @ 2021-11-19 14:48 UTC (permalink / raw)
To: Akhil R
Cc: Laxman Dewangan, Dmitry Osipenko, Thierry Reding, Jon Hunter,
Philipp Zabel, Sumit Semwal, Christian Koenig, linux-i2c,
linux-tegra, Linux Kernel Mailing List, Linux Media Mailing List,
dri-devel, linaro-mm-sig
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH] i2c: tegra: Add ACPI support
2021-11-19 14:48 ` Andy Shevchenko
@ 2021-11-20 7:36 ` Akhil R
2021-11-22 10:33 ` Dmitry Osipenko
1 sibling, 0 replies; 13+ messages in thread
From: Akhil R @ 2021-11-20 7:36 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Laxman Dewangan, Dmitry Osipenko, Thierry Reding,
Jonathan Hunter, Philipp Zabel, Sumit Semwal, Christian Koenig,
linux-i2c, linux-tegra, Linux Kernel Mailing List,
Linux Media Mailing List, dri-devel, linaro-mm-sig
>
>
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] i2c: tegra: Add ACPI support
2021-11-19 14:48 ` Andy Shevchenko
2021-11-20 7:36 ` Akhil R
@ 2021-11-22 10:33 ` Dmitry Osipenko
1 sibling, 0 replies; 13+ messages in thread
From: Dmitry Osipenko @ 2021-11-22 10:33 UTC (permalink / raw)
To: Andy Shevchenko, Akhil R
Cc: Laxman Dewangan, Thierry Reding, Jon Hunter, Philipp Zabel,
Sumit Semwal, Christian Koenig, linux-i2c, linux-tegra,
Linux Kernel Mailing List, Linux Media Mailing List, dri-devel,
linaro-mm-sig
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] i2c: tegra: Add ACPI support
2021-11-19 13:32 [PATCH] i2c: tegra: Add ACPI support Akhil R
2021-11-19 14:48 ` Andy Shevchenko
@ 2021-11-22 10:40 ` Dmitry Osipenko
2021-11-23 7:15 ` [PATCH v2] " Akhil R
2 siblings, 0 replies; 13+ messages in thread
From: Dmitry Osipenko @ 2021-11-22 10:40 UTC (permalink / raw)
To: Akhil R, ldewangan, thierry.reding, jonathanh, p.zabel,
sumit.semwal, christian.koenig, linux-i2c, linux-tegra,
linux-kernel, linux-media, dri-devel, linaro-mm-sig,
andy.shevchenko
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] i2c: tegra: Add ACPI support
2021-11-19 13:32 [PATCH] i2c: tegra: Add ACPI support Akhil R
2021-11-19 14:48 ` Andy Shevchenko
2021-11-22 10:40 ` Dmitry Osipenko
@ 2021-11-23 7:15 ` Akhil R
2021-11-23 14:00 ` kernel test robot
` (2 more replies)
2 siblings, 3 replies; 13+ messages in thread
From: Akhil R @ 2021-11-23 7:15 UTC (permalink / raw)
To: andy.shevchenko, christian.koenig, digetx, dri-devel, jonathanh,
ldewangan, linaro-mm-sig, linux-i2c, linux-kernel, linux-media,
linux-tegra, p.zabel, sumit.semwal, thierry.reding
Cc: 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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] i2c: tegra: Add ACPI support
2021-11-23 7:15 ` [PATCH v2] " Akhil R
@ 2021-11-23 14:00 ` kernel test robot
2021-11-23 14:22 ` Dmitry Osipenko
2021-11-23 17:04 ` kernel test robot
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-11-23 14:00 UTC (permalink / raw)
To: Akhil R, andy.shevchenko, christian.koenig, digetx, dri-devel,
jonathanh, ldewangan, linaro-mm-sig, linux-i2c, linux-kernel,
linux-media
Cc: kbuild-all
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] i2c: tegra: Add ACPI support
2021-11-23 7:15 ` [PATCH v2] " Akhil R
2021-11-23 14:00 ` kernel test robot
@ 2021-11-23 14:22 ` Dmitry Osipenko
2021-11-24 7:18 ` Akhil R
2021-11-23 17:04 ` kernel test robot
2 siblings, 1 reply; 13+ messages in thread
From: Dmitry Osipenko @ 2021-11-23 14:22 UTC (permalink / raw)
To: Akhil R, andy.shevchenko, christian.koenig, dri-devel, jonathanh,
ldewangan, linaro-mm-sig, linux-i2c, linux-kernel, linux-media,
linux-tegra, p.zabel, sumit.semwal, thierry.reding
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 ...
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] i2c: tegra: Add ACPI support
2021-11-23 7:15 ` [PATCH v2] " Akhil R
2021-11-23 14:00 ` kernel test robot
2021-11-23 14:22 ` Dmitry Osipenko
@ 2021-11-23 17:04 ` kernel test robot
2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-11-23 17:04 UTC (permalink / raw)
To: Akhil R, andy.shevchenko, christian.koenig, digetx, dri-devel,
jonathanh, ldewangan, linaro-mm-sig, linux-i2c, linux-kernel,
linux-media
Cc: llvm, kbuild-all
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v2] i2c: tegra: Add ACPI support
2021-11-23 14:22 ` Dmitry Osipenko
@ 2021-11-24 7:18 ` Akhil R
2021-11-24 16:05 ` Dmitry Osipenko
0 siblings, 1 reply; 13+ messages in thread
From: Akhil R @ 2021-11-24 7:18 UTC (permalink / raw)
To: Dmitry Osipenko, andy.shevchenko, christian.koenig, dri-devel,
Jonathan Hunter, Laxman Dewangan, linaro-mm-sig, linux-i2c,
linux-kernel, linux-media, linux-tegra, p.zabel, sumit.semwal,
thierry.reding
Cc: Shardar Mohammed
> 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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] i2c: tegra: Add ACPI support
2021-11-24 7:18 ` Akhil R
@ 2021-11-24 16:05 ` Dmitry Osipenko
2021-11-24 16:40 ` Akhil R
0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Osipenko @ 2021-11-24 16:05 UTC (permalink / raw)
To: Akhil R, andy.shevchenko, christian.koenig, dri-devel,
Jonathan Hunter, Laxman Dewangan, linaro-mm-sig, linux-i2c,
linux-kernel, linux-media, linux-tegra, p.zabel, sumit.semwal,
thierry.reding
Cc: Shardar Mohammed
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?
^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH v2] i2c: tegra: Add ACPI support
2021-11-24 16:05 ` Dmitry Osipenko
@ 2021-11-24 16:40 ` Akhil R
2021-11-24 17:29 ` Dmitry Osipenko
0 siblings, 1 reply; 13+ messages in thread
From: Akhil R @ 2021-11-24 16:40 UTC (permalink / raw)
To: Dmitry Osipenko, andy.shevchenko, christian.koenig, dri-devel,
Jonathan Hunter, Laxman Dewangan, linaro-mm-sig, linux-i2c,
linux-kernel, linux-media, linux-tegra, p.zabel, sumit.semwal,
thierry.reding
Cc: Shardar Mohammed
> 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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] i2c: tegra: Add ACPI support
2021-11-24 16:40 ` Akhil R
@ 2021-11-24 17:29 ` Dmitry Osipenko
0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Osipenko @ 2021-11-24 17:29 UTC (permalink / raw)
To: Akhil R, andy.shevchenko, christian.koenig, dri-devel,
Jonathan Hunter, Laxman Dewangan, linaro-mm-sig, linux-i2c,
linux-kernel, linux-media, linux-tegra, p.zabel, sumit.semwal,
thierry.reding
Cc: Shardar Mohammed
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-11-24 17:29 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-19 13:32 [PATCH] i2c: tegra: Add ACPI support Akhil R
2021-11-19 14:48 ` Andy Shevchenko
2021-11-20 7:36 ` Akhil R
2021-11-22 10:33 ` Dmitry Osipenko
2021-11-22 10:40 ` Dmitry Osipenko
2021-11-23 7:15 ` [PATCH v2] " Akhil R
2021-11-23 14:00 ` kernel test robot
2021-11-23 14:22 ` Dmitry Osipenko
2021-11-24 7:18 ` Akhil R
2021-11-24 16:05 ` Dmitry Osipenko
2021-11-24 16:40 ` Akhil R
2021-11-24 17:29 ` Dmitry Osipenko
2021-11-23 17:04 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).