On 26/07/2023 12:08, Hans Verkuil wrote:
> On 04/07/2023 15:13, tumic@xxxxxxxxxx wrote:
>> From: Martin Tůma <martin.tuma@xxxxxxxxxxxxxxxxxxxxx>
>>
>> Digiteq Automotive MGB4 is a modular frame grabber PCIe card for automotive
>> video interfaces. As for now, two modules - FPD-Link and GMSL - are
>> available and supported by the driver. The card has two inputs and two
>> outputs (FPD-Link only).
>>
>> In addition to the video interfaces it also provides a trigger signal
>> interface and a MTD interface for FPGA firmware upload.
>>
>> Signed-off-by: Martin Tůma <martin.tuma@xxxxxxxxxxxxxxxxxxxxx>
>> ---
>> MAINTAINERS | 7 +
>> drivers/media/pci/Kconfig | 1 +
>> drivers/media/pci/Makefile | 1 +
>> drivers/media/pci/mgb4/Kconfig | 17 +
>> drivers/media/pci/mgb4/Makefile | 6 +
>> drivers/media/pci/mgb4/mgb4_cmt.c | 244 +++++++
>> drivers/media/pci/mgb4/mgb4_cmt.h | 17 +
>> drivers/media/pci/mgb4/mgb4_core.c | 711 ++++++++++++++++++
>> drivers/media/pci/mgb4/mgb4_core.h | 72 ++
>> drivers/media/pci/mgb4/mgb4_dma.c | 123 ++++
>> drivers/media/pci/mgb4/mgb4_dma.h | 18 +
>> drivers/media/pci/mgb4/mgb4_i2c.c | 140 ++++
>> drivers/media/pci/mgb4/mgb4_i2c.h | 35 +
>> drivers/media/pci/mgb4/mgb4_io.h | 33 +
>> drivers/media/pci/mgb4/mgb4_regs.c | 30 +
>> drivers/media/pci/mgb4/mgb4_regs.h | 35 +
>> drivers/media/pci/mgb4/mgb4_sysfs.h | 18 +
>> drivers/media/pci/mgb4/mgb4_sysfs_in.c | 757 +++++++++++++++++++
>> drivers/media/pci/mgb4/mgb4_sysfs_out.c | 700 ++++++++++++++++++
>> drivers/media/pci/mgb4/mgb4_sysfs_pci.c | 71 ++
>> drivers/media/pci/mgb4/mgb4_trigger.c | 208 ++++++
>> drivers/media/pci/mgb4/mgb4_trigger.h | 8 +
>> drivers/media/pci/mgb4/mgb4_vin.c | 930 ++++++++++++++++++++++++
>> drivers/media/pci/mgb4/mgb4_vin.h | 69 ++
>> drivers/media/pci/mgb4/mgb4_vout.c | 594 +++++++++++++++
>> drivers/media/pci/mgb4/mgb4_vout.h | 65 ++
>> 26 files changed, 4910 insertions(+)
>> create mode 100644 drivers/media/pci/mgb4/Kconfig
>> create mode 100644 drivers/media/pci/mgb4/Makefile
>> create mode 100644 drivers/media/pci/mgb4/mgb4_cmt.c
>> create mode 100644 drivers/media/pci/mgb4/mgb4_cmt.h
>> create mode 100644 drivers/media/pci/mgb4/mgb4_core.c
>> create mode 100644 drivers/media/pci/mgb4/mgb4_core.h
>> create mode 100644 drivers/media/pci/mgb4/mgb4_dma.c
>> create mode 100644 drivers/media/pci/mgb4/mgb4_dma.h
>> create mode 100644 drivers/media/pci/mgb4/mgb4_i2c.c
>> create mode 100644 drivers/media/pci/mgb4/mgb4_i2c.h
>> create mode 100644 drivers/media/pci/mgb4/mgb4_io.h
>> create mode 100644 drivers/media/pci/mgb4/mgb4_regs.c
>> create mode 100644 drivers/media/pci/mgb4/mgb4_regs.h
>> create mode 100644 drivers/media/pci/mgb4/mgb4_sysfs.h
>> create mode 100644 drivers/media/pci/mgb4/mgb4_sysfs_in.c
>> create mode 100644 drivers/media/pci/mgb4/mgb4_sysfs_out.c
>> create mode 100644 drivers/media/pci/mgb4/mgb4_sysfs_pci.c
>> create mode 100644 drivers/media/pci/mgb4/mgb4_trigger.c
>> create mode 100644 drivers/media/pci/mgb4/mgb4_trigger.h
>> create mode 100644 drivers/media/pci/mgb4/mgb4_vin.c
>> create mode 100644 drivers/media/pci/mgb4/mgb4_vin.h
>> create mode 100644 drivers/media/pci/mgb4/mgb4_vout.c
>> create mode 100644 drivers/media/pci/mgb4/mgb4_vout.h
>>
<snip>
>> +
>> + /* Register the video device */
>> + rv = video_register_device(&vindev->vdev, VFL_TYPE_VIDEO, -1);
>> + if (rv) {
>> + dev_err(dev, "failed to register video device\n");
>> + goto err_v4l2_dev;
>> + }
>> +
>> + /* Module sysfs attributes */
>> + module_attr = MGB4_IS_GMSL(mgbdev)
>> + ? mgb4_gmsl_in_attrs : mgb4_fpdl3_in_attrs;
>> + for (attr = module_attr; *attr; attr++)
>> + device_create_file(&vindev->vdev.dev, *attr);
>
> I believe this can be simplified by using ATTRIBUTE_GROUPS and the
> dev_groups field in the struct pci_driver. This will then be automatically
> created and removed.
While this is true for the global PCI attrs, for the per-video device
attrs there is no dev_groups. However, you can still call device_add_groups
and device_remove_groups instead of device_create_file, AFAICT.
That should be a lot easier. Note that you do need to check the error code:
if it fails, then the probe() should fail as well.
Regards,
Hans
>
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> + debugfs_init(vindev);
>> +#endif
>> +
>> + return vindev;
>> +
>> +err_v4l2_dev:
>> + v4l2_device_unregister(&vindev->v4l2dev);
>> +err_err_irq:
>> + free_irq(err_irq, vindev);
>> +err_vin_irq:
>> + free_irq(vin_irq, vindev);
>> +err_alloc:
>> + kfree(vindev);
>> +
>> + return NULL;
>> +}
