diff mbox series

[2/8] video: Provide screen_info_get_pci_dev() to find screen_info's PCI device

Message ID 20240117125527.23324-3-tzimmermann@suse.de (mailing list archive)
State Handled Elsewhere, archived
Headers show
Series firmware/sysfb: Track parent device for screen_info | expand

Commit Message

Thomas Zimmermann Jan. 17, 2024, 12:39 p.m. UTC
Add screen_info_get_pci_dev() to find the PCI device of an instance
of screen_info. Does nothing on systems without PCI bus.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/Makefile          |  1 +
 drivers/video/screen_info_pci.c | 54 +++++++++++++++++++++++++++++++++
 include/linux/screen_info.h     | 10 ++++++
 3 files changed, 65 insertions(+)
 create mode 100644 drivers/video/screen_info_pci.c

Comments

Javier Martinez Canillas Jan. 29, 2024, 11:04 a.m. UTC | #1
Thomas Zimmermann <tzimmermann@suse.de> writes:

> Add screen_info_get_pci_dev() to find the PCI device of an instance
> of screen_info. Does nothing on systems without PCI bus.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

[...]

> +struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
> +{
> +	struct resource res[SCREEN_INFO_MAX_RESOURCES];
> +	size_t i, numres;
> +	int ret;
> +
> +	ret = screen_info_resources(si, res, ARRAY_SIZE(res));
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +	numres = ret;
> +

I would just drop the ret variable and assign the screen_info_resources()
return value to numres. I think that makes the code easier to follow.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Thomas Zimmermann Jan. 30, 2024, 10:12 a.m. UTC | #2
Hi

Am 29.01.24 um 12:04 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
>> Add screen_info_get_pci_dev() to find the PCI device of an instance
>> of screen_info. Does nothing on systems without PCI bus.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> [...]
> 
>> +struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
>> +{
>> +	struct resource res[SCREEN_INFO_MAX_RESOURCES];
>> +	size_t i, numres;
>> +	int ret;
>> +
>> +	ret = screen_info_resources(si, res, ARRAY_SIZE(res));
>> +	if (ret < 0)
>> +		return ERR_PTR(ret);
>> +	numres = ret;
>> +
> 
> I would just drop the ret variable and assign the screen_info_resources()
> return value to numres. I think that makes the code easier to follow.

The value of ret could be an errno code. We would effectively return 
NULL for errors. And I just noticed that the function docs imply this. 
But NULL is also a valid value if there is no PCI device. I'd prefer to 
keep the errno-pointer around.

Best regards
Thomas

> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>
Javier Martinez Canillas Jan. 30, 2024, 10:23 a.m. UTC | #3
Thomas Zimmermann <tzimmermann@suse.de> writes:

> Hi
>
> Am 29.01.24 um 12:04 schrieb Javier Martinez Canillas:
>> Thomas Zimmermann <tzimmermann@suse.de> writes:
>> 
>>> Add screen_info_get_pci_dev() to find the PCI device of an instance
>>> of screen_info. Does nothing on systems without PCI bus.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>> 
>> [...]
>> 
>>> +struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
>>> +{
>>> +	struct resource res[SCREEN_INFO_MAX_RESOURCES];
>>> +	size_t i, numres;
>>> +	int ret;
>>> +
>>> +	ret = screen_info_resources(si, res, ARRAY_SIZE(res));
>>> +	if (ret < 0)
>>> +		return ERR_PTR(ret);
>>> +	numres = ret;
>>> +
>> 
>> I would just drop the ret variable and assign the screen_info_resources()
>> return value to numres. I think that makes the code easier to follow.
>
> The value of ret could be an errno code. We would effectively return 
> NULL for errors. And I just noticed that the function docs imply this. 
> But NULL is also a valid value if there is no PCI device. I'd prefer to 
> keep the errno-pointer around.
>

Yes. I meant making numres an int instead of size_t (SCREEN_INFO_MAX_RESOURCES
is only 3 after all). That way you could just return ERR_PTR(numres) if is < 0.

No strong preference, just think that the code is easier to read in that case.
diff mbox series

Patch

diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index b929b664d52c..6bbf87c1b579 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -9,6 +9,7 @@  obj-$(CONFIG_VIDEO_NOMODESET)     += nomodeset.o
 obj-$(CONFIG_HDMI)                += hdmi.o
 
 screen_info-y			  := screen_info_generic.o
+screen_info-$(CONFIG_PCI)         += screen_info_pci.o
 
 obj-$(CONFIG_VT)		  += console/
 obj-$(CONFIG_FB_STI)		  += console/
diff --git a/drivers/video/screen_info_pci.c b/drivers/video/screen_info_pci.c
new file mode 100644
index 000000000000..16fe4afa3377
--- /dev/null
+++ b/drivers/video/screen_info_pci.c
@@ -0,0 +1,54 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/pci.h>
+#include <linux/screen_info.h>
+
+static struct pci_dev *__screen_info_pci_dev(struct resource *res)
+{
+	struct pci_dev *pdev;
+
+	if (!(res->flags & IORESOURCE_MEM))
+		return NULL;
+
+	for_each_pci_dev(pdev) {
+		const struct resource *r;
+
+		if ((pdev->class >> 16) != PCI_BASE_CLASS_DISPLAY)
+			continue;
+
+		r = pci_find_resource(pdev, res);
+		if (r)
+			return pdev;
+	}
+
+	return NULL;
+}
+
+/**
+ * screen_info_pci_dev() - Return PCI parent device that contains screen_info's framebuffer
+ * @si: the screen_info
+ *
+ * Returns:
+ * The screen_info's parent device on success, or NULL otherwise.
+ */
+struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
+{
+	struct resource res[SCREEN_INFO_MAX_RESOURCES];
+	size_t i, numres;
+	int ret;
+
+	ret = screen_info_resources(si, res, ARRAY_SIZE(res));
+	if (ret < 0)
+		return ERR_PTR(ret);
+	numres = ret;
+
+	for (i = 0; i < numres; ++i) {
+		struct pci_dev *pdev = __screen_info_pci_dev(&res[i]);
+
+		if (pdev)
+			return pdev;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(screen_info_pci_dev);
diff --git a/include/linux/screen_info.h b/include/linux/screen_info.h
index d4d45395df19..746645b8ee83 100644
--- a/include/linux/screen_info.h
+++ b/include/linux/screen_info.h
@@ -9,6 +9,7 @@ 
  */
 #define SCREEN_INFO_MAX_RESOURCES	3
 
+struct pci_dev;
 struct resource;
 
 static inline bool __screen_info_has_lfb(unsigned int type)
@@ -104,6 +105,15 @@  static inline unsigned int screen_info_video_type(const struct screen_info *si)
 
 int screen_info_resources(const struct screen_info *si, struct resource *r, size_t num);
 
+#if defined(CONFIG_PCI)
+struct pci_dev *screen_info_pci_dev(const struct screen_info *si);
+#else
+static inline struct pci_dev *screen_info_pci_dev(const struct screen_info *si)
+{
+	return NULL;
+}
+#endif
+
 extern struct screen_info screen_info;
 
 #endif /* _SCREEN_INFO_H */