From: Johan Hovold <johan@kernel.org>
To: Lars Poeschel <poeschel@lemonage.de>
Cc: Johan Hovold <johan@kernel.org>,
devicetree@vger.kernel.org, Samuel Ortiz <sameo@linux.intel.com>,
open list <linux-kernel@vger.kernel.org>,
"open list:NFC SUBSYSTEM" <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH v4 4/6] nfc: pn533: add UART phy driver
Date: Wed, 5 Dec 2018 16:29:52 +0100 [thread overview]
Message-ID: <20181205152952.GK15689@localhost> (raw)
In-Reply-To: <20181203142622.GA8057@lem-wkst-02.lemonage>
On Mon, Dec 03, 2018 at 03:26:22PM +0100, Lars Poeschel wrote:
> On Wed, Nov 14, 2018 at 04:35:17PM +0100, Johan Hovold wrote:
> > On Thu, Nov 01, 2018 at 11:02:12AM +0100, Lars Poeschel wrote:
> > > This adds the UART phy interface for the pn533 driver.
> > > The pn533 driver can be used through UART interface this way.
> > > It is implemented as a serdev device.
> > >
> > > Signed-off-by: Lars Poeschel <poeschel@lemonage.de>
> >
> > Please make sure to include reviewers on CC.
>
> It's hard to do all things right, about how to correctly email patches,
> patch sets and follow ups and send what to whom.
> I am still learning. Sorry about that.
No problem, I fully understand that.
> > > + err = serdev_device_open(serdev);
> > > + if (err) {
> > > + dev_err(&serdev->dev, "Unable to open device\n");
> > > + goto err_skb;
> > > + }
> > > +
> > > + err = serdev_device_set_baudrate(serdev, 115200);
> > > + if (err != 115200) {
> > > + err = -EINVAL;
> > > + goto err_serdev;
> > > + }
> > > +
> > > + serdev_device_set_flow_control(serdev, false);
> > > + pn532->send_wakeup = PN532_SEND_WAKEUP;
> > > + timer_setup(&pn532->cmd_timeout, pn532_cmd_timeout, 0);
> > > + priv = pn533_register_device(PN533_DEVICE_PN532,
> > > + PN533_NO_TYPE_B_PROTOCOLS,
> > > + PN533_PROTO_REQ_ACK_RESP,
> > > + pn532, &uart_phy_ops, NULL,
> > > + &pn532->serdev->dev,
> > > + &serdev->dev);
> > > + if (IS_ERR(priv)) {
> > > + err = PTR_ERR(priv);
> > > + goto err_serdev;
> > > + }
> > > +
> > > + pn532->priv = priv;
> > > + err = pn533_finalize_setup(pn532->priv);
> > > + if (err)
> > > + goto err_unregister;
> > > +
> > > + serdev_device_close(serdev);
> >
> > This looks broken; what if the NFC interface is brought up before this
> > point? You'd get a double open, which is likely to crash things, but
> > even if you survive that, the port would not be closed despite the
> > interface being up.
>
> I understand the problem and would like to solve it with a mutex. I will
> not have time to work on that until next year. Please be patient. I will
> send a new patchset.
I'm in no hurry here. :)
But I still think doing that setup before registering the device might
be preferred if it's possible as you wouldn't need a mutex then.
> > Can't you finalise your setup before registering the interface?
>
> Well, propably I can do that. But I did it the way the other drivers
> (usb and i2c) are already doing and reusing the code of the pn533 core
> driver. Since their probe works very similar to mine, I suspect them to
> have the same problems.
Quite possibly.
> I can rewrite the probe for my driver, but not for the other two. I can
> not test them.
> Would you prefer that I rewrite my own _register_device and
> _finalize_setup functions, not using the ones from the core driver ?
If you add common paths that you can test using your driver I think it
should be fine to convert the others unless it ends up being really
complicated. Perhaps the authors of those driver can help out with
testing too.
> Thank you for your very valuable feedback.
You're welcome.
Johan
next prev parent reply other threads:[~2018-12-05 15:29 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-01 10:02 [PATCH v4 1/6] nfc: pn533: i2c: "pn532" as dt compatible string Lars Poeschel
2018-11-01 10:02 ` [PATCH v4 3/6] nfc: pn533: Add dev_up/dev_down hooks to phy_ops Lars Poeschel
2018-11-01 10:02 ` [PATCH v4 4/6] nfc: pn533: add UART phy driver Lars Poeschel
2018-11-14 15:35 ` Johan Hovold
2018-12-03 14:26 ` Lars Poeschel
2018-12-05 15:29 ` Johan Hovold [this message]
2018-11-01 10:02 ` [PATCH v4 5/6] nfc: pn533: Add autopoll capability Lars Poeschel
2018-11-01 10:02 ` [PATCH v4 6/6] nfc: pn532_uart: Make use of pn532 autopoll Lars Poeschel
Reply instructions:
You may reply publicly 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=20181205152952.GK15689@localhost \
--to=johan@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=poeschel@lemonage.de \
--cc=sameo@linux.intel.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).