On Tue, 2019-08-13 at 13:52 +0300, Heikki Krogerus wrote:
> Hi Hans,
>
> On Tue, Aug 13, 2019 at 12:15:24PM +0200, Hans de Goede wrote:
> > tcpm_register_port() will call some of the fusb302 code's callbacks
> > wich in turn will call fusb302_log(). So we need to call
> > fusb302_debugfs_init() before we call tcpm_register_port().
<...>
> >
> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > ---
> > drivers/usb/typec/tcpm/fusb302.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> > index ccfc7e91e7a3..04c76b9d0065 100644
> > --- a/drivers/usb/typec/tcpm/fusb302.c
> > +++ b/drivers/usb/typec/tcpm/fusb302.c
> > @@ -1759,6 +1759,7 @@ static int fusb302_probe(struct i2c_client *client,
> > INIT_WORK(&chip->irq_work, fusb302_irq_work);
> > INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work);
> > init_tcpc_dev(&chip->tcpc_dev);
> > + fusb302_debugfs_init(chip);
> >
> > if (client->irq) {
> > chip->gpio_int_n_irq = client->irq;
> > @@ -1784,7 +1785,6 @@ static int fusb302_probe(struct i2c_client *client,
> > goto tcpm_unregister_port;
> > }
> > enable_irq_wake(chip->gpio_int_n_irq);
> > - fusb302_debugfs_init(chip);
> > i2c_set_clientdata(client, chip);
>
> That leaves the rootdir variable pointing to something again for
> example if a failure happens (like -EPROBE_AGAIN) during probe (the
> "fusb302" directory is removed, but the rootdir static variable still
> points to something).
>
> Let's just create that rootdir directory during driver init. I don't
> really understand why should we only create it when/if the first
> instance of fusb302 is registered. I think something like this would
> work:
>
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index c524088246ee..7a950a6e5f0d 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -212,9 +212,6 @@ static struct dentry *rootdir;
> static void fusb302_debugfs_init(struct fusb302_chip *chip)
> {
> mutex_init(&chip->logbuffer_lock);
> - if (!rootdir)
> - rootdir = debugfs_create_dir("fusb302", NULL);
> -
> chip->dentry = debugfs_create_file(dev_name(chip->dev),
> S_IFREG | 0444, rootdir,
> chip, &fusb302_debug_fops);
> @@ -223,7 +220,6 @@ static void fusb302_debugfs_init(struct fusb302_chip *chip)
> static void fusb302_debugfs_exit(struct fusb302_chip *chip)
> {
> debugfs_remove(chip->dentry);
> - debugfs_remove(rootdir);
> }
>
> #else
> @@ -1863,7 +1859,24 @@ static struct i2c_driver fusb302_driver = {
> .remove = fusb302_remove,
> .id_table = fusb302_i2c_device_id,
> };
> -module_i2c_driver(fusb302_driver);
> +
> +static int __init fusb302_init(void)
> +{
> + rootdir = debugfs_create_dir("fusb302", NULL);
> + if (IS_ERR(rootdir))
> + return PTR_ERR(rootdir);
This doesn't support multi-instance?
> +
> + return i2c_add_driver(&fusb302_driver);
> +}
> +
> +static void __exit fusb302_exit(void)
> +{
> + i2c_del_driver(&fusb302_driver);
> + debugfs_remove(rootdir);
> +}
> +
> +module_init(fusb302_init);
> +module_exit(fusb302_exit);
>
> MODULE_AUTHOR("Yueyao Zhu <yueyao.zhu@xxxxxxxxx>");
> MODULE_DESCRIPTION("Fairchild FUSB302 Type-C Chip Driver");
>
>
> If it's OK, could you include that into this patch?
>
> thanks,
>
