[PATCH v1 0/9] i2c: designware: code consolidation & cleanups

[PATCH v1 0/9] i2c: designware: code consolidation & cleanups

[Andy Shevchenko]

Mainly this is about firmware parsing and configuring code
consolidation. Besides that some cleanups here and there.

Andy Shevchenko (9):
  i2c: designware: Move has_acpi_companion() to common code
  i2c: designware: Change i2c_dw_acpi_configure() prototype
  i2c: designware: Align dw_i2c_of_configure() with
    i2c_dw_acpi_configure()
  i2c: designware: Propagate firmware node
  i2c: designware: Always provide ID tables
  i2c: designware: Consolidate firmware parsing and configure code
  i2c: desingware: Unify firmware type checks
  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  |  84 +++++++++++---
 drivers/i2c/busses/i2c-designware-core.h    |  25 ++---
 drivers/i2c/busses/i2c-designware-master.c  |  15 ++-
 drivers/i2c/busses/i2c-designware-pcidrv.c  |  13 +--
 drivers/i2c/busses/i2c-designware-platdrv.c | 117 ++++++--------------
 drivers/i2c/busses/i2c-designware-slave.c   |   6 +-
 7 files changed, 131 insertions(+), 139 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 1/9] i2c: designware: Move has_acpi_companion() to common code

[Andy Shevchenko]

Instead of checking in callers, move the call to the callee.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-common.c  | 11 +++++++++--
 drivers/i2c/busses/i2c-designware-pcidrv.c  |  3 +--
 drivers/i2c/busses/i2c-designware-platdrv.c |  3 +--
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index cdd8c67d9129..683f7a9beb46 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -255,9 +255,8 @@ static void i2c_dw_acpi_params(struct device *device, char method[],
 	kfree(buf.pointer);
 }
 
-int i2c_dw_acpi_configure(struct device *device)
+static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device)
 {
-	struct dw_i2c_dev *dev = dev_get_drvdata(device);
 	struct i2c_timings *t = &dev->timings;
 	u32 ss_ht = 0, fp_ht = 0, hs_ht = 0, fs_ht = 0;
 
@@ -285,6 +284,14 @@ int i2c_dw_acpi_configure(struct device *device)
 		dev->sda_hold_time = fs_ht;
 		break;
 	}
+}
+
+int i2c_dw_acpi_configure(struct device *device)
+{
+	struct dw_i2c_dev *dev = dev_get_drvdata(device);
+
+	if (has_acpi_companion(device))
+		i2c_dw_acpi_do_configure(dev, device);
 
 	return 0;
 }
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 61d7a27aa070..d9d80650fbdc 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -303,8 +303,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 
 	i2c_dw_adjust_bus_speed(dev);
 
-	if (has_acpi_companion(&pdev->dev))
-		i2c_dw_acpi_configure(&pdev->dev);
+	i2c_dw_acpi_configure(&pdev->dev);
 
 	r = i2c_dw_validate_speed(dev);
 	if (r) {
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 970c1c3b0402..4eedb0734438 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -314,8 +314,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	if (pdev->dev.of_node)
 		dw_i2c_of_configure(pdev);
 
-	if (has_acpi_companion(&pdev->dev))
-		i2c_dw_acpi_configure(&pdev->dev);
+	i2c_dw_acpi_configure(&pdev->dev);
 
 	ret = i2c_dw_validate_speed(dev);
 	if (ret)
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 1/9] i2c: designware: Move has_acpi_companion() to common code

[Andi Shyti]

Hi Andy,

On Tue, Jul 25, 2023 at 05:30:15PM +0300, Andy Shevchenko wrote:
> Instead of checking in callers, move the call to the callee.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-common.c  | 11 +++++++++--
>  drivers/i2c/busses/i2c-designware-pcidrv.c  |  3 +--
>  drivers/i2c/busses/i2c-designware-platdrv.c |  3 +--
>  3 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index cdd8c67d9129..683f7a9beb46 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -255,9 +255,8 @@ static void i2c_dw_acpi_params(struct device *device, char method[],
>  	kfree(buf.pointer);
>  }
>  
> -int i2c_dw_acpi_configure(struct device *device)
> +static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device)
>  {
> -	struct dw_i2c_dev *dev = dev_get_drvdata(device);
>  	struct i2c_timings *t = &dev->timings;
>  	u32 ss_ht = 0, fp_ht = 0, hs_ht = 0, fs_ht = 0;
>  
> @@ -285,6 +284,14 @@ int i2c_dw_acpi_configure(struct device *device)
>  		dev->sda_hold_time = fs_ht;
>  		break;
>  	}
> +}
> +
> +int i2c_dw_acpi_configure(struct device *device)

I was about to ask you why are we keeping this int, but then I
saw that you are making it void in the next patch :)

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

Re: [PATCH v1 1/9] i2c: designware: Move has_acpi_companion() to common code

[Jarkko Nikula]

On 7/26/23 00:45, Andi Shyti wrote:
> Hi Andy,
> 
> On Tue, Jul 25, 2023 at 05:30:15PM +0300, Andy Shevchenko wrote:
>> Instead of checking in callers, move the call to the callee.
>>
>> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> ---
>>   drivers/i2c/busses/i2c-designware-common.c  | 11 +++++++++--
>>   drivers/i2c/busses/i2c-designware-pcidrv.c  |  3 +--
>>   drivers/i2c/busses/i2c-designware-platdrv.c |  3 +--
>>   3 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
>> index cdd8c67d9129..683f7a9beb46 100644
>> --- a/drivers/i2c/busses/i2c-designware-common.c
>> +++ b/drivers/i2c/busses/i2c-designware-common.c
>> @@ -255,9 +255,8 @@ static void i2c_dw_acpi_params(struct device *device, char method[],
>>   	kfree(buf.pointer);
>>   }
>>   
>> -int i2c_dw_acpi_configure(struct device *device)
>> +static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device)

Because of this dual dev pointer obscurity which is cleaned in the next 
patch and Andi's comment below in my opinion it makes sense to combine 
patches 1 and 2.

>>   {
>> -	struct dw_i2c_dev *dev = dev_get_drvdata(device);
>>   	struct i2c_timings *t = &dev->timings;
>>   	u32 ss_ht = 0, fp_ht = 0, hs_ht = 0, fs_ht = 0;
>>   
>> @@ -285,6 +284,14 @@ int i2c_dw_acpi_configure(struct device *device)
>>   		dev->sda_hold_time = fs_ht;
>>   		break;
>>   	}
>> +}
>> +
>> +int i2c_dw_acpi_configure(struct device *device)
> 
> I was about to ask you why are we keeping this int, but then I
> saw that you are making it void in the next patch :)
> 

Re: [PATCH v1 1/9] i2c: designware: Move has_acpi_companion() to common code

[Andy Shevchenko]

On Fri, Jul 28, 2023 at 02:33:07PM +0300, Jarkko Nikula wrote:
> On 7/26/23 00:45, Andi Shyti wrote:
> > On Tue, Jul 25, 2023 at 05:30:15PM +0300, Andy Shevchenko wrote:

...

> > > -int i2c_dw_acpi_configure(struct device *device)
> > > +static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device)
> 
> Because of this dual dev pointer obscurity which is cleaned in the next
> patch and Andi's comment below in my opinion it makes sense to combine
> patches 1 and 2.

Besides that these 2 are logically slightly different, the changes don't drop
the duality here. And there is also the other patch at the end of the series
that makes the below disappear.

Not sure that any of these would be the best approach (Git commit is cheap,
maintenance and backporting might be harder). So, ideas are welcome!

...

> > > +int i2c_dw_acpi_configure(struct device *device)
> > 
> > I was about to ask you why are we keeping this int, but then I
> > saw that you are making it void in the next patch :)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/9] i2c: designware: Move has_acpi_companion() to common code

[Jarkko Nikula]

On 7/31/23 23:14, Andy Shevchenko wrote:
> On Fri, Jul 28, 2023 at 02:33:07PM +0300, Jarkko Nikula wrote:
>> On 7/26/23 00:45, Andi Shyti wrote:
>>> On Tue, Jul 25, 2023 at 05:30:15PM +0300, Andy Shevchenko wrote:
> 
> ...
> 
>>>> -int i2c_dw_acpi_configure(struct device *device)
>>>> +static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device)
>>
>> Because of this dual dev pointer obscurity which is cleaned in the next
>> patch and Andi's comment below in my opinion it makes sense to combine
>> patches 1 and 2.
> 
> Besides that these 2 are logically slightly different, the changes don't drop
> the duality here. And there is also the other patch at the end of the series
> that makes the below disappear.
> 
> Not sure that any of these would be the best approach (Git commit is cheap,
> maintenance and backporting might be harder). So, ideas are welcome!
> 
Unless I'm missing something you won't need to carry both struct 
dw_i2c_dev *dev and struct device *device since struct dw_i2c_dev 
carries it already and it's set before calling the dw_i2c_of_configure() 
and i2c_dw_acpi_configure().

Also it feels needless to add new _do_configure() functions since only 
reason for them seems to be how patches are organized now.

So if instead of this in i2c_dw_fw_parse_and_configure()

	if (is_of_node(fwnode))
		i2c_dw_of_do_configure(dev, dev->dev);
	else if (is_acpi_node(fwnode))
		i2c_dw_acpi_do_configure(dev, dev->dev);

let end result be

	if (is_of_node(fwnode))
		i2c_dw_of_configure(dev);
	else if (is_acpi_node(fwnode))
		i2c_dw_acpi_configure(dev);

My gut feeling says patchset would be a bit simpler if we aim for this 
end result in mind.

Simplest patches like int to void return type conversion first since 
either i2c_dw_acpi_configure() and dw_i2c_of_configure() return is not 
used now. Then perhaps dw_i2c_of_configure() renaming.

Moving to common code I don't know how well it's splittable into smaller 
patches or would single bigger patch look better.

Re: [PATCH v1 1/9] i2c: designware: Move has_acpi_companion() to common code

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2303 bytes --]

On Thu, Aug 03, 2023 at 04:43:32PM +0300, Jarkko Nikula wrote:
> On 7/31/23 23:14, Andy Shevchenko wrote:
> > On Fri, Jul 28, 2023 at 02:33:07PM +0300, Jarkko Nikula wrote:
> > > On 7/26/23 00:45, Andi Shyti wrote:
> > > > On Tue, Jul 25, 2023 at 05:30:15PM +0300, Andy Shevchenko wrote:
> > 
> > ...
> > 
> > > > > -int i2c_dw_acpi_configure(struct device *device)
> > > > > +static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device)
> > > 
> > > Because of this dual dev pointer obscurity which is cleaned in the next
> > > patch and Andi's comment below in my opinion it makes sense to combine
> > > patches 1 and 2.
> > 
> > Besides that these 2 are logically slightly different, the changes don't drop
> > the duality here. And there is also the other patch at the end of the series
> > that makes the below disappear.
> > 
> > Not sure that any of these would be the best approach (Git commit is cheap,
> > maintenance and backporting might be harder). So, ideas are welcome!
> > 
> Unless I'm missing something you won't need to carry both struct dw_i2c_dev
> *dev and struct device *device since struct dw_i2c_dev carries it already
> and it's set before calling the dw_i2c_of_configure() and
> i2c_dw_acpi_configure().
> 
> Also it feels needless to add new _do_configure() functions since only
> reason for them seems to be how patches are organized now.
> 
> So if instead of this in i2c_dw_fw_parse_and_configure()
> 
> 	if (is_of_node(fwnode))
> 		i2c_dw_of_do_configure(dev, dev->dev);
> 	else if (is_acpi_node(fwnode))
> 		i2c_dw_acpi_do_configure(dev, dev->dev);
> 
> let end result be
> 
> 	if (is_of_node(fwnode))
> 		i2c_dw_of_configure(dev);
> 	else if (is_acpi_node(fwnode))
> 		i2c_dw_acpi_configure(dev);
> 
> My gut feeling says patchset would be a bit simpler if we aim for this end
> result in mind.
> 
> Simplest patches like int to void return type conversion first since either
> i2c_dw_acpi_configure() and dw_i2c_of_configure() return is not used now.
> Then perhaps dw_i2c_of_configure() renaming.
> 
> Moving to common code I don't know how well it's splittable into smaller
> patches or would single bigger patch look better.

Does this all mean that the series needs to be refactored?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 1/9] i2c: designware: Move has_acpi_companion() to common code

[Andy Shevchenko]

On Sun, Sep 24, 2023 at 10:56:00PM +0200, Wolfram Sang wrote:
> On Thu, Aug 03, 2023 at 04:43:32PM +0300, Jarkko Nikula wrote:
> > On 7/31/23 23:14, Andy Shevchenko wrote:

...

> > Moving to common code I don't know how well it's splittable into smaller
> > patches or would single bigger patch look better.
> 
> Does this all mean that the series needs to be refactored?

At least that is my impression. Just have no time to look at it (yet).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/9] i2c: designware: Move has_acpi_companion() to common code

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 221 bytes --]


> > Does this all mean that the series needs to be refactored?
> 
> At least that is my impression. Just have no time to look at it (yet).

Okay, I'll set it to 'changes requested' then. Thanks for the heads up!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 2/9] i2c: designware: Change i2c_dw_acpi_configure() prototype

[Andy Shevchenko]

There is no point to have it being int and at the same time
it may take struct dw_i2c_dev pointer instead of plain device.

Change the prototype and implementation accordingly.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-common.c  | 10 +++-------
 drivers/i2c/busses/i2c-designware-core.h    |  4 ++--
 drivers/i2c/busses/i2c-designware-pcidrv.c  |  2 +-
 drivers/i2c/busses/i2c-designware-platdrv.c |  2 +-
 4 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 683f7a9beb46..222b530c0441 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -286,14 +286,10 @@ static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *devi
 	}
 }
 
-int i2c_dw_acpi_configure(struct device *device)
+void i2c_dw_acpi_configure(struct dw_i2c_dev *dev)
 {
-	struct dw_i2c_dev *dev = dev_get_drvdata(device);
-
-	if (has_acpi_companion(device))
-		i2c_dw_acpi_do_configure(dev, device);
-
-	return 0;
+	if (has_acpi_companion(dev->dev))
+		i2c_dw_acpi_do_configure(dev, dev->dev);
 }
 EXPORT_SYMBOL_GPL(i2c_dw_acpi_configure);
 
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index cf4f684f5356..03f4d44ae94c 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -394,7 +394,7 @@ int i2c_dw_validate_speed(struct dw_i2c_dev *dev);
 void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev);
 
 #if IS_ENABLED(CONFIG_ACPI)
-int i2c_dw_acpi_configure(struct device *device);
+void i2c_dw_acpi_configure(struct dw_i2c_dev *dev);
 #else
-static inline int i2c_dw_acpi_configure(struct device *device) { return -ENODEV; }
+static inline void i2c_dw_acpi_configure(struct dw_i2c_dev *dev) { }
 #endif
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index d9d80650fbdc..7f5a04538c71 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -303,7 +303,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 
 	i2c_dw_adjust_bus_speed(dev);
 
-	i2c_dw_acpi_configure(&pdev->dev);
+	i2c_dw_acpi_configure(dev);
 
 	r = i2c_dw_validate_speed(dev);
 	if (r) {
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 4eedb0734438..c60e55b8398e 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -314,7 +314,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	if (pdev->dev.of_node)
 		dw_i2c_of_configure(pdev);
 
-	i2c_dw_acpi_configure(&pdev->dev);
+	i2c_dw_acpi_configure(dev);
 
 	ret = i2c_dw_validate_speed(dev);
 	if (ret)
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 2/9] i2c: designware: Change i2c_dw_acpi_configure() prototype

[Andi Shyti]

Hi Andy,

On Tue, Jul 25, 2023 at 05:30:16PM +0300, Andy Shevchenko wrote:
> There is no point to have it being int and at the same time
> it may take struct dw_i2c_dev pointer instead of plain device.
> 
> Change the prototype and implementation accordingly.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

[PATCH v1 3/9] i2c: designware: Align dw_i2c_of_configure() with i2c_dw_acpi_configure()

[Andy Shevchenko]

For the sake of consistency align dw_i2c_of_configure() with
i2c_dw_acpi_configure() and rename the former to i2c_dw_of_configure().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index c60e55b8398e..d35a6bbcb6fb 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -132,9 +132,9 @@ static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
 	return 0;
 }
 
-static int dw_i2c_of_configure(struct platform_device *pdev)
+static void i2c_dw_of_do_configure(struct dw_i2c_dev *dev, struct device *device)
 {
-	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
+	struct platform_device *pdev = to_platform_device(device);
 
 	switch (dev->flags & MODEL_MASK) {
 	case MODEL_MSCC_OCELOT:
@@ -145,8 +145,12 @@ static int dw_i2c_of_configure(struct platform_device *pdev)
 	default:
 		break;
 	}
+}
 
-	return 0;
+static void i2c_dw_of_configure(struct dw_i2c_dev *dev)
+{
+	if (dev_of_node(dev->dev))
+		i2c_dw_of_do_configure(dev, dev->dev);
 }
 
 static const struct of_device_id dw_i2c_of_match[] = {
@@ -162,10 +166,7 @@ static int bt1_i2c_request_regs(struct dw_i2c_dev *dev)
 	return -ENODEV;
 }
 
-static inline int dw_i2c_of_configure(struct platform_device *pdev)
-{
-	return -ENODEV;
-}
+static inline void i2c_dw_of_configure(struct dw_i2c_dev *dev) { }
 #endif
 
 static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev)
@@ -311,9 +312,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(dev);
 	i2c_dw_acpi_configure(dev);
 
 	ret = i2c_dw_validate_speed(dev);
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 3/9] i2c: designware: Align dw_i2c_of_configure() with i2c_dw_acpi_configure()

[Andi Shyti]

Hi Andy,

On Tue, Jul 25, 2023 at 05:30:17PM +0300, Andy Shevchenko wrote:
> For the sake of consistency align dw_i2c_of_configure() with
> i2c_dw_acpi_configure() and rename the former to i2c_dw_of_configure().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index c60e55b8398e..d35a6bbcb6fb 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -132,9 +132,9 @@ static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
>  	return 0;
>  }
>  
> -static int dw_i2c_of_configure(struct platform_device *pdev)
> +static void i2c_dw_of_do_configure(struct dw_i2c_dev *dev, struct device *device)
>  {
> -	struct dw_i2c_dev *dev = platform_get_drvdata(pdev);
> +	struct platform_device *pdev = to_platform_device(device);
>  
>  	switch (dev->flags & MODEL_MASK) {
>  	case MODEL_MSCC_OCELOT:
> @@ -145,8 +145,12 @@ static int dw_i2c_of_configure(struct platform_device *pdev)
>  	default:
>  		break;
>  	}
> +}
>  
> -	return 0;
> +static void i2c_dw_of_configure(struct dw_i2c_dev *dev)
> +{
> +	if (dev_of_node(dev->dev))
> +		i2c_dw_of_do_configure(dev, dev->dev);

You could add this check above and avoid this micro-if-functions.

	if (!dev_of_node(dev->dev))
		return;

up to you...

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

>  }
>  
>  static const struct of_device_id dw_i2c_of_match[] = {
> @@ -162,10 +166,7 @@ static int bt1_i2c_request_regs(struct dw_i2c_dev *dev)
>  	return -ENODEV;
>  }
>  
> -static inline int dw_i2c_of_configure(struct platform_device *pdev)
> -{
> -	return -ENODEV;
> -}
> +static inline void i2c_dw_of_configure(struct dw_i2c_dev *dev) { }
>  #endif
>  
>  static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev)
> @@ -311,9 +312,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(dev);
>  	i2c_dw_acpi_configure(dev);
>  
>  	ret = i2c_dw_validate_speed(dev);
> -- 
> 2.40.0.1.gaa8946217a0b
> 

Re: [PATCH v1 3/9] i2c: designware: Align dw_i2c_of_configure() with i2c_dw_acpi_configure()

[Andy Shevchenko]

On Tue, Jul 25, 2023 at 11:48:36PM +0200, Andi Shyti wrote:
> On Tue, Jul 25, 2023 at 05:30:17PM +0300, Andy Shevchenko wrote:

...

> > +static void i2c_dw_of_configure(struct dw_i2c_dev *dev)
> > +{
> > +	if (dev_of_node(dev->dev))
> > +		i2c_dw_of_do_configure(dev, dev->dev);
> 
> You could add this check above and avoid this micro-if-functions.
> 
> 	if (!dev_of_node(dev->dev))
> 		return;
> 
> up to you...

Have you had a chance to look into patch 7? Maybe you can come up with some
advice or ideas on how to make the series better...

> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thank you!

> >  }

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 4/9] i2c: designware: Propagate firmware node

[Andy Shevchenko]

Propagate firmware node by using a specific API call, i.e. device_set_node().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-core.h    | 6 ++++--
 drivers/i2c/busses/i2c-designware-pcidrv.c  | 2 --
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 --
 3 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 03f4d44ae94c..f0c683ad860f 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -10,11 +10,11 @@
  */
 
 #include <linux/bits.h>
-#include <linux/compiler_types.h>
 #include <linux/completion.h>
-#include <linux/dev_printk.h>
+#include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/i2c.h>
+#include <linux/property.h>
 #include <linux/regmap.h>
 #include <linux/types.h>
 
@@ -363,6 +363,8 @@ static inline int i2c_dw_probe_slave(struct dw_i2c_dev *dev) { return -EINVAL; }
 
 static inline int i2c_dw_probe(struct dw_i2c_dev *dev)
 {
+	device_set_node(&dev->adapter.dev, dev_fwnode(dev->dev));
+
 	switch (dev->mode) {
 	case DW_IC_SLAVE:
 		return i2c_dw_probe_slave(dev);
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 7f5a04538c71..a42a47e0032d 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -9,7 +9,6 @@
  * Copyright (C) 2009 Provigent Ltd.
  * Copyright (C) 2011, 2015, 2016 Intel Corporation.
  */
-#include <linux/acpi.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/errno.h>
@@ -325,7 +324,6 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	adap = &dev->adapter;
 	adap->owner = THIS_MODULE;
 	adap->class = 0;
-	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
 	adap->nr = controller->bus_num;
 
 	r = i2c_dw_probe(dev);
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d35a6bbcb6fb..512fb1d8ddfc 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -357,8 +357,6 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 	adap->owner = THIS_MODULE;
 	adap->class = dmi_check_system(dw_i2c_hwmon_class_dmi) ?
 					I2C_CLASS_HWMON : I2C_CLASS_DEPRECATED;
-	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
-	adap->dev.of_node = pdev->dev.of_node;
 	adap->nr = -1;
 
 	if (dev->flags & ACCESS_NO_IRQ_SUSPEND) {
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 4/9] i2c: designware: Propagate firmware node

[Jarkko Nikula]

On 7/25/23 17:30, Andy Shevchenko wrote:
> Propagate firmware node by using a specific API call, i.e. device_set_node().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/i2c/busses/i2c-designware-core.h    | 6 ++++--
>   drivers/i2c/busses/i2c-designware-pcidrv.c  | 2 --
>   drivers/i2c/busses/i2c-designware-platdrv.c | 2 --
>   3 files changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 03f4d44ae94c..f0c683ad860f 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -10,11 +10,11 @@
>    */
>   
>   #include <linux/bits.h>
> -#include <linux/compiler_types.h>
>   #include <linux/completion.h>
> -#include <linux/dev_printk.h>
> +#include <linux/device.h>
>   #include <linux/errno.h>
>   #include <linux/i2c.h>
> +#include <linux/property.h>
>   #include <linux/regmap.h>
>   #include <linux/types.h>
>   
> @@ -363,6 +363,8 @@ static inline int i2c_dw_probe_slave(struct dw_i2c_dev *dev) { return -EINVAL; }
>   
>   static inline int i2c_dw_probe(struct dw_i2c_dev *dev)
>   {
> +	device_set_node(&dev->adapter.dev, dev_fwnode(dev->dev));
> +

Would this be better to put in the same place where ACPI_COMPANION_SET() 
is removed like below? I'd keep this static inline function in the 
header file as simple as possible. All extra code might invite adding 
even more.

>   	switch (dev->mode) {
>   	case DW_IC_SLAVE:
>   		return i2c_dw_probe_slave(dev);
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 7f5a04538c71..a42a47e0032d 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -9,7 +9,6 @@
>    * Copyright (C) 2009 Provigent Ltd.
>    * Copyright (C) 2011, 2015, 2016 Intel Corporation.
>    */
> -#include <linux/acpi.h>
>   #include <linux/delay.h>
>   #include <linux/err.h>
>   #include <linux/errno.h>
> @@ -325,7 +324,6 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
>   	adap = &dev->adapter;
>   	adap->owner = THIS_MODULE;
>   	adap->class = 0;
> -	ACPI_COMPANION_SET(&adap->dev, ACPI_COMPANION(&pdev->dev));
>   	adap->nr = controller->bus_num;

Re: [PATCH v1 4/9] i2c: designware: Propagate firmware node

[Andy Shevchenko]

On Fri, Jul 28, 2023 at 03:25:58PM +0300, Jarkko Nikula wrote:
> On 7/25/23 17:30, Andy Shevchenko wrote:
> > Propagate firmware node by using a specific API call, i.e. device_set_node().

...

> > +	device_set_node(&dev->adapter.dev, dev_fwnode(dev->dev));
> 
> Would this be better to put in the same place where ACPI_COMPANION_SET() is
> removed like below? I'd keep this static inline function in the header file
> as simple as possible. All extra code might invite adding even more.

We come again to the duplication and prone to deviation code, I wouldn't like
to go this way. The idea of this call is to unify and avoid mistakes, like
updating only in ACPI or DT (or any new one if happens in the future) case
and leaving the second one unconsidered.

That said, I would rather drop this patch until i2c core will take this
once for all (may be never in the reasonable future :-).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/9] i2c: designware: Propagate firmware node

[Andi Shyti]

Hi Andy,

On Mon, Jul 31, 2023 at 11:09:24PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 28, 2023 at 03:25:58PM +0300, Jarkko Nikula wrote:
> > On 7/25/23 17:30, Andy Shevchenko wrote:
> > > Propagate firmware node by using a specific API call, i.e. device_set_node().
> 
> ...
> 
> > > +	device_set_node(&dev->adapter.dev, dev_fwnode(dev->dev));
> > 
> > Would this be better to put in the same place where ACPI_COMPANION_SET() is
> > removed like below? I'd keep this static inline function in the header file
> > as simple as possible. All extra code might invite adding even more.
> 
> We come again to the duplication and prone to deviation code, I wouldn't like
> to go this way. The idea of this call is to unify and avoid mistakes, like
> updating only in ACPI or DT (or any new one if happens in the future) case
> and leaving the second one unconsidered.

it's anyway an inline function becoming a bit too fat. Can't we
make it not inline?

> That said, I would rather drop this patch until i2c core will take this
> once for all (may be never in the reasonable future :-).

Which patch are you referring to that should be taken into i2c
core?

Andi

Re: [PATCH v1 4/9] i2c: designware: Propagate firmware node

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 10:59:56PM +0200, Andi Shyti wrote:
> On Mon, Jul 31, 2023 at 11:09:24PM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 28, 2023 at 03:25:58PM +0300, Jarkko Nikula wrote:
> > > On 7/25/23 17:30, Andy Shevchenko wrote:
> > > > Propagate firmware node by using a specific API call, i.e. device_set_node().

...

> > > > +	device_set_node(&dev->adapter.dev, dev_fwnode(dev->dev));
> > > 
> > > Would this be better to put in the same place where ACPI_COMPANION_SET() is
> > > removed like below? I'd keep this static inline function in the header file
> > > as simple as possible. All extra code might invite adding even more.
> > 
> > We come again to the duplication and prone to deviation code, I wouldn't like
> > to go this way. The idea of this call is to unify and avoid mistakes, like
> > updating only in ACPI or DT (or any new one if happens in the future) case
> > and leaving the second one unconsidered.
> 
> it's anyway an inline function becoming a bit too fat. Can't we
> make it not inline?
> 
> > That said, I would rather drop this patch until i2c core will take this
> > once for all (may be never in the reasonable future :-).
> 
> Which patch are you referring to that should be taken into i2c
> core?

Something I tried to do in the past but failed.
https://lore.kernel.org/linux-i2c/20211207162457.18450-1-andriy.shevchenko@linux.intel.com/

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 5/9] i2c: designware: Always provide ID tables

[Andy Shevchenko]

There is no need to have ugly ifdeffery and additional macros
for the ID tables. Always provide them. Since we touch the
ACPI table, make it sorted by ID.

While at it, group MODULE_ALIAS() with other MODULE_*() macros.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 65 ++++++++++-----------
 1 file changed, 31 insertions(+), 34 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 512fb1d8ddfc..d2ffd041c0c7 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -40,28 +40,6 @@ static u32 i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
 	return clk_get_rate(dev->clk) / KILO;
 }
 
-#ifdef CONFIG_ACPI
-static const struct acpi_device_id dw_i2c_acpi_match[] = {
-	{ "INT33C2", 0 },
-	{ "INT33C3", 0 },
-	{ "INT3432", 0 },
-	{ "INT3433", 0 },
-	{ "80860F41", ACCESS_NO_IRQ_SUSPEND },
-	{ "808622C1", ACCESS_NO_IRQ_SUSPEND },
-	{ "AMD0010", ACCESS_INTR_MASK },
-	{ "AMDI0010", ACCESS_INTR_MASK },
-	{ "AMDI0019", ACCESS_INTR_MASK | ARBITRATION_SEMAPHORE },
-	{ "AMDI0510", 0 },
-	{ "APMC0D0F", 0 },
-	{ "HISI02A1", 0 },
-	{ "HISI02A2", 0 },
-	{ "HISI02A3", 0 },
-	{ "HYGO0010", ACCESS_INTR_MASK },
-	{ }
-};
-MODULE_DEVICE_TABLE(acpi, dw_i2c_acpi_match);
-#endif
-
 #ifdef CONFIG_OF
 #define BT1_I2C_CTL			0x100
 #define BT1_I2C_CTL_ADDR_MASK		GENMASK(7, 0)
@@ -152,14 +130,6 @@ static void i2c_dw_of_configure(struct dw_i2c_dev *dev)
 	if (dev_of_node(dev->dev))
 		i2c_dw_of_do_configure(dev, dev->dev);
 }
-
-static const struct of_device_id dw_i2c_of_match[] = {
-	{ .compatible = "snps,designware-i2c", },
-	{ .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
-	{ .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
-	{},
-};
-MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
 #else
 static int bt1_i2c_request_regs(struct dw_i2c_dev *dev)
 {
@@ -485,16 +455,41 @@ static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
 #define DW_I2C_DEV_PMOPS NULL
 #endif
 
-/* Work with hotplug and coldplug */
-MODULE_ALIAS("platform:i2c_designware");
+static const struct of_device_id dw_i2c_of_match[] = {
+	{ .compatible = "snps,designware-i2c", },
+	{ .compatible = "mscc,ocelot-i2c", .data = (void *)MODEL_MSCC_OCELOT },
+	{ .compatible = "baikal,bt1-sys-i2c", .data = (void *)MODEL_BAIKAL_BT1 },
+	{}
+};
+MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
+
+static const struct acpi_device_id dw_i2c_acpi_match[] = {
+	{ "80860F41", ACCESS_NO_IRQ_SUSPEND },
+	{ "808622C1", ACCESS_NO_IRQ_SUSPEND },
+	{ "AMD0010", ACCESS_INTR_MASK },
+	{ "AMDI0010", ACCESS_INTR_MASK },
+	{ "AMDI0019", ACCESS_INTR_MASK | ARBITRATION_SEMAPHORE },
+	{ "AMDI0510", 0 },
+	{ "APMC0D0F", 0 },
+	{ "HISI02A1", 0 },
+	{ "HISI02A2", 0 },
+	{ "HISI02A3", 0 },
+	{ "HYGO0010", ACCESS_INTR_MASK },
+	{ "INT33C2", 0 },
+	{ "INT33C3", 0 },
+	{ "INT3432", 0 },
+	{ "INT3433", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, dw_i2c_acpi_match);
 
 static struct platform_driver dw_i2c_driver = {
 	.probe = dw_i2c_plat_probe,
 	.remove_new = dw_i2c_plat_remove,
 	.driver		= {
 		.name	= "i2c_designware",
-		.of_match_table = of_match_ptr(dw_i2c_of_match),
-		.acpi_match_table = ACPI_PTR(dw_i2c_acpi_match),
+		.of_match_table = dw_i2c_of_match,
+		.acpi_match_table = dw_i2c_acpi_match,
 		.pm	= DW_I2C_DEV_PMOPS,
 	},
 };
@@ -511,6 +506,8 @@ static void __exit dw_i2c_exit_driver(void)
 }
 module_exit(dw_i2c_exit_driver);
 
+/* Work with hotplug and coldplug */
+MODULE_ALIAS("platform:i2c_designware");
 MODULE_AUTHOR("Baruch Siach <baruch@tkos.co.il>");
 MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter");
 MODULE_LICENSE("GPL");
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 5/9] i2c: designware: Always provide ID tables

[Jarkko Nikula]

On 7/25/23 17:30, Andy Shevchenko wrote:
> There is no need to have ugly ifdeffery and additional macros
> for the ID tables. Always provide them. Since we touch the
> ACPI table, make it sorted by ID.
> 
> While at it, group MODULE_ALIAS() with other MODULE_*() macros.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/i2c/busses/i2c-designware-platdrv.c | 65 ++++++++++-----------
>   1 file changed, 31 insertions(+), 34 deletions(-)
> 
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

(minor comment below if you plan to update)

> +/* Work with hotplug and coldplug */
> +MODULE_ALIAS("platform:i2c_designware");

Perhaps this comment can be retired, i.e. dropped.

Re: [PATCH v1 5/9] i2c: designware: Always provide ID tables

[Andy Shevchenko]

On Fri, Jul 28, 2023 at 03:33:59PM +0300, Jarkko Nikula wrote:
> On 7/25/23 17:30, Andy Shevchenko wrote:

...

> > +/* Work with hotplug and coldplug */
> > +MODULE_ALIAS("platform:i2c_designware");
> 
> Perhaps this comment can be retired, i.e. dropped.

Then it needs to be done in a separate patch, because in the other file the
comment will be left untouched.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/9] i2c: designware: Always provide ID tables

[Andi Shyti]

Hi Andy,

On Mon, Jul 31, 2023 at 11:05:05PM +0300, Andy Shevchenko wrote:
> On Fri, Jul 28, 2023 at 03:33:59PM +0300, Jarkko Nikula wrote:
> > On 7/25/23 17:30, Andy Shevchenko wrote:
> 
> ...
> 
> > > +/* Work with hotplug and coldplug */
> > > +MODULE_ALIAS("platform:i2c_designware");
> > 
> > Perhaps this comment can be retired, i.e. dropped.
> 
> Then it needs to be done in a separate patch, because in the other file the
> comment will be left untouched.

You are being a bit too religios here... if you want to stick to
this, then you need to send a patch for sorting by ID, a patch
for grouping together MODULE_*, a patch to remove this comment
and a patch to always provide the id table.

I think, "while at it", you can safely remove the redundant
comment :)

It doesn't make too much difference to me anyway:

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

Re: [PATCH v1 5/9] i2c: designware: Always provide ID tables

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 11:00:55PM +0200, Andi Shyti wrote:
> On Mon, Jul 31, 2023 at 11:05:05PM +0300, Andy Shevchenko wrote:
> > On Fri, Jul 28, 2023 at 03:33:59PM +0300, Jarkko Nikula wrote:
> > > On 7/25/23 17:30, Andy Shevchenko wrote:

...

> > > > +/* Work with hotplug and coldplug */
> > > > +MODULE_ALIAS("platform:i2c_designware");
> > > 
> > > Perhaps this comment can be retired, i.e. dropped.
> > 
> > Then it needs to be done in a separate patch, because in the other file the
> > comment will be left untouched.
> 
> You are being a bit too religios here...

No, it's being consistent. Either we remove them both or don't touch.

> if you want to stick to
> this, then you need to send a patch for sorting by ID, a patch
> for grouping together MODULE_*, a patch to remove this comment
> and a patch to always provide the id table.
> 
> I think, "while at it", you can safely remove the redundant
> comment :)
> 
> It doesn't make too much difference to me anyway:
> 
> Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Thank you!

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 6/9] i2c: designware: Consolidate firmware parsing and configure code

[Andy Shevchenko]

We have two same code flows in the PCI and plaform drivers. Moreover,
the flow requires the common code to export a few functions. Instead,
consolidate that flow under new function called
i2c_dw_fw_parse_and_configure() and drop unneeded exports.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-common.c  | 70 +++++++++++++++++++--
 drivers/i2c/busses/i2c-designware-core.h    |  9 +--
 drivers/i2c/busses/i2c-designware-pcidrv.c  | 10 +--
 drivers/i2c/busses/i2c-designware-platdrv.c | 48 +-------------
 4 files changed, 68 insertions(+), 69 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 222b530c0441..443426474cfc 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -20,6 +20,7 @@
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/swab.h>
@@ -188,7 +189,7 @@ static const u32 supported_speeds[] = {
 	I2C_MAX_STANDARD_MODE_FREQ,
 };
 
-int i2c_dw_validate_speed(struct dw_i2c_dev *dev)
+static int i2c_dw_validate_speed(struct dw_i2c_dev *dev)
 {
 	struct i2c_timings *t = &dev->timings;
 	unsigned int i;
@@ -208,7 +209,49 @@ int i2c_dw_validate_speed(struct dw_i2c_dev *dev)
 
 	return -EINVAL;
 }
-EXPORT_SYMBOL_GPL(i2c_dw_validate_speed);
+
+#ifdef CONFIG_OF
+
+#include <linux/platform_device.h>
+
+#define MSCC_ICPU_CFG_TWI_DELAY		0x0
+#define MSCC_ICPU_CFG_TWI_DELAY_ENABLE	BIT(0)
+#define MSCC_ICPU_CFG_TWI_SPIKE_FILTER	0x4
+
+static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
+{
+	writel((dev->sda_hold_time << 1) | MSCC_ICPU_CFG_TWI_DELAY_ENABLE,
+	       dev->ext + MSCC_ICPU_CFG_TWI_DELAY);
+
+	return 0;
+}
+
+static void i2c_dw_of_do_configure(struct dw_i2c_dev *dev, struct device *device)
+{
+	struct platform_device *pdev = dev_is_platform(device) ? to_platform_device(device) : NULL;
+
+	switch (dev->flags & MODEL_MASK) {
+	case MODEL_MSCC_OCELOT:
+		dev->ext = devm_platform_ioremap_resource(pdev, 1);
+		if (!IS_ERR(dev->ext))
+			dev->set_sda_hold_time = mscc_twi_set_sda_hold_time;
+		break;
+	default:
+		break;
+	}
+}
+
+static void i2c_dw_of_configure(struct dw_i2c_dev *dev)
+{
+	if (dev_of_node(dev->dev))
+		i2c_dw_of_do_configure(dev, dev->dev);
+}
+
+#else	/* CONFIG_OF */
+
+static inline void i2c_dw_of_configure(struct dw_i2c_dev *dev) { }
+
+#endif	/* CONFIG_OF */
 
 #ifdef CONFIG_ACPI
 
@@ -286,12 +329,11 @@ static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *devi
 	}
 }
 
-void i2c_dw_acpi_configure(struct dw_i2c_dev *dev)
+static void i2c_dw_acpi_configure(struct dw_i2c_dev *dev)
 {
 	if (has_acpi_companion(dev->dev))
 		i2c_dw_acpi_do_configure(dev, dev->dev);
 }
-EXPORT_SYMBOL_GPL(i2c_dw_acpi_configure);
 
 static u32 i2c_dw_acpi_round_bus_speed(struct device *device)
 {
@@ -313,11 +355,13 @@ static u32 i2c_dw_acpi_round_bus_speed(struct device *device)
 
 #else	/* CONFIG_ACPI */
 
+static inline void i2c_dw_acpi_configure(struct dw_i2c_dev *dev) { }
+
 static inline u32 i2c_dw_acpi_round_bus_speed(struct device *device) { return 0; }
 
 #endif	/* CONFIG_ACPI */
 
-void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev)
+static void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev)
 {
 	u32 acpi_speed = i2c_dw_acpi_round_bus_speed(dev->dev);
 	struct i2c_timings *t = &dev->timings;
@@ -333,7 +377,21 @@ void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev)
 	else
 		t->bus_freq_hz = I2C_MAX_FAST_MODE_FREQ;
 }
-EXPORT_SYMBOL_GPL(i2c_dw_adjust_bus_speed);
+
+int i2c_dw_fw_parse_and_configure(struct dw_i2c_dev *dev)
+{
+	struct i2c_timings *t = &dev->timings;
+
+	i2c_parse_fw_timings(dev->dev, t, false);
+
+	i2c_dw_adjust_bus_speed(dev);
+
+	i2c_dw_of_configure(dev);
+	i2c_dw_acpi_configure(dev);
+
+	return i2c_dw_validate_speed(dev);
+}
+EXPORT_SYMBOL_GPL(i2c_dw_fw_parse_and_configure);
 
 u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
 {
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index f0c683ad860f..8547590fc91b 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -392,11 +392,4 @@ int i2c_dw_baytrail_probe_lock_support(struct dw_i2c_dev *dev);
 int i2c_dw_amdpsp_probe_lock_support(struct dw_i2c_dev *dev);
 #endif
 
-int i2c_dw_validate_speed(struct dw_i2c_dev *dev);
-void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev);
-
-#if IS_ENABLED(CONFIG_ACPI)
-void i2c_dw_acpi_configure(struct dw_i2c_dev *dev);
-#else
-static inline void i2c_dw_acpi_configure(struct dw_i2c_dev *dev) { }
-#endif
+int i2c_dw_fw_parse_and_configure(struct dw_i2c_dev *dev);
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index a42a47e0032d..28a60fdb9ca2 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -252,7 +252,6 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	int r;
 	struct dw_pci_controller *controller;
 	struct dw_scl_sda_cfg *cfg;
-	struct i2c_timings *t;
 
 	if (id->driver_data >= ARRAY_SIZE(dw_pci_controllers))
 		return dev_err_probe(&pdev->dev, -EINVAL,
@@ -287,9 +286,6 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	dev->irq = pci_irq_vector(pdev, 0);
 	dev->flags |= controller->flags;
 
-	t = &dev->timings;
-	i2c_parse_fw_timings(&pdev->dev, t, false);
-
 	pci_set_drvdata(pdev, dev);
 
 	if (controller->setup) {
@@ -300,11 +296,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 		}
 	}
 
-	i2c_dw_adjust_bus_speed(dev);
-
-	i2c_dw_acpi_configure(dev);
-
-	r = i2c_dw_validate_speed(dev);
+	r = i2c_dw_fw_parse_and_configure(dev);
 	if (r) {
 		pci_free_irq_vectors(pdev);
 		return r;
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d2ffd041c0c7..c73cba7db65f 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -21,7 +21,6 @@
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
-#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
@@ -97,46 +96,11 @@ static int bt1_i2c_request_regs(struct dw_i2c_dev *dev)
 	dev->map = devm_regmap_init(dev->dev, NULL, dev, &bt1_i2c_cfg);
 	return PTR_ERR_OR_ZERO(dev->map);
 }
-
-#define MSCC_ICPU_CFG_TWI_DELAY		0x0
-#define MSCC_ICPU_CFG_TWI_DELAY_ENABLE	BIT(0)
-#define MSCC_ICPU_CFG_TWI_SPIKE_FILTER	0x4
-
-static int mscc_twi_set_sda_hold_time(struct dw_i2c_dev *dev)
-{
-	writel((dev->sda_hold_time << 1) | MSCC_ICPU_CFG_TWI_DELAY_ENABLE,
-	       dev->ext + MSCC_ICPU_CFG_TWI_DELAY);
-
-	return 0;
-}
-
-static void i2c_dw_of_do_configure(struct dw_i2c_dev *dev, struct device *device)
-{
-	struct platform_device *pdev = to_platform_device(device);
-
-	switch (dev->flags & MODEL_MASK) {
-	case MODEL_MSCC_OCELOT:
-		dev->ext = devm_platform_ioremap_resource(pdev, 1);
-		if (!IS_ERR(dev->ext))
-			dev->set_sda_hold_time = mscc_twi_set_sda_hold_time;
-		break;
-	default:
-		break;
-	}
-}
-
-static void i2c_dw_of_configure(struct dw_i2c_dev *dev)
-{
-	if (dev_of_node(dev->dev))
-		i2c_dw_of_do_configure(dev, dev->dev);
-}
 #else
 static int bt1_i2c_request_regs(struct dw_i2c_dev *dev)
 {
 	return -ENODEV;
 }
-
-static inline void i2c_dw_of_configure(struct dw_i2c_dev *dev) { }
 #endif
 
 static int txgbe_i2c_request_regs(struct dw_i2c_dev *dev)
@@ -248,7 +212,6 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 {
 	struct i2c_adapter *adap;
 	struct dw_i2c_dev *dev;
-	struct i2c_timings *t;
 	int irq, ret;
 
 	irq = platform_get_irq(pdev, 0);
@@ -277,15 +240,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 
 	reset_control_deassert(dev->rst);
 
-	t = &dev->timings;
-	i2c_parse_fw_timings(&pdev->dev, t, false);
-
-	i2c_dw_adjust_bus_speed(dev);
-
-	i2c_dw_of_configure(dev);
-	i2c_dw_acpi_configure(dev);
-
-	ret = i2c_dw_validate_speed(dev);
+	ret = i2c_dw_fw_parse_and_configure(dev);
 	if (ret)
 		goto exit_reset;
 
@@ -313,6 +268,7 @@ static int dw_i2c_plat_probe(struct platform_device *pdev)
 		goto exit_reset;
 
 	if (dev->clk) {
+		struct i2c_timings *t = &dev->timings;
 		u64 clk_khz;
 
 		dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 6/9] i2c: designware: Consolidate firmware parsing and configure code

[Andi Shyti]

Hi Andy,

On Tue, Jul 25, 2023 at 05:30:20PM +0300, Andy Shevchenko wrote:
> We have two same code flows in the PCI and plaform drivers. Moreover,
> the flow requires the common code to export a few functions. Instead,
> consolidate that flow under new function called
> i2c_dw_fw_parse_and_configure() and drop unneeded exports.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

this is just some code refactoring... didn't see anything wrong
here:

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

[PATCH v1 7/9] i2c: desingware: Unify firmware type checks

[Andy Shevchenko]

Instead of asymmetrical checks for the firmware use is_*_node()
calls. With that, drop now local wrappers against

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-common.c | 23 +++++++---------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index 443426474cfc..e6df6a484955 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -241,15 +241,9 @@ static void i2c_dw_of_do_configure(struct dw_i2c_dev *dev, struct device *device
 	}
 }
 
-static void i2c_dw_of_configure(struct dw_i2c_dev *dev)
-{
-	if (dev_of_node(dev->dev))
-		i2c_dw_of_do_configure(dev, dev->dev);
-}
-
 #else	/* CONFIG_OF */
 
-static inline void i2c_dw_of_configure(struct dw_i2c_dev *dev) { }
+static inline void i2c_dw_of_do_configure(struct dw_i2c_dev *dev, struct device *device) { }
 
 #endif	/* CONFIG_OF */
 
@@ -329,12 +323,6 @@ static void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *devi
 	}
 }
 
-static void i2c_dw_acpi_configure(struct dw_i2c_dev *dev)
-{
-	if (has_acpi_companion(dev->dev))
-		i2c_dw_acpi_do_configure(dev, dev->dev);
-}
-
 static u32 i2c_dw_acpi_round_bus_speed(struct device *device)
 {
 	u32 acpi_speed;
@@ -355,7 +343,7 @@ static u32 i2c_dw_acpi_round_bus_speed(struct device *device)
 
 #else	/* CONFIG_ACPI */
 
-static inline void i2c_dw_acpi_configure(struct dw_i2c_dev *dev) { }
+static inline void i2c_dw_acpi_do_configure(struct dw_i2c_dev *dev, struct device *device) { }
 
 static inline u32 i2c_dw_acpi_round_bus_speed(struct device *device) { return 0; }
 
@@ -380,14 +368,17 @@ static void i2c_dw_adjust_bus_speed(struct dw_i2c_dev *dev)
 
 int i2c_dw_fw_parse_and_configure(struct dw_i2c_dev *dev)
 {
+	struct fwnode_handle *fwnode = dev_fwnode(dev->dev);
 	struct i2c_timings *t = &dev->timings;
 
 	i2c_parse_fw_timings(dev->dev, t, false);
 
 	i2c_dw_adjust_bus_speed(dev);
 
-	i2c_dw_of_configure(dev);
-	i2c_dw_acpi_configure(dev);
+	if (is_of_node(fwnode))
+		i2c_dw_of_do_configure(dev, dev->dev);
+	else if (is_acpi_node(fwnode))
+		i2c_dw_acpi_do_configure(dev, dev->dev);
 
 	return i2c_dw_validate_speed(dev);
 }
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 7/9] i2c: desingware: Unify firmware type checks

[Andi Shyti]

Hi Andy,

On Tue, Jul 25, 2023 at 05:30:21PM +0300, Andy Shevchenko wrote:
> Instead of asymmetrical checks for the firmware use is_*_node()
> calls. With that, drop now local wrappers against
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-common.c | 23 +++++++---------------
>  1 file changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
> index 443426474cfc..e6df6a484955 100644
> --- a/drivers/i2c/busses/i2c-designware-common.c
> +++ b/drivers/i2c/busses/i2c-designware-common.c
> @@ -241,15 +241,9 @@ static void i2c_dw_of_do_configure(struct dw_i2c_dev *dev, struct device *device
>  	}
>  }
>  
> -static void i2c_dw_of_configure(struct dw_i2c_dev *dev)
> -{
> -	if (dev_of_node(dev->dev))
> -		i2c_dw_of_do_configure(dev, dev->dev);
> -}
> -

I have to partially agree with Jarkko here, the patch splitting
of this series is a bit too exotic. Series need to be understood
by reading them forward, not backward.

Oversplitting sometimes might even reduce readability and
"reviewability" (can I say so?). And this function, in seven
patches, has been added, moved and removed, and I had to read the
series twice :)

Anyway, I won't ask you to refactor the whole series, I
understand your logic.

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

[PATCH v1 8/9] i2c: designware: Get rid of redundant 'else'

[Andy Shevchenko]

In the snippets like the following

	if (...)
		return / goto / break / continue ...;
	else
		...

the 'else' is redundant. Get rid of it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index e6df6a484955..de28dd66c5eb 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -615,10 +615,10 @@ int i2c_dw_handle_tx_abort(struct dw_i2c_dev *dev)
 
 	if (abort_source & DW_IC_TX_ARB_LOST)
 		return -EAGAIN;
-	else if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
+	if (abort_source & DW_IC_TX_ABRT_GCALL_READ)
 		return -EINVAL; /* wrong msgs[] data */
-	else
-		return -EIO;
+
+	return -EIO;
 }
 
 int i2c_dw_set_fifo_size(struct dw_i2c_dev *dev)
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 8/9] i2c: designware: Get rid of redundant 'else'

[Andi Shyti]

On Tue, Jul 25, 2023 at 05:30:22PM +0300, Andy Shevchenko wrote:
> In the snippets like the following
> 
> 	if (...)
> 		return / goto / break / continue ...;
> 	else
> 		...
> 
> the 'else' is redundant. Get rid of it.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

[PATCH v1 9/9] i2c: designware: Fix spelling and other issues in the comments

[Andy Shevchenko]

Fix spelling and other issues, such as kernel doc reported about,
in the comments.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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-slave.c  |  6 ++++--
 5 files changed, 28 insertions(+), 21 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 de28dd66c5eb..beb190f7d005 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -123,6 +123,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)
 {
@@ -170,7 +172,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);
@@ -330,7 +332,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++) {
@@ -508,7 +510,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 8547590fc91b..8eb037298670 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -139,10 +139,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
@@ -197,7 +197,7 @@ struct reset_control;
  * @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
@@ -232,7 +232,7 @@ struct reset_control;
  * @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
  * @disable: function to disable the controller
  * @init: function to initialize the I2C hardware
  * @set_sda_hold_time: callback to retrieve IP specific SDA hold timing
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 3bfd7a2232db..e81c5b6188a6 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -165,12 +165,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)
 {
@@ -311,7 +313,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)
@@ -418,7 +420,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.
  */
@@ -456,7 +458,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.
 			 */
@@ -910,7 +913,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;
@@ -1015,7 +1018,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-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
index 2e079cf20bb5..bb44f41efdfc 100644
--- a/drivers/i2c/busses/i2c-designware-slave.c
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -30,12 +30,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)
 {
@@ -263,7 +265,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;
 	}
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 9/9] i2c: designware: Fix spelling and other issues in the comments

[Andi Shyti]

Hi Andy,

[...]

> @@ -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 */

I think "was malfunctioned" is not really correct... maybe "has
malfunctioned"? "is malfunctioning"?

>  	if (psp_i2c_mbox_fail)
>  		goto cleanup;

[...]

> @@ -508,7 +510,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

what did you change here? :)

The rest looks good.

Thanks,
Andi

>  		 * 25us) as described in the DesignWare I2C databook.
>  		 */
>  		usleep_range(25, 250);

Re: [PATCH v1 9/9] i2c: designware: Fix spelling and other issues in the comments

[Andy Shevchenko]

On Fri, Aug 04, 2023 at 11:41:57PM +0200, Andi Shyti wrote:

...

> > @@ -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 */
> 
> I think "was malfunctioned" is not really correct... maybe "has
> malfunctioned"? "is malfunctioning"?

I first stumbled over this, but than I read the function name...
I guess they are correct. So I think I need to drop this hunk.

...

> > -		 * transfer supported by the driver (for 400KHz this is
> > +		 * transfer supported by the driver (for 400kHz this is
> 
> what did you change here? :)

Proper units. k is the official SI prefix for KILO.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 9/9] i2c: designware: Fix spelling and other issues in the comments

[Andi Shyti]

On Mon, Aug 07, 2023 at 05:37:28PM +0300, Andy Shevchenko wrote:
> On Fri, Aug 04, 2023 at 11:41:57PM +0200, Andi Shyti wrote:
> 
> ...
> 
> > > @@ -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 */
> > 
> > I think "was malfunctioned" is not really correct... maybe "has
> > malfunctioned"? "is malfunctioning"?
> 
> I first stumbled over this, but than I read the function name...
> I guess they are correct. So I think I need to drop this hunk.

"malfunctional" actually doesn't exist as a word. I think this is
correct:

  "Return early if the mailbox is malfunctioning"

> ...
> 
> > > -		 * transfer supported by the driver (for 400KHz this is
> > > +		 * transfer supported by the driver (for 400kHz this is
> > 
> > what did you change here? :)
> 
> Proper units. k is the official SI prefix for KILO.

oh right! didn't see it :)

Thanks,
Andi

Re: [PATCH v1 0/9] i2c: designware: code consolidation & cleanups

[Limonciello, Mario]

On 7/25/2023 09:30, Andy Shevchenko wrote:
> Mainly this is about firmware parsing and configuring code
> consolidation. Besides that some cleanups here and there.
> 
> Andy Shevchenko (9):
>    i2c: designware: Move has_acpi_companion() to common code
>    i2c: designware: Change i2c_dw_acpi_configure() prototype
>    i2c: designware: Align dw_i2c_of_configure() with
>      i2c_dw_acpi_configure()
>    i2c: designware: Propagate firmware node
>    i2c: designware: Always provide ID tables
>    i2c: designware: Consolidate firmware parsing and configure code
>    i2c: desingware: Unify firmware type checks
>    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  |  84 +++++++++++---
>   drivers/i2c/busses/i2c-designware-core.h    |  25 ++---
>   drivers/i2c/busses/i2c-designware-master.c  |  15 ++-
>   drivers/i2c/busses/i2c-designware-pcidrv.c  |  13 +--
>   drivers/i2c/busses/i2c-designware-platdrv.c | 117 ++++++--------------
>   drivers/i2c/busses/i2c-designware-slave.c   |   6 +-
>   7 files changed, 131 insertions(+), 139 deletions(-)
> 

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>