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

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

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