Re: [PATCH 1/4] ratp: implement i2c read/write support
Hey Andrey,
Thanks for the review :) see some comments below.
>> + /* 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).
>
Don't think I can do that. In the if (size > 0) {} just a bit above,
size is not modified but ret may become an error. We do want to make
sure 0 is returned as size when there is an error, so cannot move it
outside the if() as you suggest here.
--
Aleksander
https://aleksander.es
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH 1/4] ratp: implement i2c read/write support
On Thu, Aug 23, 2018 at 10:54:58PM +0200, Aleksander Morgado wrote:
> >> +struct ratp_bb_i2c_read_request {
> >> + struct ratp_bb header;
> >> + uint16_t buffer_offset;
> >> + uint8_t bus;
> >> + uint8_t addr;
> >
> > I wonder how we see the RATP support. If it's for adhoc debugging then
> > bus/addr is fine. The caller should have no expectations that the bus
> > number is constant though. Likewise for the address which might change
> > across different board revisions.
> >
> > Should we have support for resolving names, which could be provided by
> > aliases in dt?
> >
> > We could still add name resolving support later as a separate call, I
> > just thought that now is the time to think how we proceed.
> >
>
> I truly have no opinion here, but if name resolving is added at some
> point I can either update this operation or even add a new one.
Ok, as said, we can add the name resolving functionality later. So, as
long as you don't see such numbering issues with the things you want to
do with RATP then I am fine with adding this when it's needed.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH 1/4] ratp: implement i2c read/write support
>> +struct ratp_bb_i2c_read_request {
>> + struct ratp_bb header;
>> + uint16_t buffer_offset;
>> + uint8_t bus;
>> + uint8_t addr;
>
> I wonder how we see the RATP support. If it's for adhoc debugging then
> bus/addr is fine. The caller should have no expectations that the bus
> number is constant though. Likewise for the address which might change
> across different board revisions.
>
> Should we have support for resolving names, which could be provided by
> aliases in dt?
>
> We could still add name resolving support later as a separate call, I
> just thought that now is the time to think how we proceed.
>
I truly have no opinion here, but if name resolving is added at some
point I can either update this operation or even add a new one.
--
Aleksander
https://aleksander.es
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH 1/4] ratp: implement i2c read/write support
On Tue, Aug 21, 2018 at 05:19:58PM +0200, Aleksander Morgado 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.
>
> Signed-off-by: Aleksander Morgado
> ---
> 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 0..b8d055b67
> --- /dev/null
> +++ b/common/ratp/i2c.c
> @@ -0,0 +1,281 @@
> +/*
> + * Copyright (c) 2011-2018 Sascha Hauer , Pengutronix
> + *
> + * 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* 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)
> +
> +struct ratp_bb_i2c_read_request {
> + struct ratp_bb header;
> + uint16_t buffer_offset;
> + uint8_t bus;
> + uint8_t addr;
I wonder how we see the RATP support. If it's for adhoc debugging then
bus/addr is fine. The caller should have no expectations that the bus
number is constant though. Likewise for the address which might change
across different board revisions.
Should we have support for resolving names, which could be provided by
aliases in dt?
We could still add name resolving support later as a separate call, I
just thought that now is the time to think how we proceed.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox
Re: [PATCH 1/4] ratp: implement i2c read/write support
On Tue, Aug 21, 2018 at 8:20 AM Aleksander Morgado
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
> ---
> 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 0..b8d055b67
> --- /dev/null
> +++ b/common/ratp/i2c.c
> @@ -0,0 +1,281 @@
> +/*
> + * Copyright (c) 2011-2018 Sascha Hauer , 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
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/* 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 y
[PATCH 1/4] ratp: implement i2c read/write support
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.
Signed-off-by: Aleksander Morgado
---
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 0..b8d055b67
--- /dev/null
+++ b/common/ratp/i2c.c
@@ -0,0 +1,281 @@
+/*
+ * Copyright (c) 2011-2018 Sascha Hauer , Pengutronix
+ *
+ * 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
+#include
+#include
+#include
+#include
+#include
+#include
+
+/* 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)
+
+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));
+
+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",
+ req_len, sizeof (*i2c_read_req));
+ 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);
+ if (i2c_read_req->flags & I2C_FLAG_WIDE_ADDRESS)
+ wide = I2C_ADDR_16_BIT;
+
+out_rsp:
+ /* Avoid reading anything on error */
+ if (ret != 0)
