| Message ID | 20220309163112.11708-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Set bus_info field in framework | expand |
Hi Sakari, Thank you for the patch. On Wed, Mar 09, 2022 at 06:31:10PM +0200, Sakari Ailus wrote: > The bus_info or a similar field exists in a lot of structs, yet drivers > tend to set the value of that field by themselves in a determinable way. > Thus provide a helper for doing this. To be used in subsequent patches. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > include/media/media-device.h | 30 +++++++++++++++++++++++++++--- > 1 file changed, 27 insertions(+), 3 deletions(-) > > diff --git a/include/media/media-device.h b/include/media/media-device.h > index bc015d2cf541..2122d15bde4e 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -13,12 +13,13 @@ > > #include <linux/list.h> > #include <linux/mutex.h> > +#include <linux/pci.h> > +#include <linux/platform_device.h> > > #include <media/media-devnode.h> > #include <media/media-entity.h> > > struct ida; > -struct device; > struct media_device; > > /** > @@ -181,8 +182,7 @@ struct media_device { > atomic_t request_id; > }; > > -/* We don't need to include pci.h or usb.h here */ > -struct pci_dev; > +/* We don't need to include usb.h here */ > struct usb_device; > > #ifdef CONFIG_MEDIA_CONTROLLER > @@ -502,4 +502,28 @@ static inline void __media_device_usb_init(struct media_device *mdev, > #define media_device_usb_init(mdev, udev, name) \ > __media_device_usb_init(mdev, udev, name, KBUILD_MODNAME) > > + > +/** > + * media_set_bus_info() - Set bus_info field > + * > + * @bus_info: Variable where to write the bus info (char array) > + * @bus_info_size: Length of the bus_info > + * @dev: Related struct device > + * > + * Sets bus information based on &dev. This is currently done for PCI and > + * platform devices. dev is required to be non-NULL for this to happen. > + * > + * This function is not meant to be called from drivers. > + */ > +static inline void > +media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev) > +{ > + if (!dev) > + strscpy(bus_info, "no bus info", bus_info_size); > + else if (dev_is_platform(dev)) > + snprintf(bus_info, bus_info_size, "platform:%s", dev_name(dev)); > + else if (dev_is_pci(dev)) > + snprintf(bus_info, bus_info_size, "PCI:%s", dev_name(dev)); > +} I wouldn't inline this, as it's not used in any hot path, and will generate quite a bit of code. Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > #endif
Hi Sakari, Thank you for the patch. On Wed, Mar 09, 2022 at 06:31:11PM +0200, Sakari Ailus wrote: > Set bus_info field based on struct device in media_device_init() and > remove corresponding code from drivers. > > Also update media_device_init() documentation: the dev field must be now > initialised before calling it. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > drivers/media/mc/mc-device.c | 4 ++++ > drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 2 -- > drivers/media/platform/rcar-vin/rcar-core.c | 2 -- > drivers/media/platform/stm32/stm32-dcmi.c | 2 -- > drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c | 2 -- > drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c | 2 -- > drivers/media/platform/ti-vpe/cal.c | 2 -- > drivers/media/platform/vsp1/vsp1_drv.c | 2 -- > include/media/media-device.h | 6 +++--- > 9 files changed, 7 insertions(+), 17 deletions(-) > > diff --git a/drivers/media/mc/mc-device.c b/drivers/media/mc/mc-device.c > index 094647fdb866..824d89b325a6 100644 > --- a/drivers/media/mc/mc-device.c > +++ b/drivers/media/mc/mc-device.c > @@ -700,6 +700,10 @@ void media_device_init(struct media_device *mdev) > > atomic_set(&mdev->request_id, 0); > > + if (!*mdev->bus_info) if (!mdev->bus_info[0]) could be a bit more readable, up to you. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + media_set_bus_info(mdev->bus_info, sizeof(mdev->bus_info), > + mdev->dev); > + > dev_dbg(mdev->dev, "Media device initialized\n"); > } > EXPORT_SYMBOL_GPL(media_device_init); > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c > index 0e9b0503b62a..b15fac775e14 100644 > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c > @@ -1777,8 +1777,6 @@ static int cio2_pci_probe(struct pci_dev *pci_dev, > cio2->media_dev.dev = dev; > strscpy(cio2->media_dev.model, CIO2_DEVICE_NAME, > sizeof(cio2->media_dev.model)); > - snprintf(cio2->media_dev.bus_info, sizeof(cio2->media_dev.bus_info), > - "PCI:%s", pci_name(cio2->pci_dev)); > cio2->media_dev.hw_revision = 0; > > media_device_init(&cio2->media_dev); > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index 0186ae235113..1099cab7d95d 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -94,8 +94,6 @@ static int rvin_group_init(struct rvin_group *group, struct rvin_dev *vin, > > strscpy(mdev->driver_name, KBUILD_MODNAME, sizeof(mdev->driver_name)); > strscpy(mdev->model, match->compatible, sizeof(mdev->model)); > - snprintf(mdev->bus_info, sizeof(mdev->bus_info), "platform:%s", > - dev_name(mdev->dev)); > > media_device_init(mdev); > > diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c > index c4c65d852525..09a743cd7004 100644 > --- a/drivers/media/platform/stm32/stm32-dcmi.c > +++ b/drivers/media/platform/stm32/stm32-dcmi.c > @@ -1997,8 +1997,6 @@ static int dcmi_probe(struct platform_device *pdev) > > /* Initialize media device */ > strscpy(dcmi->mdev.model, DRV_NAME, sizeof(dcmi->mdev.model)); > - snprintf(dcmi->mdev.bus_info, sizeof(dcmi->mdev.bus_info), > - "platform:%s", DRV_NAME); > dcmi->mdev.dev = &pdev->dev; > media_device_init(&dcmi->mdev); > > diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c > index 80a10f238bbe..18e6c65f4737 100644 > --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c > +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_csi.c > @@ -173,8 +173,6 @@ static int sun4i_csi_probe(struct platform_device *pdev) > strscpy(csi->mdev.model, "Allwinner Video Capture Device", > sizeof(csi->mdev.model)); > csi->mdev.hw_revision = 0; > - snprintf(csi->mdev.bus_info, sizeof(csi->mdev.bus_info), "platform:%s", > - dev_name(csi->dev)); > media_device_init(&csi->mdev); > csi->v4l.mdev = &csi->mdev; > > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > index fc96921b0583..a971587dbbd1 100644 > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c > @@ -733,8 +733,6 @@ static int sun6i_csi_v4l2_init(struct sun6i_csi *csi) > strscpy(csi->media_dev.model, "Allwinner Video Capture Device", > sizeof(csi->media_dev.model)); > csi->media_dev.hw_revision = 0; > - snprintf(csi->media_dev.bus_info, sizeof(csi->media_dev.bus_info), > - "platform:%s", dev_name(csi->dev)); > > media_device_init(&csi->media_dev); > v4l2_async_nf_init(&csi->notifier); > diff --git a/drivers/media/platform/ti-vpe/cal.c b/drivers/media/platform/ti-vpe/cal.c > index 4a4a6c5983f7..11f67abc2f38 100644 > --- a/drivers/media/platform/ti-vpe/cal.c > +++ b/drivers/media/platform/ti-vpe/cal.c > @@ -884,8 +884,6 @@ static int cal_media_init(struct cal_dev *cal) > mdev->dev = cal->dev; > mdev->hw_revision = cal->revision; > strscpy(mdev->model, "CAL", sizeof(mdev->model)); > - snprintf(mdev->bus_info, sizeof(mdev->bus_info), "platform:%s", > - dev_name(mdev->dev)); > media_device_init(mdev); > > /* > diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c > index 502c7d9d6890..1f73c48eb738 100644 > --- a/drivers/media/platform/vsp1/vsp1_drv.c > +++ b/drivers/media/platform/vsp1/vsp1_drv.c > @@ -243,8 +243,6 @@ static int vsp1_create_entities(struct vsp1_device *vsp1) > mdev->dev = vsp1->dev; > mdev->hw_revision = vsp1->version; > strscpy(mdev->model, vsp1->info->model, sizeof(mdev->model)); > - snprintf(mdev->bus_info, sizeof(mdev->bus_info), "platform:%s", > - dev_name(mdev->dev)); > media_device_init(mdev); > > vsp1->media_ops.link_setup = vsp1_entity_link_setup; > diff --git a/include/media/media-device.h b/include/media/media-device.h > index 2122d15bde4e..9e71a951f412 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -225,6 +225,9 @@ static inline __must_check int media_entity_enum_init( > * > * - dev must point to the parent device > * - model must be filled with the device model name > + * > + * The bus_info field is set by media_device_init() for PCI and platform devices > + * if the field begins with '\0'. > */ > void media_device_init(struct media_device *mdev); > > @@ -249,9 +252,6 @@ void media_device_cleanup(struct media_device *mdev); > * The caller is responsible for initializing the &media_device structure > * before registration. The following fields of &media_device must be set: > * > - * - &media_device.dev must point to the parent device (usually a &pci_dev, > - * &usb_interface or &platform_device instance). > - * > * - &media_device.model must be filled with the device model name as a > * NUL-terminated UTF-8 string. The device/model revision must not be > * stored in this field.
On Wed, Mar 16, 2022 at 11:18:13AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Wed, Mar 09, 2022 at 06:31:10PM +0200, Sakari Ailus wrote: > > The bus_info or a similar field exists in a lot of structs, yet drivers > > tend to set the value of that field by themselves in a determinable way. > > Thus provide a helper for doing this. To be used in subsequent patches. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > include/media/media-device.h | 30 +++++++++++++++++++++++++++--- > > 1 file changed, 27 insertions(+), 3 deletions(-) > > > > diff --git a/include/media/media-device.h b/include/media/media-device.h > > index bc015d2cf541..2122d15bde4e 100644 > > --- a/include/media/media-device.h > > +++ b/include/media/media-device.h > > @@ -13,12 +13,13 @@ > > > > #include <linux/list.h> > > #include <linux/mutex.h> > > +#include <linux/pci.h> > > +#include <linux/platform_device.h> > > > > #include <media/media-devnode.h> > > #include <media/media-entity.h> > > > > struct ida; > > -struct device; > > struct media_device; > > > > /** > > @@ -181,8 +182,7 @@ struct media_device { > > atomic_t request_id; > > }; > > > > -/* We don't need to include pci.h or usb.h here */ > > -struct pci_dev; > > +/* We don't need to include usb.h here */ > > struct usb_device; > > > > #ifdef CONFIG_MEDIA_CONTROLLER > > @@ -502,4 +502,28 @@ static inline void __media_device_usb_init(struct media_device *mdev, > > #define media_device_usb_init(mdev, udev, name) \ > > __media_device_usb_init(mdev, udev, name, KBUILD_MODNAME) > > > > + > > +/** > > + * media_set_bus_info() - Set bus_info field > > + * > > + * @bus_info: Variable where to write the bus info (char array) > > + * @bus_info_size: Length of the bus_info > > + * @dev: Related struct device > > + * > > + * Sets bus information based on &dev. This is currently done for PCI and > > + * platform devices. dev is required to be non-NULL for this to happen. > > + * > > + * This function is not meant to be called from drivers. > > + */ > > +static inline void > > +media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev) > > +{ > > + if (!dev) > > + strscpy(bus_info, "no bus info", bus_info_size); > > + else if (dev_is_platform(dev)) > > + snprintf(bus_info, bus_info_size, "platform:%s", dev_name(dev)); > > + else if (dev_is_pci(dev)) > > + snprintf(bus_info, bus_info_size, "PCI:%s", dev_name(dev)); > > +} > > I wouldn't inline this, as it's not used in any hot path, and will > generate quite a bit of code. Apart from that, But the function is only called in two places, and we'd have to export it, and handle the case where MC is disabled. Possibly not worth it, although it would be nice to not inline it if possible. If there's a suitable location to make that easy let's do so, otherwise you can keep it inline. (By the way, at some point we may want to not make MC optional) > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + > > #endif
On Wed, Mar 16, 2022 at 11:24:32AM +0200, Laurent Pinchart wrote: > On Wed, Mar 16, 2022 at 11:18:13AM +0200, Laurent Pinchart wrote: > > Hi Sakari, > > > > Thank you for the patch. > > > > On Wed, Mar 09, 2022 at 06:31:10PM +0200, Sakari Ailus wrote: > > > The bus_info or a similar field exists in a lot of structs, yet drivers > > > tend to set the value of that field by themselves in a determinable way. > > > Thus provide a helper for doing this. To be used in subsequent patches. > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > --- > > > include/media/media-device.h | 30 +++++++++++++++++++++++++++--- > > > 1 file changed, 27 insertions(+), 3 deletions(-) > > > > > > diff --git a/include/media/media-device.h b/include/media/media-device.h > > > index bc015d2cf541..2122d15bde4e 100644 > > > --- a/include/media/media-device.h > > > +++ b/include/media/media-device.h > > > @@ -13,12 +13,13 @@ > > > > > > #include <linux/list.h> > > > #include <linux/mutex.h> > > > +#include <linux/pci.h> > > > +#include <linux/platform_device.h> > > > > > > #include <media/media-devnode.h> > > > #include <media/media-entity.h> > > > > > > struct ida; > > > -struct device; > > > struct media_device; > > > > > > /** > > > @@ -181,8 +182,7 @@ struct media_device { > > > atomic_t request_id; > > > }; > > > > > > -/* We don't need to include pci.h or usb.h here */ > > > -struct pci_dev; > > > +/* We don't need to include usb.h here */ > > > struct usb_device; > > > > > > #ifdef CONFIG_MEDIA_CONTROLLER > > > @@ -502,4 +502,28 @@ static inline void __media_device_usb_init(struct media_device *mdev, > > > #define media_device_usb_init(mdev, udev, name) \ > > > __media_device_usb_init(mdev, udev, name, KBUILD_MODNAME) > > > > > > + > > > +/** > > > + * media_set_bus_info() - Set bus_info field > > > + * > > > + * @bus_info: Variable where to write the bus info (char array) > > > + * @bus_info_size: Length of the bus_info > > > + * @dev: Related struct device > > > + * > > > + * Sets bus information based on &dev. This is currently done for PCI and > > > + * platform devices. dev is required to be non-NULL for this to happen. > > > + * > > > + * This function is not meant to be called from drivers. > > > + */ > > > +static inline void > > > +media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev) > > > +{ > > > + if (!dev) > > > + strscpy(bus_info, "no bus info", bus_info_size); > > > + else if (dev_is_platform(dev)) > > > + snprintf(bus_info, bus_info_size, "platform:%s", dev_name(dev)); > > > + else if (dev_is_pci(dev)) > > > + snprintf(bus_info, bus_info_size, "PCI:%s", dev_name(dev)); > > > +} > > > > I wouldn't inline this, as it's not used in any hot path, and will > > generate quite a bit of code. Apart from that, > > But the function is only called in two places, and we'd have to export > it, and handle the case where MC is disabled. Possibly not worth it, > although it would be nice to not inline it if possible. If there's a > suitable location to make that easy let's do so, otherwise you can keep > it inline. There's no such location currently. If one will be added, this should be moved there. But it's not really worth a new kernel module at the moment. > > (By the way, at some point we may want to not make MC optional) Yes. > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks!