Re: [PATCH 2/2] [media] media-device: split media initialization and registration
Hi Mauro, On Tue, Dec 15, 2015 at 09:13:42AM -0200, Mauro Carvalho Chehab wrote: > Em Thu, 10 Sep 2015 20:14:04 +0300 > Sakari Ailus escreveu: > > > Hi Javier, > > > > Thanks for the set! A few comments below. > > > > Javier Martinez Canillas wrote: > > > The media device node is registered and so made visible to user-space > > > before entities are registered and links created which means that the > > > media graph obtained by user-space could be only partially enumerated > > > if that happens too early before all the graph has been created. > > > > > > To avoid this race condition, split the media init and registration > > > in separate functions and only register the media device node when > > > all the pending subdevices have been registered, either explicitly > > > by the driver or asynchronously using v4l2_async_register_subdev(). > > > > > > Also, add a media_entity_cleanup() function that will destroy the > > > graph_mutex that is initialized in media_entity_init(). > > > > > > Suggested-by: Sakari Ailus > > > Signed-off-by: Javier Martinez Canillas > > > > > > --- > > > > > > drivers/media/common/siano/smsdvb-main.c | 1 + > > > drivers/media/media-device.c | 38 > > > +++ > > > drivers/media/platform/exynos4-is/media-dev.c | 12 ++--- > > > drivers/media/platform/omap3isp/isp.c | 11 +--- > > > drivers/media/platform/s3c-camif/camif-core.c | 13 ++--- > > > drivers/media/platform/vsp1/vsp1_drv.c| 19 ++ > > > drivers/media/platform/xilinx/xilinx-vipp.c | 11 +--- > > > drivers/media/usb/au0828/au0828-core.c| 26 +- > > > drivers/media/usb/cx231xx/cx231xx-cards.c | 22 +++- > > > drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 11 +--- > > > drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 13 ++--- > > > drivers/media/usb/siano/smsusb.c | 14 -- > > > drivers/media/usb/uvc/uvc_driver.c| 9 +-- > > > include/media/media-device.h | 2 ++ > > > 14 files changed, 156 insertions(+), 46 deletions(-) > > > > > > diff --git a/drivers/media/common/siano/smsdvb-main.c > > > b/drivers/media/common/siano/smsdvb-main.c > > > index ab345490a43a..8a1ea2192439 100644 > > > --- a/drivers/media/common/siano/smsdvb-main.c > > > +++ b/drivers/media/common/siano/smsdvb-main.c > > > @@ -617,6 +617,7 @@ static void smsdvb_media_device_unregister(struct > > > smsdvb_client_t *client) > > > if (!coredev->media_dev) > > > return; > > > media_device_unregister(coredev->media_dev); > > > + media_device_cleanup(coredev->media_dev); > > > kfree(coredev->media_dev); > > > coredev->media_dev = NULL; > > > #endif > > > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c > > > index 745defb34b33..a8beb0b445a6 100644 > > > --- a/drivers/media/media-device.c > > > +++ b/drivers/media/media-device.c > > > @@ -526,7 +526,7 @@ static void media_device_release(struct media_devnode > > > *mdev) > > > } > > > > > > /** > > > - * media_device_register - register a media device > > > + * media_device_init() - initialize a media device > > > * @mdev:The media device > > > * > > > * The caller is responsible for initializing the media device before > > > @@ -534,12 +534,11 @@ static void media_device_release(struct > > > media_devnode *mdev) > > > * > > > * - dev must point to the parent device > > > * - model must be filled with the device model name > > > + * > > > + * returns zero on success or a negative error code. > > > */ > > > -int __must_check __media_device_register(struct media_device *mdev, > > > - struct module *owner) > > > +int __must_check media_device_init(struct media_device *mdev) > > > > I think I suggested making media_device_init() return void as the only > > remaining source of errors would be driver bugs. > > > > I'd simply replace the WARN_ON() below with BUG(). > > That sounds like bad idea to me, and it is against the current > Kernel policy of using BUG() only when there's no other way, e. g. on > event so severe that the Kernel has no other
[PATCH 1/1] media: Correctly determine whether an entity is a sub-device
If the function of an entity is not one of the pre-defined ones, it is not correctly recognised as a V4L2 sub-device. Signed-off-by: Sakari Ailus --- include/media/media-entity.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/media/media-entity.h b/include/media/media-entity.h index a60872a..76e9a124 100644 --- a/include/media/media-entity.h +++ b/include/media/media-entity.h @@ -328,6 +328,7 @@ static inline bool is_media_entity_v4l2_subdev(struct media_entity *entity) case MEDIA_ENT_F_LENS: case MEDIA_ENT_F_ATV_DECODER: case MEDIA_ENT_F_TUNER: + case MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN: return true; default: -- 2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 32/55] [media] media: use macros to check for V4L2 subdev entities
Hi Mauro,
On Sun, Oct 11, 2015 at 09:56:25PM -0300, Mauro Carvalho Chehab wrote:
> Em Mon, 12 Oct 2015 00:07:52 +0300
> Sakari Ailus escreveu:
>
> > Hi Mauro,
> >
> > On Sun, Aug 30, 2015 at 12:06:43AM -0300, Mauro Carvalho Chehab wrote:
> > > Instead of relying on media subtype, use the new macros to detect
> > > if an entity is a subdev or an A/V DMA entity.
> > >
> > > Please note that most drivers assume that there's just AV_DMA or
> > > V4L2 subdevs. This is not true anymore, as we've added MC support
> > > for DVB, and there are plans to add support for ALSA and FB/DRM
> > > too.
> > >
> > > Ok, on the current pipelines supported by those drivers, just V4L
> > > stuff are there, but, assuming that some day a pipeline that also
> > > works with other subsystems will ever added, it is better to add
> > > explicit checks for the AV_DMA stuff.
> > >
> > > Signed-off-by: Mauro Carvalho Chehab
> > >
> > > diff --git a/drivers/media/platform/exynos4-is/common.c
> > > b/drivers/media/platform/exynos4-is/common.c
> > > index 0eb34ecb8ee4..8c9a29e0e294 100644
> > > --- a/drivers/media/platform/exynos4-is/common.c
> > > +++ b/drivers/media/platform/exynos4-is/common.c
> > > @@ -22,8 +22,7 @@ struct v4l2_subdev *fimc_find_remote_sensor(struct
> > > media_entity *entity)
> > > while (pad->flags & MEDIA_PAD_FL_SINK) {
> > > /* source pad */
> > > pad = media_entity_remote_pad(pad);
> > > - if (pad == NULL ||
> > > - media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> > > + if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> > > break;
> > >
> > > sd = media_entity_to_v4l2_subdev(pad->entity);
> > > diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c
> > > b/drivers/media/platform/exynos4-is/fimc-capture.c
> > > index 0627a93b2f3b..e9810fee4c30 100644
> > > --- a/drivers/media/platform/exynos4-is/fimc-capture.c
> > > +++ b/drivers/media/platform/exynos4-is/fimc-capture.c
> > > @@ -1141,8 +1141,7 @@ static int fimc_pipeline_validate(struct fimc_dev
> > > *fimc)
> > > }
> > > }
> > >
> > > - if (src_pad == NULL ||
> > > - media_entity_type(src_pad->entity) !=
> > > MEDIA_ENT_T_V4L2_SUBDEV)
> > > + if (!src_pad || !is_media_entity_v4l2_subdev(src_pad->entity))
> > > break;
> > >
> > > /* Don't call FIMC subdev operation to avoid nested locking */
> > > @@ -1397,7 +1396,7 @@ static int fimc_link_setup(struct media_entity
> > > *entity,
> > > struct fimc_vid_cap *vc = &fimc->vid_cap;
> > > struct v4l2_subdev *sensor;
> > >
> > > - if (media_entity_type(remote->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> > > + if (!is_media_entity_v4l2_subdev(remote->entity))
> > > return -EINVAL;
> > >
> > > if (WARN_ON(fimc == NULL))
> > > diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c
> > > b/drivers/media/platform/exynos4-is/fimc-isp-video.c
> > > index 3d9ccbf5f10f..5fbaf5e39903 100644
> > > --- a/drivers/media/platform/exynos4-is/fimc-isp-video.c
> > > +++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c
> > > @@ -467,8 +467,7 @@ static int isp_video_pipeline_validate(struct
> > > fimc_isp *isp)
> > >
> > > /* Retrieve format at the source pad */
> > > pad = media_entity_remote_pad(pad);
> > > - if (pad == NULL ||
> > > - media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> > > + if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> > > break;
> > >
> > > sd = media_entity_to_v4l2_subdev(pad->entity);
> > > diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c
> > > b/drivers/media/platform/exynos4-is/fimc-lite.c
> > > index b2607da4ad14..c2327147b360 100644
> > > --- a/drivers/media/platform/exynos4-is/fimc-lite.c
> > > +++ b/drivers/media/platform/exynos4-is/fimc-lite.c
> > > @@ -814,8 +814,7 @@ static int fimc_pipeline_validate(struct fimc_lite
> > > *fimc)
> > > }
> > > /* Retrieve format at the source pad */
> > > pad = media_ent
Re: [PATCH v8 32/55] [media] media: use macros to check for V4L2 subdev entities
@@ -419,7 +419,7 @@ static int iss_pipeline_pm_power_one(struct media_entity
> *entity, int change)
> {
> struct v4l2_subdev *subdev;
>
> - subdev = media_entity_type(entity) == MEDIA_ENT_T_V4L2_SUBDEV
> + subdev = is_media_entity_v4l2_subdev(entity)
> ? media_entity_to_v4l2_subdev(entity) : NULL;
>
> if (entity->use_count == 0 && change > 0 && subdev != NULL) {
> @@ -461,7 +461,7 @@ static int iss_pipeline_pm_power(struct media_entity
> *entity, int change)
> media_entity_graph_walk_start(&graph, entity);
>
> while (!ret && (entity = media_entity_graph_walk_next(&graph)))
> - if (media_entity_type(entity) != MEDIA_ENT_T_DEVNODE)
> + if (is_media_entity_v4l2_subdev(entity))
> ret = iss_pipeline_pm_power_one(entity, change);
>
> if (!ret)
> @@ -471,7 +471,7 @@ static int iss_pipeline_pm_power(struct media_entity
> *entity, int change)
>
> while ((first = media_entity_graph_walk_next(&graph))
> && first != entity)
> - if (media_entity_type(first) != MEDIA_ENT_T_DEVNODE)
> + if (is_media_entity_v4l2_subdev(first))
> iss_pipeline_pm_power_one(first, -change);
>
> return ret;
> @@ -590,8 +590,7 @@ static int iss_pipeline_disable(struct iss_pipeline *pipe,
> break;
>
> pad = media_entity_remote_pad(pad);
> - if (pad == NULL ||
> - media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> + if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> break;
>
> entity = pad->entity;
> @@ -658,8 +657,7 @@ static int iss_pipeline_enable(struct iss_pipeline *pipe,
> break;
>
> pad = media_entity_remote_pad(pad);
> - if (pad == NULL ||
> - media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> + if (!pad || !is_media_entity_v4l2_subdev(pad->entity))
> break;
>
> entity = pad->entity;
> diff --git a/drivers/staging/media/omap4iss/iss_video.c
> b/drivers/staging/media/omap4iss/iss_video.c
> index 45a3f2d778fc..cbe5783735dc 100644
> --- a/drivers/staging/media/omap4iss/iss_video.c
> +++ b/drivers/staging/media/omap4iss/iss_video.c
> @@ -191,8 +191,7 @@ iss_video_remote_subdev(struct iss_video *video, u32 *pad)
>
> remote = media_entity_remote_pad(&video->pad);
>
> - if (remote == NULL ||
> - media_entity_type(remote->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> + if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> return NULL;
>
> if (pad)
> @@ -217,7 +216,7 @@ iss_video_far_end(struct iss_video *video)
> if (entity == &video->video.entity)
> continue;
>
> - if (media_entity_type(entity) != MEDIA_ENT_T_DEVNODE)
> + if (!is_media_entity_v4l2_io(entity))
> continue;
>
> far_end = to_iss_video(media_entity_to_video_device(entity));
I finally got around to test these patches eventually, and after some
debugging found this one. I think it's a good idea to have macros to
determine whether an entity exposes a V4L2 sub-device interface but it
should be more robust than is_media_entity_v4l2_subdev() right now is.
Incorrect result can have consequences from a pipeline start failure (due to
a failure in link validation) to memory corruption as struct media_entity
pointer is assumed to be a pointer to a field of another struct.
How about assigning that in V4L2 sub-device / video device registration,
for example?
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] [media] media-device: split media initialization and registration
Hi Javier, Javier Martinez Canillas wrote: > The media device node is registered and so made visible to user-space > before entities are registered and links created which means that the > media graph obtained by user-space could be only partially enumerated > if that happens too early before all the graph has been created. > > To avoid this race condition, split the media init and registration > in separate functions and only register the media device node when > all the pending subdevices have been registered, either explicitly > by the driver or asynchronously using v4l2_async_register_subdev(). > > The media_device_register() had a check for drivers not filling dev > and model fields but all drivers in mainline set them and not doing > it will be a driver bug so change the function return to void and > add a BUG_ON() for dev being NULL instead. > > Also, add a media_device_cleanup() function that will destroy the > graph_mutex that is initialized in media_device_init(). > > Suggested-by: Sakari Ailus > Signed-off-by: Javier Martinez Canillas Thanks! For drivers/media/media-device.c, drivers/media/platform/omap3isp/isp.c and include/media/media-device.h: Acked-by: Sakari Ailus Laurent, could you ack these if you're fine with them? They depend on Mauro's patches so they should be applied after them. -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] [media] media-device: split media initialization and registration
Hi Mauro, Mauro Carvalho Chehab wrote: > Em Fri, 11 Sep 2015 09:31:36 +0200 > Javier Martinez Canillas escreveu: > >> Hello Sakari, >> >> On 09/11/2015 07:51 AM, Sakari Ailus wrote: >>> Hi Javier, >>> >>> Javier Martinez Canillas wrote: >>>> Hello Sakari, >>>> >>>> On 09/10/2015 07:14 PM, Sakari Ailus wrote: >>>>> Hi Javier, >>>>> >>>>> Thanks for the set! A few comments below. >>>>> >>>> >>>> Thanks to you for your feedback. >>>> >>>>> Javier Martinez Canillas wrote: >>>>>> The media device node is registered and so made visible to user-space >>>>>> before entities are registered and links created which means that the >>>>>> media graph obtained by user-space could be only partially enumerated >>>>>> if that happens too early before all the graph has been created. >>>>>> >>>>>> To avoid this race condition, split the media init and registration >>>>>> in separate functions and only register the media device node when >>>>>> all the pending subdevices have been registered, either explicitly >>>>>> by the driver or asynchronously using v4l2_async_register_subdev(). >>>>>> >>>>>> Also, add a media_entity_cleanup() function that will destroy the >>>>>> graph_mutex that is initialized in media_entity_init(). >>>>>> >>>>>> Suggested-by: Sakari Ailus >>>>>> Signed-off-by: Javier Martinez Canillas >>>>>> >>>>>> --- >>>>>> >>>>>> drivers/media/common/siano/smsdvb-main.c | 1 + >>>>>> drivers/media/media-device.c | 38 >>>>>> +++ >>>>>> drivers/media/platform/exynos4-is/media-dev.c | 12 ++--- >>>>>> drivers/media/platform/omap3isp/isp.c | 11 +--- >>>>>> drivers/media/platform/s3c-camif/camif-core.c | 13 ++--- >>>>>> drivers/media/platform/vsp1/vsp1_drv.c| 19 ++ >>>>>> drivers/media/platform/xilinx/xilinx-vipp.c | 11 +--- >>>>>> drivers/media/usb/au0828/au0828-core.c| 26 +- >>>>>> drivers/media/usb/cx231xx/cx231xx-cards.c | 22 +++- >>>>>> drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 11 +--- >>>>>> drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 13 ++--- >>>>>> drivers/media/usb/siano/smsusb.c | 14 -- >>>>>> drivers/media/usb/uvc/uvc_driver.c| 9 +-- >>>>>> include/media/media-device.h | 2 ++ >>>>>> 14 files changed, 156 insertions(+), 46 deletions(-) >>>>>> >>>>>> diff --git a/drivers/media/common/siano/smsdvb-main.c >>>>>> b/drivers/media/common/siano/smsdvb-main.c >>>>>> index ab345490a43a..8a1ea2192439 100644 >>>>>> --- a/drivers/media/common/siano/smsdvb-main.c >>>>>> +++ b/drivers/media/common/siano/smsdvb-main.c >>>>>> @@ -617,6 +617,7 @@ static void smsdvb_media_device_unregister(struct >>>>>> smsdvb_client_t *client) >>>>>> if (!coredev->media_dev) >>>>>> return; >>>>>> media_device_unregister(coredev->media_dev); >>>>>> +media_device_cleanup(coredev->media_dev); >>>>>> kfree(coredev->media_dev); >>>>>> coredev->media_dev = NULL; >>>>>> #endif >>>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c >>>>>> index 745defb34b33..a8beb0b445a6 100644 >>>>>> --- a/drivers/media/media-device.c >>>>>> +++ b/drivers/media/media-device.c >>>>>> @@ -526,7 +526,7 @@ static void media_device_release(struct >>>>>> media_devnode *mdev) >>>>>> } >>>>>> >>>>>> /** >>>>>> - * media_device_register - register a media device >>>>>> + * media_device_init() - initialize a media device >>>>>>* @mdev:The media device >>>>>>* >>>>>>* The caller is responsible for initializing the media device
Re: [PATCH 2/2] [media] media-device: split media initialization and registration
Hi Javier, Javier Martinez Canillas wrote: Hello Sakari, On 09/10/2015 07:14 PM, Sakari Ailus wrote: Hi Javier, Thanks for the set! A few comments below. Thanks to you for your feedback. Javier Martinez Canillas wrote: The media device node is registered and so made visible to user-space before entities are registered and links created which means that the media graph obtained by user-space could be only partially enumerated if that happens too early before all the graph has been created. To avoid this race condition, split the media init and registration in separate functions and only register the media device node when all the pending subdevices have been registered, either explicitly by the driver or asynchronously using v4l2_async_register_subdev(). Also, add a media_entity_cleanup() function that will destroy the graph_mutex that is initialized in media_entity_init(). Suggested-by: Sakari Ailus Signed-off-by: Javier Martinez Canillas --- drivers/media/common/siano/smsdvb-main.c | 1 + drivers/media/media-device.c | 38 +++ drivers/media/platform/exynos4-is/media-dev.c | 12 ++--- drivers/media/platform/omap3isp/isp.c | 11 +--- drivers/media/platform/s3c-camif/camif-core.c | 13 ++--- drivers/media/platform/vsp1/vsp1_drv.c| 19 ++ drivers/media/platform/xilinx/xilinx-vipp.c | 11 +--- drivers/media/usb/au0828/au0828-core.c| 26 +- drivers/media/usb/cx231xx/cx231xx-cards.c | 22 +++- drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 11 +--- drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 13 ++--- drivers/media/usb/siano/smsusb.c | 14 -- drivers/media/usb/uvc/uvc_driver.c| 9 +-- include/media/media-device.h | 2 ++ 14 files changed, 156 insertions(+), 46 deletions(-) diff --git a/drivers/media/common/siano/smsdvb-main.c b/drivers/media/common/siano/smsdvb-main.c index ab345490a43a..8a1ea2192439 100644 --- a/drivers/media/common/siano/smsdvb-main.c +++ b/drivers/media/common/siano/smsdvb-main.c @@ -617,6 +617,7 @@ static void smsdvb_media_device_unregister(struct smsdvb_client_t *client) if (!coredev->media_dev) return; media_device_unregister(coredev->media_dev); + media_device_cleanup(coredev->media_dev); kfree(coredev->media_dev); coredev->media_dev = NULL; #endif diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c index 745defb34b33..a8beb0b445a6 100644 --- a/drivers/media/media-device.c +++ b/drivers/media/media-device.c @@ -526,7 +526,7 @@ static void media_device_release(struct media_devnode *mdev) } /** - * media_device_register - register a media device + * media_device_init() - initialize a media device * @mdev: The media device * * The caller is responsible for initializing the media device before @@ -534,12 +534,11 @@ static void media_device_release(struct media_devnode *mdev) * * - dev must point to the parent device * - model must be filled with the device model name + * + * returns zero on success or a negative error code. */ -int __must_check __media_device_register(struct media_device *mdev, -struct module *owner) +int __must_check media_device_init(struct media_device *mdev) I think I suggested making media_device_init() return void as the only remaining source of errors would be driver bugs. Yes you did and I think I explained why I preferred to leave it as is and I thought we agreed :) I thought we agreed, too. But my understanding was that the agreement was different. ;-) Currently media_device_register() is failing gracefully if a buggy driver is not setting mdev->dev. So changing media_device_init() to return void instead, would be a semantic change and if drivers are not checking that anymore, can lead to NULL pointer dereference bugs. Do we have such drivers? Would they ever have worked in the first place, as media device registration would have failed? I'd simply replace the WARN_ON() below with BUG(). Sorry but I disagree, I think that BUG() should only be used for exceptional cases in which execution can't really continue without causing data corruption or something like that, so bringing down the machine is the safest and least bad option. I think it's also fine to use that for basic sanity checks on code paths that will be run early and every time. To support what I'm saying, just do this: $ grep BUG_ON drivers/media/v4l2-core/* Even though most of that is in videobuf, V4L2 core does that, too, and there's a case of especially delicious usage in v4l2_subdev_init(). :-) But that's not the case here, if there is a buggy driver then the worst thing that would happen is that a driver probe function is g
Re: [PATCH 2/2] [media] media-device: split media initialization and registration
Javier Martinez Canillas wrote: > Also, add a media_entity_cleanup() function that will destroy the > graph_mutex that is initialized in media_entity_init(). media_device_init() and media_device_cleanup()? -- Sakari Ailus sakari.ai...@linux.intel.com -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] [media] media-device: split media initialization and registration
Hi Javier,
Thanks for the set! A few comments below.
Javier Martinez Canillas wrote:
> The media device node is registered and so made visible to user-space
> before entities are registered and links created which means that the
> media graph obtained by user-space could be only partially enumerated
> if that happens too early before all the graph has been created.
>
> To avoid this race condition, split the media init and registration
> in separate functions and only register the media device node when
> all the pending subdevices have been registered, either explicitly
> by the driver or asynchronously using v4l2_async_register_subdev().
>
> Also, add a media_entity_cleanup() function that will destroy the
> graph_mutex that is initialized in media_entity_init().
>
> Suggested-by: Sakari Ailus
> Signed-off-by: Javier Martinez Canillas
>
> ---
>
> drivers/media/common/siano/smsdvb-main.c | 1 +
> drivers/media/media-device.c | 38
> +++
> drivers/media/platform/exynos4-is/media-dev.c | 12 ++---
> drivers/media/platform/omap3isp/isp.c | 11 +---
> drivers/media/platform/s3c-camif/camif-core.c | 13 ++---
> drivers/media/platform/vsp1/vsp1_drv.c| 19 ++
> drivers/media/platform/xilinx/xilinx-vipp.c | 11 +---
> drivers/media/usb/au0828/au0828-core.c| 26 +-
> drivers/media/usb/cx231xx/cx231xx-cards.c | 22 +++-
> drivers/media/usb/dvb-usb-v2/dvb_usb_core.c | 11 +---
> drivers/media/usb/dvb-usb/dvb-usb-dvb.c | 13 ++---
> drivers/media/usb/siano/smsusb.c | 14 --
> drivers/media/usb/uvc/uvc_driver.c| 9 +--
> include/media/media-device.h | 2 ++
> 14 files changed, 156 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/media/common/siano/smsdvb-main.c
> b/drivers/media/common/siano/smsdvb-main.c
> index ab345490a43a..8a1ea2192439 100644
> --- a/drivers/media/common/siano/smsdvb-main.c
> +++ b/drivers/media/common/siano/smsdvb-main.c
> @@ -617,6 +617,7 @@ static void smsdvb_media_device_unregister(struct
> smsdvb_client_t *client)
> if (!coredev->media_dev)
> return;
> media_device_unregister(coredev->media_dev);
> + media_device_cleanup(coredev->media_dev);
> kfree(coredev->media_dev);
> coredev->media_dev = NULL;
> #endif
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 745defb34b33..a8beb0b445a6 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -526,7 +526,7 @@ static void media_device_release(struct media_devnode
> *mdev)
> }
>
> /**
> - * media_device_register - register a media device
> + * media_device_init() - initialize a media device
> * @mdev:The media device
> *
> * The caller is responsible for initializing the media device before
> @@ -534,12 +534,11 @@ static void media_device_release(struct media_devnode
> *mdev)
> *
> * - dev must point to the parent device
> * - model must be filled with the device model name
> + *
> + * returns zero on success or a negative error code.
> */
> -int __must_check __media_device_register(struct media_device *mdev,
> - struct module *owner)
> +int __must_check media_device_init(struct media_device *mdev)
I think I suggested making media_device_init() return void as the only
remaining source of errors would be driver bugs.
I'd simply replace the WARN_ON() below with BUG().
> {
> - int ret;
> -
> if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0))
> return -EINVAL;
>
> @@ -550,6 +549,35 @@ int __must_check __media_device_register(struct
> media_device *mdev,
> spin_lock_init(&mdev->lock);
> mutex_init(&mdev->graph_mutex);
>
> + dev_dbg(mdev->dev, "Media device initialized\n");
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(media_device_init);
> +
> +/**
> + * media_device_cleanup() - Cleanup a media device
> + * @mdev:The media device
> + *
> + */
> +void media_device_cleanup(struct media_device *mdev)
> +{
> + mutex_destroy(&mdev->graph_mutex);
Very nice!
> +}
> +EXPORT_SYMBOL_GPL(media_device_cleanup);
> +
> +/**
> + * __media_device_register() - register a media device
> + * @mdev:The media device
> + * @owner: The module owner
> + *
> + * returns zero on success or a negative error code.
> + */
> +int __must_check __media_device_register(struct media_device *mdev,
> + struct mo
Re: [PATCH RFC v3 07/16] media: get rid of unused "extra_links" param on media_entity_init()
Hi Mauro, On Wed, Aug 12, 2015 at 05:14:51PM -0300, Mauro Carvalho Chehab wrote: > Currently, media_entity_init() creates an array with the links, > allocated at init time. It provides a parameter (extra_links) > that would allocate more links than the current needs, but this > is not used by any driver. > > As we want to be able to do dynamic link allocation/removal, > we'll need to change the implementation of the links. So, > before doing that, let's first remove that extra unused > parameter, in order to cleanup the interface first. > > Signed-off-by: Mauro Carvalho Chehab > ... > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index 738e1d5d25dc..be6885e7c8ed 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -177,7 +177,7 @@ void graph_obj_init(struct media_device *mdev, > void graph_obj_remove(struct media_graph_obj *gobj); > > int media_entity_init(struct media_entity *entity, u16 num_pads, > - struct media_pad *pads, u16 extra_links); > + struct media_pad *pads); > void media_entity_cleanup(struct media_entity *entity); > > int media_entity_create_link(struct media_entity *source, u16 source_pad, How about putting this in front of the set? It has no dependencies to the other patches, does it? Acked-by: Sakari Ailus -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] media: v4l2: make alloction alogthim more robust and flexible
Hi Zhaowei, My apologies for the delayed reply. On Wed, Jul 30, 2014 at 11:55:32AM +0800, Zhaowei Yuan wrote: > Current algothim relies on the fact that caller will align the > size to PAGE_SIZE, otherwise order will be decreased to negative > when remain size is less than PAGE_SIZE, it makes the function > hard to be migrated. > This patch sloves the hidden problem. > > Signed-off-by: Zhaowei Yuan > --- > drivers/media/v4l2-core/videobuf2-dma-sg.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-dma-sg.c > b/drivers/media/v4l2-core/videobuf2-dma-sg.c > index adefc31..40d18aa 100644 > --- a/drivers/media/v4l2-core/videobuf2-dma-sg.c > +++ b/drivers/media/v4l2-core/videobuf2-dma-sg.c > @@ -58,7 +58,7 @@ static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf > *buf, > > order = get_order(size); > /* Dont over allocate*/ > - if ((PAGE_SIZE << order) > size) > + if (order > 0 && (PAGE_SIZE << order) > size) > order--; > > pages = NULL; With comments from Andreas taken into account, Acked-by: Sakari Ailus I'd consider this for the stable series as well. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 03/12] [media] exynos5-fimc-is: Add common driver header files
Hi Sylwester and Arun,
On Wed, Nov 06, 2013 at 12:23:07PM +0100, Sylwester Nawrocki wrote:
> Hi,
>
> On 05/11/13 14:16, Arun Kumar K wrote:
> >>> +struct is_common_reg {
> >>> + u32 hicmd;
> >>> + u32 hic_sensorid;
> >>> + u32 hic_param[4];
> >>> +
> >>> + u32 reserved1[3];
> [...]
> >>> + u32 meta_iflag;
> >>> + u32 meta_sensor_id;
> >>> + u32 meta_param1;
> >>> +
> >>> + u32 reserved9[1];
> >>> +
> >>> + u32 fcount;
> >>
> >> If these structs define an interface that's not used by the driver only it
> >> might be a good idea to use __packed to ensure no padding is added.
> >>
> >
> > The same structure is used as is in the firmware code and so it is retained
> > in the driver.
>
> I agree it makes sense to use __packed attribute to ensure no padding is
> added by the compiler. The firmware source and the driver will likely be
> compiled with different toolchains, and in both cases we should ensure
> no padding is added.
Agreed.
> >>> diff --git a/drivers/media/platform/exynos5-is/fimc-is-metadata.h
> >>> b/drivers/media/platform/exynos5-is/fimc-is-metadata.h
> >>> new file mode 100644
> >>> index 000..02367c4
> >>> --- /dev/null
> >>> +++ b/drivers/media/platform/exynos5-is/fimc-is-metadata.h
> >>> @@ -0,0 +1,767 @@
> [..]
> >>> +enum metadata_mode {
> >>> + METADATA_MODE_NONE,
> >>> + METADATA_MODE_FULL
> >>> +};
> >>> +
> >>> +struct camera2_request_ctl {
> >>> + uint32_tid;
> >>> + enum metadata_mode metadatamode;
> >>> + uint8_t outputstreams[16];
> >>> + uint32_tframecount;
> >>> +};
> >>> +
> >>> +struct camera2_request_dm {
> >>> + uint32_tid;
> >>> + enum metadata_mode metadatamode;
> >>> + uint32_tframecount;
> >>> +};
> [...]
> >>> +struct camera2_lens_ctl {
> >>> + uint32_tfocus_distance;
> >>> + float aperture;
> >>
> >> Floating point numbers? Really? :-)
> >>
> >
> > Yes as mentioned, the same structure is used by the firmware and
> > so it is used as is in the kernel.
>
> These floating numbers are pretty painful, but I don't think they can
> be avoided unless the firmware is changed. I hope there is no need to
> touch those in the kernel.
>
> There are already precedents of using floating point numbers in driver's
> public interface, e.g. some gpu/drm drivers.
As long as you can somehow ensure these will never end up to FPU registers,
I think that should be fine. Just copying the struct elsewhere using
memcpy() will be good, I believe.
> I noticed there is another issue in this firmware/kernel interface, i.e.
> some data structures contain enums in them, e.g.
>
> struct camera2_lens_ctl {
> uint32_tfocus_distance;
> float aperture;
> float focal_length;
> float filter_density;
> enum optical_stabilization_mode optical_stabilization_mode;
> };
>
> It looks like a mistake in the interface design, as size of an enum is
> implementation specific.
>
> I guess size of those enum types is supposed to be 4 bytes ? Presumably
> you should, e.g. use fixed data type like uin32_t or __u32 instead of those
> enums. It looks pretty fragile as it is now.
Good point; I agree.
> In addition all those data structures should be declared with __packed
> attribute, to ensure a specific data structure layout and to avoid
> an unexpected padding.
>
> >> diff --git a/drivers/media/platform/exynos5-is/fimc-is-param.h
> >> b/drivers/media/platform/exynos5-is/fimc-is-param.h
> >> new file mode 100644
> >> index 000..015cc13
> >> --- /dev/null
> >> +++ b/drivers/media/platform/exynos5-is/fimc-is-param.h
> > ...
> >> +struct param_control {
> >> + u32 cmd;
> >
> > You use uint32_t in some other headers. It's not wrong to use both C99 and
> > Linux types but I'd try to stick to either one.
>
> I tend to agree with that, it's probably better to use one convention, u32
> for kernel internal structures and __u32 for any public interfaces. I don't
> think it is e requirement but would be nice to keep it more consistent.
>
> Even if we wanted to keep the firmware defined data structures in sync with
> the Linux driver, there are already some Linux types used within the firmware
> interface. if I understood things correctly.
I guess it wouldn't hurt to use uint32_t there instead of u32 (and __u32).
entirely up to you.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 03/12] [media] exynos5-fimc-is: Add common driver header files
t; + u32 scp_iflag;
> + u32 scp_sensor_id;
> + u32 scp_param[3];
> +
> + u32 reserved6[1];
> +
> + u32 isp_yuv_iflag;
> + u32 isp_yuv_sensor_id;
> + u32 isp_yuv_param[2];
> +
> + u32 reserved7[1];
> +
> + u32 shot_iflag;
> + u32 shot_sensor_id;
> + u32 shot_param[2];
> +
> + u32 reserved8[1];
> +
> + u32 meta_iflag;
> + u32 meta_sensor_id;
> + u32 meta_param1;
> +
> + u32 reserved9[1];
> +
> + u32 fcount;
If these structs define an interface that's not used by the driver only it
might be a good idea to use __packed to ensure no padding is added.
> +};
> +
> +struct is_mcuctl_reg {
> + u32 mcuctl;
> + u32 bboar;
> +
> + u32 intgr0;
> + u32 intcr0;
> + u32 intmr0;
> + u32 intsr0;
> + u32 intmsr0;
> +
> + u32 intgr1;
> + u32 intcr1;
> + u32 intmr1;
> + u32 intsr1;
> + u32 intmsr1;
> +
> + u32 intcr2;
> + u32 intmr2;
> + u32 intsr2;
> + u32 intmsr2;
> +
> + u32 gpoctrl;
> + u32 cpoenctlr;
> + u32 gpictlr;
> +
> + u32 pad[0xD];
> +
> + struct is_common_reg common_reg;
> +};
> +#endif
...
> diff --git a/drivers/media/platform/exynos5-is/fimc-is-metadata.h
> b/drivers/media/platform/exynos5-is/fimc-is-metadata.h
> new file mode 100644
> index 000..02367c4
> --- /dev/null
> +++ b/drivers/media/platform/exynos5-is/fimc-is-metadata.h
> @@ -0,0 +1,767 @@
> +/*
> + * Samsung EXYNOS5 FIMC-IS (Imaging Subsystem) driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Kil-yeon Lim
> + * Arun Kumar K
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef FIMC_IS_METADATA_H_
> +#define FIMC_IS_METADATA_H_
> +
> +struct rational {
> + uint32_t num;
> + uint32_t den;
> +};
> +
> +#define CAMERA2_MAX_AVAILABLE_MODE 21
> +#define CAMERA2_MAX_FACES16
> +
> +/*
> + * Controls/dynamic metadata
> + */
> +
> +enum metadata_mode {
> + METADATA_MODE_NONE,
> + METADATA_MODE_FULL
> +};
> +
> +struct camera2_request_ctl {
> + uint32_tid;
> + enum metadata_mode metadatamode;
> + uint8_t outputstreams[16];
> + uint32_tframecount;
> +};
> +
> +struct camera2_request_dm {
> + uint32_tid;
> + enum metadata_mode metadatamode;
> + uint32_tframecount;
> +};
> +
> +
> +
> +enum optical_stabilization_mode {
> + OPTICAL_STABILIZATION_MODE_OFF,
> + OPTICAL_STABILIZATION_MODE_ON
> +};
> +
> +enum lens_facing {
> + LENS_FACING_BACK,
> + LENS_FACING_FRONT
> +};
> +
> +struct camera2_lens_ctl {
> + uint32_tfocus_distance;
> + float aperture;
Floating point numbers? Really? :-)
> + float focal_length;
> + float filter_density;
> + enum optical_stabilization_mode optical_stabilization_mode;
> +};
> +
> +struct camera2_lens_dm {
> + uint32_tfocus_distance;
> + float aperture;
> + float focal_length;
> + float filter_density;
> + enum optical_stabilization_mode optical_stabilization_mode;
> + float focus_range[2];
> +};
> +
> +struct camera2_lens_sm {
> + float minimum_focus_distance;
> + float hyper_focal_distance;
> + float available_focal_length[2];
> + float available_apertures;
> + /* assuming 1 aperture */
> + float available_filter_densities;
> + /* assuming 1 ND filter value */
> + enum optical_stabilization_mode available_optical_stabilization;
> + /* assuming 1 */
> + uint32_tshading_map_size;
> + float shading_map[3][40][30];
> + uint32_tgeometric_correction_map_size;
> + float geometric_correction_map[2][3][40][30];
> + enum lens_facingfacing;
> + float position[2];
> +};
...
> diff --git a/drivers/media/platform/exynos5-is/fimc-is-param.h
> b/drivers/media/platform/exynos5-is/fimc-is-param.h
> new file mode 100644
> index 000..015cc13
> --- /dev/null
> +++ b/drivers/media/platform/exynos5-is/fimc-is-param.h
...
> +struct param_control {
> + u32 cmd;
You use uint32_t in some other headers. It's not wrong to use both C99 and
Linux types but I'd try to stick to either one.
> + u32 bypass;
> + u32 buffer_address;
> + u32 buffer_number;
> + /* 0: continuous, 1: single */
> + u32 run_mode;
> + u32 reserved[PARAMETER_MAX_MEMBER - 6];
> + u32 err;
> +};
...
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 02/12] [media] exynos5-fimc-is: Add driver core files
t; +/*
> + * Samsung EXYNOS5 FIMC-IS (Imaging Subsystem) driver
> + *
> + * Copyright (C) 2013 Samsung Electronics Co., Ltd.
> + * Arun Kumar K
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef FIMC_IS_CORE_H_
> +#define FIMC_IS_CORE_H_
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Same here: do you really need all these headers here?
> +#define FIMC_IS_DRV_NAME "exynos5-fimc-is"
> +
> +#define FIMC_IS_COMMAND_TIMEOUT (10 * HZ)
> +#define FIMC_IS_STARTUP_TIMEOUT (3 * HZ)
> +#define FIMC_IS_SHUTDOWN_TIMEOUT (10 * HZ)
> +
> +#define FW_SHARED_OFFSET (0x8c)
> +#define DEBUG_CNT(500 * 1024)
> +#define DEBUG_OFFSET (0x84)
> +#define DEBUGCTL_OFFSET (0x8bd000)
> +#define DEBUG_FCOUNT (0x8c64c0)
> +
> +#define FIMC_IS_MAX_INSTANCES1
> +
> +#define FIMC_IS_NUM_SENSORS 2
> +#define FIMC_IS_NUM_PIPELINES1
> +
> +#define FIMC_IS_MAX_PLANES 3
> +#define FIMC_IS_NUM_SCALERS 2
> +
> +enum fimc_is_clks {
> + IS_CLK_ISP,
> + IS_CLK_MCU_ISP,
> + IS_CLK_ISP_DIV0,
> + IS_CLK_ISP_DIV1,
> + IS_CLK_ISP_DIVMPWM,
> + IS_CLK_MCU_ISP_DIV0,
> + IS_CLK_MCU_ISP_DIV1,
> + IS_CLK_MAX_NUM
> +};
> +
> +/* Video capture states */
> +enum fimc_is_video_state {
> + STATE_INIT,
> + STATE_BUFS_ALLOCATED,
> + STATE_RUNNING,
> +};
> +
> +enum fimc_is_scaler_id {
> + SCALER_SCC,
> + SCALER_SCP
> +};
> +
> +enum fimc_is_sensor_pos {
> + SENSOR_CAM0,
> + SENSOR_CAM1
> +};
> +
> +struct fimc_is_buf {
> + struct vb2_buffer vb;
> + struct list_head list;
> + unsigned int paddr[FIMC_IS_MAX_PLANES];
> +};
> +
> +struct fimc_is_memory {
> + /* physical base address */
> + dma_addr_t paddr;
> + /* virtual base address */
> + void *vaddr;
> + /* total length */
> + unsigned int size;
> +};
> +
> +struct fimc_is_meminfo {
> + struct fimc_is_memory fw;
> + struct fimc_is_memory shot;
> + struct fimc_is_memory region;
> + struct fimc_is_memory shared;
> +};
> +
> +struct fimc_is_drvdata {
> + unsigned intnum_instances;
> + char*fw_name;
> +};
> +
> +/**
> + * struct fimc_is_fmt - the driver's internal color format data
> + * @name: format description
> + * @fourcc: the fourcc code for this format
> + * @depth: number of bytes per pixel
> + * @num_planes: number of planes for this color format
> + */
> +struct fimc_is_fmt {
> + char*name;
> + unsigned intfourcc;
> + unsigned intdepth[FIMC_IS_MAX_PLANES];
> + unsigned intnum_planes;
> +};
> +
> +#endif
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH 09/13] media: s5k6aa: Add support for device tree based instantiation
Hi Laurent and Sylwester, My apologies for the late answer. On Fri, Jul 27, 2012 at 12:50:13AM +0200, Laurent Pinchart wrote: > Hi Sylwester, > > On Thursday 26 July 2012 22:39:31 Sylwester Nawrocki wrote: > > On 07/26/2012 05:21 PM, Laurent Pinchart wrote: > > > On Friday 25 May 2012 21:52:48 Sylwester Nawrocki wrote: > > >> The driver initializes all board related properties except the s_power() > > >> callback to board code. The platforms that require this callback are not > > >> supported by this driver yet for CONFIG_OF=y. > > >> > > >> Signed-off-by: Sylwester Nawrocki > > >> Signed-off-by: Bartlomiej Zolnierkiewicz > > >> Signed-off-by: Kyungmin Park > > >> --- > > >> > > >> .../bindings/camera/samsung-s5k6aafx.txt | 57 + > > >> drivers/media/video/s5k6aa.c | 129 + > > >> 2 files changed, 146 insertions(+), 40 deletions(-) > > >> create mode 100644 > > >> > > >> Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt > > >> > > >> diff --git > > >> a/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt > > >> b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt new file > > >> mode 100644 > > >> index 000..6685a9c > > >> --- /dev/null > > >> +++ b/Documentation/devicetree/bindings/camera/samsung-s5k6aafx.txt > > >> @@ -0,0 +1,57 @@ > > >> +Samsung S5K6AAFX camera sensor > > >> +-- > > >> + > > >> +Required properties: > > >> + > > >> +- compatible : "samsung,s5k6aafx"; > > >> +- reg : base address of the device on I2C bus; > > >> +- video-itu-601-bus : parallel bus with HSYNC and VSYNC - ITU-R BT.601; > > >> +- vdd_core-supply : digital core voltage supply 1.5V (1.4V to 1.6V); > > >> +- vdda-supply : analog power voltage supply 2.8V (2.6V to 3.0V); > > >> +- vdd_reg-supply : regulator input power voltage supply 1.8V (1.7V to > > >> 1.9V) + or 2.8V (2.6V to 3.0); > > >> +- vddio-supply : I/O voltage supply 1.8V (1.65V to 1.95V) > > >> + or 2.8V (2.5V to 3.1V); > > >> + > > >> +Optional properties: > > >> + > > >> +- clock-frequency : the IP's main (system bus) clock frequency in Hz, > > >> the default > > > > > > Is that the input clock frequency ? Can't it vary ? Instead of accessing > > > the > > Yes, the description is incorrect in this patch, it should read: > > > > +- clock-frequency : the sensor's master clock frequency in Hz; > > > > and be a required property. As in this patch: > > https://github.com/snawrocki/linux/commit/e8a5f890dec0d7414b656bb1d1ac97d5e7 > > abe563 > > > > It could vary (as this is a PLL input frequency), so probably a range would > > be a better alternative. Given that host device won't always be able to set > > this exact value... > > A range sounds good, or perhaps a list of ranges. Sakari, what would you need > for the SMIA++ driver ? Typically the sensor's external clock is derived from another clock using a divisor. This means there's usually quite limited selection of possible clock frequencies since the sensors usually have a small range of possible frequencies such as 6 -- 27 MHz or even less. Also it's important to choose the frequency in such a way that it doesn't interfere with other devices in the system. The frequency also must be picked so that one can achieve the desired highest data transfer rate. The sensors are also quite flexible in their internal clock tree configuration, but in the situation where the desired data rate is close to sensor limits there may be additional constraints. Still it's always possible to come up with a best value for the board while other values are inferior. For these reasons I see little point in providing anything else than just a single value for the external clock frequency. This value is board-specific. > > > sensor clock frequency from the FIMC driver you should reference a clock > > > in the sensor DT node. That obviously requires generic clock support, > > > which might not be available for your platform yet (that's one of the > > > reasons the OMAP3 ISP driver doesn't support DT yet). > > > > I agree it might be better, but waiting unknown number of kernel releases > > for the platforms to get converted to common clock API is not a good > > alternative either. I guess we could have some transitional solutions while > > other subsystems are getting adapted. > > I agree, we need an interim solution. The SMIA++ driver allows the platform data to have either the clock name or a function pointer to set the clock frequency. If the clock name is there it'll be used instead. The function pointer can be removed once it's no longer needed. Kind regards, -- Sakari Ailus e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
