| Message ID | 1637651753-5067-1-git-send-email-akhilrajeev@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i2c: tegra: Add ACPI support | expand |
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
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 ...
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
> 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.
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?
> 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
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.
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, }, };
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(-)