On Wed, Jun 5, 2024 at 8:52 AM Ratheesh Kannoth <rkannoth@xxxxxxxxxxx> wrote: > > On 2024-05-29 at 00:33:18, Bartosz Golaszewski (brgl@xxxxxxxx) wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx> > > > > Implement the power sequencing subsystem allowing devices to share > > complex powering-up and down procedures. It's split into the consumer > > and provider parts but does not implement any new DT bindings so that > > the actual power sequencing is never revealed in the DT representation. > > > > Tested-by: Amit Pundir <amit.pundir@xxxxxxxxxx> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx> > > --- > > + > > +static struct pwrseq_unit_dep *pwrseq_unit_dep_new(struct pwrseq_unit *unit) > nit. pwrseq_unit_dep_alloc/create rhymes well with pwrseq_unit_dep_free(), So what? > > +static void pwrseq_unit_free_deps(struct list_head *list) > > +{ > > + struct pwrseq_unit_dep *dep, *next; > > + > > + list_for_each_entry_safe(dep, next, list, list) { > no need of 'locks' to protect against simutaneous 'add' ? No, this only happens once during release. > > + > > +static int pwrseq_unit_setup_deps(const struct pwrseq_unit_data **data, > > + struct list_head *dep_list, > > + struct list_head *unit_list, > > + struct radix_tree_root *processed_units) > > +{ > > + const struct pwrseq_unit_data *pos; > > + struct pwrseq_unit_dep *dep; > > + struct pwrseq_unit *unit; > > + int i; > > + > > + for (i = 0; data[i]; i++) { > Can we add range for i ? just depending on data[i] to be zero looks to be risky. > Why? It's perfectly normal to expect users to end the array with a NULL pointer. The docs say these arrays must be NULL-terminated. > > + pos = data[i]; > > + > > + unit = pwrseq_unit_setup(pos, unit_list, processed_units); > > + if (IS_ERR(unit)) > > + return PTR_ERR(unit); > > + > > + dep = pwrseq_unit_dep_new(unit); > > + if (!dep) { > > + pwrseq_unit_decref(unit); > This frees only one 'unit'. is there any chance for multiple 'unit', then better clean > up here ? The references to those will be dropped in pwrseq_release(). > > + > > + /* > > + * From this point onwards the device's release() callback is > > + * responsible for freeing resources. > > + */ > > + device_initialize(&pwrseq->dev); > > + > > + ret = dev_set_name(&pwrseq->dev, "pwrseq.%d", pwrseq->id); > > + if (ret) > > + goto err_put_pwrseq; > > + > > + pwrseq->owner = config->owner ?: THIS_MODULE; > > + pwrseq->match = config->match; > > + > > + init_rwsem(&pwrseq->rw_lock); > > + mutex_init(&pwrseq->state_lock); > > + INIT_LIST_HEAD(&pwrseq->targets); > > + INIT_LIST_HEAD(&pwrseq->units); > > + > > + ret = pwrseq_setup_targets(config->targets, pwrseq); > > + if (ret) > > + goto err_put_pwrseq; > > + > > + scoped_guard(rwsem_write, &pwrseq_sem) { > > + ret = device_add(&pwrseq->dev); > > + if (ret) > > + goto err_put_pwrseq; > > + } > > + > > + return pwrseq; > > + > > +err_put_pwrseq: > no need to kfree(pwrseq) ? It's literally put on the next line? Bart