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

[Mauro Carvalho Chehab <mchehab+huawei@kernel.org>]

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