Re: [PATCH 3/3] i2c: qup: Vote for interconnect bandwidth to DRAM

[Stephan Gerhold <stephan.gerhold@linaro.org>]

On Wed, Feb 19, 2025 at 08:30:35PM +0100, Andi Shyti wrote:
> On Wed, Feb 19, 2025 at 11:40:16AM +0100, Stephan Gerhold wrote:
> > On Wed, Feb 19, 2025 at 12:02:06AM +0100, Andi Shyti wrote:
> > > 
> > > sorry for the very late reply here. Just one question.
> > > 
> > 
> > Thanks for bringing the patch back up after such a long time. I've been
> > meaning to resend it, but never found the time to do so... :-)
> 
> We have a long list of forgotten patches that belong to the far
> past. I'm trying to revive them.
> 

Thanks, this is much appreciated!

> [...]
> > > > @@ -1745,6 +1775,11 @@ static int qup_i2c_probe(struct platform_device *pdev)
> > > >  			goto fail_dma;
> > > >  		}
> > > >  		qup->is_dma = true;
> > > > +
> > > > +		qup->icc_path = devm_of_icc_get(&pdev->dev, NULL);
> > > > +		if (IS_ERR(qup->icc_path))
> > > > +			return dev_err_probe(&pdev->dev, PTR_ERR(qup->icc_path),
> > > > +					     "failed to get interconnect path\n");
> > > 
> > > Can we live without it if it fails?
> > > 
> > 
> > of_icc_get() returns NULL if the interconnect API is disabled, or if
> > "interconnects" is not defined in the device tree, so this is already
> > handled. If "interconnects" is enabled and defined, I think we shouldn't
> > ignore errors. Therefore, this should work as intended.
> 
> yes, because qup_i2c_vote_bw() checks inside for NULL values.
> 
> My idea was that:
> 
> 	if (IS_ERR(...)) {
> 		dev_warn(...)
> 		qup->icc_path = NULL;
> 	}
> 
> and let things work. Anyway, if you want to keep it this way,
> fine with me, I don't have a strong opinion, rather than a
> preference to keep going.

I would prefer to keep it the way it is. It's okay to omit the
"interconnects" in the DT (either for old device trees, or because you
don't define the "dmas" either). But if they are defined, we should not
be ignoring errors. -EPROBE_DEFER definitely needs to be handled, but
even for -EINVAL or similar it would be better to make it obvious in my
opinion.

None of the existing users should be affected, since no one defines
"interconnects" at the moment.

Thanks,
Stephan