Re: [PATCH v3 2/5] soc: qcom: Introduce QMI helpers

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Thu 16 Nov 04:11 PST 2017, Srinivas Kandagatla wrote:
> On 15/11/17 20:10, Bjorn Andersson wrote:
[..]
> > diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> > index 91b70b170a82..9718f1c41e3d 100644
> > --- a/drivers/soc/qcom/Kconfig
> > +++ b/drivers/soc/qcom/Kconfig
> > @@ -37,6 +37,7 @@ config QCOM_PM
> >   config QCOM_QMI_HELPERS
> >   	tristate
> > +	depends on ARCH_QCOM
> 
> This bit is confusing!!, first patch added this symbol and this patch added
> the depends. either we move this to first patch or move out the
> QCOM_QMI_HELPERS from first patch and include in this patch
> 

Ohh, that's not right. The depends should be in the same patch.

And we don't really depends on ARCH_QCOM here, but without it Kconfig
doesn't know that make won't enter drivers/soc/qcom so we will end up
with a link error. I'm hoping we can fix this issue, to get some more
compile testing of these things.

> >   	help
> >   	  Helper library for handling QMI encoded messages.  QMI encoded
> >   	  messages are used in communication between the majority of QRTR
> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
[..]
> >   obj-$(CONFIG_QCOM_QMI_HELPERS)	+= qmi_helpers.o
> >   qmi_helpers-y	+= qmi_encdec.o
> > +qmi_helpers-y	+= qmi_interface.o
> for better reading.. i would suggest..
> qmi_helpers-y	+= qmi_encdec.o qmi_interface.o
> 

Sounds reasonable.

> 
> >   obj-$(CONFIG_QCOM_SMD_RPM)	+= smd-rpm.o
> >   obj-$(CONFIG_QCOM_SMEM) +=	smem.o
> >   obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> > diff --git a/drivers/soc/qcom/qmi_interface.c b/drivers/soc/qcom/qmi_interface.c
[..]
> > +#include <linux/platform_device.h>
> 
> Do we need this?
> 

I don't think so.

[..]
> > +/**
> > + * qmi_recv_new_server() - handler of NEW_SERVER control message
> > + * @qmi:	qmi handle
> > + * @node:	node of the new server
> > + * @port:	port of the new server
> > + *
> service and instance is not documented here.
> 

Thanks for noticing, will fix these.

[..]
> > +/**
> > + * qmi_handle_init() - initialize a QMI client handle
> > + * @qmi:	QMI handle to initialize
> > + * @max_msg_len: maximum size of incoming message
> do we need this??
> 

We need a buffer into which we can receive incoming packets, so either
we allocate a large enough buffer up front or we have to ask qrtr before
each packet how much space we will need.

I think largest possible buffer is 64kB, but typically we need much
less. And the IDL compiler seems to output this constant, so it seems
reasonable to pass it here.

> 
> > + * @handlers:	NULL-terminated list of QMI message handlers
> > + *
> recv_buf_size and ops not documented
> 

Looks like I've renamed max_msg_len to recv_buf_size, and forgot to add
ops. Will fix.

[..]
> > +
> > +/**
> > + * qmi_send_indication() - send an indication QMI message
> > + * @qmi:	QMI client handle
> > + * @sq:		destination sockaddr
> > + * @txn:	transaction object to use for the message
> 
> txn is not required here?
> 

No. Indications are fire-and-forget messages, but still need a
transaction id, so it's confusing for the client to have to create a
txn, use it and then cancel it (or to add another txn operation for
this). Therefor I'm hiding the txn handling inside this function.

Will fix the kerneldoc.

[..]
> > diff --git a/include/linux/soc/qcom/qmi.h b/include/linux/soc/qcom/qmi.h
> > index 5df7edfc6bfd..9b4f4fa5bed6 100644
> > --- a/include/linux/soc/qcom/qmi.h
> > +++ b/include/linux/soc/qcom/qmi.h
> 
> Looks like this patch added few things like list, wq and so to this header
> file, should we not add headers for those ??
> 

Will review these.

Thanks for the review!

Regards,
Bjorn