[PATCH v3 0/7] ipu6: get rid of all the IS_ENABLED(CONFIG_ACPI)

[PATCH v3 0/7] ipu6: get rid of all the IS_ENABLED(CONFIG_ACPI)

[Ricardo Ribalda]

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,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v3 1/7] media: ipu-bridge: Fix warning when !ACPI

[Ricardo Ribalda]

One of the quirks that we introduced to build with !ACPI && COMPILE_TEST
throws the following smatch warning:
drivers/media/pci/intel/ipu-bridge.c:752 ipu_bridge_ivsc_is_ready() warn: iterator 'i' not incremented

Fix it by replacing the condition.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/pci/intel/ipu-bridge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index a0e9a71580b5..be82bc3e27d0 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -774,7 +774,7 @@ static int ipu_bridge_ivsc_is_ready(void)
 
 		for_each_acpi_dev_match(sensor_adev, cfg->hid, NULL, -1) {
 #else
-		while (true) {
+		while (false) {
 			sensor_adev = NULL;
 #endif
 			if (!ACPI_PTR(sensor_adev->status.enabled))

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 1/7] media: ipu-bridge: Fix warning when !ACPI

[Sakari Ailus]

Hi Ricardo,

On Tue, Dec 10, 2024 at 07:55:58PM +0000, Ricardo Ribalda wrote:
> One of the quirks that we introduced to build with !ACPI && COMPILE_TEST
> throws the following smatch warning:
> drivers/media/pci/intel/ipu-bridge.c:752 ipu_bridge_ivsc_is_ready() warn: iterator 'i' not incremented
> 
> Fix it by replacing the condition.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

I've picked this to my tree and I'll take the last one, too, once the rest
reaches the media tree.

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 1/7] media: ipu-bridge: Fix warning when !ACPI

[Ricardo Ribalda]

On Tue, 10 Dec 2024 at 22:04, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Ricardo,
>
> On Tue, Dec 10, 2024 at 07:55:58PM +0000, Ricardo Ribalda wrote:
> > One of the quirks that we introduced to build with !ACPI && COMPILE_TEST
> > throws the following smatch warning:
> > drivers/media/pci/intel/ipu-bridge.c:752 ipu_bridge_ivsc_is_ready() warn: iterator 'i' not incremented
> >
> > Fix it by replacing the condition.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>
> I've picked this to my tree and I'll take the last one, too, once the rest
> reaches the media tree.

Thanks!

If you do not mind, I will keep sending 1/7 when I send v3, to make
sure it is tested by the CI. I will mark it as duplicate in patchwork.

Thanks!

>
> --
> Regards,
>
> Sakari Ailus



-- 
Ricardo Ribalda

Re: [PATCH v3 1/7] media: ipu-bridge: Fix warning when !ACPI

[Mauro Carvalho Chehab]

Em Tue, 10 Dec 2024 22:27:32 +0100
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> On Tue, 10 Dec 2024 at 22:04, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Ricardo,
> >
> > On Tue, Dec 10, 2024 at 07:55:58PM +0000, Ricardo Ribalda wrote:  
> > > One of the quirks that we introduced to build with !ACPI && COMPILE_TEST
> > > throws the following smatch warning:
> > > drivers/media/pci/intel/ipu-bridge.c:752 ipu_bridge_ivsc_is_ready() warn: iterator 'i' not incremented
> > >
> > > Fix it by replacing the condition.
> > >
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>  
> >
> > I've picked this to my tree and I'll take the last one, too, once the rest
> > reaches the media tree.  
> 
> Thanks!
> 
> If you do not mind, I will keep sending 1/7 when I send v3, to make
> sure it is tested by the CI. I will mark it as duplicate in patchwork.

Patches should not be designed to make CI happy, but to ensure that we
have a nice history at Kernel's log. Patch 1/7 shall be merged with
7/7, as you're just artificially breaking it into without a good
reason, making CI happy, but reviewers and maintainers unhappy :-)


Thanks,
Mauro

Re: [PATCH v3 1/7] media: ipu-bridge: Fix warning when !ACPI

[Mauro Carvalho Chehab]

Em Tue, 10 Dec 2024 21:03:56 +0000
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Hi Ricardo,
> 
> On Tue, Dec 10, 2024 at 07:55:58PM +0000, Ricardo Ribalda wrote:
> > One of the quirks that we introduced to build with !ACPI && COMPILE_TEST
> > throws the following smatch warning:
> > drivers/media/pci/intel/ipu-bridge.c:752 ipu_bridge_ivsc_is_ready() warn: iterator 'i' not incremented
> > 
> > Fix it by replacing the condition.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>  
> 
> I've picked this to my tree and I'll take the last one, too, once the rest
> reaches the media tree.

I prefer not merging it via media tree, except if we get an explicit ack
to do so from ACPI maintainers, as most of the stuff here are at ACPI
 headers.

Thanks,
Mauro

Re: [PATCH v3 1/7] media: ipu-bridge: Fix warning when !ACPI

[Mauro Carvalho Chehab]

Em Tue, 10 Dec 2024 19:55:58 +0000
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> One of the quirks that we introduced to build with !ACPI && COMPILE_TEST
> throws the following smatch warning:
> drivers/media/pci/intel/ipu-bridge.c:752 ipu_bridge_ivsc_is_ready() warn: iterator 'i' not incremented
> 
> Fix it by replacing the condition.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/pci/intel/ipu-bridge.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index a0e9a71580b5..be82bc3e27d0 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -774,7 +774,7 @@ static int ipu_bridge_ivsc_is_ready(void)
>  
>  		for_each_acpi_dev_match(sensor_adev, cfg->hid, NULL, -1) {
>  #else
> -		while (true) {
> +		while (false) {
>  			sensor_adev = NULL;
>  #endif

The better would be to just remove all #if and handle ACPI compatibility
with COMPILE_TEST inside acpi headers.

Besides that, t sounds that patch 2 makes this hack unneeded, as you added
a false check at the for macro:

	#define for_each_acpi_dev_match(adev, hid, uid, hrv)			\
	for (adev = NULL; false && (hid) && (uid) && (hrv);)

Please place only one set of subsystem maintainers at the To: line,
directing to the one(s) you expect to merge this series.

In this particular case, the one to be added should be the ACPI
maintainers.

Regards,
Mauro

Re: [PATCH v3 1/7] media: ipu-bridge: Fix warning when !ACPI

[Ricardo Ribalda]

Hi Mauro

On Wed, 11 Dec 2024 at 09:15, Mauro Carvalho Chehab
<maurochehab@gmail.com> wrote:
>
> Em Tue, 10 Dec 2024 19:55:58 +0000
> Ricardo Ribalda <ribalda@chromium.org> escreveu:
>
> > One of the quirks that we introduced to build with !ACPI && COMPILE_TEST
> > throws the following smatch warning:
> > drivers/media/pci/intel/ipu-bridge.c:752 ipu_bridge_ivsc_is_ready() warn: iterator 'i' not incremented
> >
> > Fix it by replacing the condition.
> >
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/r/202411221147.N6w23gDo-lkp@intel.com/
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/pci/intel/ipu-bridge.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> > index a0e9a71580b5..be82bc3e27d0 100644
> > --- a/drivers/media/pci/intel/ipu-bridge.c
> > +++ b/drivers/media/pci/intel/ipu-bridge.c
> > @@ -774,7 +774,7 @@ static int ipu_bridge_ivsc_is_ready(void)
> >
> >               for_each_acpi_dev_match(sensor_adev, cfg->hid, NULL, -1) {
> >  #else
> > -             while (true) {
> > +             while (false) {
> >                       sensor_adev = NULL;
> >  #endif
>
> The better would be to just remove all #if and handle ACPI compatibility
> with COMPILE_TEST inside acpi headers.
>
> Besides that, t sounds that patch 2 makes this hack unneeded, as you added
> a false check at the for macro:
>
>         #define for_each_acpi_dev_match(adev, hid, uid, hrv)                    \
>         for (adev = NULL; false && (hid) && (uid) && (hrv);)
>
> Please place only one set of subsystem maintainers at the To: line,
> directing to the one(s) you expect to merge this series.
>
> In this particular case, the one to be added should be the ACPI
> maintainers.

The plan was to land 1/7 via the media tree with a PR from Sakari soonish.

I believe he has already picked it on his tree. I will remove you from
Cc in the next version

thanks :)

>
> Regards,
> Mauro



-- 
Ricardo Ribalda

[PATCH v3 2/7] ACPI: bus: implement for_each_acpi_dev_match when !ACPI

[Ricardo Ribalda]

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);)
+
 #endif				/* CONFIG_ACPI */
 
 #endif /*__ACPI_BUS_H__*/

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 2/7] ACPI: bus: implement for_each_acpi_dev_match when !ACPI

[Sakari Ailus]

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__*/
> 

-- 
Regards,

Sakari Ailus

[PATCH v3 3/7] ACPI: bus: implement acpi_get_physical_device_location when !ACPI

[Ricardo Ribalda]

Provide an implementation of acpi_get_physical_device_location that can
be used when CONFIG_ACPI is not set.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

dasadsd

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 include/acpi/acpi_bus.h | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index eaafca41cf02..520f12155e7f 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -43,9 +43,6 @@ acpi_status
 acpi_evaluate_ost(acpi_handle handle, u32 source_event, u32 status_code,
 		  struct acpi_buffer *status_buf);
 
-acpi_status
-acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld);
-
 bool acpi_has_method(acpi_handle handle, char *name);
 acpi_status acpi_execute_simple_method(acpi_handle handle, char *method,
 				       u64 arg);
@@ -60,6 +57,9 @@ bool acpi_check_dsm(acpi_handle handle, const guid_t *guid, u64 rev, u64 funcs);
 union acpi_object *acpi_evaluate_dsm(acpi_handle handle, const guid_t *guid,
 			u64 rev, u64 func, union acpi_object *argv4);
 #ifdef CONFIG_ACPI
+acpi_status
+acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld);
+
 static inline union acpi_object *
 acpi_evaluate_dsm_typed(acpi_handle handle, const guid_t *guid, u64 rev,
 			u64 func, union acpi_object *argv4,
@@ -1003,6 +1003,12 @@ static inline int unregister_acpi_bus_type(void *bus) { return 0; }
 
 static inline int acpi_wait_for_acpi_ipmi(void) { return 0; }
 
+static inline acpi_status
+acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld)
+{
+	return AE_ERROR;
+}
+
 #define for_each_acpi_dev_match(adev, hid, uid, hrv)			\
 	for (adev = NULL; false && (hid) && (uid) && (hrv);)
 

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 3/7] ACPI: bus: implement acpi_get_physical_device_location when !ACPI

[Sakari Ailus]

Hi Ricardo,

On Tue, Dec 10, 2024 at 07:56:00PM +0000, Ricardo Ribalda wrote:
> Provide an implementation of acpi_get_physical_device_location that can
> be used when CONFIG_ACPI is not set.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> 
> dasadsd

Yes?

Apart from this,

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 3/7] ACPI: bus: implement acpi_get_physical_device_location when !ACPI

[Ricardo Ribalda]

On Tue, 10 Dec 2024 at 21:53, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Ricardo,
>
> On Tue, Dec 10, 2024 at 07:56:00PM +0000, Ricardo Ribalda wrote:
> > Provide an implementation of acpi_get_physical_device_location that can
> > be used when CONFIG_ACPI is not set.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> >
> > dasadsd
>
> Yes?
Leftovers from a squash :)

I will fix it on the next version if needed.
>
> Apart from this,
>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
>
> --
> Regards,
>
> Sakari Ailus



-- 
Ricardo Ribalda

Re: [PATCH v3 3/7] ACPI: bus: implement acpi_get_physical_device_location when !ACPI

[Mauro Carvalho Chehab]

Em Tue, 10 Dec 2024 19:56:00 +0000
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> Provide an implementation of acpi_get_physical_device_location that can
> be used when CONFIG_ACPI is not set.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> 


> dasadsd
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Please remove duplicated SOB.

What "dasadsd" mean?


> ---
>  include/acpi/acpi_bus.h | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index eaafca41cf02..520f12155e7f 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -43,9 +43,6 @@ acpi_status
>  acpi_evaluate_ost(acpi_handle handle, u32 source_event, u32 status_code,
>  		  struct acpi_buffer *status_buf);
>  
> -acpi_status
> -acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld);
> -
>  bool acpi_has_method(acpi_handle handle, char *name);
>  acpi_status acpi_execute_simple_method(acpi_handle handle, char *method,
>  				       u64 arg);
> @@ -60,6 +57,9 @@ bool acpi_check_dsm(acpi_handle handle, const guid_t *guid, u64 rev, u64 funcs);
>  union acpi_object *acpi_evaluate_dsm(acpi_handle handle, const guid_t *guid,
>  			u64 rev, u64 func, union acpi_object *argv4);
>  #ifdef CONFIG_ACPI
> +acpi_status
> +acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld);
> +
>  static inline union acpi_object *
>  acpi_evaluate_dsm_typed(acpi_handle handle, const guid_t *guid, u64 rev,
>  			u64 func, union acpi_object *argv4,
> @@ -1003,6 +1003,12 @@ static inline int unregister_acpi_bus_type(void *bus) { return 0; }
>  
>  static inline int acpi_wait_for_acpi_ipmi(void) { return 0; }
>  
> +static inline acpi_status
> +acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld)
> +{
> +	return AE_ERROR;
> +}
> +
>  #define for_each_acpi_dev_match(adev, hid, uid, hrv)			\
>  	for (adev = NULL; false && (hid) && (uid) && (hrv);)
>  
> 



Thanks,
Mauro

[PATCH v3 4/7] ACPI: header: implement acpi_device_handle when !ACPI

[Ricardo Ribalda]

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)
 {

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 4/7] ACPI: header: implement acpi_device_handle when !ACPI

[Sakari Ailus]

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.

With that,

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 4/7] ACPI: header: implement acpi_device_handle when !ACPI

[Ricardo Ribalda]

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



-- 
Ricardo Ribalda

Re: [PATCH v3 4/7] ACPI: header: implement acpi_device_handle when !ACPI

[Sakari Ailus]

On Tue, Dec 10, 2024 at 11:31:57PM +0100, Ricardo Ribalda wrote:
> 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.

The same order in which the functions are found in the actual
implementation would be my suggestion. Rafael could also have an opinion.

-- 
Sakari Ailus

Re: [PATCH v3 4/7] ACPI: header: implement acpi_device_handle when !ACPI

[Rafael J. Wysocki]

On Wed, Dec 11, 2024 at 8:59 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> On Tue, Dec 10, 2024 at 11:31:57PM +0100, Ricardo Ribalda wrote:
> > 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.
>
> The same order in which the functions are found in the actual
> implementation would be my suggestion. Rafael could also have an opinion.

It is nice, but it is not a requirement.

[PATCH v3 5/7] ACPI: bus: implement for_each_acpi_consumer_dev when !ACPI

[Ricardo Ribalda]

Provide an implementation of for_each_acpi_consumer_dev that can be use
used when CONFIG_ACPI is not set.

The expression `false && supplier` 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 520f12155e7f..4f1b3a6f107b 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -1009,6 +1009,9 @@ acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld
 	return AE_ERROR;
 }
 
+#define for_each_acpi_consumer_dev(supplier, consumer)			\
+	for (consumer = NULL; false && (supplier);)
+
 #define for_each_acpi_dev_match(adev, hid, uid, hrv)			\
 	for (adev = NULL; false && (hid) && (uid) && (hrv);)
 

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 5/7] ACPI: bus: implement for_each_acpi_consumer_dev when !ACPI

[Sakari Ailus]

On Tue, Dec 10, 2024 at 07:56:02PM +0000, Ricardo Ribalda wrote:
> Provide an implementation of for_each_acpi_consumer_dev that can be use
> used when CONFIG_ACPI is not set.
> 
> The expression `false && supplier` is used to avoid "variable not used"
> warnings.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Sakari Ailus

[PATCH v3 6/7] ACPI: bus: implement acpi_device_hid when !ACPI

[Ricardo Ribalda]

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 "";
+}
+
 static inline acpi_status
 acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld)
 {

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 6/7] ACPI: bus: implement acpi_device_hid when !ACPI

[Sakari Ailus]

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.

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> +
>  static inline acpi_status
>  acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld)
>  {
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 6/7] ACPI: bus: implement acpi_device_hid when !ACPI

[Ricardo Ribalda]

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"

Thanks!

>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>
> > +
> >  static inline acpi_status
> >  acpi_get_physical_device_location(acpi_handle handle, struct acpi_pld_info **pld)
> >  {
> >
>
> --
> Regards,
>
> Sakari Ailus



-- 
Ricardo Ribalda

Re: [PATCH v3 6/7] ACPI: bus: implement acpi_device_hid when !ACPI

[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.

-- 
Sakari Ailus

Re: [PATCH v3 6/7] ACPI: bus: implement acpi_device_hid when !ACPI

[Mauro Carvalho Chehab]

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.

Thanks,
Mauro

Re: [PATCH v3 6/7] ACPI: bus: implement acpi_device_hid when !ACPI

[Sakari Ailus]

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

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 6/7] ACPI: bus: implement acpi_device_hid when !ACPI

[Mauro Carvalho Chehab]

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

[PATCH v3 7/7] media: ipu-bridge: Remove unneeded conditional compilations

[Ricardo Ribalda]

The ACPI headers have introduced implementations for some of their
functions when the kernel is not configured with ACPI.

Let's use them instead of our conditional compilation. It is easier to
maintain and less prone to errors.

Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/pci/intel/ipu-bridge.c | 28 +++++-----------------------
 1 file changed, 5 insertions(+), 23 deletions(-)

diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
index be82bc3e27d0..1db994338fdf 100644
--- a/drivers/media/pci/intel/ipu-bridge.c
+++ b/drivers/media/pci/intel/ipu-bridge.c
@@ -2,6 +2,7 @@
 /* Author: Dan Scally <djrscally@gmail.com> */
 
 #include <linux/acpi.h>
+#include <acpi/acpi_bus.h>
 #include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/i2c.h>
@@ -107,7 +108,6 @@ static const char * const ipu_vcm_types[] = {
 	"lc898212axb",
 };
 
-#if IS_ENABLED(CONFIG_ACPI)
 /*
  * Used to figure out IVSC acpi device by ipu_bridge_get_ivsc_acpi_dev()
  * instead of device and driver match to probe IVSC device.
@@ -127,11 +127,11 @@ static struct acpi_device *ipu_bridge_get_ivsc_acpi_dev(struct acpi_device *adev
 		const struct acpi_device_id *acpi_id = &ivsc_acpi_ids[i];
 		struct acpi_device *consumer, *ivsc_adev;
 
-		acpi_handle handle = acpi_device_handle(adev);
+		acpi_handle handle = acpi_device_handle(ACPI_PTR(adev));
 		for_each_acpi_dev_match(ivsc_adev, acpi_id->id, NULL, -1)
 			/* camera sensor depends on IVSC in DSDT if exist */
 			for_each_acpi_consumer_dev(ivsc_adev, consumer)
-				if (consumer->handle == handle) {
+				if (ACPI_PTR(consumer->handle) == handle) {
 					acpi_dev_put(consumer);
 					return ivsc_adev;
 				}
@@ -139,12 +139,6 @@ static struct acpi_device *ipu_bridge_get_ivsc_acpi_dev(struct acpi_device *adev
 
 	return NULL;
 }
-#else
-static struct acpi_device *ipu_bridge_get_ivsc_acpi_dev(struct acpi_device *adev)
-{
-	return NULL;
-}
-#endif
 
 static int ipu_bridge_match_ivsc_dev(struct device *dev, const void *adev)
 {
@@ -261,9 +255,8 @@ static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_dev
 	struct acpi_pld_info *pld = NULL;
 	acpi_status status = AE_ERROR;
 
-#if IS_ENABLED(CONFIG_ACPI)
-	status = acpi_get_physical_device_location(adev->handle, &pld);
-#endif
+	status = acpi_get_physical_device_location(ACPI_PTR(adev->handle),
+						   &pld);
 	if (ACPI_FAILURE(status)) {
 		dev_warn(ADEV_DEV(adev), "_PLD call failed, using default orientation\n");
 		return V4L2_FWNODE_ORIENTATION_EXTERNAL;
@@ -498,9 +491,7 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
 	if (sensor->csi_dev) {
 		const char *device_hid = "";
 
-#if IS_ENABLED(CONFIG_ACPI)
 		device_hid = acpi_device_hid(sensor->ivsc_adev);
-#endif
 
 		snprintf(sensor->ivsc_name, sizeof(sensor->ivsc_name), "%s-%u",
 			 device_hid, sensor->link);
@@ -671,11 +662,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
 	struct acpi_device *adev = NULL;
 	int ret;
 
-#if IS_ENABLED(CONFIG_ACPI)
 	for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
-#else
-	while (true) {
-#endif
 		if (!ACPI_PTR(adev->status.enabled))
 			continue;
 
@@ -768,15 +755,10 @@ static int ipu_bridge_ivsc_is_ready(void)
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {
-#if IS_ENABLED(CONFIG_ACPI)
 		const struct ipu_sensor_config *cfg =
 			&ipu_supported_sensors[i];
 
 		for_each_acpi_dev_match(sensor_adev, cfg->hid, NULL, -1) {
-#else
-		while (false) {
-			sensor_adev = NULL;
-#endif
 			if (!ACPI_PTR(sensor_adev->status.enabled))
 				continue;
 

-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v3 7/7] media: ipu-bridge: Remove unneeded conditional compilations

[Mauro Carvalho Chehab]

Em Tue, 10 Dec 2024 19:56:04 +0000
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> The ACPI headers have introduced implementations for some of their
> functions when the kernel is not configured with ACPI.
> 
> Let's use them instead of our conditional compilation. It is easier to
> maintain and less prone to errors.
> 
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/pci/intel/ipu-bridge.c | 28 +++++-----------------------
>  1 file changed, 5 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index be82bc3e27d0..1db994338fdf 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -2,6 +2,7 @@
>  /* Author: Dan Scally <djrscally@gmail.com> */
>  
>  #include <linux/acpi.h>
> +#include <acpi/acpi_bus.h>
>  #include <linux/cleanup.h>
>  #include <linux/device.h>
>  #include <linux/i2c.h>
> @@ -107,7 +108,6 @@ static const char * const ipu_vcm_types[] = {
>  	"lc898212axb",
>  };
>  
> -#if IS_ENABLED(CONFIG_ACPI)
>  /*
>   * Used to figure out IVSC acpi device by ipu_bridge_get_ivsc_acpi_dev()
>   * instead of device and driver match to probe IVSC device.
> @@ -127,11 +127,11 @@ static struct acpi_device *ipu_bridge_get_ivsc_acpi_dev(struct acpi_device *adev
>  		const struct acpi_device_id *acpi_id = &ivsc_acpi_ids[i];
>  		struct acpi_device *consumer, *ivsc_adev;
>  
> -		acpi_handle handle = acpi_device_handle(adev);
> +		acpi_handle handle = acpi_device_handle(ACPI_PTR(adev));
>  		for_each_acpi_dev_match(ivsc_adev, acpi_id->id, NULL, -1)
>  			/* camera sensor depends on IVSC in DSDT if exist */
>  			for_each_acpi_consumer_dev(ivsc_adev, consumer)
> -				if (consumer->handle == handle) {
> +				if (ACPI_PTR(consumer->handle) == handle) {
>  					acpi_dev_put(consumer);
>  					return ivsc_adev;
>  				}
> @@ -139,12 +139,6 @@ static struct acpi_device *ipu_bridge_get_ivsc_acpi_dev(struct acpi_device *adev
>  
>  	return NULL;
>  }
> -#else
> -static struct acpi_device *ipu_bridge_get_ivsc_acpi_dev(struct acpi_device *adev)
> -{
> -	return NULL;
> -}
> -#endif
>  
>  static int ipu_bridge_match_ivsc_dev(struct device *dev, const void *adev)
>  {
> @@ -261,9 +255,8 @@ static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_dev
>  	struct acpi_pld_info *pld = NULL;
>  	acpi_status status = AE_ERROR;
>  
> -#if IS_ENABLED(CONFIG_ACPI)
> -	status = acpi_get_physical_device_location(adev->handle, &pld);
> -#endif
> +	status = acpi_get_physical_device_location(ACPI_PTR(adev->handle),
> +						   &pld);
>  	if (ACPI_FAILURE(status)) {
>  		dev_warn(ADEV_DEV(adev), "_PLD call failed, using default orientation\n");
>  		return V4L2_FWNODE_ORIENTATION_EXTERNAL;
> @@ -498,9 +491,7 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
>  	if (sensor->csi_dev) {
>  		const char *device_hid = "";
>  
> -#if IS_ENABLED(CONFIG_ACPI)
>  		device_hid = acpi_device_hid(sensor->ivsc_adev);
> -#endif
>  
>  		snprintf(sensor->ivsc_name, sizeof(sensor->ivsc_name), "%s-%u",
>  			 device_hid, sensor->link);
> @@ -671,11 +662,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
>  	struct acpi_device *adev = NULL;
>  	int ret;
>  
> -#if IS_ENABLED(CONFIG_ACPI)
>  	for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> -#else
> -	while (true) {
> -#endif
>  		if (!ACPI_PTR(adev->status.enabled))
>  			continue;

Heh, that's what I pointed on patch 1: you don't need it there, as this
will be handled on patch 2.

>  
> @@ -768,15 +755,10 @@ static int ipu_bridge_ivsc_is_ready(void)
>  	unsigned int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {
> -#if IS_ENABLED(CONFIG_ACPI)
>  		const struct ipu_sensor_config *cfg =
>  			&ipu_supported_sensors[i];
>  
>  		for_each_acpi_dev_match(sensor_adev, cfg->hid, NULL, -1) {
> -#else
> -		while (false) {
> -			sensor_adev = NULL;
> -#endif
>  			if (!ACPI_PTR(sensor_adev->status.enabled))
>  				continue;
>  
> 

Considering that you drop patch 1, and keep the ACPI dependencies
at the header, as proposed by patches 2-6:

Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kerenel.org>

Thanks,
Mauro

Re: [PATCH v3 7/7] media: ipu-bridge: Remove unneeded conditional compilations

[Ricardo Ribalda]

[only Mauro]

On Wed, 11 Dec 2024 at 09:20, Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
>
> Em Tue, 10 Dec 2024 19:56:04 +0000
> Ricardo Ribalda <ribalda@chromium.org> escreveu:
>
> > The ACPI headers have introduced implementations for some of their
> > functions when the kernel is not configured with ACPI.
> >
> > Let's use them instead of our conditional compilation. It is easier to
> > maintain and less prone to errors.
> >
> > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > ---
> >  drivers/media/pci/intel/ipu-bridge.c | 28 +++++-----------------------
> >  1 file changed, 5 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> > index be82bc3e27d0..1db994338fdf 100644
> > --- a/drivers/media/pci/intel/ipu-bridge.c
> > +++ b/drivers/media/pci/intel/ipu-bridge.c
> > @@ -2,6 +2,7 @@
> >  /* Author: Dan Scally <djrscally@gmail.com> */
> >
> >  #include <linux/acpi.h>
> > +#include <acpi/acpi_bus.h>
> >  #include <linux/cleanup.h>
> >  #include <linux/device.h>
> >  #include <linux/i2c.h>
> > @@ -107,7 +108,6 @@ static const char * const ipu_vcm_types[] = {
> >       "lc898212axb",
> >  };
> >
> > -#if IS_ENABLED(CONFIG_ACPI)
> >  /*
> >   * Used to figure out IVSC acpi device by ipu_bridge_get_ivsc_acpi_dev()
> >   * instead of device and driver match to probe IVSC device.
> > @@ -127,11 +127,11 @@ static struct acpi_device *ipu_bridge_get_ivsc_acpi_dev(struct acpi_device *adev
> >               const struct acpi_device_id *acpi_id = &ivsc_acpi_ids[i];
> >               struct acpi_device *consumer, *ivsc_adev;
> >
> > -             acpi_handle handle = acpi_device_handle(adev);
> > +             acpi_handle handle = acpi_device_handle(ACPI_PTR(adev));
> >               for_each_acpi_dev_match(ivsc_adev, acpi_id->id, NULL, -1)
> >                       /* camera sensor depends on IVSC in DSDT if exist */
> >                       for_each_acpi_consumer_dev(ivsc_adev, consumer)
> > -                             if (consumer->handle == handle) {
> > +                             if (ACPI_PTR(consumer->handle) == handle) {
> >                                       acpi_dev_put(consumer);
> >                                       return ivsc_adev;
> >                               }
> > @@ -139,12 +139,6 @@ static struct acpi_device *ipu_bridge_get_ivsc_acpi_dev(struct acpi_device *adev
> >
> >       return NULL;
> >  }
> > -#else
> > -static struct acpi_device *ipu_bridge_get_ivsc_acpi_dev(struct acpi_device *adev)
> > -{
> > -     return NULL;
> > -}
> > -#endif
> >
> >  static int ipu_bridge_match_ivsc_dev(struct device *dev, const void *adev)
> >  {
> > @@ -261,9 +255,8 @@ static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_dev
> >       struct acpi_pld_info *pld = NULL;
> >       acpi_status status = AE_ERROR;
> >
> > -#if IS_ENABLED(CONFIG_ACPI)
> > -     status = acpi_get_physical_device_location(adev->handle, &pld);
> > -#endif
> > +     status = acpi_get_physical_device_location(ACPI_PTR(adev->handle),
> > +                                                &pld);
> >       if (ACPI_FAILURE(status)) {
> >               dev_warn(ADEV_DEV(adev), "_PLD call failed, using default orientation\n");
> >               return V4L2_FWNODE_ORIENTATION_EXTERNAL;
> > @@ -498,9 +491,7 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
> >       if (sensor->csi_dev) {
> >               const char *device_hid = "";
> >
> > -#if IS_ENABLED(CONFIG_ACPI)
> >               device_hid = acpi_device_hid(sensor->ivsc_adev);
> > -#endif
> >
> >               snprintf(sensor->ivsc_name, sizeof(sensor->ivsc_name), "%s-%u",
> >                        device_hid, sensor->link);
> > @@ -671,11 +662,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
> >       struct acpi_device *adev = NULL;
> >       int ret;
> >
> > -#if IS_ENABLED(CONFIG_ACPI)
> >       for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> > -#else
> > -     while (true) {
> > -#endif
> >               if (!ACPI_PTR(adev->status.enabled))
> >                       continue;
>
> Heh, that's what I pointed on patch 1: you don't need it there, as this
> will be handled on patch 2.
>
> >
> > @@ -768,15 +755,10 @@ static int ipu_bridge_ivsc_is_ready(void)
> >       unsigned int i;
> >
> >       for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {
> > -#if IS_ENABLED(CONFIG_ACPI)
> >               const struct ipu_sensor_config *cfg =
> >                       &ipu_supported_sensors[i];
> >
> >               for_each_acpi_dev_match(sensor_adev, cfg->hid, NULL, -1) {
> > -#else
> > -             while (false) {
> > -                     sensor_adev = NULL;
> > -#endif
> >                       if (!ACPI_PTR(sensor_adev->status.enabled))
> >                               continue;
> >
> >
>
> Considering that you drop patch 1, and keep the ACPI dependencies
> at the header, as proposed by patches 2-6:
>
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kerenel.org>
I will fix the typo in your email


>
> Thanks,
> Mauro



-- 
Ricardo Ribalda

Re: [PATCH v3 7/7] media: ipu-bridge: Remove unneeded conditional compilations

[Mauro Carvalho Chehab]

Em Wed, 11 Dec 2024 09:25:33 +0100
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> [only Mauro]
> 
> On Wed, 11 Dec 2024 at 09:20, Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> >
> > Em Tue, 10 Dec 2024 19:56:04 +0000
> > Ricardo Ribalda <ribalda@chromium.org> escreveu:
> >  
> > > The ACPI headers have introduced implementations for some of their
> > > functions when the kernel is not configured with ACPI.
> > >
> > > Let's use them instead of our conditional compilation. It is easier to
> > > maintain and less prone to errors.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> > > ---
> > >  drivers/media/pci/intel/ipu-bridge.c | 28 +++++-----------------------
> > >  1 file changed, 5 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> > > index be82bc3e27d0..1db994338fdf 100644
> > > --- a/drivers/media/pci/intel/ipu-bridge.c
> > > +++ b/drivers/media/pci/intel/ipu-bridge.c
> > > @@ -2,6 +2,7 @@
> > >  /* Author: Dan Scally <djrscally@gmail.com> */
> > >
> > >  #include <linux/acpi.h>
> > > +#include <acpi/acpi_bus.h>
> > >  #include <linux/cleanup.h>
> > >  #include <linux/device.h>
> > >  #include <linux/i2c.h>
> > > @@ -107,7 +108,6 @@ static const char * const ipu_vcm_types[] = {
> > >       "lc898212axb",
> > >  };
> > >
> > > -#if IS_ENABLED(CONFIG_ACPI)
> > >  /*
> > >   * Used to figure out IVSC acpi device by ipu_bridge_get_ivsc_acpi_dev()
> > >   * instead of device and driver match to probe IVSC device.
> > > @@ -127,11 +127,11 @@ static struct acpi_device *ipu_bridge_get_ivsc_acpi_dev(struct acpi_device *adev
> > >               const struct acpi_device_id *acpi_id = &ivsc_acpi_ids[i];
> > >               struct acpi_device *consumer, *ivsc_adev;
> > >
> > > -             acpi_handle handle = acpi_device_handle(adev);
> > > +             acpi_handle handle = acpi_device_handle(ACPI_PTR(adev));
> > >               for_each_acpi_dev_match(ivsc_adev, acpi_id->id, NULL, -1)
> > >                       /* camera sensor depends on IVSC in DSDT if exist */
> > >                       for_each_acpi_consumer_dev(ivsc_adev, consumer)
> > > -                             if (consumer->handle == handle) {
> > > +                             if (ACPI_PTR(consumer->handle) == handle) {
> > >                                       acpi_dev_put(consumer);
> > >                                       return ivsc_adev;
> > >                               }
> > > @@ -139,12 +139,6 @@ static struct acpi_device *ipu_bridge_get_ivsc_acpi_dev(struct acpi_device *adev
> > >
> > >       return NULL;
> > >  }
> > > -#else
> > > -static struct acpi_device *ipu_bridge_get_ivsc_acpi_dev(struct acpi_device *adev)
> > > -{
> > > -     return NULL;
> > > -}
> > > -#endif
> > >
> > >  static int ipu_bridge_match_ivsc_dev(struct device *dev, const void *adev)
> > >  {
> > > @@ -261,9 +255,8 @@ static enum v4l2_fwnode_orientation ipu_bridge_parse_orientation(struct acpi_dev
> > >       struct acpi_pld_info *pld = NULL;
> > >       acpi_status status = AE_ERROR;
> > >
> > > -#if IS_ENABLED(CONFIG_ACPI)
> > > -     status = acpi_get_physical_device_location(adev->handle, &pld);
> > > -#endif
> > > +     status = acpi_get_physical_device_location(ACPI_PTR(adev->handle),
> > > +                                                &pld);
> > >       if (ACPI_FAILURE(status)) {
> > >               dev_warn(ADEV_DEV(adev), "_PLD call failed, using default orientation\n");
> > >               return V4L2_FWNODE_ORIENTATION_EXTERNAL;
> > > @@ -498,9 +491,7 @@ static void ipu_bridge_create_connection_swnodes(struct ipu_bridge *bridge,
> > >       if (sensor->csi_dev) {
> > >               const char *device_hid = "";
> > >
> > > -#if IS_ENABLED(CONFIG_ACPI)
> > >               device_hid = acpi_device_hid(sensor->ivsc_adev);
> > > -#endif
> > >
> > >               snprintf(sensor->ivsc_name, sizeof(sensor->ivsc_name), "%s-%u",
> > >                        device_hid, sensor->link);
> > > @@ -671,11 +662,7 @@ static int ipu_bridge_connect_sensor(const struct ipu_sensor_config *cfg,
> > >       struct acpi_device *adev = NULL;
> > >       int ret;
> > >
> > > -#if IS_ENABLED(CONFIG_ACPI)
> > >       for_each_acpi_dev_match(adev, cfg->hid, NULL, -1) {
> > > -#else
> > > -     while (true) {
> > > -#endif
> > >               if (!ACPI_PTR(adev->status.enabled))
> > >                       continue;  
> >
> > Heh, that's what I pointed on patch 1: you don't need it there, as this
> > will be handled on patch 2.
> >  
> > >
> > > @@ -768,15 +755,10 @@ static int ipu_bridge_ivsc_is_ready(void)
> > >       unsigned int i;
> > >
> > >       for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {
> > > -#if IS_ENABLED(CONFIG_ACPI)
> > >               const struct ipu_sensor_config *cfg =
> > >                       &ipu_supported_sensors[i];
> > >
> > >               for_each_acpi_dev_match(sensor_adev, cfg->hid, NULL, -1) {
> > > -#else
> > > -             while (false) {
> > > -                     sensor_adev = NULL;
> > > -#endif
> > >                       if (!ACPI_PTR(sensor_adev->status.enabled))
> > >                               continue;
> > >
> > >  
> >
> > Considering that you drop patch 1, and keep the ACPI dependencies
> > at the header, as proposed by patches 2-6:
> >
> > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kerenel.org>  
> I will fix the typo in your email


Heh, slow start... I didn't take any caffeine today yet :-)

Thanks for noticing it!


Thanks,
Mauro

Re: [PATCH v3 7/7] media: ipu-bridge: Remove unneeded conditional compilations

[Sakari Ailus]

Hi Mauro,

On Wed, Dec 11, 2024 at 09:19:54AM +0100, Mauro Carvalho Chehab wrote:
> > @@ -768,15 +755,10 @@ static int ipu_bridge_ivsc_is_ready(void)
> >  	unsigned int i;
> >  
> >  	for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {
> > -#if IS_ENABLED(CONFIG_ACPI)
> >  		const struct ipu_sensor_config *cfg =
> >  			&ipu_supported_sensors[i];
> >  
> >  		for_each_acpi_dev_match(sensor_adev, cfg->hid, NULL, -1) {
> > -#else
> > -		while (false) {
> > -			sensor_adev = NULL;
> > -#endif
> >  			if (!ACPI_PTR(sensor_adev->status.enabled))
> >  				continue;
> >  
> > 
> 
> Considering that you drop patch 1, and keep the ACPI dependencies
> at the header, as proposed by patches 2-6:
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kerenel.org>

The 1st patch fixes a compilation warning when CONFIG_ACPI is disabled.
Merging that patch as a temporary solution is simply easier than making
arragements for merging the ACPI patches to the Media tree so the last
patch may be merged, too.

Besides, the fix should also be backported.

Ricardo: how about adding Cc: stable for that one?

-- 
Regards,

Sakari Ailus

Re: [PATCH v3 7/7] media: ipu-bridge: Remove unneeded conditional compilations

[Ricardo Ribalda]

On Wed, 11 Dec 2024 at 09:32, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Mauro,
>
> On Wed, Dec 11, 2024 at 09:19:54AM +0100, Mauro Carvalho Chehab wrote:
> > > @@ -768,15 +755,10 @@ static int ipu_bridge_ivsc_is_ready(void)
> > >     unsigned int i;
> > >
> > >     for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {
> > > -#if IS_ENABLED(CONFIG_ACPI)
> > >             const struct ipu_sensor_config *cfg =
> > >                     &ipu_supported_sensors[i];
> > >
> > >             for_each_acpi_dev_match(sensor_adev, cfg->hid, NULL, -1) {
> > > -#else
> > > -           while (false) {
> > > -                   sensor_adev = NULL;
> > > -#endif
> > >                     if (!ACPI_PTR(sensor_adev->status.enabled))
> > >                             continue;
> > >
> > >
> >
> > Considering that you drop patch 1, and keep the ACPI dependencies
> > at the header, as proposed by patches 2-6:
> >
> > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kerenel.org>
>
> The 1st patch fixes a compilation warning when CONFIG_ACPI is disabled.
> Merging that patch as a temporary solution is simply easier than making
> arragements for merging the ACPI patches to the Media tree so the last
> patch may be merged, too.
>
> Besides, the fix should also be backported.
>
> Ricardo: how about adding Cc: stable for that one?

Adding:
    Cc: stable@kernel.org
    Fixes: 8810e055b575 ("media: intel/ipu6: Fix build with !ACPI")
to v4

Regards!

>
> --
> Regards,
>
> Sakari Ailus



-- 
Ricardo Ribalda

Re: [PATCH v3 7/7] media: ipu-bridge: Remove unneeded conditional compilations

[Mauro Carvalho Chehab]

Em Wed, 11 Dec 2024 09:37:07 +0100
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> On Wed, 11 Dec 2024 at 09:32, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Mauro,
> >
> > On Wed, Dec 11, 2024 at 09:19:54AM +0100, Mauro Carvalho Chehab wrote:  
> > > > @@ -768,15 +755,10 @@ static int ipu_bridge_ivsc_is_ready(void)
> > > >     unsigned int i;
> > > >
> > > >     for (i = 0; i < ARRAY_SIZE(ipu_supported_sensors); i++) {
> > > > -#if IS_ENABLED(CONFIG_ACPI)
> > > >             const struct ipu_sensor_config *cfg =
> > > >                     &ipu_supported_sensors[i];
> > > >
> > > >             for_each_acpi_dev_match(sensor_adev, cfg->hid, NULL, -1) {
> > > > -#else
> > > > -           while (false) {
> > > > -                   sensor_adev = NULL;
> > > > -#endif
> > > >                     if (!ACPI_PTR(sensor_adev->status.enabled))
> > > >                             continue;
> > > >
> > > >  
> > >
> > > Considering that you drop patch 1, and keep the ACPI dependencies
> > > at the header, as proposed by patches 2-6:
> > >
> > > Reviewed-by: Mauro Carvalho Chehab <mchehab+huawei@kerenel.org>  
> >
> > The 1st patch fixes a compilation warning when CONFIG_ACPI is disabled.
> > Merging that patch as a temporary solution is simply easier than making
> > arragements for merging the ACPI patches to the Media tree so the last
> > patch may be merged, too.

If this is breaking compilation with W=0, then this is a different
matter: this one shall be submitted in separate, against fixes branch,
and the patch title shall be something like:

	media: ipu6: fix compilation when ACPI is disabled

And have cc stable ad fixes.

Once merged upstream, a separate patch series (without patch 1)
should be placed against the -rc kernel where the fix land.

Yet, based on the title, enforced by its description:

	> One of the quirks that we introduced to build with !ACPI && COMPILE_TEST
	> throws the following smatch warning:
	> drivers/media/pci/intel/ipu-bridge.c:752 ipu_bridge_ivsc_is_ready() warn: iterator 'i' not incremented

I don't think it makes sense to c/c stable, as this is just a smatch
warning, for a configuration that will never be used in production.


> > Besides, the fix should also be backported.
> >
> > Ricardo: how about adding Cc: stable for that one?  
> 
> Adding:
>     Cc: stable@kernel.org
>     Fixes: 8810e055b575 ("media: intel/ipu6: Fix build with !ACPI")
> to v4

Thanks,
Mauro

Re: [PATCH v3 7/7] media: ipu-bridge: Remove unneeded conditional compilations

[Dan Carpenter]

On Wed, Dec 11, 2024 at 09:48:54AM +0100, Mauro Carvalho Chehab wrote:
> Yet, based on the title, enforced by its description:
> 
> 	> One of the quirks that we introduced to build with !ACPI && COMPILE_TEST
> 	> throws the following smatch warning:
> 	> drivers/media/pci/intel/ipu-bridge.c:752 ipu_bridge_ivsc_is_ready() warn: iterator 'i' not incremented
> 
> I don't think it makes sense to c/c stable, as this is just a smatch
> warning, for a configuration that will never be used in production.

Yes.  Plus that check has a lot of false positives if you don't have the cross
function DB enabled.  I thought I had fixed it, but I still need to work on it
more.

regards,
dan carpenter

Re: [PATCH v3 0/7] ipu6: get rid of all the IS_ENABLED(CONFIG_ACPI)

[Mauro Carvalho Chehab]

Em Tue, 10 Dec 2024 19:55:57 +0000
Ricardo Ribalda <ribalda@chromium.org> escreveu:

> 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

Not needed, as patch 7 will revert it.

>       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

patches 2-6 look ok to me, but I'll leave it to ACPI maintainers
to review. From my side:

Acked-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

>       media: ipu-bridge: Remove unneeded conditional compilations

See my review with my R-B.

As the crucial changes are at ACPI side, I'm assuming that this will
be merged via ACPI tree.

Regards,
Maur

Thanks,
Mauro