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