Re: [PATCH v6 6/7] nfc: pn533: Add autopoll capability

[Lars Poeschel <poeschel@lemonage.de>]

On Tue, Aug 20, 2019 at 02:23:44PM +0200, Johan Hovold wrote:
> On Tue, Aug 20, 2019 at 02:03:43PM +0200, Lars Poeschel wrote:

> >  drivers/nfc/pn533/pn533.c | 193 +++++++++++++++++++++++++++++++++++++-
> >  drivers/nfc/pn533/pn533.h |  10 +-
> >  2 files changed, 197 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/nfc/pn533/pn533.c b/drivers/nfc/pn533/pn533.c
> > index a8c756caa678..7e915239222b 100644
> > --- a/drivers/nfc/pn533/pn533.c
> > +++ b/drivers/nfc/pn533/pn533.c
> > @@ -185,6 +185,32 @@ struct pn533_cmd_jump_dep_response {
> >  	u8 gt[];
> >  } __packed;
> >  
> > +struct pn532_autopoll_resp {
> > +	u8 type;
> > +	u8 ln;
> > +	u8 tg;
> > +	u8 tgdata[];
> > +} __packed;
> 
> No need for __packed.

I did a quick test without the __packed and indeed: It worked. I'd
sworn that it is needed in this place, because this autopoll response is
data that the nfc chip puts on the serial wire without padding inbetween
and this struct is used to access this data and without the __packed the
compiler should be allowed to put padding bytes between the struct
fields. But it choose to not do it. I am still not shure, why this
happens, but ok. I can remove the __packed.

> > +static int pn533_autopoll_complete(struct pn533 *dev, void *arg,
> > +			       struct sk_buff *resp)
> > +{
> > +	u8 nbtg;
> > +	int rc;
> > +	struct pn532_autopoll_resp *apr;
> > +	struct nfc_target nfc_tgt;
> > +
> > +	if (IS_ERR(resp)) {
> > +		rc = PTR_ERR(resp);
> > +
> > +		nfc_err(dev->dev, "%s  autopoll complete error %d\n",
> > +			__func__, rc);
> > +
> > +		if (rc == -ENOENT) {
> > +			if (dev->poll_mod_count != 0)
> > +				return rc;
> > +			goto stop_poll;
> > +		} else if (rc < 0) {
> > +			nfc_err(dev->dev,
> > +				"Error %d when running autopoll\n", rc);
> > +			goto stop_poll;
> > +		}
> > +	}
> > +
> > +	nbtg = resp->data[0];
> > +	if ((nbtg > 2) || (nbtg <= 0))
> > +		return -EAGAIN;
> > +
> > +	apr = (struct pn532_autopoll_resp *)&resp->data[1];
> > +	while (nbtg--) {
> > +		memset(&nfc_tgt, 0, sizeof(struct nfc_target));
> > +		switch (apr->type) {
> > +		case PN532_AUTOPOLL_TYPE_ISOA:
> > +			dev_dbg(dev->dev, "ISOA");
> 
> You forgot the '\n' here and elsewhere (some nfc_err as well).

I can add them. I will wait a bit for more review comments before
posting a new patchset.

Thanks so far for your quick review,
Lars