Re: [PATCH v4 2/4] drm/xe: Support for I2C attached MCUs

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Fri, Jun 27, 2025 at 11:56:51AM +0300, Heikki Krogerus wrote:
> On Thu, Jun 26, 2025 at 05:21:02PM +0300, Andy Shevchenko wrote:
> > On Thu, Jun 26, 2025 at 04:56:07PM +0300, Heikki Krogerus wrote:

...

> > > +	regmap = devm_regmap_init(i2c->drm_dev, NULL, i2c, &i2c_regmap_config);
> > 
> > Use of i2c->drm_dev makes harder to maintain and understand the code.
> > Managed resources should be carefully attached to the correct device,
> > otherwise it's inevitable object lifetime related issues.
> > 
> > With
> > 
> > 	struct device *dev = xe->drm.dev;
> > 
> > and using local dev, it becomes easier to get and avoid such subtle mistakes.
> 
> I have to disagree with you on this one. Local dev pointers create
> problems because of the assumption that there is only a single device
> in the function to deal with (especially if they are named "dev"),
> which is almost never the case - this function is no exception.

Hmm... In my experience more than 70% of the drivers are okay with this
approach as they do *not* use multiple device pointers. Usually, if we
take IIO for the example, they have a physical device, which is depicted
by the local variable named 'dev' and a Linux IIO device, which is container
that has struct device inside, but it's not used explicitly, IIO APIs use
the pointer to the container (and of course its name differs).

That said, it seems we have different experience (you are most likely talking
about USB cases, where that is indeed quite complicated already).

> But I'll add the local variable as you requested - I'll just name it
> carefully.

Sure.

...

> This kinda related but off topic. IMO in cases like this the regmap
> should be assigned to the child device that is being created instead
> of the parent device. That is currently prevented by the current
> regmap API - the device has to be fully registered before the regmap
> can be assigned (and I'm not referring to the resource managed devm_*
> API), but I'm not convinced that it has to be like that. The problem
> is that the parent device may have multiple child devices that each
> need a dedicated regmag. So just as a note to self: check if we can
> improve the regmap API.

I'm not sure I follow. The (some of) MFD drivers, for instance, work
with many children and one regmapi that is split over the devices.
I don't see any issue with that. The problem is when one tries to mix
regmap and non-regmap approaches in the same (big) driver. That's
a road to mine field.

-- 
With Best Regards,
Andy Shevchenko