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

[Enric Balletbo i Serra]

Hi,

On 12/4/19 17:22, Srinivas Pandruvada wrote:
> On Thu, 2019-04-11 at 15:54 +0200, Enric Balletbo i Serra wrote:
>> Hi,
>>
>> On 11/4/19 13:10, Rushikesh S Kadam wrote:
>>> Hi Enric, Srinivas
>>>
>>> On Thu, Apr 11, 2019 at 12:55:13PM +0200, Enric Balletbo i Serra
>>> wrote:
>>>> Hi,
>>>>
>>>> On 10/4/19 17:31, Jett Rink wrote:
>>>>> Reviewed-by: Jett Rink <jettrink@xxxxxxxxxxxx>
>>>>> Tested-by: Jett Rink <jettrink@xxxxxxxxxxxx>
>>>>>
>>>>>
>>>>> On Sun, Apr 7, 2019 at 6:10 AM Rushikesh S Kadam
>>>>> <rushikesh.s.kadam@xxxxxxxxx> wrote:
>>>>>>
>>>>>> This driver implements a slim layer to enable the ChromeOS
>>>>>> EC kernel stack (cros_ec) to communicate with ChromeOS EC
>>>>>> firmware running on the Intel Integrated Sensor Hub (ISH).
>>>>>>
>>>>>> The driver registers a ChromeOS EC MFD device to connect
>>>>>> with cros_ec kernel stack (upper layer), and it registers a
>>>>>> client with the ISH Transport Protocol bus (lower layer) to
>>>>>> talk with the ISH firwmare. See description of the ISHTP
>>>>>> protocol at Documentation/hid/intel-ish-hid.txt
>>>>>>
>>>>>> Signed-off-by: Rushikesh S Kadam <rushikesh.s.kadam@xxxxxxxxx
>>>>>>>
>>>>>> ---
>>>>>>
>>>>>> v3
>>>>>>  - Made several changes to improve code readability. Replaced
>>>>>>    multiple cl_data_to_dev(client_data) with dev variable.
>>>>>> Use
>>>>>>    reverse Xmas tree for variable defintion where it made
>>>>>> sense.
>>>>>>    Dropped few debug prints. Add docstring for function
>>>>>>    prepare_cros_ec_rx().
>>>>>>  - Fix code in function prepare_cros_ec_rx() under label
>>>>>>    end_cros_ec_dev_init_error.
>>>>>>  - Recycle buffer in process_recv() on failing to obtain the
>>>>>>    semaphore.
>>>>>>  - Increase ISHTP TX/RX ring buffer size to 8.
>>>>>>  - Alphabetically ordered CROS_EC_ISHTP entries in Makefile
>>>>>> and
>>>>>>    Kconfig.
>>>>>>  - Updated commit message.
>>>>>>
>>>>>> v2
>>>>>>  - Dropped unused "reset" parameter in function
>>>>>> cros_ec_init()
>>>>>>  - Change driver name to cros_ec_ishtp to be consistent with
>>>>>> other
>>>>>>    references in the code.
>>>>>>  - Fixed a few typos.
>>>>>>
>>>>>> v1
>>>>>>  - Initial version
>>>>>>
>>>>>>  drivers/platform/chrome/Kconfig         |  13 +
>>>>>>  drivers/platform/chrome/Makefile        |   1 +
>>>>>>  drivers/platform/chrome/cros_ec_ishtp.c | 765
>>>>>> ++++++++++++++++++++++++++++++++
>>>>>>  3 files changed, 779 insertions(+)
>>>>>>  create mode 100644 drivers/platform/chrome/cros_ec_ishtp.c
>>>>>>
>>>>>> diff --git a/drivers/platform/chrome/Kconfig
>>>>>> b/drivers/platform/chrome/Kconfig
>>>>>> index 16b1615..5848179 100644
>>>>>> --- a/drivers/platform/chrome/Kconfig
>>>>>> +++ b/drivers/platform/chrome/Kconfig
>>>>>> @@ -62,6 +62,19 @@ config CROS_EC_I2C
>>>>>>           a checksum. Failing accesses will be retried three
>>>>>> times to
>>>>>>           improve reliability.
>>>>>>
>>>>>> +config CROS_EC_ISHTP
>>>>>> +       tristate "ChromeOS Embedded Controller (ISHTP)"
>>>>>> +       depends on MFD_CROS_EC
>>>>>> +       depends on INTEL_ISH_HID
>>>>>> +       help
>>>>>> +         If you say Y here, you get support for talking to
>>>>>> the ChromeOS EC
>>>>>> +         firmware running on Intel Integrated Sensor Hub
>>>>>> (ISH), using the
>>>>>> +         ISH Transport protocol (ISH-TP). This uses a simple
>>>>>> byte-level
>>>>>> +         protocol with a checksum.
>>>>>> +
>>>>>> +         To compile this driver as a module, choose M here:
>>>>>> the
>>>>>> +         module will be called cros_ec_ishtp.
>>>>>> +
>>>>>>  config CROS_EC_SPI
>>>>>>         tristate "ChromeOS Embedded Controller (SPI)"
>>>>>>         depends on MFD_CROS_EC && SPI
>>>>>> diff --git a/drivers/platform/chrome/Makefile
>>>>>> b/drivers/platform/chrome/Makefile
>>>>>> index cd591bf..4efe102 100644
>>>>>> --- a/drivers/platform/chrome/Makefile
>>>>>> +++ b/drivers/platform/chrome/Makefile
>>>>>> @@ -7,6 +7,7 @@ cros_ec_ctl-objs                        :=
>>>>>> cros_ec_sysfs.o cros_ec_lightbar.o \
>>>>>>                                            cros_ec_vbc.o
>>>>>> cros_ec_debugfs.o
>>>>>>  obj-$(CONFIG_CROS_EC_CTL)              += cros_ec_ctl.o
>>>>>>  obj-$(CONFIG_CROS_EC_I2C)              += cros_ec_i2c.o
>>>>>> +obj-$(CONFIG_CROS_EC_ISHTP)            += cros_ec_ishtp.o
>>>>>>  obj-$(CONFIG_CROS_EC_SPI)              += cros_ec_spi.o
>>>>>>  cros_ec_lpcs-objs                      := cros_ec_lpc.o
>>>>>> cros_ec_lpc_reg.o
>>>>>>  cros_ec_lpcs-$(CONFIG_CROS_EC_LPC_MEC) += cros_ec_lpc_mec.o
>>>>>> diff --git a/drivers/platform/chrome/cros_ec_ishtp.c
>>>>>> b/drivers/platform/chrome/cros_ec_ishtp.c
>>>>>> new file mode 100644
>>>>>> index 0000000..b1d19c4
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/platform/chrome/cros_ec_ishtp.c
>>>>>> @@ -0,0 +1,765 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>>> +/*
>>>>>> + * ISHTP client driver for talking to the Chrome OS EC
>>>>>> firmware running
>>>>>> + * on Intel Integrated Sensor Hub (ISH) using the ISH
>>>>>> Transport protocol
>>>>>> + * (ISH-TP).
>>>>>> + *
>>>>>> + * Copyright (c) 2019, Intel Corporation.
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/delay.h>
>>>>>> +#include <linux/mfd/core.h>
>>>>>> +#include <linux/mfd/cros_ec.h>
>>>>>> +#include <linux/mfd/cros_ec_commands.h>
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/pci.h>
>>>>>> +#include <linux/intel-ish-client-if.h>
>>>>>> +
>>>>
>>>> I think that this patch depends on another patchset that's in
>>>> linux-next but
>>>> diddn't land yet to mainline. Do you know if the dependencies are
>>>> queued for
>>>> next merge window? Can you provide the exact patches that this
>>>> patch depends on?
>>>
>>> Enric, 
>>> Sorry I missed mentioning this. 
>>>  
>>> The patch have dependency on intel-ish-hid stack on hid git tree,
>>> branch for-5.2/ish
>>>
> https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-5.2/ish
>>>
>>> Srinivas, 
>>> Could you tell if the patches are queued for next merge window?
>>>
> Yes they are queued up for the next merge window. They are already in
> linux-next. One option is to check if Jiri pick this patch as part of
> his pull request. I am assuming that this patch doesn't depend on any
> other ChromeOS EC patches.
> 

Ok, in such case a v4 will be needed, just few style changes that was thinking
to change myself if had the patch had gone through the platform tree.

Thanks,
 Enric