[v3,00/25] i2c: designware: code consolidation & cleanups

  The series now consists the following groups of patches:
- fixing cleanup order in error path and remove (patches 1-5)
- refactoring i2c_dw_*_lock_support() (patches 6-7)
- refactoring module alias and device ID tables (patches 8-11)
- unifying firmware parsing and configuring code (patches 12-16)
- miscellaneous cleanups (patches 17-18,22-25)
- consolidating PM ops (patch 19)
- using device_set_node() for all drivers (patches 20-21)

The "Consolidate PM ops" might be considered as rft, however I don't think
we have any hardware where the behaviour will be changed, anyways, good
to test.

Changelog v3:
- doubled the size of the series
- fixed compilation error (LKP)
- added tags (Andi)

v2: https://lore.kernel.org/r/20231109182823.3531846-1-andriy.shevchenko@linux.intel.com

Changelog v2:
- reworked the series to make it less twisted (Jarkko, Andi)
- added tags to the patches that have been rebased (Andi, Mario, Jarkko)
- introduced a few new changes (PM ops, export namespace)

v1: https://lore.kernel.org/r/20230725143023.86325-1-andriy.shevchenko@linux.intel.com

Andy Shevchenko (25):
  i2c: designware: Delete adapter before disabling in
    i2c_dw_pci_remove()
  i2c: designware: Fix PM calls order in dw_i2c_plat_probe()
  i2c: designware: Fix reset call order in dw_i2c_plat_probe()
  i2c: designware: Let PCI core to take care about interrupt vectors
  i2c: designware: Fix lock probe call order in dw_i2c_plat_probe()
  i2c: designware: Replace a while-loop by for-loop
  i2c: designware: Save pointer to semaphore callbacks instead of index
  i2c: designware: Add missing 'c' into PCI IDs variable name
  i2c: designware: Replace MODULE_ALIAS() with MODULE_DEVICE_TABLE()
  i2c: designware: Unify terminator in device ID tables
  i2c: designware: Always provide device ID tables
  i2c: designware: Drop return value from i2c_dw_acpi_configure()
  i2c: designware: Drop return value from dw_i2c_of_configure()
  i2c: designware: Rename dw_i2c_of_configure() -> i2c_dw_of_configure()
  i2c: designware: Consolidate firmware parsing and configuring code
  i2c: designware: Unify the firmware type checks
  i2c: designware: Move exports to I2C_DW namespaces
  i2c: designware: Remove ->disable() callback
  i2c: designware: Consolidate PM ops
  i2c: designware: Uninline i2c_dw_probe()
  i2c: designware: Propagate firmware node
  i2c: designware: Use pci_get_drvdata()
  i2c: designware: Use temporary variable for struct device
  i2c: designware: Get rid of redundant 'else'
  i2c: designware: Fix spelling and other issues in the comments

 drivers/i2c/busses/i2c-designware-amdpsp.c  |  10 +-
 drivers/i2c/busses/i2c-designware-common.c  | 166 +++++++++-
 drivers/i2c/busses/i2c-designware-core.h    |  47 +--
 drivers/i2c/busses/i2c-designware-master.c  |  19 +-
 drivers/i2c/busses/i2c-designware-pcidrv.c  | 118 ++-----
 drivers/i2c/busses/i2c-designware-platdrv.c | 337 +++++++-------------
 drivers/i2c/busses/i2c-designware-slave.c   |  12 +-
 7 files changed, 338 insertions(+), 371 deletions(-)
  

On 11/10/2023 12:11, Andy Shevchenko wrote:
> Fix spelling and other issues, such as kernel-doc reported about,
> in the comments. While at it, fix some indentation issues as well.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/i2c/busses/i2c-designware-amdpsp.c  | 10 +++++-----
>   drivers/i2c/busses/i2c-designware-common.c  |  8 +++++---
>   drivers/i2c/busses/i2c-designware-core.h    | 10 +++++-----
>   drivers/i2c/busses/i2c-designware-master.c  | 15 +++++++++------
>   drivers/i2c/busses/i2c-designware-platdrv.c |  4 ++--
>   drivers/i2c/busses/i2c-designware-slave.c   |  6 ++++--
>   6 files changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-amdpsp.c b/drivers/i2c/busses/i2c-designware-amdpsp.c
> index 63454b06e5da..8fbd2a10c31a 100644
> --- a/drivers/i2c/busses/i2c-designware-amdpsp.c
> +++ b/drivers/i2c/busses/i2c-designware-amdpsp.c
> @@ -155,7 +155,7 @@ static void psp_release_i2c_bus_deferred(struct work_struct *work)
>   
>   	/*
>   	 * If there is any pending transaction, cannot release the bus here.
> -	 * psp_release_i2c_bus will take care of this later.
> +	 * psp_release_i2c_bus() will take care of this later.
>   	 */
>   	if (psp_i2c_access_count)
>   		goto cleanup;
> @@ -210,12 +210,12 @@ static void psp_release_i2c_bus(void)
>   {
>   	mutex_lock(&psp_i2c_access_mutex);
>   
> -	/* Return early if mailbox was malfunctional */
> +	/* Return early if mailbox was malfunctioned */
>   	if (psp_i2c_mbox_fail)
>   		goto cleanup;
>   
>   	/*
> -	 * If we are last owner of PSP semaphore, need to release aribtration
> +	 * If we are last owner of PSP semaphore, need to release arbitration
>   	 * via mailbox.
>   	 */
>   	psp_i2c_access_count--;
> @@ -235,9 +235,9 @@ static void psp_release_i2c_bus(void)
>   
>   /*
>    * Locking methods are based on the default implementation from
> - * drivers/i2c/i2c-core-base.c, but with psp acquire and release operations
> + * drivers/i2c/i2c-core-base.c, but with PSP acquire and release operations
>    * added. With this in place we can ensure that i2c clients on the bus shared
> - * with psp are able to lock HW access to the bus for arbitrary number of
> + * with PSP are able to lock HW access to the bus for arbitrary number of
>    * operations - that is e.g. write-wait-read.
>    */
>   static void i2c_adapter_dw_psp_lock_bus(struct i2c_adapter *adapter,
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index 5e28f50e4ce6..91bdedbe1a02 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -127,6 +127,8 @@ static int dw_reg_write_word(void *context, unsigned int reg, unsigned int val)
>    * Autodetects needed register access mode and creates the regmap with
>    * corresponding read/write callbacks. This must be called before doing any
>    * other register access.
> + *
> + * Return: 0 on success, or negative errno otherwise.
>    */
>   int i2c_dw_init_regmap(struct dw_i2c_dev *dev)
>   {
> @@ -174,7 +176,7 @@ int i2c_dw_init_regmap(struct dw_i2c_dev *dev)
>   	/*
>   	 * Note we'll check the return value of the regmap IO accessors only
>   	 * at the probe stage. The rest of the code won't do this because
> -	 * basically we have MMIO-based regmap so non of the read/write methods
> +	 * basically we have MMIO-based regmap, so none of the read/write methods
>   	 * can fail.
>   	 */
>   	dev->map = devm_regmap_init(dev->dev, NULL, dev, &map_cfg);
> @@ -336,7 +338,7 @@ static u32 i2c_dw_acpi_round_bus_speed(struct device *device)
>   
>   	acpi_speed = i2c_acpi_find_bus_speed(device);
>   	/*
> -	 * Some DSTDs use a non standard speed, round down to the lowest
> +	 * Some DSDTs use a non standard speed, round down to the lowest
>   	 * standard speed.
>   	 */
>   	for (i = 0; i < ARRAY_SIZE(supported_speeds); i++) {
> @@ -532,7 +534,7 @@ void __i2c_dw_disable(struct dw_i2c_dev *dev)
>   
>   		/*
>   		 * Wait 10 times the signaling period of the highest I2C
> -		 * transfer supported by the driver (for 400KHz this is
> +		 * transfer supported by the driver (for 400kHz this is
>   		 * 25us) as described in the DesignWare I2C databook.
>   		 */
>   		usleep_range(25, 250);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 7479cb3a450d..5405d4da2b7d 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -141,10 +141,10 @@
>   #define DW_IC_SLAVE				1
>   
>   /*
> - * Hardware abort codes from the DW_IC_TX_ABRT_SOURCE register
> + * Hardware abort codes from the DW_IC_TX_ABRT_SOURCE register.
>    *
> - * Only expected abort codes are listed here
> - * refer to the datasheet for the full list
> + * Only expected abort codes are listed here,
> + * refer to the datasheet for the full list.
>    */
>   #define ABRT_7B_ADDR_NOACK			0
>   #define ABRT_10ADDR1_NOACK			1
> @@ -201,7 +201,7 @@ struct i2c_dw_semaphore_callbacks;
>    * @rst: optional reset for the controller
>    * @slave: represent an I2C slave device
>    * @get_clk_rate_khz: callback to retrieve IP specific bus speed
> - * @cmd_err: run time hadware error code
> + * @cmd_err: run time hardware error code
>    * @msgs: points to an array of messages currently being transferred
>    * @msgs_num: the number of elements in msgs
>    * @msg_write_idx: the element index of the current tx message in the msgs array
> @@ -236,7 +236,7 @@ struct i2c_dw_semaphore_callbacks;
>    * @release_lock: function to release a hardware lock on the bus
>    * @semaphore_idx: Index of table with semaphore type attached to the bus. It's
>    *	-1 if there is no semaphore.
> - * @shared_with_punit: true if this bus is shared with the SoCs PUNIT
> + * @shared_with_punit: true if this bus is shared with the SoC's PUNIT
>    * @init: function to initialize the I2C hardware
>    * @set_sda_hold_time: callback to retrieve IP specific SDA hold timing
>    * @mode: operation mode - DW_IC_MASTER or DW_IC_SLAVE
> diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
> index bd3c8db521de..4ef4160db01f 100644
> --- a/drivers/i2c/busses/i2c-designware-master.c
> +++ b/drivers/i2c/busses/i2c-designware-master.c
> @@ -168,12 +168,14 @@ static int i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
>   }
>   
>   /**
> - * i2c_dw_init_master() - Initialize the designware I2C master hardware
> + * i2c_dw_init_master() - Initialize the DesignWare I2C master hardware
>    * @dev: device private data
>    *
>    * This functions configures and enables the I2C master.
>    * This function is called during I2C init function, and in case of timeout at
>    * run time.
> + *
> + * Return: 0 on success, or negative errno otherwise.
>    */
>   static int i2c_dw_init_master(struct dw_i2c_dev *dev)
>   {
> @@ -314,7 +316,7 @@ static int amd_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msgs,
>   		/*
>   		 * Initiate the i2c read/write transaction of buffer length,
>   		 * and poll for bus busy status. For the last message transfer,
> -		 * update the command with stopbit enable.
> +		 * update the command with stop bit enable.
>   		 */
>   		for (msg_itr_lmt = buf_len; msg_itr_lmt > 0; msg_itr_lmt--) {
>   			if (msg_wrt_idx == num_msgs - 1 && msg_itr_lmt == 1)
> @@ -421,7 +423,7 @@ static int txgbe_i2c_dw_xfer_quirk(struct i2c_adapter *adap, struct i2c_msg *msg
>   
>   /*
>    * Initiate (and continue) low level master read/write transaction.
> - * This function is only called from i2c_dw_isr, and pumping i2c_msg
> + * This function is only called from i2c_dw_isr(), and pumping i2c_msg
>    * messages into the tx buffer.  Even if the size of i2c_msg data is
>    * longer than the size of the tx buffer, it handles everything.
>    */
> @@ -459,7 +461,8 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
>   			buf = msgs[dev->msg_write_idx].buf;
>   			buf_len = msgs[dev->msg_write_idx].len;
>   
> -			/* If both IC_EMPTYFIFO_HOLD_MASTER_EN and
> +			/*
> +			 * If both IC_EMPTYFIFO_HOLD_MASTER_EN and
>   			 * IC_RESTART_EN are set, we must manually
>   			 * set restart bit between messages.
>   			 */
> @@ -949,7 +952,7 @@ static int i2c_dw_init_recovery_info(struct dw_i2c_dev *dev)
>   	rinfo->unprepare_recovery = i2c_dw_unprepare_recovery;
>   	adap->bus_recovery_info = rinfo;
>   
> -	dev_info(dev->dev, "running with gpio recovery mode! scl%s",
> +	dev_info(dev->dev, "running with GPIO recovery mode! scl%s",
>   		 rinfo->sda_gpiod ? ",sda" : "");
>   
>   	return 0;
> @@ -1053,7 +1056,7 @@ int i2c_dw_probe_master(struct dw_i2c_dev *dev)
>   	ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr, irq_flags,
>   			       dev_name(dev->dev), dev);
>   	if (ret) {
> -		dev_err(dev->dev, "failure requesting irq %i: %d\n",
> +		dev_err(dev->dev, "failure requesting IRQ %i: %d\n",
>   			dev->irq, ret);
>   		return ret;
>   	}
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 02dc1d1001f2..fa9c0c56b11e 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -72,7 +72,7 @@ static int bt1_i2c_write(void *context, unsigned int reg, unsigned int val)
>   		return ret;
>   
>   	return regmap_write(dev->sysmap, BT1_I2C_CTL,
> -		BT1_I2C_CTL_GO | BT1_I2C_CTL_WR | (reg & BT1_I2C_CTL_ADDR_MASK));
> +			    BT1_I2C_CTL_GO | BT1_I2C_CTL_WR | (reg & BT1_I2C_CTL_ADDR_MASK));
>   }
>   
>   static struct regmap_config bt1_i2c_cfg = {
> @@ -304,7 +304,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>   	adap = &dev->adapter;
>   	adap->owner = THIS_MODULE;
>   	adap->class = dmi_check_system(dw_i2c_hwmon_class_dmi) ?
> -					I2C_CLASS_HWMON : I2C_CLASS_DEPRECATED;
> +				       I2C_CLASS_HWMON : I2C_CLASS_DEPRECATED;
>   	adap->nr = -1;
>   
>   	if (dev->flags & ACCESS_NO_IRQ_SUSPEND)
> diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
> index 5abd5ec41954..21d80fe81cfe 100644
> --- a/drivers/i2c/busses/i2c-designware-slave.c
> +++ b/drivers/i2c/busses/i2c-designware-slave.c
> @@ -32,12 +32,14 @@ static void i2c_dw_configure_fifo_slave(struct dw_i2c_dev *dev)
>   }
>   
>   /**
> - * i2c_dw_init_slave() - Initialize the designware i2c slave hardware
> + * i2c_dw_init_slave() - Initialize the DesignWare i2c slave hardware
>    * @dev: device private data
>    *
>    * This function configures and enables the I2C in slave mode.
>    * This function is called during I2C init function, and in case of timeout at
>    * run time.
> + *
> + * Return: 0 on success, or negative errno otherwise.
>    */
>   static int i2c_dw_init_slave(struct dw_i2c_dev *dev)
>   {
> @@ -264,7 +266,7 @@ int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
>   	ret = devm_request_irq(dev->dev, dev->irq, i2c_dw_isr_slave,
>   			       IRQF_SHARED, dev_name(dev->dev), dev);
>   	if (ret) {
> -		dev_err(dev->dev, "failure requesting irq %i: %d\n",
> +		dev_err(dev->dev, "failure requesting IRQ %i: %d\n",
>   			dev->irq, ret);
>   		return ret;
>   	}
  

On 11/10/2023 12:11, Andy Shevchenko wrote:
> For the sake of consistency, rename dw_i2c_of_configure() and change
> its parameter to be aligned with the i2c_dw_acpi_configure().
> 
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 7449f1b37ecf..b8606b651feb 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -110,9 +110,10 @@ static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
>   	return 0;
>   }
>   
> -static void dw_i2c_of_configure(struct platform_device *pdev)
> +static void i2c_dw_of_configure(struct device *device)
>   {
> -	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> +	struct platform_device *pdev = to_platform_device(device);
> +	struct dw_i2c_dev *dev = dev_get_drvdata(device);
>   
>   	switch (dev->flags & MODEL_MASK) {
>   	case MODEL_MSCC_OCELOT:
> @@ -130,7 +131,7 @@ static int bt1_i2c_request_regs(struct dw_i2c_dev *dev)
>   	return -ENODEV;
>   }
>   
> -static inline void dw_i2c_of_configure(struct platform_device *pdev)
> +static inline void i2c_dw_of_configure(struct device *device)
>   {
>   }
>   #endif
> @@ -306,7 +307,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
>   	i2c_dw_adjust_bus_speed(dev);
>   
>   	if (pdev->dev.of_node)
> -		dw_i2c_of_configure(pdev);
> +		i2c_dw_of_configure(&pdev->dev);
>   
>   	if (has_acpi_companion(&pdev->dev))
>   		i2c_dw_acpi_configure(&pdev->dev);