| Message ID | 20250609093420.3050641-3-kkartik@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add I2C support for Tegra264 | expand |
On Mon, Jun 09, 2025 at 03:04:17PM +0530, Kartik Rajput wrote: > On Tegra264, not all I2C controllers have the necessary interface to > GPC DMA, this causes failures when function tegra_i2c_init_dma() > is called. > > Ensure that "dmas" device-tree property is present before initializing > DMA in function tegra_i2c_init_dma(). > > Signed-off-by: Kartik Rajput <kkartik@nvidia.com> > --- > v1 -> v2: > * Update commit message to clarify that some I2C controllers may > not have the necessary interface to GPC DMA. > --- > drivers/i2c/busses/i2c-tegra.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c > index ebd51165c46b..c7237d26b813 100644 > --- a/drivers/i2c/busses/i2c-tegra.c > +++ b/drivers/i2c/busses/i2c-tegra.c > @@ -448,6 +448,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) > if (IS_VI(i2c_dev)) > return 0; > > + if (!device_property_present(i2c_dev->dev, "dmas")) > + return 0; I know that you use the OF-independent variant here, but has this been tested on ACPI? Originally the intention behind this code was to get some sort of validation of the DT (i.e. dmas property is desired, so we want to flag if it isn't provided) with the fallback existing mostly just so things can operate in the absence (or if APB/GPC DMA isn't available for some reason). If we now solely make this depend on the availability of the DT (or ACPI) property, then we loose all of that validation. I suppose we have DT schema to check for these kinds of things now, but since we're not marking these properties as required, there's really no validation at all anymore. My concern is that if somebody's left out the dmas/dma-names properties by accident, they may not get what they were asking for and we have no hints to provide whatsoever. Maybe that's okay if we provide the base DT, which has been unmodified for a while. If that's what we want to do, it no longer makes sense to keep the IS_VI() check above, though, because that's just redundant now. Thierry
Thanks for reviewing the patch Thierry! On Tue, 2025-06-10 at 10:28 +0200, Thierry Reding wrote: > On Mon, Jun 09, 2025 at 03:04:17PM +0530, Kartik Rajput wrote: > > On Tegra264, not all I2C controllers have the necessary interface > > to > > GPC DMA, this causes failures when function tegra_i2c_init_dma() > > is called. > > > > Ensure that "dmas" device-tree property is present before > > initializing > > DMA in function tegra_i2c_init_dma(). > > > > Signed-off-by: Kartik Rajput <kkartik@nvidia.com> > > --- > > v1 -> v2: > > * Update commit message to clarify that some I2C > > controllers may > > not have the necessary interface to GPC DMA. > > --- > > drivers/i2c/busses/i2c-tegra.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/i2c/busses/i2c-tegra.c > > b/drivers/i2c/busses/i2c-tegra.c > > index ebd51165c46b..c7237d26b813 100644 > > --- a/drivers/i2c/busses/i2c-tegra.c > > +++ b/drivers/i2c/busses/i2c-tegra.c > > @@ -448,6 +448,9 @@ static int tegra_i2c_init_dma(struct > > tegra_i2c_dev *i2c_dev) > > if (IS_VI(i2c_dev)) > > return 0; > > > > + if (!device_property_present(i2c_dev->dev, "dmas")) > > + return 0; > > I know that you use the OF-independent variant here, but has this > been > tested on ACPI? No, Tegra I2C driver does not support DMA with ACPI boot. > > Originally the intention behind this code was to get some sort of > validation of the DT (i.e. dmas property is desired, so we want to > flag > if it isn't provided) with the fallback existing mostly just so > things > can operate in the absence (or if APB/GPC DMA isn't available for > some > reason). > > If we now solely make this depend on the availability of the DT (or > ACPI) property, then we loose all of that validation. I suppose we > have > DT schema to check for these kinds of things now, but since we're not > marking these properties as required, there's really no validation at > all anymore. > > My concern is that if somebody's left out the dmas/dma-names > properties > by accident, they may not get what they were asking for and we have > no > hints to provide whatsoever. Maybe that's okay if we provide the base > DT, which has been unmodified for a while. Should I add an info print here, to indicate that we are missing the "dmas" property? > > If that's what we want to do, it no longer makes sense to keep the > IS_VI() check above, though, because that's just redundant now. Ack, I will move this check above. > > Thierry Thanks & Regards, Kartik
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index ebd51165c46b..c7237d26b813 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -448,6 +448,9 @@ static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev) if (IS_VI(i2c_dev)) return 0; + if (!device_property_present(i2c_dev->dev, "dmas")) + return 0; + if (i2c_dev->hw->has_apb_dma) { if (!IS_ENABLED(CONFIG_TEGRA20_APB_DMA)) { dev_dbg(i2c_dev->dev, "APB DMA support not enabled\n");
On Tegra264, not all I2C controllers have the necessary interface to GPC DMA, this causes failures when function tegra_i2c_init_dma() is called. Ensure that "dmas" device-tree property is present before initializing DMA in function tegra_i2c_init_dma(). Signed-off-by: Kartik Rajput <kkartik@nvidia.com> --- v1 -> v2: * Update commit message to clarify that some I2C controllers may not have the necessary interface to GPC DMA. --- drivers/i2c/busses/i2c-tegra.c | 3 +++ 1 file changed, 3 insertions(+)