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
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
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
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);
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);@@ -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
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;
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);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);@@ -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
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
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
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@lists.freedesktop.org
-
Daniel Vetter -
Hans de Goede -
Thomas Zimmermann