diff mbox series

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

Message ID 20230725143023.86325-2-andriy.shevchenko@linux.intel.com
State Changes Requested
Delegated to: Andi Shyti
Headers show
Series i2c: designware: code consolidation & cleanups | expand

Commit Message

'Andy Shevchenko' July 25, 2023, 2:30 p.m. UTC
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(-)

Comments

Andi Shyti July 25, 2023, 9:45 p.m. UTC | #1
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
Jarkko Nikula July 28, 2023, 11:33 a.m. UTC | #2
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 :)
>
'Andy Shevchenko' July 31, 2023, 8:14 p.m. UTC | #3
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 :)
Jarkko Nikula Aug. 3, 2023, 1:43 p.m. UTC | #4
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.
Wolfram Sang Sept. 24, 2023, 8:56 p.m. UTC | #5
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?
'Andy Shevchenko' Sept. 25, 2023, 6:46 a.m. UTC | #6
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).
Wolfram Sang Sept. 25, 2023, 6:55 a.m. UTC | #7
> > 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!
diff mbox series

Patch

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)