Re: [PATCH v2 6/7] i2c: ChromeOS EC tunnel driver

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 4674 bytes --]

Hi,

just a basic review to keep things rolling...

> On the original Samsung ARM Chromebook these devices were on an I2C
> bus that was shared between the AP and the EC and arbitrated using
> some extranal GPIOs (see i2c-arb-gpio-challenge).
> 
> The original arbitration scheme worked well enough but had some
> downsides:
> * It was nonstandard (not using standard I2C multimaster)
> * It only worked if the EC-AP communication was I2C
> * It was relatively hard to debug problems (hard to tell if i2c issues
>   were caused by the EC, the AP, or some device on the bus).
> 
> On the HP Chromebook 11 the design was changed to:

This paragraph would be a nice update for the gpio-arbitration docs.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt b/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt

The bindings should independently be sent to the devicetree list.

> new file mode 100644
> index 0000000..898f030
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-cros-ec-tunnel.txt
> @@ -0,0 +1,39 @@
> +I2C bus that tunnels through the ChromeOS EC (cros-ec)
> +======================================================
> +On some ChromeOS board designs we've got a connection to the EC (embedded
> +controller) but no direct connection to some devices on the other side of
> +the EC (like a battery and PMIC).  To get access to those devices we need
> +to tunnel our i2c commands through the EC.
> +
> +The node for this device should be under a cros-ec node like google,cros-ec-spi
> +or google,cros-ec-i2c.
> +
> +
> +Required properties:
> +- compatible: google,cros-ec-i2c-tunnel
> +- google,remote-bus: The EC bus we'd like to talk to.
> +
> +Optional child nodes:
> +- One node per I2C device connected to the tunnelled I2C bus.
> +
> +
> +Example:
> +	cros-ec@0 {
> +		compatible = "google,cros-ec-spi";
> +
> +		...
> +
> +		i2c-tunnel {
> +			compatible = "google,cros-ec-i2c-tunnel";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			google,remote-bus = <0>;
> +
> +			battery: sbs-battery@b {
> +				compatible = "sbs,sbs-battery";
> +				reg = <0xb>;
> +				sbs,poll-retry-count = <1>;
> +			};
> +		};
> +	}

Can the tunnel have n busses? How to represent them then? I think the
remote-bus property should go in favor of proper sub-nodes? Wouldn't it
also be more generic to have the tunnel node seperate and reference the
tunnel-mechanism (spi here) as a phandle?

> +/**
> + * ec_i2c_construct_message - construct a message to go to the EC
> + *
> + * This function effectively stuffs the standard i2c_msg format of Linux into
> + * a format that the EC understands.
> + *
> + * @buf: The buffer to fill.  Can pass NULL to count how many bytes the message
> + *       would be.

I wonder about this NULL thing. That means the size is calculated twice.
Why not make two functions instead, one fir size calc, one for setting
up?

> +/**
> + * ec_i2c_parse_response - Parse a response from the EC
> + *
> + * We'll take the EC's response and copy it back into msgs.
> + *
> + * @buf: The buffer to parse.  Can pass NULL to count how many bytes we expect
> + *	 the response to be. Otherwise we assume that the right number of
> + *	 bytes are available.

Ditto.

> +	result = bus->ec->command_sendrecv(bus->ec, EC_CMD_I2C_PASSTHRU,
> +					   request, request_len,
> +					   response, response_len);

This function pointer should be checked against NULL in probe, I
think.

> +static int ec_i2c_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
> +	struct device *dev = &pdev->dev;
> +	struct ec_i2c_device *bus = NULL;
> +	u32 remote_bus;
> +	int err;
> +
> +	dev_dbg(dev, "Probing\n");

Drop. Device core has it already.

> +
> +	if (!np) {
> +		dev_err(dev, "no device node\n");
> +		return -ENOENT;
> +	}

Can this happen?

> +
> +	bus = devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL);
> +	if (bus == NULL) {
> +		dev_err(dev, "cannot allocate bus device\n");

No need for error strings when allocating.

> +		return -ENOMEM;
> +	}
> +
> +	dev_dbg(dev, "ChromeOS EC I2C tunnel adapter\n");

Drop. Device core debug has it, too.

> +
> +	return err;
> +}
> +
> +static int ec_i2c_remove(struct platform_device *dev)
> +{
> +	struct ec_i2c_device *bus = platform_get_drvdata(dev);
> +
> +	platform_set_drvdata(dev, NULL);

Not needed.

> +
> +	i2c_del_adapter(&bus->adap);
> +
> +	return 0;
> +}
> +

Regards,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]