Re: [PATCH v3 2/2] drm/tinydrm: Add devres versions of drm_of_find_backlight

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Sep 29, 2017 at 02:20:16PM +0200, Noralf Trønnes wrote:
> 
> Den 29.09.2017 05.17, skrev Meghana Madhyastha:
> >On Thu, Sep 28, 2017 at 06:19:35PM +0200, Noralf Trønnes wrote:
> >>Den 28.09.2017 11.15, skrev Meghana Madhyastha:
> >>>Add devm_drm_of_find_backlight and the corresponding release
> >>>function because some drivers such as tinydrm use devres versions
> >>>of functions for requiring device resources.
> >>>
> >>>Signed-off-by: Meghana Madhyastha <meghana.madhyastha@xxxxxxxxx>
> >>>---
> >>>Changes in v3:
> >>>-None
> >>>
> >>>  drivers/gpu/drm/drm_of.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>>  include/drm/drm_of.h     |  2 ++
> >>>  2 files changed, 50 insertions(+)
> >>>
> >>>diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> >>>index d878d3a..238e8e5 100644
> >>>--- a/drivers/gpu/drm/drm_of.c
> >>>+++ b/drivers/gpu/drm/drm_of.c
> >>>@@ -304,3 +304,51 @@ struct backlight_device *drm_of_find_backlight(struct device *dev)
> >>>  	return backlight;
> >>>  }
> >>>  EXPORT_SYMBOL(drm_of_find_backlight);
> >>>+
> >>>+/**
> >>>+ * devm_drm_of_find_backlight_release - Release backlight device
> >>>+ * @dev: Device
> >>>+ *
> >>>+ * This is the release function corresponding to the devm_drm_of_find_backlight.
> >>>+ * Each devres entry is associated with a release function.
> >>>+ */
> >>This is an internal function so no need for docs or exporting. I know
> >>that some devm_ functions have explicit release functions, but I don't
> >>think this is necessary since those users can just use
> >>drm_of_find_backlight() directly instead.
> >I have a question here. devm_drm_of_find_backlight_release is a
> >wrapper around put_device which is passed as a parameter to
> >devm_add_action in devm_drm_of_find_backlight. So isn't the function
> >useful here ?
> 
> We need the function for devm_add_action(), but no one outside of this file
> needs
> it. Hence the static definition.
> 
> We can't use put_device directly:
>     ret = devm_add_action(dev, put_device,
>                   &backlight->dev);
> because devm_add_action expects an argument like this: void (*action)(void
> *)
> and put_device is: void put_device(struct device *dev)
> They differ in their argument type: void * vs. struct device *
> The compiler would complain.
> 
> So we need a wrapper.

Yes this is what I thought. Okay, I misunderstood your previous
suggestion as removing the release function entirely which is why I
asked my question. Now it makes sense and I understood.

Thanks for the clarifications.

Regards,
Meghana
> >>>+static void devm_drm_of_find_backlight_release(void *data)
> >>>+{
> >>>+	put_device(data);
> >>>+}
> >>>+EXPORT_SYMBOL(devm_drm_of_find_backlight_release);
> >>>+
> >>>+/**
> >>>+ * devm_drm_of_find_backlight - Find backlight device in device-tree
> >>>+ * devres version of the function
> >>>+ * @dev: Device
> >>>+ *
> >>>+ * This is the devres version of the function drm_of_find_backlight.
> >>>+ * Some drivers such as tinydrm use devres versions of functions for
> >>No need to mention tinydrm here.
> >>
> >>>+ * requiring device resources.
> >>>+ *
> >>>+ * Returns:
> >>>+ * NULL if there's no backlight property.
> >>>+ * Error pointer -EPROBE_DEFER if the DT node is found, but no backlight device
> >>>+ * is found.
> >>>+ * If the backlight device is found, a pointer to the structure is returned.
> >>>+ */
> >>>+struct backlight_device *devm_drm_of_find_backlight(struct device *dev)
> >>>+{
> >>>+	struct backlight_device *backlight;
> >>>+	int ret;
> >>>+
> >>>+	backlight = drm_of_find_backlight(dev);
> >>>+	if (IS_ERR_OR_NULL(backlight))
> >>>+		return backlight;
> >>>+
> >>>+	ret = devm_add_action(dev, devm_drm_of_find_backlight_release,
> >>>+			      &backlight->dev);
> >>>+	if (ret) {
> >>>+		put_device(&backlight->dev);
> >>>+		return ERR_PTR(ret);
> >>>+	}
> >>>+
> >>>+	return backlight;
> >>>+}
> >>>+EXPORT_SYMBOL(devm_drm_of_find_backlight);
> >>>diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> >>>index e8fba5b..071fb3b 100644
> >>>--- a/include/drm/drm_of.h
> >>>+++ b/include/drm/drm_of.h
> >>>@@ -30,7 +30,9 @@ int drm_of_find_panel_or_bridge(const struct device_node *np,
> >>>  				struct drm_panel **panel,
> >>>  				struct drm_bridge **bridge);
> >>>  struct backlight_device *drm_of_find_backlight(struct device *dev);
> >>>+struct backlight_device *devm_drm_of_find_backlight(struct device *dev);
> >>>  #else
> >>We need a dummy version of devm_drm_of_find_backlight() here that
> >>returns NULL as in the previous patch.
> >>
> >>>+static void devm_drm_of_find_backlight_release(void *data);
> >>And this isn't needed as explained above.
> >>>  static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> >>>  						  struct device_node *port)
> >>>  {
> >>I'd appreciate if you could also switch mi0283qt over to this helper :-)
> >>
> >>Noralf.
> >>
> >>
> 
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux