Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Chunfeng Yun <chunfeng.yun@mediatek.com>
To: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Hans de Goede <hdegoede@redhat.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Guenter Roeck <linux@roeck-us.net>, <linux-usb@vger.kernel.org>
Subject: Re: [PATCH] usb: typec: fusb302: Call fusb302_debugfs_init earlier
Date: Wed, 14 Aug 2019 10:17:14 +0800
Message-ID: <1565749034.7317.6.camel@mhfsdcap03> (raw)
In-Reply-To: <20190813105216.GD4691@kuha.fi.intel.com>

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@redhat.com>
> > ---
> >  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@gmail.com>");
>  MODULE_DESCRIPTION("Fairchild FUSB302 Type-C Chip Driver");
> 
> 
> If it's OK, could you include that into this patch?
> 
> thanks,
> 



  parent reply index

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-13 10:15 Hans de Goede
2019-08-13 10:52 ` Heikki Krogerus
2019-08-13 12:13   ` Hans de Goede
2019-08-13 13:22     ` Heikki Krogerus
2019-08-13 13:34   ` Guenter Roeck
2019-08-14  2:17   ` Chunfeng Yun [this message]
2019-08-14  7:37     ` Heikki Krogerus
2019-08-15 12:54   ` Greg Kroah-Hartman
2019-08-15 13:31     ` Heikki Krogerus
2019-08-15 15:20       ` Guenter Roeck
2019-08-15 15:24         ` Hans de Goede

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1565749034.7317.6.camel@mhfsdcap03 \
    --to=chunfeng.yun@mediatek.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hdegoede@redhat.com \
    --cc=heikki.krogerus@linux.intel.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=linux@roeck-us.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox