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