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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Thanks for the review Andy.

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:
> > Adding adaption/glue layer where the I2C host adapter
> > (Synopsys DesignWare I2C adapter) and the I2C clients (the
> > microcontroller units) are enumerated.
> > 
> > The microcontroller units (MCU) that are attached to the GPU
> > depend on the OEM. The initially supported MCU will be the
> > Add-In Management Controller (AMC).
> 
> ...
> 
> > +static int xe_i2c_register_adapter(struct xe_i2c *i2c)
> > +{
> > +	struct pci_dev *pci = to_pci_dev(i2c->drm_dev);
> > +	struct platform_device *pdev;
> > +	struct fwnode_handle *fwnode;
> > +	int ret;
> > +
> > +	fwnode = fwnode_create_software_node(xe_i2c_adapter_properties, NULL);
> > +	if (!fwnode)
> > +		return -ENOMEM;
> > +
> > +	/*
> > +	 * Not using platform_device_register_full() here because we don't have
> > +	 * a handle to the platform_device before it returns. xe_i2c_notifier()
> > +	 * uses that handle, but it may be called before
> > +	 * platform_device_register_full() is done.
> > +	 */
> > +	pdev = platform_device_alloc(adapter_name, pci_dev_id(pci));
> > +	if (!pdev) {
> > +		ret = -ENOMEM;
> > +		goto err_fwnode_remove;
> > +	}
> > +
> > +	if (i2c->adapter_irq) {
> 
> > +		struct resource	res = { };
> > +
> > +		res.start = i2c->adapter_irq;
> > +		res.name = "xe_i2c";
> > +		res.flags = IORESOURCE_IRQ;
> 
> 
> 		struct resource	res;
> 
> 		res = DEFINE_RES_IRQ_NAMED(i2c->adapter_irq, "xe_i2c");

I have to check what happened during the internal review round,
because I used that originally. But If there's no real reason not to
continue with it, then yes, I'll use it of course.

> > +		ret = platform_device_add_resources(pdev, &res, 1);
> > +		if (ret)
> > +			goto err_pdev_put;
> > +	}
> > +
> > +	pdev->dev.parent = i2c->drm_dev;
> > +	pdev->dev.fwnode = fwnode;
> > +	i2c->adapter_node = fwnode;
> > +	i2c->pdev = pdev;
> > +
> > +	ret = platform_device_add(pdev);
> > +	if (ret)
> > +		goto err_pdev_put;
> > +
> > +	return 0;
> > +
> > +err_pdev_put:
> > +	platform_device_put(pdev);
> > +err_fwnode_remove:
> > +	fwnode_remove_software_node(fwnode);
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +static int xe_i2c_irq_map(struct irq_domain *h, unsigned int virq,
> > +			  irq_hw_number_t hw_irq_num)
> > +{
> > +	irq_set_chip_and_handler(virq, &dummy_irq_chip, handle_simple_irq);
> 
> Wondering if you need to setup a custom lockdep class here.
> 
> > +	return 0;
> > +}
> 
> ...
> 
> > +static void xe_i2c_remove_irq(struct xe_i2c *i2c)
> > +{
> > +	if (i2c->irqdomain) {
> 
> 	if (!i2c->irqdomain)
> 		return;
> 
> > +		irq_dispose_mapping(i2c->adapter_irq);
> > +		irq_domain_remove(i2c->irqdomain);
> > +	}
> > +}
> 
> ...
> 
> > +static void xe_i2c_remove(void *data)
> > +{
> > +	struct xe_i2c *i2c = data;
> > +	int i;
> 
> unsigned?
> 
> > +	for (i = 0; i < XE_I2C_MAX_CLIENTS; i++)
> > +		i2c_unregister_device(i2c->client[i]);
> > +
> > +	bus_unregister_notifier(&i2c_bus_type, &i2c->bus_notifier);
> > +	xe_i2c_unregister_adapter(i2c);
> > +	xe_i2c_remove_irq(i2c);
> > +}
> 
> ...
> 
> > +int xe_i2c_probe(struct xe_device *xe)
> > +{
> > +	struct xe_i2c_endpoint ep;
> > +	struct regmap *regmap;
> > +	struct xe_i2c *i2c;
> > +	int ret;
> > +
> > +	xe_i2c_read_endpoint(xe_root_tile_mmio(xe), &ep);
> > +	if (ep.cookie != XE_I2C_EP_COOKIE_DEVICE)
> > +		return 0;
> > +
> > +	i2c = devm_kzalloc(xe->drm.dev, sizeof(*i2c), GFP_KERNEL);
> > +	if (!i2c)
> > +		return -ENOMEM;
> > +
> > +	INIT_WORK(&i2c->work, xe_i2c_client_work);
> > +	i2c->mmio = xe_root_tile_mmio(xe);
> > +	i2c->drm_dev = xe->drm.dev;
> > +	i2c->ep = ep;
> 
> > +	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.

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

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.

cheers,

-- 
heikki