Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor
On 26.04.2018 15:04, Sakari Ailus wrote: > On Thu, Apr 26, 2018 at 10:16:56AM +0300, Sakari Ailus wrote: >> On Thu, Apr 26, 2018 at 10:04:25AM +0300, Todor Tomov wrote: >>> Hi Sakari, >>> >>> On 26.04.2018 09:50, Sakari Ailus wrote: Hi Todor, On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote: ... > +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) > +{ > + u8 regbuf[3]; > + int ret; > + > + regbuf[0] = reg >> 8; > + regbuf[1] = reg & 0xff; > + regbuf[2] = val; > + > + ret = i2c_master_send(ov7251->i2c_client, regbuf, 3); > + if (ret < 0) { > + dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n", > + __func__, ret, reg, val); > + return ret; > + } > + > + return 0; How about: return ov7251_write_seq_regs(ov7251, reg, , 1); And put the function below ov2751_write_seq_regs(). >>> >>> I'm not sure... It will calculate message length each time and then check >>> that it is not greater than 5, which it is. Seems redundant. >>> > +} > + > +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, > + u8 num) > +{ > + const u8 maxregbuf = 5; > + u8 regbuf[maxregbuf]; > > Apparently this leads to bad positive sparse warning. I'd fix it by: > > diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c > index 3e2c0c03dfa9..d3ebb7529fca 100644 > --- a/drivers/media/i2c/ov7251.c > +++ b/drivers/media/i2c/ov7251.c > @@ -643,12 +643,11 @@ static int ov7251_write_reg(struct ov7251 *ov7251, u16 > reg, u8 val) > static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, > u8 num) > { > - const u8 maxregbuf = 5; > - u8 regbuf[maxregbuf]; > + u8 regbuf[5]; > u8 nregbuf = sizeof(reg) + num * sizeof(*val); > int ret = 0; > > - if (nregbuf > maxregbuf) > + if (nregbuf > sizeof(regbuf)) > return -EINVAL; > > regbuf[0] = reg >> 8; > > Let me know if you're happy with that; I can merge it to the original > patch. Yes, thanks. > > + u8 nregbuf = sizeof(reg) + num * sizeof(*val); > + int ret = 0; > + > + if (nregbuf > maxregbuf) > + return -EINVAL; > + > + regbuf[0] = reg >> 8; > + regbuf[1] = reg & 0xff; > + > + memcpy(regbuf + 2, val, num); > + > + ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf); > + if (ret < 0) { > + dev_err(ov7251->dev, "%s: write seq regs error %d: first > reg=%x\n", This line is over 80... >>> >>> Yes indeed. Somehow checkpatch does not report this line, I don't know why. >>> If you're happy with these, I can make the changes, too; they're trivial. >>> >>> Only the second one? Thanks :) >> >> Works for me. I'd still think the overhead of managing the buffer is >> irrelevant where to having an extra function to do essentially the same >> thing is a source of maintenance and review work. Note that we're even now >> spending time to discuss it. ;-) >> >> -- >> Kind regards, >> >> Sakari Ailus >> e-mail: sakari.ai...@iki.fi > -- Best regards, Todor Tomov
Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor
On Thu, Apr 26, 2018 at 10:16:56AM +0300, Sakari Ailus wrote: > On Thu, Apr 26, 2018 at 10:04:25AM +0300, Todor Tomov wrote: > > Hi Sakari, > > > > On 26.04.2018 09:50, Sakari Ailus wrote: > > > Hi Todor, > > > > > > On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote: > > > ... > > >> +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) > > >> +{ > > >> +u8 regbuf[3]; > > >> +int ret; > > >> + > > >> +regbuf[0] = reg >> 8; > > >> +regbuf[1] = reg & 0xff; > > >> +regbuf[2] = val; > > >> + > > >> +ret = i2c_master_send(ov7251->i2c_client, regbuf, 3); > > >> +if (ret < 0) { > > >> +dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, > > >> val=%x\n", > > >> +__func__, ret, reg, val); > > >> +return ret; > > >> +} > > >> + > > >> +return 0; > > > > > > How about: > > > > > > return ov7251_write_seq_regs(ov7251, reg, , 1); > > > > > > And put the function below ov2751_write_seq_regs(). > > > > I'm not sure... It will calculate message length each time and then check > > that it is not greater than 5, which it is. Seems redundant. > > > > > > > >> +} > > >> + > > >> +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 > > >> *val, > > >> + u8 num) > > >> +{ > > >> +const u8 maxregbuf = 5; > > >> +u8 regbuf[maxregbuf]; Apparently this leads to bad positive sparse warning. I'd fix it by: diff --git a/drivers/media/i2c/ov7251.c b/drivers/media/i2c/ov7251.c index 3e2c0c03dfa9..d3ebb7529fca 100644 --- a/drivers/media/i2c/ov7251.c +++ b/drivers/media/i2c/ov7251.c @@ -643,12 +643,11 @@ static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, u8 num) { - const u8 maxregbuf = 5; - u8 regbuf[maxregbuf]; + u8 regbuf[5]; u8 nregbuf = sizeof(reg) + num * sizeof(*val); int ret = 0; - if (nregbuf > maxregbuf) + if (nregbuf > sizeof(regbuf)) return -EINVAL; regbuf[0] = reg >> 8; Let me know if you're happy with that; I can merge it to the original patch. > > >> +u8 nregbuf = sizeof(reg) + num * sizeof(*val); > > >> +int ret = 0; > > >> + > > >> +if (nregbuf > maxregbuf) > > >> +return -EINVAL; > > >> + > > >> +regbuf[0] = reg >> 8; > > >> +regbuf[1] = reg & 0xff; > > >> + > > >> +memcpy(regbuf + 2, val, num); > > >> + > > >> +ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf); > > >> +if (ret < 0) { > > >> +dev_err(ov7251->dev, "%s: write seq regs error %d: > > >> first reg=%x\n", > > > > > > This line is over 80... > > > > Yes indeed. Somehow checkpatch does not report this line, I don't know why. > > > > > > > > If you're happy with these, I can make the changes, too; they're trivial. > > > > Only the second one? Thanks :) > > Works for me. I'd still think the overhead of managing the buffer is > irrelevant where to having an extra function to do essentially the same > thing is a source of maintenance and review work. Note that we're even now > spending time to discuss it. ;-) > > -- > Kind regards, > > Sakari Ailus > e-mail: sakari.ai...@iki.fi -- Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor
On Thu, Apr 26, 2018 at 10:04:25AM +0300, Todor Tomov wrote: > Hi Sakari, > > On 26.04.2018 09:50, Sakari Ailus wrote: > > Hi Todor, > > > > On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote: > > ... > >> +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) > >> +{ > >> + u8 regbuf[3]; > >> + int ret; > >> + > >> + regbuf[0] = reg >> 8; > >> + regbuf[1] = reg & 0xff; > >> + regbuf[2] = val; > >> + > >> + ret = i2c_master_send(ov7251->i2c_client, regbuf, 3); > >> + if (ret < 0) { > >> + dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n", > >> + __func__, ret, reg, val); > >> + return ret; > >> + } > >> + > >> + return 0; > > > > How about: > > > > return ov7251_write_seq_regs(ov7251, reg, , 1); > > > > And put the function below ov2751_write_seq_regs(). > > I'm not sure... It will calculate message length each time and then check > that it is not greater than 5, which it is. Seems redundant. > > > > >> +} > >> + > >> +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, > >> + u8 num) > >> +{ > >> + const u8 maxregbuf = 5; > >> + u8 regbuf[maxregbuf]; > >> + u8 nregbuf = sizeof(reg) + num * sizeof(*val); > >> + int ret = 0; > >> + > >> + if (nregbuf > maxregbuf) > >> + return -EINVAL; > >> + > >> + regbuf[0] = reg >> 8; > >> + regbuf[1] = reg & 0xff; > >> + > >> + memcpy(regbuf + 2, val, num); > >> + > >> + ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf); > >> + if (ret < 0) { > >> + dev_err(ov7251->dev, "%s: write seq regs error %d: first > >> reg=%x\n", > > > > This line is over 80... > > Yes indeed. Somehow checkpatch does not report this line, I don't know why. > > > > > If you're happy with these, I can make the changes, too; they're trivial. > > Only the second one? Thanks :) Works for me. I'd still think the overhead of managing the buffer is irrelevant where to having an extra function to do essentially the same thing is a source of maintenance and review work. Note that we're even now spending time to discuss it. ;-) -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi
Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor
Hi Sakari, On 26.04.2018 09:50, Sakari Ailus wrote: > Hi Todor, > > On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote: > ... >> +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) >> +{ >> +u8 regbuf[3]; >> +int ret; >> + >> +regbuf[0] = reg >> 8; >> +regbuf[1] = reg & 0xff; >> +regbuf[2] = val; >> + >> +ret = i2c_master_send(ov7251->i2c_client, regbuf, 3); >> +if (ret < 0) { >> +dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n", >> +__func__, ret, reg, val); >> +return ret; >> +} >> + >> +return 0; > > How about: > > return ov7251_write_seq_regs(ov7251, reg, , 1); > > And put the function below ov2751_write_seq_regs(). I'm not sure... It will calculate message length each time and then check that it is not greater than 5, which it is. Seems redundant. > >> +} >> + >> +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, >> + u8 num) >> +{ >> +const u8 maxregbuf = 5; >> +u8 regbuf[maxregbuf]; >> +u8 nregbuf = sizeof(reg) + num * sizeof(*val); >> +int ret = 0; >> + >> +if (nregbuf > maxregbuf) >> +return -EINVAL; >> + >> +regbuf[0] = reg >> 8; >> +regbuf[1] = reg & 0xff; >> + >> +memcpy(regbuf + 2, val, num); >> + >> +ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf); >> +if (ret < 0) { >> +dev_err(ov7251->dev, "%s: write seq regs error %d: first >> reg=%x\n", > > This line is over 80... Yes indeed. Somehow checkpatch does not report this line, I don't know why. > > If you're happy with these, I can make the changes, too; they're trivial. Only the second one? Thanks :) > >> +__func__, ret, reg); >> +return ret; >> +} >> + >> +return 0; >> +} > -- Best regards, Todor Tomov
Re: [PATCH v4 2/2] media: Add a driver for the ov7251 camera sensor
Hi Todor, On Wed, Apr 25, 2018 at 07:20:46PM +0300, Todor Tomov wrote: ... > +static int ov7251_write_reg(struct ov7251 *ov7251, u16 reg, u8 val) > +{ > + u8 regbuf[3]; > + int ret; > + > + regbuf[0] = reg >> 8; > + regbuf[1] = reg & 0xff; > + regbuf[2] = val; > + > + ret = i2c_master_send(ov7251->i2c_client, regbuf, 3); > + if (ret < 0) { > + dev_err(ov7251->dev, "%s: write reg error %d: reg=%x, val=%x\n", > + __func__, ret, reg, val); > + return ret; > + } > + > + return 0; How about: return ov7251_write_seq_regs(ov7251, reg, , 1); And put the function below ov2751_write_seq_regs(). > +} > + > +static int ov7251_write_seq_regs(struct ov7251 *ov7251, u16 reg, u8 *val, > + u8 num) > +{ > + const u8 maxregbuf = 5; > + u8 regbuf[maxregbuf]; > + u8 nregbuf = sizeof(reg) + num * sizeof(*val); > + int ret = 0; > + > + if (nregbuf > maxregbuf) > + return -EINVAL; > + > + regbuf[0] = reg >> 8; > + regbuf[1] = reg & 0xff; > + > + memcpy(regbuf + 2, val, num); > + > + ret = i2c_master_send(ov7251->i2c_client, regbuf, nregbuf); > + if (ret < 0) { > + dev_err(ov7251->dev, "%s: write seq regs error %d: first > reg=%x\n", This line is over 80... If you're happy with these, I can make the changes, too; they're trivial. > + __func__, ret, reg); > + return ret; > + } > + > + return 0; > +} -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi