On Thu, Mar 28, 2019 at 02:17:38PM +0100, Christian Gromm wrote:
> This patch makes the driver accept a link confiiguration eventhough no
> device is attached to the bus. Instead the configuration is being applied
> as soon as a device is being registered with the core.
>
> Signed-off-by: Christian Gromm <christian.gromm@xxxxxxxxxxxxx>
> ---
> v2:
> - follow-up adaptions due to changes introduced w/ [PATCH v2 01/14]
>
> drivers/staging/most/configfs.c | 60 ++++++++++++++++++++++++++++----------
> drivers/staging/most/core.c | 16 +++-------
> drivers/staging/most/core.h | 1 +
> drivers/staging/most/sound/sound.c | 6 ++--
> 4 files changed, 53 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/staging/most/configfs.c b/drivers/staging/most/configfs.c
> index 6968b299..3a7c54d 100644
> --- a/drivers/staging/most/configfs.c
> +++ b/drivers/staging/most/configfs.c
> @@ -14,6 +14,7 @@
>
> struct mdev_link {
> struct config_item item;
> + struct list_head list;
> bool create;
> u16 num_buffers;
> u16 buffer_size;
> @@ -29,6 +30,8 @@ struct mdev_link {
> char param[PAGE_SIZE];
> };
>
> +struct list_head mdev_link_list;
> +
> static int set_cfg_buffer_size(struct mdev_link *link)
> {
> if (!link->buffer_size)
> @@ -105,33 +108,41 @@ static ssize_t mdev_link_create_show(struct config_item *item, char *page)
> return snprintf(page, PAGE_SIZE, "%d\n", to_mdev_link(item)->create);
> }
>
> +static int set_config_and_add_link(struct mdev_link *mdev_link)
> +{
> + int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(set_config_val); i++) {
> + ret = set_config_val[i](mdev_link);
> + if (ret == -ENODATA) {
I've read the commit description but I don't really understand the
error codes. Here only -ENODATA is an error. But later -ENODEV
is success. Why not "if (ret) {" here?
> + pr_err("Config failed\n");
> + return ret;
> + }
> + }
> +
> + return most_add_link(mdev_link->mdev, mdev_link->channel,
> + mdev_link->comp, mdev_link->name,
> + mdev_link->param);
> +}
> +
> static ssize_t mdev_link_create_store(struct config_item *item,
> const char *page, size_t count)
> {
> struct mdev_link *mdev_link = to_mdev_link(item);
> bool tmp;
> int ret;
> - int i;
>
> ret = kstrtobool(page, &tmp);
> if (ret)
> return ret;
> -
> - for (i = 0; i < ARRAY_SIZE(set_config_val); i++) {
> - ret = set_config_val[i](mdev_link);
> - if (ret < 0)
> - return ret;
> - }
> -
> - if (tmp)
> - ret = most_add_link(mdev_link->mdev, mdev_link->channel,
> - mdev_link->comp, mdev_link->name,
> - mdev_link->param);
> - else
> - ret = most_remove_link(mdev_link->mdev, mdev_link->channel,
> - mdev_link->comp);
> - if (ret)
> + if (!tmp)
> + return most_remove_link(mdev_link->mdev, mdev_link->channel,
> + mdev_link->comp);
> + ret = set_config_and_add_link(mdev_link);
> + if (ret && ret != -ENODEV)
ENODEV is success. I feel like this needs to be explained in comments
in the code.
> return ret;
> + list_add_tail(&mdev_link->list, &mdev_link_list);
> mdev_link->create = tmp;
> return count;
> }
> @@ -590,6 +601,22 @@ int most_register_configfs_subsys(struct core_component *c)
> }
> EXPORT_SYMBOL_GPL(most_register_configfs_subsys);
>
> +void most_interface_register_notify(const char *mdev)
> +{
> + bool register_snd_card = false;
> + struct mdev_link *mdev_link;
> +
> + list_for_each_entry(mdev_link, &mdev_link_list, list) {
> + if (!strcmp(mdev_link->mdev, mdev)) {
> + set_config_and_add_link(mdev_link);
Here the errors are not checked.
> + if (!strcmp(mdev_link->comp, "sound"))
> + register_snd_card = true;
> + }
> + }
> + if (register_snd_card)
> + most_cfg_complete("sound");
> +}
> +
[ snip ]
> diff --git a/drivers/staging/most/sound/sound.c b/drivers/staging/most/sound/sound.c
> index fd089e6..44c9146 100644
> --- a/drivers/staging/most/sound/sound.c
> +++ b/drivers/staging/most/sound/sound.c
> @@ -20,6 +20,7 @@
> #include <most/core.h>
>
> #define DRIVER_NAME "sound"
> +#define STRING_SIZE 80
>
> static struct core_component comp;
>
> @@ -582,6 +583,7 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
> int direction;
> u16 ch_num;
> char *sample_res;
> + char arg_list_cpy[STRING_SIZE];
>
> if (!iface)
> return -EINVAL;
> @@ -590,8 +592,8 @@ static int audio_probe_channel(struct most_interface *iface, int channel_id,
> pr_err("Incompatible channel type\n");
> return -EINVAL;
> }
> -
> - ret = split_arg_list(arg_list, &ch_num, &sample_res);
> + strlcpy(arg_list_cpy, arg_list, STRING_SIZE);
> + ret = split_arg_list(arg_list_cpy, &ch_num, &sample_res);
I don't understand why we need a copy of arg_list or how this relates
to the rest of the patch.
> if (ret < 0)
> return ret;
>
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
