| Message ID | 20220122163656.168440-2-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Set bus_info field in framework | expand |
Hi Sakari, Thank you for the patch. On Sat, Jan 22, 2022 at 06:36:54PM +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 1345e6da688a..9f0458068196 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 > @@ -496,4 +496,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) > > +static inline void > +__media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev) > +{ > + if (!dev || *bus_info) > + return; > + > + 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)); > +} Does this have to be inline ? > + > +/** > + * media_set_bus_info() - Conditionally set bus_info > + * > + * @bus_info: Variable where to write the bus info (char array) > + * @dev: Related struct device > + * > + * Sets bus information based on device conditionally, if the first character of > + * &bus_info is not '\0' and dev is non-NULL. > + */ > +#define media_set_bus_info(bus_info, dev) \ > + __media_set_bus_info(bus_info, sizeof(bus_info), dev) I like the idea, but if the bus_info passed to the macro is a char * instead of a char[], I think this will still compile, with sizeof(bus_info) not giving the expected value. Could we either get a compilation failure in that case, of maybe turn this into two inline functions, one for media_device and the other one for v4l2_capability, that would both call __media_set_bus_info() ? The latter may be better. > + > #endif
Hi Laurent, Thanks for the review. On Sun, Jan 23, 2022 at 01:44:02AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Sat, Jan 22, 2022 at 06:36:54PM +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 1345e6da688a..9f0458068196 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 > > @@ -496,4 +496,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) > > > > +static inline void > > +__media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev) > > +{ > > + if (!dev || *bus_info) > > + return; > > + > > + 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)); > > +} > > Does this have to be inline ? Not necessarily. But we'll need a new module if it isn't --- this code will be needed in both MC and V4L2 separately. > > > + > > +/** > > + * media_set_bus_info() - Conditionally set bus_info > > + * > > + * @bus_info: Variable where to write the bus info (char array) > > + * @dev: Related struct device > > + * > > + * Sets bus information based on device conditionally, if the first character of > > + * &bus_info is not '\0' and dev is non-NULL. > > + */ > > +#define media_set_bus_info(bus_info, dev) \ > > + __media_set_bus_info(bus_info, sizeof(bus_info), dev) > > I like the idea, but if the bus_info passed to the macro is a char * > instead of a char[], I think this will still compile, with > sizeof(bus_info) not giving the expected value. Could we either get a > compilation failure in that case, of maybe turn this into two inline > functions, one for media_device and the other one for v4l2_capability, > that would both call __media_set_bus_info() ? The latter may be better. The latter, yes. There will be so few users that's entirely fine.
Hi Sakari, On 22/01/2022 17:36, 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 1345e6da688a..9f0458068196 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 > @@ -496,4 +496,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) > > +static inline void > +__media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev) > +{ > + if (!dev || *bus_info) > + return; > + > + 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)); > +} > + > +/** > + * media_set_bus_info() - Conditionally set bus_info > + * > + * @bus_info: Variable where to write the bus info (char array) > + * @dev: Related struct device > + * > + * Sets bus information based on device conditionally, if the first character of > + * &bus_info is not '\0' and dev is non-NULL. > + */ > +#define media_set_bus_info(bus_info, dev) \ > + __media_set_bus_info(bus_info, sizeof(bus_info), dev) Wouldn't it be simpler to make two #defines: #define media_set_bus_info(mdev, dev) \ __media_set_bus_info(mdev->bus_info, sizeof(mdev->bus_info), dev) and: #define v4l2_cap_set_bus_info(cap, dev) \ __media_set_bus_info(cap->bus_info, sizeof(cap->bus_info), dev) That way the sizeof() always works correctly. This could also be static inlines to have better type checking, of course. Another option is: #define media_set_bus_info(s, dev) \ __media_set_bus_info((s)->bus_info, sizeof((s)->bus_info), dev) That's more generic, but it does make the assumption that the struct s has a field bus_info. Which is a reasonable assumption IMHO. I do like the idea of this series. Regards, Hans > + > #endif
Hi Hans, On Tue, Jan 25, 2022 at 01:54:45PM +0100, Hans Verkuil wrote: > Hi Sakari, > > On 22/01/2022 17:36, 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 1345e6da688a..9f0458068196 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 > > @@ -496,4 +496,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) > > > > +static inline void > > +__media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev) > > +{ > > + if (!dev || *bus_info) > > + return; > > + > > + 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)); > > +} > > + > > +/** > > + * media_set_bus_info() - Conditionally set bus_info > > + * > > + * @bus_info: Variable where to write the bus info (char array) > > + * @dev: Related struct device > > + * > > + * Sets bus information based on device conditionally, if the first character of > > + * &bus_info is not '\0' and dev is non-NULL. > > + */ > > +#define media_set_bus_info(bus_info, dev) \ > > + __media_set_bus_info(bus_info, sizeof(bus_info), dev) > > Wouldn't it be simpler to make two #defines: > > #define media_set_bus_info(mdev, dev) \ > __media_set_bus_info(mdev->bus_info, sizeof(mdev->bus_info), dev) > > and: > > #define v4l2_cap_set_bus_info(cap, dev) \ > __media_set_bus_info(cap->bus_info, sizeof(cap->bus_info), dev) > > That way the sizeof() always works correctly. > > This could also be static inlines to have better type checking, of course. > > Another option is: > > #define media_set_bus_info(s, dev) \ > __media_set_bus_info((s)->bus_info, sizeof((s)->bus_info), dev) > > That's more generic, but it does make the assumption that the struct s > has a field bus_info. Which is a reasonable assumption IMHO. > > I do like the idea of this series. Thanks for the comments. I prefer Laurent's suggestion of removing the macro and simply using sizeof() in the caller. Note that there will be a very small number of these calls and so far none in the drivers (nor there should be any).
diff --git a/include/media/media-device.h b/include/media/media-device.h index 1345e6da688a..9f0458068196 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 @@ -496,4 +496,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) +static inline void +__media_set_bus_info(char *bus_info, size_t bus_info_size, struct device *dev) +{ + if (!dev || *bus_info) + return; + + 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)); +} + +/** + * media_set_bus_info() - Conditionally set bus_info + * + * @bus_info: Variable where to write the bus info (char array) + * @dev: Related struct device + * + * Sets bus information based on device conditionally, if the first character of + * &bus_info is not '\0' and dev is non-NULL. + */ +#define media_set_bus_info(bus_info, dev) \ + __media_set_bus_info(bus_info, sizeof(bus_info), dev) + #endif
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(-)