Re: [PATCH v3 2/5] i2c: tegra: Do not configure DMA if not supported

Thierry Reding
2025-07-07T15:24:12Z

[-- Attachment #1: Type: text/plain, Size: 3424 bytes --]

On Mon, Jun 16, 2025 at 10:01:39AM +0000, Kartik Rajput wrote:
> 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.

Maybe in that case we should just use the of_property_present() here?
It's not a big deal, but it could help point out that this is only meant
to work with DT.

> > 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?

That's not ideal either, because it would cause a print about missing
DMA for ACPI where DMA is expected not to work. And it will cause the
messages to be printed for I2C controllers that don't have the GPC DMA
interface.

Properly dealing with this would mean annotating each instance so we
know when this should be an error/warning. I don't think it's worth it
at this point. People will probably rely on the upstream DTSI file for
their needs anyway and we've got the dmas properties there. I think it
can be left as-is.

Or maybe we can add a dev_dbg() message to point this out, that's better
than nothing at all and shouldn't confuse people unnecessarily.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]