Hi,
On 26/2/19 18:21, Jett Rink wrote:
> We are specifically wanting userspace applications to not worry/confuse
> cros_ish
> with a normal cros_ec. Adding an attribute instead of changing the path would
> make it easy for userspace application to forget to check properly before
> accessing the ish as an EC when it shouldn't.
>
> On Mon, Feb 25, 2019 at 4:37 PM Guenter Roeck <gro...@google.com
> <mailto:gro...@google.com>> wrote:
>
> On Mon, Feb 25, 2019 at 3:22 PM Jett Rink <jettr...@chromium.org
> <mailto:jettr...@chromium.org>> wrote:
>
> A cros_ec and cros_ish device could both be present on the same
> system.
> We want to change the device path to ensure that drivers/code further
> up
> the stack does not get confuse the ISH with as an EC.
>
> The ISH device can export a similar sysfs interface as they both use
> the
> same command interface for communication (although they will use
> different transport layers). The common cros code will detect certain
> EC_FEATURES and enable the correct subsystem based on the level of
> functionality the device supports. In the case of the ISH, the sensor
> subsystem will be enabled.
>
> Seems to me it would make more sense to handle that difference with a
> sysfs
> attribute (instead of forcing each userspace application to support
> multiple
> sysfs paths).
>
Is still unclear to me what's that ISH device, so I'd appreciate if you can give
some more background. Trying to understand the topology, makes sense that block
diagram to you?
---------------------------
| User Space Applications |
---------------------------
----------------IIO ABI----------------
-----------------------------
| CrOS EC IIO Sensor Drivers |
-----------------------------
--------------------------
| CrOS EC over ISH Driver |
--------------------------
---------------- OS ------------------
--------------------------
| CrOS EC Firmware |
--------------------------
--------------------------
| ISH Hardware/Firmware |
--------------------------
So I'm right assuming that this CrOS EC will implement only the sensor features?
And then, the system will have another CrOS EC implementing other features like
RTC, USBPD-charger, etc?
Apart from the sensors features, will the CrOS EC ISH implement other features?
I'm a bit worried about the increasing way to use a particular name for
different CrOS EC, actually we have only cros_ec and cros_pd. But in the
chromeos kernels there is /dev/cros_fp, /dev/cros_tp, /dev/cros_ish,
/dev/cros_scp and who knows how many more in the future. So I'm wondering if
wouldn't be better use standard names, i.e /dev/cros_ec0, /dev/cros_ec1, etc. as
userspace, for those cases, should be able to query the EC_FEATURE_ISH/FP/TP/SCP
and know against which EC the device is attached.
Cheers,
Enric
> Guenter
>
>
> -Jett
>
> On Sun, Feb 24, 2019 at 4:03 PM Guenter Roeck <gro...@google.com
> <mailto:gro...@google.com>> wrote:
>
> On Sun, Feb 24, 2019 at 1:14 AM Rushikesh S Kadam
> <rushikesh.s.ka...@intel.com <mailto:rushikesh.s.ka...@intel.com>>
> wrote:
>
> Intel Integrated Sensor Hub (ISH) is also a MCU running EC
> having feature bit EC_FEATURE_ISH. Instantiate it as a special
> CrOS EC device with device name 'cros_ish'.
>
>
> The type of MCU doesn't really have to be reflected in the sysfs
> directory path. cros_ec uses different
> MCUs over time.
>
> Will the new path exist in parallel with cros_ec (in other words,
> will there also be a stand-alone EC in the same system) ? Does it
> have different or the same sysfs attributes as cros_ec ?
>
> Also,, what is the impact on userspace ?
>
> Thanks,
> Guenter
>
> Signed-off-by: Rushikesh S Kadam <rushikesh.s.ka...@intel.com
> <mailto:rushikesh.s.ka...@intel.com>>
> ---
> drivers/mfd/cros_ec_dev.c | 10 ++++++++++
> include/linux/mfd/cros_ec.h | 1 +
> include/linux/mfd/cros_ec_commands.h | 2 ++
> 3 files changed, 13 insertions(+)
>
> diff --git a/drivers/mfd/cros_ec_dev.c
> b/drivers/mfd/cros_ec_dev.c
> index 2d0fee4..be499b8 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -414,6 +414,16 @@ static int ec_device_probe(struct
> platform_device *pdev)
> device_initialize(&ec->class_dev);
> cdev_init(&ec->cdev, &fops);
>
> + /* check whether this is actually a Intel ISH rather
> than an EC */
> + if (cros_ec_check_features(ec, EC_FEATURE_ISH)) {
> + dev_info(dev, "Intel ISH MCU detected.\n");
> + /*
> + * Help userspace differentiating ECs from
> ISH MCU,
> + * regardless of the probing order.
> + */
> + ec_platform->ec_name = CROS_EC_DEV_ISH_NAME;
> + }
> +
> /*
> * Add the class device
> * Link to the character device for creating the /dev
> entry
> diff --git a/include/linux/mfd/cros_ec.h
> b/include/linux/mfd/cros_ec.h
> index de8b588..00c5765 100644
> --- a/include/linux/mfd/cros_ec.h
> +++ b/include/linux/mfd/cros_ec.h
> @@ -24,6 +24,7 @@
>
> #define CROS_EC_DEV_NAME "cros_ec"
> #define CROS_EC_DEV_PD_NAME "cros_pd"
> +#define CROS_EC_DEV_ISH_NAME "cros_ish"
>
> /*
> * The EC is unresponsive for a time after a reboot command.
> Add a
> diff --git a/include/linux/mfd/cros_ec_commands.h
> b/include/linux/mfd/cros_ec_commands.h
> index fc91082..9276c3c 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -856,6 +856,8 @@ enum ec_feature_code {
> EC_FEATURE_RTC = 27,
> /* EC supports CEC commands */
> EC_FEATURE_CEC = 35,
> + /* The MCU is an Intel Integrated Sensor Hub */
> + EC_FEATURE_ISH = 40,
> };
>
> #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code %
> 32))
> --
> 1.9.1
>
