Hi, On 9/2/21 2:07 PM, K Naduvalath, Sumesh wrote: > Thank you Randy for the review. Please find my comments inline - > >> -----Original Message----- >> From: Randy Dunlap <rdunlap@xxxxxxxxxxxxx> >> Sent: Thursday, September 2, 2021 1:02 AM >> To: K Naduvalath, Sumesh <sumesh.k.naduvalath@xxxxxxxxx>; >> hdegoede@xxxxxxxxxx; mgross@xxxxxxxxxxxxxxx; >> srinivas.pandruvada@xxxxxxxxxxxxxxx >> Cc: Pandruvada, Srinivas <srinivas.pandruvada@xxxxxxxxx>; platform-driver- >> x86@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Chinnu, Ganapathi >> <ganapathi.chinnu@xxxxxxxxx>; Kumar, Nachiketa >> <nachiketa.kumar@xxxxxxxxx> >> Subject: Re: [PATCH v3 1/1] ishtp: Add support for Intel ishtp eclite driver >> >> On 9/1/21 5:43 AM, sumesh.k.naduvalath@xxxxxxxxx wrote: >>> diff --git a/drivers/platform/x86/Kconfig >>> b/drivers/platform/x86/Kconfig index d12db6c316ea..8a0945ed1182 >> 100644 >>> --- a/drivers/platform/x86/Kconfig >>> +++ b/drivers/platform/x86/Kconfig >>> @@ -1176,6 +1176,22 @@ config INTEL_CHTDC_TI_PWRBTN >>> To compile this driver as a module, choose M here: the module >>> will be called intel_chtdc_ti_pwrbtn. >>> >>> +config INTEL_ISHTP_ECLITE >>> + tristate "Intel ISHTP eclite controller" >>> + depends on INTEL_ISH_HID >>> + depends on ACPI >>> + help >>> + This driver is for accessing the PSE (Programmable Service Engine), >>> + an Embedded Controller like IP, using ISHTP (Integrated Sensor Hub >> >> Which "IP" is that? "an Embedded Controller like IP" doesn't make sense >> IMO. > > This is Intel Elkhartlake's PSE IP. This IP is a dedicated low power real-time ARM > based microcontroller within the SoC. More information here: > https://www.intel.in/content/www/in/en/products/platforms/details/elkhart-lake.html > > Do you suggest to reword like below? > > This driver is for accessing the PSE (Programmable Service Engine) - an Embedded > Controller like IP - using ISHTP... That seems fine to me, can send a v4 with this and the other thing Randy pointed out fixed please ? Regards, Hans > > Or reword it with more details? > >> >> >>> + Transport Protocol) to get battery, thermal and UCSI (USB Type-C >>> + Connector System Software Interface) related data from the >> platform. >> >> Indent above line with tab + 2 spaces instead of many spaces. >> > Sure. Will fix in V4. > >>> + Users who don't want to use discrete Embedded Controller on >> Intel's >>> + Elkhartlake platform, can leverage this integrated solution of >> >> Drop this comma ^ >> > Sure. I'll fix it. > >>> + ECLite which is part of PSE subsystem. >>> + >>> + To compile this driver as a module, choose M here: the module >>> + will be called intel_ishtp_eclite >> >> End the last sentence with a period ('.'). >> >> > Sure. I'll fix it, thanks. > >> thanks. >> -- >> ~Randy >
