| Message ID | 20200529123317.20470-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] drm/i915/dsi: Drop double check for ACPI companion device | expand |
On Fri, May 29, 2020 at 03:33:17PM +0300, Andy Shevchenko wrote: > acpi_dev_get_resources() does perform the NULL pointer check against > ACPI companion device which is given as function parameter. Thus, > there is no need to duplicate this check in the caller. Any comment so far? > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 24 ++++++++------------ > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c > index 574dcfec9577..6f9e08cda964 100644 > --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c > +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c > @@ -426,23 +426,19 @@ static void i2c_acpi_find_adapter(struct intel_dsi *intel_dsi, > { > struct drm_device *drm_dev = intel_dsi->base.base.dev; > struct device *dev = &drm_dev->pdev->dev; > - struct acpi_device *acpi_dev; > + struct acpi_device *acpi_dev = ACPI_COMPANION(dev); > struct list_head resource_list; > struct i2c_adapter_lookup lookup; > > - acpi_dev = ACPI_COMPANION(dev); > - if (acpi_dev) { > - memset(&lookup, 0, sizeof(lookup)); > - lookup.slave_addr = slave_addr; > - lookup.intel_dsi = intel_dsi; > - lookup.dev_handle = acpi_device_handle(acpi_dev); > - > - INIT_LIST_HEAD(&resource_list); > - acpi_dev_get_resources(acpi_dev, &resource_list, > - i2c_adapter_lookup, > - &lookup); > - acpi_dev_free_resource_list(&resource_list); > - } > + memset(&lookup, 0, sizeof(lookup)); > + lookup.slave_addr = slave_addr; > + lookup.intel_dsi = intel_dsi; > + lookup.dev_handle = acpi_device_handle(acpi_dev); > + > + INIT_LIST_HEAD(&resource_list); > + acpi_dev_get_resources(acpi_dev, &resource_list, > + i2c_adapter_lookup, &lookup); > + acpi_dev_free_resource_list(&resource_list); > } > #else > static inline void i2c_acpi_find_adapter(struct intel_dsi *intel_dsi, > -- > 2.26.2 >
On Fri, May 29, 2020 at 03:33:17PM +0300, Andy Shevchenko wrote: > acpi_dev_get_resources() does perform the NULL pointer check against > ACPI companion device which is given as function parameter. Thus, > there is no need to duplicate this check in the caller. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Sorry, I did look at this but apparently forgot to reply... > --- > drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 24 ++++++++------------ > 1 file changed, 10 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c > index 574dcfec9577..6f9e08cda964 100644 > --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c > +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c > @@ -426,23 +426,19 @@ static void i2c_acpi_find_adapter(struct intel_dsi *intel_dsi, > { > struct drm_device *drm_dev = intel_dsi->base.base.dev; > struct device *dev = &drm_dev->pdev->dev; > - struct acpi_device *acpi_dev; > + struct acpi_device *acpi_dev = ACPI_COMPANION(dev); > struct list_head resource_list; > struct i2c_adapter_lookup lookup; > > - acpi_dev = ACPI_COMPANION(dev); > - if (acpi_dev) { > - memset(&lookup, 0, sizeof(lookup)); > - lookup.slave_addr = slave_addr; > - lookup.intel_dsi = intel_dsi; > - lookup.dev_handle = acpi_device_handle(acpi_dev); > - > - INIT_LIST_HEAD(&resource_list); > - acpi_dev_get_resources(acpi_dev, &resource_list, > - i2c_adapter_lookup, > - &lookup); > - acpi_dev_free_resource_list(&resource_list); > - } > + memset(&lookup, 0, sizeof(lookup)); > + lookup.slave_addr = slave_addr; > + lookup.intel_dsi = intel_dsi; > + lookup.dev_handle = acpi_device_handle(acpi_dev); struct i2c_adapter_lookup lookup = { .slave_addr = ... }; ? > + > + INIT_LIST_HEAD(&resource_list); Declare as LIST_HEAD(resource_list); ? > + acpi_dev_get_resources(acpi_dev, &resource_list, > + i2c_adapter_lookup, &lookup); > + acpi_dev_free_resource_list(&resource_list); I was very confused by this code since on the first glance it appears to absolutely nothing. After a deeper look it looks like i2c_adapter_lookup() magically mutates intel_dsi->i2c_bus_num. Did I mention I hate functions with side effects? IMO would be much better if i2c_adapter_lookup() did what it says on the tin and just returned the adapter number and let the caller deal with it. But this is a pre-existing issue with the code and so not directly related to your patch. > } > #else > static inline void i2c_acpi_find_adapter(struct intel_dsi *intel_dsi, > -- > 2.26.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c index 574dcfec9577..6f9e08cda964 100644 --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c @@ -426,23 +426,19 @@ static void i2c_acpi_find_adapter(struct intel_dsi *intel_dsi, { struct drm_device *drm_dev = intel_dsi->base.base.dev; struct device *dev = &drm_dev->pdev->dev; - struct acpi_device *acpi_dev; + struct acpi_device *acpi_dev = ACPI_COMPANION(dev); struct list_head resource_list; struct i2c_adapter_lookup lookup; - acpi_dev = ACPI_COMPANION(dev); - if (acpi_dev) { - memset(&lookup, 0, sizeof(lookup)); - lookup.slave_addr = slave_addr; - lookup.intel_dsi = intel_dsi; - lookup.dev_handle = acpi_device_handle(acpi_dev); - - INIT_LIST_HEAD(&resource_list); - acpi_dev_get_resources(acpi_dev, &resource_list, - i2c_adapter_lookup, - &lookup); - acpi_dev_free_resource_list(&resource_list); - } + memset(&lookup, 0, sizeof(lookup)); + lookup.slave_addr = slave_addr; + lookup.intel_dsi = intel_dsi; + lookup.dev_handle = acpi_device_handle(acpi_dev); + + INIT_LIST_HEAD(&resource_list); + acpi_dev_get_resources(acpi_dev, &resource_list, + i2c_adapter_lookup, &lookup); + acpi_dev_free_resource_list(&resource_list); } #else static inline void i2c_acpi_find_adapter(struct intel_dsi *intel_dsi,
acpi_dev_get_resources() does perform the NULL pointer check against ACPI companion device which is given as function parameter. Thus, there is no need to duplicate this check in the caller. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 24 ++++++++------------ 1 file changed, 10 insertions(+), 14 deletions(-)