lkml.org 
[lkml]   [2017]   [Nov]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 2/5] soc: qcom: Introduce QMI helpers
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
\
 
 \ /
  Last update: 2017-11-17 17:18    [W:0.055 / U:2.688 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site