Re: [PATCH v2 2/2] platform/chrome: cros_ec_lpc: Add Support for direct EC register memory access

[Tzung-Bi Shih <tzungbi@kernel.org>]

On Mon, Jan 06, 2025 at 10:52:32AM -0800, Gwendal Grignou wrote:
> Add support to access EC memory region HOST command and ACPI memory
> region directly.
> 
> The memory region comes from the CRS ACPI resource descriptor.
> 
> Able to communicate with the EC:
> ```
>  ectool version
> ...
> Build info:    brtk-0.0.0-df0ad93+ 2024-12-13 19:25:55 elmo@396-a1a
> ```

Asked in v1 too: why the build info for `ectool` is important?  Or is it
just an example for communicating with EC?

> Coreboot must exposed the memory region:
> ```
> cat /proc/iomem | grep GOOG0004
>     fe0b0000-fe0bffff : GOOG0004:00
> ```

Asked in v1 too: `grep GOOG0004 /proc/iomem` does the same thing and should
be more concise.  It's fine if you'd like to keep it.

> Can observe the commands flowing between AP and EC.

Could you provide some more context?  Couldn't we see the trace without the
patch?  I guess these (includes above) is just a test scenario.

If I understand the whole series correctly, it tries to support some Realtek
EC which needs to read the memory region info from ACPI instead of the
well-known ones.  If so, the commit message should provide some more context
about the new way for probing.

> @@ -498,29 +555,52 @@ static int cros_ec_lpc_probe(struct platform_device *pdev)
[...]
> +	if (adev) {
> +		/*
> +		 * Retrieve the resource information in the CRS register, if available.
> +		 */
> +		status = acpi_walk_resources(adev->handle, METHOD_NAME__CRS,
> +					     cros_ec_lpc_resources, ec_lpc);
> +		if (ACPI_FAILURE(status)) {
> +			dev_err(dev, "failed to get resources\n");
> +			return -ENODEV;
> +		}
> +		if (ec_lpc->mem32.address_length) {
> +			ec_lpc->base = devm_ioremap(dev,
> +						    ec_lpc->mem32.address,
> +						    ec_lpc->mem32.address_length);
> +			if (IS_ERR(ec_lpc->base))
> +				return -EINVAL;

Just realized that ioremap*() doesn't return ERR but NULL.  It should use
`!ec_lpc->base`.