Hi Jett
On Tue, Apr 02, 2019 at 09:48:12AM -0600, Jett Rink wrote:
> On Sun, Mar 31, 2019 at 1:25 PM Rushikesh S Kadam
> <rushikesh.s.ka...@intel.com> wrote:
> > +
> > +/*
> > + * The Read-Write Semaphore is used to prevent message TX or RX while
> > + * the ishtp client is being initialized or undergoing reset.
> > + *
> > + * The readers are the kernel function calls responsibile for IA->ISH
> nit: s/responsibile/responsible
Thanks
> > +/**
> > + * ish_send() - Send message from host to firmware
> > + * @client_data: Client data instance
> > + * @out_msg: Message buffer to be sent to firmware
> > + * @out_size: Size of out going message
> > + * @in_msg: Message buffer where the incoming data copied.
> > + * This buffer is allocated by calling
> > + * @in_size: Max size of incoming message
> > + *
> > + * Return: Number of bytes copied in the in_msg on success, negative
> > + * error code on failure.
> > + */
> > +static int ish_send(struct ishtp_cl_data *client_data,
> > + u8 *out_msg, size_t out_size,
> > + u8 *in_msg, size_t in_size)
> > +{
> > + int rv;
> > + struct header *out_hdr = (struct header *)out_msg;
> > + struct ishtp_cl *cros_ish_cl = client_data->cros_ish_cl;
> > +
> > + dev_dbg(cl_data_to_dev(client_data),
> > + "%s: channel=%02u status=%02u\n",
> > + __func__, out_hdr->channel, out_hdr->status);
> > +
> > + /* Setup for in-coming response */
> nit:s/in-coming/incoming/
Thanks
> > +/**
> > + * ish_event_cb() - bus driver callback for incoming
> > + * message
> > + * @cl_device: ISHTP client device for which this message is
> > + * targeted.
> > + *
> > + * Remove the packet from the list and process the message by calling
> > + * process_recv.
> > + */
> > +static void ish_event_cb(struct ishtp_cl_device *cl_device)
> > +{
> > + struct ishtp_cl_rb *rb_in_proc;
> > + struct ishtp_cl_data *client_data;
> > + struct ishtp_cl *cros_ish_cl = ishtp_get_drvdata(cl_device);
> > +
> > + client_data = ishtp_get_client_data(cros_ish_cl);
> > +
> > + while ((rb_in_proc = ishtp_cl_rx_get_rb(cros_ish_cl)) != NULL) {
> > + /* Decide what to do with received data */
> > + process_recv(cros_ish_cl, rb_in_proc);
> > + }
> > +}
> > +
> > +/**
> > + * cros_ish_init() - Init function for ISHTP client
> > + * @cros_ish_cl: ISHTP client instance
> > + * @reset: true if called for init after reset
> > + *
> > + * This function complete the initializtion of the client.
> > + *
> > + * Return: 0 on success, non zero on error
> > + */
> > +static int cros_ish_init(struct ishtp_cl *cros_ish_cl, int reset)
> > +{
> > + int rv;
> > + struct ishtp_device *dev;
> > + struct ishtp_fw_client *fw_client;
> > + struct ishtp_cl_data *client_data =
> > + ishtp_get_client_data(cros_ish_cl);
> > +
> > + dev_dbg(cl_data_to_dev(client_data), "reset flag: %d\n", reset);
> Why are we passing in the reset flag here, we are only using it for a
> debug message; it
> doesn't seem worth passing reset as a parameter.
Yes, will drop the reset flag.
>
> > +late_initcall(cros_ec_ishtp_mod_init);
> > +module_exit(cros_ec_ishtp_mod_exit);
> > +
> > +MODULE_DESCRIPTION("ChromeOS EC ISHTP Client Driver");
> > +MODULE_AUTHOR("Rushikesh S Kadam <rushikesh.s.ka...@intel.com>");
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("ishtp:*");
> > --
> > 1.9.1
> >
>
> Besides the minor comments, this looks good to me, and I will add
> review and tested tags on subsequent patch.
I'll send v2 shortly
Regards
Rushikesh
--
