[PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Export serdev table to the module header, allowing module autoload via
udev/modprobe.

Cc: Lino Sanfilippo <LinoSanfilippo@gmx.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Stefan Wahren <stefan.wahren@i2se.com>
Cc: Rob Herring <robh@kernel.org>
Cc: Johan Hovold <johan@kernel.org>
Cc: netdev@vger.kernel.org
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/net/ethernet/qualcomm/qca_uart.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/qca_uart.c b/drivers/net/ethernet/qualcomm/qca_uart.c
index db6068cd7a1f..6d2ac6cae63f 100644
--- a/drivers/net/ethernet/qualcomm/qca_uart.c
+++ b/drivers/net/ethernet/qualcomm/qca_uart.c
@@ -405,6 +405,12 @@ static void qca_uart_remove(struct serdev_device *serdev)
 	free_netdev(qca->net_dev);
 }
 
+static struct serdev_device_id qca_uart_serdev_id[] = {
+	{ QCAUART_DRV_NAME, },
+	{}
+};
+MODULE_DEVICE_TABLE(serdev, qca_uart_serdev_id);
+
 static struct serdev_device_driver qca_uart_driver = {
 	.probe = qca_uart_probe,
 	.remove = qca_uart_remove,
@@ -412,6 +418,7 @@ static struct serdev_device_driver qca_uart_driver = {
 		.name = QCAUART_DRV_NAME,
 		.of_match_table = of_match_ptr(qca_uart_of_match),
 	},
+	.id_table = qca_uart_serdev_id,
 };
 
 module_serdev_device_driver(qca_uart_driver);
-- 
2.17.1

Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Marcel Holtmann]

Hi Ricardo,

> Export serdev table to the module header, allowing module autoload via
> udev/modprobe.
> 
> Cc: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Stefan Wahren <stefan.wahren@i2se.com>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Johan Hovold <johan@kernel.org>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
> drivers/net/ethernet/qualcomm/qca_uart.c | 7 +++++++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/ethernet/qualcomm/qca_uart.c b/drivers/net/ethernet/qualcomm/qca_uart.c
> index db6068cd7a1f..6d2ac6cae63f 100644
> --- a/drivers/net/ethernet/qualcomm/qca_uart.c
> +++ b/drivers/net/ethernet/qualcomm/qca_uart.c
> @@ -405,6 +405,12 @@ static void qca_uart_remove(struct serdev_device *serdev)
> 	free_netdev(qca->net_dev);
> }
> 
> +static struct serdev_device_id qca_uart_serdev_id[] = {
> +	{ QCAUART_DRV_NAME, },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(serdev, qca_uart_serdev_id);
> +
> static struct serdev_device_driver qca_uart_driver = {
> 	.probe = qca_uart_probe,
> 	.remove = qca_uart_remove,
> @@ -412,6 +418,7 @@ static struct serdev_device_driver qca_uart_driver = {
> 		.name = QCAUART_DRV_NAME,
> 		.of_match_table = of_match_ptr(qca_uart_of_match),
> 	},
> +	.id_table = qca_uart_serdev_id,
> };

the commit message is misleading me. If I build something with ACPI or DT support, then modinfo will show all modalias information for ACPI and DT compatible strings. What else does udev/modprobe actually need? Is something broken with the modalias export?

Regards

Marcel

Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Hi Marcel,
On Mon, Jun 11, 2018 at 3:01 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
>
> the commit message is misleading me. If I build something with ACPI or DT support, then modinfo will show all modalias information for ACPI and DT compatible strings. What else does udev/modprobe actually need? Is something broken with the modalias export?

The main purpose is to autoload drivers for devices that have been
created via sysfs or another module.

Eg1: We have a serial port on a standard computer that has connected a
GPS module. Since it is something that is not in the ACPI nor the DT
table the user will run

echo serdev_gps > /sys/bus/serial/devices/serial0/new_device

Eg2 module: https://github.com/ribalda/linux/blob/415bb3f0076c2b846ebe5409589b8e1e3004f55a/drivers/tty/serdev/test_platform.c

Modprobe does not know what module to load for that device unless
there is a matching MODULE_DEVICE_TABLE
Today, we have the same functionality for i2c devices
https://www.kernel.org/doc/Documentation/i2c/instantiating-devices

I guess the commit message is really bad :), sorry about that. Any
suggestions to improve it?

Thanks!

>
> Regards
>
> Marcel
>


-- 
Ricardo Ribalda

Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Marcel Holtmann]

Hi Marcel,

>> the commit message is misleading me. If I build something with ACPI or DT support, then modinfo will show all modalias information for ACPI and DT compatible strings. What else does udev/modprobe actually need? Is something broken with the modalias export?
> 
> The main purpose is to autoload drivers for devices that have been
> created via sysfs or another module.
> 
> Eg1: We have a serial port on a standard computer that has connected a
> GPS module. Since it is something that is not in the ACPI nor the DT
> table the user will run
> 
> echo serdev_gps > /sys/bus/serial/devices/serial0/new_device
> 
> Eg2 module: https://github.com/ribalda/linux/blob/415bb3f0076c2b846ebe5409589b8e1e3004f55a/drivers/tty/serdev/test_platform.c
> 
> Modprobe does not know what module to load for that device unless
> there is a matching MODULE_DEVICE_TABLE
> Today, we have the same functionality for i2c devices
> https://www.kernel.org/doc/Documentation/i2c/instantiating-devices

but why does this have to be the driver name? I would rather say, create a generic string that describes the hardware and then use that. I think you also want the special handling, as from USB for example, that lets you reference an existing .compatible or ACPI ID as reference so that the driver_data is copied.

Regards

Marcel

Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Hi Marcel,
On Mon, Jun 11, 2018 at 5:28 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Marcel,
>
> >> the commit message is misleading me. If I build something with ACPI or DT support, then modinfo will show all modalias information for ACPI and DT compatible strings. What else does udev/modprobe actually need? Is something broken with the modalias export?
> >
> > The main purpose is to autoload drivers for devices that have been
> > created via sysfs or another module.
> >
> > Eg1: We have a serial port on a standard computer that has connected a
> > GPS module. Since it is something that is not in the ACPI nor the DT
> > table the user will run
> >
> > echo serdev_gps > /sys/bus/serial/devices/serial0/new_device
> >
> > Eg2 module: https://github.com/ribalda/linux/blob/415bb3f0076c2b846ebe5409589b8e1e3004f55a/drivers/tty/serdev/test_platform.c
> >
> > Modprobe does not know what module to load for that device unless
> > there is a matching MODULE_DEVICE_TABLE
> > Today, we have the same functionality for i2c devices
> > https://www.kernel.org/doc/Documentation/i2c/instantiating-devices
>
> but why does this have to be the driver name? I would rather say, create a generic string that describes the hardware and then use that. I think you also want the special handling, as from USB for example, that lets you reference an existing .compatible or ACPI ID as reference so that the driver_data is copied.

We can choose any name, but if there are no special variants (like the
rave-sp driver) I would prefer using the module name. We can of course
use more names, like the part number of the the device, but it is very
convenient to also use the module name, and other subsystems also do
that.

If you want to add specific names for this device please let me know
and I will add them to the list. You know much more about this module
than myself.

Thanks!

>
> Regards
>
> Marcel
>


-- 
Ricardo Ribalda

Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Marcel Holtmann]

Hi Ricardo,

>>>> the commit message is misleading me. If I build something with ACPI or DT support, then modinfo will show all modalias information for ACPI and DT compatible strings. What else does udev/modprobe actually need? Is something broken with the modalias export?
>>> 
>>> The main purpose is to autoload drivers for devices that have been
>>> created via sysfs or another module.
>>> 
>>> Eg1: We have a serial port on a standard computer that has connected a
>>> GPS module. Since it is something that is not in the ACPI nor the DT
>>> table the user will run
>>> 
>>> echo serdev_gps > /sys/bus/serial/devices/serial0/new_device
>>> 
>>> Eg2 module: https://github.com/ribalda/linux/blob/415bb3f0076c2b846ebe5409589b8e1e3004f55a/drivers/tty/serdev/test_platform.c
>>> 
>>> Modprobe does not know what module to load for that device unless
>>> there is a matching MODULE_DEVICE_TABLE
>>> Today, we have the same functionality for i2c devices
>>> https://www.kernel.org/doc/Documentation/i2c/instantiating-devices
>> 
>> but why does this have to be the driver name? I would rather say, create a generic string that describes the hardware and then use that. I think you also want the special handling, as from USB for example, that lets you reference an existing .compatible or ACPI ID as reference so that the driver_data is copied.
> 
> We can choose any name, but if there are no special variants (like the
> rave-sp driver) I would prefer using the module name. We can of course
> use more names, like the part number of the the device, but it is very
> convenient to also use the module name, and other subsystems also do
> that.
> 
> If you want to add specific names for this device please let me know
> and I will add them to the list. You know much more about this module
> than myself.

if we want to use the driver name, then why not build this into the new_device handling itself instead of duplicating that name in each serdev_device_id table. What is the reference here? For example platform device do matching on driver name if I recall this correctly.

However what is most important is that device_get_match_data() actually works. There should be no difference between DT, ACPI or a dynamic device.

Regards

Marcel

Re: [PATCH v2 15/24] net: qualcomm: MODULE_DEVICE_TABLE(serdev)

[Ricardo Ribalda Delgado]

Hi Marcel,
On Mon, Jun 11, 2018 at 5:52 PM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Ricardo,
>
> >>>> the commit message is misleading me. If I build something with ACPI or DT support, then modinfo will show all modalias information for ACPI and DT compatible strings. What else does udev/modprobe actually need? Is something broken with the modalias export?
> >>>
> >>> The main purpose is to autoload drivers for devices that have been
> >>> created via sysfs or another module.
> >>>
> >>> Eg1: We have a serial port on a standard computer that has connected a
> >>> GPS module. Since it is something that is not in the ACPI nor the DT
> >>> table the user will run
> >>>
> >>> echo serdev_gps > /sys/bus/serial/devices/serial0/new_device
> >>>
> >>> Eg2 module: https://github.com/ribalda/linux/blob/415bb3f0076c2b846ebe5409589b8e1e3004f55a/drivers/tty/serdev/test_platform.c
> >>>
> >>> Modprobe does not know what module to load for that device unless
> >>> there is a matching MODULE_DEVICE_TABLE
> >>> Today, we have the same functionality for i2c devices
> >>> https://www.kernel.org/doc/Documentation/i2c/instantiating-devices
> >>
> >> but why does this have to be the driver name? I would rather say, create a generic string that describes the hardware and then use that. I think you also want the special handling, as from USB for example, that lets you reference an existing .compatible or ACPI ID as reference so that the driver_data is copied.
> >
> > We can choose any name, but if there are no special variants (like the
> > rave-sp driver) I would prefer using the module name. We can of course
> > use more names, like the part number of the the device, but it is very
> > convenient to also use the module name, and other subsystems also do
> > that.
> >
> > If you want to add specific names for this device please let me know
> > and I will add them to the list. You know much more about this module
> > than myself.
>
> if we want to use the driver name, then why not build this into the new_device handling itself instead of duplicating that name in each serdev_device_id table. What is the reference here? For example platform device do matching on driver name if I recall this correctly.

We do the matching also over driver name at serdev_device_match():

if (sdrv->id_table)
return !!serdev_match_id(sdrv->id_table, sdev);

return strcmp(sdev->modalias, drv->name) == 0;

This is just for the module autoloading

>
> However what is most important is that device_get_match_data() actually works. There should be no difference between DT, ACPI or a dynamic device.

Agree, will take a look to this tomorrow morning.

>
> Regards
>
> Marcel
>


-- 
Ricardo Ribalda