Messages in this thread | | | Date | Mon, 3 Jul 2023 18:15:25 +0200 | Subject | Re: [PATCH v8 1/5] staging: vc04_services: vchiq_arm: Add new bus type and device type | From | Umang Jain <> |
| |
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
| |