linux-kernel - Re: [PATCH v2] i2c: tegra: Add ACPI support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <eebf20ea-6a7f-1120-5ad8-b6dc1f9935e6@gmail.com>
Date:   Tue, 23 Nov 2021 17:22:45 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Akhil R <akhilrajeev@...dia.com>, andy.shevchenko@...il.com,
        christian.koenig@....com, dri-devel@...ts.freedesktop.org,
        jonathanh@...dia.com, ldewangan@...dia.com,
        linaro-mm-sig@...ts.linaro.org, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        linux-tegra@...r.kernel.org, p.zabel@...gutronix.de,
        sumit.semwal@...aro.org, thierry.reding@...il.com
Subject: Re: [PATCH v2] i2c: tegra: Add ACPI support

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@...dia.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@...roid.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@...dia.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 ...