| 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 |
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
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 :) >
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 :)
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.
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?
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).
> > 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 --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)
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(-)