| Message ID | 20200813175729.15088-3-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | None | expand |
On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > Some already present users may utilize resource_union() helper. > Provide it for them and for wider use in the future. > > Deliberately avoid min()/max() macro as they are still parts of > kernel.h which is quite a burden to be included here in order > to avoid circular dependencies. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: linux-pci@vger.kernel.org > --- > include/linux/ioport.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > index 0193987b9968..c98df0ec7422 100644 > --- a/include/linux/ioport.h > +++ b/include/linux/ioport.h > @@ -232,6 +232,16 @@ static inline bool resource_overlaps(struct resource *r1, struct resource *r2) > return (r1->start <= r2->end && r1->end >= r2->start); > } > > +static inline bool > +resource_union(struct resource *r1, struct resource *r2, struct resource *r) > +{ > + if (!resource_overlaps(r1, r2)) > + return false; I tend to add empty lines after return statements like this to make them more clearly visible. > + r->start = r2->start < r1->start ? r2->start : r1->start; > + r->end = r2->end > r1->end ? r2->end : r1->end; Well, what about using min() and max() here? > + return true; > +} > + > /* Convenience shorthand with allocation */ > #define request_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), 0) > #define request_muxed_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED) > -- > 2.28.0 >
On Fri, Aug 14, 2020 at 05:23:07PM +0200, Rafael J. Wysocki wrote: > On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > Some already present users may utilize resource_union() helper. > > Provide it for them and for wider use in the future. > > > > Deliberately avoid min()/max() macro as they are still parts of > > kernel.h which is quite a burden to be included here in order > > to avoid circular dependencies. ... > > + if (!resource_overlaps(r1, r2)) > > + return false; > > I tend to add empty lines after return statements like this to make > them more clearly visible. Okay! > > + r->start = r2->start < r1->start ? r2->start : r1->start; > > + r->end = r2->end > r1->end ? r2->end : r1->end; > > Well, what about using min() and max() here? I devoted one paragraph in the commit message to answer this. The kernel.h (which I'm planning to split at some point) is a monster which brings more pain than solves here. Note, this is a header file and it's quite clean from dependencies perspective.
On Fri, Aug 14, 2020 at 5:37 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Aug 14, 2020 at 05:23:07PM +0200, Rafael J. Wysocki wrote: > > On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > > Some already present users may utilize resource_union() helper. > > > Provide it for them and for wider use in the future. > > > > > > Deliberately avoid min()/max() macro as they are still parts of > > > kernel.h which is quite a burden to be included here in order > > > to avoid circular dependencies. > > ... > > > > + if (!resource_overlaps(r1, r2)) > > > + return false; > > > > I tend to add empty lines after return statements like this to make > > them more clearly visible. > > Okay! > > > > + r->start = r2->start < r1->start ? r2->start : r1->start; > > > + r->end = r2->end > r1->end ? r2->end : r1->end; > > > > Well, what about using min() and max() here? > > I devoted one paragraph in the commit message to answer this. The kernel.h > (which I'm planning to split at some point) is a monster which brings more pain > than solves here. Note, this is a header file and it's quite clean from > dependencies perspective. But this is code duplication (even if really small) and it is not entirely clean too. Maybe move the definitions of min() and max() to a separate header file?
On Fri, Aug 14, 2020 at 06:09:53PM +0200, Rafael J. Wysocki wrote: > On Fri, Aug 14, 2020 at 5:37 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Aug 14, 2020 at 05:23:07PM +0200, Rafael J. Wysocki wrote: > > > On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: ... > > > Well, what about using min() and max() here? > > > > I devoted one paragraph in the commit message to answer this. The kernel.h > > (which I'm planning to split at some point) is a monster which brings more pain > > than solves here. Note, this is a header file and it's quite clean from > > dependencies perspective. > > But this is code duplication (even if really small) and it is not > entirely clean too. > > Maybe move the definitions of min() and max() to a separate header file? That is the plan in the kernel.h splitting project. But do you want me to do it here? I can try to bring that patch into this series.
On Fri, Aug 14, 2020 at 6:23 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Aug 14, 2020 at 06:09:53PM +0200, Rafael J. Wysocki wrote: > > On Fri, Aug 14, 2020 at 5:37 PM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Fri, Aug 14, 2020 at 05:23:07PM +0200, Rafael J. Wysocki wrote: > > > > On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > ... > > > > > Well, what about using min() and max() here? > > > > > > I devoted one paragraph in the commit message to answer this. The kernel.h > > > (which I'm planning to split at some point) is a monster which brings more pain > > > than solves here. Note, this is a header file and it's quite clean from > > > dependencies perspective. > > > > But this is code duplication (even if really small) and it is not > > entirely clean too. > > > > Maybe move the definitions of min() and max() to a separate header file? > > That is the plan in the kernel.h splitting project. But do you want me to do it > here? I can try to bring that patch into this series. Well, ostensibly the purpose of this series is to reduce code duplication, but if it adds code duplication, that kind of defeats the purpose IMO.
On Fri, Aug 14, 2020 at 07:17:18PM +0200, Rafael J. Wysocki wrote: > On Fri, Aug 14, 2020 at 6:23 PM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Aug 14, 2020 at 06:09:53PM +0200, Rafael J. Wysocki wrote: > > > On Fri, Aug 14, 2020 at 5:37 PM Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Fri, Aug 14, 2020 at 05:23:07PM +0200, Rafael J. Wysocki wrote: > > > > > On Thu, Aug 13, 2020 at 7:57 PM Andy Shevchenko > > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > ... > > > > > > > Well, what about using min() and max() here? > > > > > > > > I devoted one paragraph in the commit message to answer this. The kernel.h > > > > (which I'm planning to split at some point) is a monster which brings more pain > > > > than solves here. Note, this is a header file and it's quite clean from > > > > dependencies perspective. > > > > > > But this is code duplication (even if really small) and it is not > > > entirely clean too. > > > > > > Maybe move the definitions of min() and max() to a separate header file? > > > > That is the plan in the kernel.h splitting project. But do you want me to do it > > here? I can try to bring that patch into this series. > > Well, ostensibly the purpose of this series is to reduce code > duplication, but if it adds code duplication, that kind of defeats the > purpose IMO. Okay, I will append minmax.h split in v2.
diff --git a/include/linux/ioport.h b/include/linux/ioport.h index 0193987b9968..c98df0ec7422 100644 --- a/include/linux/ioport.h +++ b/include/linux/ioport.h @@ -232,6 +232,16 @@ static inline bool resource_overlaps(struct resource *r1, struct resource *r2) return (r1->start <= r2->end && r1->end >= r2->start); } +static inline bool +resource_union(struct resource *r1, struct resource *r2, struct resource *r) +{ + if (!resource_overlaps(r1, r2)) + return false; + r->start = r2->start < r1->start ? r2->start : r1->start; + r->end = r2->end > r1->end ? r2->end : r1->end; + return true; +} + /* Convenience shorthand with allocation */ #define request_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), 0) #define request_muxed_region(start,n,name) __request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
Some already present users may utilize resource_union() helper. Provide it for them and for wider use in the future. Deliberately avoid min()/max() macro as they are still parts of kernel.h which is quite a burden to be included here in order to avoid circular dependencies. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: linux-pci@vger.kernel.org --- include/linux/ioport.h | 10 ++++++++++ 1 file changed, 10 insertions(+)