Messages in this thread | | | From | Bartosz Golaszewski <> | Date | Tue, 19 Oct 2021 16:16:10 +0200 | Subject | Re: [PATCH v7 5/8] gpio: sim: new testing module |
| |
On Mon, Oct 18, 2021 at 12:40 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Fri, Oct 08, 2021 at 10:17:36AM +0200, Bartosz Golaszewski wrote: > > Implement a new, modern GPIO testing module controlled by configfs > > attributes instead of module parameters. The goal of this driver is > > to provide a replacement for gpio-mockup that will be easily extensible > > with new features and doesn't require reloading the module to change > > the setup. > > > > Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl> > > [Andy: Initialize attribute allocated on the heap] > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > [Colin: Fix dereference of free'd pointer config] > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Some nit-picks below, up to you to address. > > ... > > > + ret = gpio_sim_setup_sysfs(chip); > > + if (ret) > > + return ret; > > + > > + return 0; > > return gpio_sim_...(chip); ? >
Sure, can do.
> ... > > > + > > Redundant empty line. > > > +CONFIGFS_ATTR_RO(gpio_sim_config_, dev_name); > > ... > > > + > > Ditto. > > > +CONFIGFS_ATTR_RO(gpio_sim_config_, chip_name); > > ... > > > + > > Ditto. > > > +CONFIGFS_ATTR(gpio_sim_config_, label); > > ... > > > + > > Ditto. > > > +CONFIGFS_ATTR(gpio_sim_config_, num_lines); > > ... > > > + > > Ditto. > > > +CONFIGFS_ATTR(gpio_sim_config_, line_names); >
These are on purpose - there's the store and show function and putting it next to only one is misleading IMO.
> ... > > > + fwnode = fwnode_create_software_node(properties, NULL); > > + if (IS_ERR(fwnode)) > > + return PTR_ERR(fwnode); > > > > + fwnode = dev_fwnode(&config->pdev->dev); > > + platform_device_unregister(config->pdev); > > + fwnode_remove_software_node(fwnode); > > This seems correct, thank you for modifying the code. > > ... > > > + config->pdev = NULL; > > + mutex_unlock(&config->lock); > > mutex_destroy() ? > Or is it done in the upper level? >
It's done in the release callback.
Bart
> -- > With Best Regards, > Andy Shevchenko > >
| |