Re: [PATCH] platform: chrome: Add ChromeOS EC ISHTP driver

[Rushikesh S Kadam <rushikesh.s.kadam@intel.com>]

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.kadam@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.kadam@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

--