On Do, 2019-03-28 at 17:12 +0300, Dan Carpenter wrote:
> External E-Mail
>
>
> 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?
>
The most_set_cfg_* functions return success, ENODEV or ENODATA.
We want to stop only in case there is something wrong with the
provided data. A missing device is not an issue here. In this
case we want to keep the configuration as is and complete stuff
once a device is being attached.
>
> >
> > + 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.
ENODEV is not a show-stopper. It only postpones the configuration
process until the driver is being notified about a new device.
>
> >
> > 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.
We are in the notify function. That means a device is present
now. And if the list isn't empty, there is also a valid configuration
available. So, set_config_and_add_link should not fail.
>
> >
> > + 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.
The function split_arg_list modifies the argument. So if we want to
read it back later in a show function, we need to have a clean copy.
>
> >
> > if (ret < 0)
> > return ret;
> >
> regards,
> dan carpenter
>
>
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
