Re: [PATCH v8 1/2] Added Digiteq Automotive MGB4 driver

[Hans Verkuil]

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;
>> +}