| Message ID | 20230725143023.86325-1-andriy.shevchenko@linux.intel.com |
|---|---|
| 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 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?
> > 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!
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(-)