Hi Dafna,
Thank you for the patch,
One more comment and few nitpicks follow.
With that addressed you can add
Reviewed-by: Andrzej Pietrasiewicz <andrzej.p@xxxxxxxxxxxxx>
W dniu 26.11.2019 o 13:10, Dafna Hirschfeld pisze:
Add the code that implements the usage of configfs in order
to create and configure a device topology from userspace.
The code is only added in this patch but is not used.
It will be used in next patch in the series.
<snip>
+struct vimc_cfs_ent_type vimc_cap_cfs_ent_type = {
+ .name = VIMC_CAP_NAME,
+ .create_pads = vimc_cap_create_cfs_pads,
+};
You have some instances of struct vimc_cfs_ent_type,
but at the same time you have an instance of struct
config_item_type called vimc_cfs_ent_type.
I haven't noticed it earlier, this is confusing.
<snip>
diff --git a/drivers/media/platform/vimc/vimc-configfs.c b/drivers/media/platform/vimc/vimc-configfs.c
new file mode 100644
index 000000000000..81e6be5b30c5
--- /dev/null
+++ b/drivers/media/platform/vimc/vimc-configfs.c
@@ -0,0 +1,720 @@
<snip>
+static struct vimc_cfs_subsystem {
+ struct configfs_subsystem subsys;
+ struct list_head ent_types;
+} vimc_cfs_subsys = {
+ .subsys = {
+ .su_group = {
+ .cg_item = {
+ .ci_namebuf = CFS_SUBSYS_NAME,
+ .ci_type = &vimc_cfs_subsys_type,
+ },
+ },
+ .su_mutex = __MUTEX_INITIALIZER(vimc_cfs_subsys.subsys.su_mutex),
A newline and tabs to fit 80 chars limit?
.su_mutex =
__MUTEX_INITIALIZER(vimc_cfs_subsys.subsys.su_mutex),
<snip>
+
+static struct config_group *vimc_cfs_dev_make_ent_group(
+ struct config_group *group, const char *name)
+{
+ struct vimc_cfs_device *cfs =
+ container_of(group, struct vimc_cfs_device, gdev);
+ char *type_name, *ent_name, *sep;
+ struct vimc_cfs_ent *c_ent;
+ struct vimc_entity_data *ent;
+ struct vimc_cfs_ent_type *c_ent_type = NULL;
+ struct vimc_cfs_ent_type *found_ent_type = NULL;
+ char buf[VIMC_MAX_CFS_NAME_LEN];
+
+ cg_dbg(group, "trying to make entity '%s'\n", name);
+ if (snprintf(buf, VIMC_MAX_CFS_NAME_LEN, "%s", name) >= sizeof(buf))
+ return ERR_PTR(-ENAMETOOLONG);
+
+ /* Parse format "type_name:ent_name" */
+ sep = strchr(buf, CHAR_SEPARATOR);
+ if (!sep) {
+ cg_warn(&cfs->gdev,
+ "Could not find separator '%c'\n", CHAR_SEPARATOR);
+ goto syntax_error;
+ }
+ *sep = '\0';
+
+ ent_name = &sep[1];
+ type_name = buf;
+
+ if (!*ent_name || sep == type_name) {
+ cg_warn(&cfs->gdev,
+ "%s: Driver name and entity name can't be empty.\n",
+ name);
+ goto syntax_error;
+ }
+ if (strlen(ent_name) >= VIMC_MAX_NAME_LEN) {
+ cg_err(&cfs->gdev,
+ "%s: Driver name length should be less than %u.\n",
+ name, VIMC_MAX_NAME_LEN);
+ goto syntax_error;
+ }
+ mutex_lock(&cfs->pdata.topology_mutex);
+ list_for_each_entry(ent, &cfs->pdata.ents, entry) {
+ if (!strncmp(ent->name, ent_name, sizeof(ent->name))) {
+ cg_err(&cfs->gdev, "entity `%s` already exist\n",
+ ent->name);
+ mutex_unlock(&cfs->pdata.topology_mutex);
+ goto syntax_error;
+ }
+ }
+
+ c_ent = kzalloc(sizeof(*c_ent), GFP_KERNEL);
+ if (!c_ent) {
+ mutex_unlock(&cfs->pdata.topology_mutex);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ strscpy(c_ent->ent.name, ent_name, sizeof(c_ent->ent.name));
+
+ list_for_each_entry(c_ent_type, &vimc_cfs_subsys.ent_types, entry) {
+ if (!strcmp(type_name, c_ent_type->name)) {
+ found_ent_type = c_ent_type;
+ break;
+ }
+ }
list_for_each_entry() ultimately expands to a "for" loop, and inside it
you only have a single statement (the "if") so the outer braces seem redundant.
