Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
To: Chunfeng Yun <chunfeng.yun@mediatek.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:37:30 +0300
Message-ID: <20190814073730.GA4140@kuha.fi.intel.com> (raw)
In-Reply-To: <1565749034.7317.6.camel@mhfsdcap03>

On Wed, Aug 14, 2019 at 10:17:14AM +0800, Chunfeng Yun wrote:
> 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?

Yes it does. That only creates the root directory "fusb302". For every
instance of fusb302 on the system that is registered and probed by the
driver, you will still have a separate file added to that directory,
just like before.

The only difference is that now we don't wait for the first instance
of fusb302 to be registered before we create that "fusb302" directory.
Instead, the directory is simply created the moment the driver is
loaded. On a system that does not have fusb302 controllers, the
directory will now stay empty, but that is not a problem.


thanks,

-- 
heikki

  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
2019-08-14  7:37     ` Heikki Krogerus [this message]
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=20190814073730.GA4140@kuha.fi.intel.com \
    --to=heikki.krogerus@linux.intel.com \
    --cc=chunfeng.yun@mediatek.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hdegoede@redhat.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
	public-inbox-index linux-usb

Example config snippet for mirrors

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.git