| Message ID | 20230719164427.1383646-1-umang.jain@ideasonboard.com |
|---|---|
| Headers | show |
| Series | staging: vc04_services: vchiq: Register devices with a custom bus_type | expand |
Hi Umang, Am 19.07.23 um 18:44 schrieb Umang Jain: > 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 | 102 ++++++++++++++++++ > .../interface/vchiq_arm/vchiq_device.h | 54 ++++++++++ > 3 files changed, 157 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..d7dfe4173579 > --- /dev/null > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.c > @@ -0,0 +1,102 @@ > +// 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) > +{ > + if (dev->bus == &vchiq_bus_type && > + strcmp(dev_name(dev), drv->name) == 0) > + return 1; > + > + return 0; > +} > + > +static int vchiq_bus_uevent(const struct device *dev, struct kobj_uevent_env *env) > +{ > + const struct vchiq_device *device = container_of_const(dev, struct vchiq_device, dev); > + > + return add_uevent_var(env, "MODALIAS=%s", dev_name(&device->dev)); > +} > + > +static int vchiq_bus_probe(struct device *dev) > +{ > + struct vchiq_device *device = to_vchiq_device(dev); > + struct vchiq_driver *driver = to_vchiq_driver(dev->driver); > + int ret; > + > + ret = driver->probe(device); > + if (ret == 0) > + return 0; > + > + return ret; Why not returning the result of probe directly? > +} > + > +struct bus_type vchiq_bus_type = { > + .name = "vchiq-bus", > + .match = vchiq_bus_type_match, > + .uevent = vchiq_bus_uevent, > + .probe = vchiq_bus_probe, > +}; > + > +static void vchiq_device_release(struct device *dev) > +{ > + struct vchiq_device *device = to_vchiq_device(dev); > + > + kfree(device); > +} > + > +struct vchiq_device * > +vchiq_device_register(struct device *parent, const char *name) > +{ > + struct vchiq_device *device; > + int ret; > + > + device = kzalloc(sizeof(*device), GFP_KERNEL); > + if (!device) { > + dev_err(parent, "Cannot register %s: Insufficient memory\n", > + name); > + return NULL; > + } > + > + device->dev.init_name = name; > + device->dev.parent = parent; > + device->dev.bus = &vchiq_bus_type; > + device->dev.release = vchiq_device_release; Not sure, but maybe a good place to set the DMA mask? > + > + ret = device_register(&device->dev); > + if (ret) { > + dev_err(parent, "Cannot register %s: %d\n", name, ret); > + put_device(&device->dev); > + return NULL; > + } > + > + return device; > +} > + > +void vchiq_device_unregister(struct vchiq_device *vchiq_dev) > +{ > + device_unregister(&vchiq_dev->dev); > +} > + > +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..7eaaf9a91cda > --- /dev/null > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_device.h > @@ -0,0 +1,54 @@ > +/* 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; > +}; > + > +static inline struct vchiq_device *to_vchiq_device(struct device *d) > +{ > + return container_of(d, struct vchiq_device, dev); > +} > + > +static inline struct vchiq_driver *to_vchiq_driver(struct device_driver *d) > +{ > + return container_of(d, struct vchiq_driver, driver); > +} > + > +extern struct bus_type vchiq_bus_type; > + > +struct vchiq_device * > +vchiq_device_register(struct device *parent, const char *name); > +void vchiq_device_unregister(struct vchiq_device *dev); > + > +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 */
Hi Umang, Am 19.07.23 um 18:44 schrieb Umang Jain: > Similar to how bcm2385-camera device is registered, register the > bcm2835-audio with vchiq_bus_type as well. > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > .../vc04_services/bcm2835-audio/bcm2835.c | 20 +++++++++---------- > .../interface/vchiq_arm/vchiq_arm.c | 6 +++--- > 2 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c > index 00bc898b0189..70e5e0942743 100644 > --- a/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c > +++ b/drivers/staging/vc04_services/bcm2835-audio/bcm2835.c > @@ -1,12 +1,12 @@ > // SPDX-License-Identifier: GPL-2.0 > /* Copyright 2011 Broadcom Corporation. All rights reserved. */ > > -#include <linux/platform_device.h> > - > #include <linux/init.h> > #include <linux/slab.h> > #include <linux/module.h> > > +#include "../interface/vchiq_arm/vchiq_arm.h" > +#include "../interface/vchiq_arm/vchiq_device.h" > #include "bcm2835.h" > > static bool enable_hdmi; > @@ -268,9 +268,9 @@ static int snd_add_child_devices(struct device *device, u32 numchans) > return 0; > } > > -static int snd_bcm2835_alsa_probe(struct platform_device *pdev) > +static int snd_bcm2835_alsa_probe(struct vchiq_device *device) > { > - struct device *dev = &pdev->dev; > + struct device *dev = &device->dev; > int err; > > if (num_channels <= 0 || num_channels > MAX_SUBSTREAMS) { > @@ -292,32 +292,32 @@ static int snd_bcm2835_alsa_probe(struct platform_device *pdev) > > #ifdef CONFIG_PM > > -static int snd_bcm2835_alsa_suspend(struct platform_device *pdev, > +static int snd_bcm2835_alsa_suspend(struct vchiq_device *device, > pm_message_t state) > { > return 0; > } > > -static int snd_bcm2835_alsa_resume(struct platform_device *pdev) > +static int snd_bcm2835_alsa_resume(struct vchiq_device *device) > { > return 0; > } > > #endif > > -static struct platform_driver bcm2835_alsa_driver = { > +static struct vchiq_driver bcm2835_alsa_driver = { > .probe = snd_bcm2835_alsa_probe, > #ifdef CONFIG_PM > .suspend = snd_bcm2835_alsa_suspend, > .resume = snd_bcm2835_alsa_resume, > #endif > .driver = { > - .name = "bcm2835_audio", > + .name = "bcm2835-audio", At least this change is not mentioned in the commit log. Thanks > }, > }; > -module_platform_driver(bcm2835_alsa_driver); > +module_vchiq_driver(bcm2835_alsa_driver); > > MODULE_AUTHOR("Dom Cobley"); > MODULE_DESCRIPTION("Alsa driver for BCM2835 chip"); > MODULE_LICENSE("GPL"); > -MODULE_ALIAS("platform:bcm2835_audio"); > +MODULE_ALIAS("bcm2835-audio"); > diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > index d941e9640415..f7c2dce5ab09 100644 > --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c > @@ -67,12 +67,12 @@ int vchiq_susp_log_level = VCHIQ_LOG_ERROR; > DEFINE_SPINLOCK(msg_queue_spinlock); > struct vchiq_state g_state; > > -static struct platform_device *bcm2835_audio; > /* > * The devices implemented in the VCHIQ firmware are not discoverable, > * so we need to maintain a list of them in order to register them with > * the interface. > */ > +static struct vchiq_device *bcm2835_audio; > static struct vchiq_device *bcm2835_camera; > > struct vchiq_drvdata { > @@ -1845,7 +1845,7 @@ static int vchiq_probe(struct platform_device *pdev) > goto error_exit; > } > > - bcm2835_audio = vchiq_register_child(pdev, "bcm2835_audio"); > + bcm2835_audio = vchiq_device_register(&pdev->dev, "bcm2835-audio"); > bcm2835_camera = vchiq_device_register(&pdev->dev, "bcm2835-camera"); > > return 0; > @@ -1858,7 +1858,7 @@ static int vchiq_probe(struct platform_device *pdev) > > static void vchiq_remove(struct platform_device *pdev) > { > - platform_device_unregister(bcm2835_audio); > + vchiq_device_unregister(bcm2835_audio); > vchiq_device_unregister(bcm2835_camera); > vchiq_debugfs_deinit(); > vchiq_deregister_chrdev();
Hi Stefan , On 7/19/23 11:05 PM, Stefan Wahren wrote: > Hi Umang, > > Am 19.07.23 um 18:54 schrieb Umang Jain: >> Hi, >> >> One comment, >> >> On 7/19/23 10:14 PM, Umang Jain wrote: >>> The patch series added a new bus type vchiq_bus_type and registers >>> child devices in order to move them away from using platform >>> device/driver. >>> >>> Patch 1/5 and 2/5 adds a new bus_type and registers them to vchiq >>> interface >>> >>> Patch 3/5 and 4/5 moves the bcm2835-camera and bcm2835-audio >>> to the new bus respectively >>> >>> Patch 5/5 removes a platform registeration helper which is no >>> longer required. >>> >>> Changes in v9: >>> - Fix module autoloading >> >> While the autoloading of bcm2835-audio, bcm2835-camera is fixed as >> part of this series, there is one WARN coming in when bcm2835-audio >> is loaded regarding dma_alloc_attr >> >> dmesg output: https://paste.debian.net/plain/1286359 > > is it possible that after your patch series no DMA mask like > DMA_BIT_MASK(32) is provided? I am trying to set DMA_BIT_MASK(32) via dma_set_mask_and_coherent() but it fails with -EIO > >> >> I am investigating further... >>> - Implement bus_type's probe() callback to load drivers >>> - Implement bus_type's uevent() to make sure appropriate drivers are >>> loaded when device are registed from vchiq. >>> >>> Changes in v8: >>> - Drop dual licensing. Instead use GPL-2.0 only for patch 1/5 >>> >>> Changes in v7: >>> (5 out of 6 patches from v6 merged) >>> - Split the main patch (6/6) as requested. >>> - Use struct vchiq_device * instead of struct device * in >>> all bus functions. >>> - Drop additional name attribute displayed in sysfs (redundant info) >>> - Document vchiq_interface doesn't enumerate device discovery >>> - remove EXPORT_SYMBOL_GPL(vchiq_bus_type) >>> >>> Changes in v6: >>> - Split struct device and struct driver wrappers in vchiq_device.[ch] >>> - Move vchiq_bus_type definition to vchiq_device.[ch] as well >>> - return error on bus_register() failure >>> - drop dma_set_mask_and_coherent >>> - trivial variable name change >>> >>> Changes in v5: >>> - Fixup missing "staging: " in commits' subject line >>> - No code changes from v4 >>> >>> Changes in v4: >>> - Introduce patches to drop include directives from Makefile >>> >>> Changes in v3: >>> - Rework entirely to replace platform devices/driver model >>> >>> -v2: >>> https://lore.kernel.org/all/20221222191500.515795-1-umang.jain@ideasonboard.com/ >>> >>> >>> -v1: >>> https://lore.kernel.org/all/20221220084404.19280-1-umang.jain@ideasonboard.com/ >>> >>> >>> Umang Jain (5): >>> staging: vc04_services: vchiq_arm: Add new bus type and device type >>> staging: vc04_services: vchiq_arm: Register vchiq_bus_type >>> staging: bcm2835-camera: Register bcm2835-camera with vchiq_bus_type >>> staging: bcm2835-audio: Register bcm2835-audio with vchiq_bus_type >>> staging: vc04_services: vchiq_arm: Remove vchiq_register_child() >>> >>> drivers/staging/vc04_services/Makefile | 1 + >>> .../vc04_services/bcm2835-audio/bcm2835.c | 20 ++-- >>> .../bcm2835-camera/bcm2835-camera.c | 17 +-- >>> .../interface/vchiq_arm/vchiq_arm.c | 48 ++++----- >>> .../interface/vchiq_arm/vchiq_device.c | 102 >>> ++++++++++++++++++ >>> .../interface/vchiq_arm/vchiq_device.h | 54 ++++++++++ >>> 6 files changed, 196 insertions(+), 46 deletions(-) >>> 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 >>> >>
Hi Umang, Am 20.07.23 um 14:08 schrieb Umang Jain: > Hi Stefan , > > > On 7/19/23 11:05 PM, Stefan Wahren wrote: >> Hi Umang, >> >> Am 19.07.23 um 18:54 schrieb Umang Jain: >>> Hi, >>> >>> One comment, >>> >>> On 7/19/23 10:14 PM, Umang Jain wrote: >>>> The patch series added a new bus type vchiq_bus_type and registers >>>> child devices in order to move them away from using platform >>>> device/driver. >>>> >>>> Patch 1/5 and 2/5 adds a new bus_type and registers them to vchiq >>>> interface >>>> >>>> Patch 3/5 and 4/5 moves the bcm2835-camera and bcm2835-audio >>>> to the new bus respectively >>>> >>>> Patch 5/5 removes a platform registeration helper which is no >>>> longer required. >>>> >>>> Changes in v9: >>>> - Fix module autoloading >>> >>> While the autoloading of bcm2835-audio, bcm2835-camera is fixed as >>> part of this series, there is one WARN coming in when bcm2835-audio >>> is loaded regarding dma_alloc_attr >>> >>> dmesg output: https://paste.debian.net/plain/1286359 >> >> is it possible that after your patch series no DMA mask like >> DMA_BIT_MASK(32) is provided? > > I am trying to set DMA_BIT_MASK(32) via dma_set_mask_and_coherent() but > it fails with -EIO what happens if you assign DMA_BIT_MASK(32) to device->dev.dma_mask within vchiq_device_register()? >> >>> >>> I am investigating further... >>>> - Implement bus_type's probe() callback to load drivers >>>> - Implement bus_type's uevent() to make sure appropriate drivers are >>>> loaded when device are registed from vchiq. >>>> >>>> Changes in v8: >>>> - Drop dual licensing. Instead use GPL-2.0 only for patch 1/5 >>>> >>>> Changes in v7: >>>> (5 out of 6 patches from v6 merged) >>>> - Split the main patch (6/6) as requested. >>>> - Use struct vchiq_device * instead of struct device * in >>>> all bus functions. >>>> - Drop additional name attribute displayed in sysfs (redundant info) >>>> - Document vchiq_interface doesn't enumerate device discovery >>>> - remove EXPORT_SYMBOL_GPL(vchiq_bus_type) >>>> >>>> Changes in v6: >>>> - Split struct device and struct driver wrappers in vchiq_device.[ch] >>>> - Move vchiq_bus_type definition to vchiq_device.[ch] as well >>>> - return error on bus_register() failure >>>> - drop dma_set_mask_and_coherent >>>> - trivial variable name change >>>> >>>> Changes in v5: >>>> - Fixup missing "staging: " in commits' subject line >>>> - No code changes from v4 >>>> >>>> Changes in v4: >>>> - Introduce patches to drop include directives from Makefile >>>> >>>> Changes in v3: >>>> - Rework entirely to replace platform devices/driver model >>>> >>>> -v2: >>>> https://lore.kernel.org/all/20221222191500.515795-1-umang.jain@ideasonboard.com/ >>>> >>>> -v1: >>>> https://lore.kernel.org/all/20221220084404.19280-1-umang.jain@ideasonboard.com/ >>>> >>>> Umang Jain (5): >>>> staging: vc04_services: vchiq_arm: Add new bus type and device type >>>> staging: vc04_services: vchiq_arm: Register vchiq_bus_type >>>> staging: bcm2835-camera: Register bcm2835-camera with vchiq_bus_type >>>> staging: bcm2835-audio: Register bcm2835-audio with vchiq_bus_type >>>> staging: vc04_services: vchiq_arm: Remove vchiq_register_child() >>>> >>>> drivers/staging/vc04_services/Makefile | 1 + >>>> .../vc04_services/bcm2835-audio/bcm2835.c | 20 ++-- >>>> .../bcm2835-camera/bcm2835-camera.c | 17 +-- >>>> .../interface/vchiq_arm/vchiq_arm.c | 48 ++++----- >>>> .../interface/vchiq_arm/vchiq_device.c | 102 >>>> ++++++++++++++++++ >>>> .../interface/vchiq_arm/vchiq_device.h | 54 ++++++++++ >>>> 6 files changed, 196 insertions(+), 46 deletions(-) >>>> 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 >>>> >>> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel