Re: [PATCH v2 3/6] i2c: designware: Combine some of the common functions

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

On Thu, Dec 18, 2025 at 04:15:02PM +0100, Heikki Krogerus wrote:
> The adapter can be registered just in the core instead of
> separately in the master and slave drivers. The same applies
> to the interrupt.

...

>  int i2c_dw_probe(struct dw_i2c_dev *dev)
>  {
> +	struct i2c_adapter *adap = &dev->adapter;
> +	unsigned long irq_flags;
> +	int ret;
> +
>  	device_set_node(&dev->adapter.dev, dev_fwnode(dev->dev));
>  
> +	ret = i2c_dw_init_regmap(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_dw_set_sda_hold(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = i2c_dw_set_fifo_size(dev);
> +	if (ret)
> +		return ret;
> +
>  	switch (dev->mode) {
>  	case DW_IC_SLAVE:
> -		return i2c_dw_probe_slave(dev);
> +		ret = i2c_dw_probe_slave(dev);
> +		break;
>  	case DW_IC_MASTER:
> -		return i2c_dw_probe_master(dev);
> +		ret = i2c_dw_probe_master(dev);
> +		break;
>  	default:
> -		dev_err(dev->dev, "Wrong operation mode: %d\n", dev->mode);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		break;
>  	}
> +	if (ret)
> +		return ret;
> +
> +	ret = dev->init(dev);
> +	if (ret)
> +		return ret;

> +	if (!adap->name[0])
> +		strscpy(adap->name, "Synopsys DesignWare I2C adapter");

Hmm... See below.

> +	adap->retries = 3;
> +	adap->algo = &i2c_dw_algo;
> +	adap->quirks = &i2c_dw_quirks;
> +	adap->dev.parent = dev->dev;
> +	i2c_set_adapdata(adap, dev);
> +
> +	/*
> +	 * REVISIT: The mode check may not be necessary.
> +	 * For now keeping the flags as they were originally.
> +	 */
> +	if (dev->mode == DW_IC_SLAVE)
> +		irq_flags = IRQF_SHARED;
> +	else if (dev->flags & ACCESS_NO_IRQ_SUSPEND)
> +		irq_flags = IRQF_NO_SUSPEND;
> +	else
> +		irq_flags = IRQF_SHARED | IRQF_COND_SUSPEND;
> +
> +	ret = i2c_dw_acquire_lock(dev);
> +	if (ret)
> +		return ret;
> +
> +	__i2c_dw_write_intr_mask(dev, 0);
> +	i2c_dw_release_lock(dev);
> +
> +	if (!(dev->flags & ACCESS_POLLING)) {
> +		ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr,
> +				       irq_flags, dev_name(dev->dev), dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Increment PM usage count during adapter registration in order to
> +	 * avoid possible spurious runtime suspend when adapter device is
> +	 * registered to the device core and immediate resume in case bus has
> +	 * registered I2C slaves that do I2C transfers in their probe.
> +	 */
> +	ACQUIRE(pm_runtime_noresume, pm)(dev->dev);
> +	ret = ACQUIRE_ERR(pm_runtime_noresume, &pm);
> +	if (ret)
> +		return ret;
> +
> +	return i2c_add_numbered_adapter(adap);
>  }

...

> -	snprintf(adap->name, sizeof(adap->name),
> -		 "Synopsys DesignWare I2C Slave adapter");

This patch changes the user visible strings (via `i2cdetect`) or module names
in case we want to find it by name (note, we already have such precedents for
other adapters). Currently we have three variants if I not miss anything:
Generic for master (as in this change), Generic for slave, and AMD platform
driver case. If you think this is okay change, then just drop the AMD case
as well, and hence remove the no more needed conditional. Otherwise I would
somehow group this naming in one place, if possible.

-- 
With Best Regards,
Andy Shevchenko