| Message ID | 20250306130629.885163-4-stanislaw.gruszka@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: intel/ipu6: minor cleanups | expand |
Hi Stanislaw, Thanks for the set. A few minor comments below. On Thu, Mar 06, 2025 at 02:06:29PM +0100, Stanislaw Gruszka wrote: > Make ipu6_buttress_ctrl constant since it is not modified > any longer. Fits on previous line. > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > --- > drivers/media/pci/intel/ipu6/ipu6-bus.c | 2 +- > drivers/media/pci/intel/ipu6/ipu6-bus.h | 4 ++-- > drivers/media/pci/intel/ipu6/ipu6-buttress.c | 2 +- > drivers/media/pci/intel/ipu6/ipu6-buttress.h | 3 ++- > 4 files changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-bus.c b/drivers/media/pci/intel/ipu6/ipu6-bus.c > index 37d88ddb6ee7..5cee2748983b 100644 > --- a/drivers/media/pci/intel/ipu6/ipu6-bus.c > +++ b/drivers/media/pci/intel/ipu6/ipu6-bus.c > @@ -82,7 +82,7 @@ static void ipu6_bus_release(struct device *dev) > > struct ipu6_bus_device * > ipu6_bus_initialize_device(struct pci_dev *pdev, struct device *parent, > - void *pdata, struct ipu6_buttress_ctrl *ctrl, > + void *pdata, const struct ipu6_buttress_ctrl *ctrl, > char *name) > { > struct auxiliary_device *auxdev; > diff --git a/drivers/media/pci/intel/ipu6/ipu6-bus.h b/drivers/media/pci/intel/ipu6/ipu6-bus.h > index ebf470806a74..b790f9cc37e3 100644 > --- a/drivers/media/pci/intel/ipu6/ipu6-bus.h > +++ b/drivers/media/pci/intel/ipu6/ipu6-bus.h > @@ -25,7 +25,7 @@ struct ipu6_bus_device { > void *pdata; > struct ipu6_mmu *mmu; > struct ipu6_device *isp; > - struct ipu6_buttress_ctrl *ctrl; > + const struct ipu6_buttress_ctrl *ctrl; > u64 dma_mask; > const struct firmware *fw; > struct sg_table fw_sgt; > @@ -48,7 +48,7 @@ struct ipu6_auxdrv_data { > > struct ipu6_bus_device * > ipu6_bus_initialize_device(struct pci_dev *pdev, struct device *parent, > - void *pdata, struct ipu6_buttress_ctrl *ctrl, > + void *pdata, const struct ipu6_buttress_ctrl *ctrl, pdata should be const, too, btw. > char *name); > int ipu6_bus_add_device(struct ipu6_bus_device *adev); > void ipu6_bus_del_devices(struct pci_dev *pdev); > diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c > index 787fcbd1df09..f8fdc07a953c 100644 > --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c > +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c > @@ -443,7 +443,7 @@ irqreturn_t ipu6_buttress_isr_threaded(int irq, void *isp_ptr) > return ret; > } > > -int ipu6_buttress_power(struct device *dev, struct ipu6_buttress_ctrl *ctrl, > +int ipu6_buttress_power(struct device *dev, const struct ipu6_buttress_ctrl *ctrl, > bool on) But this is over 80. > { > struct ipu6_device *isp = to_ipu6_bus_device(dev)->isp; > diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.h b/drivers/media/pci/intel/ipu6/ipu6-buttress.h > index 4b9763acdfdd..cb008964f870 100644 > --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.h > +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.h > @@ -65,7 +65,8 @@ int ipu6_buttress_map_fw_image(struct ipu6_bus_device *sys, > struct sg_table *sgt); > void ipu6_buttress_unmap_fw_image(struct ipu6_bus_device *sys, > struct sg_table *sgt); > -int ipu6_buttress_power(struct device *dev, struct ipu6_buttress_ctrl *ctrl, > +int ipu6_buttress_power(struct device *dev, > + const struct ipu6_buttress_ctrl *ctrl, > bool on); And this fits on the previous line, too. > bool ipu6_buttress_get_secure_mode(struct ipu6_device *isp); > int ipu6_buttress_authenticate(struct ipu6_device *isp);
Hi Sakari, On Fri, Mar 07, 2025 at 07:45:03AM +0000, Sakari Ailus wrote: > > ipu6_bus_initialize_device(struct pci_dev *pdev, struct device *parent, > > - void *pdata, struct ipu6_buttress_ctrl *ctrl, > > + void *pdata, const struct ipu6_buttress_ctrl *ctrl, > > pdata should be const, too, btw. > > > char *name); > > int ipu6_bus_add_device(struct ipu6_bus_device *adev); > > void ipu6_bus_del_devices(struct pci_dev *pdev); > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c > > index 787fcbd1df09..f8fdc07a953c 100644 > > --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c > > +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c > > @@ -443,7 +443,7 @@ irqreturn_t ipu6_buttress_isr_threaded(int irq, void *isp_ptr) > > return ret; > > } > > > > -int ipu6_buttress_power(struct device *dev, struct ipu6_buttress_ctrl *ctrl, > > +int ipu6_buttress_power(struct device *dev, const struct ipu6_buttress_ctrl *ctrl, > > bool on) > > But this is over 80. On official kernel doc the limit is 100 (with 80 being preferred). I run chackpatch.pl on this patch and it was just fine. However clang-format change this to: int ipu6_buttress_power(struct device *dev, const struct ipu6_buttress_ctrl *ctrl, bool on) which is less than 80 characters. So I guess I need to use auto formatter for lines I change (for whole file clang-format change lot unrelated things). Regards Stanislaw
Hi Stanislaw, On Mon, Mar 10, 2025 at 09:37:55AM +0100, Stanislaw Gruszka wrote: > Hi Sakari, > > On Fri, Mar 07, 2025 at 07:45:03AM +0000, Sakari Ailus wrote: > > > ipu6_bus_initialize_device(struct pci_dev *pdev, struct device *parent, > > > - void *pdata, struct ipu6_buttress_ctrl *ctrl, > > > + void *pdata, const struct ipu6_buttress_ctrl *ctrl, > > > > pdata should be const, too, btw. > > > > > char *name); > > > int ipu6_bus_add_device(struct ipu6_bus_device *adev); > > > void ipu6_bus_del_devices(struct pci_dev *pdev); > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c > > > index 787fcbd1df09..f8fdc07a953c 100644 > > > --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c > > > @@ -443,7 +443,7 @@ irqreturn_t ipu6_buttress_isr_threaded(int irq, void *isp_ptr) > > > return ret; > > > } > > > > > > -int ipu6_buttress_power(struct device *dev, struct ipu6_buttress_ctrl *ctrl, > > > +int ipu6_buttress_power(struct device *dev, const struct ipu6_buttress_ctrl *ctrl, > > > bool on) > > > > But this is over 80. > > On official kernel doc the limit is 100 (with 80 being preferred). > I run chackpatch.pl on this patch and it was just fine. The Media tree driver documentation suggests: $ ./scripts/checkpatch.pl --strict --max-line-length=80 > > However clang-format change this to: > > int ipu6_buttress_power(struct device *dev, > const struct ipu6_buttress_ctrl *ctrl, bool on) > > which is less than 80 characters. So I guess I need to use auto formatter > for lines I change (for whole file clang-format change lot unrelated things).
On Mon, Mar 10, 2025 at 10:30:40AM +0000, Sakari Ailus wrote: > Hi Stanislaw, > > On Mon, Mar 10, 2025 at 09:37:55AM +0100, Stanislaw Gruszka wrote: > > Hi Sakari, > > > > On Fri, Mar 07, 2025 at 07:45:03AM +0000, Sakari Ailus wrote: > > > > ipu6_bus_initialize_device(struct pci_dev *pdev, struct device *parent, > > > > - void *pdata, struct ipu6_buttress_ctrl *ctrl, > > > > + void *pdata, const struct ipu6_buttress_ctrl *ctrl, > > > > > > pdata should be const, too, btw. > > > > > > > char *name); > > > > int ipu6_bus_add_device(struct ipu6_bus_device *adev); > > > > void ipu6_bus_del_devices(struct pci_dev *pdev); > > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c > > > > index 787fcbd1df09..f8fdc07a953c 100644 > > > > --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c > > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c > > > > @@ -443,7 +443,7 @@ irqreturn_t ipu6_buttress_isr_threaded(int irq, void *isp_ptr) > > > > return ret; > > > > } > > > > > > > > -int ipu6_buttress_power(struct device *dev, struct ipu6_buttress_ctrl *ctrl, > > > > +int ipu6_buttress_power(struct device *dev, const struct ipu6_buttress_ctrl *ctrl, > > > > bool on) > > > > > > But this is over 80. > > > > On official kernel doc the limit is 100 (with 80 being preferred). > > I run chackpatch.pl on this patch and it was just fine. > > The Media tree driver documentation suggests: > > $ ./scripts/checkpatch.pl --strict --max-line-length=80 TBH, in context of ipu6 enforcing 80 characters instead of 100, frequently makes more harm then good IMHO, for example: const struct ipu6_isys_pixelformat ipu6_isys_pfmts[] = { { V4L2_PIX_FMT_SBGGR12, 16, 12, MEDIA_BUS_FMT_SBGGR12_1X12, IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, { V4L2_PIX_FMT_SGBRG12, 16, 12, MEDIA_BUS_FMT_SGBRG12_1X12, IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, { V4L2_PIX_FMT_SGRBG12, 16, 12, MEDIA_BUS_FMT_SGRBG12_1X12, IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, { V4L2_PIX_FMT_SRGGB12, 16, 12, MEDIA_BUS_FMT_SRGGB12_1X12, vs: const struct ipu6_isys_pixelformat ipu6_isys_pfmts[] = { { V4L2_PIX_FMT_SBGGR12, 16, 12, MEDIA_BUS_FMT_SBGGR12_1X12, IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, { V4L2_PIX_FMT_SGBRG12, 16, 12, MEDIA_BUS_FMT_SGBRG12_1X12, IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, { V4L2_PIX_FMT_SGRBG12, 16, 12, MEDIA_BUS_FMT_SGRBG12_1X12, IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, { V4L2_PIX_FMT_SRGGB12, 16, 12, MEDIA_BUS_FMT_SRGGB12_1X12, IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, Or: if (type && ((!pfmt->is_meta && type != V4L2_BUF_TYPE_VIDEO_CAPTURE) || (pfmt->is_meta && type != V4L2_BUF_TYPE_META_CAPTURE))) continue; vs: if (type && ((!pfmt->is_meta && type != V4L2_BUF_TYPE_VIDEO_CAPTURE) || (pfmt->is_meta && type != V4L2_BUF_TYPE_META_CAPTURE))) continue; Do we really need 80 chars limit in ipu drivers ? Regards Stanislaw
On Mon, Mar 10, 2025 at 12:18:25PM +0100, Stanislaw Gruszka wrote: > On Mon, Mar 10, 2025 at 10:30:40AM +0000, Sakari Ailus wrote: > > Hi Stanislaw, > > > > On Mon, Mar 10, 2025 at 09:37:55AM +0100, Stanislaw Gruszka wrote: > > > Hi Sakari, > > > > > > On Fri, Mar 07, 2025 at 07:45:03AM +0000, Sakari Ailus wrote: > > > > > ipu6_bus_initialize_device(struct pci_dev *pdev, struct device *parent, > > > > > - void *pdata, struct ipu6_buttress_ctrl *ctrl, > > > > > + void *pdata, const struct ipu6_buttress_ctrl *ctrl, > > > > > > > > pdata should be const, too, btw. > > > > > > > > > char *name); > > > > > int ipu6_bus_add_device(struct ipu6_bus_device *adev); > > > > > void ipu6_bus_del_devices(struct pci_dev *pdev); > > > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c > > > > > index 787fcbd1df09..f8fdc07a953c 100644 > > > > > --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c > > > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c > > > > > @@ -443,7 +443,7 @@ irqreturn_t ipu6_buttress_isr_threaded(int irq, void *isp_ptr) > > > > > return ret; > > > > > } > > > > > > > > > > -int ipu6_buttress_power(struct device *dev, struct ipu6_buttress_ctrl *ctrl, > > > > > +int ipu6_buttress_power(struct device *dev, const struct ipu6_buttress_ctrl *ctrl, > > > > > bool on) > > > > > > > > But this is over 80. > > > > > > On official kernel doc the limit is 100 (with 80 being preferred). > > > I run chackpatch.pl on this patch and it was just fine. > > > > The Media tree driver documentation suggests: > > > > $ ./scripts/checkpatch.pl --strict --max-line-length=80 > > TBH, in context of ipu6 enforcing 80 characters instead of 100, > frequently makes more harm then good IMHO, for example: > > const struct ipu6_isys_pixelformat ipu6_isys_pfmts[] = { > { V4L2_PIX_FMT_SBGGR12, 16, 12, MEDIA_BUS_FMT_SBGGR12_1X12, > IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, > { V4L2_PIX_FMT_SGBRG12, 16, 12, MEDIA_BUS_FMT_SGBRG12_1X12, > IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, > { V4L2_PIX_FMT_SGRBG12, 16, 12, MEDIA_BUS_FMT_SGRBG12_1X12, > IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, > { V4L2_PIX_FMT_SRGGB12, 16, 12, MEDIA_BUS_FMT_SRGGB12_1X12, > vs: > > const struct ipu6_isys_pixelformat ipu6_isys_pfmts[] = { > { V4L2_PIX_FMT_SBGGR12, 16, 12, MEDIA_BUS_FMT_SBGGR12_1X12, IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, > { V4L2_PIX_FMT_SGBRG12, 16, 12, MEDIA_BUS_FMT_SGBRG12_1X12, IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, > { V4L2_PIX_FMT_SGRBG12, 16, 12, MEDIA_BUS_FMT_SGRBG12_1X12, IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, > { V4L2_PIX_FMT_SRGGB12, 16, 12, MEDIA_BUS_FMT_SRGGB12_1X12, IPU6_FW_ISYS_FRAME_FORMAT_RAW16 }, > > > Or: > if (type && ((!pfmt->is_meta && > type != V4L2_BUF_TYPE_VIDEO_CAPTURE) || > (pfmt->is_meta && > type != V4L2_BUF_TYPE_META_CAPTURE))) > continue; > > vs: > > if (type && ((!pfmt->is_meta && type != V4L2_BUF_TYPE_VIDEO_CAPTURE) || > (pfmt->is_meta && type != V4L2_BUF_TYPE_META_CAPTURE))) > continue; > > > Do we really need 80 chars limit in ipu drivers ? In the former case I'd keep data related to an array index on the same line, they're often more readable like that. It's not a strict rule. In the latter case wrapping after first && may be more readable than either of the two.
diff --git a/drivers/media/pci/intel/ipu6/ipu6-bus.c b/drivers/media/pci/intel/ipu6/ipu6-bus.c index 37d88ddb6ee7..5cee2748983b 100644 --- a/drivers/media/pci/intel/ipu6/ipu6-bus.c +++ b/drivers/media/pci/intel/ipu6/ipu6-bus.c @@ -82,7 +82,7 @@ static void ipu6_bus_release(struct device *dev) struct ipu6_bus_device * ipu6_bus_initialize_device(struct pci_dev *pdev, struct device *parent, - void *pdata, struct ipu6_buttress_ctrl *ctrl, + void *pdata, const struct ipu6_buttress_ctrl *ctrl, char *name) { struct auxiliary_device *auxdev; diff --git a/drivers/media/pci/intel/ipu6/ipu6-bus.h b/drivers/media/pci/intel/ipu6/ipu6-bus.h index ebf470806a74..b790f9cc37e3 100644 --- a/drivers/media/pci/intel/ipu6/ipu6-bus.h +++ b/drivers/media/pci/intel/ipu6/ipu6-bus.h @@ -25,7 +25,7 @@ struct ipu6_bus_device { void *pdata; struct ipu6_mmu *mmu; struct ipu6_device *isp; - struct ipu6_buttress_ctrl *ctrl; + const struct ipu6_buttress_ctrl *ctrl; u64 dma_mask; const struct firmware *fw; struct sg_table fw_sgt; @@ -48,7 +48,7 @@ struct ipu6_auxdrv_data { struct ipu6_bus_device * ipu6_bus_initialize_device(struct pci_dev *pdev, struct device *parent, - void *pdata, struct ipu6_buttress_ctrl *ctrl, + void *pdata, const struct ipu6_buttress_ctrl *ctrl, char *name); int ipu6_bus_add_device(struct ipu6_bus_device *adev); void ipu6_bus_del_devices(struct pci_dev *pdev); diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c index 787fcbd1df09..f8fdc07a953c 100644 --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c @@ -443,7 +443,7 @@ irqreturn_t ipu6_buttress_isr_threaded(int irq, void *isp_ptr) return ret; } -int ipu6_buttress_power(struct device *dev, struct ipu6_buttress_ctrl *ctrl, +int ipu6_buttress_power(struct device *dev, const struct ipu6_buttress_ctrl *ctrl, bool on) { struct ipu6_device *isp = to_ipu6_bus_device(dev)->isp; diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.h b/drivers/media/pci/intel/ipu6/ipu6-buttress.h index 4b9763acdfdd..cb008964f870 100644 --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.h +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.h @@ -65,7 +65,8 @@ int ipu6_buttress_map_fw_image(struct ipu6_bus_device *sys, struct sg_table *sgt); void ipu6_buttress_unmap_fw_image(struct ipu6_bus_device *sys, struct sg_table *sgt); -int ipu6_buttress_power(struct device *dev, struct ipu6_buttress_ctrl *ctrl, +int ipu6_buttress_power(struct device *dev, + const struct ipu6_buttress_ctrl *ctrl, bool on); bool ipu6_buttress_get_secure_mode(struct ipu6_device *isp); int ipu6_buttress_authenticate(struct ipu6_device *isp);
Make ipu6_buttress_ctrl constant since it is not modified any longer. Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> --- drivers/media/pci/intel/ipu6/ipu6-bus.c | 2 +- drivers/media/pci/intel/ipu6/ipu6-bus.h | 4 ++-- drivers/media/pci/intel/ipu6/ipu6-buttress.c | 2 +- drivers/media/pci/intel/ipu6/ipu6-buttress.h | 3 ++- 4 files changed, 6 insertions(+), 5 deletions(-)