* [PATCH v3] media: si2168: Refactor command setup code
@ 2019-07-04 10:33 Marc Gonzalez
2019-07-12 8:43 ` Uwe Kleine-König
2019-07-12 15:47 ` Brad Love
0 siblings, 2 replies; 9+ messages in thread
From: Marc Gonzalez @ 2019-07-04 10:33 UTC (permalink / raw)
To: Antti Palosaari, Mauro Carvalho Chehab, Jonathan Neuschäfer
Cc: linux-media, LKML, MSM, Brad Love, Bjorn Andersson
Refactor the command setup code, and let the compiler determine
the size of each command.
Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
---
Changes from v1:
- Use a real function to populate struct si2168_cmd *cmd, and a trivial
macro wrapping it (macro because sizeof).
Changes from v2:
- Fix header mess
- Add Jonathan's tag
---
drivers/media/dvb-frontends/si2168.c | 146 +++++++++------------------
1 file changed, 45 insertions(+), 101 deletions(-)
diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
index c64b360ce6b5..5e81e076369c 100644
--- a/drivers/media/dvb-frontends/si2168.c
+++ b/drivers/media/dvb-frontends/si2168.c
@@ -12,6 +12,16 @@
static const struct dvb_frontend_ops si2168_ops;
+static void cmd_setup(struct si2168_cmd *cmd, char *args, int wlen, int rlen)
+{
+ memcpy(cmd->args, args, wlen);
+ cmd->wlen = wlen;
+ cmd->rlen = rlen;
+}
+
+#define CMD_SETUP(cmd, args, rlen) \
+ cmd_setup(cmd, args, sizeof(args) - 1, rlen)
+
/* execute firmware command */
static int si2168_cmd_execute(struct i2c_client *client, struct si2168_cmd *cmd)
{
@@ -84,15 +94,13 @@ static int si2168_ts_bus_ctrl(struct dvb_frontend *fe, int acquire)
dev_dbg(&client->dev, "%s acquire: %d\n", __func__, acquire);
/* set TS_MODE property */
- memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6);
+ CMD_SETUP(&cmd, "\x14\x00\x01\x10\x10\x00", 4);
if (acquire)
cmd.args[4] |= dev->ts_mode;
else
cmd.args[4] |= SI2168_TS_TRISTATE;
if (dev->ts_clock_gapped)
cmd.args[4] |= 0x40;
- cmd.wlen = 6;
- cmd.rlen = 4;
ret = si2168_cmd_execute(client, &cmd);
return ret;
@@ -116,19 +124,13 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status)
switch (c->delivery_system) {
case SYS_DVBT:
- memcpy(cmd.args, "\xa0\x01", 2);
- cmd.wlen = 2;
- cmd.rlen = 13;
+ CMD_SETUP(&cmd, "\xa0\x01", 13);
break;
case SYS_DVBC_ANNEX_A:
- memcpy(cmd.args, "\x90\x01", 2);
- cmd.wlen = 2;
- cmd.rlen = 9;
+ CMD_SETUP(&cmd, "\x90\x01", 9);
break;
case SYS_DVBT2:
- memcpy(cmd.args, "\x50\x01", 2);
- cmd.wlen = 2;
- cmd.rlen = 14;
+ CMD_SETUP(&cmd, "\x50\x01", 14);
break;
default:
ret = -EINVAL;
@@ -165,9 +167,7 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status)
/* BER */
if (*status & FE_HAS_VITERBI) {
- memcpy(cmd.args, "\x82\x00", 2);
- cmd.wlen = 2;
- cmd.rlen = 3;
+ CMD_SETUP(&cmd, "\x82\x00", 3);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
@@ -198,9 +198,7 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status)
/* UCB */
if (*status & FE_HAS_SYNC) {
- memcpy(cmd.args, "\x84\x01", 2);
- cmd.wlen = 2;
- cmd.rlen = 3;
+ CMD_SETUP(&cmd, "\x84\x01", 3);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
@@ -286,22 +284,18 @@ static int si2168_set_frontend(struct dvb_frontend *fe)
goto err;
}
- memcpy(cmd.args, "\x88\x02\x02\x02\x02", 5);
- cmd.wlen = 5;
- cmd.rlen = 5;
+ CMD_SETUP(&cmd, "\x88\x02\x02\x02\x02", 5);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
/* that has no big effect */
if (c->delivery_system == SYS_DVBT)
- memcpy(cmd.args, "\x89\x21\x06\x11\xff\x98", 6);
+ CMD_SETUP(&cmd, "\x89\x21\x06\x11\xff\x98", 3);
else if (c->delivery_system == SYS_DVBC_ANNEX_A)
- memcpy(cmd.args, "\x89\x21\x06\x11\x89\xf0", 6);
+ CMD_SETUP(&cmd, "\x89\x21\x06\x11\x89\xf0", 3);
else if (c->delivery_system == SYS_DVBT2)
- memcpy(cmd.args, "\x89\x21\x06\x11\x89\x20", 6);
- cmd.wlen = 6;
- cmd.rlen = 3;
+ CMD_SETUP(&cmd, "\x89\x21\x06\x11\x89\x20", 3);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
@@ -318,103 +312,77 @@ static int si2168_set_frontend(struct dvb_frontend *fe)
goto err;
}
- memcpy(cmd.args, "\x51\x03", 2);
- cmd.wlen = 2;
- cmd.rlen = 12;
+ CMD_SETUP(&cmd, "\x51\x03", 12);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
- memcpy(cmd.args, "\x12\x08\x04", 3);
- cmd.wlen = 3;
- cmd.rlen = 3;
+ CMD_SETUP(&cmd, "\x12\x08\x04", 3);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
- memcpy(cmd.args, "\x14\x00\x0c\x10\x12\x00", 6);
- cmd.wlen = 6;
- cmd.rlen = 4;
+ CMD_SETUP(&cmd, "\x14\x00\x0c\x10\x12\x00", 4);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
- memcpy(cmd.args, "\x14\x00\x06\x10\x24\x00", 6);
- cmd.wlen = 6;
- cmd.rlen = 4;
+ CMD_SETUP(&cmd, "\x14\x00\x06\x10\x24\x00", 4);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
- memcpy(cmd.args, "\x14\x00\x07\x10\x00\x24", 6);
- cmd.wlen = 6;
- cmd.rlen = 4;
+ CMD_SETUP(&cmd, "\x14\x00\x07\x10\x00\x24", 4);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
- memcpy(cmd.args, "\x14\x00\x0a\x10\x00\x00", 6);
+ CMD_SETUP(&cmd, "\x14\x00\x0a\x10\x00\x00", 4);
cmd.args[4] = delivery_system | bandwidth;
if (dev->spectral_inversion)
cmd.args[5] |= 1;
- cmd.wlen = 6;
- cmd.rlen = 4;
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
/* set DVB-C symbol rate */
if (c->delivery_system == SYS_DVBC_ANNEX_A) {
- memcpy(cmd.args, "\x14\x00\x02\x11", 4);
+ CMD_SETUP(&cmd, "\x14\x00\x02\x11\x00\x00", 4);
cmd.args[4] = ((c->symbol_rate / 1000) >> 0) & 0xff;
cmd.args[5] = ((c->symbol_rate / 1000) >> 8) & 0xff;
- cmd.wlen = 6;
- cmd.rlen = 4;
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
}
- memcpy(cmd.args, "\x14\x00\x0f\x10\x10\x00", 6);
- cmd.wlen = 6;
- cmd.rlen = 4;
+ CMD_SETUP(&cmd, "\x14\x00\x0f\x10\x10\x00", 4);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
- memcpy(cmd.args, "\x14\x00\x09\x10\xe3\x08", 6);
+ CMD_SETUP(&cmd, "\x14\x00\x09\x10\xe3\x08", 4);
cmd.args[5] |= dev->ts_clock_inv ? 0x00 : 0x10;
- cmd.wlen = 6;
- cmd.rlen = 4;
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
- memcpy(cmd.args, "\x14\x00\x08\x10\xd7\x05", 6);
+ CMD_SETUP(&cmd, "\x14\x00\x08\x10\xd7\x05", 4);
cmd.args[5] |= dev->ts_clock_inv ? 0x00 : 0x10;
- cmd.wlen = 6;
- cmd.rlen = 4;
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
- memcpy(cmd.args, "\x14\x00\x01\x12\x00\x00", 6);
- cmd.wlen = 6;
- cmd.rlen = 4;
+ CMD_SETUP(&cmd, "\x14\x00\x01\x12\x00\x00", 4);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
- memcpy(cmd.args, "\x14\x00\x01\x03\x0c\x00", 6);
- cmd.wlen = 6;
- cmd.rlen = 4;
+ CMD_SETUP(&cmd, "\x14\x00\x01\x03\x0c\x00", 4);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
- memcpy(cmd.args, "\x85", 1);
- cmd.wlen = 1;
- cmd.rlen = 1;
+ CMD_SETUP(&cmd, "\x85", 1);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
@@ -444,26 +412,20 @@ static int si2168_init(struct dvb_frontend *fe)
dev_dbg(&client->dev, "\n");
/* initialize */
- memcpy(cmd.args, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13);
- cmd.wlen = 13;
- cmd.rlen = 0;
+ CMD_SETUP(&cmd, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 0);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
if (dev->warm) {
/* resume */
- memcpy(cmd.args, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 8);
- cmd.wlen = 8;
- cmd.rlen = 1;
+ CMD_SETUP(&cmd, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 1);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
udelay(100);
- memcpy(cmd.args, "\x85", 1);
- cmd.wlen = 1;
- cmd.rlen = 1;
+ CMD_SETUP(&cmd, "\x85", 1);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
@@ -472,9 +434,7 @@ static int si2168_init(struct dvb_frontend *fe)
}
/* power up */
- memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8);
- cmd.wlen = 8;
- cmd.rlen = 1;
+ CMD_SETUP(&cmd, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 1);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
@@ -542,17 +502,13 @@ static int si2168_init(struct dvb_frontend *fe)
release_firmware(fw);
- memcpy(cmd.args, "\x01\x01", 2);
- cmd.wlen = 2;
- cmd.rlen = 1;
+ CMD_SETUP(&cmd, "\x01\x01", 1);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
/* query firmware version */
- memcpy(cmd.args, "\x11", 1);
- cmd.wlen = 1;
- cmd.rlen = 10;
+ CMD_SETUP(&cmd, "\x11", 10);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
@@ -610,9 +566,7 @@ static int si2168_sleep(struct dvb_frontend *fe)
if (dev->version > ('B' << 24 | 4 << 16 | 0 << 8 | 11 << 0))
dev->warm = false;
- memcpy(cmd.args, "\x13", 1);
- cmd.wlen = 1;
- cmd.rlen = 0;
+ CMD_SETUP(&cmd, "\x13", 0);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
@@ -638,9 +592,7 @@ static int si2168_select(struct i2c_mux_core *muxc, u32 chan)
struct si2168_cmd cmd;
/* open I2C gate */
- memcpy(cmd.args, "\xc0\x0d\x01", 3);
- cmd.wlen = 3;
- cmd.rlen = 0;
+ CMD_SETUP(&cmd, "\xc0\x0d\x01", 0);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
@@ -658,9 +610,7 @@ static int si2168_deselect(struct i2c_mux_core *muxc, u32 chan)
struct si2168_cmd cmd;
/* close I2C gate */
- memcpy(cmd.args, "\xc0\x0d\x00", 3);
- cmd.wlen = 3;
- cmd.rlen = 0;
+ CMD_SETUP(&cmd, "\xc0\x0d\x00", 0);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err;
@@ -753,25 +703,19 @@ static int si2168_probe(struct i2c_client *client,
mutex_init(&dev->i2c_mutex);
/* Initialize */
- memcpy(cmd.args, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13);
- cmd.wlen = 13;
- cmd.rlen = 0;
+ CMD_SETUP(&cmd, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 0);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err_kfree;
/* Power up */
- memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8);
- cmd.wlen = 8;
- cmd.rlen = 1;
+ CMD_SETUP(&cmd, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 1);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err_kfree;
/* Query chip revision */
- memcpy(cmd.args, "\x02", 1);
- cmd.wlen = 1;
- cmd.rlen = 13;
+ CMD_SETUP(&cmd, "\x02", 13);
ret = si2168_cmd_execute(client, &cmd);
if (ret)
goto err_kfree;
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH v3] media: si2168: Refactor command setup code
2019-07-04 10:33 [PATCH v3] media: si2168: Refactor command setup code Marc Gonzalez
@ 2019-07-12 8:43 ` Uwe Kleine-König
2019-07-12 9:37 ` Marc Gonzalez
2019-07-12 15:47 ` Brad Love
1 sibling, 1 reply; 9+ messages in thread
From: Uwe Kleine-König @ 2019-07-12 8:43 UTC (permalink / raw)
To: Marc Gonzalez
Cc: Antti Palosaari, Mauro Carvalho Chehab, Jonathan Neuschäfer,
linux-media, LKML, MSM, Brad Love, Bjorn Andersson
[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]
Hello,
On Thu, Jul 04, 2019 at 12:33:22PM +0200, Marc Gonzalez wrote:
> Refactor the command setup code, and let the compiler determine
> the size of each command.
>
> Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
> ---
> Changes from v1:
> - Use a real function to populate struct si2168_cmd *cmd, and a trivial
> macro wrapping it (macro because sizeof).
> Changes from v2:
> - Fix header mess
> - Add Jonathan's tag
> ---
> drivers/media/dvb-frontends/si2168.c | 146 +++++++++------------------
> 1 file changed, 45 insertions(+), 101 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
> index c64b360ce6b5..5e81e076369c 100644
> --- a/drivers/media/dvb-frontends/si2168.c
> +++ b/drivers/media/dvb-frontends/si2168.c
> @@ -12,6 +12,16 @@
>
> static const struct dvb_frontend_ops si2168_ops;
>
> +static void cmd_setup(struct si2168_cmd *cmd, char *args, int wlen, int rlen)
I'd add an "inline" here. And you could add a const for *args.
> +{
> + memcpy(cmd->args, args, wlen);
> + cmd->wlen = wlen;
> + cmd->rlen = rlen;
> +}
> +
> +#define CMD_SETUP(cmd, args, rlen) \
> + cmd_setup(cmd, args, sizeof(args) - 1, rlen)
Here is the chance to add some static checking. Also it is a good habit
to put parens around macro arguments.
Something like:
#define CMD_SETUP(cmd, args, rlen) ({ \
BUILD_BUG_ON(sizeof((args)) - 1 > SI2168_ARGLEN);
cmd_setup((cmd), (args), __must_be_array((args)) + sizeof((args)) - 1, (rlen));
Maybe let this macro live in drivers/media/dvb-frontends/si2168_priv.h
where struct si2168_cmd is defined?
I looked over the transformations in the rest of the patch and this
looks good.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3] media: si2168: Refactor command setup code
2019-07-12 8:43 ` Uwe Kleine-König
@ 2019-07-12 9:37 ` Marc Gonzalez
0 siblings, 0 replies; 9+ messages in thread
From: Marc Gonzalez @ 2019-07-12 9:37 UTC (permalink / raw)
To: Uwe Kleine-König, Antti Palosaari, Brad Love, Sean Young
Cc: Mauro Carvalho Chehab, Jonathan Neuschäfer, linux-media,
LKML, MSM, Bjorn Andersson
+ Sean
On 12/07/2019 10:43, Uwe Kleine-König wrote:
> On Thu, Jul 04, 2019 at 12:33:22PM +0200, Marc Gonzalez wrote:
>
>> Refactor the command setup code, and let the compiler determine
>> the size of each command.
>>
>> Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
>> ---
>> Changes from v1:
>> - Use a real function to populate struct si2168_cmd *cmd, and a trivial
>> macro wrapping it (macro because sizeof).
>> Changes from v2:
>> - Fix header mess
>> - Add Jonathan's tag
>> ---
>> drivers/media/dvb-frontends/si2168.c | 146 +++++++++------------------
>> 1 file changed, 45 insertions(+), 101 deletions(-)
>>
>> diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
>> index c64b360ce6b5..5e81e076369c 100644
>> --- a/drivers/media/dvb-frontends/si2168.c
>> +++ b/drivers/media/dvb-frontends/si2168.c
>> @@ -12,6 +12,16 @@
>>
>> static const struct dvb_frontend_ops si2168_ops;
>>
>> +static void cmd_setup(struct si2168_cmd *cmd, char *args, int wlen, int rlen)
>
> I'd add an "inline" here. And you could add a const for *args.
I was under the (vague) impression that it's better to let the compiler
decide when to inline, except for trivial alternatives in headers.
David Miller wrote: "Please do not use the inline directive in foo.c
files, let the compiler decide."
Antti, Sean, what do you think?
For my notes: https://gcc.gnu.org/onlinedocs/gcc/Inline.html
>> +{
>> + memcpy(cmd->args, args, wlen);
>> + cmd->wlen = wlen;
>> + cmd->rlen = rlen;
>> +}
>> +
>> +#define CMD_SETUP(cmd, args, rlen) \
>> + cmd_setup(cmd, args, sizeof(args) - 1, rlen)
>
> Here is the chance to add some static checking. Also it is a good habit
> to put parens around macro arguments.
Wrt parens around arguments, I figured they are not required here, since they
are used as function arguments. Though you may have a valid point.
Antti, Sean?
> Something like:
>
> #define CMD_SETUP(cmd, args, rlen) ({ \
> BUILD_BUG_ON(sizeof((args)) - 1 > SI2168_ARGLEN);
> cmd_setup((cmd), (args), __must_be_array((args)) + sizeof((args)) - 1, (rlen));
>
> Maybe let this macro live in drivers/media/dvb-frontends/si2168_priv.h
> where struct si2168_cmd is defined?
Antti, Sean?
> I looked over the transformations in the rest of the patch and this
> looks good.
Thanks for taking a look!
Regards.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] media: si2168: Refactor command setup code
2019-07-04 10:33 [PATCH v3] media: si2168: Refactor command setup code Marc Gonzalez
2019-07-12 8:43 ` Uwe Kleine-König
@ 2019-07-12 15:47 ` Brad Love
2019-07-12 17:45 ` Mauro Carvalho Chehab
2019-07-12 21:41 ` Marc Gonzalez
1 sibling, 2 replies; 9+ messages in thread
From: Brad Love @ 2019-07-12 15:47 UTC (permalink / raw)
To: Marc Gonzalez, Antti Palosaari, Mauro Carvalho Chehab,
Jonathan Neuschäfer
Cc: linux-media, LKML, MSM, Brad Love, Bjorn Andersson
Hi Marc,
Replying inline.
On 04/07/2019 05.33, Marc Gonzalez wrote:
> Refactor the command setup code, and let the compiler determine
> the size of each command.
>
> Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
> ---
> Changes from v1:
> - Use a real function to populate struct si2168_cmd *cmd, and a trivial
> macro wrapping it (macro because sizeof).
> Changes from v2:
> - Fix header mess
> - Add Jonathan's tag
> ---
> drivers/media/dvb-frontends/si2168.c | 146 +++++++++------------------
> 1 file changed, 45 insertions(+), 101 deletions(-)
>
> diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
> index c64b360ce6b5..5e81e076369c 100644
> --- a/drivers/media/dvb-frontends/si2168.c
> +++ b/drivers/media/dvb-frontends/si2168.c
> @@ -12,6 +12,16 @@
>
> static const struct dvb_frontend_ops si2168_ops;
>
> +static void cmd_setup(struct si2168_cmd *cmd, char *args, int wlen, int rlen)
> +{
> + memcpy(cmd->args, args, wlen);
> + cmd->wlen = wlen;
> + cmd->rlen = rlen;
> +}
> +
struct si2168_cmd.args is u8, not char. I also think const should apply
to the pointer.
> +#define CMD_SETUP(cmd, args, rlen) \
> + cmd_setup(cmd, args, sizeof(args) - 1, rlen)
> +
This is only a valid helper if args is a null terminated string. It just
so happens that every instance in this driver is, but that could be a
silent pitfall if someone used a u8 array with this macro.
Otherwise I'm ok with the refactoring.
Regards,
Brad
> /* execute firmware command */
> static int si2168_cmd_execute(struct i2c_client *client, struct si2168_cmd *cmd)
> {
> @@ -84,15 +94,13 @@ static int si2168_ts_bus_ctrl(struct dvb_frontend *fe, int acquire)
> dev_dbg(&client->dev, "%s acquire: %d\n", __func__, acquire);
>
> /* set TS_MODE property */
> - memcpy(cmd.args, "\x14\x00\x01\x10\x10\x00", 6);
> + CMD_SETUP(&cmd, "\x14\x00\x01\x10\x10\x00", 4);
> if (acquire)
> cmd.args[4] |= dev->ts_mode;
> else
> cmd.args[4] |= SI2168_TS_TRISTATE;
> if (dev->ts_clock_gapped)
> cmd.args[4] |= 0x40;
> - cmd.wlen = 6;
> - cmd.rlen = 4;
> ret = si2168_cmd_execute(client, &cmd);
>
> return ret;
> @@ -116,19 +124,13 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status)
>
> switch (c->delivery_system) {
> case SYS_DVBT:
> - memcpy(cmd.args, "\xa0\x01", 2);
> - cmd.wlen = 2;
> - cmd.rlen = 13;
> + CMD_SETUP(&cmd, "\xa0\x01", 13);
> break;
> case SYS_DVBC_ANNEX_A:
> - memcpy(cmd.args, "\x90\x01", 2);
> - cmd.wlen = 2;
> - cmd.rlen = 9;
> + CMD_SETUP(&cmd, "\x90\x01", 9);
> break;
> case SYS_DVBT2:
> - memcpy(cmd.args, "\x50\x01", 2);
> - cmd.wlen = 2;
> - cmd.rlen = 14;
> + CMD_SETUP(&cmd, "\x50\x01", 14);
> break;
> default:
> ret = -EINVAL;
> @@ -165,9 +167,7 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status)
>
> /* BER */
> if (*status & FE_HAS_VITERBI) {
> - memcpy(cmd.args, "\x82\x00", 2);
> - cmd.wlen = 2;
> - cmd.rlen = 3;
> + CMD_SETUP(&cmd, "\x82\x00", 3);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
> @@ -198,9 +198,7 @@ static int si2168_read_status(struct dvb_frontend *fe, enum fe_status *status)
>
> /* UCB */
> if (*status & FE_HAS_SYNC) {
> - memcpy(cmd.args, "\x84\x01", 2);
> - cmd.wlen = 2;
> - cmd.rlen = 3;
> + CMD_SETUP(&cmd, "\x84\x01", 3);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
> @@ -286,22 +284,18 @@ static int si2168_set_frontend(struct dvb_frontend *fe)
> goto err;
> }
>
> - memcpy(cmd.args, "\x88\x02\x02\x02\x02", 5);
> - cmd.wlen = 5;
> - cmd.rlen = 5;
> + CMD_SETUP(&cmd, "\x88\x02\x02\x02\x02", 5);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> /* that has no big effect */
> if (c->delivery_system == SYS_DVBT)
> - memcpy(cmd.args, "\x89\x21\x06\x11\xff\x98", 6);
> + CMD_SETUP(&cmd, "\x89\x21\x06\x11\xff\x98", 3);
> else if (c->delivery_system == SYS_DVBC_ANNEX_A)
> - memcpy(cmd.args, "\x89\x21\x06\x11\x89\xf0", 6);
> + CMD_SETUP(&cmd, "\x89\x21\x06\x11\x89\xf0", 3);
> else if (c->delivery_system == SYS_DVBT2)
> - memcpy(cmd.args, "\x89\x21\x06\x11\x89\x20", 6);
> - cmd.wlen = 6;
> - cmd.rlen = 3;
> + CMD_SETUP(&cmd, "\x89\x21\x06\x11\x89\x20", 3);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
> @@ -318,103 +312,77 @@ static int si2168_set_frontend(struct dvb_frontend *fe)
> goto err;
> }
>
> - memcpy(cmd.args, "\x51\x03", 2);
> - cmd.wlen = 2;
> - cmd.rlen = 12;
> + CMD_SETUP(&cmd, "\x51\x03", 12);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> - memcpy(cmd.args, "\x12\x08\x04", 3);
> - cmd.wlen = 3;
> - cmd.rlen = 3;
> + CMD_SETUP(&cmd, "\x12\x08\x04", 3);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> - memcpy(cmd.args, "\x14\x00\x0c\x10\x12\x00", 6);
> - cmd.wlen = 6;
> - cmd.rlen = 4;
> + CMD_SETUP(&cmd, "\x14\x00\x0c\x10\x12\x00", 4);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> - memcpy(cmd.args, "\x14\x00\x06\x10\x24\x00", 6);
> - cmd.wlen = 6;
> - cmd.rlen = 4;
> + CMD_SETUP(&cmd, "\x14\x00\x06\x10\x24\x00", 4);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> - memcpy(cmd.args, "\x14\x00\x07\x10\x00\x24", 6);
> - cmd.wlen = 6;
> - cmd.rlen = 4;
> + CMD_SETUP(&cmd, "\x14\x00\x07\x10\x00\x24", 4);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> - memcpy(cmd.args, "\x14\x00\x0a\x10\x00\x00", 6);
> + CMD_SETUP(&cmd, "\x14\x00\x0a\x10\x00\x00", 4);
> cmd.args[4] = delivery_system | bandwidth;
> if (dev->spectral_inversion)
> cmd.args[5] |= 1;
> - cmd.wlen = 6;
> - cmd.rlen = 4;
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> /* set DVB-C symbol rate */
> if (c->delivery_system == SYS_DVBC_ANNEX_A) {
> - memcpy(cmd.args, "\x14\x00\x02\x11", 4);
> + CMD_SETUP(&cmd, "\x14\x00\x02\x11\x00\x00", 4);
> cmd.args[4] = ((c->symbol_rate / 1000) >> 0) & 0xff;
> cmd.args[5] = ((c->symbol_rate / 1000) >> 8) & 0xff;
> - cmd.wlen = 6;
> - cmd.rlen = 4;
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
> }
>
> - memcpy(cmd.args, "\x14\x00\x0f\x10\x10\x00", 6);
> - cmd.wlen = 6;
> - cmd.rlen = 4;
> + CMD_SETUP(&cmd, "\x14\x00\x0f\x10\x10\x00", 4);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> - memcpy(cmd.args, "\x14\x00\x09\x10\xe3\x08", 6);
> + CMD_SETUP(&cmd, "\x14\x00\x09\x10\xe3\x08", 4);
> cmd.args[5] |= dev->ts_clock_inv ? 0x00 : 0x10;
> - cmd.wlen = 6;
> - cmd.rlen = 4;
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> - memcpy(cmd.args, "\x14\x00\x08\x10\xd7\x05", 6);
> + CMD_SETUP(&cmd, "\x14\x00\x08\x10\xd7\x05", 4);
> cmd.args[5] |= dev->ts_clock_inv ? 0x00 : 0x10;
> - cmd.wlen = 6;
> - cmd.rlen = 4;
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> - memcpy(cmd.args, "\x14\x00\x01\x12\x00\x00", 6);
> - cmd.wlen = 6;
> - cmd.rlen = 4;
> + CMD_SETUP(&cmd, "\x14\x00\x01\x12\x00\x00", 4);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> - memcpy(cmd.args, "\x14\x00\x01\x03\x0c\x00", 6);
> - cmd.wlen = 6;
> - cmd.rlen = 4;
> + CMD_SETUP(&cmd, "\x14\x00\x01\x03\x0c\x00", 4);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> - memcpy(cmd.args, "\x85", 1);
> - cmd.wlen = 1;
> - cmd.rlen = 1;
> + CMD_SETUP(&cmd, "\x85", 1);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
> @@ -444,26 +412,20 @@ static int si2168_init(struct dvb_frontend *fe)
> dev_dbg(&client->dev, "\n");
>
> /* initialize */
> - memcpy(cmd.args, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13);
> - cmd.wlen = 13;
> - cmd.rlen = 0;
> + CMD_SETUP(&cmd, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 0);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> if (dev->warm) {
> /* resume */
> - memcpy(cmd.args, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 8);
> - cmd.wlen = 8;
> - cmd.rlen = 1;
> + CMD_SETUP(&cmd, "\xc0\x06\x08\x0f\x00\x20\x21\x01", 1);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> udelay(100);
> - memcpy(cmd.args, "\x85", 1);
> - cmd.wlen = 1;
> - cmd.rlen = 1;
> + CMD_SETUP(&cmd, "\x85", 1);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
> @@ -472,9 +434,7 @@ static int si2168_init(struct dvb_frontend *fe)
> }
>
> /* power up */
> - memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8);
> - cmd.wlen = 8;
> - cmd.rlen = 1;
> + CMD_SETUP(&cmd, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 1);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
> @@ -542,17 +502,13 @@ static int si2168_init(struct dvb_frontend *fe)
>
> release_firmware(fw);
>
> - memcpy(cmd.args, "\x01\x01", 2);
> - cmd.wlen = 2;
> - cmd.rlen = 1;
> + CMD_SETUP(&cmd, "\x01\x01", 1);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
>
> /* query firmware version */
> - memcpy(cmd.args, "\x11", 1);
> - cmd.wlen = 1;
> - cmd.rlen = 10;
> + CMD_SETUP(&cmd, "\x11", 10);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
> @@ -610,9 +566,7 @@ static int si2168_sleep(struct dvb_frontend *fe)
> if (dev->version > ('B' << 24 | 4 << 16 | 0 << 8 | 11 << 0))
> dev->warm = false;
>
> - memcpy(cmd.args, "\x13", 1);
> - cmd.wlen = 1;
> - cmd.rlen = 0;
> + CMD_SETUP(&cmd, "\x13", 0);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
> @@ -638,9 +592,7 @@ static int si2168_select(struct i2c_mux_core *muxc, u32 chan)
> struct si2168_cmd cmd;
>
> /* open I2C gate */
> - memcpy(cmd.args, "\xc0\x0d\x01", 3);
> - cmd.wlen = 3;
> - cmd.rlen = 0;
> + CMD_SETUP(&cmd, "\xc0\x0d\x01", 0);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
> @@ -658,9 +610,7 @@ static int si2168_deselect(struct i2c_mux_core *muxc, u32 chan)
> struct si2168_cmd cmd;
>
> /* close I2C gate */
> - memcpy(cmd.args, "\xc0\x0d\x00", 3);
> - cmd.wlen = 3;
> - cmd.rlen = 0;
> + CMD_SETUP(&cmd, "\xc0\x0d\x00", 0);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err;
> @@ -753,25 +703,19 @@ static int si2168_probe(struct i2c_client *client,
> mutex_init(&dev->i2c_mutex);
>
> /* Initialize */
> - memcpy(cmd.args, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 13);
> - cmd.wlen = 13;
> - cmd.rlen = 0;
> + CMD_SETUP(&cmd, "\xc0\x12\x00\x0c\x00\x0d\x16\x00\x00\x00\x00\x00\x00", 0);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err_kfree;
>
> /* Power up */
> - memcpy(cmd.args, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 8);
> - cmd.wlen = 8;
> - cmd.rlen = 1;
> + CMD_SETUP(&cmd, "\xc0\x06\x01\x0f\x00\x20\x20\x01", 1);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err_kfree;
>
> /* Query chip revision */
> - memcpy(cmd.args, "\x02", 1);
> - cmd.wlen = 1;
> - cmd.rlen = 13;
> + CMD_SETUP(&cmd, "\x02", 13);
> ret = si2168_cmd_execute(client, &cmd);
> if (ret)
> goto err_kfree;
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3] media: si2168: Refactor command setup code
2019-07-12 15:47 ` Brad Love
@ 2019-07-12 17:45 ` Mauro Carvalho Chehab
2019-07-12 22:11 ` Marc Gonzalez
2019-07-12 21:41 ` Marc Gonzalez
1 sibling, 1 reply; 9+ messages in thread
From: Mauro Carvalho Chehab @ 2019-07-12 17:45 UTC (permalink / raw)
To: Brad Love
Cc: Marc Gonzalez, Antti Palosaari, Jonathan Neuschäfer,
linux-media, LKML, MSM, Bjorn Andersson
Em Fri, 12 Jul 2019 10:47:17 -0500
Brad Love <brad@nextdimension.cc> escreveu:
> Hi Marc,
>
> Replying inline.
>
>
> On 04/07/2019 05.33, Marc Gonzalez wrote:
> > Refactor the command setup code, and let the compiler determine
> > the size of each command.
> >
> > Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
> > ---
> > Changes from v1:
> > - Use a real function to populate struct si2168_cmd *cmd, and a trivial
> > macro wrapping it (macro because sizeof).
> > Changes from v2:
> > - Fix header mess
> > - Add Jonathan's tag
> > ---
> > drivers/media/dvb-frontends/si2168.c | 146 +++++++++------------------
> > 1 file changed, 45 insertions(+), 101 deletions(-)
> >
> > diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
> > index c64b360ce6b5..5e81e076369c 100644
> > --- a/drivers/media/dvb-frontends/si2168.c
> > +++ b/drivers/media/dvb-frontends/si2168.c
> > @@ -12,6 +12,16 @@
> >
> > static const struct dvb_frontend_ops si2168_ops;
> >
> > +static void cmd_setup(struct si2168_cmd *cmd, char *args, int wlen, int rlen)
> > +{
> > + memcpy(cmd->args, args, wlen);
> > + cmd->wlen = wlen;
> > + cmd->rlen = rlen;
> > +}
> > +
>
>
> struct si2168_cmd.args is u8, not char. I also think const should apply
> to the pointer.
>
>
> > +#define CMD_SETUP(cmd, args, rlen) \
> > + cmd_setup(cmd, args, sizeof(args) - 1, rlen)
> > +
>
>
> This is only a valid helper if args is a null terminated string. It just
> so happens that every instance in this driver is, but that could be a
> silent pitfall if someone used a u8 array with this macro.
Actually, it is uglier than that. Of one writes something like:
char buf[20];
buf[0] = 0x20;
buf[1] = 0x03;
CMD_SETUP(cmd, buf, 0);
// some other init, up to 5 values, then another CMD_SETUP()
sizeof() will evaluate to 20, and not to 2, with would be the
expected buffer size, and it will pass 18 random values.
IMHO, using sizeof() here is a very bad idea.
Regards,
Mauro
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v3] media: si2168: Refactor command setup code
2019-07-12 17:45 ` Mauro Carvalho Chehab
@ 2019-07-12 22:11 ` Marc Gonzalez
2019-07-13 10:02 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 9+ messages in thread
From: Marc Gonzalez @ 2019-07-12 22:11 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Brad Love
Cc: Antti Palosaari, Jonathan Neuschäfer, linux-media, LKML,
MSM, Bjorn Andersson
On 12/07/2019 19:45, Mauro Carvalho Chehab wrote:
> Brad Love <brad@nextdimension.cc> escreveu:
>
>> On 04/07/2019 05.33, Marc Gonzalez wrote:
>>
>>> +#define CMD_SETUP(cmd, args, rlen) \
>>> + cmd_setup(cmd, args, sizeof(args) - 1, rlen)
>>> +
>>
>> This is only a valid helper if args is a null terminated string. It just
>> so happens that every instance in this driver is, but that could be a
>> silent pitfall if someone used a u8 array with this macro.
>
> Actually, it is uglier than that. If one writes something like:
>
> char buf[20];
>
> buf[0] = 0x20;
> buf[1] = 0x03;
>
> CMD_SETUP(cmd, buf, 0);
>
> // some other init, up to 5 values, then another CMD_SETUP()
I'm not sure what you mean in the // comment.
What kind of init? Why up to 5 values? Why another CMD_SETUP?
> sizeof() will evaluate to 20, and not to 2, with would be the
> expected buffer size, and it will pass 18 random values.
>
> IMHO, using sizeof() here is a very bad idea.
You may have a point...
(Though I'm not proposing a kernel API function, merely code
refactoring for a single file that's unlikely to change going
forward.)
It's also bad form to repeat the cmd size (twice) when the compiler
can figure it out automatically for string literals (which is 95%
of the use-cases).
I can drop the macro, and just use the helper...
Or maybe there's a GCC extension to test that an argument is a
string literal...
Regards.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] media: si2168: Refactor command setup code
2019-07-12 22:11 ` Marc Gonzalez
@ 2019-07-13 10:02 ` Mauro Carvalho Chehab
2019-07-14 18:31 ` Matthias Schwarzott
0 siblings, 1 reply; 9+ messages in thread
From: Mauro Carvalho Chehab @ 2019-07-13 10:02 UTC (permalink / raw)
To: Marc Gonzalez
Cc: Brad Love, Antti Palosaari, Jonathan Neuschäfer,
linux-media, LKML, MSM, Bjorn Andersson
Em Sat, 13 Jul 2019 00:11:12 +0200
Marc Gonzalez <marc.w.gonzalez@free.fr> escreveu:
> On 12/07/2019 19:45, Mauro Carvalho Chehab wrote:
>
> > Brad Love <brad@nextdimension.cc> escreveu:
> >
> >> On 04/07/2019 05.33, Marc Gonzalez wrote:
> >>
> >>> +#define CMD_SETUP(cmd, args, rlen) \
> >>> + cmd_setup(cmd, args, sizeof(args) - 1, rlen)
> >>> +
> >>
> >> This is only a valid helper if args is a null terminated string. It just
> >> so happens that every instance in this driver is, but that could be a
> >> silent pitfall if someone used a u8 array with this macro.
> >
> > Actually, it is uglier than that. If one writes something like:
> >
> > char buf[20];
> >
> > buf[0] = 0x20;
> > buf[1] = 0x03;
> >
> > CMD_SETUP(cmd, buf, 0);
> >
> > // some other init, up to 5 values, then another CMD_SETUP()
>
> I'm not sure what you mean in the // comment.
> What kind of init? Why up to 5 values? Why another CMD_SETUP?
I mean that the same buffer could be re-used to do something like:
char buf[20];
buf[0] = 0x20;
buf[1] = 0x03;
CMD_SETUP(cmd, buf, 0); // write size here should be 2
buf[2] = 0x04
buf[3] = 0x00
buf[4] = 0x05
CMD_SETUP(cmd, buf, 0); // write size here should be 5
This kind of pattern happens on other drivers and someone may
end needing something like that at this driver on some future.
> > sizeof() will evaluate to 20, and not to 2, with would be the
> > expected buffer size, and it will pass 18 random values.
> >
> > IMHO, using sizeof() here is a very bad idea.
>
> You may have a point...
> (Though I'm not proposing a kernel API function, merely code
> refactoring for a single file that's unlikely to change going
> forward.)
Yes, I know, but we had already some bugs due to the usage of
sizeof() on similar macros at drivers in the past.
> It's also bad form to repeat the cmd size (twice) when the compiler
> can figure it out automatically for string literals (which is 95%
> of the use-cases).
>
> I can drop the macro, and just use the helper...
The helper function sounds fine.
>
> Or maybe there's a GCC extension to test that an argument is a
> string literal...
If this could be evaluated by some advanced macro logic that
would work not only with gcc but also with clang, then a
macro that does what you proposed could be useful.
There are some ways to check the type of a macro argument, but I'm
not sure if are there any way for it to distinguish between a
string constant from a char * array.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] media: si2168: Refactor command setup code
2019-07-13 10:02 ` Mauro Carvalho Chehab
@ 2019-07-14 18:31 ` Matthias Schwarzott
0 siblings, 0 replies; 9+ messages in thread
From: Matthias Schwarzott @ 2019-07-14 18:31 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Marc Gonzalez
Cc: Brad Love, Antti Palosaari, Jonathan Neuschäfer,
linux-media, LKML, MSM, Bjorn Andersson
Am 13.07.19 um 12:02 schrieb Mauro Carvalho Chehab:
> Em Sat, 13 Jul 2019 00:11:12 +0200
> Marc Gonzalez <marc.w.gonzalez@free.fr> escreveu:
>
>> On 12/07/2019 19:45, Mauro Carvalho Chehab wrote:
>>
>>> Brad Love <brad@nextdimension.cc> escreveu:
>>>
>>> IMHO, using sizeof() here is a very bad idea.
>>
>> You may have a point...
>> (Though I'm not proposing a kernel API function, merely code
>> refactoring for a single file that's unlikely to change going
>> forward.)
>
> Yes, I know, but we had already some bugs due to the usage of
> sizeof() on similar macros at drivers in the past.
>
>> It's also bad form to repeat the cmd size (twice) when the compiler
>> can figure it out automatically for string literals (which is 95%
>> of the use-cases).
>>
>> I can drop the macro, and just use the helper...
>
> The helper function sounds fine.
>
>>
>> Or maybe there's a GCC extension to test that an argument is a
>> string literal...
>
> If this could be evaluated by some advanced macro logic that
> would work not only with gcc but also with clang, then a
> macro that does what you proposed could be useful.
>
> There are some ways to check the type of a macro argument, but I'm
> not sure if are there any way for it to distinguish between a
> string constant from a char * array.
>
Maybe something like this will prevent compilation if the argument is no
string literal:
#define CMD_SETUP(cmd, args, rlen) \
cmd_setup(cmd, args "", sizeof(args) - 1, rlen)
Another idea is a check like:
#define CMD_SETUP(cmd, args, rlen) \
do { \
BUILD_BUG_ON(#args[0] != "\""); \
cmd_setup(cmd, args "", sizeof(args) - 1, rlen) \
} while(0)
Regards
Matthias
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] media: si2168: Refactor command setup code
2019-07-12 15:47 ` Brad Love
2019-07-12 17:45 ` Mauro Carvalho Chehab
@ 2019-07-12 21:41 ` Marc Gonzalez
1 sibling, 0 replies; 9+ messages in thread
From: Marc Gonzalez @ 2019-07-12 21:41 UTC (permalink / raw)
To: Brad Love, Antti Palosaari, Mauro Carvalho Chehab,
Jonathan Neuschäfer
Cc: linux-media, LKML, MSM, Bjorn Andersson
On 12/07/2019 17:47, Brad Love wrote:
> On 04/07/2019 05.33, Marc Gonzalez wrote:
>
>> Refactor the command setup code, and let the compiler determine
>> the size of each command.
>>
>> Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
>> ---
>> Changes from v1:
>> - Use a real function to populate struct si2168_cmd *cmd, and a trivial
>> macro wrapping it (macro because sizeof).
>> Changes from v2:
>> - Fix header mess
>> - Add Jonathan's tag
>> ---
>> drivers/media/dvb-frontends/si2168.c | 146 +++++++++------------------
>> 1 file changed, 45 insertions(+), 101 deletions(-)
>>
>> diff --git a/drivers/media/dvb-frontends/si2168.c b/drivers/media/dvb-frontends/si2168.c
>> index c64b360ce6b5..5e81e076369c 100644
>> --- a/drivers/media/dvb-frontends/si2168.c
>> +++ b/drivers/media/dvb-frontends/si2168.c
>> @@ -12,6 +12,16 @@
>>
>> static const struct dvb_frontend_ops si2168_ops;
>>
>> +static void cmd_setup(struct si2168_cmd *cmd, char *args, int wlen, int rlen)
>> +{
>> + memcpy(cmd->args, args, wlen);
>> + cmd->wlen = wlen;
>> + cmd->rlen = rlen;
>> +}
>
> struct si2168_cmd.args is u8, not char.
Yes, struct si2168_cmd.args is an u8 array.
However, string literals such as "\xa0\x01" are char arrays.
memcpy() ignores the types altogether.
> I also think const should apply to the pointer.
I can do that, even though it's obvious we're not writing
to args in the trivial cmd_setup() body.
>> +#define CMD_SETUP(cmd, args, rlen) \
>> + cmd_setup(cmd, args, sizeof(args) - 1, rlen)
>> +
>
> This is only a valid helper if args is a null terminated string.
You say that because of the "-1" arithmetic, I assume?
> It just so happens that every instance in this driver is,
FWIW, there are 2 calls where it is not.
memcpy(cmd.args, &fw->data[(fw->size - remaining) + 1], len);
memcpy(cmd.args, &fw->data[fw->size - remaining], len);
> but that could be a silent pitfall if someone used a u8 array
> with this macro.
Actually, the compiler warns if we pass an u8 array instead of
a char array. IMO, the type is actually a good thing, since it
warns for cases where we don't use a string literal.
> Otherwise I'm ok with the refactoring.
I'll see what I can do...
Regards.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-14 18:31 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-04 10:33 [PATCH v3] media: si2168: Refactor command setup code Marc Gonzalez
2019-07-12 8:43 ` Uwe Kleine-König
2019-07-12 9:37 ` Marc Gonzalez
2019-07-12 15:47 ` Brad Love
2019-07-12 17:45 ` Mauro Carvalho Chehab
2019-07-12 22:11 ` Marc Gonzalez
2019-07-13 10:02 ` Mauro Carvalho Chehab
2019-07-14 18:31 ` Matthias Schwarzott
2019-07-12 21:41 ` Marc Gonzalez
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).