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