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
signature.asc
Description: Digital signature
