diff mbox series

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

Message ID 20241210-fix-ipu-v3-7-00e409c84a6c@chromium.org (mailing list archive)
State Superseded, archived
Headers show
Series ipu6: get rid of all the IS_ENABLED(CONFIG_ACPI) | expand

Commit Message

Ricardo Ribalda Dec. 10, 2024, 7:56 p.m. UTC
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(-)

Comments

Mauro Carvalho Chehab Dec. 11, 2024, 8:19 a.m. UTC | #1
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
Ricardo Ribalda Dec. 11, 2024, 8:25 a.m. UTC | #2
[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
Sakari Ailus Dec. 11, 2024, 8:32 a.m. UTC | #3
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?
Ricardo Ribalda Dec. 11, 2024, 8:37 a.m. UTC | #4
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
Mauro Carvalho Chehab Dec. 11, 2024, 8:48 a.m. UTC | #5
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
Mauro Carvalho Chehab Dec. 11, 2024, 8:50 a.m. UTC | #6
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
Dan Carpenter Dec. 11, 2024, 9:14 a.m. UTC | #7
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
diff mbox series

Patch

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;