Message ID | 20201013153633.31022-3-sakari.ailus@linux.intel.com |
---|---|
State | Accepted |
Commit | 4b6c129e87a31289f21077090698711f3c3ea459 |
Headers | show |
Series | Link frequency helper for receivers | expand |
Hi Sakari, I love your patch! Perhaps something to improve: [auto build test WARNING on linuxtv-media/master] [also build test WARNING on next-20201013] [cannot apply to v5.9] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Sakari-Ailus/Link-frequency-helper-for-receivers/20201013-233742 base: git://linuxtv.org/media_tree.git master config: i386-allyesconfig (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce (this is a W=1 build): # https://github.com/0day-ci/linux/commit/cf4ab5e39eb042b02f1d5660b5cbd88197a05520 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Sakari-Ailus/Link-frequency-helper-for-receivers/20201013-233742 git checkout cf4ab5e39eb042b02f1d5660b5cbd88197a05520 # save the attached .config to linux build tree make W=1 ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): In file included from include/linux/device.h:15, from include/linux/pci.h:37, from drivers/media/pci/intel/ipu3/ipu3-cio2.c:18: drivers/media/pci/intel/ipu3/ipu3-cio2.c: In function 'cio2_csi2_calc_timing': >> drivers/media/pci/intel/ipu3/ipu3-cio2.c:308:16: warning: format '%ld' expects argument of type 'long int', but argument 3 has type 's64' {aka 'long long int'} [-Wformat=] 308 | dev_err(dev, "error %ld, invalid link_freq\n", freq); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt' 19 | #define dev_fmt(fmt) fmt | ^~~ drivers/media/pci/intel/ipu3/ipu3-cio2.c:308:3: note: in expansion of macro 'dev_err' 308 | dev_err(dev, "error %ld, invalid link_freq\n", freq); | ^~~~~~~ drivers/media/pci/intel/ipu3/ipu3-cio2.c:308:25: note: format string is defined here 308 | dev_err(dev, "error %ld, invalid link_freq\n", freq); | ~~^ | | | long int | %lld drivers/media/pci/intel/ipu3/ipu3-cio2.c:301:6: warning: unused variable 'r' [-Wunused-variable] 301 | int r; | ^ vim +308 drivers/media/pci/intel/ipu3/ipu3-cio2.c 293 294 /* Calculate the the delay value for termination enable of clock lane HS Rx */ 295 static int cio2_csi2_calc_timing(struct cio2_device *cio2, struct cio2_queue *q, 296 struct cio2_csi2_timing *timing, 297 unsigned int bpp, unsigned int lanes) 298 { 299 struct device *dev = &cio2->pci_dev->dev; 300 s64 freq; 301 int r; 302 303 if (!q->sensor) 304 return -ENODEV; 305 306 freq = v4l2_get_link_rate(q->sensor->ctrl_handler, bpp, lanes); 307 if (freq < 0) { > 308 dev_err(dev, "error %ld, invalid link_freq\n", freq); 309 return freq; 310 } 311 312 timing->clk_termen = cio2_rx_timing(CIO2_CSIRX_DLY_CNT_TERMEN_CLANE_A, 313 CIO2_CSIRX_DLY_CNT_TERMEN_CLANE_B, 314 freq, 315 CIO2_CSIRX_DLY_CNT_TERMEN_DEFAULT); 316 timing->clk_settle = cio2_rx_timing(CIO2_CSIRX_DLY_CNT_SETTLE_CLANE_A, 317 CIO2_CSIRX_DLY_CNT_SETTLE_CLANE_B, 318 freq, 319 CIO2_CSIRX_DLY_CNT_SETTLE_DEFAULT); 320 timing->dat_termen = cio2_rx_timing(CIO2_CSIRX_DLY_CNT_TERMEN_DLANE_A, 321 CIO2_CSIRX_DLY_CNT_TERMEN_DLANE_B, 322 freq, 323 CIO2_CSIRX_DLY_CNT_TERMEN_DEFAULT); 324 timing->dat_settle = cio2_rx_timing(CIO2_CSIRX_DLY_CNT_SETTLE_DLANE_A, 325 CIO2_CSIRX_DLY_CNT_SETTLE_DLANE_B, 326 freq, 327 CIO2_CSIRX_DLY_CNT_SETTLE_DEFAULT); 328 329 dev_dbg(dev, "freq ct value is %d\n", timing->clk_termen); 330 dev_dbg(dev, "freq cs value is %d\n", timing->clk_settle); 331 dev_dbg(dev, "freq dt value is %d\n", timing->dat_termen); 332 dev_dbg(dev, "freq ds value is %d\n", timing->dat_settle); 333 334 return 0; 335 }; 336 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Sakari, I love your patch! Perhaps something to improve: [auto build test WARNING on linuxtv-media/master] [also build test WARNING on next-20201015] [cannot apply to v5.9] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Sakari-Ailus/Link-frequency-helper-for-receivers/20201013-233742 base: git://linuxtv.org/media_tree.git master config: x86_64-randconfig-a012-20201015 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project e7b4feea8e1bf520b34ad8c116abab6677344b74) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install x86_64 cross compiling tool for clang build # apt-get install binutils-x86-64-linux-gnu # https://github.com/0day-ci/linux/commit/cf4ab5e39eb042b02f1d5660b5cbd88197a05520 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Sakari-Ailus/Link-frequency-helper-for-receivers/20201013-233742 git checkout cf4ab5e39eb042b02f1d5660b5cbd88197a05520 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> drivers/media/pci/intel/ipu3/ipu3-cio2.c:308:50: warning: format specifies type 'long' but the argument has type 's64' (aka 'long long') [-Wformat] dev_err(dev, "error %ld, invalid link_freq\n", freq); ~~~ ^~~~ %lld include/linux/dev_printk.h:104:32: note: expanded from macro 'dev_err' _dev_err(dev, dev_fmt(fmt), ##__VA_ARGS__) ~~~ ^~~~~~~~~~~ drivers/media/pci/intel/ipu3/ipu3-cio2.c:301:6: warning: unused variable 'r' [-Wunused-variable] int r; ^ 2 warnings generated. vim +308 drivers/media/pci/intel/ipu3/ipu3-cio2.c 293 294 /* Calculate the the delay value for termination enable of clock lane HS Rx */ 295 static int cio2_csi2_calc_timing(struct cio2_device *cio2, struct cio2_queue *q, 296 struct cio2_csi2_timing *timing, 297 unsigned int bpp, unsigned int lanes) 298 { 299 struct device *dev = &cio2->pci_dev->dev; 300 s64 freq; 301 int r; 302 303 if (!q->sensor) 304 return -ENODEV; 305 306 freq = v4l2_get_link_rate(q->sensor->ctrl_handler, bpp, lanes); 307 if (freq < 0) { > 308 dev_err(dev, "error %ld, invalid link_freq\n", freq); 309 return freq; 310 } 311 312 timing->clk_termen = cio2_rx_timing(CIO2_CSIRX_DLY_CNT_TERMEN_CLANE_A, 313 CIO2_CSIRX_DLY_CNT_TERMEN_CLANE_B, 314 freq, 315 CIO2_CSIRX_DLY_CNT_TERMEN_DEFAULT); 316 timing->clk_settle = cio2_rx_timing(CIO2_CSIRX_DLY_CNT_SETTLE_CLANE_A, 317 CIO2_CSIRX_DLY_CNT_SETTLE_CLANE_B, 318 freq, 319 CIO2_CSIRX_DLY_CNT_SETTLE_DEFAULT); 320 timing->dat_termen = cio2_rx_timing(CIO2_CSIRX_DLY_CNT_TERMEN_DLANE_A, 321 CIO2_CSIRX_DLY_CNT_TERMEN_DLANE_B, 322 freq, 323 CIO2_CSIRX_DLY_CNT_TERMEN_DEFAULT); 324 timing->dat_settle = cio2_rx_timing(CIO2_CSIRX_DLY_CNT_SETTLE_DLANE_A, 325 CIO2_CSIRX_DLY_CNT_SETTLE_DLANE_B, 326 freq, 327 CIO2_CSIRX_DLY_CNT_SETTLE_DEFAULT); 328 329 dev_dbg(dev, "freq ct value is %d\n", timing->clk_termen); 330 dev_dbg(dev, "freq cs value is %d\n", timing->clk_settle); 331 dev_dbg(dev, "freq dt value is %d\n", timing->dat_termen); 332 dev_dbg(dev, "freq ds value is %d\n", timing->dat_settle); 333 334 return 0; 335 }; 336 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Sakari, Replying to an old thread. On Wed, Oct 14, 2020 at 11:30:15AM +0300, Sakari Ailus wrote: > Use v4l2_get_link_freq helper and add support for sensor drivers > implementing only V4L2_CID_PIXEL_RATE. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > since v1: > > - Use %lld for printing long long int > > - Remove r (unused variable) > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 34 +++++++++--------------- > 1 file changed, 12 insertions(+), 22 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > index c557d189200b..d060cfe473d8 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > @@ -33,6 +33,7 @@ struct ipu3_cio2_fmt { > u32 mbus_code; > u32 fourcc; > u8 mipicode; > + u8 bpp; > }; > > /* > @@ -46,18 +47,22 @@ static const struct ipu3_cio2_fmt formats[] = { > .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, > .fourcc = V4L2_PIX_FMT_IPU3_SGRBG10, > .mipicode = 0x2b, > + .bpp = 10, > }, { > .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, > .fourcc = V4L2_PIX_FMT_IPU3_SGBRG10, > .mipicode = 0x2b, > + .bpp = 10, > }, { > .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, > .fourcc = V4L2_PIX_FMT_IPU3_SBGGR10, > .mipicode = 0x2b, > + .bpp = 10, > }, { > .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, > .fourcc = V4L2_PIX_FMT_IPU3_SRGGB10, > .mipicode = 0x2b, > + .bpp = 10, > }, > }; > > @@ -288,35 +293,20 @@ static s32 cio2_rx_timing(s32 a, s32 b, s64 freq, int def) > > /* Calculate the the delay value for termination enable of clock lane HS Rx */ > static int cio2_csi2_calc_timing(struct cio2_device *cio2, struct cio2_queue *q, > - struct cio2_csi2_timing *timing) > + struct cio2_csi2_timing *timing, > + unsigned int bpp, unsigned int lanes) > { > struct device *dev = &cio2->pci_dev->dev; > - struct v4l2_querymenu qm = { .id = V4L2_CID_LINK_FREQ }; > - struct v4l2_ctrl *link_freq; > s64 freq; > - int r; > > if (!q->sensor) > return -ENODEV; > > - link_freq = v4l2_ctrl_find(q->sensor->ctrl_handler, V4L2_CID_LINK_FREQ); > - if (!link_freq) { > - dev_err(dev, "failed to find LINK_FREQ\n"); > - return -EPIPE; > - } > - > - qm.index = v4l2_ctrl_g_ctrl(link_freq); > - r = v4l2_querymenu(q->sensor->ctrl_handler, &qm); > - if (r) { > - dev_err(dev, "failed to get menu item\n"); > - return r; > - } > - > - if (!qm.value) { > - dev_err(dev, "error invalid link_freq\n"); > - return -EINVAL; > + freq = v4l2_get_link_rate(q->sensor->ctrl_handler, bpp, lanes); Shouldn't this use lanes * 2 ? > + if (freq < 0) { > + dev_err(dev, "error %lld, invalid link_freq\n", freq); > + return freq; > } > - freq = qm.value; > > timing->clk_termen = cio2_rx_timing(CIO2_CSIRX_DLY_CNT_TERMEN_CLANE_A, > CIO2_CSIRX_DLY_CNT_TERMEN_CLANE_B, > @@ -364,7 +354,7 @@ static int cio2_hw_init(struct cio2_device *cio2, struct cio2_queue *q) > > lanes = q->csi2.lanes; > > - r = cio2_csi2_calc_timing(cio2, q, &timing); > + r = cio2_csi2_calc_timing(cio2, q, &timing, fmt->bpp, lanes); > if (r) > return r; > -- Regards, Laurent Pinchart
Hi Laurent, On Mon, Feb 15, 2021 at 02:12:23AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Replying to an old thread. > > On Wed, Oct 14, 2020 at 11:30:15AM +0300, Sakari Ailus wrote: > > Use v4l2_get_link_freq helper and add support for sensor drivers > > implementing only V4L2_CID_PIXEL_RATE. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > since v1: > > > > - Use %lld for printing long long int > > > > - Remove r (unused variable) > > > > drivers/media/pci/intel/ipu3/ipu3-cio2.c | 34 +++++++++--------------- > > 1 file changed, 12 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > index c557d189200b..d060cfe473d8 100644 > > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c > > @@ -33,6 +33,7 @@ struct ipu3_cio2_fmt { > > u32 mbus_code; > > u32 fourcc; > > u8 mipicode; > > + u8 bpp; > > }; > > > > /* > > @@ -46,18 +47,22 @@ static const struct ipu3_cio2_fmt formats[] = { > > .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, > > .fourcc = V4L2_PIX_FMT_IPU3_SGRBG10, > > .mipicode = 0x2b, > > + .bpp = 10, > > }, { > > .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, > > .fourcc = V4L2_PIX_FMT_IPU3_SGBRG10, > > .mipicode = 0x2b, > > + .bpp = 10, > > }, { > > .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, > > .fourcc = V4L2_PIX_FMT_IPU3_SBGGR10, > > .mipicode = 0x2b, > > + .bpp = 10, > > }, { > > .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, > > .fourcc = V4L2_PIX_FMT_IPU3_SRGGB10, > > .mipicode = 0x2b, > > + .bpp = 10, > > }, > > }; > > > > @@ -288,35 +293,20 @@ static s32 cio2_rx_timing(s32 a, s32 b, s64 freq, int def) > > > > /* Calculate the the delay value for termination enable of clock lane HS Rx */ > > static int cio2_csi2_calc_timing(struct cio2_device *cio2, struct cio2_queue *q, > > - struct cio2_csi2_timing *timing) > > + struct cio2_csi2_timing *timing, > > + unsigned int bpp, unsigned int lanes) > > { > > struct device *dev = &cio2->pci_dev->dev; > > - struct v4l2_querymenu qm = { .id = V4L2_CID_LINK_FREQ }; > > - struct v4l2_ctrl *link_freq; > > s64 freq; > > - int r; > > > > if (!q->sensor) > > return -ENODEV; > > > > - link_freq = v4l2_ctrl_find(q->sensor->ctrl_handler, V4L2_CID_LINK_FREQ); > > - if (!link_freq) { > > - dev_err(dev, "failed to find LINK_FREQ\n"); > > - return -EPIPE; > > - } > > - > > - qm.index = v4l2_ctrl_g_ctrl(link_freq); > > - r = v4l2_querymenu(q->sensor->ctrl_handler, &qm); > > - if (r) { > > - dev_err(dev, "failed to get menu item\n"); > > - return r; > > - } > > - > > - if (!qm.value) { > > - dev_err(dev, "error invalid link_freq\n"); > > - return -EINVAL; > > + freq = v4l2_get_link_rate(q->sensor->ctrl_handler, bpp, lanes); > > Shouldn't this use lanes * 2 ? Yes, it should. Good catch! I'll send a patch... > > > + if (freq < 0) { > > + dev_err(dev, "error %lld, invalid link_freq\n", freq); > > + return freq; > > } > > - freq = qm.value; > > > > timing->clk_termen = cio2_rx_timing(CIO2_CSIRX_DLY_CNT_TERMEN_CLANE_A, > > CIO2_CSIRX_DLY_CNT_TERMEN_CLANE_B, > > @@ -364,7 +354,7 @@ static int cio2_hw_init(struct cio2_device *cio2, struct cio2_queue *q) > > > > lanes = q->csi2.lanes; > > > > - r = cio2_csi2_calc_timing(cio2, q, &timing); > > + r = cio2_csi2_calc_timing(cio2, q, &timing, fmt->bpp, lanes); > > if (r) > > return r; > > > > -- > Regards, > > Laurent Pinchart -- Sakari Ailus
diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c index c557d189200b..7acfdb3511e3 100644 --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c @@ -33,6 +33,7 @@ struct ipu3_cio2_fmt { u32 mbus_code; u32 fourcc; u8 mipicode; + u8 bpp; }; /* @@ -46,18 +47,22 @@ static const struct ipu3_cio2_fmt formats[] = { .mbus_code = MEDIA_BUS_FMT_SGRBG10_1X10, .fourcc = V4L2_PIX_FMT_IPU3_SGRBG10, .mipicode = 0x2b, + .bpp = 10, }, { .mbus_code = MEDIA_BUS_FMT_SGBRG10_1X10, .fourcc = V4L2_PIX_FMT_IPU3_SGBRG10, .mipicode = 0x2b, + .bpp = 10, }, { .mbus_code = MEDIA_BUS_FMT_SBGGR10_1X10, .fourcc = V4L2_PIX_FMT_IPU3_SBGGR10, .mipicode = 0x2b, + .bpp = 10, }, { .mbus_code = MEDIA_BUS_FMT_SRGGB10_1X10, .fourcc = V4L2_PIX_FMT_IPU3_SRGGB10, .mipicode = 0x2b, + .bpp = 10, }, }; @@ -288,36 +293,22 @@ static s32 cio2_rx_timing(s32 a, s32 b, s64 freq, int def) /* Calculate the the delay value for termination enable of clock lane HS Rx */ static int cio2_csi2_calc_timing(struct cio2_device *cio2, struct cio2_queue *q, - struct cio2_csi2_timing *timing) + struct cio2_csi2_timing *timing, + unsigned int bpp, unsigned int lanes) { struct device *dev = &cio2->pci_dev->dev; - struct v4l2_querymenu qm = { .id = V4L2_CID_LINK_FREQ }; - struct v4l2_ctrl *link_freq; s64 freq; int r; if (!q->sensor) return -ENODEV; - link_freq = v4l2_ctrl_find(q->sensor->ctrl_handler, V4L2_CID_LINK_FREQ); - if (!link_freq) { - dev_err(dev, "failed to find LINK_FREQ\n"); - return -EPIPE; + freq = v4l2_get_link_rate(q->sensor->ctrl_handler, bpp, lanes); + if (freq < 0) { + dev_err(dev, "error %ld, invalid link_freq\n", freq); + return freq; } - qm.index = v4l2_ctrl_g_ctrl(link_freq); - r = v4l2_querymenu(q->sensor->ctrl_handler, &qm); - if (r) { - dev_err(dev, "failed to get menu item\n"); - return r; - } - - if (!qm.value) { - dev_err(dev, "error invalid link_freq\n"); - return -EINVAL; - } - freq = qm.value; - timing->clk_termen = cio2_rx_timing(CIO2_CSIRX_DLY_CNT_TERMEN_CLANE_A, CIO2_CSIRX_DLY_CNT_TERMEN_CLANE_B, freq, @@ -364,7 +355,7 @@ static int cio2_hw_init(struct cio2_device *cio2, struct cio2_queue *q) lanes = q->csi2.lanes; - r = cio2_csi2_calc_timing(cio2, q, &timing); + r = cio2_csi2_calc_timing(cio2, q, &timing, fmt->bpp, lanes); if (r) return r;
Use v4l2_get_link_freq helper and add support for sensor drivers implementing only V4L2_CID_PIXEL_RATE. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/pci/intel/ipu3/ipu3-cio2.c | 33 +++++++++--------------- 1 file changed, 12 insertions(+), 21 deletions(-)