Re: [PATCH 1/4] ratp: implement i2c read/write support

[Andrey Smirnov <andrew.smirnov@gmail.com>]

On Tue, Aug 21, 2018 at 8:20 AM Aleksander Morgado
<aleksander@aleksander.es> wrote:
>
> Introduce two new RATP commands that allow running i2c read/write
> operations, very similar in format to the already existing md/mw
> RATP commands.
>
> The messages are defined with a fixed 16-bit long register field, but
> it will only be treated as a 16-bit address if I2C_FLAG_WIDE_ADDRESS
> is set in the message flags field. If this flag is unset, the start
> register address is assumed 8-bit long.
>
> If the message includes the I2C_FLAG_MASTER_MODE flag, the start
> register field is ignored and a i2c master send/receive operation is
> performed.
>

LGTM, minor nitpicks below.

> Signed-off-by: Aleksander Morgado <aleksander@aleksander.es>
> ---
>  common/ratp/Makefile |   1 +
>  common/ratp/i2c.c    | 281 +++++++++++++++++++++++++++++++++++++++++++
>  include/ratp_bb.h    |   4 +
>  3 files changed, 286 insertions(+)
>  create mode 100644 common/ratp/i2c.c
>
> diff --git a/common/ratp/Makefile b/common/ratp/Makefile
> index 2c6d674f6..0234b55c1 100644
> --- a/common/ratp/Makefile
> +++ b/common/ratp/Makefile
> @@ -4,3 +4,4 @@ obj-y += getenv.o
>  obj-y += md.o
>  obj-y += mw.o
>  obj-y += reset.o
> +obj-y += i2c.o
> diff --git a/common/ratp/i2c.c b/common/ratp/i2c.c
> new file mode 100644
> index 000000000..b8d055b67
> --- /dev/null
> +++ b/common/ratp/i2c.c
> @@ -0,0 +1,281 @@
> +/*
> + * Copyright (c) 2011-2018 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
> + *

Is the above really true?

> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <common.h>
> +#include <ratp_bb.h>
> +#include <init.h>
> +#include <driver.h>
> +#include <malloc.h>
> +#include <errno.h>
> +#include <i2c/i2c.h>
> +
> +/* NOTE:
> + *  - Fixed-size fields (e.g. integers) are given just after the header.
> + *  - Variable-length fields are stored inside the buffer[] and their position
> + *    within the buffer[] and their size are given as fixed-sized fields after
> + *    the header.
> + *  The message may be extended at any time keeping backwards compatibility,
> + *  as the position of the buffer[] is given by the buffer_offset field. i.e.
> + *  increasing the buffer_offset field we can extend the fixed-sized section
> + *  to add more fields.
> + */
> +
> +#define I2C_FLAG_WIDE_ADDRESS (1 << 0)
> +#define I2C_FLAG_MASTER_MODE  (1 << 1)

BIT() might be a bit more concise

> +
> +struct ratp_bb_i2c_read_request {
> +       struct ratp_bb header;
> +       uint16_t buffer_offset;
> +       uint8_t  bus;
> +       uint8_t  addr;
> +       uint16_t reg;
> +       uint8_t  flags;
> +       uint16_t size;
> +       uint8_t  buffer[];
> +} __attribute__((packed));

There should already be __packed shortcut, so you can use it instead
of the "full form". Same for all other headers below.

> +
> +struct ratp_bb_i2c_read_response {
> +       struct ratp_bb header;
> +       uint16_t buffer_offset;
> +       uint32_t errno;
> +       uint16_t data_size;
> +       uint16_t data_offset;
> +       uint8_t  buffer[];
> +} __attribute__((packed));
> +
> +struct ratp_bb_i2c_write_request {
> +       struct ratp_bb header;
> +       uint16_t buffer_offset;
> +       uint8_t  bus;
> +       uint8_t  addr;
> +       uint16_t reg;
> +       uint8_t  flags;
> +       uint16_t data_size;
> +       uint16_t data_offset;
> +       uint8_t  buffer[];
> +} __attribute__((packed));
> +
> +struct ratp_bb_i2c_write_response {
> +       struct ratp_bb header;
> +       uint16_t buffer_offset;
> +       uint32_t errno;
> +       uint16_t written;
> +       uint8_t  buffer[];
> +} __attribute__((packed));
> +
> +static int ratp_cmd_i2c_read(const struct ratp_bb *req, int req_len,
> +                            struct ratp_bb **rsp, int *rsp_len)
> +{
> +       struct ratp_bb_i2c_read_request *i2c_read_req = (struct ratp_bb_i2c_read_request *)req;
> +       struct ratp_bb_i2c_read_response *i2c_read_rsp;
> +       struct i2c_adapter *adapter;
> +       struct i2c_client client;
> +       uint16_t buffer_offset;
> +       int i2c_read_rsp_len;
> +       uint16_t reg;
> +       uint16_t size;
> +       uint32_t wide = 0;
> +       int ret = 0;
> +
> +       /* At least message header should be valid */
> +       if (req_len < sizeof(*i2c_read_req)) {
> +               printf("ratp i2c read ignored: size mismatch (%d < %zu)\n",

You can use pr_err() here instead. This way this error message would
have log level embedded in it and on builds/terminals that support it
it would be colored accordingly (red). You can also look into
utilizing pr_fmt() for providing common prefix for all of your pr_*
printed messages. Like, for example, is done here:
https://git.pengutronix.de/cgit/barebox/tree/drivers/pci/pci.c#n1

> +                      req_len, sizeof (*i2c_read_req));

Looks like there's a whitespace before "(" (GNU coding style?).

> +               ret = -EINVAL;
> +               goto out_rsp;
> +       }
> +
> +       /* We don't require any buffer here, but just validate buffer position and size */
> +       buffer_offset = be16_to_cpu(i2c_read_req->buffer_offset);
> +       if (buffer_offset != req_len) {
> +               printf("ratp i2c read ignored: invalid buffer offset (%hu != %d)\n",
> +                      buffer_offset, req_len);
> +               ret = -EINVAL;
> +               goto out_rsp;
> +       }
> +
> +       reg  = be16_to_cpu (i2c_read_req->reg);
> +       size = be16_to_cpu (i2c_read_req->size);

Looks like more of whitespace before opening parenthesis.

> +       if (i2c_read_req->flags & I2C_FLAG_WIDE_ADDRESS)
> +               wide = I2C_ADDR_16_BIT;
> +
> +out_rsp:
> +       /* Avoid reading anything on error */
> +       if (ret != 0)
> +               size = 0;
> +
> +       i2c_read_rsp_len = sizeof(*i2c_read_rsp) + size;
> +       i2c_read_rsp = xzalloc(i2c_read_rsp_len);
> +       i2c_read_rsp->header.type = cpu_to_be16(BB_RATP_TYPE_I2C_READ_RETURN);
> +       i2c_read_rsp->buffer_offset = cpu_to_be16(sizeof(*i2c_read_rsp));
> +       i2c_read_rsp->data_offset = 0;
> +
> +       /* Don't read anything on error or if 0 bytes were requested */
> +       if (size > 0) {
> +               adapter = i2c_get_adapter(i2c_read_req->bus);
> +               if (!adapter) {
> +                       printf("ratp i2c read ignored: i2c bus %u not found\n", i2c_read_req->bus);
> +                       ret = -ENODEV;
> +                       goto out;
> +               }
> +
> +               client.adapter = adapter;
> +               client.addr = i2c_read_req->addr;
> +
> +               if (i2c_read_req->flags & I2C_FLAG_MASTER_MODE) {
> +                       ret = i2c_master_recv(&client, i2c_read_rsp->buffer, size);
> +               } else {
> +                       ret = i2c_read_reg(&client, reg | wide, i2c_read_rsp->buffer, size);
> +               }
> +               if (ret != size) {
> +                       printf("ratp i2c read ignored: not all bytes read (%u < %u)\n", ret, size);
> +                       ret = -EIO;
> +                       goto out;
> +               }
> +               ret = 0;
> +       }
> +
> +out:
> +       if (ret != 0) {
> +               i2c_read_rsp->data_size = 0;
> +               i2c_read_rsp->errno = cpu_to_be32(ret);
> +               i2c_read_rsp_len = sizeof(*i2c_read_rsp);
> +       } else {
> +               i2c_read_rsp->data_size = cpu_to_be16(size);
> +               i2c_read_rsp->errno = 0;
> +       }
> +

It looks like you can move:

i2c_read_rsp->data_size = cpu_to_be16(size);
i2c_read_rsp->errno = cpu_to_be32(ret);

outside of if since it should work as intended for both cases (size is
0 if ret != 0).

> +       *rsp = (struct ratp_bb *)i2c_read_rsp;
> +       *rsp_len = i2c_read_rsp_len;
> +
> +       return ret;
> +}
> +
> +BAREBOX_RATP_CMD_START(I2C_READ)
> +       .request_id = BB_RATP_TYPE_I2C_READ,
> +       .response_id = BB_RATP_TYPE_I2C_READ_RETURN,
> +       .cmd = ratp_cmd_i2c_read
> +BAREBOX_RATP_CMD_END
> +
> +
> +static int ratp_cmd_i2c_write(const struct ratp_bb *req, int req_len,
> +                            struct ratp_bb **rsp, int *rsp_len)
> +{
> +       struct ratp_bb_i2c_write_request *i2c_write_req = (struct ratp_bb_i2c_write_request *)req;
> +       struct ratp_bb_i2c_write_response *i2c_write_rsp;
> +       struct i2c_adapter *adapter;
> +       struct i2c_client client;
> +       uint8_t *buffer;
> +       uint16_t buffer_offset;
> +       uint16_t buffer_size;
> +       uint16_t reg;
> +       uint16_t data_offset;
> +       uint16_t data_size;
> +       uint32_t wide = 0;
> +       int ret = 0;
> +       int written = 0;
> +
> +       /* At least message header should be valid */
> +       if (req_len < sizeof(*i2c_write_req)) {
> +               printf("ratp i2c write ignored: size mismatch (%d < %zu)\n",
> +                      req_len, sizeof (*i2c_write_req));
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       /* Validate buffer position and size */
> +       buffer_offset = be16_to_cpu(i2c_write_req->buffer_offset);
> +       if (req_len < buffer_offset) {
> +               printf("ratp i2c write ignored: invalid buffer offset (%d < %hu)\n",
> +                      req_len, buffer_offset);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +       buffer_size = req_len - buffer_offset;
> +       buffer = ((uint8_t *)i2c_write_req) + buffer_offset;
> +
> +       /* Validate data position and size */
> +       data_offset = be16_to_cpu(i2c_write_req->data_offset);
> +       if (data_offset != 0) {
> +               printf("ratp i2c write ignored: invalid data offset\n");
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +       data_size = be16_to_cpu(i2c_write_req->data_size);
> +       if (!data_size) {
> +               /* Success */
> +               goto out;
> +       }
> +
> +       /* Validate buffer size */
> +       if (buffer_size < data_size) {
> +               printf("ratp i2c write ignored: size mismatch (%d < %hu): data not fully given\n",
> +                      buffer_size, data_size);
> +               ret = -EINVAL;
> +               goto out;
> +       }
> +
> +       reg  = be16_to_cpu (i2c_write_req->reg);
> +       if (i2c_write_req->flags & I2C_FLAG_WIDE_ADDRESS)
> +               wide = I2C_ADDR_16_BIT;
> +
> +       adapter = i2c_get_adapter(i2c_write_req->bus);
> +       if (!adapter) {
> +               printf("ratp i2c write ignored: i2c bus %u not found\n", i2c_write_req->bus);
> +               ret = -ENODEV;
> +               goto out;
> +       }
> +
> +       client.adapter = adapter;
> +       client.addr = i2c_write_req->addr;

This pattern seems to have quite a bit of users in the codebase
already, so if you feel like going through the motions needed, this
can probably added as a simple helper function in include/i2c/i2c.h
and drivers/i2c/i2c.c.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox