dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] drm: Store USB device in struct drm_device
@ 2020-11-03 10:36 Thomas Zimmermann
  2020-11-03 10:36 ` [PATCH v2 1/3] drm: Add USB helpers Thomas Zimmermann
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Thomas Zimmermann @ 2020-11-03 10:36 UTC (permalink / raw)
  To: hdegoede, airlied, daniel, sean, maarten.lankhorst, mripard
  Cc: Thomas Zimmermann, dri-devel

The drivers gm12u320 and udl operate on USB devices. They leave the PCI
device in struct drm_device empty and store the USB device in their own
driver structure. It's expected that DRM core and helpers only touch the
PCI-device field for actual PCI devices.

Fix this special case by upcasting struct drm_device.dev to the USB
device. The drivers' udev variables are being removed.

v2:
	* upcast USB device from struct drm_device.dev (Daniel)

Thomas Zimmermann (3):
  drm: Add USB helpers
  drm/tiny/gm12u320: Retrieve USB device from struct drm_device.dev
  drm/udl: Retrieve USB device from struct drm_device.dev

 Documentation/gpu/drm-internals.rst |  5 +++
 drivers/gpu/drm/tiny/gm12u320.c     | 52 +++++++++++++----------------
 drivers/gpu/drm/udl/udl_connector.c |  9 ++---
 drivers/gpu/drm/udl/udl_drv.c       |  3 --
 drivers/gpu/drm/udl/udl_drv.h       |  1 -
 drivers/gpu/drm/udl/udl_main.c      | 25 ++++++++------
 include/drm/drm_usb_helper.h        | 25 ++++++++++++++
 7 files changed, 73 insertions(+), 47 deletions(-)
 create mode 100644 include/drm/drm_usb_helper.h

--
2.29.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/3] drm: Add USB helpers
  2020-11-03 10:36 [PATCH v2 0/3] drm: Store USB device in struct drm_device Thomas Zimmermann
@ 2020-11-03 10:36 ` Thomas Zimmermann
  2020-11-03 10:48   ` Daniel Vetter
  2020-11-03 10:36 ` [PATCH v2 2/3] drm/tiny/gm12u320: Retrieve USB device from struct drm_device.dev Thomas Zimmermann
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Thomas Zimmermann @ 2020-11-03 10:36 UTC (permalink / raw)
  To: hdegoede, airlied, daniel, sean, maarten.lankhorst, mripard
  Cc: Thomas Zimmermann, dri-devel

DRM drivers for USB devices can share a few helpers. It's currently only
a function to retrieve the USB device's structure from the DRM device.

Putting this code next to the DRM device would make all of DRM depend on
USB headers. So it's in a separate header file.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 Documentation/gpu/drm-internals.rst |  5 +++++
 include/drm/drm_usb_helper.h        | 25 +++++++++++++++++++++++++
 2 files changed, 30 insertions(+)
 create mode 100644 include/drm/drm_usb_helper.h

diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
index 12272b168580..642679407f36 100644
--- a/Documentation/gpu/drm-internals.rst
+++ b/Documentation/gpu/drm-internals.rst
@@ -197,6 +197,11 @@ Utilities
 .. kernel-doc:: include/drm/drm_util.h
    :internal:
 
+USB helpers
+-----------
+
+.. kernel-doc:: include/drm/drm_usb_helper.h
+   :internal:
 
 Legacy Support Code
 ===================
diff --git a/include/drm/drm_usb_helper.h b/include/drm/drm_usb_helper.h
new file mode 100644
index 000000000000..6e8feff890ac
--- /dev/null
+++ b/include/drm/drm_usb_helper.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 or MIT */
+
+#ifndef DRM_USB_HELPER_H
+#define DRM_USB_HELPER_H
+
+#include <linux/usb.h>
+
+#include <drm/drm_device.h>
+
+/**
+ * drm_dev_get_usb_device - Returns a DRM device's USB device
+ * @dev:	The DRM device
+ *
+ * For USB-based DRM devices, returns the corresponding USB device. The
+ * DRM device must store the USB interface's device in its dev field.
+ *
+ * RETURNS:
+ * The USB device
+ */
+static inline struct usb_device *drm_dev_get_usb_device(struct drm_device *dev)
+{
+	return interface_to_usbdev(to_usb_interface(dev->dev));
+}
+
+#endif
-- 
2.29.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/3] drm/tiny/gm12u320: Retrieve USB device from struct drm_device.dev
  2020-11-03 10:36 [PATCH v2 0/3] drm: Store USB device in struct drm_device Thomas Zimmermann
  2020-11-03 10:36 ` [PATCH v2 1/3] drm: Add USB helpers Thomas Zimmermann
@ 2020-11-03 10:36 ` Thomas Zimmermann
  2020-11-03 10:56   ` Daniel Vetter
  2020-11-03 10:36 ` [PATCH v2 3/3] drm/udl: " Thomas Zimmermann
  2020-11-03 11:09 ` [PATCH v2 0/3] drm: Store USB device in struct drm_device Hans de Goede
  3 siblings, 1 reply; 10+ messages in thread
From: Thomas Zimmermann @ 2020-11-03 10:36 UTC (permalink / raw)
  To: hdegoede, airlied, daniel, sean, maarten.lankhorst, mripard
  Cc: Thomas Zimmermann, dri-devel

Drop the driver's udev field in favor of struct drm_device.dev. No
functional changes made.

v2:
	* upcast dev with drm_dev_get_usb_device()

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/tiny/gm12u320.c | 52 +++++++++++++++------------------
 1 file changed, 24 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index cc397671f689..414f08c0bab9 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
@@ -23,6 +23,7 @@
 #include <drm/drm_modeset_helper_vtables.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
+#include <drm/drm_usb_helper.h>
 
 static bool eco_mode;
 module_param(eco_mode, bool, 0644);
@@ -45,7 +46,7 @@ MODULE_PARM_DESC(eco_mode, "Turn on Eco mode (less bright, more silent)");
 #define GM12U320_BLOCK_COUNT		20
 
 #define GM12U320_ERR(fmt, ...) \
-	DRM_DEV_ERROR(&gm12u320->udev->dev, fmt, ##__VA_ARGS__)
+	DRM_DEV_ERROR(gm12u320->dev.dev, fmt, ##__VA_ARGS__)
 
 #define MISC_RCV_EPT			1
 #define DATA_RCV_EPT			2
@@ -85,7 +86,6 @@ struct gm12u320_device {
 	struct drm_device	         dev;
 	struct drm_simple_display_pipe   pipe;
 	struct drm_connector	         conn;
-	struct usb_device               *udev;
 	unsigned char                   *cmd_buf;
 	unsigned char                   *data_buf[GM12U320_BLOCK_COUNT];
 	struct {
@@ -191,6 +191,7 @@ static int gm12u320_misc_request(struct gm12u320_device *gm12u320,
 				 u8 req_a, u8 req_b,
 				 u8 arg_a, u8 arg_b, u8 arg_c, u8 arg_d)
 {
+	struct usb_device *udev = drm_dev_get_usb_device(&gm12u320->dev);
 	int ret, len;
 
 	memcpy(gm12u320->cmd_buf, &cmd_misc, CMD_SIZE);
@@ -202,8 +203,7 @@ static int gm12u320_misc_request(struct gm12u320_device *gm12u320,
 	gm12u320->cmd_buf[25] = arg_d;
 
 	/* Send request */
-	ret = usb_bulk_msg(gm12u320->udev,
-			   usb_sndbulkpipe(gm12u320->udev, MISC_SND_EPT),
+	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, MISC_SND_EPT),
 			   gm12u320->cmd_buf, CMD_SIZE, &len, CMD_TIMEOUT);
 	if (ret || len != CMD_SIZE) {
 		GM12U320_ERR("Misc. req. error %d\n", ret);
@@ -211,8 +211,7 @@ static int gm12u320_misc_request(struct gm12u320_device *gm12u320,
 	}
 
 	/* Read value */
-	ret = usb_bulk_msg(gm12u320->udev,
-			   usb_rcvbulkpipe(gm12u320->udev, MISC_RCV_EPT),
+	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, MISC_RCV_EPT),
 			   gm12u320->cmd_buf, MISC_VALUE_SIZE, &len,
 			   DATA_TIMEOUT);
 	if (ret || len != MISC_VALUE_SIZE) {
@@ -222,8 +221,7 @@ static int gm12u320_misc_request(struct gm12u320_device *gm12u320,
 	/* cmd_buf[0] now contains the read value, which we don't use */
 
 	/* Read status */
-	ret = usb_bulk_msg(gm12u320->udev,
-			   usb_rcvbulkpipe(gm12u320->udev, MISC_RCV_EPT),
+	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, MISC_RCV_EPT),
 			   gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
 			   CMD_TIMEOUT);
 	if (ret || len != READ_STATUS_SIZE) {
@@ -331,6 +329,7 @@ static void gm12u320_fb_update_work(struct work_struct *work)
 	struct gm12u320_device *gm12u320 =
 		container_of(to_delayed_work(work), struct gm12u320_device,
 			     fb_update.work);
+	struct usb_device *udev = drm_dev_get_usb_device(&gm12u320->dev);
 	int block, block_size, len;
 	int ret = 0;
 
@@ -350,43 +349,41 @@ static void gm12u320_fb_update_work(struct work_struct *work)
 		gm12u320->cmd_buf[21] =
 			block | (gm12u320->fb_update.frame << 7);
 
-		ret = usb_bulk_msg(gm12u320->udev,
-			usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT),
-			gm12u320->cmd_buf, CMD_SIZE, &len,
-			CMD_TIMEOUT);
+		ret = usb_bulk_msg(udev,
+				   usb_sndbulkpipe(udev, DATA_SND_EPT),
+				   gm12u320->cmd_buf, CMD_SIZE, &len,
+				   CMD_TIMEOUT);
 		if (ret || len != CMD_SIZE)
 			goto err;
 
 		/* Send data block to device */
-		ret = usb_bulk_msg(gm12u320->udev,
-			usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT),
-			gm12u320->data_buf[block], block_size,
-			&len, DATA_TIMEOUT);
+		ret = usb_bulk_msg(udev,
+				   usb_sndbulkpipe(udev, DATA_SND_EPT),
+				   gm12u320->data_buf[block], block_size,
+				   &len, DATA_TIMEOUT);
 		if (ret || len != block_size)
 			goto err;
 
 		/* Read status */
-		ret = usb_bulk_msg(gm12u320->udev,
-			usb_rcvbulkpipe(gm12u320->udev, DATA_RCV_EPT),
-			gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
-			CMD_TIMEOUT);
+		ret = usb_bulk_msg(udev,
+				   usb_rcvbulkpipe(udev, DATA_RCV_EPT),
+				   gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
+				   CMD_TIMEOUT);
 		if (ret || len != READ_STATUS_SIZE)
 			goto err;
 	}
 
 	/* Send draw command to device */
 	memcpy(gm12u320->cmd_buf, cmd_draw, CMD_SIZE);
-	ret = usb_bulk_msg(gm12u320->udev,
-		usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT),
-		gm12u320->cmd_buf, CMD_SIZE, &len, CMD_TIMEOUT);
+	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, DATA_SND_EPT),
+			   gm12u320->cmd_buf, CMD_SIZE, &len, CMD_TIMEOUT);
 	if (ret || len != CMD_SIZE)
 		goto err;
 
 	/* Read status */
-	ret = usb_bulk_msg(gm12u320->udev,
-		usb_rcvbulkpipe(gm12u320->udev, DATA_RCV_EPT),
-		gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
-		gm12u320->fb_update.draw_status_timeout);
+	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, DATA_RCV_EPT),
+			   gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
+			   gm12u320->fb_update.draw_status_timeout);
 	if (ret || len != READ_STATUS_SIZE)
 		goto err;
 
@@ -638,7 +635,6 @@ static int gm12u320_usb_probe(struct usb_interface *interface,
 	if (IS_ERR(gm12u320))
 		return PTR_ERR(gm12u320);
 
-	gm12u320->udev = interface_to_usbdev(interface);
 	INIT_DELAYED_WORK(&gm12u320->fb_update.work, gm12u320_fb_update_work);
 	mutex_init(&gm12u320->fb_update.lock);
 
-- 
2.29.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/3] drm/udl: Retrieve USB device from struct drm_device.dev
  2020-11-03 10:36 [PATCH v2 0/3] drm: Store USB device in struct drm_device Thomas Zimmermann
  2020-11-03 10:36 ` [PATCH v2 1/3] drm: Add USB helpers Thomas Zimmermann
  2020-11-03 10:36 ` [PATCH v2 2/3] drm/tiny/gm12u320: Retrieve USB device from struct drm_device.dev Thomas Zimmermann
@ 2020-11-03 10:36 ` Thomas Zimmermann
  2020-11-03 10:56   ` Daniel Vetter
  2020-11-03 11:09 ` [PATCH v2 0/3] drm: Store USB device in struct drm_device Hans de Goede
  3 siblings, 1 reply; 10+ messages in thread
From: Thomas Zimmermann @ 2020-11-03 10:36 UTC (permalink / raw)
  To: hdegoede, airlied, daniel, sean, maarten.lankhorst, mripard
  Cc: Thomas Zimmermann, dri-devel

Drop the driver's udev field in favor of struct drm_device.dev. No
functional changes made.

v2:
	* upcast dev with drm_dev_get_usb_device()

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/udl/udl_connector.c |  9 +++++----
 drivers/gpu/drm/udl/udl_drv.c       |  3 ---
 drivers/gpu/drm/udl/udl_drv.h       |  1 -
 drivers/gpu/drm/udl/udl_main.c      | 25 ++++++++++++++-----------
 4 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
index cdc1c42e1669..487e03e1727c 100644
--- a/drivers/gpu/drm/udl/udl_connector.c
+++ b/drivers/gpu/drm/udl/udl_connector.c
@@ -10,6 +10,7 @@
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_usb_helper.h>
 
 #include "udl_connector.h"
 #include "udl_drv.h"
@@ -20,6 +21,7 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block,
 	int ret, i;
 	u8 *read_buff;
 	struct udl_device *udl = data;
+	struct usb_device *udev = drm_dev_get_usb_device(&udl->drm);
 
 	read_buff = kmalloc(2, GFP_KERNEL);
 	if (!read_buff)
@@ -27,10 +29,9 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block,
 
 	for (i = 0; i < len; i++) {
 		int bval = (i + block * EDID_LENGTH) << 8;
-		ret = usb_control_msg(udl->udev,
-				      usb_rcvctrlpipe(udl->udev, 0),
-					  (0x02), (0x80 | (0x02 << 5)), bval,
-					  0xA1, read_buff, 2, HZ);
+		ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
+				      0x02, (0x80 | (0x02 << 5)), bval,
+				      0xA1, read_buff, 2, HZ);
 		if (ret < 1) {
 			DRM_ERROR("Read EDID byte %d failed err %x\n", i, ret);
 			kfree(read_buff);
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 96d4317a2c1b..993469d152da 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -53,7 +53,6 @@ static struct drm_driver driver = {
 
 static struct udl_device *udl_driver_create(struct usb_interface *interface)
 {
-	struct usb_device *udev = interface_to_usbdev(interface);
 	struct udl_device *udl;
 	int r;
 
@@ -62,8 +61,6 @@ static struct udl_device *udl_driver_create(struct usb_interface *interface)
 	if (IS_ERR(udl))
 		return udl;
 
-	udl->udev = udev;
-
 	r = udl_init(udl);
 	if (r)
 		return ERR_PTR(r);
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index b1461f30780b..889bfa21deb0 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -50,7 +50,6 @@ struct urb_list {
 struct udl_device {
 	struct drm_device drm;
 	struct device *dev;
-	struct usb_device *udev;
 
 	struct drm_simple_display_pipe display_pipe;
 
diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
index f5d27f2a5654..208505a39ace 100644
--- a/drivers/gpu/drm/udl/udl_main.c
+++ b/drivers/gpu/drm/udl/udl_main.c
@@ -11,6 +11,7 @@
 #include <drm/drm.h>
 #include <drm/drm_print.h>
 #include <drm/drm_probe_helper.h>
+#include <drm/drm_usb_helper.h>
 
 #include "udl_drv.h"
 
@@ -26,10 +27,10 @@
 #define GET_URB_TIMEOUT	HZ
 #define FREE_URB_TIMEOUT (HZ*2)
 
-static int udl_parse_vendor_descriptor(struct drm_device *dev,
-				       struct usb_device *usbdev)
+static int udl_parse_vendor_descriptor(struct udl_device *udl)
 {
-	struct udl_device *udl = to_udl(dev);
+	struct drm_device *dev = &udl->drm;
+	struct usb_device *udev = drm_dev_get_usb_device(dev);
 	char *desc;
 	char *buf;
 	char *desc_end;
@@ -41,7 +42,7 @@ static int udl_parse_vendor_descriptor(struct drm_device *dev,
 		return false;
 	desc = buf;
 
-	total_len = usb_get_descriptor(usbdev, 0x5f, /* vendor specific */
+	total_len = usb_get_descriptor(udev, 0x5f, /* vendor specific */
 				    0, desc, MAX_VENDOR_DESCRIPTOR_SIZE);
 	if (total_len > 5) {
 		DRM_INFO("vendor descriptor length:%x data:%11ph\n",
@@ -98,19 +99,20 @@ static int udl_parse_vendor_descriptor(struct drm_device *dev,
  */
 static int udl_select_std_channel(struct udl_device *udl)
 {
-	int ret;
 	static const u8 set_def_chn[] = {0x57, 0xCD, 0xDC, 0xA7,
 					 0x1C, 0x88, 0x5E, 0x15,
 					 0x60, 0xFE, 0xC6, 0x97,
 					 0x16, 0x3D, 0x47, 0xF2};
+
 	void *sendbuf;
+	int ret;
+	struct usb_device *udev = drm_dev_get_usb_device(&udl->drm);
 
 	sendbuf = kmemdup(set_def_chn, sizeof(set_def_chn), GFP_KERNEL);
 	if (!sendbuf)
 		return -ENOMEM;
 
-	ret = usb_control_msg(udl->udev,
-			      usb_sndctrlpipe(udl->udev, 0),
+	ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
 			      NR_USB_REQUEST_CHANNEL,
 			      (USB_DIR_OUT | USB_TYPE_VENDOR), 0, 0,
 			      sendbuf, sizeof(set_def_chn),
@@ -202,6 +204,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 	struct urb_node *unode;
 	char *buf;
 	size_t wanted_size = count * size;
+	struct usb_device *udev = drm_dev_get_usb_device(&udl->drm);
 
 	spin_lock_init(&udl->urbs.lock);
 
@@ -229,7 +232,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 		}
 		unode->urb = urb;
 
-		buf = usb_alloc_coherent(udl->udev, size, GFP_KERNEL,
+		buf = usb_alloc_coherent(udev, size, GFP_KERNEL,
 					 &urb->transfer_dma);
 		if (!buf) {
 			kfree(unode);
@@ -243,8 +246,8 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
 		}
 
 		/* urb->transfer_buffer_length set to actual before submit */
-		usb_fill_bulk_urb(urb, udl->udev, usb_sndbulkpipe(udl->udev, 1),
-			buf, size, udl_urb_completion, unode);
+		usb_fill_bulk_urb(urb, udev, usb_sndbulkpipe(udev, 1),
+				  buf, size, udl_urb_completion, unode);
 		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
 
 		list_add_tail(&unode->entry, &udl->urbs.list);
@@ -316,7 +319,7 @@ int udl_init(struct udl_device *udl)
 
 	mutex_init(&udl->gem_lock);
 
-	if (!udl_parse_vendor_descriptor(dev, udl->udev)) {
+	if (!udl_parse_vendor_descriptor(udl)) {
 		ret = -ENODEV;
 		DRM_ERROR("firmware not recognized. Assume incompatible device\n");
 		goto err;
-- 
2.29.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/3] drm: Add USB helpers
  2020-11-03 10:36 ` [PATCH v2 1/3] drm: Add USB helpers Thomas Zimmermann
@ 2020-11-03 10:48   ` Daniel Vetter
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2020-11-03 10:48 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: airlied, hdegoede, dri-devel, sean

On Tue, Nov 03, 2020 at 11:36:54AM +0100, Thomas Zimmermann wrote:
> DRM drivers for USB devices can share a few helpers. It's currently only
> a function to retrieve the USB device's structure from the DRM device.
> 
> Putting this code next to the DRM device would make all of DRM depend on
> USB headers. So it's in a separate header file.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

This seems like overkill for just sharing 1 inline function. Plus it might
tempt people into adding more bus functions again, and maybe I'm a bit too
much burned on the midlayer here, but that doesn't sound like a great
idea.

If we need bus helpers, they should be in the bus library (maybe there
should be a combo of interface_to_usbdev(to_usb_interface()) in the usb
code, but not in drm code).

The pci helpers are really the awkward historical exception because of the
utter horror show that is DRIVER_LEGACY shadow attach driver mode.
-Daniel

> ---
>  Documentation/gpu/drm-internals.rst |  5 +++++
>  include/drm/drm_usb_helper.h        | 25 +++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
>  create mode 100644 include/drm/drm_usb_helper.h
> 
> diff --git a/Documentation/gpu/drm-internals.rst b/Documentation/gpu/drm-internals.rst
> index 12272b168580..642679407f36 100644
> --- a/Documentation/gpu/drm-internals.rst
> +++ b/Documentation/gpu/drm-internals.rst
> @@ -197,6 +197,11 @@ Utilities
>  .. kernel-doc:: include/drm/drm_util.h
>     :internal:
>  
> +USB helpers
> +-----------
> +
> +.. kernel-doc:: include/drm/drm_usb_helper.h
> +   :internal:
>  
>  Legacy Support Code
>  ===================
> diff --git a/include/drm/drm_usb_helper.h b/include/drm/drm_usb_helper.h
> new file mode 100644
> index 000000000000..6e8feff890ac
> --- /dev/null
> +++ b/include/drm/drm_usb_helper.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: GPL-2.0 or MIT */
> +
> +#ifndef DRM_USB_HELPER_H
> +#define DRM_USB_HELPER_H
> +
> +#include <linux/usb.h>
> +
> +#include <drm/drm_device.h>
> +
> +/**
> + * drm_dev_get_usb_device - Returns a DRM device's USB device
> + * @dev:	The DRM device
> + *
> + * For USB-based DRM devices, returns the corresponding USB device. The
> + * DRM device must store the USB interface's device in its dev field.
> + *
> + * RETURNS:
> + * The USB device
> + */
> +static inline struct usb_device *drm_dev_get_usb_device(struct drm_device *dev)
> +{
> +	return interface_to_usbdev(to_usb_interface(dev->dev));
> +}
> +
> +#endif
> -- 
> 2.29.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/3] drm/tiny/gm12u320: Retrieve USB device from struct drm_device.dev
  2020-11-03 10:36 ` [PATCH v2 2/3] drm/tiny/gm12u320: Retrieve USB device from struct drm_device.dev Thomas Zimmermann
@ 2020-11-03 10:56   ` Daniel Vetter
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2020-11-03 10:56 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: airlied, hdegoede, dri-devel, sean

On Tue, Nov 03, 2020 at 11:36:55AM +0100, Thomas Zimmermann wrote:
> Drop the driver's udev field in favor of struct drm_device.dev. No
> functional changes made.
> 
> v2:
> 	* upcast dev with drm_dev_get_usb_device()
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

With the static inline helper either moved to gm12u320.c code or into usb
code this here is 

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/tiny/gm12u320.c | 52 +++++++++++++++------------------
>  1 file changed, 24 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
> index cc397671f689..414f08c0bab9 100644
> --- a/drivers/gpu/drm/tiny/gm12u320.c
> +++ b/drivers/gpu/drm/tiny/gm12u320.c
> @@ -23,6 +23,7 @@
>  #include <drm/drm_modeset_helper_vtables.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_simple_kms_helper.h>
> +#include <drm/drm_usb_helper.h>
>  
>  static bool eco_mode;
>  module_param(eco_mode, bool, 0644);
> @@ -45,7 +46,7 @@ MODULE_PARM_DESC(eco_mode, "Turn on Eco mode (less bright, more silent)");
>  #define GM12U320_BLOCK_COUNT		20
>  
>  #define GM12U320_ERR(fmt, ...) \
> -	DRM_DEV_ERROR(&gm12u320->udev->dev, fmt, ##__VA_ARGS__)
> +	DRM_DEV_ERROR(gm12u320->dev.dev, fmt, ##__VA_ARGS__)
>  
>  #define MISC_RCV_EPT			1
>  #define DATA_RCV_EPT			2
> @@ -85,7 +86,6 @@ struct gm12u320_device {
>  	struct drm_device	         dev;
>  	struct drm_simple_display_pipe   pipe;
>  	struct drm_connector	         conn;
> -	struct usb_device               *udev;
>  	unsigned char                   *cmd_buf;
>  	unsigned char                   *data_buf[GM12U320_BLOCK_COUNT];
>  	struct {
> @@ -191,6 +191,7 @@ static int gm12u320_misc_request(struct gm12u320_device *gm12u320,
>  				 u8 req_a, u8 req_b,
>  				 u8 arg_a, u8 arg_b, u8 arg_c, u8 arg_d)
>  {
> +	struct usb_device *udev = drm_dev_get_usb_device(&gm12u320->dev);
>  	int ret, len;
>  
>  	memcpy(gm12u320->cmd_buf, &cmd_misc, CMD_SIZE);
> @@ -202,8 +203,7 @@ static int gm12u320_misc_request(struct gm12u320_device *gm12u320,
>  	gm12u320->cmd_buf[25] = arg_d;
>  
>  	/* Send request */
> -	ret = usb_bulk_msg(gm12u320->udev,
> -			   usb_sndbulkpipe(gm12u320->udev, MISC_SND_EPT),
> +	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, MISC_SND_EPT),
>  			   gm12u320->cmd_buf, CMD_SIZE, &len, CMD_TIMEOUT);
>  	if (ret || len != CMD_SIZE) {
>  		GM12U320_ERR("Misc. req. error %d\n", ret);
> @@ -211,8 +211,7 @@ static int gm12u320_misc_request(struct gm12u320_device *gm12u320,
>  	}
>  
>  	/* Read value */
> -	ret = usb_bulk_msg(gm12u320->udev,
> -			   usb_rcvbulkpipe(gm12u320->udev, MISC_RCV_EPT),
> +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, MISC_RCV_EPT),
>  			   gm12u320->cmd_buf, MISC_VALUE_SIZE, &len,
>  			   DATA_TIMEOUT);
>  	if (ret || len != MISC_VALUE_SIZE) {
> @@ -222,8 +221,7 @@ static int gm12u320_misc_request(struct gm12u320_device *gm12u320,
>  	/* cmd_buf[0] now contains the read value, which we don't use */
>  
>  	/* Read status */
> -	ret = usb_bulk_msg(gm12u320->udev,
> -			   usb_rcvbulkpipe(gm12u320->udev, MISC_RCV_EPT),
> +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, MISC_RCV_EPT),
>  			   gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
>  			   CMD_TIMEOUT);
>  	if (ret || len != READ_STATUS_SIZE) {
> @@ -331,6 +329,7 @@ static void gm12u320_fb_update_work(struct work_struct *work)
>  	struct gm12u320_device *gm12u320 =
>  		container_of(to_delayed_work(work), struct gm12u320_device,
>  			     fb_update.work);
> +	struct usb_device *udev = drm_dev_get_usb_device(&gm12u320->dev);
>  	int block, block_size, len;
>  	int ret = 0;
>  
> @@ -350,43 +349,41 @@ static void gm12u320_fb_update_work(struct work_struct *work)
>  		gm12u320->cmd_buf[21] =
>  			block | (gm12u320->fb_update.frame << 7);
>  
> -		ret = usb_bulk_msg(gm12u320->udev,
> -			usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT),
> -			gm12u320->cmd_buf, CMD_SIZE, &len,
> -			CMD_TIMEOUT);
> +		ret = usb_bulk_msg(udev,
> +				   usb_sndbulkpipe(udev, DATA_SND_EPT),
> +				   gm12u320->cmd_buf, CMD_SIZE, &len,
> +				   CMD_TIMEOUT);
>  		if (ret || len != CMD_SIZE)
>  			goto err;
>  
>  		/* Send data block to device */
> -		ret = usb_bulk_msg(gm12u320->udev,
> -			usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT),
> -			gm12u320->data_buf[block], block_size,
> -			&len, DATA_TIMEOUT);
> +		ret = usb_bulk_msg(udev,
> +				   usb_sndbulkpipe(udev, DATA_SND_EPT),
> +				   gm12u320->data_buf[block], block_size,
> +				   &len, DATA_TIMEOUT);
>  		if (ret || len != block_size)
>  			goto err;
>  
>  		/* Read status */
> -		ret = usb_bulk_msg(gm12u320->udev,
> -			usb_rcvbulkpipe(gm12u320->udev, DATA_RCV_EPT),
> -			gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
> -			CMD_TIMEOUT);
> +		ret = usb_bulk_msg(udev,
> +				   usb_rcvbulkpipe(udev, DATA_RCV_EPT),
> +				   gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
> +				   CMD_TIMEOUT);
>  		if (ret || len != READ_STATUS_SIZE)
>  			goto err;
>  	}
>  
>  	/* Send draw command to device */
>  	memcpy(gm12u320->cmd_buf, cmd_draw, CMD_SIZE);
> -	ret = usb_bulk_msg(gm12u320->udev,
> -		usb_sndbulkpipe(gm12u320->udev, DATA_SND_EPT),
> -		gm12u320->cmd_buf, CMD_SIZE, &len, CMD_TIMEOUT);
> +	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, DATA_SND_EPT),
> +			   gm12u320->cmd_buf, CMD_SIZE, &len, CMD_TIMEOUT);
>  	if (ret || len != CMD_SIZE)
>  		goto err;
>  
>  	/* Read status */
> -	ret = usb_bulk_msg(gm12u320->udev,
> -		usb_rcvbulkpipe(gm12u320->udev, DATA_RCV_EPT),
> -		gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
> -		gm12u320->fb_update.draw_status_timeout);
> +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, DATA_RCV_EPT),
> +			   gm12u320->cmd_buf, READ_STATUS_SIZE, &len,
> +			   gm12u320->fb_update.draw_status_timeout);
>  	if (ret || len != READ_STATUS_SIZE)
>  		goto err;
>  
> @@ -638,7 +635,6 @@ static int gm12u320_usb_probe(struct usb_interface *interface,
>  	if (IS_ERR(gm12u320))
>  		return PTR_ERR(gm12u320);
>  
> -	gm12u320->udev = interface_to_usbdev(interface);
>  	INIT_DELAYED_WORK(&gm12u320->fb_update.work, gm12u320_fb_update_work);
>  	mutex_init(&gm12u320->fb_update.lock);
>  
> -- 
> 2.29.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/3] drm/udl: Retrieve USB device from struct drm_device.dev
  2020-11-03 10:36 ` [PATCH v2 3/3] drm/udl: " Thomas Zimmermann
@ 2020-11-03 10:56   ` Daniel Vetter
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2020-11-03 10:56 UTC (permalink / raw)
  To: Thomas Zimmermann; +Cc: airlied, hdegoede, dri-devel, sean

On Tue, Nov 03, 2020 at 11:36:56AM +0100, Thomas Zimmermann wrote:
> Drop the driver's udev field in favor of struct drm_device.dev. No
> functional changes made.
> 
> v2:
> 	* upcast dev with drm_dev_get_usb_device()

Again, witht that helper either moved to udl_drv.h or to usb code:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/udl/udl_connector.c |  9 +++++----
>  drivers/gpu/drm/udl/udl_drv.c       |  3 ---
>  drivers/gpu/drm/udl/udl_drv.h       |  1 -
>  drivers/gpu/drm/udl/udl_main.c      | 25 ++++++++++++++-----------
>  4 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/udl/udl_connector.c b/drivers/gpu/drm/udl/udl_connector.c
> index cdc1c42e1669..487e03e1727c 100644
> --- a/drivers/gpu/drm/udl/udl_connector.c
> +++ b/drivers/gpu/drm/udl/udl_connector.c
> @@ -10,6 +10,7 @@
>  #include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_probe_helper.h>
> +#include <drm/drm_usb_helper.h>
>  
>  #include "udl_connector.h"
>  #include "udl_drv.h"
> @@ -20,6 +21,7 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block,
>  	int ret, i;
>  	u8 *read_buff;
>  	struct udl_device *udl = data;
> +	struct usb_device *udev = drm_dev_get_usb_device(&udl->drm);
>  
>  	read_buff = kmalloc(2, GFP_KERNEL);
>  	if (!read_buff)
> @@ -27,10 +29,9 @@ static int udl_get_edid_block(void *data, u8 *buf, unsigned int block,
>  
>  	for (i = 0; i < len; i++) {
>  		int bval = (i + block * EDID_LENGTH) << 8;
> -		ret = usb_control_msg(udl->udev,
> -				      usb_rcvctrlpipe(udl->udev, 0),
> -					  (0x02), (0x80 | (0x02 << 5)), bval,
> -					  0xA1, read_buff, 2, HZ);
> +		ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0),
> +				      0x02, (0x80 | (0x02 << 5)), bval,
> +				      0xA1, read_buff, 2, HZ);
>  		if (ret < 1) {
>  			DRM_ERROR("Read EDID byte %d failed err %x\n", i, ret);
>  			kfree(read_buff);
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index 96d4317a2c1b..993469d152da 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -53,7 +53,6 @@ static struct drm_driver driver = {
>  
>  static struct udl_device *udl_driver_create(struct usb_interface *interface)
>  {
> -	struct usb_device *udev = interface_to_usbdev(interface);
>  	struct udl_device *udl;
>  	int r;
>  
> @@ -62,8 +61,6 @@ static struct udl_device *udl_driver_create(struct usb_interface *interface)
>  	if (IS_ERR(udl))
>  		return udl;
>  
> -	udl->udev = udev;
> -
>  	r = udl_init(udl);
>  	if (r)
>  		return ERR_PTR(r);
> diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
> index b1461f30780b..889bfa21deb0 100644
> --- a/drivers/gpu/drm/udl/udl_drv.h
> +++ b/drivers/gpu/drm/udl/udl_drv.h
> @@ -50,7 +50,6 @@ struct urb_list {
>  struct udl_device {
>  	struct drm_device drm;
>  	struct device *dev;
> -	struct usb_device *udev;
>  
>  	struct drm_simple_display_pipe display_pipe;
>  
> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c
> index f5d27f2a5654..208505a39ace 100644
> --- a/drivers/gpu/drm/udl/udl_main.c
> +++ b/drivers/gpu/drm/udl/udl_main.c
> @@ -11,6 +11,7 @@
>  #include <drm/drm.h>
>  #include <drm/drm_print.h>
>  #include <drm/drm_probe_helper.h>
> +#include <drm/drm_usb_helper.h>
>  
>  #include "udl_drv.h"
>  
> @@ -26,10 +27,10 @@
>  #define GET_URB_TIMEOUT	HZ
>  #define FREE_URB_TIMEOUT (HZ*2)
>  
> -static int udl_parse_vendor_descriptor(struct drm_device *dev,
> -				       struct usb_device *usbdev)
> +static int udl_parse_vendor_descriptor(struct udl_device *udl)
>  {
> -	struct udl_device *udl = to_udl(dev);
> +	struct drm_device *dev = &udl->drm;
> +	struct usb_device *udev = drm_dev_get_usb_device(dev);
>  	char *desc;
>  	char *buf;
>  	char *desc_end;
> @@ -41,7 +42,7 @@ static int udl_parse_vendor_descriptor(struct drm_device *dev,
>  		return false;
>  	desc = buf;
>  
> -	total_len = usb_get_descriptor(usbdev, 0x5f, /* vendor specific */
> +	total_len = usb_get_descriptor(udev, 0x5f, /* vendor specific */
>  				    0, desc, MAX_VENDOR_DESCRIPTOR_SIZE);
>  	if (total_len > 5) {
>  		DRM_INFO("vendor descriptor length:%x data:%11ph\n",
> @@ -98,19 +99,20 @@ static int udl_parse_vendor_descriptor(struct drm_device *dev,
>   */
>  static int udl_select_std_channel(struct udl_device *udl)
>  {
> -	int ret;
>  	static const u8 set_def_chn[] = {0x57, 0xCD, 0xDC, 0xA7,
>  					 0x1C, 0x88, 0x5E, 0x15,
>  					 0x60, 0xFE, 0xC6, 0x97,
>  					 0x16, 0x3D, 0x47, 0xF2};
> +
>  	void *sendbuf;
> +	int ret;
> +	struct usb_device *udev = drm_dev_get_usb_device(&udl->drm);
>  
>  	sendbuf = kmemdup(set_def_chn, sizeof(set_def_chn), GFP_KERNEL);
>  	if (!sendbuf)
>  		return -ENOMEM;
>  
> -	ret = usb_control_msg(udl->udev,
> -			      usb_sndctrlpipe(udl->udev, 0),
> +	ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
>  			      NR_USB_REQUEST_CHANNEL,
>  			      (USB_DIR_OUT | USB_TYPE_VENDOR), 0, 0,
>  			      sendbuf, sizeof(set_def_chn),
> @@ -202,6 +204,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
>  	struct urb_node *unode;
>  	char *buf;
>  	size_t wanted_size = count * size;
> +	struct usb_device *udev = drm_dev_get_usb_device(&udl->drm);
>  
>  	spin_lock_init(&udl->urbs.lock);
>  
> @@ -229,7 +232,7 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
>  		}
>  		unode->urb = urb;
>  
> -		buf = usb_alloc_coherent(udl->udev, size, GFP_KERNEL,
> +		buf = usb_alloc_coherent(udev, size, GFP_KERNEL,
>  					 &urb->transfer_dma);
>  		if (!buf) {
>  			kfree(unode);
> @@ -243,8 +246,8 @@ static int udl_alloc_urb_list(struct drm_device *dev, int count, size_t size)
>  		}
>  
>  		/* urb->transfer_buffer_length set to actual before submit */
> -		usb_fill_bulk_urb(urb, udl->udev, usb_sndbulkpipe(udl->udev, 1),
> -			buf, size, udl_urb_completion, unode);
> +		usb_fill_bulk_urb(urb, udev, usb_sndbulkpipe(udev, 1),
> +				  buf, size, udl_urb_completion, unode);
>  		urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
>  
>  		list_add_tail(&unode->entry, &udl->urbs.list);
> @@ -316,7 +319,7 @@ int udl_init(struct udl_device *udl)
>  
>  	mutex_init(&udl->gem_lock);
>  
> -	if (!udl_parse_vendor_descriptor(dev, udl->udev)) {
> +	if (!udl_parse_vendor_descriptor(udl)) {
>  		ret = -ENODEV;
>  		DRM_ERROR("firmware not recognized. Assume incompatible device\n");
>  		goto err;
> -- 
> 2.29.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/3] drm: Store USB device in struct drm_device
  2020-11-03 10:36 [PATCH v2 0/3] drm: Store USB device in struct drm_device Thomas Zimmermann
                   ` (2 preceding siblings ...)
  2020-11-03 10:36 ` [PATCH v2 3/3] drm/udl: " Thomas Zimmermann
@ 2020-11-03 11:09 ` Hans de Goede
  2020-11-03 11:30   ` Thomas Zimmermann
  3 siblings, 1 reply; 10+ messages in thread
From: Hans de Goede @ 2020-11-03 11:09 UTC (permalink / raw)
  To: Thomas Zimmermann, airlied, daniel, sean, maarten.lankhorst, mripard
  Cc: dri-devel

Hi,

On 11/3/20 11:36 AM, Thomas Zimmermann wrote:
> The drivers gm12u320 and udl operate on USB devices. They leave the PCI
> device in struct drm_device empty and store the USB device in their own
> driver structure. It's expected that DRM core and helpers only touch the
> PCI-device field for actual PCI devices.
> 
> Fix this special case by upcasting struct drm_device.dev to the USB
> device. The drivers' udev variables are being removed.
> 
> v2:
> 	* upcast USB device from struct drm_device.dev (Daniel)
> 
> Thomas Zimmermann (3):
>   drm: Add USB helpers
>   drm/tiny/gm12u320: Retrieve USB device from struct drm_device.dev
>   drm/udl: Retrieve USB device from struct drm_device.dev

Thanks.

The entire series looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Note you may want to wait with pushing this to drm-misc until the
first patch gets at least one other review.

Or at least give others some time to possibly react :)

Regards,

Hans




> 
>  Documentation/gpu/drm-internals.rst |  5 +++
>  drivers/gpu/drm/tiny/gm12u320.c     | 52 +++++++++++++----------------
>  drivers/gpu/drm/udl/udl_connector.c |  9 ++---
>  drivers/gpu/drm/udl/udl_drv.c       |  3 --
>  drivers/gpu/drm/udl/udl_drv.h       |  1 -
>  drivers/gpu/drm/udl/udl_main.c      | 25 ++++++++------
>  include/drm/drm_usb_helper.h        | 25 ++++++++++++++
>  7 files changed, 73 insertions(+), 47 deletions(-)
>  create mode 100644 include/drm/drm_usb_helper.h
> 
> --
> 2.29.0
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/3] drm: Store USB device in struct drm_device
  2020-11-03 11:09 ` [PATCH v2 0/3] drm: Store USB device in struct drm_device Hans de Goede
@ 2020-11-03 11:30   ` Thomas Zimmermann
  2020-11-03 11:45     ` Hans de Goede
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Zimmermann @ 2020-11-03 11:30 UTC (permalink / raw)
  To: Hans de Goede, airlied, daniel, sean, maarten.lankhorst, mripard
  Cc: dri-devel


[-- Attachment #1.1.1.1: Type: text/plain, Size: 2215 bytes --]

Hi

Am 03.11.20 um 12:09 schrieb Hans de Goede:
> Hi,
> 
> On 11/3/20 11:36 AM, Thomas Zimmermann wrote:
>> The drivers gm12u320 and udl operate on USB devices. They leave the PCI
>> device in struct drm_device empty and store the USB device in their own
>> driver structure. It's expected that DRM core and helpers only touch the
>> PCI-device field for actual PCI devices.
>>
>> Fix this special case by upcasting struct drm_device.dev to the USB
>> device. The drivers' udev variables are being removed.
>>
>> v2:
>> 	* upcast USB device from struct drm_device.dev (Daniel)
>>
>> Thomas Zimmermann (3):
>>   drm: Add USB helpers
>>   drm/tiny/gm12u320: Retrieve USB device from struct drm_device.dev
>>   drm/udl: Retrieve USB device from struct drm_device.dev
> 
> Thanks.
> 
> The entire series looks good to me:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Thanks!

> 
> Note you may want to wait with pushing this to drm-misc until the
> first patch gets at least one other review.

Following Daniel's request, I'll drop the first patch and put the helper
into the drivers.

> 
> Or at least give others some time to possibly react :)

Sure, I'll merge it later this week.

Best regards
Thomas

> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>>
>>  Documentation/gpu/drm-internals.rst |  5 +++
>>  drivers/gpu/drm/tiny/gm12u320.c     | 52 +++++++++++++----------------
>>  drivers/gpu/drm/udl/udl_connector.c |  9 ++---
>>  drivers/gpu/drm/udl/udl_drv.c       |  3 --
>>  drivers/gpu/drm/udl/udl_drv.h       |  1 -
>>  drivers/gpu/drm/udl/udl_main.c      | 25 ++++++++------
>>  include/drm/drm_usb_helper.h        | 25 ++++++++++++++
>>  7 files changed, 73 insertions(+), 47 deletions(-)
>>  create mode 100644 include/drm/drm_usb_helper.h
>>
>> --
>> 2.29.0
>>
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 4259 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/3] drm: Store USB device in struct drm_device
  2020-11-03 11:30   ` Thomas Zimmermann
@ 2020-11-03 11:45     ` Hans de Goede
  0 siblings, 0 replies; 10+ messages in thread
From: Hans de Goede @ 2020-11-03 11:45 UTC (permalink / raw)
  To: Thomas Zimmermann, airlied, daniel, sean, maarten.lankhorst, mripard
  Cc: dri-devel

Hi,

On 11/3/20 12:30 PM, Thomas Zimmermann wrote:
> Hi
> 
> Am 03.11.20 um 12:09 schrieb Hans de Goede:
>> Hi,
>>
>> On 11/3/20 11:36 AM, Thomas Zimmermann wrote:
>>> The drivers gm12u320 and udl operate on USB devices. They leave the PCI
>>> device in struct drm_device empty and store the USB device in their own
>>> driver structure. It's expected that DRM core and helpers only touch the
>>> PCI-device field for actual PCI devices.
>>>
>>> Fix this special case by upcasting struct drm_device.dev to the USB
>>> device. The drivers' udev variables are being removed.
>>>
>>> v2:
>>> 	* upcast USB device from struct drm_device.dev (Daniel)
>>>
>>> Thomas Zimmermann (3):
>>>   drm: Add USB helpers
>>>   drm/tiny/gm12u320: Retrieve USB device from struct drm_device.dev
>>>   drm/udl: Retrieve USB device from struct drm_device.dev
>>
>> Thanks.
>>
>> The entire series looks good to me:
>>
>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> 
> Thanks!
> 
>>
>> Note you may want to wait with pushing this to drm-misc until the
>> first patch gets at least one other review.
> 
> Following Daniel's request, I'll drop the first patch and put the helper
> into the drivers.

Ok, that works for me.

>> Or at least give others some time to possibly react :)
> 
> Sure, I'll merge it later this week.

My remark was mostly to give Daniel time to react...

BTW I missed Daniel's reaction again. Now I have figured out why
though for some reason RH's email system is marking Daniels
mails as spam, so they were in my spam folder ?

Regards,

Hans




_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-11-03 11:45 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-03 10:36 [PATCH v2 0/3] drm: Store USB device in struct drm_device Thomas Zimmermann
2020-11-03 10:36 ` [PATCH v2 1/3] drm: Add USB helpers Thomas Zimmermann
2020-11-03 10:48   ` Daniel Vetter
2020-11-03 10:36 ` [PATCH v2 2/3] drm/tiny/gm12u320: Retrieve USB device from struct drm_device.dev Thomas Zimmermann
2020-11-03 10:56   ` Daniel Vetter
2020-11-03 10:36 ` [PATCH v2 3/3] drm/udl: " Thomas Zimmermann
2020-11-03 10:56   ` Daniel Vetter
2020-11-03 11:09 ` [PATCH v2 0/3] drm: Store USB device in struct drm_device Hans de Goede
2020-11-03 11:30   ` Thomas Zimmermann
2020-11-03 11:45     ` Hans de Goede

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).