Hi Dafna,
Thanks for working on this.
On 11/5/19 1:20 PM, Dafna Hirschfeld wrote:
> Use a bitmask to get the next available device index
> to be used for the bus_info field in the v4l2_capability
> struct in the VIDIOC_QUERYCAP ioctl
>
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
> ---
> drivers/media/platform/vimc/vimc-capture.c | 8 +++++--
> drivers/media/platform/vimc/vimc-common.h | 2 ++
> drivers/media/platform/vimc/vimc-core.c | 25 +++++++++++++++++++++-
> 3 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/platform/vimc/vimc-capture.c b/drivers/media/platform/vimc/vimc-capture.c
> index 72577b85619d..592ce72e820d 100644
> --- a/drivers/media/platform/vimc/vimc-capture.c
> +++ b/drivers/media/platform/vimc/vimc-capture.c
> @@ -31,6 +31,7 @@ struct vimc_cap_device {
> u32 sequence;
> struct vimc_stream stream;
> struct media_pad pad;
> + char bus_info[32];
> };
>
> static const struct v4l2_pix_format fmt_default = {
> @@ -55,10 +56,12 @@ struct vimc_cap_buffer {
> static int vimc_cap_querycap(struct file *file, void *priv,
> struct v4l2_capability *cap)
> {
> + struct vimc_cap_device *vcap = video_drvdata(file);
> +
> strscpy(cap->driver, VIMC_PDEV_NAME, sizeof(cap->driver));
> strscpy(cap->card, KBUILD_MODNAME, sizeof(cap->card));
> - snprintf(cap->bus_info, sizeof(cap->bus_info),
> - "platform:%s", VIMC_PDEV_NAME);
> + strscpy(cap->bus_info, vcap->bus_info, sizeof(cap->bus_info));
> +
>
> return 0;
> }
> @@ -459,6 +462,7 @@ struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
> vcap->ved.process_frame = vimc_cap_process_frame;
> vcap->ved.vdev_get_format = vimc_cap_get_format;
> vcap->ved.dev = vimc->mdev.dev;
> + strscpy(vcap->bus_info, vimc->mdev.bus_info, sizeof(vcap->bus_info));
>
> /* Initialize the video_device struct */
> vdev = &vcap->vdev;
> diff --git a/drivers/media/platform/vimc/vimc-common.h b/drivers/media/platform/vimc/vimc-common.h
> index 17576d60a99c..6347135c504a 100644
> --- a/drivers/media/platform/vimc/vimc-common.h
> +++ b/drivers/media/platform/vimc/vimc-common.h
> @@ -166,12 +166,14 @@ struct vimc_ent_device {
> * struct vimc_device - main device for vimc driver
> *
> * @mdev the associated media_device parent
> + * @idx the index of the device, used for info_bus
> * @v4l2_dev Internal v4l2 parent device
> * @ents list of vimc_ent_device objects
> */
> struct vimc_device {
> struct media_device mdev;
> struct v4l2_device v4l2_dev;
> + unsigned int idx;
>
> struct list_head ents;
> };
> diff --git a/drivers/media/platform/vimc/vimc-core.c b/drivers/media/platform/vimc/vimc-core.c
> index c8612bfd8a2c..c1b360970a49 100644
> --- a/drivers/media/platform/vimc/vimc-core.c
> +++ b/drivers/media/platform/vimc/vimc-core.c
> @@ -14,9 +14,15 @@
> #include "vimc-common.h"
>
> #define VIMC_MDEV_MODEL_NAME "VIMC MDEV"
> +#define VIMC_MAX_DEVS 64
>
> #include "vimc-configfs.h"
>
> +
> +
> +static DECLARE_BITMAP(used, VIMC_MAX_DEVS);
> +static DEFINE_MUTEX(vimc_dev_idx_lock);
> +
> static const struct vimc_ent_type ent_types[] = {
> {
> .name = VIMC_SEN_NAME,
> @@ -237,9 +243,20 @@ static int vimc_probe(struct platform_device *pdev)
> pdev->dev.platform_data;
> struct vimc_device *vimc;
> int ret;
> + unsigned int idx;
>
> dev_dbg(&pdev->dev, "probe\n");
>
> + mutex_lock(&vimc_dev_idx_lock);
> + idx = find_first_zero_bit(used, VIMC_MAX_DEVS);
> + if (idx == VIMC_MAX_DEVS) {
> + mutex_unlock(&vimc_dev_idx_lock);
> + dev_err(&pdev->dev, "there are already %u devs which is the max allowed\n",
> + VIMC_MAX_DEVS);
> + return -EBUSY;
> + }
> + set_bit(idx, used);
> + mutex_unlock(&vimc_dev_idx_lock);
> vimc = devm_kzalloc(&pdev->dev, sizeof(*vimc),
> GFP_KERNEL);
> if (!vimc)
> @@ -247,6 +264,7 @@ static int vimc_probe(struct platform_device *pdev)
>
> memset(&vimc->mdev, 0, sizeof(vimc->mdev));
> INIT_LIST_HEAD(&vimc->ents);
> + vimc->idx = idx;
>
> /* Link the media device within the v4l2_device */
> vimc->v4l2_dev.mdev = &vimc->mdev;
> @@ -255,7 +273,8 @@ static int vimc_probe(struct platform_device *pdev)
> strscpy(vimc->mdev.model, VIMC_MDEV_MODEL_NAME,
> sizeof(vimc->mdev.model));
> snprintf(vimc->mdev.bus_info, sizeof(vimc->mdev.bus_info),
> - "platform:%s", VIMC_PDEV_NAME);
> + "platform:%s-%03u", VIMC_PDEV_NAME, idx);
> +
I was wondering that instead of using an index number here, you could just use
the name of the device the user created.
For instance:
$ mkdir /configfs/vimc/mdev
then bus_info would be "platform:vimc-mdev"
I also think that the model name vimc->mdev.model should be the name the user
created, and we can get rid of the VIMC_MDEV_MODEL_NAME macro.
You can pass the name to vimc_probe using the platform_data.
In this way you don't need to keep an index and it should simplify your code.
Regards,
Helen
> vimc->mdev.dev = &pdev->dev;
> media_device_init(&vimc->mdev);
>
> @@ -269,6 +288,7 @@ static int vimc_probe(struct platform_device *pdev)
> static int vimc_remove(struct platform_device *pdev)
> {
> struct vimc_device *vimc = platform_get_drvdata(pdev);
> + unsigned long idx = vimc->idx;
>
> dev_dbg(&pdev->dev, "remove\n");
>
> @@ -277,6 +297,9 @@ static int vimc_remove(struct platform_device *pdev)
> media_device_cleanup(&vimc->mdev);
> v4l2_device_unregister(&vimc->v4l2_dev);
>
> + mutex_lock(&vimc_dev_idx_lock);
> + clear_bit(idx, used);
> + mutex_unlock(&vimc_dev_idx_lock);
> return 0;
> }
>
>
