| Message ID | 20241210-fix-ipu-v3-0-00e409c84a6c@chromium.org |
|---|---|
| Headers | show |
| Series | ipu6: get rid of all the IS_ENABLED(CONFIG_ACPI) | expand |
Hi Ricardo, On Tue, Dec 10, 2024 at 07:55:59PM +0000, Ricardo Ribalda wrote: > Provide an implementation of for_each_acpi_dev_match that can be used > when CONFIG_ACPI is not set. > > The condition `false && hid && uid && hrv` is used to avoid "variable > not used" warnings. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > include/acpi/acpi_bus.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > index b2e377b7f337..eaafca41cf02 100644 > --- a/include/acpi/acpi_bus.h > +++ b/include/acpi/acpi_bus.h > @@ -1003,6 +1003,9 @@ static inline int unregister_acpi_bus_type(void *bus) { return 0; } > > static inline int acpi_wait_for_acpi_ipmi(void) { return 0; } > > +#define for_each_acpi_dev_match(adev, hid, uid, hrv) \ > + for (adev = NULL; false && (hid) && (uid) && (hrv);) There should be a space after the semicolon. I guess this might depend on who you ask. Either way, Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > + > #endif /* CONFIG_ACPI */ > > #endif /*__ACPI_BUS_H__*/ >
On Tue, 10 Dec 2024 at 21:56, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Hi Ricardo, > > On Tue, Dec 10, 2024 at 07:56:01PM +0000, Ricardo Ribalda wrote: > > Provide an implementation of acpi_device_handle that can be used when > > CONFIG_ACPI is not set. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > include/linux/acpi.h | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/include/linux/acpi.h b/include/linux/acpi.h > > index 05f39fbfa485..59a5d110ff54 100644 > > --- a/include/linux/acpi.h > > +++ b/include/linux/acpi.h > > @@ -787,6 +787,12 @@ const char *acpi_get_subsystem_id(acpi_handle handle); > > #define acpi_dev_hid_uid_match(adev, hid2, uid2) (adev && false) > > > > struct fwnode_handle; > > +struct acpi_device; > > + > > +static inline acpi_handle acpi_device_handle(struct acpi_device *adev) > > +{ > > + return NULL; > > +} > > > > static inline bool acpi_dev_found(const char *hid) > > { > > > > Please remove the extra forward declaration of struct acpi_device a few > lines below this. Instead I have moved the function under the forward declaration. Let me know if you disagree. > > With that, > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > -- > Regards, > > Sakari Ailus
Hi Ricardo, On Tue, Dec 10, 2024 at 11:35:35PM +0100, Ricardo Ribalda wrote: > On Tue, 10 Dec 2024 at 22:01, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > > > Hi Ricardo, > > > > On Tue, Dec 10, 2024 at 07:56:03PM +0000, Ricardo Ribalda wrote: > > > Provide an implementation of acpi_device_hid that can be used when > > > CONFIG_ACPI is not set. > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > include/acpi/acpi_bus.h | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > > > index 4f1b3a6f107b..c25914a152ee 100644 > > > --- a/include/acpi/acpi_bus.h > > > +++ b/include/acpi/acpi_bus.h > > > @@ -1003,6 +1003,11 @@ static inline int unregister_acpi_bus_type(void *bus) { return 0; } > > > > > > static inline int acpi_wait_for_acpi_ipmi(void) { return 0; } > > > > > > +static inline const char *acpi_device_hid(struct acpi_device *device) > > > +{ > > > + return ""; > > > +} > > > > I wonder if any caller might expect something of a string if provided? > > Valid _HIDs are either 7 or 8 characters whereas the proper version of the > > function returns "device" when one cannot be found (dummy_hid in > > drivers/acpi/scan.c). Unlikely to be a problem perhaps. > > Good point. I changed it to return "device" When ACPI is disabled, it's unlikely that string would be used anyway, vs. the case when ACPI is enabled but there's no _HID. So I think an empty string should be fine. I wonder what others think.
Em Wed, 11 Dec 2024 08:48:51 +0000 Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > Hi Mauro, > > On Wed, Dec 11, 2024 at 09:40:37AM +0100, Mauro Carvalho Chehab wrote: > > Em Wed, 11 Dec 2024 07:57:06 +0000 > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > > > > > Hi Ricardo, > > > > > > On Tue, Dec 10, 2024 at 11:35:35PM +0100, Ricardo Ribalda wrote: > > > > On Tue, 10 Dec 2024 at 22:01, Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > > > > > > > > > Hi Ricardo, > > > > > > > > > > On Tue, Dec 10, 2024 at 07:56:03PM +0000, Ricardo Ribalda wrote: > > > > > > Provide an implementation of acpi_device_hid that can be used when > > > > > > CONFIG_ACPI is not set. > > > > > > > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > > > --- > > > > > > include/acpi/acpi_bus.h | 5 +++++ > > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > > > diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h > > > > > > index 4f1b3a6f107b..c25914a152ee 100644 > > > > > > --- a/include/acpi/acpi_bus.h > > > > > > +++ b/include/acpi/acpi_bus.h > > > > > > @@ -1003,6 +1003,11 @@ static inline int unregister_acpi_bus_type(void *bus) { return 0; } > > > > > > > > > > > > static inline int acpi_wait_for_acpi_ipmi(void) { return 0; } > > > > > > > > > > > > +static inline const char *acpi_device_hid(struct acpi_device *device) > > > > > > +{ > > > > > > + return ""; > > > > > > +} > > > > > > > > > > I wonder if any caller might expect something of a string if provided? > > > > > Valid _HIDs are either 7 or 8 characters whereas the proper version of the > > > > > function returns "device" when one cannot be found (dummy_hid in > > > > > drivers/acpi/scan.c). Unlikely to be a problem perhaps. > > > > > > > > Good point. I changed it to return "device" > > > > > > When ACPI is disabled, it's unlikely that string would be used anyway, vs. > > > the case when ACPI is enabled but there's no _HID. So I think an empty > > > string should be fine. I wonder what others think. > > > > > Returning "" also caused me some attention at the original patch. IMO, > > placing a pseudo-valid HID would be better, but I guess "device" is also > > invalid, as, at least I always saw HIDs in uppercase. Also, I guess it > > is always a vendor ID + a 4 digit number. > > > > so, IMHO, something like "DEVC9999" would be a better name if we fill it. > > How about post a patch changing "device" in drivers/acpi/scan.c? :-) Yeah, keeping it coherent makes sense, but see: static const char *dummy_hid = "device"; This is compiled for production kernels, and not just for COMPILE_TEST, while: static inline const char *acpi_device_hid(struct acpi_device *device) { return "foo"; } is only COMPILE_TEST. They don't need to be aligned. > But I > think the string also needs to be an invalid as a _HID object so it's not > masking an actual hardware ID used by a real device. It doesn't matter if if ever conflicts to a real device, as this is for COMPILE_TEST only. Anyway, from my side, I'm just giving my 2 cents. I'm ok either way: "", "device", "DEVC999", ... Thanks, Mauro
We want to be able to compile_test the ipu6 driver in situations with !ACPI. In order to do this we had to add some conditional #ifs, which lead to false positives on the static analysers. Let's implement some helpers when !ACPI in the acpi headers to make the code more easier to maintain. We can land the first patch of this series ASAP to fix the current smatch warning. To: Mauro Carvalho Chehab <mchehab@kernel.org> To: Rafael J. Wysocki <rafael@kernel.org> To: Len Brown <lenb@kernel.org> To: Robert Moore <robert.moore@intel.com> To: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Dan Carpenter <dan.carpenter@linaro.org> Cc: linux-media@vger.kernel.org Cc: linux-kernel@vger.kernel.org Cc: linux-acpi@vger.kernel.org Cc: acpica-devel@lists.linux.dev Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Changes in v3: - Prefer static inlines to macros (Thanks Rafael). - Link to v2: https://lore.kernel.org/r/20241122-fix-ipu-v2-0-bba65856e9ff@chromium.org Changes in v2: - Add helpers in acpi to avoid conditional compilation - Link to v1: https://lore.kernel.org/r/20241122-fix-ipu-v1-1-246e254cb77c@chromium.org --- Ricardo Ribalda (7): media: ipu-bridge: Fix warning when !ACPI ACPI: bus: implement for_each_acpi_dev_match when !ACPI ACPI: bus: implement acpi_get_physical_device_location when !ACPI ACPI: header: implement acpi_device_handle when !ACPI ACPI: bus: implement for_each_acpi_consumer_dev when !ACPI ACPI: bus: implement acpi_device_hid when !ACPI media: ipu-bridge: Remove unneeded conditional compilations drivers/media/pci/intel/ipu-bridge.c | 28 +++++----------------------- include/acpi/acpi_bus.h | 23 ++++++++++++++++++++--- include/linux/acpi.h | 6 ++++++ 3 files changed, 31 insertions(+), 26 deletions(-) --- base-commit: 6c10d1adae82e1c8da16e7ebd2320e69f20b9d6f change-id: 20241122-fix-ipu-a2fe28908964 Best regards,