Re: [PATCH v1 1/2] drm/xe/i2c: Handler for SMBus Alerts

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

On Tue, Jun 23, 2026 at 01:54:21PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 22, 2026 at 01:47:58PM +0200, Heikki Krogerus wrote:
> > Some devices that are attached to the I2C controller use the
> > SMBus Alert signal for example to inform the host about
> > thermal events, so registering the default SMBus Alert
> > device device for them. The alert device makes sure that
> > the alert is processed and passed to the correct I2C client
> > driver.
> 
> ...
> 
> > +	if (i2c->smbus_alert)
> 
> Unneeded. Also the below incorporates error pointer cases.
> 
> > +		i2c_unregister_device(i2c->smbus_alert);
> > +
> >  	if (!i2c->irqdomain)
> >  		return;
> 
> ...
> 
> > +++ b/drivers/gpu/drm/xe/xe_i2c_smbus.c
> > @@ -0,0 +1,59 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Intel Xe SMBus handling
> > + *
> > + * Copyright (C) 2026 Intel Corporation.
> > + */
> 
> + bits.h // BIT()
> + err.h // IS_ERR()
> 
> > +#include <linux/i2c-smbus.h>
> > +#include <linux/regmap.h>
> > +#include <linux/types.h>
> 
> ...
> 
> > +static struct i2c_smbus_alert_setup xe_i2c_smbus_setup;
> > +
> > +int xe_i2c_register_smbus_alert(struct xe_i2c *i2c)
> > +{
> > +	struct i2c_client *alert;
> > +
> > +	alert = i2c_new_smbus_alert_device(i2c->adapter, &xe_i2c_smbus_setup);
> > +	if (IS_ERR(alert))
> > +		return PTR_ERR(alert);
> > +
> > +	i2c->smbus_alert = alert;
> > +
> > +	return 0;
> > +}
> 
> As per above comment, this IS_ERR() is not need, as the removal API is optional-aware.
> OTOH, the other NULL-check in the driver in that case needs to be IS_ERR_OR_NULL().
> 
> But I'm not sure if you want to have alert setup to always succeed.

Thanks Andy. I'll get these fixed.

I have to refactor the code a little. It does not seem to be possible
to try to conditionally include this file based on CONFIG_I2C_SMBUS
like I'm doing now, so I'm going to propose that CONFIG_XE selects
CONFIG_I2C_SMBUS in v2. Since there are only two functions here, I'll
just move them to xe_i2c.c

cheers,

-- 
heikki