| Message ID | 20230627201628.207483-2-umang.jain@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: vc04_services: vchiq: Register devices with a custom bus_type | expand |
On Tue, Jun 27, 2023 at 10:16:24PM +0200, Umang Jain wrote: > The devices that the vchiq interface registers (bcm2835-audio, > bcm2835-camera) are implemented and exposed by the VC04 firmware. > The device tree describes the VC04 itself with the resources required > to communicate with it through a mailbox interface. However, the > vchiq interface registers these devices as platform devices. This > also means the specific drivers for these devices are getting > registered as platform drivers. This is not correct and a blatant > abuse of platform device/driver. > > Add a new bus type, vchiq_bus_type and device type (struct vchiq_device) > which will be used to migrate child devices that the vchiq interfaces > creates/registers from the platform device/driver. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > drivers/staging/vc04_services/Makefile | 1 + > .../interface/vchiq_arm/vchiq_device.c | 78 +++++++++++++++++++ > .../interface/vchiq_arm/vchiq_device.h | 43 ++++++++++ > 3 files changed, 122 insertions(+) > create mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c > create mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.h > > diff --git a/drivers/staging/vc04_services/Makefile b/drivers/staging/vc04_services/Makefile > index 44794bdf6173..2d071e55e175 100644 > --- a/drivers/staging/vc04_services/Makefile > +++ b/drivers/staging/vc04_services/Makefile > @@ -5,6 +5,7 @@ vchiq-objs := \ > interface/vchiq_arm/vchiq_core.o \ > interface/vchiq_arm/vchiq_arm.o \ > interface/vchiq_arm/vchiq_debugfs.o \ > + interface/vchiq_arm/vchiq_device.o \ > interface/vchiq_arm/vchiq_connected.o \ > > ifdef CONFIG_VCHIQ_CDEV > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c > new file mode 100644 > index 000000000000..dff312e9735c > --- /dev/null > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c > @@ -0,0 +1,78 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * vchiq_device.c - VCHIQ generic device and bus-type > + * > + * Copyright (c) 2023 Ideas On Board Oy > + */ > + > +#include <linux/device/bus.h> > +#include <linux/slab.h> > +#include <linux/string.h> > + > +#include "vchiq_device.h" > + > +static int vchiq_bus_type_match(struct device *dev, struct device_driver *drv); > + > +struct bus_type vchiq_bus_type = { > + .name = "vchiq-bus", > + .match = vchiq_bus_type_match, > +}; > + > +static int vchiq_bus_type_match(struct device *dev, struct device_driver *drv) > +{ > + if (dev->bus == &vchiq_bus_type && > + strcmp(dev_name(dev), drv->name) == 0) > + return 1; > + return 0; > +} > + > +static void vchiq_device_release(struct device *dev) > +{ > + struct vchiq_device *device; > + > + device = container_of(dev, struct vchiq_device, dev); > + kfree(device); > +} > + > +int vchiq_device_register(struct device *parent, const char *name) > +{ > + struct vchiq_device *device = NULL; No need to set this to NULL. > + int ret; > + > + device = kzalloc(sizeof(*device), GFP_KERNEL); > + if (!device) > + return -ENOMEM; > + > + device->dev.init_name = name; > + device->dev.parent = parent; > + device->dev.bus = &vchiq_bus_type; > + device->dev.release = vchiq_device_release; > + > + ret = device_register(&device->dev); > + if (ret) { > + put_device(&device->dev); > + return -EINVAL; Why not return the error given to you? > + } > + > + return 0; You create a new device, shouldn't you return it? How is it going to be looked up again? > +} > + > +int vchiq_device_unregister(struct device *dev, void *data) You should be passing in a sruct vchiq_device *device here, right? And why the void pointer you do nothing with? > +{ > + device_unregister(dev); > + return 0; > +} No need to export this? thanks, greg k-h
Hi Greg, (resending because it got bounced off a few lists due a line a copied with HTML tag..) On 7/3/23 3:28 PM, Greg KH wrote: > On Tue, Jun 27, 2023 at 10:16:24PM +0200, Umang Jain wrote: >> The devices that the vchiq interface registers (bcm2835-audio, >> bcm2835-camera) are implemented and exposed by the VC04 firmware. >> The device tree describes the VC04 itself with the resources required >> to communicate with it through a mailbox interface. However, the >> vchiq interface registers these devices as platform devices. This >> also means the specific drivers for these devices are getting >> registered as platform drivers. This is not correct and a blatant >> abuse of platform device/driver. >> >> Add a new bus type, vchiq_bus_type and device type (struct vchiq_device) >> which will be used to migrate child devices that the vchiq interfaces >> creates/registers from the platform device/driver. >> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> drivers/staging/vc04_services/Makefile | 1 + >> .../interface/vchiq_arm/vchiq_device.c | 78 +++++++++++++++++++ >> .../interface/vchiq_arm/vchiq_device.h | 43 ++++++++++ >> 3 files changed, 122 insertions(+) >> create mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c >> create mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.h >> >> diff --git a/drivers/staging/vc04_services/Makefile b/drivers/staging/vc04_services/Makefile >> index 44794bdf6173..2d071e55e175 100644 >> --- a/drivers/staging/vc04_services/Makefile >> +++ b/drivers/staging/vc04_services/Makefile >> @@ -5,6 +5,7 @@ vchiq-objs := \ >> interface/vchiq_arm/vchiq_core.o \ >> interface/vchiq_arm/vchiq_arm.o \ >> interface/vchiq_arm/vchiq_debugfs.o \ >> + interface/vchiq_arm/vchiq_device.o \ >> interface/vchiq_arm/vchiq_connected.o \ >> >> ifdef CONFIG_VCHIQ_CDEV >> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c >> new file mode 100644 >> index 000000000000..dff312e9735c >> --- /dev/null >> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c >> @@ -0,0 +1,78 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * vchiq_device.c - VCHIQ generic device and bus-type >> + * >> + * Copyright (c) 2023 Ideas On Board Oy >> + */ >> + >> +#include <linux/device/bus.h> >> +#include <linux/slab.h> >> +#include <linux/string.h> >> + >> +#include "vchiq_device.h" >> + >> +static int vchiq_bus_type_match(struct device *dev, struct device_driver *drv); >> + >> +struct bus_type vchiq_bus_type = { >> + .name = "vchiq-bus", >> + .match = vchiq_bus_type_match, >> +}; >> + >> +static int vchiq_bus_type_match(struct device *dev, struct device_driver *drv) >> +{ >> + if (dev->bus == &vchiq_bus_type && >> + strcmp(dev_name(dev), drv->name) == 0) >> + return 1; >> + return 0; >> +} >> + >> +static void vchiq_device_release(struct device *dev) >> +{ >> + struct vchiq_device *device; >> + >> + device = container_of(dev, struct vchiq_device, dev); >> + kfree(device); >> +} >> + >> +int vchiq_device_register(struct device *parent, const char *name) >> +{ >> + struct vchiq_device *device = NULL; > No need to set this to NULL. > >> + int ret; >> + >> + device = kzalloc(sizeof(*device), GFP_KERNEL); >> + if (!device) >> + return -ENOMEM; >> + >> + device->dev.init_name = name; >> + device->dev.parent = parent; >> + device->dev.bus = &vchiq_bus_type; >> + device->dev.release = vchiq_device_release; >> + >> + ret = device_register(&device->dev); >> + if (ret) { >> + put_device(&device->dev); >> + return -EINVAL; > Why not return the error given to you? > >> + } >> + >> + return 0; > You create a new device, shouldn't you return it? How is it going to be > looked up again? So I haven't come across usage of the device other than unregistered right now. If you look at the platform_device static instances of bcm2835_camera, bcm2835-audio, they are also just used for unregistering the devices only. So for unregistering devices on vchiq_bus - I am using the bus_for_each_dev(&vchiq_bus_type, NULL, NULL, vchiq_device_unregister); in patch 3/5. So basically I can return the instances right now, but there would be no user of those instances. > >> +} >> + >> +int vchiq_device_unregister(struct device *dev, void *data) > You should be passing in a sruct vchiq_device *device here, right? > > And why the void pointer you do nothing with? ack > > >> +{ >> + device_unregister(dev); >> + return 0; >> +} > No need to export this? In the previous reviews you mentioned not to export it. The vchiq_bus_type is internal to the vchiq_driver to register it's child devices. I don't think any other module will be using this bus directly? Maybe I'm mistaken? > > thanks, > > greg k-h
On Mon, Jul 03, 2023 at 06:15:25PM +0200, Umang Jain wrote: > Hi Greg, > > (resending because it got bounced off a few lists due a line a copied with > HTML tag..) > > On 7/3/23 3:28 PM, Greg KH wrote: > > On Tue, Jun 27, 2023 at 10:16:24PM +0200, Umang Jain wrote: > > > The devices that the vchiq interface registers (bcm2835-audio, > > > bcm2835-camera) are implemented and exposed by the VC04 firmware. > > > The device tree describes the VC04 itself with the resources required > > > to communicate with it through a mailbox interface. However, the > > > vchiq interface registers these devices as platform devices. This > > > also means the specific drivers for these devices are getting > > > registered as platform drivers. This is not correct and a blatant > > > abuse of platform device/driver. > > > > > > Add a new bus type, vchiq_bus_type and device type (struct vchiq_device) > > > which will be used to migrate child devices that the vchiq interfaces > > > creates/registers from the platform device/driver. > > > > > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > > --- > > > drivers/staging/vc04_services/Makefile | 1 + > > > .../interface/vchiq_arm/vchiq_device.c | 78 +++++++++++++++++++ > > > .../interface/vchiq_arm/vchiq_device.h | 43 ++++++++++ > > > 3 files changed, 122 insertions(+) > > > create mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c > > > create mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.h > > > > > > diff --git a/drivers/staging/vc04_services/Makefile b/drivers/staging/vc04_services/Makefile > > > index 44794bdf6173..2d071e55e175 100644 > > > --- a/drivers/staging/vc04_services/Makefile > > > +++ b/drivers/staging/vc04_services/Makefile > > > @@ -5,6 +5,7 @@ vchiq-objs := \ > > > interface/vchiq_arm/vchiq_core.o \ > > > interface/vchiq_arm/vchiq_arm.o \ > > > interface/vchiq_arm/vchiq_debugfs.o \ > > > + interface/vchiq_arm/vchiq_device.o \ > > > interface/vchiq_arm/vchiq_connected.o \ > > > ifdef CONFIG_VCHIQ_CDEV > > > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c > > > new file mode 100644 > > > index 000000000000..dff312e9735c > > > --- /dev/null > > > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c > > > @@ -0,0 +1,78 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * vchiq_device.c - VCHIQ generic device and bus-type > > > + * > > > + * Copyright (c) 2023 Ideas On Board Oy > > > + */ > > > + > > > +#include <linux/device/bus.h> > > > +#include <linux/slab.h> > > > +#include <linux/string.h> > > > + > > > +#include "vchiq_device.h" > > > + > > > +static int vchiq_bus_type_match(struct device *dev, struct device_driver *drv); > > > + > > > +struct bus_type vchiq_bus_type = { > > > + .name = "vchiq-bus", > > > + .match = vchiq_bus_type_match, > > > +}; > > > + > > > +static int vchiq_bus_type_match(struct device *dev, struct device_driver *drv) > > > +{ > > > + if (dev->bus == &vchiq_bus_type && > > > + strcmp(dev_name(dev), drv->name) == 0) > > > + return 1; > > > + return 0; > > > +} > > > + > > > +static void vchiq_device_release(struct device *dev) > > > +{ > > > + struct vchiq_device *device; > > > + > > > + device = container_of(dev, struct vchiq_device, dev); > > > + kfree(device); > > > +} > > > + > > > +int vchiq_device_register(struct device *parent, const char *name) > > > +{ > > > + struct vchiq_device *device = NULL; > > No need to set this to NULL. > > > > > + int ret; > > > + > > > + device = kzalloc(sizeof(*device), GFP_KERNEL); > > > + if (!device) > > > + return -ENOMEM; > > > + > > > + device->dev.init_name = name; > > > + device->dev.parent = parent; > > > + device->dev.bus = &vchiq_bus_type; > > > + device->dev.release = vchiq_device_release; > > > + > > > + ret = device_register(&device->dev); > > > + if (ret) { > > > + put_device(&device->dev); > > > + return -EINVAL; > > Why not return the error given to you? > > > > > + } > > > + > > > + return 0; > > You create a new device, shouldn't you return it? How is it going to be > > looked up again? > > So I haven't come across usage of the device other than unregistered right > now. If you look at the platform_device static instances of bcm2835_camera, > bcm2835-audio, they are also just used for unregistering the devices only. That feels odd, you need to register a device, use it, and then unregister it when finished. Otherwise why have it? > So for unregistering devices on vchiq_bus - I am using the > > bus_for_each_dev(&vchiq_bus_type, NULL, NULL, vchiq_device_unregister); Ick, no, that is horrid, unregister the device that was created, that's how all other busses work. > So basically I can return the instances right now, but there would be no > user of those instances. You should save it :) > > > > > +} > > > + > > > +int vchiq_device_unregister(struct device *dev, void *data) > > You should be passing in a sruct vchiq_device *device here, right? > > > > And why the void pointer you do nothing with? > > ack > > > > > > > +{ > > > + device_unregister(dev); > > > + return 0; > > > +} > > No need to export this? > > In the previous reviews you mentioned not to export it. The vchiq_bus_type > is internal to the vchiq_driver to register it's child devices. I don't > think any other module will be using this bus directly? Maybe I'm mistaken? Ok, just didn't match up with the register function export. thanks, greg k-h
diff --git a/drivers/staging/vc04_services/Makefile b/drivers/staging/vc04_services/Makefile index 44794bdf6173..2d071e55e175 100644 --- a/drivers/staging/vc04_services/Makefile +++ b/drivers/staging/vc04_services/Makefile @@ -5,6 +5,7 @@ vchiq-objs := \ interface/vchiq_arm/vchiq_core.o \ interface/vchiq_arm/vchiq_arm.o \ interface/vchiq_arm/vchiq_debugfs.o \ + interface/vchiq_arm/vchiq_device.o \ interface/vchiq_arm/vchiq_connected.o \ ifdef CONFIG_VCHIQ_CDEV diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c new file mode 100644 index 000000000000..dff312e9735c --- /dev/null +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c @@ -0,0 +1,78 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * vchiq_device.c - VCHIQ generic device and bus-type + * + * Copyright (c) 2023 Ideas On Board Oy + */ + +#include <linux/device/bus.h> +#include <linux/slab.h> +#include <linux/string.h> + +#include "vchiq_device.h" + +static int vchiq_bus_type_match(struct device *dev, struct device_driver *drv); + +struct bus_type vchiq_bus_type = { + .name = "vchiq-bus", + .match = vchiq_bus_type_match, +}; + +static int vchiq_bus_type_match(struct device *dev, struct device_driver *drv) +{ + if (dev->bus == &vchiq_bus_type && + strcmp(dev_name(dev), drv->name) == 0) + return 1; + return 0; +} + +static void vchiq_device_release(struct device *dev) +{ + struct vchiq_device *device; + + device = container_of(dev, struct vchiq_device, dev); + kfree(device); +} + +int vchiq_device_register(struct device *parent, const char *name) +{ + struct vchiq_device *device = NULL; + int ret; + + device = kzalloc(sizeof(*device), GFP_KERNEL); + if (!device) + return -ENOMEM; + + device->dev.init_name = name; + device->dev.parent = parent; + device->dev.bus = &vchiq_bus_type; + device->dev.release = vchiq_device_release; + + ret = device_register(&device->dev); + if (ret) { + put_device(&device->dev); + return -EINVAL; + } + + return 0; +} + +int vchiq_device_unregister(struct device *dev, void *data) +{ + device_unregister(dev); + return 0; +} + +int vchiq_driver_register(struct vchiq_driver *vchiq_drv) +{ + vchiq_drv->driver.bus = &vchiq_bus_type; + + return driver_register(&vchiq_drv->driver); +} +EXPORT_SYMBOL_GPL(vchiq_driver_register); + +void vchiq_driver_unregister(struct vchiq_driver *vchiq_drv) +{ + driver_unregister(&vchiq_drv->driver); +} +EXPORT_SYMBOL_GPL(vchiq_driver_unregister); diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.h b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.h new file mode 100644 index 000000000000..dcd8a3d9eebe --- /dev/null +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.h @@ -0,0 +1,43 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (c) 2023 Ideas On Board Oy + */ + +#ifndef _VCHIQ_DEVICE_H +#define _VCHIQ_DEVICE_H + +#include <linux/device.h> + +struct vchiq_device { + struct device dev; +}; + +struct vchiq_driver { + int (*probe)(struct vchiq_device *device); + void (*remove)(struct vchiq_device *device); + int (*resume)(struct vchiq_device *device); + int (*suspend)(struct vchiq_device *device, + pm_message_t state); + struct device_driver driver; +}; + +extern struct bus_type vchiq_bus_type; + +int vchiq_device_register(struct device *parent, const char *name); +int vchiq_device_unregister(struct device *dev, void *data); + +int vchiq_driver_register(struct vchiq_driver *vchiq_drv); +void vchiq_driver_unregister(struct vchiq_driver *vchiq_drv); + +/** + * module_vchiq_driver() - Helper macro for registering a vchiq driver + * @__vchiq_driver: vchiq driver struct + * + * Helper macro for vchiq drivers which do not do anything special in + * module init/exit. This eliminates a lot of boilerplate. Each module may only + * use this macro once, and calling it replaces module_init() and module_exit() + */ +#define module_vchiq_driver(__vchiq_driver) \ + module_driver(__vchiq_driver, vchiq_driver_register, vchiq_driver_unregister) + +#endif /* _VCHIQ_DEVICE_H */
The devices that the vchiq interface registers (bcm2835-audio, bcm2835-camera) are implemented and exposed by the VC04 firmware. The device tree describes the VC04 itself with the resources required to communicate with it through a mailbox interface. However, the vchiq interface registers these devices as platform devices. This also means the specific drivers for these devices are getting registered as platform drivers. This is not correct and a blatant abuse of platform device/driver. Add a new bus type, vchiq_bus_type and device type (struct vchiq_device) which will be used to migrate child devices that the vchiq interfaces creates/registers from the platform device/driver. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- drivers/staging/vc04_services/Makefile | 1 + .../interface/vchiq_arm/vchiq_device.c | 78 +++++++++++++++++++ .../interface/vchiq_arm/vchiq_device.h | 43 ++++++++++ 3 files changed, 122 insertions(+) create mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c create mode 100644 drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.h