Re: [PATCH v9 2/8] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek,
On Mon, Jun 08, 2015 at 09:21:10AM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
>
> On 06/03/2015 10:59 PM, Sakari Ailus wrote:
> >Hi Jacek,
> >
> >On Wed, Jun 03, 2015 at 09:56:39AM +0200, Jacek Anaszewski wrote:
> >>Hi Sakari,
> >>
> >>On 06/02/2015 05:32 PM, Sakari Ailus wrote:
> >>>Hi, Jacek!
> >>>
> >>>On Tue, Jun 02, 2015 at 11:13:54AM +0200, Jacek Anaszewski wrote:
> >>>>Hi Sakari,
> >>>>
> >>>>On 06/01/2015 10:59 PM, Sakari Ailus wrote:
> >>>>>Hi Jacek,
> >>>>>
> >>>>>On Mon, May 25, 2015 at 05:13:57PM +0200, Jacek Anaszewski wrote:
> >>>>>>This patch adds helper functions for registering/unregistering
> >>>>>>LED Flash class devices as V4L2 sub-devices. The functions should
> >>>>>>be called from the LED subsystem device driver. In case the
> >>>>>>support for V4L2 Flash sub-devices is disabled in the kernel
> >>>>>>config the functions' empty versions will be used.
> >>>>>>
> >>>>>>Signed-off-by: Jacek Anaszewski
> >>>>>>Acked-by: Kyungmin Park
> >>>>>>Cc: Sakari Ailus
> >>>>>>Cc: Hans Verkuil
> >>>>>
> >>>>>Thanks for adding indicator support!
> >>>>>
> >>>>>Acked-by: Sakari Ailus
> >>>>>
> >>>>
> >>>>I missed one thing - sysfs interface of the indicator LED class
> >>>>also has to be disabled/enabled of v4l2_flash_open/close.
> >>>
> >>>Good catch.
> >>>
> >>>>
> >>>>I am planning to reimplement the functions as follows,
> >>>>please let me know if you see any issues here:
> >>>>
> >>>>static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> >>>>*fh)
> >>>>{
> >>>> struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
> >>>>
> >>>> struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> >>>>
> >>>> struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> >>>> struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
> >>>>
> >>>> struct led_classdev *led_cdev_ind;
> >>>>
> >>>> int ret = 0;
> >>>
> >>>I think you could spare some newlines above (and below as well).
> >>
> >>There must have been some issue on thunderbird side,
> >>originally there were no newlines above.
> >
> >Ok. I've used Seamonkey, I believe it uses the same editor. Some bugs were
> >unintroduced a few years ago, and since that I've mostly used a different
> >editor (and MUA) when replying to patches.
> >
> >>>>
> >>>>
> >>>> mutex_lock(&led_cdev->led_access);
> >>>>
> >>>>
> >>>> if (!v4l2_fh_is_singular(&fh->vfh))
> >>>>
> >>>> goto unlock;
> >>>>
> >>>>
> >>>> led_sysfs_disable(led_cdev);
> >>>> led_trigger_remove(led_cdev);
> >>>>
> >>>>
> >>>> if (iled_cdev) {
> >>>> led_cdev_ind = &iled_cdev->led_cdev;
> >>>
> >>>You can also declare led_cdev_ind here as you don't need it outside the
> >>>basic block.
> >>
> >>With new approach it will be required also in error path.
> >
> >True.
> >
> >>>>
> >>>>
> >>>> mutex_lock(&led_cdev_ind->led_access);
> >>>>
> >>>>
> >>>> led_sysfs_disable(led_cdev_ind);
> >>>> led_trigger_remove(led_cdev_ind);
> >>>>
> >>>>
> >>>> mutex_unlock(&led_cdev_ind->led_access);
> >>>
> >>>Please don't acquire the indicator mutex while holding the flash mutex. I
> >>>don't think there's a need to do so, thus creating a dependency between the
> >>>two.Just remember to check for v4l2_fh_is_singular() in both cases.
> >>
> >>I thought that the code would be simpler this way
Re: [PATCH v9 2/8] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek,
On Wed, Jun 03, 2015 at 09:56:39AM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
>
> On 06/02/2015 05:32 PM, Sakari Ailus wrote:
> >Hi, Jacek!
> >
> >On Tue, Jun 02, 2015 at 11:13:54AM +0200, Jacek Anaszewski wrote:
> >>Hi Sakari,
> >>
> >>On 06/01/2015 10:59 PM, Sakari Ailus wrote:
> >>>Hi Jacek,
> >>>
> >>>On Mon, May 25, 2015 at 05:13:57PM +0200, Jacek Anaszewski wrote:
> >>>>This patch adds helper functions for registering/unregistering
> >>>>LED Flash class devices as V4L2 sub-devices. The functions should
> >>>>be called from the LED subsystem device driver. In case the
> >>>>support for V4L2 Flash sub-devices is disabled in the kernel
> >>>>config the functions' empty versions will be used.
> >>>>
> >>>>Signed-off-by: Jacek Anaszewski
> >>>>Acked-by: Kyungmin Park
> >>>>Cc: Sakari Ailus
> >>>>Cc: Hans Verkuil
> >>>
> >>>Thanks for adding indicator support!
> >>>
> >>>Acked-by: Sakari Ailus
> >>>
> >>
> >>I missed one thing - sysfs interface of the indicator LED class
> >>also has to be disabled/enabled of v4l2_flash_open/close.
> >
> >Good catch.
> >
> >>
> >>I am planning to reimplement the functions as follows,
> >>please let me know if you see any issues here:
> >>
> >>static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> >>*fh)
> >>{
> >> struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
> >>
> >> struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> >>
> >> struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> >> struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
> >>
> >> struct led_classdev *led_cdev_ind;
> >>
> >> int ret = 0;
> >
> >I think you could spare some newlines above (and below as well).
>
> There must have been some issue on thunderbird side,
> originally there were no newlines above.
Ok. I've used Seamonkey, I believe it uses the same editor. Some bugs were
unintroduced a few years ago, and since that I've mostly used a different
editor (and MUA) when replying to patches.
> >>
> >>
> >> mutex_lock(&led_cdev->led_access);
> >>
> >>
> >> if (!v4l2_fh_is_singular(&fh->vfh))
> >>
> >> goto unlock;
> >>
> >>
> >> led_sysfs_disable(led_cdev);
> >> led_trigger_remove(led_cdev);
> >>
> >>
> >> if (iled_cdev) {
> >> led_cdev_ind = &iled_cdev->led_cdev;
> >
> >You can also declare led_cdev_ind here as you don't need it outside the
> >basic block.
>
> With new approach it will be required also in error path.
True.
> >>
> >>
> >> mutex_lock(&led_cdev_ind->led_access);
> >>
> >>
> >> led_sysfs_disable(led_cdev_ind);
> >> led_trigger_remove(led_cdev_ind);
> >>
> >>
> >> mutex_unlock(&led_cdev_ind->led_access);
> >
> >Please don't acquire the indicator mutex while holding the flash mutex. I
> >don't think there's a need to do so, thus creating a dependency between the
> >two.Just remember to check for v4l2_fh_is_singular() in both cases.
>
> I thought that the code would be simpler this way. Nevertheless I
> produced a new version by following your advise.
>
> >
> >>
> >> }
> >>
> >>
> >> ret = __sync_device_with_v4l2_controls(v4l2_flash);
> >
> >If ret is < 0 here, you end up disabling the sysfs interface while open()
> >fails (and v4l2_flash_close() will not be run). Shouldn't you handle that?
>
> Good point.
>
> Please find the new version of v4l2_flash{open|close} functions below:
>
> static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> *fh)
> {
> struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
> struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
> struct led_classdev *led_cdev_ind = NULL;
&
Re: [PATCH v9 2/8] media: Add registration helpers for V4L2 flash sub-devices
Hi, Jacek!
On Tue, Jun 02, 2015 at 11:13:54AM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
>
> On 06/01/2015 10:59 PM, Sakari Ailus wrote:
> >Hi Jacek,
> >
> >On Mon, May 25, 2015 at 05:13:57PM +0200, Jacek Anaszewski wrote:
> >>This patch adds helper functions for registering/unregistering
> >>LED Flash class devices as V4L2 sub-devices. The functions should
> >>be called from the LED subsystem device driver. In case the
> >>support for V4L2 Flash sub-devices is disabled in the kernel
> >>config the functions' empty versions will be used.
> >>
> >>Signed-off-by: Jacek Anaszewski
> >>Acked-by: Kyungmin Park
> >>Cc: Sakari Ailus
> >>Cc: Hans Verkuil
> >
> >Thanks for adding indicator support!
> >
> >Acked-by: Sakari Ailus
> >
>
> I missed one thing - sysfs interface of the indicator LED class
> also has to be disabled/enabled of v4l2_flash_open/close.
Good catch.
>
> I am planning to reimplement the functions as follows,
> please let me know if you see any issues here:
>
> static int v4l2_flash_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> *fh)
> {
> struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
>
> struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>
> struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
>
> struct led_classdev *led_cdev_ind;
>
> int ret = 0;
I think you could spare some newlines above (and below as well).
>
>
> mutex_lock(&led_cdev->led_access);
>
>
> if (!v4l2_fh_is_singular(&fh->vfh))
>
> goto unlock;
>
>
> led_sysfs_disable(led_cdev);
> led_trigger_remove(led_cdev);
>
>
> if (iled_cdev) {
> led_cdev_ind = &iled_cdev->led_cdev;
You can also declare led_cdev_ind here as you don't need it outside the
basic block.
>
>
> mutex_lock(&led_cdev_ind->led_access);
>
>
> led_sysfs_disable(led_cdev_ind);
> led_trigger_remove(led_cdev_ind);
>
>
> mutex_unlock(&led_cdev_ind->led_access);
Please don't acquire the indicator mutex while holding the flash mutex. I
don't think there's a need to do so, thus creating a dependency between the
two. Just remember to check for v4l2_fh_is_singular() in both cases.
>
> }
>
>
> ret = __sync_device_with_v4l2_controls(v4l2_flash);
If ret is < 0 here, you end up disabling the sysfs interface while open()
fails (and v4l2_flash_close() will not be run). Shouldn't you handle that?
>
>
> unlock:
> mutex_unlock(&led_cdev->led_access);
>
> return ret;
>
> }
>
>
> static int v4l2_flash_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> *fh)
> {
> struct v4l2_flash *v4l2_flash = v4l2_subdev_to_v4l2_flash(sd);
>
> struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>
> struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> struct led_classdev_flash *iled_cdev = v4l2_flash->iled_cdev;
>
> struct led_classdev *led_cdev_ind;
>
> int ret = 0;
>
>
> mutex_lock(&led_cdev->led_access);
>
>
> if (v4l2_fh_is_singular(&fh->vfh)) {
> if (v4l2_flash->ctrls[STROBE_SOURCE])
> ret =
> v4l2_ctrl_s_ctrl(v4l2_flash->ctrls[STROBE_SV4L2_FLASH_STROBE_SOURCE_SOFTWARE);
>
> led_sysfs_enable(led_cdev);
>
>
> if (iled_cdev) {
> led_cdev_ind = &iled_cdev->led_cdev;
>
>
> mutex_lock(&led_cdev_ind->led_access);
Ditto.
>
>
> led_sysfs_enable(led_cdev_ind);
>
>
> mutex_unlock(&led_cdev_ind->led_access);
>
> }
>
>
> }
>
>
> mutex_unlock(&led_cdev->led_access);
>
>
> return ret;
>
> }
>
>
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 3/8] leds: max77693: add support for V4L2 Flash sub-device
Hi Jacek, On Mon, May 25, 2015 at 05:13:58PM +0200, Jacek Anaszewski wrote: > Add support for V4L2 Flash sub-device to the max77693 LED Flash class > driver. The support allows for V4L2 Flash sub-device to take the control > of the LED Flash class device. > > Signed-off-by: Jacek Anaszewski > Acked-by: Kyungmin Park > Cc: Bryan Wu > Cc: Richard Purdie > Cc: Sakari Ailus For this and the rest in the set: Acked-by: Sakari Ailus Many thanks to you for your efforts on this! It's great to see it in this state after all the review rounds. :-) -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 8/8] DT: samsung-fimc: Add examples for samsung,flash-led property
Hi Sylwester, On Mon, May 25, 2015 at 02:00:33PM +0200, Sylwester Nawrocki wrote: > Hi, > > On 23/05/15 14:03, Sakari Ailus wrote: > > On Thu, May 21, 2015 at 03:28:40PM +0200, Sylwester Nawrocki wrote: > >> flash-leds = <&flash_xx &image_sensor_x>, <...>; > > > > One more matter to consider: xenon flash devices. > > > > How about samsung,camera-flashes (and ti,camera-flashes)? After pondering > > this awhile, I'm ok with removing the vendor prefix as well. > > > > Let me know what you think. > > I thought about it a bit more and I have some doubts about semantics > as above. I'm fine with 'camera-flashes' as far as name is concerned. > > Perhaps we should put only phandles to leds or xenon flash devices > in the 'camera-flashes' property. I think it would be more future > proof in case there is more nodes needed to describe the camera flash > (or a camera module) than the above two. And phandles to corresponding > image sensor device nodes would be put in a separate property. > > camera-flashes = <&flash_xx>, ... > camera-flash-masters = <&image_sensor_x>, ... > > Then pairs at same index would describe a single flash, 0 would indicate > a null entry if needed. > Similarly we could create properties for other sub-devices of a camera > module, like lenses, etc. This arrangement would be advantageous compared to a single property when adding modules (or lenses) to the equation, and probably more future proof than "samsung,camera-flashes" / "ti,camera-flashes" I believe. I'm also ok with keeping it as-is though. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 2/8] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek, On Mon, May 25, 2015 at 05:13:57PM +0200, Jacek Anaszewski wrote: > This patch adds helper functions for registering/unregistering > LED Flash class devices as V4L2 sub-devices. The functions should > be called from the LED subsystem device driver. In case the > support for V4L2 Flash sub-devices is disabled in the kernel > config the functions' empty versions will be used. > > Signed-off-by: Jacek Anaszewski > Acked-by: Kyungmin Park > Cc: Sakari Ailus > Cc: Hans Verkuil Thanks for adding indicator support! 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 devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 1/8] Documentation: leds: Add description of v4l2-flash sub-device
Hi Jacek, On Mon, May 25, 2015 at 05:13:56PM +0200, Jacek Anaszewski wrote: > +On remove the v4l2_flash_release function has to be called, which takes one > +argument - struct v4l2_flash pointer returned previously by v4l2_flash_init. You might want to add this function can be safely called with NULL or error pointer argument. Acked-by: Sakari Ailus -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 8/8] DT: samsung-fimc: Add examples for samsung,flash-led property
Hi Sylwester, On Mon, May 25, 2015 at 04:28:22PM +0200, Sylwester Nawrocki wrote: > On 25/05/15 14:50, Jacek Anaszewski wrote: > >> On 23/05/15 14:03, Sakari Ailus wrote: > >>> >> On Thu, May 21, 2015 at 03:28:40PM +0200, Sylwester Nawrocki wrote: > >>>> >>> flash-leds = <&flash_xx &image_sensor_x>, <...>; > >>> >> > >>> >> One more matter to consider: xenon flash devices. > >>> >> > >>> >> How about samsung,camera-flashes (and ti,camera-flashes)? After > >>> >> pondering > >>> >> this awhile, I'm ok with removing the vendor prefix as well. > >>> >> > >>> >> Let me know what you think. > >> > > >> > I thought about it a bit more and I have some doubts about semantics > >> > as above. I'm fine with 'camera-flashes' as far as name is concerned. > >> > > >> > Perhaps we should put only phandles to leds or xenon flash devices > >> > in the 'camera-flashes' property. I think it would be more future > >> > proof in case there is more nodes needed to describe the camera flash > >> > (or a camera module) than the above two. And phandles to corresponding > >> > image sensor device nodes would be put in a separate property. > > > > Could you give examples of the cases you are thinking of? > > I don't have any examples in mind ATM, I just wanted to point out > the above convention might not be flexible enough. Especially since > we already know there is more sub-devices within the camera module > than just flashes and image sensors. I have to admit I've never seen a camera module with an integrated flash. The lens is part of the module but typically flash is not. That doesn't say there aren't such devices though. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 8/8] DT: samsung-fimc: Add examples for samsung,flash-led property
Hi Sylwester and Jacek, On Thu, May 21, 2015 at 03:28:40PM +0200, Sylwester Nawrocki wrote: > flash-leds = <&flash_xx &image_sensor_x>, <...>; One more matter to consider: xenon flash devices. How about samsung,camera-flashes (and ti,camera-flashes)? After pondering this awhile, I'm ok with removing the vendor prefix as well. Let me know what you think. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 8/8] DT: samsung-fimc: Add examples for samsung,flash-led property
Hi Sylwester,
On Thu, May 21, 2015 at 06:58:59PM +0200, Sylwester Nawrocki wrote:
> Hi Sakari,
>
> On 21/05/15 16:20, Sakari Ailus wrote:
> > On Thu, May 21, 2015 at 03:28:40PM +0200, Sylwester Nawrocki wrote:
> >> > On 21/05/15 13:32, Sakari Ailus wrote:
> >>>>>> > >>>> @@ -147,6 +149,8 @@ Example:
> >>>>>>>>> > >>>> > >> clocks = <&camera 0>;
> >>>>>>>>> > >>>> > >> clock-names = "mclk";
> >>>>>>>>> > >>>> > >>
> >>>>>>>>> > >>>> > >>+samsung,flash-led =
> >>>>>>>>> > >>>> > >><&rear_cam_flash>;
> >>>>>>>>> > >>>> > >>+
> >>>>>>>>> > >>>> > >> port {
> >>>>>>>>> > >>>> > >> s5c73m3_1: endpoint {
> >>>>>>>>> > >>>> > >> data-lanes = <1
> >>>>>>>>> > >>>> > >> 2 3 4>;
> >>>>>>> > >>> > >
> >>>>>>> > >>> > >Oops. I missed this property would have ended to the
> >>>>>>> > >>> > >sensor's DT node. I
> >>>>>>> > >>> > >don't think we should have properties here that are parsed
> >>>>>>> > >>> > >by another
> >>>>>>> > >>> > >driver --- let's discuss this tomorrow.
> >>>>> > >> >
> >>>>> > >> > exynos4-is driver already parses sensor nodes (at least their
> >>>>> > >> > 'port'
> >>>>> > >> > sub-nodes).
> >>> > >
> >>> > > If you read the code and the comment, it looks like something that
> >>> > > should be
> >>> > > done better but hasn't been done yet. :-) That's something we should
> >>> > > avoid.
> >>> > > Also, flash devices are by far more common than external ISPs I
> >>> > > presume.
> >> >
> >> > Yes, especially let's not require any samsung specific properties in
> >> > other vendors' sensor bindings.
> >> >
> >> > One way of modelling [flash led]/[image sensor] association I imagine
> >> > would be to put, e.g. 'flash-leds' property in the SoC camera host
> >> > interface/ISP DT node. This property would then contain pairs of
> >> > phandles,
> >> > first to the led node and the second to the sensor node, e.g.
> >> >
> >> > i2c_controller {
> >> > ...
> >> > flash_xx@NN {
> >> > ...
> >> > led_a {
> >> > ...
> >> > }
> >> > };
> >> >
> >> > image_sensor_x@NN {
> >> > ...
> >> > };
> >> > };
> >> >
> >> > flash-leds = <&flash_xx &image_sensor_x>, <...>;
> >
> > Maybe a stupid question, but how do you access this in a driver? I have to
> > admit I'm no DT expert.
>
> You could get of_node pointers with of_parse_phandle() call and then
> lookup related flash and sensor devices based on that.
Ack. Looks good to me.
> >> > For the purpose of this patch set presumably just samsung specific
> >> > property name could be used (i.e. samsung,flash-leds).
> >
> > I agree. I'll add similar support for the omap3isp driver in the near future
> > though. Let's see how the camera modules will get modelled, if they will,
> > and if this property still fits to the picture by that time, then we make it
> > more generic.
> >
> > What do you think?
>
> I think we could do that, perhaps we could get some more opinions and
> use generic name already in this series? I'm not sure what are exact
> plans for this series, I guess it is targeted for 4.2?
There have been very few opinions expressed besides yours, mine and Jacek's,
unfortunately. I'm also not very certain on the future-proofness of this
solution until we have better understanding of how modules would best be
expressed in DT.
v4.2 would be nice target for these, yes.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 8/8] DT: samsung-fimc: Add examples for samsung,flash-led property
Hi Sylwester,
On Thu, May 21, 2015 at 03:28:40PM +0200, Sylwester Nawrocki wrote:
> On 21/05/15 13:32, Sakari Ailus wrote:
> >>>> @@ -147,6 +149,8 @@ Example:
> >>>> > >> clocks = <&camera 0>;
> >>>> > >> clock-names = "mclk";
> >>>> > >>
> >>>> > >>+samsung,flash-led = <&rear_cam_flash>;
> >>>> > >>+
> >>>> > >> port {
> >>>> > >> s5c73m3_1: endpoint {
> >>>> > >> data-lanes = <1 2 3 4>;
> >>> > >
> >>> > >Oops. I missed this property would have ended to the sensor's DT node.
> >>> > >I
> >>> > >don't think we should have properties here that are parsed by another
> >>> > >driver --- let's discuss this tomorrow.
> >> >
> >> > exynos4-is driver already parses sensor nodes (at least their 'port'
> >> > sub-nodes).
> >
> > If you read the code and the comment, it looks like something that should be
> > done better but hasn't been done yet. :-) That's something we should avoid.
> > Also, flash devices are by far more common than external ISPs I presume.
>
> Yes, especially let's not require any samsung specific properties in
> other vendors' sensor bindings.
>
> One way of modelling [flash led]/[image sensor] association I imagine
> would be to put, e.g. 'flash-leds' property in the SoC camera host
> interface/ISP DT node. This property would then contain pairs of phandles,
> first to the led node and the second to the sensor node, e.g.
>
> i2c_controller {
> ...
> flash_xx@NN {
> ...
> led_a {
> ...
> }
> };
>
> image_sensor_x@NN {
> ...
> };
> };
>
> flash-leds = <&flash_xx &image_sensor_x>, <...>;
Maybe a stupid question, but how do you access this in a driver? I have to
admit I'm no DT expert.
>
> For the purpose of this patch set presumably just samsung specific
> property name could be used (i.e. samsung,flash-leds).
I agree. I'll add similar support for the omap3isp driver in the near future
though. Let's see how the camera modules will get modelled, if they will,
and if this property still fits to the picture by that time, then we make it
more generic.
What do you think?
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 8/8] DT: samsung-fimc: Add examples for samsung,flash-led property
Hi Jacek,
On Thu, May 21, 2015 at 11:10:49AM +0200, Jacek Anaszewski wrote:
> Hi Sakari,
>
> On 05/21/2015 12:00 AM, Sakari Ailus wrote:
> >Hi Jacek,
> >
> >On Wed, May 20, 2015 at 04:10:15PM +0200, Jacek Anaszewski wrote:
> >>This patch adds examples for samsung,flash-led property to the
> >>samsung-fimc.txt.
> >>
> >>Signed-off-by: Jacek Anaszewski
> >>Acked-by: Kyungmin Park
> >>Cc: Sylwester Nawrocki
> >>Cc: devicetree@vger.kernel.org
> >>---
> >> .../devicetree/bindings/media/samsung-fimc.txt |4
> >> 1 file changed, 4 insertions(+)
> >>
> >>diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt
> >>b/Documentation/devicetree/bindings/media/samsung-fimc.txt
> >>index 922d6f8..57edffa 100644
> >>--- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
> >>+++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
> >>@@ -126,6 +126,8 @@ Example:
> >>clocks = <&camera 1>;
> >>clock-names = "mclk";
> >>
> >>+ samsung,flash-led = <&front_cam_flash>;
> >>+
> >>port {
> >>s5k6aa_ep: endpoint {
> >>remote-endpoint = <&fimc0_ep>;
> >>@@ -147,6 +149,8 @@ Example:
> >>clocks = <&camera 0>;
> >>clock-names = "mclk";
> >>
> >>+ samsung,flash-led = <&rear_cam_flash>;
> >>+
> >>port {
> >>s5c73m3_1: endpoint {
> >>data-lanes = <1 2 3 4>;
> >
> >Oops. I missed this property would have ended to the sensor's DT node. I
> >don't think we should have properties here that are parsed by another
> >driver --- let's discuss this tomorrow.
>
> exynos4-is driver already parses sensor nodes (at least their 'port'
> sub-nodes).
If you read the code and the comment, it looks like something that should be
done better but hasn't been done yet. :-) That's something we should avoid.
Also, flash devices are by far more common than external ISPs I presume.
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 8/8] DT: samsung-fimc: Add examples for samsung,flash-led property
Hi Jacek,
On Wed, May 20, 2015 at 04:10:15PM +0200, Jacek Anaszewski wrote:
> This patch adds examples for samsung,flash-led property to the
> samsung-fimc.txt.
>
> Signed-off-by: Jacek Anaszewski
> Acked-by: Kyungmin Park
> Cc: Sylwester Nawrocki
> Cc: devicetree@vger.kernel.org
> ---
> .../devicetree/bindings/media/samsung-fimc.txt |4
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt
> b/Documentation/devicetree/bindings/media/samsung-fimc.txt
> index 922d6f8..57edffa 100644
> --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
> +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
> @@ -126,6 +126,8 @@ Example:
> clocks = <&camera 1>;
> clock-names = "mclk";
>
> + samsung,flash-led = <&front_cam_flash>;
> +
> port {
> s5k6aa_ep: endpoint {
> remote-endpoint = <&fimc0_ep>;
> @@ -147,6 +149,8 @@ Example:
> clocks = <&camera 0>;
> clock-names = "mclk";
>
> + samsung,flash-led = <&rear_cam_flash>;
> +
> port {
> s5c73m3_1: endpoint {
> data-lanes = <1 2 3 4>;
Oops. I missed this property would have ended to the sensor's DT node. I
don't think we should have properties here that are parsed by another
driver --- let's discuss this tomorrow.
There are two main options that I can think of --- either put the property
under the bridge (ISP) driver's device node as a temporary solution that
works on a few ISP drivers, or think how sensor modules should be modelled,
in which case we'd have some idea how lens device would be taken into
account.
Cc Sebastian.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 04/10] DT: Add documentation for the Skyworks AAT1290
On Wed, Apr 15, 2015 at 08:48:34AM +0200, Jacek Anaszewski wrote: > This patch adds device tree binding documentation for > 1.5A Step-Up Current Regulator for Flash LEDs. > > Signed-off-by: Jacek Anaszewski > Acked-by: Kyungmin Park > Cc: Bryan Wu > Cc: Richard Purdie > Cc: devicetree@vger.kernel.org Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 1/1] media: i2c/adp1653: Devicetree support for adp1653
Hi Sebastian,
On Thu, Apr 16, 2015 at 07:24:42AM +0200, Sebastian Reichel wrote:
> Hi Sakari,
>
> Since this driver won't make it into 4.1 anyways, I have one more
> comment:
>
> On Thu, Apr 16, 2015 at 02:37:13AM +0300, Sakari Ailus wrote:
...
> > @@ -308,16 +311,28 @@ __adp1653_set_power(struct adp1653_flash *flash, int
> > on)
> > {
> > int ret;
> >
> > - ret = flash->platform_data->power(&flash->subdev, on);
> > - if (ret < 0)
> > - return ret;
> > + if (flash->platform_data->power) {
> > + ret = flash->platform_data->power(&flash->subdev, on);
> > + if (ret < 0)
> > + return ret;
> > + } else {
> > + gpiod_set_value(flash->platform_data->enable_gpio, on);
> > + if (on)
> > + /* Some delay is apparently required. */
> > + udelay(20);
> > + }
>
> I suggest to remove the power callback from platform data. Instead
> you can require to setup a gpiod lookup table in the boardcode, if
> platform data based initialization is used (see for example si4713
> initialization in board-rx51-periphals.c).
>
> This will reduce complexity in the driver and should be fairly easy
> to implement, since there is no adp1653 platform code user in the
> mainline kernel anyways.
There are a couple of out-of-tree users perhaps. I think that I'd rather
remove platform data support altogether than trying to polish it.
The timeline could be about the same than with the omap3isp driver, that
shouldn't be too many minor kernel versions either.
What do you think?
...
> > diff --git a/include/media/adp1653.h b/include/media/adp1653.h
> > index 1d9b48a..9779c85 100644
> > --- a/include/media/adp1653.h
> > +++ b/include/media/adp1653.h
> > @@ -100,9 +100,11 @@ struct adp1653_platform_data {
> > int (*power)(struct v4l2_subdev *sd, int on);
> >
> > u32 max_flash_timeout; /* flash light timeout in us */
> > - u32 max_flash_intensity;/* led intensity, flash mode */
> > - u32 max_torch_intensity;/* led intensity, torch mode */
> > - u32 max_indicator_intensity;/* indicator led intensity */
> > + u32 max_flash_intensity;/* led intensity, flash mode, mA */
> > + u32 max_torch_intensity;/* led intensity, torch mode, mA */
> > + u32 max_indicator_intensity;/* indicator led intensity, uA */
> > +
> > + struct gpio_desc *enable_gpio; /* for device-tree based boot */
>
> IMHO this should become part of "struct adp1653_flash", so that
> adp1653_platform_data only contains variables, which should be
> filled by boardcode / manual DT parsing code.
We'll get rid of the whole header with platform data support removal. :-)
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v8 1/1] media: i2c/adp1653: Devicetree support for adp1653
From: Pavel Machek
Add device tree support for adp1653 flash LED driver.
Signed-off-by: Pavel Machek
Signed-off-by: Sakari Ailus
---
Hi folks,
Here's an updated adp1653 DT patch, with changes since v7:
- Include of.h and gpio/consumer.h instead of of_gpio.h and gpio.h.
- Don't initialise ret as zero in __adp1653_set_power(), check ret only when
it's been set.
- Don't check for node non-NULL in adp1653_of_init(). It never is NULL.
- Remove temporary variable val in adp1653_of_init().
- If the device has no of_node, check that platform data is non-NULL;
otherwise return an error.
- Assign flash->platform_data only if dev->of_node is NULL.
drivers/media/i2c/adp1653.c | 100 ++-
include/media/adp1653.h |8 ++--
2 files changed, 95 insertions(+), 13 deletions(-)
diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
index 873fe19..c70abab 100644
--- a/drivers/media/i2c/adp1653.c
+++ b/drivers/media/i2c/adp1653.c
@@ -8,6 +8,7 @@
* Contributors:
* Sakari Ailus
* Tuukka Toivonen
+ * Pavel Machek
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU General Public License
@@ -34,6 +35,8 @@
#include
#include
#include
+#include
+#include
#include
#include
@@ -308,16 +311,28 @@ __adp1653_set_power(struct adp1653_flash *flash, int on)
{
int ret;
- ret = flash->platform_data->power(&flash->subdev, on);
- if (ret < 0)
- return ret;
+ if (flash->platform_data->power) {
+ ret = flash->platform_data->power(&flash->subdev, on);
+ if (ret < 0)
+ return ret;
+ } else {
+ gpiod_set_value(flash->platform_data->enable_gpio, on);
+ if (on)
+ /* Some delay is apparently required. */
+ udelay(20);
+ }
if (!on)
return 0;
ret = adp1653_init_device(flash);
- if (ret < 0)
+ if (ret >= 0)
+ return ret;
+
+ if (flash->platform_data->power)
flash->platform_data->power(&flash->subdev, 0);
+ else
+ gpiod_set_value(flash->platform_data->enable_gpio, 0);
return ret;
}
@@ -407,21 +422,85 @@ static int adp1653_resume(struct device *dev)
#endif /* CONFIG_PM */
+static int adp1653_of_init(struct i2c_client *client,
+ struct adp1653_flash *flash,
+ struct device_node *node)
+{
+ struct adp1653_platform_data *pd;
+ struct device_node *child;
+
+ pd = devm_kzalloc(&client->dev, sizeof(*pd), GFP_KERNEL);
+ if (!pd)
+ return -ENOMEM;
+ flash->platform_data = pd;
+
+ child = of_get_child_by_name(node, "flash");
+ if (!child)
+ return -EINVAL;
+
+ if (of_property_read_u32(child, "flash-timeout-us",
+&pd->max_flash_timeout))
+ goto err;
+
+ if (of_property_read_u32(child, "flash-max-microamp",
+&pd->max_flash_intensity))
+ goto err;
+
+ pd->max_flash_intensity /= 1000;
+
+ if (of_property_read_u32(child, "led-max-microamp",
+&pd->max_torch_intensity))
+ goto err;
+
+ pd->max_torch_intensity /= 1000;
+ of_node_put(child);
+
+ child = of_get_child_by_name(node, "indicator");
+ if (!child)
+ return -EINVAL;
+
+ if (of_property_read_u32(child, "led-max-microamp",
+&pd->max_indicator_intensity))
+ goto err;
+
+ of_node_put(child);
+
+ pd->enable_gpio = devm_gpiod_get(&client->dev, "enable");
+ if (!pd->enable_gpio) {
+ dev_err(&client->dev, "Error getting GPIO\n");
+ return -EINVAL;
+ }
+
+ return 0;
+err:
+ dev_err(&client->dev, "Required property not found\n");
+ of_node_put(child);
+ return -EINVAL;
+}
+
+
static int adp1653_probe(struct i2c_client *client,
const struct i2c_device_id *devid)
{
struct adp1653_flash *flash;
int ret;
- /* we couldn't work without platform data */
- if (client->dev.platform_data == NULL)
- return -ENODEV;
-
flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
if (flash == NULL)
return -ENOMEM;
- flash->platform_data = client->dev.platform_data;
+ if (client->dev.of_node) {
+ ret = adp1653_of_in
Re: [PATCH v5 02/10] DT: Add documentation for the mfd Maxim max77693
Hi Jacek, On Wed, Apr 15, 2015 at 08:48:32AM +0200, Jacek Anaszewski wrote: > This patch adds device tree binding documentation for > the flash cell of the Maxim max77693 multifunctional device. > > Signed-off-by: Jacek Anaszewski > Signed-off-by: Andrzej Hajda > Acked-by: Kyungmin Park > Cc: Lee Jones > Cc: Chanwoo Choi > Cc: Bryan Wu > Cc: Richard Purdie > Cc: devicetree@vger.kernel.org Thanks! Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] DT: leds: Improve description of flash LEDs related properties
Hi Jacek and Sylwester, Jacek Anaszewski wrote: > On 04/08/2015 05:15 PM, Sylwester Nawrocki wrote: >> On 08/04/15 16:31, Jacek Anaszewski wrote: >>> Properties defining maximum values for LED currents and timeout should >>> be mandatory to avoid the risk of hardware damage. This patch fixes >>> the issue. >>> >>> Signed-off-by: Jacek Anaszewski >>> Acked-by: Kyungmin Park >> >>> --- >>> Documentation/devicetree/bindings/leds/common.txt | 19 >>> +++ >>> 1 file changed, 11 insertions(+), 8 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/leds/common.txt >>> b/Documentation/devicetree/bindings/leds/common.txt >>> index 747c538..e478ac6 100644 >>> --- a/Documentation/devicetree/bindings/leds/common.txt >>> +++ b/Documentation/devicetree/bindings/leds/common.txt >>> @@ -10,6 +10,17 @@ can influence the way of the LED device >>> initialization, the LED components >>> have to be tightly coupled with the LED device binding. They are >>> represented >>> by child nodes of the parent LED device binding. >>> >>> +Required properties for child nodes: >> >> These properties are mandatory only for LEDs with Flash/Torch >> capabilities, >> aren't they? > > flash-max-microamp and flash-timeout-us properties description mention > that they refer to flash LEDs. Perhaps this should be made indeed more > explicit. Sounds good to me, after all these shouldn't be defined for devices they don't make sense for. Alternatively, you could add another section for "required properties for flash LEDs". > >> Requiring those properties for all led nodes would make all >> current dtses not compliant with the DT binding specification AFAICT. Some drivers also define these explicitly as optional, the behaviour for those obviously can't be changed. This should apply to new drivers only, and I think documenting the matter in this file would be the best way to do that. > > I was also worrying about making led-max-microamp required, but others > didn't share my objections. I think that we have to reexamine this. > > Please refer to the discussion under [PATCH v4]. The role of this > led-max-microamp property would be preventing hardware damage. > > >> How about: >> >> "Required properties for child nodes for LEDs with Flash/Torch >> capabilities:" ? >> >>> +- led-max-microamp : Maximum LED supply current in microamperes >>> + (torch LED for flash devices). Controllers that Do you think we can just rename max-microamp as led-max-microamp? It's been there since v3.19. Or is it because no driver has been using it? >>> have no >>> + configurable current can omit this property. >>> +- flash-max-microamp : Maximum flash LED supply current in >>> microamperes. >>> +- flash-timeout-us : Timeout in microseconds after which the flash >>> + LED is turned off. >>> + >> >>> +Above properties determine a LED driver IC settings required for safe >>> +operation. They should be also used as the initial settings for the IC. >> >> Shouldn't "Controllers that have no configurable current can omit this >> property" refer to both led-max-microamp and flash-max-microamp? >> >> I would drop the "Above...for the IC." paragraph and instead add >> something like: >> >> "For controllers that have no configurable current the led-max-microamp, >> flash-max-microamp properties respectively can be omitted. For >> controllers that >> have no configurable timeout the flash-timeout-us property can be >> omitted." > > Are we going to avoid mentioning about their role in preventing > hardware damage (former "for safe operation" sequence)? That'd make it understandable why the properties are mandatory. Sounds good to me. -- Kind regards, Sakari Ailus sakari.ai...@linux.intel.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 06/12] of: Add Skyworks Solutions, Inc. vendor prefix
On Tue, Mar 31, 2015 at 03:52:42PM +0200, Jacek Anaszewski wrote: > Use "skyworks" as the vendor prefix for the Skyworks Solutions, Inc. > > Signed-off-by: Jacek Anaszewski > Acked-by: Kyungmin Park > Cc: devicetree@vger.kernel.org > --- > .../devicetree/bindings/vendor-prefixes.txt|1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt > b/Documentation/devicetree/bindings/vendor-prefixes.txt > index 42b3dab..4cd18bb 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt > @@ -163,6 +163,7 @@ ricoh Ricoh Co. Ltd. > rockchip Fuzhou Rockchip Electronics Co., Ltd > samsung Samsung Semiconductor > sandisk Sandisk Corporation > +skyworks Skyworks Solutions, Inc. Please maintain the alphabetic order. With that fixed, Acked-by: Sakari Ailus > sbs Smart Battery System > schindler Schindler > seagate Seagate Technology PLC -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 01/12] DT: leds: Improve description of flash LEDs related properties
Hi Jacek, On Wed, Apr 08, 2015 at 12:23:23PM +0200, Jacek Anaszewski wrote: > Hi Sakari, > > On 04/08/2015 11:11 AM, Sakari Ailus wrote: > >Hi Jacek, > > > >On Wed, Apr 08, 2015 at 10:54:52AM +0200, Jacek Anaszewski wrote: > >>Hi Sakari, > >> > >>On 04/03/2015 02:09 PM, Sakari Ailus wrote: > >>>Hi Jacek, > >>> > >>>On Tue, Mar 31, 2015 at 03:52:37PM +0200, Jacek Anaszewski wrote: > >>>>Description of flash LEDs related properties was not precise regarding > >>>>the state of corresponding settings in case a property is missing. > >>>>Add relevant statements. > >>>>Removed is also the requirement making the flash-max-microamp > >>>>property obligatory for flash LEDs. It was inconsistent as the property > >>>>is defined as optional. Devices which require the property will have > >>>>to assert this in their DT bindings. > >>>> > >>>>Signed-off-by: Jacek Anaszewski > >>>>Acked-by: Kyungmin Park > >>>>Cc: Bryan Wu > >>>>Cc: Richard Purdie > >>>>Cc: Pavel Machek > >>>>Cc: Sakari Ailus > >>>>Cc: devicetree@vger.kernel.org > >>>>--- > >>>> Documentation/devicetree/bindings/leds/common.txt | 16 > >>>> +--- > >>>> 1 file changed, 9 insertions(+), 7 deletions(-) > >>>> > >>>>diff --git a/Documentation/devicetree/bindings/leds/common.txt > >>>>b/Documentation/devicetree/bindings/leds/common.txt > >>>>index 747c538..21a25e4 100644 > >>>>--- a/Documentation/devicetree/bindings/leds/common.txt > >>>>+++ b/Documentation/devicetree/bindings/leds/common.txt > >>>>@@ -29,13 +29,15 @@ Optional properties for child nodes: > >>>> "ide-disk" - LED indicates disk activity > >>>> "timer" - LED flashes at a fixed, configurable rate > >>>> > >>>>-- max-microamp : maximum intensity in microamperes of the LED > >>>>- (torch LED for flash devices) > >>>>-- flash-max-microamp : maximum intensity in microamperes of the > >>>>- flash LED; it is mandatory if the LED should > >>>>-support the flash mode > >>>>-- flash-timeout-us : timeout in microseconds after which the flash > >>>>- LED is turned off > >>>>+- max-microamp : Maximum intensity in microamperes of the LED > >>>>+ (torch LED for flash devices). If omitted this will default > >>>>+ to the maximum current allowed by the device. > >>>>+- flash-max-microamp : Maximum intensity in microamperes of the flash > >>>>LED. > >>>>+If omitted this will default to the maximum > >>>>+current allowed by the device. > >>>>+- flash-timeout-us : Timeout in microseconds after which the flash > >>>>+ LED is turned off. If omitted this will default to > >>>>the > >>>>+ maximum timeout allowed by the device. > >>>> > >>>> > >>>> Examples: > >>> > >>>Pavel pointed out that the brightness between maximum current and the > >>>maximum *allowed* another current might not be noticeable, leading a > >>>potential spelling error to cause the LED being run at too high current. > >> > >>I think that a board designed so that it can be damaged because of > >>software bugs should be considered not eligible for commercial use. > >>Any self-esteeming manufacturer will not connect a LED to the output > >>that can produce the current greater than the LED's absolute maximum > >>current. > > > >The maximum current *is* used to prevent potential hardware damage. > > What hardware are we talking about - LED controller or the discrete > LED component attached to the LED controller's current output? Generally the LED itself, not the controller. Most controllers have overheating protection while the LEDs do not. > > The maximum current the LED controller can produce is fixed or depends > on external components like resistors. On some controllers perhaps, but not on most of them. > > This is at least the case for max77693 and aat1290 device I've been > working with. If a LED is rated to max 1A and it will be connected > to the out
Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties
Hi Jacek, On Wed, Apr 08, 2015 at 10:54:52AM +0200, Jacek Anaszewski wrote: > Hi Sakari, > > On 04/03/2015 02:09 PM, Sakari Ailus wrote: > >Hi Jacek, > > > >On Tue, Mar 31, 2015 at 03:52:37PM +0200, Jacek Anaszewski wrote: > >>Description of flash LEDs related properties was not precise regarding > >>the state of corresponding settings in case a property is missing. > >>Add relevant statements. > >>Removed is also the requirement making the flash-max-microamp > >>property obligatory for flash LEDs. It was inconsistent as the property > >>is defined as optional. Devices which require the property will have > >>to assert this in their DT bindings. > >> > >>Signed-off-by: Jacek Anaszewski > >>Acked-by: Kyungmin Park > >>Cc: Bryan Wu > >>Cc: Richard Purdie > >>Cc: Pavel Machek > >>Cc: Sakari Ailus > >>Cc: devicetree@vger.kernel.org > >>--- > >> Documentation/devicetree/bindings/leds/common.txt | 16 +--- > >> 1 file changed, 9 insertions(+), 7 deletions(-) > >> > >>diff --git a/Documentation/devicetree/bindings/leds/common.txt > >>b/Documentation/devicetree/bindings/leds/common.txt > >>index 747c538..21a25e4 100644 > >>--- a/Documentation/devicetree/bindings/leds/common.txt > >>+++ b/Documentation/devicetree/bindings/leds/common.txt > >>@@ -29,13 +29,15 @@ Optional properties for child nodes: > >> "ide-disk" - LED indicates disk activity > >> "timer" - LED flashes at a fixed, configurable rate > >> > >>-- max-microamp : maximum intensity in microamperes of the LED > >>-(torch LED for flash devices) > >>-- flash-max-microamp : maximum intensity in microamperes of the > >>- flash LED; it is mandatory if the LED should > >>- support the flash mode > >>-- flash-timeout-us : timeout in microseconds after which the flash > >>- LED is turned off > >>+- max-microamp : Maximum intensity in microamperes of the LED > >>+(torch LED for flash devices). If omitted this will default > >>+to the maximum current allowed by the device. > >>+- flash-max-microamp : Maximum intensity in microamperes of the flash LED. > >>+ If omitted this will default to the maximum > >>+ current allowed by the device. > >>+- flash-timeout-us : Timeout in microseconds after which the flash > >>+ LED is turned off. If omitted this will default to the > >>+maximum timeout allowed by the device. > >> > >> > >> Examples: > > > >Pavel pointed out that the brightness between maximum current and the > >maximum *allowed* another current might not be noticeable, leading a > >potential spelling error to cause the LED being run at too high current. > > I think that a board designed so that it can be damaged because of > software bugs should be considered not eligible for commercial use. > Any self-esteeming manufacturer will not connect a LED to the output > that can produce the current greater than the LED's absolute maximum > current. The maximum current *is* used to prevent potential hardware damage. This is how mobile phones typically are, probably also the one you're using. :-) I don't believe there's really a difference between vendors in this respect. We still lack a proper way to model the temperature of the flash LED, so what we have now is a bit incomplete, but at least it prevents causing damage unintentionally. > The DT properties could be useful for devices like aat1290 device I was > writing a driver for, which has the maximum current and timeout values > depending on corresponding capacitor and resistor values respectively. > Such devices should make the properties required in their bindings. > > >The three drivers I've looked also require these properties, which I think > >is in the line with the above. > > > >How about either dropping the patch, or changing maximum to minimum and > >will to should? The drivers could also behave this way instead of requiring > >the properties, but I don't think there's anything wrong with requiring the > >properties either. > > As I mentioned in the previous message in this subject, the max-microamp > property refers also to non-flash LEDs. Since existing LED class devices > does not require them, then it should be left optional and default to > max. It would however be inconsistent with flash LEDs related > properties. I do agree with Pavel here, these should be mandatory (at least for new drivers) OR default to minimum. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 01/12] DT: leds: Improve description of flash LEDs related properties
Hi Bryan, Bryan Wu wrote: > On Fri, Apr 3, 2015 at 1:37 PM, Pavel Machek wrote: >> Hi! >> >>>>> +- flash-timeout-us : Timeout in microseconds after which the flash >>>>> + LED is turned off. If omitted this will default to >>>>> the >>>>> +maximum timeout allowed by the device. >>>>> >>>>> >>>>> Examples: >>>> >>>> Pavel pointed out that the brightness between maximum current and the >>>> maximum *allowed* another current might not be noticeable,leading a >>>> potential spelling error to cause the LED being run at too high current. >>> >>> Where did he point this out? Do you think about the current version >>> of the leds/common.txt documentation or there was some other message, >>> that I don't see? >> >> Date: Thu, 2 Apr 2015 22:30:44 +0200 >> From: Pavel Machek >> To: Sakari Ailus >> Subject: Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653 >> >>> Besides, I can't understand your point. Could you express it in other >>> words, please? >> >> Typo in device tree would cause hardware damage. But idea. Make the >> properties mandatory. >> Pavel > > I don't quite follow there. I think Pavel acked this patch right? So > what's left to hold here? LED flash controllers are capable of providing more current than the LEDs attached to them can withstand without hardware damage. This is the reason the maximum current limits lower than the LED controller maximums are there. Pavel, quite rightly so in my opinion, is suggesting these properties are made mandatory. A typo in the DT source could cause the controller maximum current used instead of LED maximum which is often lower. That kind of a problem would easily go unnoticed since there isn't necessarily any perceivable change in the functionality of the board. You'd only notice later on, when the LEDs stop working. -- Regards, Sakari Ailus sakari.ai...@linux.intel.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653
On Sat, Apr 04, 2015 at 07:11:16PM +0200, Pavel Machek wrote: > Hi! > > > enable-gpios: Specifier of the GPIO connected to EN pin > > > > I can make the changes if you're ok with that, otherwise please send v7. > > Then > > I'll apply that to my tree. > > I'm ok with that. Thanks. The patch is applied here: git://linuxtv.org/sailus/media_tree.git, branch adp1653 -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653
Hi Pavel, On Sat, Apr 04, 2015 at 09:43:37AM +0200, Pavel Machek wrote: > > Documentation for adp1653 binding. s/binding/bindings/ > > Signed-off-by: Pavel Machek > > --- > > Please apply. > > Sorry, wrong version of patch was sent last time. > Pavel > > diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt > b/Documentation/devicetree/bindings/media/i2c/adp1653.txt > new file mode 100644 > index 000..4607ce3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt > @@ -0,0 +1,37 @@ > +* Analog Devices ADP1653 flash LED driver > + > +Required Properties: > + > + - compatible: Must contain be "adi,adp1653" s/be // > + > + - reg: I2C slave address > + > + - enable-gpios: Reference to the GPIO that controls the power for the chip. How about: enable-gpios: Specifier of the GPIO connected to EN pin I can make the changes if you're ok with that, otherwise please send v7. Then I'll apply that to my tree. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653
On Fri, Apr 03, 2015 at 10:26:24PM +0200, Pavel Machek wrote:
> > > + - power-gpios: Reference to the GPIO that controls the power for the
> > > chip.
> >
> > You're using power-gpios in documentation only.
>
> Which is ok, because generic code adds "-gpios" itself.
Do you think you need this part:
+ if (!of_find_property(node, "gpios", NULL)) {
+ dev_err(&client->dev, "No gpio node\n");
+ return -EINVAL;
+ }
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv6] media: i2c/adp1653: Documentation for devicetree support for adp1653
Hi Pavel,
On Fri, Apr 03, 2015 at 10:26:24PM +0200, Pavel Machek wrote:
>
> Documentation for adp1653 binding.
>
> ---
>
> > Please split this as Javier suggested. I'd think both could go through
> > the media-tree unless someone objects.
>
> Please apply.
>
> > > + - power-gpios: Reference to the GPIO that controls the power for the
> > > chip.
> >
> > You're using power-gpios in documentation only.
>
> Which is ok, because generic code adds "-gpios" itself.
>
> > The spec refers to this by "EN". How about "en-gpios" instead? This
> > definitely isn't about power, but about resetting the chip. It gets the
> > power through another pin.
>
> It controls power of the chip. Noone gets _power_ through gpios,
> hopefully. Yes, I can rename it. "en-gpios" is too ugly to
> live. Sebastian suggested "enable". Hope that's okay with you.
"enable-gpios" sounds fine for me.
>
> Pavel
>
> new file mode 100644
> index 000..da9934a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> + - compatible: Must contain be "adi,adp1653"
> +
> + - reg: I2C slave address
> +
> + - power-gpios: Reference to the GPIO that controls the power for the chip.
> +
> +There are two LED outputs available - flash and indicator. One LED is
> +represented by one child node, nodes need to be named "flash" and
> "indicator".
> +
> +Required properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Required properties of the flash LED child node:
> +
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +
> + adp1653: led-controller@30 {
> + compatible = "adi,adp1653";
> + reg = <0x30>;
> + power-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
> +
> + flash {
> + flash-timeout-us = <50>;
> + flash-max-microamp = <32>;
> + max-microamp = <5>;
> + };
> + indicator {
> + max-microamp = <17500>;
> + };
> + };
>
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653
On Fri, Apr 03, 2015 at 10:29:53PM +0200, Pavel Machek wrote: > On Fri 2015-04-03 14:23:56, Sakari Ailus wrote: > > Hi Pavel, > > > > On Fri, Apr 03, 2015 at 10:23:44AM +0200, Pavel Machek wrote: > > > Hi! > > > > > > > Hi Pawel, > > > > > > I'm still Pavel. v, not w. > > > > I know too many Pawels. Sorry about that. :-) > > > > > > I guess it uses adp1653_id_table. I'd hade to add redundand > > > information, because if it would just mask the errors if the code > > > changed... > > > > Indeed, that's true. This is comparing "adp1653" vs. comparing > > "adi,adp1653". I think I still prefer the latter since it's got also the > > vendor prefix included. > > > > Suppose we change this later and someone misspelled the vendor prefix --- > > their board would break. > > Suppose we do what you suggest. That does not fix the problem, since > code will still match the "adp1653" in case someone misspells it. > > If you want to change how i2c device matching works, well, you can do > it, but my patch is not right place to do that. Good point. Let's leave it as-is. -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 09/11] DT: Add documentation for exynos4-is 'flashes' property
Hi Jacek, On Wed, Mar 25, 2015 at 09:52:02AM +0100, Jacek Anaszewski wrote: > Hi Sakari, > > On 03/25/2015 02:06 AM, Sakari Ailus wrote: > >Hi Jacek, > > > >On Fri, Mar 20, 2015 at 04:03:29PM +0100, Jacek Anaszewski wrote: > >>This patch adds a description of 'flashes' property > >>to the samsung-fimc.txt. > >> > >>Signed-off-by: Jacek Anaszewski > >>Acked-by: Kyungmin Park > >>Cc: Sylwester Nawrocki > >>--- > >> .../devicetree/bindings/media/samsung-fimc.txt |8 > >> 1 file changed, 8 insertions(+) > >> > >>diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt > >>b/Documentation/devicetree/bindings/media/samsung-fimc.txt > >>index 922d6f8..cb0e263 100644 > >>--- a/Documentation/devicetree/bindings/media/samsung-fimc.txt > >>+++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt > >>@@ -40,6 +40,13 @@ should be inactive. For the "active-a" state the camera > >>port A must be activated > >> and the port B deactivated and for the state "active-b" it should be the > >> other > >> way around. > >> > >>+Optional properties: > >>+ > >>+- flashes - Array of phandles to the flash LEDs that can be controlled by > >>the > >>+ sub-devices contained in this media device. Flash LED is > >>+ represented by a child node of a flash LED device > > > >This should be in > >Documentation/devicetree/bindings/media/video-interfaces.txt. > > > >Should flash devices be associated with sensors somehow rather than ISPs? > >That's how they commonly are arranged, however that doesn't limit placing > >them in silly places. > > > >I'm not necessarily saying the flashes-property should be present in > >sensor's DT nodes, but it'd be good to be able to make the association if > >it's there. > > I know of a SoC, which drives the flash from its on-chip ISP. The GPIO > connected to the flash controller's external strobe pin can be > configured so that the signal is routed to it from the ISP or from > CPU (for software strobe mode). > > I think that Sylwester could say more in this subject. Yeah, some ISPs can also generate strobe signals. Otherwise this would be the job of the sensor, sadly many module vendors ignore the sensor strobe signal routing even when it's available. The sensor is by far in the best position to trigger the flash since it has the best readout timing information. Currently we have no way to express this, I think what's first needed is the data in the DT. Probably the flash driver and the driver that provides the strobe signal should both be aware of this. User space interface wise the strobe source control could be used --- we'd just need new menu entries. > > > >>+ (see Documentation/devicetree/bindings/leds/common.txt). > >>+ > >> The 'camera' node must include at least one 'fimc' child node. > >> > >> > >>@@ -166,6 +173,7 @@ Example: > >>clock-output-names = "cam_a_clkout", "cam_b_clkout"; > >>pinctrl-names = "default"; > >>pinctrl-0 = <&cam_port_a_clk_active>; > >>+ flashes = <&camera_flash>, <&system_torch>; > >>status = "okay"; > >> #address-cells = <1>; > >>#size-cells = <1>; > > > >There will be other kind of devices that have somewhat similar relationship. > >They just haven't been defined yet. Lens controllers or EEPROM for instance. > >The two are an integral part of a module, something which is not modelled in > >DT in any way, but perhaps should be. > > Do you suggest using more generic name than 'flashes'? So far I don't have a better suggestion for the name of the property. However, it'd be good to be able to associate the flash to a sensor. I wonder how wrong would it be to put the flashes property to the port node... -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 01/12] DT: leds: Improve description of flash LEDs related properties
Hi Jacek, On Tue, Mar 31, 2015 at 03:52:37PM +0200, Jacek Anaszewski wrote: > Description of flash LEDs related properties was not precise regarding > the state of corresponding settings in case a property is missing. > Add relevant statements. > Removed is also the requirement making the flash-max-microamp > property obligatory for flash LEDs. It was inconsistent as the property > is defined as optional. Devices which require the property will have > to assert this in their DT bindings. > > Signed-off-by: Jacek Anaszewski > Acked-by: Kyungmin Park > Cc: Bryan Wu > Cc: Richard Purdie > Cc: Pavel Machek > Cc: Sakari Ailus > Cc: devicetree@vger.kernel.org > --- > Documentation/devicetree/bindings/leds/common.txt | 16 +--- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt > b/Documentation/devicetree/bindings/leds/common.txt > index 747c538..21a25e4 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -29,13 +29,15 @@ Optional properties for child nodes: > "ide-disk" - LED indicates disk activity > "timer" - LED flashes at a fixed, configurable rate > > -- max-microamp : maximum intensity in microamperes of the LED > - (torch LED for flash devices) > -- flash-max-microamp : maximum intensity in microamperes of the > - flash LED; it is mandatory if the LED should > -support the flash mode > -- flash-timeout-us : timeout in microseconds after which the flash > - LED is turned off > +- max-microamp : Maximum intensity in microamperes of the LED > + (torch LED for flash devices). If omitted this will default > + to the maximum current allowed by the device. > +- flash-max-microamp : Maximum intensity in microamperes of the flash LED. > +If omitted this will default to the maximum > +current allowed by the device. > +- flash-timeout-us : Timeout in microseconds after which the flash > + LED is turned off. If omitted this will default to the > + maximum timeout allowed by the device. > > > Examples: Pavel pointed out that the brightness between maximum current and the maximum *allowed* another current might not be noticeable, leading a potential spelling error to cause the LED being run at too high current. The three drivers I've looked also require these properties, which I think is in the line with the above. How about either dropping the patch, or changing maximum to minimum and will to should? The drivers could also behave this way instead of requiring the properties, but I don't think there's anything wrong with requiring the properties either. I think this is worth considering now as we can't change this later without breaking something. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv5] media: i2c/adp1653: devicetree support for adp1653
Hi Pavel,
Thanks for the update.
On Fri, Apr 03, 2015 at 10:33:53AM +0200, Pavel Machek wrote:
>
> We are moving to device tree support on OMAP3, but that currently
> breaks ADP1653 driver. This adds device tree support, plus required
> documentation.
>
> Signed-off-by: Pavel Machek
>
> ---
>
> Switched to gpiod_, as requested by Javier.
>
> Please apply,
Let's properly review this first.
> Pavel
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> new file mode 100644
> index 000..da9934a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
Please split this as Javier suggested. I'd think both could go through
the media-tree unless someone objects.
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> + - compatible: Must contain be "adi,adp1653"
> +
> + - reg: I2C slave address
> +
> + - power-gpios: Reference to the GPIO that controls the power for the chip.
You're using power-gpios in documentation only.
The spec refers to this by "EN". How about "en-gpios" instead? This
definitely isn't about power, but about resetting the chip. It gets the
power through another pin.
> +
> +There are two LED outputs available - flash and indicator. One LED is
> +represented by one child node, nodes need to be named "flash" and
> "indicator".
> +
> +Required properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Required properties of the flash LED child node:
> +
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +
> + adp1653: led-controller@30 {
> + compatible = "adi,adp1653";
> + reg = <0x30>;
> + power-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
> +
> + flash {
> + flash-timeout-us = <50>;
> + flash-max-microamp = <32>;
> + max-microamp = <5>;
> + };
> + indicator {
> + max-microamp = <17500>;
> + };
> + };
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index 873fe19..ba7f43d 100644
> --- a/drivers/media/i2c/adp1653.c
> +++ b/drivers/media/i2c/adp1653.c
> @@ -8,6 +8,7 @@
> * Contributors:
> * Sakari Ailus
> * Tuukka Toivonen
> + * Pavel Machek
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License
> @@ -34,6 +35,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> #include
>
> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
> static int
> __adp1653_set_power(struct adp1653_flash *flash, int on)
> {
> - int ret;
> + int ret = 0;
> +
> + if (flash->platform_data->power) {
> + ret = flash->platform_data->power(&flash->subdev, on);
> + } else {
> + gpiod_set_value(flash->platform_data->power_gpio, on);
> + if (on)
> + /* Some delay is apparently required. */
> + udelay(20);
> + }
>
> - ret = flash->platform_data->power(&flash->subdev, on);
> if (ret < 0)
> return ret;
>
> @@ -316,8 +327,13 @@ __adp1653_set_power(struct adp1653_flash *flash, int on)
> return 0;
>
> ret = adp1653_init_device(flash);
> - if (ret < 0)
> + if (ret >= 0)
> + return ret;
> +
> + if (flash->platform_data->power)
> flash->platform_data->power(&flash->subdev, 0);
> + else
> + gpiod_set_value(flash->platform_data->power_gpio, 0);
>
> return ret;
> }
> @@ -407,21 +423,75 @@ static int adp1653_resume(struct device *dev)
>
> #endif /* CONFIG_PM */
>
> +static int adp1653_of_init(struct i2c_client *client,
> +struct adp1653_flash *flash,
> +struct device_node *node)
> +{
> + u32 val;
> + struct adp1653_platform_data *pd;
> + struct device_node *child;
> +
> + if (!node)
> + return -EINVAL;
> +
> + pd =
Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653
Hi Pavel, On Fri, Apr 03, 2015 at 10:23:44AM +0200, Pavel Machek wrote: > Hi! > > > Hi Pawel, > > I'm still Pavel. v, not w. I know too many Pawels. Sorry about that. :-) > > > > > Hi Pawel, > > > > > A corresponding change to the N900 dts would be very nice. > > > > > > Corresponding change to the dts will come in separate patch. Or do you > > > have n900 for testing? > > > > Yes, it should be a separate patch, I agree. > > > > I do have one but I can't say when I'd have time to test it. I'm fine with > > you having tested it though. > > > > > > I think you're missing change to > > > > adp1653_i2c_driver.driver.of_match_table. > > > > > > It actually worked for me, which means device tree somehow does it > > > magic. > > > > By magic? :-) It probably just ends up comparing the device and the driver > > names. How about adding the of_match_table? > > I guess it uses adp1653_id_table. I'd hade to add redundand > information, because if it would just mask the errors if the code > changed... Indeed, that's true. This is comparing "adp1653" vs. comparing "adi,adp1653". I think I still prefer the latter since it's got also the vendor prefix included. Suppose we change this later and someone misspelled the vendor prefix --- their board would break. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653
Hi Pawel,
On Thu, Apr 02, 2015 at 10:30:44PM +0200, Pavel Machek wrote:
> Hi!
>
> > Hi Pawel,
> >
> > My apologies for the very late reply.
> >
> > On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote:
> > >
> > >
> > > We are moving to device tree support on OMAP3, but that currently
> > > breaks ADP1653 driver. This adds device tree support, plus required
> > > documentation.
> > >
> > > Signed-off-by: Pavel Machek
> > >
> > > ---
> > >
> > > I'm not sure if it is device tree or media framework, either everyone
> > > waits for someone else, or noone really cares.
> >
> > Neither. Some people are unfortuantely very busy with many other things as
> > well. :-P
>
> Well.. Being busy is ok. Nitpicking is also ok. But both at the same
> time... is not good.
Good. Then we should be fine. :-)
>
> > > Andrew, can you just merge it?
> > >
> > > Please apply,
> >
> > Please wait just a while.
> >
> > I think we can merge this eventually through the linux-media tree, but
> > please first see the comments below.
> >
>
> > > +Required properties of the flash LED child node:
> > > +
> > > +- flash-max-microamp : see
> > > Documentation/devicetree/bindings/leds/common.txt
> > > +- flash-timeout-us : see
> > > Documentation/devicetree/bindings/leds/common.txt
> >
> > The documentation says that the maximum value is used if these values are
> > not specified. I think I'd make these optional.
>
> I'd rather not: when you make a typo in dts, it would supply maximum
> available current, potentially damaging the LED. You will not be able
> to tell brightness difference with naked eye...
Fine for me.
> > > __adp1653_set_power(struct adp1653_flash *flash, int on)
> > > {
> > > - int ret;
> > > + int ret = 0;
> > > +
> > > + if (flash->platform_data->power) {
> > > + ret = flash->platform_data->power(&flash->subdev, on);
> >
> > The power() callback should be dropped. It's controlling a GPIO. But that
> > can be done later on. The alternative is a patch before this one.
>
> I'd prefer to do it later; we want to keep functionality on N900
> without DTS, too.
Fine as well.
> > > flash = devm_kzalloc(&client->dev, sizeof(*flash), GFP_KERNEL);
> > > if (flash == NULL)
> > > return -ENOMEM;
> > >
> > > flash->platform_data = client->dev.platform_data;
> > > + if (!flash->platform_data) {
> >
> > I'd check whether dev->of_node is non-NULL instead.
>
> Ok.
>
> > > @@ -438,10 +510,10 @@ static int adp1653_probe(struct i2c_client *client,
> > > goto free_and_quit;
> > >
> > > flash->subdev.entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
> > > -
> >
> > I rather liked the newline here. Please don't remove it. :-)
>
> Ok.
>
> > > @@ -464,7 +536,7 @@ static const struct i2c_device_id adp1653_id_table[]
> > > = {
> > > };
> > > MODULE_DEVICE_TABLE(i2c, adp1653_id_table);
> > >
> > > -static struct dev_pm_ops adp1653_pm_ops = {
> > > +static const struct dev_pm_ops adp1653_pm_ops = {
> > > .suspend = adp1653_suspend,
> > > .resume = adp1653_resume,
> > > };
> > >
> > >
> >
> > A corresponding change to the N900 dts would be very nice.
>
> Corresponding change to the dts will come in separate patch. Or do you
> have n900 for testing?
Yes, it should be a separate patch, I agree.
I do have one but I can't say when I'd have time to test it. I'm fine with
you having tested it though.
> > I think you're missing change to adp1653_i2c_driver.driver.of_match_table.
>
> It actually worked for me, which means device tree somehow does it
> magic.
By magic? :-) It probably just ends up comparing the device and the driver
names. How about adding the of_match_table?
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653
Hi Pawel,
My apologies for the very late reply.
On Thu, Apr 02, 2015 at 04:38:46PM +0200, Pavel Machek wrote:
>
>
> We are moving to device tree support on OMAP3, but that currently
> breaks ADP1653 driver. This adds device tree support, plus required
> documentation.
>
> Signed-off-by: Pavel Machek
>
> ---
>
> I'm not sure if it is device tree or media framework, either everyone
> waits for someone else, or noone really cares.
Neither. Some people are unfortuantely very busy with many other things as
well. :-P
> Andrew, can you just merge it?
>
> Please apply,
Please wait just a while.
I think we can merge this eventually through the linux-media tree, but
please first see the comments below.
> Pavel
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> new file mode 100644
> index 000..0fc28a9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/adp1653.txt
> @@ -0,0 +1,37 @@
> +* Analog Devices ADP1653 flash LED driver
> +
> +Required Properties:
> +
> + - compatible: Must contain be "adi,adp1653"
> +
> + - reg: I2C slave address
> +
> + - gpios: References to the GPIO that controls the power for the chip.
> +
> +There are two led outputs available - flash and indicator. One led is
> +represented by one child node, nodes need to be named "flash" and
> "indicator".
> +
> +Required properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +
> +Required properties of the flash LED child node:
> +
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
The documentation says that the maximum value is used if these values are
not specified. I think I'd make these optional.
> +
> +Example:
> +
> + adp1653: led-controller@30 {
> + compatible = "adi,adp1653";
> + reg = <0x30>;
> + gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; /* 88 */
> +
> + flash {
> + flash-timeout-us = <50>;
> + flash-max-microamp = <32>;
> + max-microamp = <5>;
> + };
> + indicator {
> + max-microamp = <17500>;
> + };
> + };
> diff --git a/drivers/media/i2c/adp1653.c b/drivers/media/i2c/adp1653.c
> index 873fe19..0341009 100644
> --- a/drivers/media/i2c/adp1653.c
> +++ b/drivers/media/i2c/adp1653.c
> @@ -8,6 +8,7 @@
> * Contributors:
> * Sakari Ailus
> * Tuukka Toivonen
> + * Pavel Machek
> *
> * This program is free software; you can redistribute it and/or
> * modify it under the terms of the GNU General Public License
> @@ -34,6 +35,8 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
> #include
>
> @@ -306,9 +309,17 @@ adp1653_init_device(struct adp1653_flash *flash)
> static int
> __adp1653_set_power(struct adp1653_flash *flash, int on)
> {
> - int ret;
> + int ret = 0;
> +
> + if (flash->platform_data->power) {
> + ret = flash->platform_data->power(&flash->subdev, on);
The power() callback should be dropped. It's controlling a GPIO. But that
can be done later on. The alternative is a patch before this one.
> + } else {
> + gpio_set_value(flash->platform_data->power_gpio, on);
> + if (on)
> + /* Some delay is apparently required. */
> + udelay(20);
> + }
>
> - ret = flash->platform_data->power(&flash->subdev, on);
> if (ret < 0)
> return ret;
>
> @@ -316,8 +327,13 @@ __adp1653_set_power(struct adp1653_flash *flash, int on)
> return 0;
>
> ret = adp1653_init_device(flash);
> - if (ret < 0)
> + if (ret >= 0)
> + return ret;
> +
> + if (flash->platform_data->power)
> flash->platform_data->power(&flash->subdev, 0);
> + else
> + gpio_set_value(flash->platform_data->power_gpio, 0);
>
> return ret;
> }
> @@ -407,21 +423,77 @@ static int adp1653_resume(struct device *dev)
>
> #endif /* CONFIG_PM */
>
> +static int adp1653_of_init(struct i2c_client *client,
> +struct adp1653_flash *flash,
> +struct
Re: [PATCH v2 04/11] DT: Add documentation for the mfd Maxim max77693
Hi Jacek, On Mon, Mar 30, 2015 at 09:36:37AM +0200, Jacek Anaszewski wrote: > Hi Sakari, > > On 03/28/2015 11:55 PM, Sakari Ailus wrote: > >On Fri, Mar 27, 2015 at 02:49:38PM +0100, Jacek Anaszewski wrote: > >>This patch adds device tree binding documentation for > >>the flash cell of the Maxim max77693 multifunctional device. > >> > >>Signed-off-by: Jacek Anaszewski > >>Signed-off-by: Andrzej Hajda > >>Acked-by: Kyungmin Park > >>Acked-by: Sakari Ailus > >>Cc: Lee Jones > >>Cc: Chanwoo Choi > >>Cc: Bryan Wu > >>Cc: Richard Purdie > >>--- > >> Documentation/devicetree/bindings/mfd/max77693.txt | 61 > >> > >> 1 file changed, 61 insertions(+) > >> > >>diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt > >>b/Documentation/devicetree/bindings/mfd/max77693.txt > >>index 38e6440..15c546ea 100644 > >>--- a/Documentation/devicetree/bindings/mfd/max77693.txt > >>+++ b/Documentation/devicetree/bindings/mfd/max77693.txt > >>@@ -76,7 +76,53 @@ Optional properties: > >> Valid values: 430, 470, 480, 490 > >> Default: 430 > >> > >>+- led : the LED submodule device node > >>+ > >>+There are two LED outputs available - FLED1 and FLED2. Each of them can > >>+control a separate LED or they can be connected together to double > >>+the maximum current for a single connected LED. One LED is represented > >>+by one child node. > >>+ > >>+Required properties: > >>+- compatible : Must be "maxim,max77693-led". > >>+ > >>+Optional properties: > >>+- maxim,trigger-type : Flash trigger type. > >>+ Possible trigger types: > >>+ LEDS_TRIG_TYPE_EDGE (0) - Rising edge of the signal triggers > >>+ the flash, > >>+ LEDS_TRIG_TYPE_LEVEL (1) - Strobe pulse length controls duration > >>+ of the flash. > >>+- maxim,boost-mode : > >>+ In boost mode the device can produce up to 1.2A of total current > >>+ on both outputs. The maximum current on each output is reduced > >>+ to 625mA then. If not enabled explicitly, boost setting defaults to > >>+ LEDS_BOOST_FIXED in case both current sources are used. > >>+ Possible values: > >>+ LEDS_BOOST_OFF (0) - no boost, > >>+ LEDS_BOOST_ADAPTIVE (1) - adaptive mode, > >>+ LEDS_BOOST_FIXED (2) - fixed mode. > >>+- maxim,boost-mvout : Output voltage of the boost module in millivolts. > > > >What are the possible values for this? > > maxim,boost-mvout : Output voltage of the boost module in millivolts > Range: 3300 - 5500 Could you please add that? > > Do you think it is necessary to mention also allowed step for all the > values? That's a good question. They probably are more or less visible in the driver code. I think I'd document them here, but I'm fine with not adding them as well. You probably wouldn't be able to meaningfully use the chip without the datasheet anyway. > >Is the datasheet publicly available btw.? > > I have an access only to the non-public one. I googled a bit, couldn't find anything relevant immediately at least. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 09/11] Documentation: leds: Add description of v4l2-flash sub-device
Hi Jacek, On Fri, Mar 27, 2015 at 02:49:43PM +0100, Jacek Anaszewski wrote: > This patch extends LED Flash class documention by > the description of interactions with v4l2-flash sub-device. > > Signed-off-by: Jacek Anaszewski > Acked-by: Kyungmin Park > Acked-by: Sakari Ailus > Cc: Bryan Wu > Cc: Richard Purdie > --- > Documentation/leds/leds-class-flash.txt | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/Documentation/leds/leds-class-flash.txt > b/Documentation/leds/leds-class-flash.txt > index 19bb673..8623413 100644 > --- a/Documentation/leds/leds-class-flash.txt > +++ b/Documentation/leds/leds-class-flash.txt > @@ -20,3 +20,16 @@ Following sysfs attributes are exposed for controlling > flash LED devices: > - max_flash_timeout > - flash_strobe > - flash_fault > + > +A LED subsystem driver can be controlled also from the level of > VideoForLinux2 > +subsystem. In order to enable this CONFIG_V4L2_FLASH_LED_CLASS symbol has to > +be defined in the kernel config. The driver must call the v4l2_flash_init > +function to get registered in the V4L2 subsystem. On remove the > +v4l2_flash_release function has to be called (see ). More information on using the interface functions would be nice, with a pointer to a driver that uses the interface. The simpler, the better I guess. > + > +After proper initialization a V4L2 Flash sub-device is created. The > sub-device > +exposes a number of V4L2 controls, which allow for controlling a LED Flash > class > +device with use of its internal kernel API. How about this: Once the V4L2 sub-device is registered by the driver which created the Media controller device, the sub-device node acts just as a node of a native V4L2 flash API device would. The calls are simply routed to the LED flash API. > +Opening the V4L2 Flash sub-device makes the LED subsystem sysfs interface > +unavailable. The interface is re-enabled after the V4L2 Flash sub-device > +is closed. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 05/11] leds: Add driver for AAT1290 flash LED controller
On Fri, Mar 27, 2015 at 02:49:39PM +0100, Jacek Anaszewski wrote: > This patch adds a driver for the 1.5A Step-Up Current Regulator > for Flash LEDs. The device is programmed through a Skyworks proprietary > AS2Cwire serial digital interface. > > Signed-off-by: Jacek Anaszewski > Acked-by: Kyungmin Park > Cc: Bryan Wu > Cc: Richard Purdie Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/11] DT: Add documentation for the mfd Maxim max77693
On Fri, Mar 27, 2015 at 02:49:38PM +0100, Jacek Anaszewski wrote:
> This patch adds device tree binding documentation for
> the flash cell of the Maxim max77693 multifunctional device.
>
> Signed-off-by: Jacek Anaszewski
> Signed-off-by: Andrzej Hajda
> Acked-by: Kyungmin Park
> Acked-by: Sakari Ailus
> Cc: Lee Jones
> Cc: Chanwoo Choi
> Cc: Bryan Wu
> Cc: Richard Purdie
> ---
> Documentation/devicetree/bindings/mfd/max77693.txt | 61
>
> 1 file changed, 61 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt
> b/Documentation/devicetree/bindings/mfd/max77693.txt
> index 38e6440..15c546ea 100644
> --- a/Documentation/devicetree/bindings/mfd/max77693.txt
> +++ b/Documentation/devicetree/bindings/mfd/max77693.txt
> @@ -76,7 +76,53 @@ Optional properties:
> Valid values: 430, 470, 480, 490
> Default: 430
>
> +- led : the LED submodule device node
> +
> +There are two LED outputs available - FLED1 and FLED2. Each of them can
> +control a separate LED or they can be connected together to double
> +the maximum current for a single connected LED. One LED is represented
> +by one child node.
> +
> +Required properties:
> +- compatible : Must be "maxim,max77693-led".
> +
> +Optional properties:
> +- maxim,trigger-type : Flash trigger type.
> + Possible trigger types:
> + LEDS_TRIG_TYPE_EDGE (0) - Rising edge of the signal triggers
> + the flash,
> + LEDS_TRIG_TYPE_LEVEL (1) - Strobe pulse length controls duration
> + of the flash.
> +- maxim,boost-mode :
> + In boost mode the device can produce up to 1.2A of total current
> + on both outputs. The maximum current on each output is reduced
> + to 625mA then. If not enabled explicitly, boost setting defaults to
> + LEDS_BOOST_FIXED in case both current sources are used.
> + Possible values:
> + LEDS_BOOST_OFF (0) - no boost,
> + LEDS_BOOST_ADAPTIVE (1) - adaptive mode,
> + LEDS_BOOST_FIXED (2) - fixed mode.
> +- maxim,boost-mvout : Output voltage of the boost module in millivolts.
What are the possible values for this?
Is the datasheet publicly available btw.?
> +- maxim,mvsys-min : Low input voltage level in millivolts. Flash is not fired
> + if chip estimates that system voltage could drop below this level due
> + to flash power consumption.
> +
> +Required properties of the LED child node:
> +- led-sources : see Documentation/devicetree/bindings/leds/common.txt;
> + device current output identifiers: 0 - FLED1, 1 - FLED2
> +
> +Optional properties of the LED child node:
> +- label : see Documentation/devicetree/bindings/leds/common.txt
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> + Range: 15625 - 25
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> + Range: 15625 - 100
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> + Range: 62500 - 100
> +
> Example:
> +#include
> +
> max77693@66 {
> compatible = "maxim,max77693";
> reg = <0x66>;
> @@ -117,5 +163,20 @@ Example:
> maxim,thermal-regulation-celsius = <75>;
> maxim,battery-overcurrent-microamp = <300>;
> maxim,charge-input-threshold-microvolt = <430>;
> +
> + led {
> + compatible = "maxim,max77693-led";
> + maxim,trigger-type = ;
> + maxim,boost-mode = ;
> + maxim,boost-mvout = <5000>;
> + maxim,mvsys-min = <2400>;
> +
> + camera_flash: flash-led {
> + label = "max77693-flash";
> + led-sources = <0>, <1>;
> + max-microamp = <50>;
> + flash-max-microamp = <125>;
> + flash-timeout-us = <100>;
> + };
> };
> };
--
Kind regards,
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 03/11] leds: Add support for max77693 mfd flash cell
Hi Jacek, On Fri, Mar 27, 2015 at 02:49:37PM +0100, Jacek Anaszewski wrote: > This patch adds led-flash support to Maxim max77693 chipset. > A device can be exposed to user space through LED subsystem > sysfs interface. Device supports up to two leds which can > work in flash and torch mode. The leds can be triggered > externally or by software. > > Signed-off-by: Jacek Anaszewski > Signed-off-by: Andrzej Hajda > Acked-by: Kyungmin Park > Cc: Bryan Wu > Cc: Richard Purdie > Cc: Lee Jones > Cc: Chanwoo Choi ... > diff --git a/drivers/leds/leds-max77693.c b/drivers/leds/leds-max77693.c > new file mode 100644 > index 000..65c910d > --- /dev/null > +++ b/drivers/leds/leds-max77693.c > @@ -0,0 +1,973 @@ > +/* > + * LED Flash class driver for the flash cell of max77693 mfd. > + * > + * Copyright (C) 2015, Samsung Electronics Co., Ltd. > + * > + * Authors: Jacek Anaszewski > + *Andrzej Hajda > + * > + * 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. > + */ > + > +#include You might not need this anymore. Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 02/11] leds: add uapi header file
Hi Jacek, On Fri, Mar 27, 2015 at 02:49:36PM +0100, Jacek Anaszewski wrote: > This patch adds header file for LED subsystem definitions and > declarations. The initial need for the header is allowing the > user space to discover the semantics of flash fault bits. Where does the user space need these? The fault codes are strings in the sysfs interface. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/11] leds: unify the location of led-trigger API
On Fri, Mar 27, 2015 at 02:49:35PM +0100, Jacek Anaszewski wrote: > Part of led-trigger API was in the private drivers/leds/leds.h header. > Move it to the include/linux/leds.h header to unify the API location > and announce it as public. It has been already exported from > led-triggers.c with EXPORT_SYMBOL_GPL macro. > > Signed-off-by: Jacek Anaszewski > Acked-by: Kyungmin Park > Cc: Bryan Wu > Cc: Richard Purdie Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] DT: leds: Add uniqueness requirement for 'label' property.
Hi Jacek, Jacek Anaszewski wrote: > Label is used for naming LED class devices. Since ePAPR > doesn't require uniqueness for label properties, it has to be > explicitly required in the LEDs common bindings documentation. > > Signed-off-by: Jacek Anaszewski > Acked-by: Kyungmin Park > Cc: Bryan Wu > Cc: Richard Purdie > Cc: Sakari Ailus > Cc: devicetree@vger.kernel.org Thanks for the patch! Acked-by: Sakari Ailus -- Sakari Ailus sakari.ai...@iki.fi -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 3/3] leds: Add ktd2692 flash LED driver
You could unify the above two lines.
> >
>
> Is it better to unify code than separate?
I'd think so.
>
> >> +
> >> + mutex_init(&led->lock);
> >> + INIT_WORK(&led->work_brightness_set, ktd2692_brightness_set_work);
> >> +
> >> + platform_set_drvdata(pdev, led);
> >> +
> >> + ret = led_classdev_flash_register(&pdev->dev, fled_cdev);
> >> + if (ret) {
> >> + dev_err(&pdev->dev, "can't register LED %s\n", led_cdev->name);
> >
> > You should do mutex_destroy() here.
> >
>
> Right. I should do
> Thanks
>
> >> + cancel_work_sync(&led->work_brightness_set);
> >> + return ret;
> >> + }
> >> +
> >
> > This is a LED flash device. Do you intend to add support for the V4L2 flash
> > API as well?
> >
>
> I hope :-)
> maybe I would support extend version later
Another patch later is fine IMO.
>
> >> + ktd2692_setup(led);
> >> + ktd2692_led_regulator_enable(led);
> >
> > Hmm. I guess the regulator was already enabled, assuming you have tested
> > this. :-)
> >
>
> Oh, I'll check.
> Isn't it necessary to use this API if the regulator was enabled?
It is. Because you use the ktd2692 chip before enabling the regulator, the
regulator must have been enabled previously by something else than ktd2692
driver.
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 3/3] leds: Add ktd2692 flash LED driver
_led_flash_strobe_set,
> + .timeout_set = ktd2692_led_flash_timeout_set,
> +};
> +
> +static int ktd2692_probe(struct platform_device *pdev)
> +{
> + struct ktd2692_context *led;
> + struct led_classdev *led_cdev;
> + struct led_classdev_flash *fled_cdev;
> + struct led_flash_setting flash_timeout;
> + u32 flash_timeout_us;
> + int ret;
> +
> + led = devm_kzalloc(&pdev->dev, sizeof(*led), GFP_KERNEL);
> + if (!led)
> + return -ENOMEM;
> +
> + if (!pdev->dev.of_node)
> + return -ENXIO;
> +
> + fled_cdev = &led->fled_cdev;
> + led_cdev = &fled_cdev->led_cdev;
> +
> + ret = ktd2692_parse_dt(led, &pdev->dev, &flash_timeout_us);
> + if (ret)
> + return ret;
> +
> + led->regulator = devm_regulator_get(&pdev->dev, "vin");
> + if (IS_ERR(led->regulator)) {
> + dev_err(&pdev->dev, "regulator get failed\n");
> + return PTR_ERR(led->regulator);
> + }
> +
> + ktd2692_init_flash_timeout(flash_timeout_us, &flash_timeout);
> +
> + fled_cdev->timeout = flash_timeout;
> + fled_cdev->ops = &flash_ops;
> +
> + led_cdev->name = KTD2692_DEFAULT_NAME;
> + led_cdev->brightness_set = ktd2692_led_brightness_set;
> + led_cdev->brightness_set_sync = ktd2692_led_brightness_set_sync;
> + led_cdev->flags |= LED_CORE_SUSPENDRESUME;
> + led_cdev->flags |= LED_DEV_CAP_FLASH;
You could unify the above two lines.
> +
> + mutex_init(&led->lock);
> + INIT_WORK(&led->work_brightness_set, ktd2692_brightness_set_work);
> +
> + platform_set_drvdata(pdev, led);
> +
> + ret = led_classdev_flash_register(&pdev->dev, fled_cdev);
> + if (ret) {
> + dev_err(&pdev->dev, "can't register LED %s\n", led_cdev->name);
You should do mutex_destroy() here.
> + cancel_work_sync(&led->work_brightness_set);
> + return ret;
> + }
> +
This is a LED flash device. Do you intend to add support for the V4L2 flash
API as well?
> + ktd2692_setup(led);
> + ktd2692_led_regulator_enable(led);
Hmm. I guess the regulator was already enabled, assuming you have tested
this. :-)
> +
> + return 0;
> +}
> +
> +static int ktd2692_remove(struct platform_device *pdev)
> +{
> + struct ktd2692_context *led = platform_get_drvdata(pdev);
> +
> + ktd2692_led_regulator_disable(led);
> + led_classdev_flash_unregister(&led->fled_cdev);
> + cancel_work_sync(&led->work_brightness_set);
> +
> + mutex_destroy(&led->lock);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id ktd2692_match[] = {
> + { .compatible = "kinetic,ktd2692", },
> + { /* sentinel */ },
> +};
> +
> +static struct platform_driver ktd2692_driver = {
> + .driver = {
> + .name = "leds-ktd2692",
> + .of_match_table = ktd2692_match,
> + },
> + .probe = ktd2692_probe,
> + .remove = ktd2692_remove,
> +};
> +
> +module_platform_driver(ktd2692_driver);
> +
> +MODULE_AUTHOR("Ingi Kim ");
> +MODULE_DESCRIPTION("Kinetic KTD2692 LED driver");
> +MODULE_LICENSE("GPL v2");
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 09/11] DT: Add documentation for exynos4-is 'flashes' property
Hi Jacek, On Fri, Mar 20, 2015 at 04:03:29PM +0100, Jacek Anaszewski wrote: > This patch adds a description of 'flashes' property > to the samsung-fimc.txt. > > Signed-off-by: Jacek Anaszewski > Acked-by: Kyungmin Park > Cc: Sylwester Nawrocki > --- > .../devicetree/bindings/media/samsung-fimc.txt |8 > 1 file changed, 8 insertions(+) > > diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt > b/Documentation/devicetree/bindings/media/samsung-fimc.txt > index 922d6f8..cb0e263 100644 > --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt > +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt > @@ -40,6 +40,13 @@ should be inactive. For the "active-a" state the camera > port A must be activated > and the port B deactivated and for the state "active-b" it should be the > other > way around. > > +Optional properties: > + > +- flashes - Array of phandles to the flash LEDs that can be controlled by the > + sub-devices contained in this media device. Flash LED is > + represented by a child node of a flash LED device This should be in Documentation/devicetree/bindings/media/video-interfaces.txt. Should flash devices be associated with sensors somehow rather than ISPs? That's how they commonly are arranged, however that doesn't limit placing them in silly places. I'm not necessarily saying the flashes-property should be present in sensor's DT nodes, but it'd be good to be able to make the association if it's there. > + (see Documentation/devicetree/bindings/leds/common.txt). > + > The 'camera' node must include at least one 'fimc' child node. > > > @@ -166,6 +173,7 @@ Example: > clock-output-names = "cam_a_clkout", "cam_b_clkout"; > pinctrl-names = "default"; > pinctrl-0 = <&cam_port_a_clk_active>; > + flashes = <&camera_flash>, <&system_torch>; > status = "okay"; > #address-cells = <1>; > #size-cells = <1>; There will be other kind of devices that have somewhat similar relationship. They just haven't been defined yet. Lens controllers or EEPROM for instance. The two are an integral part of a module, something which is not modelled in DT in any way, but perhaps should be. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 07/11] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek, On Tue, Mar 24, 2015 at 09:35:05AM +0100, Jacek Anaszewski wrote: ... > >>>>diff --git a/drivers/media/v4l2-core/v4l2-flash.c > >>>>b/drivers/media/v4l2-core/v4l2-flash.c > >>>>new file mode 100644 > >>>>index 000..804c2e4 > >>>>--- /dev/null > >>>>+++ b/drivers/media/v4l2-core/v4l2-flash.c > >>>>@@ -0,0 +1,607 @@ > >>>>+/* > >>>>+ * V4L2 Flash LED sub-device registration helpers. > >>>>+ * > >>>>+ * Copyright (C) 2015 Samsung Electronics Co., Ltd > >>>>+ * Author: Jacek Anaszewski > >>>>+ * > >>>>+ * 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. > >>>>+ */ > >>>>+ > >>>>+#include > >>>>+#include > >>>>+#include > >>>>+#include > >>>>+#include > >>>>+#include > >>>>+#include > >>>>+#include "../../leds/leds.h" > >>> > >>>What do you need from leds.h? Shouldn't this be e.g. under include/linux > >>>instead? > > I need led_trigger_remove function. It's exported but defined in what is obviously a private header file to the framework. Could it be moved to include/linux/leds.h instead? -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 02/11] DT: Add documentation for the mfd Maxim max77693
Hi Jacek, On Mon, Mar 23, 2015 at 10:54:11AM +0100, Jacek Anaszewski wrote: > Hi Sakari, > > On 03/21/2015 11:49 PM, Sakari Ailus wrote: > >Hi Jacek, > > > >On Fri, Mar 20, 2015 at 04:03:22PM +0100, Jacek Anaszewski wrote: > >>+Optional properties of the LED child node: > >>+- label : see Documentation/devicetree/bindings/leds/common.txt > > > >I'm still not comfortable using the label field as-is as the entity name in > >the later patches, there's one important problem: it is not guaranteed to be > >unique in the system. > > I don't use it as-is in my patches. For max77603-led the i2c adapter id > and client address is added to it, and for aat1290 there is '_n' suffix > added. Nonetheless I didn't notice that the patch [1] was already > merged. It checks if a LED class device with given name isn't already > registered and adds a '_n" suffix if there was any. If it was exported > I could use it in the leds-aat1290 driver and avoid depending on the > static variable. > > Whereas for I2C devices the problem doesn't exist (it is guaranteed that > no more than one I2C client with an address can be present on the > same bus), for devices driven through GPIOs we haven't stable unique > identifier. > > I thought that we agreed on #v4l about adding numerical postfixes > in case of such devices. > > >Do you think this could be added to > >Documentation/devicetree/bindings/leds/common.txt, with perhaps enforcing it > >in the LED framework? Bryan, what do you think? > > The patch [1] seems to address the issue. Replied to that, you're cc'd. For this patch: Acked-by: Sakari Ailus -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 07/11] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek,
One more comment on this one:
On Fri, Mar 20, 2015 at 04:03:27PM +0100, Jacek Anaszewski wrote:
...
> +struct v4l2_flash *v4l2_flash_init(struct led_classdev_flash *fled_cdev,
> +const struct v4l2_flash_ops *ops,
> +struct v4l2_flash_ctrl_config *config)
> +{
> + struct v4l2_flash *v4l2_flash;
> + struct led_classdev *led_cdev = &fled_cdev->led_cdev;
> + struct v4l2_subdev *sd;
> + int ret;
> +
> + if (!fled_cdev || !ops || !config)
> + return ERR_PTR(-EINVAL);
> +
> + v4l2_flash = devm_kzalloc(led_cdev->dev, sizeof(*v4l2_flash),
> + GFP_KERNEL);
> + if (!v4l2_flash)
> + return ERR_PTR(-ENOMEM);
> +
> + sd = &v4l2_flash->sd;
> + v4l2_flash->fled_cdev = fled_cdev;
> + v4l2_flash->ops = ops;
> + sd->dev = led_cdev->dev;
> + v4l2_subdev_init(sd, &v4l2_flash_subdev_ops);
> + sd->internal_ops = &v4l2_flash_subdev_internal_ops;
> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> + strlcpy(sd->name, config->dev_name, sizeof(sd->name));
> +
> + ret = media_entity_init(&sd->entity, 0, NULL, 0);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_FLASH;
> +
> + ret = v4l2_flash_init_controls(v4l2_flash, config);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + of_node_get(led_cdev->dev->of_node);
> +
> + ret = v4l2_async_register_subdev(sd);
> + if (ret < 0)
> + goto err_async_register_sd;
> +
> + return v4l2_flash;
> +
> +err_async_register_sd:
> + of_node_put(led_cdev->dev->of_node);
> + v4l2_ctrl_handler_free(sd->ctrl_handler);
> + media_entity_cleanup(&sd->entity);
> +
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_flash_init);
> +
> +void v4l2_flash_release(struct v4l2_flash *v4l2_flash)
> +{
> + struct v4l2_subdev *sd = &v4l2_flash->sd;
> + struct led_classdev *led_cdev = &v4l2_flash->fled_cdev->led_cdev;
> +
> + v4l2_async_unregister_subdev(sd);
> + of_node_put(led_cdev->dev->of_node);
> + v4l2_ctrl_handler_free(sd->ctrl_handler);
> + media_entity_cleanup(&sd->entity);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_flash_release);
It'd be very nice if v4l2_flash_release() could graciously behave with NULL
or negative error code as an argument, such as those produced by
v4l2_flash_init(). This makes error handling a lot easier in drivers.
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 06/11] exynos4-is: Add support for v4l2-flash subdevs
Hi Jacek,
On Mon, Mar 23, 2015 at 04:32:12PM +0100, Jacek Anaszewski wrote:
> Hi Sakari,
>
> Thanks for the review.
>
> On 03/22/2015 02:21 AM, Sakari Ailus wrote:
> >Hi Jacek,
> >
> >Some comments below. Please also get an ack from Sylwester! :-)
>
> No doubt about that :)
>
> >On Fri, Mar 20, 2015 at 04:03:26PM +0100, Jacek Anaszewski wrote:
> >>This patch adds support for external v4l2-flash devices.
> >>The support includes parsing "flashes" DT property
> >>and asynchronous subdevice registration.
> >>
> >>Signed-off-by: Jacek Anaszewski
> >>Acked-by: Kyungmin Park
> >>Cc: Sylwester Nawrocki
> >>---
> >> drivers/media/platform/exynos4-is/media-dev.c | 36
> >> +++--
> >> drivers/media/platform/exynos4-is/media-dev.h | 13 -
> >> 2 files changed, 46 insertions(+), 3 deletions(-)
> >>
> >>diff --git a/drivers/media/platform/exynos4-is/media-dev.c
> >>b/drivers/media/platform/exynos4-is/media-dev.c
> >>index f315ef9..8dd0e5d 100644
> >>--- a/drivers/media/platform/exynos4-is/media-dev.c
> >>+++ b/drivers/media/platform/exynos4-is/media-dev.c
> >>@@ -451,6 +451,25 @@ rpm_put:
> >>return ret;
> >> }
> >>
> >>+static void fimc_md_register_flash_entities(struct fimc_md *fmd)
> >>+{
> >>+ struct device_node *parent = fmd->pdev->dev.of_node;
> >>+ struct device_node *np;
> >>+ int i = 0;
> >>+
> >>+ do {
> >>+ np = of_parse_phandle(parent, "flashes", i);
> >>+ if (np) {
> >
> >if (!np)
> > break;
> >
> >And you can remove checking np another time in the loop condition.
>
> Thanks, this will be cleaner indeed.
>
> >>+ fmd->flash[fmd->num_flashes].asd.match_type =
> >>+ V4L2_ASYNC_MATCH_OF;
> >>+ fmd->flash[fmd->num_flashes].asd.match.of.node = np;
> >>+ fmd->num_flashes++;
> >>+ fmd->async_subdevs[fmd->num_sensors + i] =
> >>+ &fmd->flash[i].asd;
> >
> >Have all the sensors been already registered by this point?
>
> Function fimc_md_register_sensor_entities is called before
> this one.
Ok. Then it's fine. I just thing this would be much cleaner if there was no
assumption that fmd->num_flashes is necessarily zero (and all sensors have
been registered).
> >>+ }
> >>+ } while (np && (++i < FIMC_MAX_FLASHES));
> >
> >How about instead:
> >
> >fmd->num_flashes < FIMC_MAX_FLASHES
> >
> >And drop i. Also move incrementing num_flashes as last in the if.
>
> Dropping i will enforce referring to fmd->num_flashes 7 times
> in this short fragment of code.
> Maybe it would be better to use a pointer to it?
> int *nf = &fmd=>num_flashes ?
You could also do
const int nf = fmd->num_flashes;
in the beginning of the loop.
Up to you. Either is IMO better than an unrelated counter variable i. :-)
> >>+}
> >>+
> >> static int __of_get_csis_id(struct device_node *np)
> >> {
> >>u32 reg = 0;
> >>@@ -1275,6 +1294,15 @@ static int subdev_notifier_bound(struct
> >>v4l2_async_notifier *notifier,
> >>struct fimc_sensor_info *si = NULL;
> >>int i;
> >>
> >>+ /* Register flash subdev if detected any */
> >>+ for (i = 0; i < ARRAY_SIZE(fmd->flash); i++) {
> >>+ if (fmd->flash[i].asd.match.of.node == subdev->dev->of_node) {
> >>+ fmd->flash[i].subdev = subdev;
> >>+ fmd->num_flashes++;
> >>+ return 0;
> >>+ }
> >>+ }
> >>+
> >>/* Find platform data for this sensor subdev */
> >>for (i = 0; i < ARRAY_SIZE(fmd->sensor); i++)
> >>if (fmd->sensor[i].asd.match.of.node == subdev->dev->of_node)
> >>@@ -1385,6 +1413,8 @@ static int fimc_md_probe(struct platform_device *pdev)
> >>goto err_m_ent;
> >>}
> >>
> >>+ fimc_md_register_flash_entities(fmd);
> >>+
> >>mutex_unlock(&fmd->media_dev.graph_mutex);
> >>
> >>ret = device_create_file(&pdev->dev, &dev_attr_subdev_conf_mode);
> >>@@ -1401,12 +1431,14 @@ static int fimc
Re: [PATCH v1 07/11] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek, On Mon, Mar 23, 2015 at 04:08:10PM +0100, Jacek Anaszewski wrote: > Hi Sakari, > > Thanks for the review. > > On 03/22/2015 01:22 AM, Sakari Ailus wrote: > >Hi Jacek, > > > >Thanks for the updated set. Some comments below. > > > >On Fri, Mar 20, 2015 at 04:03:27PM +0100, Jacek Anaszewski wrote: > >>This patch adds helper functions for registering/unregistering > >>LED Flash class devices as V4L2 sub-devices. The functions should > >>be called from the LED subsystem device driver. In case the > >>support for V4L2 Flash sub-devices is disabled in the kernel > >>config the functions' empty versions will be used. > >> > >>Signed-off-by: Jacek Anaszewski > >>Acked-by: Kyungmin Park > >>Cc: Sakari Ailus > >>Cc: Hans Verkuil > >>--- > >> drivers/media/v4l2-core/Kconfig | 12 + > >> drivers/media/v4l2-core/Makefile |2 + > >> drivers/media/v4l2-core/v4l2-flash.c | 607 > >> ++ > >> include/media/v4l2-flash.h | 145 > >> 4 files changed, 766 insertions(+) > >> create mode 100644 drivers/media/v4l2-core/v4l2-flash.c > >> create mode 100644 include/media/v4l2-flash.h > >> > >>diff --git a/drivers/media/v4l2-core/Kconfig > >>b/drivers/media/v4l2-core/Kconfig > >>index ba7e21a..0659598 100644 > >>--- a/drivers/media/v4l2-core/Kconfig > >>+++ b/drivers/media/v4l2-core/Kconfig > >>@@ -44,6 +44,18 @@ config V4L2_MEM2MEM_DEV > >> tristate > >> depends on VIDEOBUF2_CORE > >> > >>+# Used by LED subsystem flash drivers > >>+config V4L2_FLASH_LED_CLASS > >>+ tristate "Enable support for Flash sub-devices" > >>+ depends on VIDEO_V4L2_SUBDEV_API > >>+ depends on LEDS_CLASS_FLASH > >>+ depends on OF > > > >I think you can drop OF dependency. A no-op implementation will be used if > >it's disabled. > > The sub-devices are matched by a sub-LED's device_node. This is the only > way of matching the v4l2-flash sub-devs. Without OF a v4l2-flash > sub-device will not get registered at all. Not all systems use OF, and for those there are different async sub-device matching options such as i2c or device name. A sub-device driver should not mandate using a particular one. > What do you mean by "no-op implementation"? There are variants of most of_*() functions that do nothing. These variants are used if CONFIG_OF is disabled, so the drivers won't need lots of ugly #ifdefs. > >>+ ---help--- > >>+ Say Y here to enable support for Flash sub-devices, which allow > >>+ to control LED class devices with use of V4L2 Flash controls. > >>+ > >>+ When in doubt, say N. > >>+ > >> # Used by drivers that need Videobuf modules > >> config VIDEOBUF_GEN > >>tristate > >>diff --git a/drivers/media/v4l2-core/Makefile > >>b/drivers/media/v4l2-core/Makefile > >>index 63d29f2..44e858c 100644 > >>--- a/drivers/media/v4l2-core/Makefile > >>+++ b/drivers/media/v4l2-core/Makefile > >>@@ -22,6 +22,8 @@ obj-$(CONFIG_VIDEO_TUNER) += tuner.o > >> > >> obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o > >> > >>+obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash.o > >>+ > >> obj-$(CONFIG_VIDEOBUF_GEN) += videobuf-core.o > >> obj-$(CONFIG_VIDEOBUF_DMA_SG) += videobuf-dma-sg.o > >> obj-$(CONFIG_VIDEOBUF_DMA_CONTIG) += videobuf-dma-contig.o > >>diff --git a/drivers/media/v4l2-core/v4l2-flash.c > >>b/drivers/media/v4l2-core/v4l2-flash.c > >>new file mode 100644 > >>index 000..804c2e4 > >>--- /dev/null > >>+++ b/drivers/media/v4l2-core/v4l2-flash.c > >>@@ -0,0 +1,607 @@ > >>+/* > >>+ * V4L2 Flash LED sub-device registration helpers. > >>+ * > >>+ * Copyright (C) 2015 Samsung Electronics Co., Ltd > >>+ * Author: Jacek Anaszewski > >>+ * > >>+ * 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. > >>+ */ > >>+ > >>+#include > >>+#include > >>+#include > >>+#include > >>+#include > >>+#include > >>+#include > >>+#include "../../leds/leds.h" > > > >What do you need from leds.h? Shouldn't this be e.g.
Re: [PATCH v1 06/11] exynos4-is: Add support for v4l2-flash subdevs
ensor_info {
> struct fimc_dev *host;
> };
>
> +struct fimc_flash_info {
> + struct v4l2_subdev *subdev;
> + struct v4l2_async_subdev asd;
> +};
> +
> struct cam_clk {
> struct clk_hw hw;
> struct fimc_md *fmd;
> @@ -104,6 +111,8 @@ struct cam_clk {
> * @csis: MIPI CSIS subdevs data
> * @sensor: array of registered sensor subdevs
> * @num_sensors: actual number of registered sensors
> + * @flash: array of registered flash subdevs
> + * @num_flashes: actual number of registered flashes
> * @camclk: external sensor clock information
> * @fimc: array of registered fimc devices
> * @fimc_is: fimc-is data structure
> @@ -123,6 +132,8 @@ struct fimc_md {
> struct fimc_csis_info csis[CSIS_MAX_ENTITIES];
> struct fimc_sensor_info sensor[FIMC_MAX_SENSORS];
> int num_sensors;
> + struct fimc_flash_info flash[FIMC_MAX_FLASHES];
> + int num_flashes;
> struct fimc_camclk_info camclk[FIMC_MAX_CAMCLKS];
> struct clk *wbclk[FIMC_MAX_WBCLKS];
> struct fimc_lite *fimc_lite[FIMC_LITE_MAX_DEVS];
> @@ -149,7 +160,7 @@ struct fimc_md {
> } clk_provider;
>
> struct v4l2_async_notifier subdev_notifier;
> - struct v4l2_async_subdev *async_subdevs[FIMC_MAX_SENSORS];
> + struct v4l2_async_subdev *async_subdevs[FIMC_MAX_ASYNC_SUBDEVS];
>
> bool user_subdev_api;
> spinlock_t slock;
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 10/11] leds: max77693: add support for V4L2 Flash sub-device
Hi Jacek,
On Fri, Mar 20, 2015 at 04:03:30PM +0100, Jacek Anaszewski wrote:
> Add support for V4L2 Flash sub-device to the max77693 LED Flash class
> driver. The support allows for V4L2 Flash sub-device to take the control
> of the LED Flash class device.
>
> Signed-off-by: Jacek Anaszewski
> Acked-by: Kyungmin Park
> Cc: Bryan Wu
> Cc: Richard Purdie
> Cc: Sakari Ailus
> ---
> drivers/leds/leds-max77693.c | 149
> +++---
> 1 file changed, 141 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/leds/leds-max77693.c b/drivers/leds/leds-max77693.c
> index 7386d69..a12bd8c 100644
> --- a/drivers/leds/leds-max77693.c
> +++ b/drivers/leds/leds-max77693.c
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
>
> #define MODE_OFF 0
> #define MODE_FLASH(a)(1 << (a))
> @@ -68,6 +69,8 @@ struct max77693_sub_led {
> struct led_classdev_flash fled_cdev;
> /* assures led-triggers compatibility */
> struct work_struct work_brightness_set;
> + /* V4L2 Flash device */
> + struct v4l2_flash *v4l2_flash;
>
> /* brightness cache */
> unsigned int torch_brightness;
> @@ -651,7 +654,8 @@ static int max77693_led_flash_timeout_set(
> }
>
> static int max77693_led_parse_dt(struct max77693_led_device *led,
> - struct max77693_led_config_data *cfg)
> + struct max77693_led_config_data *cfg,
> + struct device_node **sub_nodes)
> {
> struct device *dev = &led->pdev->dev;
> struct max77693_sub_led *sub_leds = led->sub_leds;
> @@ -697,6 +701,13 @@ static int max77693_led_parse_dt(struct
> max77693_led_device *led,
> return -EINVAL;
> }
>
> + if (sub_nodes[fled_id]) {
> + dev_err(dev,
> + "Conflicting \"led-sources\" DT properties\n");
> + return -EINVAL;
> + }
> +
> + sub_nodes[fled_id] = child_node;
> sub_leds[fled_id].fled_id = fled_id;
>
> of_property_read_string(child_node, "label",
> @@ -784,11 +795,12 @@ static void max77693_led_validate_configuration(struct
> max77693_led_device *led,
> }
>
> static int max77693_led_get_configuration(struct max77693_led_device *led,
> - struct max77693_led_config_data *cfg)
> + struct max77693_led_config_data *cfg,
> + struct device_node **sub_nodes)
> {
> int ret;
>
> - ret = max77693_led_parse_dt(led, cfg);
> + ret = max77693_led_parse_dt(led, cfg, sub_nodes);
> if (ret < 0)
> return ret;
>
> @@ -855,9 +867,86 @@ static const char *max77693_get_led_name(struct
> max77693_led_device *led,
>MAX77693_LED2_NAME;
> }
>
> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
> +
> +static int max77693_led_external_strobe_set(
> + struct v4l2_flash *v4l2_flash,
> + bool enable)
> +{
> + struct max77693_sub_led *sub_led =
> + flcdev_to_sub_led(v4l2_flash->fled_cdev);
> + struct max77693_led_device *led = sub_led_to_led(sub_led);
> + int fled_id = sub_led->fled_id;
> + int ret;
> +
> + mutex_lock(&led->lock);
> +
> + if (enable)
> + ret = max77693_add_mode(led, MODE_FLASH_EXTERNAL(fled_id));
> + else
> + ret = max77693_clear_mode(led, MODE_FLASH_EXTERNAL(fled_id));
> +
> + mutex_unlock(&led->lock);
> +
> + return ret;
> +}
> +
> +static void max77693_init_v4l2_ctrl_config(struct max77693_led_device *led,
> + int fled_id,
> + struct max77693_led_settings *s,
> + struct v4l2_flash_ctrl_config *config)
> +{
> + struct device *dev = &led->pdev->dev;
> + struct max77693_dev *iodev = dev_get_drvdata(dev->parent);
> + struct i2c_client *i2c = iodev->i2c;
> + struct led_flash_setting *setting;
> + struct v4l2_ctrl_config *c;
> +
> + snprintf(config->dev_name, sizeof(config->dev_name),
> + "%s %d-%04x", max77693_get_led_name(led, fled_id),
> + i2c_adapter_id(i2c->adapter), i2c->addr);
Looks good!
> +
> + c = &config->intensity;
> + setting = &s->
Re: [PATCH v1 07/11] media: Add registration helpers for V4L2 flash sub-devices
Hi Jacek,
Thanks for the updated set. Some comments below.
On Fri, Mar 20, 2015 at 04:03:27PM +0100, Jacek Anaszewski wrote:
> This patch adds helper functions for registering/unregistering
> LED Flash class devices as V4L2 sub-devices. The functions should
> be called from the LED subsystem device driver. In case the
> support for V4L2 Flash sub-devices is disabled in the kernel
> config the functions' empty versions will be used.
>
> Signed-off-by: Jacek Anaszewski
> Acked-by: Kyungmin Park
> Cc: Sakari Ailus
> Cc: Hans Verkuil
> ---
> drivers/media/v4l2-core/Kconfig | 12 +
> drivers/media/v4l2-core/Makefile |2 +
> drivers/media/v4l2-core/v4l2-flash.c | 607
> ++
> include/media/v4l2-flash.h | 145
> 4 files changed, 766 insertions(+)
> create mode 100644 drivers/media/v4l2-core/v4l2-flash.c
> create mode 100644 include/media/v4l2-flash.h
>
> diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
> index ba7e21a..0659598 100644
> --- a/drivers/media/v4l2-core/Kconfig
> +++ b/drivers/media/v4l2-core/Kconfig
> @@ -44,6 +44,18 @@ config V4L2_MEM2MEM_DEV
> tristate
> depends on VIDEOBUF2_CORE
>
> +# Used by LED subsystem flash drivers
> +config V4L2_FLASH_LED_CLASS
> + tristate "Enable support for Flash sub-devices"
> + depends on VIDEO_V4L2_SUBDEV_API
> + depends on LEDS_CLASS_FLASH
> + depends on OF
I think you can drop OF dependency. A no-op implementation will be used if
it's disabled.
> + ---help---
> + Say Y here to enable support for Flash sub-devices, which allow
> + to control LED class devices with use of V4L2 Flash controls.
> +
> + When in doubt, say N.
> +
> # Used by drivers that need Videobuf modules
> config VIDEOBUF_GEN
> tristate
> diff --git a/drivers/media/v4l2-core/Makefile
> b/drivers/media/v4l2-core/Makefile
> index 63d29f2..44e858c 100644
> --- a/drivers/media/v4l2-core/Makefile
> +++ b/drivers/media/v4l2-core/Makefile
> @@ -22,6 +22,8 @@ obj-$(CONFIG_VIDEO_TUNER) += tuner.o
>
> obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
>
> +obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash.o
> +
> obj-$(CONFIG_VIDEOBUF_GEN) += videobuf-core.o
> obj-$(CONFIG_VIDEOBUF_DMA_SG) += videobuf-dma-sg.o
> obj-$(CONFIG_VIDEOBUF_DMA_CONTIG) += videobuf-dma-contig.o
> diff --git a/drivers/media/v4l2-core/v4l2-flash.c
> b/drivers/media/v4l2-core/v4l2-flash.c
> new file mode 100644
> index 000..804c2e4
> --- /dev/null
> +++ b/drivers/media/v4l2-core/v4l2-flash.c
> @@ -0,0 +1,607 @@
> +/*
> + * V4L2 Flash LED sub-device registration helpers.
> + *
> + * Copyright (C) 2015 Samsung Electronics Co., Ltd
> + * Author: Jacek Anaszewski
> + *
> + * 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.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "../../leds/leds.h"
What do you need from leds.h? Shouldn't this be e.g. under include/linux
instead?
> +#define has_flash_op(v4l2_flash, op) \
> + (v4l2_flash && v4l2_flash->ops->op)
> +
> +#define call_flash_op(v4l2_flash, op, arg) \
> + (has_flash_op(v4l2_flash, op) ? \
> + v4l2_flash->ops->op(v4l2_flash, arg) : \
> + -EINVAL)
> +
> +static enum led_brightness __intensity_to_led_brightness(
> + struct v4l2_ctrl *ctrl,
> + s32 intensity)
> +{
> + s64 intensity64 = intensity - ctrl->minimum;
> +
> + do_div(intensity64, ctrl->step);
> +
> + /*
> + * Indicator LEDs, unlike torch LEDs, are turned on/off basing on
> + * the state of V4L2_CID_FLASH_INDICATOR_INTENSITY control only.
> + * Therefore it must be possible to set it to 0 level which in
> + * the LED subsystem reflects LED_OFF state.
> + */
> + if (ctrl->id != V4L2_CID_FLASH_INDICATOR_INTENSITY)
> + ++intensity64;
> +
> + return intensity64;
> +}
> +
> +static s32 __led_brightness_to_intensity(struct v4l2_ctrl *ctrl,
> + enum led_brightness brightness)
> +{
> + /*
> + * Indicator LEDs, unlike torch LEDs, are turned on/off basing on
> + * the state of V4L2_CID_FLASH_INDICATOR_INTENSITY control only.
> + * Do not dec
Re: [PATCH v1 03/11] leds: Add driver for AAT1290 current regulator
Hi Jacek,
On Fri, Mar 20, 2015 at 04:03:23PM +0100, Jacek Anaszewski wrote:
...
> +static int aat1290_led_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct aat1290_led *led;
> + struct led_classdev *led_cdev;
> + struct led_classdev_flash *fled_cdev;
> + struct aat1290_led_settings settings;
> + int ret;
> +
> + led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
> + if (!led)
> + return -ENOMEM;
> +
> + led->pdev = pdev;
> + platform_set_drvdata(pdev, led);
> +
> + fled_cdev = &led->fled_cdev;
> + led_cdev = &fled_cdev->led_cdev;
> +
> + ret = aat1290_led_parse_dt(led);
> + if (ret < 0)
> + return ret;
> +
> + if (!led_cdev->name)
> + led_cdev->name = AAT1290_NAME;
> +
> + /* Init flash settings */
> + aat1290_init_flash_settings(led, &settings);
> +
> + fled_cdev->timeout = settings.flash_timeout;
> + fled_cdev->ops = &flash_ops;
> +
> + /* Init LED class */
> + led_cdev->brightness_set = aat1290_led_brightness_set;
> + led_cdev->brightness_set_sync = aat1290_led_brightness_set_sync;
> + led_cdev->max_brightness = AAT1290_MM_CURRENT_SCALE_SIZE;
> + led_cdev->flags |= LED_DEV_CAP_FLASH;
> +
> + INIT_WORK(&led->work_brightness_set, aat1290_brightness_set_work);
> +
> + /* Register in the LED subsystem. */
> + ret = led_classdev_flash_register(&pdev->dev, fled_cdev);
> + if (ret < 0)
> + return ret;
> +
> + mutex_init(&led->lock);
I think you must initialise the mutex before led_classdev_flash_register(),
as this exposes the device to the user. Remember mutex_destroy() in error
handling.
> + return 0;
> +}
> +
> +static int aat1290_led_remove(struct platform_device *pdev)
> +{
> + struct aat1290_led *led = platform_get_drvdata(pdev);
> +
> + led_classdev_flash_unregister(&led->fled_cdev);
> + cancel_work_sync(&led->work_brightness_set);
> +
> + mutex_destroy(&led->lock);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id aat1290_led_dt_match[] = {
> + {.compatible = "skyworks,aat1290"},
{ .compatible ... 1290" },
With the two issues fixed,
Acked-by: Sakari Ailus
> + {},
> +};
> +
> +static struct platform_driver aat1290_led_driver = {
> + .probe = aat1290_led_probe,
> + .remove = aat1290_led_remove,
> + .driver = {
> + .name = "aat1290",
> + .owner = THIS_MODULE,
> + .of_match_table = aat1290_led_dt_match,
> + },
> +};
> +
> +module_platform_driver(aat1290_led_driver);
> +
> +MODULE_AUTHOR("Jacek Anaszewski ");
> +MODULE_DESCRIPTION("Skyworks Current Regulator for Flash LEDs");
> +MODULE_LICENSE("GPL v2");
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 02/11] DT: Add documentation for the mfd Maxim max77693
Hi Jacek, On Fri, Mar 20, 2015 at 04:03:22PM +0100, Jacek Anaszewski wrote: > +Optional properties of the LED child node: > +- label : see Documentation/devicetree/bindings/leds/common.txt I'm still not comfortable using the label field as-is as the entity name in the later patches, there's one important problem: it is not guaranteed to be unique in the system. Do you think this could be added to Documentation/devicetree/bindings/leds/common.txt, with perhaps enforcing it in the LED framework? Bryan, what do you think? The alternative would be to simply ignore it in the entity name, but then the name of the device would be different in the LED framework and Media controller. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v1 01/11] leds: Add support for max77693 mfd flash cell
Hi Jacek, On Fri, Mar 20, 2015 at 04:03:21PM +0100, Jacek Anaszewski wrote: > This patch adds led-flash support to Maxim max77693 chipset. > A device can be exposed to user space through LED subsystem > sysfs interface. Device supports up to two leds which can > work in flash and torch mode. The leds can be triggered > externally or by software. > > Signed-off-by: Jacek Anaszewski > Signed-off-by: Andrzej Hajda > Acked-by: Kyungmin Park > Cc: Bryan Wu > Cc: Richard Purdie > Cc: Lee Jones > Cc: Chanwoo Choi Thanks for the update once again! Acked-by: Sakari Ailus -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7] media: i2c: add support for omnivision's ov2659 sensor
Hi Prabhakar,
Lad Prabhakar wrote:
...
+static int ov2659_g_volatile_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct ov2659 *ov2659 =
+ container_of(ctrl->handler, struct ov2659, ctrls);
+ struct v4l2_mbus_framefmt *fmt = &ov2659->format;
+
+ switch (ctrl->id) {
+ case V4L2_CID_PIXEL_RATE:
+ if (fmt->code != MEDIA_BUS_FMT_SBGGR8_1X8)
+ ov2659->link_frequency->val =
+ ov2659->pdata->link_frequency / 2;
+ else
+ ov2659->link_frequency->val =
+ ov2659->pdata->link_frequency;
You should simply use v4l2_ctrl_s_ctrl_int64() in ..._set_fmt() as this
isn't really a proper volatile control, but its value depends on the format.
--
Sakari Ailus
sakari.ai...@linux.intel.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] v4l: mt9v032: Add OF support
Hi Laurent,
On Wed, Mar 18, 2015 at 03:32:28PM +0200, Laurent Pinchart wrote:
...
> > > @@ -876,10 +879,59 @@ static const struct regmap_config
> > > mt9v032_regmap_config = {>
> > > * Driver initialization and probing
> > > */
> > >
> > > +static struct mt9v032_platform_data *
> > > +mt9v032_get_pdata(struct i2c_client *client)
> > > +{
> > > + struct mt9v032_platform_data *pdata;
> > > + struct v4l2_of_endpoint endpoint;
> > > + struct device_node *np;
> > > + struct property *prop;
> > > +
> > > + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
> > > + return client->dev.platform_data;
> > > +
> > > + np = of_graph_get_next_endpoint(client->dev.of_node, NULL);
> > > + if (!np)
> > > + return NULL;
> > > +
> > > + if (v4l2_of_parse_endpoint(np, &endpoint) < 0)
> > > + goto done;
> > > +
> > > + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> > > + if (!pdata)
> > > + goto done;
> > > +
> > > + prop = of_find_property(np, "link-frequencies", NULL);
> > > + if (prop) {
> > > + size_t size = prop->length / 8;
> > > + u64 *link_freqs;
> > > +
> > > + link_freqs = devm_kzalloc(&client->dev,
> > > + size * sizeof(*link_freqs),
> >
> > You could simply use prop->length here. I think that'd look nicer.
>
> How about devm_kcalloc(&client->dev, size, sizeof(*link_freqs)) as this is
> allocating an array ?
That's certainly fine as well, I think. Feel free to divide prop->length by
sizeof(*link_freqs) instead of plain 8.
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v13 10/13] Documentation: leds: Add description of v4l2-flash sub-device
Hi Jacek, On Thu, Mar 12, 2015 at 04:45:11PM +0100, Jacek Anaszewski wrote: > This patch extends LED Flash class documention by > the description of interactions with v4l2-flash sub-device. > > Signed-off-by: Jacek Anaszewski > Acked-by: Kyungmin Park > Acked-by: Sakari Ailus > Cc: Bryan Wu > Cc: Richard Purdie > --- > Documentation/leds/leds-class-flash.txt | 13 + > 1 file changed, 13 insertions(+) > > diff --git a/Documentation/leds/leds-class-flash.txt > b/Documentation/leds/leds-class-flash.txt > index 19bb673..8623413 100644 > --- a/Documentation/leds/leds-class-flash.txt > +++ b/Documentation/leds/leds-class-flash.txt > @@ -20,3 +20,16 @@ Following sysfs attributes are exposed for controlling > flash LED devices: > - max_flash_timeout > - flash_strobe > - flash_fault > + > +A LED subsystem driver can be controlled also from the level of > VideoForLinux2 > +subsystem. In order to enable this CONFIG_V4L2_FLASH_LED_CLASS symbol has to > +be defined in the kernel config. The driver must call the v4l2_flash_init > +function to get registered in the V4L2 subsystem. On remove the > +v4l2_flash_release function has to be called (see ). > + > +After proper initialization a V4L2 Flash sub-device is created. The > sub-device > +exposes a number of V4L2 controls, which allow for controlling a LED Flash > class Over 80 characters per line. With this fixed, Acked-by: Sakari Ailus > +device with use of its internal kernel API. > +Opening the V4L2 Flash sub-device makes the LED subsystem sysfs interface > +unavailable. The interface is re-enabled after the V4L2 Flash sub-device > +is closed. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 10/18] omap3isp: Move the syscon register out of the ISP register maps
On Mon, Mar 16, 2015 at 02:19:04AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Saturday 07 March 2015 23:41:07 Sakari Ailus wrote: > > The syscon register isn't part of the ISP, use it through the syscom driver > > regmap instead. The syscom block is considered to be from 343x on ISP > > revision 2.0 whereas 15.0 is assumed to have 3630 syscon. > > > > Signed-off-by: Sakari Ailus > > --- > > arch/arm/boot/dts/omap3.dtsi|2 +- > > arch/arm/mach-omap2/devices.c | 10 -- > > drivers/media/platform/omap3isp/isp.c | 19 +++ > > drivers/media/platform/omap3isp/isp.h | 19 +-- > > drivers/media/platform/omap3isp/ispcsiphy.c | 20 +--- > > I've noticed another issue, you need a "select MFD_SYSCON" in Kconfig. Thanks, fixed! -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6] media: i2c: add support for omnivision's ov2659 sensor
Hi Prabhakar,
Lad Prabhakar wrote:
...
> +static const struct ov2659_pixfmt ov2659_formats[] = {
> + {
> + .code = MEDIA_BUS_FMT_YUYV8_2X8,
> + .colorspace = V4L2_COLORSPACE_JPEG,
> + .format_ctrl_regs = ov2659_format_yuyv,
> + },
> + {
> + .code = MEDIA_BUS_FMT_UYVY8_2X8,
> + .colorspace = V4L2_COLORSPACE_JPEG,
> + .format_ctrl_regs = ov2659_format_uyvy,
> + },
> + {
> + .code = MEDIA_BUS_FMT_RGB565_2X8_BE,
> + .colorspace = V4L2_COLORSPACE_JPEG,
> + .format_ctrl_regs = ov2659_format_rgb565,
> + },
> + {
> + .code = MEDIA_BUS_FMT_SBGGR8_1X8,
> + .colorspace = V4L2_COLORSPACE_SMPTE170M,
> + .format_ctrl_regs = ov2659_format_bggr,
> + },
> +};
...
> +static int ov2659_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + const struct ov2659_platform_data *pdata = ov2659_get_pdata(client);
> + struct v4l2_subdev *sd;
> + struct ov2659 *ov2659;
> + struct clk *clk;
> + int ret;
> +
> + if (!pdata) {
> + dev_err(&client->dev, "platform data not specified\n");
> + return -EINVAL;
> + }
> +
> + ov2659 = devm_kzalloc(&client->dev, sizeof(*ov2659), GFP_KERNEL);
> + if (!ov2659)
> + return -ENOMEM;
> +
> + ov2659->pdata = pdata;
> + ov2659->client = client;
> +
> + clk = devm_clk_get(&client->dev, "xvclk");
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + ov2659->xvclk_frequency = clk_get_rate(clk);
> + if (ov2659->xvclk_frequency < 600 ||
> + ov2659->xvclk_frequency > 2700)
> + return -EINVAL;
> +
> + v4l2_ctrl_handler_init(&ov2659->ctrls, 2);
> + v4l2_ctrl_new_std(&ov2659->ctrls, &ov2659_ctrl_ops,
> + V4L2_CID_PIXEL_RATE, pdata->link_frequency,
> + pdata->link_frequency, 1, pdata->link_frequency);
Are the formats that you advertise correct? If so, you should divide the
link frequency by two to get pixel rate on formats that have two samples
per clock cycle, i.e. use v4l2_ctrl_s_ctrl_int64() /
__v4l2_ctrl_s_ctrl_int64().
--
Regards,
Sakari Ailus
sakari.ai...@linux.intel.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/2] v4l: mt9v032: Add OF support
t *client,
> const struct i2c_device_id *did)
> {
> - struct mt9v032_platform_data *pdata = client->dev.platform_data;
> + struct mt9v032_platform_data *pdata = mt9v032_get_pdata(client);
> struct mt9v032 *mt9v032;
> unsigned int i;
> int ret;
> @@ -1037,9 +1089,25 @@ static const struct i2c_device_id mt9v032_id[] = {
> };
> MODULE_DEVICE_TABLE(i2c, mt9v032_id);
>
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id mt9v032_of_match[] = {
> + { .compatible = "aptina,mt9v022" },
> + { .compatible = "aptina,mt9v022m" },
> + { .compatible = "aptina,mt9v024" },
> + { .compatible = "aptina,mt9v024m" },
> + { .compatible = "aptina,mt9v032" },
> + { .compatible = "aptina,mt9v032m" },
> + { .compatible = "aptina,mt9v034" },
> + { .compatible = "aptina,mt9v034m" },
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mt9v032_of_match);
> +#endif
> +
> static struct i2c_driver mt9v032_driver = {
> .driver = {
> .name = "mt9v032",
> + .of_match_table = of_match_ptr(mt9v032_of_match),
> },
> .probe = mt9v032_probe,
> .remove = mt9v032_remove,
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 10/18] omap3isp: Move the syscon register out of the ISP register maps
Hi Tony,
Thanks for the comments!!
On Mon, Mar 09, 2015 at 08:20:38AM -0700, Tony Lindgren wrote:
> * Sakari Ailus [150307 15:44]:
> > Hi Laurent,
> >
> > On Sun, Mar 08, 2015 at 01:34:17AM +0200, Laurent Pinchart wrote:
> > > Hi Sakari,
> > >
> > > Thank you for the patch.
> > >
> > > (CC'ing linux-omap and Tony)
> >
> > Thanks.
> >
> > > On Saturday 07 March 2015 23:41:07 Sakari Ailus wrote:
> > > > The syscon register isn't part of the ISP, use it through the syscom
> > > > driver
> > > > regmap instead. The syscom block is considered to be from 343x on ISP
> > > > revision 2.0 whereas 15.0 is assumed to have 3630 syscon.
> > > >
> > > > Signed-off-by: Sakari Ailus
> > > > ---
> > > > arch/arm/boot/dts/omap3.dtsi|2 +-
> > > > arch/arm/mach-omap2/devices.c | 10 --
> > > > drivers/media/platform/omap3isp/isp.c | 19 +++
> > > > drivers/media/platform/omap3isp/isp.h | 19 +--
> > > > drivers/media/platform/omap3isp/ispcsiphy.c | 20 +---
> > >
> > > You might be asked to split the patch into two, let's see what Tony says.
> > >
> > > > 5 files changed, 42 insertions(+), 28 deletions(-)
> > > >
> > > > diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
> > > > index 01b7111..fe0b293 100644
> > > > --- a/arch/arm/boot/dts/omap3.dtsi
> > > > +++ b/arch/arm/boot/dts/omap3.dtsi
> > > > @@ -183,7 +183,7 @@
> > > >
> > > > omap3_scm_general: tisyscon@48002270 {
> > > > compatible = "syscon";
> > > > - reg = <0x48002270 0x2f0>;
> > > > + reg = <0x48002270 0x2f4>;
> > > > };
> > > >
> > > > pbias_regulator: pbias_regulator {
>
> Can you please send the above dts change separately as a fix describing
> what goes wrong? Let's get that out of the way for the -rc, otherwise
> we're going to probably get conflicts with Tero's dts changes.
Sure.
There's one register that didn't used to be mapped to syscon.
> > > > diff --git a/arch/arm/mach-omap2/devices.c
> > > > b/arch/arm/mach-omap2/devices.c
> > > > index 1afb50d..e945957 100644
> > > > --- a/arch/arm/mach-omap2/devices.c
> > > > +++ b/arch/arm/mach-omap2/devices.c
> > > > @@ -143,16 +143,6 @@ static struct resource omap3isp_resources[] = {
> > > > .flags = IORESOURCE_MEM,
> > > > },
> > > > {
> > > > - .start = OMAP343X_CTRL_BASE +
> > > > OMAP343X_CONTROL_CSIRXFE,
> > > > - .end= OMAP343X_CTRL_BASE +
> > > > OMAP343X_CONTROL_CSIRXFE + 3,
> > > > - .flags = IORESOURCE_MEM,
> > > > - },
> > > > - {
> > > > - .start = OMAP343X_CTRL_BASE +
> > > > OMAP3630_CONTROL_CAMERA_PHY_CTRL,
> > > > - .end= OMAP343X_CTRL_BASE +
> > > > OMAP3630_CONTROL_CAMERA_PHY_CTRL + 3,
> > > > - .flags = IORESOURCE_MEM,
> > > > - },
> > > > - {
> > > > .start = 24 + OMAP_INTC_START,
> > > > .flags = IORESOURCE_IRQ,
> > > > }
>
> Looks good to me, teel free to merge this part along with the other
> isp changes:
>
> Acked-by: Tony Lindgren
Thanks!
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 16/18] arm: dts: omap3: Add DT entries for OMAP 3
On Sun, Mar 08, 2015 at 01:51:51AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Saturday 07 March 2015 23:41:13 Sakari Ailus wrote:
> > The resources the ISP needs are slightly different on 3[45]xx and 3[67]xx.
> > Especially the phy-type property is different.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > arch/arm/boot/dts/omap34xx.dtsi | 15 +++
> > arch/arm/boot/dts/omap36xx.dtsi | 15 +++
> > 2 files changed, 30 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/omap34xx.dtsi
> > b/arch/arm/boot/dts/omap34xx.dtsi index 3819c1e..4c034d0 100644
> > --- a/arch/arm/boot/dts/omap34xx.dtsi
> > +++ b/arch/arm/boot/dts/omap34xx.dtsi
> > @@ -37,6 +37,21 @@
> > pinctrl-single,register-width = <16>;
> > pinctrl-single,function-mask = <0xff1f>;
> > };
> > +
> > + omap3_isp: omap3_isp@480bc000 {
> > + compatible = "ti,omap3-isp";
> > + reg = <0x480bc000 0x12fc
> > + 0x480bd800 0x017c>;
> > + interrupts = <24>;
> > + iommus = <&mmu_isp>;
> > + syscon = <&omap3_scm_general 0xdc>;
> > + ti,phy-type = <0>;
> > + #clock-cells = <1>;
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
>
> How about predefining the ports too ?
After a short discussion, we decided not to add port nodes. The arguments
considered were:
- Port nodes could help integrators writing the DT nodes. However the port
nodes are easier to construct than endpoint nodes. Board specific
configuration would also still need to be added.
- Extra port nodes take space which could be spent more usefully for other
purposes.
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 15/18] omap3isp: Add support for the Device Tree
Hi Laurent,
On Thu, Mar 12, 2015 at 01:48:02AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Saturday 07 March 2015 23:41:12 Sakari Ailus wrote:
> > Add the ISP device to omap3 DT include file and add support to the driver to
> > use it.
> >
> > Also obtain information on the external entities and the ISP configuration
> > related to them through the Device Tree in addition to the platform data.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > drivers/media/platform/omap3isp/isp.c | 206 ++--
> > drivers/media/platform/omap3isp/isp.h | 11 ++
> > drivers/media/platform/omap3isp/ispcsiphy.c |7 +
> > 3 files changed, 213 insertions(+), 11 deletions(-)
>
> [snip]
>
> > @@ -2358,14 +2541,6 @@ static int isp_probe(struct platform_device *pdev)
> > isp->mmio_hist_base_phys =
> > mem->start + isp_res_maps[m].offset[OMAP3_ISP_IOMEM_HIST];
> >
> > - isp->syscon = syscon_regmap_lookup_by_pdevname("syscon.0");
> > - isp->syscon_offset = isp_res_maps[m].syscon_offset;
>
> You're removing syscon_offset initialization here but not adding it anywhere
> else. This patch doesn't match the commit in your rm696-053-upstream branch,
> could you send the right version ? I'll then review it.
Yeah, there have been quite a few changes since I posted this RFC set, this
including. I'll post a new version once I've been able to take into account
all the comments I've got so far.
It'd be nice if someone could test the pdata support; I haven't had a chance
to do that in a few years now. :-)
--
Cheers,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 14/18] dt: bindings: Add bindings for omap3isp
Hi Sebastian,
Thanks for the comments!
On Fri, Mar 13, 2015 at 10:34:53AM +0100, Sebastian Reichel wrote:
> Hi,
>
> On Fri, Mar 13, 2015 at 01:03:21AM +0200, Sakari Ailus wrote:
> > [...]
> >
> > > > +Required properties
> > > > +===
> > > > +
> > > > +compatible : "ti,omap3-isp"
> > >
> > > I would rephrase that using the usual wording as "compatible: Must
> > > contain
> > > "ti,omap3-isp".
> >
> > [...]
> >
> > > > +ti,phy-type: 0 -- 3430; 1 -- 3630
> > >
> > > Would it make sense to add #define's for this ?
> >
> > I'll use OMAP3ISP_PHY_TYPE_COMPLEX_IO and OMAP3ISP_PHY_TYPE_CSIPHY as
> > discussed.
> >
> > > It could also make sense to document/name them "Complex I/O" and "CSIPHY"
> > > to
> > > avoid referring to the SoC that implements them, as the ISP is also found
> > > in
> > > SoCs other than 3430 and 3630.
> > >
> > > Could the PHY type be derived from the ES revision that we query at
> > > runtime ?
> >
> > I think this would work on 3430 and 3630 but I'm not certain about others.
> >
> > > We should also take into account the fact that the DM3730 has officially
> > > no
> > > CSIPHY, but still seems to implement them in practice.
> >
> > The DT sources are for 36xx, but I'd guess it works on 37xx as well, doesn't
> > it?
>
> In other drivers this kind of information is often extracted from the
> compatible string. For example:
>
> { .compatible = "ti,omap34xx-isp", .data = OMAP3ISP_PHY_TYPE_COMPLEX_IO, },
> { .compatible = "ti,omap36xx-isp", .data = OMAP3ISP_PHY_TYPE_CSIPHY, },
> ...
As Laurent said, I'd prefer to keep it as it is now; they phy selection
isn't really a part of the ISP in hardware either. It just happens to be
that the first phy selection logic can be found in 3430 (isp rev 2.0) and
latter in 3630 (isp rev 15.0).
> > [...]
> >
> > > > +Example
> > > > +===
> > > > +
> > > > + omap3_isp: omap3_isp@480bc000 {
> > >
> > > DT node names traditionally use - as a separator. Furthermore the phandle
> > > isn't needed. This should thus probably be
> > >
> > > omap3-isp@480bc000 {
> >
> > Fixed.
>
> According to ePAPR this should be a generic name (page 19); For
> example the i2c node name should be "i2c@address" instead of
> "omap3-i2c@address". There is no recommended generic term for an
> image signal processor, "isp" looks ok to me and seems to be
> already used in NVIDIA Tegra's device tree files. So maybe:
>
> isp@480bc000 {
Thanks for the suggestion. I'll fix that.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] media: i2c: add support for omnivision's ov2659 sensor
Hi Prabhakar,
On Thu, Mar 12, 2015 at 11:22:36PM +, Lad Prabhakar wrote:
...
> +static int ov2659_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + const struct ov2659_platform_data *pdata = ov2659_get_pdata(client);
> + struct v4l2_subdev *sd;
> + struct ov2659 *ov2659;
> + struct clk *clk;
> + int ret;
> +
> + if (!pdata) {
> + dev_err(&client->dev, "platform data not specified\n");
> + return -EINVAL;
> + }
> +
> + ov2659 = devm_kzalloc(&client->dev, sizeof(*ov2659), GFP_KERNEL);
> + if (!ov2659)
> + return -ENOMEM;
> +
> + ov2659->pdata = pdata;
> + ov2659->client = client;
> +
> + clk = devm_clk_get(&client->dev, "xvclk");
> + if (IS_ERR(clk))
> + return PTR_ERR(clk);
> +
> + ov2659->xvclk_frequency = clk_get_rate(clk);
> + if (ov2659->xvclk_frequency < 600 ||
> + ov2659->xvclk_frequency > 2700)
> + return -EINVAL;
> +
> + v4l2_ctrl_handler_init(&ov2659->ctrls, 2);
> + v4l2_ctrl_new_std(&ov2659->ctrls, &ov2659_ctrl_ops,
> + V4L2_CID_PIXEL_RATE, ov2659->xvclk_frequency,
> + ov2659->xvclk_frequency, 1, ov2659->xvclk_frequency);
ov2659->xvclk_frequency is the frequency of the external clock, not the
pixel rate. If I understand correctly, you should use the value of the
link-frequency property instead (as long as it's one pixel per clock).
With this fixed,
Acked-by: Sakari Ailus
> + v4l2_ctrl_new_std_menu_items(&ov2659->ctrls, &ov2659_ctrl_ops,
> + V4L2_CID_TEST_PATTERN,
> + ARRAY_SIZE(ov2659_test_pattern_menu) - 1,
> + 0, 0, ov2659_test_pattern_menu);
> + ov2659->sd.ctrl_handler = &ov2659->ctrls;
> +
> + if (ov2659->ctrls.error) {
> + dev_err(&client->dev, "%s: control initialization error %d\n",
> + __func__, ov2659->ctrls.error);
> + return ov2659->ctrls.error;
> + }
> +
> + sd = &ov2659->sd;
> + client->flags |= I2C_CLIENT_SCCB;
> + v4l2_i2c_subdev_init(sd, client, &ov2659_subdev_ops);
> +
> + sd->internal_ops = &ov2659_subdev_internal_ops;
> + sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> + V4L2_SUBDEV_FL_HAS_EVENTS;
> +
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + ov2659->pad.flags = MEDIA_PAD_FL_SOURCE;
> + sd->entity.type = MEDIA_ENT_T_V4L2_SUBDEV_SENSOR;
> + ret = media_entity_init(&sd->entity, 1, &ov2659->pad, 0);
> + if (ret < 0) {
> + v4l2_ctrl_handler_free(&ov2659->ctrls);
> + return ret;
> + }
> +#endif
> +
> + mutex_init(&ov2659->lock);
> +
> + ov2659_get_default_format(&ov2659->format);
> + ov2659->frame_size = &ov2659_framesizes[2];
> + ov2659->format_ctrl_regs = ov2659_formats[0].format_ctrl_regs;
> +
> + ret = ov2659_detect(sd);
> + if (ret < 0)
> + goto error;
> +
> + /* Calculate the PLL register value needed */
> + ov2659_pll_calc_params(ov2659);
> +
> + ret = v4l2_async_register_subdev(&ov2659->sd);
> + if (ret)
> + goto error;
> +
> + dev_info(&client->dev, "%s sensor driver registered !!\n", sd->name);
> +
> + return 0;
> +
> +error:
> + v4l2_ctrl_handler_free(&ov2659->ctrls);
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> + media_entity_cleanup(&sd->entity);
> +#endif
> + mutex_destroy(&ov2659->lock);
> + return ret;
> +}
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 14/18] dt: bindings: Add bindings for omap3isp
Hi Laurent, On Fri, Mar 13, 2015 at 01:11:03AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > On Friday 13 March 2015 01:03:21 Sakari Ailus wrote: > > On Thu, Mar 12, 2015 at 01:39:07AM +0200, Laurent Pinchart wrote: > > > On Saturday 07 March 2015 23:41:11 Sakari Ailus wrote: > > > > Signed-off-by: Sakari Ailus > > > > --- > > > > > > > > .../devicetree/bindings/media/ti,omap3isp.txt | 64 + > > > > MAINTAINERS|1 + > > > > 2 files changed, 65 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/media/ti,omap3isp.txt > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/ti,omap3isp.txt > > > > b/Documentation/devicetree/bindings/media/ti,omap3isp.txt new file mode > > > > 100644 > > > > index 000..2059524 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/media/ti,omap3isp.txt > > [snip] > > > > > +syscon : syscon phandle and register offset > > > > > > We should document what the register offset is. > > > > This is SoC specific as is the base address. I'm not sure that would be > > relevant here. If you think so, shouldn't we also add the device base > > addresses and register block lengths? > > I meant something like > > syscon: the phandle and register offset to the Complex I/O or CSI-PHY > register. Oh, I misunderstood you. I'll use that text. > > > > +ti,phy-type: 0 -- 3430; 1 -- 3630 > > > > > > Would it make sense to add #define's for this ? > > > > I'll use OMAP3ISP_PHY_TYPE_COMPLEX_IO and OMAP3ISP_PHY_TYPE_CSIPHY as > > discussed. > > > > > It could also make sense to document/name them "Complex I/O" and "CSIPHY" > > > to avoid referring to the SoC that implements them, as the ISP is also > > > found in SoCs other than 3430 and 3630. > > > > > > Could the PHY type be derived from the ES revision that we query at > > > runtime ? > > > > I think this would work on 3430 and 3630 but I'm not certain about others. > > > > > We should also take into account the fact that the DM3730 has officially > > > no CSIPHY, but still seems to implement them in practice. > > > > The DT sources are for 36xx, but I'd guess it works on 37xx as well, doesn't > > it? > > I think so. > > > > > +#clock-cells : Must be 1 --- the ISP provides two external clocks, > > > > + cam_xclka and cam_xclkb, at indices 0 and 1, > > > > + respectively. Please find more information on common > > > > + clock bindings in ../clock/clock-bindings.txt. > > > > + > > > > +Port nodes (optional) > > > > +- > > > > > > This should refer to Documentation/devicetree/bindings/media/video- > > > interfaces.txt. > > > > There's a reference to video-interfaces.txt in the beginning of the file. Do > > you think that'd be enough? > > I've missed that. I think you could move the reference here. Done. -- Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 14/18] dt: bindings: Add bindings for omap3isp
Hi Laurent, On Thu, Mar 12, 2015 at 01:39:07AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > On Saturday 07 March 2015 23:41:11 Sakari Ailus wrote: > > Signed-off-by: Sakari Ailus > > --- > > .../devicetree/bindings/media/ti,omap3isp.txt | 64 + > > MAINTAINERS|1 + > > 2 files changed, 65 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/ti,omap3isp.txt > > > > diff --git a/Documentation/devicetree/bindings/media/ti,omap3isp.txt > > b/Documentation/devicetree/bindings/media/ti,omap3isp.txt new file mode > > 100644 > > index 000..2059524 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/ti,omap3isp.txt > > @@ -0,0 +1,64 @@ > > +OMAP 3 ISP Device Tree bindings > > +=== > > + > > +More documentation on these bindings is available in > > +video-interfaces.txt in the same directory. > > + > > +Required properties > > +=== > > + > > +compatible : "ti,omap3-isp" > > I would rephrase that using the usual wording as "compatible: Must contain > "ti,omap3-isp". Fixed. > > +reg: a set of two register block physical addresses and > > + lengths > > We should describe what each set represents and contains. As discussed: reg : the two registers sets (physical address and length) for the ISP. The first set contains the core ISP registers up to the end of the SBL block. The second set contains the CSI PHYs and receivers registers. > > +interrupts : the interrupt number > > I would keep the wording generic and refer to interrupt specifier instead of > interrupt number. > > "interrupts: the ISP interrupt specifier" Fixed. > > +iommus : phandle of the IOMMU > > Similarly, > > "iommus: phandle and IOMMU specifier for the IOMMU that serves the ISP" Ditto. > > +syscon : syscon phandle and register offset > > We should document what the register offset is. This is SoC specific as is the base address. I'm not sure that would be relevant here. If you think so, shouldn't we also add the device base addresses and register block lengths? > > +ti,phy-type: 0 -- 3430; 1 -- 3630 > > Would it make sense to add #define's for this ? I'll use OMAP3ISP_PHY_TYPE_COMPLEX_IO and OMAP3ISP_PHY_TYPE_CSIPHY as discussed. > It could also make sense to document/name them "Complex I/O" and "CSIPHY" to > avoid referring to the SoC that implements them, as the ISP is also found in > SoCs other than 3430 and 3630. > > Could the PHY type be derived from the ES revision that we query at runtime ? I think this would work on 3430 and 3630 but I'm not certain about others. > We should also take into account the fact that the DM3730 has officially no > CSIPHY, but still seems to implement them in practice. The DT sources are for 36xx, but I'd guess it works on 37xx as well, doesn't it? > > +#clock-cells : Must be 1 --- the ISP provides two external clocks, > > + cam_xclka and cam_xclkb, at indices 0 and 1, > > + respectively. Please find more information on common > > + clock bindings in ../clock/clock-bindings.txt. > > + > > +Port nodes (optional) > > +- > > This should refer to Documentation/devicetree/bindings/media/video- > interfaces.txt. There's a reference to video-interfaces.txt in the beginning of the file. Do you think that'd be enough? > > +reg: The interface: > > + 0 - parallel (CCDC) > > + 1 - CSIPHY1 -- CSI2C / CCP2B on 3630; > > + CSI1 -- CSIb on 3430 > > + 2 - CSIPHY2 -- CSI2A / CCP2B on 3630; > > + CSI2 -- CSIa on 3430 > > + > > +Optional properties > > +=== > > + > > +vdd-csiphy1-supply : voltage supply of the CSI-2 PHY 1 > > +vdd-csiphy2-supply : voltage supply of the CSI-2 PHY 2 > > + > > +Endpoint nodes > > +-- > > + > > +lane-polarity : lane polarity (required on CSI-2) > > + 0 -- not inverted; 1 -- inverted > > +data-lanes : an array of data lanes from 1 to 3. The length can > > + be either 1 or 2. (required CSI-2) > > s/required/required on/ ? Fixed. > > +clock-lanes: the clock lane (from 1 to 3). (required on C
Re: [RFC 13/18] v4l: of: Read lane-polarity endpoint property
On Fri, Mar 13, 2015 at 12:23:27AM +0200, Sakari Ailus wrote:
...
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(bus->lane_polarity); i++) {
> > > + polarity = of_prop_next_u32(prop, polarity, &v);
> > > + if (!polarity)
> > > + break;
> > > + bus->lane_polarity[i] = v;
> > > + }
> >
> > Should we check that i == num_data_lines + 1 ?
>
> Good question. I think I'd just replace this with
> of_property_read_u32_array() instead, how about that? Then there would have
> to be at least as many lane polarities defined as there are lanes (data and
> clock). Defining more wouldn't be an error.
Oh, I missed the variable in the struct is bool. I'll just add the check.
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 13/18] v4l: of: Read lane-polarity endpoint property
Hi Laurent,
On Sun, Mar 08, 2015 at 01:49:26AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Saturday 07 March 2015 23:41:10 Sakari Ailus wrote:
> > Add lane_polarity field to struct v4l2_of_bus_mipi_csi2 and write the
> > contents of the lane polarity property to it. The field tells the polarity
> > of the physical lanes starting from the first one. Any unused lanes are
> > ignored, i.e. only the polarity of the used lanes is specified.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > drivers/media/v4l2-core/v4l2-of.c | 21 -
> > include/media/v4l2-of.h |3 +++
> > 2 files changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-of.c
> > b/drivers/media/v4l2-core/v4l2-of.c index b4ed9a9..a7a855e 100644
> > --- a/drivers/media/v4l2-core/v4l2-of.c
> > +++ b/drivers/media/v4l2-core/v4l2-of.c
> > @@ -23,7 +23,6 @@ static void v4l2_of_parse_csi_bus(const struct device_node
> > *node, struct v4l2_of_endpoint *endpoint)
> > {
> > struct v4l2_of_bus_mipi_csi2 *bus = &endpoint->bus.mipi_csi2;
> > - u32 data_lanes[ARRAY_SIZE(bus->data_lanes)];
> > struct property *prop;
> > bool have_clk_lane = false;
> > unsigned int flags = 0;
> > @@ -34,14 +33,26 @@ static void v4l2_of_parse_csi_bus(const struct
> > device_node *node, const __be32 *lane = NULL;
> > int i;
> >
> > - for (i = 0; i < ARRAY_SIZE(data_lanes); i++) {
> > - lane = of_prop_next_u32(prop, lane, &data_lanes[i]);
> > + for (i = 0; i < ARRAY_SIZE(bus->data_lanes); i++) {
> > + lane = of_prop_next_u32(prop, lane, &v);
> > if (!lane)
> > break;
> > + bus->data_lanes[i] = v;
> > }
> > bus->num_data_lanes = i;
> > - while (i--)
> > - bus->data_lanes[i] = data_lanes[i];
> > + }
> > +
> > + prop = of_find_property(node, "lane-polarity", NULL);
> > + if (prop) {
> > + const __be32 *polarity = NULL;
> > + int i;
>
> Could you please use unsigned int instead of int as the loop index can't have
> negative value ? Feel free to fix the index in the previous loop too :-)
Fixed both.
> > +
> > + for (i = 0; i < ARRAY_SIZE(bus->lane_polarity); i++) {
> > + polarity = of_prop_next_u32(prop, polarity, &v);
> > + if (!polarity)
> > + break;
> > + bus->lane_polarity[i] = v;
> > + }
>
> Should we check that i == num_data_lines + 1 ?
Good question. I think I'd just replace this with
of_property_read_u32_array() instead, how about that? Then there would have
to be at least as many lane polarities defined as there are lanes (data and
clock). Defining more wouldn't be an error.
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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] media: i2c: add support for omnivision's ov2659 sensor
Hi Laurent,
On Wed, Mar 11, 2015 at 08:09:11PM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> On Wednesday 11 March 2015 13:04:43 Sakari Ailus wrote:
> > On Sun, Mar 08, 2015 at 11:33:27AM +, Lad Prabhakar wrote:
> > > From: Benoit Parrot
> > >
> > > this patch adds support for omnivision's ov2659
> > > sensor, the driver supports following features:
> > > 1: Asynchronous probing
> > > 2: DT support
> > > 3: Media controller support
> > >
> > > Signed-off-by: Benoit Parrot
> > > Signed-off-by: Lad, Prabhakar
> > > ---
> > >
> > > Sorry quick new version, I need to get this merged for next
> > > merge window.
> > >
> > > Changes for v4:
> > > 1: Renamed target frequency property to 'link-frequencies'
> > > as per Sakari's suggestion.
> > >
> > > 2: Changed the copyright to "GPL v2"
> > >
> > > v3: https://patchwork.kernel.org/patch/5959401/
> > > v2: https://patchwork.kernel.org/patch/5859801/
> > > v1: https://patchwork.linuxtv.org/patch/27919/
> > >
> > > .../devicetree/bindings/media/i2c/ov2659.txt | 38 +
> > > MAINTAINERS| 10 +
> > > drivers/media/i2c/Kconfig | 11 +
> > > drivers/media/i2c/Makefile |1 +
> > > drivers/media/i2c/ov2659.c | 1439 +++
> > > include/media/ov2659.h | 33 +
> > > 6 files changed, 1532 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2659.txt
> > > create mode 100644 drivers/media/i2c/ov2659.c
> > > create mode 100644 include/media/ov2659.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> > > b/Documentation/devicetree/bindings/media/i2c/ov2659.txt new file mode
> > > 100644
> > > index 000..a655500
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> > > @@ -0,0 +1,38 @@
> > > +* OV2659 1/5-Inch 2Mp SOC Camera
> > > +
> > > +The Omnivision OV2659 is a 1/5-inch SOC camera, with an active array size
> > > of +1632H x 1212V. It is programmable through a SCCB. The OV2659 sensor
> > > supports +multiple resolutions output, such as UXGA, SVGA, 720p. It also
> > > can support +YUV422, RGB565/555 or raw RGB output formats.
> > > +
> > > +Required Properties:
> > > +- compatible: Must be "ovti,ov2659"
> > > +- reg: I2C slave address
> > > +- clocks: reference to the xvclk input clock.
> > > +- clock-names: should be "xvclk".
> > > +- link-frequencies: target pixel clock frequency.
> > > +
> > > +For further reading on port node refer to
> > > +Documentation/devicetree/bindings/media/video-interfaces.txt.
> > > +
> > > +Example:
> > > +
> > > + i2c0@1c22000 {
> > > + ...
> > > + ...
> > > + ov2659@30 {
> > > + compatible = "ovti,ov2659";
> > > + reg = <0x30>;
> > > +
> > > + clocks = <&clk_ov2659 0>;
> > > + clock-names = "xvclk";
> > > +
> > > + port {
> > > + ov2659_0: endpoint {
> > > + remote-endpoint = <&vpfe_ep>;
> > > + link-frequencies = <7000>;
> >
> > link-frequencies = /bits/ 64 <7000>;
> >
> > > + };
> > > + };
> > > + };
> > > + ...
> > > + };
>
> [snip]
>
> > > new file mode 100644
> > > index 000..487cb19
> > > --- /dev/null
> > > +++ b/include/media/ov2659.h
> > > @@ -0,0 +1,33 @@
> > > +/*
> > > + * Omnivision OV2659 CMOS Image Sensor driver
> > > + *
> > > + * Copyright (C) 2015 Texas Instruments, Inc.
> > > + *
> > > + * Benoit Parrot
> > > + * Lad, Prabhakar
> > > + *
> > > + * This program is free software; you may redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; version 2 of the Licen
Re: [PATCH v4] media: i2c: add support for omnivision's ov2659 sensor
Hi Prabhakar,
On Sun, Mar 08, 2015 at 11:33:27AM +, Lad Prabhakar wrote:
> From: Benoit Parrot
>
> this patch adds support for omnivision's ov2659
> sensor, the driver supports following features:
> 1: Asynchronous probing
> 2: DT support
> 3: Media controller support
>
> Signed-off-by: Benoit Parrot
> Signed-off-by: Lad, Prabhakar
> ---
> Sorry quick new version, I need to get this merged for next
> merge window.
>
> Changes for v4:
> 1: Renamed target frequency property to 'link-frequencies'
> as per Sakari's suggestion.
> 2: Changed the copyright to "GPL v2"
>
> v3: https://patchwork.kernel.org/patch/5959401/
> v2: https://patchwork.kernel.org/patch/5859801/
> v1: https://patchwork.linuxtv.org/patch/27919/
>
> .../devicetree/bindings/media/i2c/ov2659.txt | 38 +
> MAINTAINERS| 10 +
> drivers/media/i2c/Kconfig | 11 +
> drivers/media/i2c/Makefile |1 +
> drivers/media/i2c/ov2659.c | 1439
>
> include/media/ov2659.h | 33 +
> 6 files changed, 1532 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/i2c/ov2659.txt
> create mode 100644 drivers/media/i2c/ov2659.c
> create mode 100644 include/media/ov2659.h
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> new file mode 100644
> index 000..a655500
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/i2c/ov2659.txt
> @@ -0,0 +1,38 @@
> +* OV2659 1/5-Inch 2Mp SOC Camera
> +
> +The Omnivision OV2659 is a 1/5-inch SOC camera, with an active array size of
> +1632H x 1212V. It is programmable through a SCCB. The OV2659 sensor supports
> +multiple resolutions output, such as UXGA, SVGA, 720p. It also can support
> +YUV422, RGB565/555 or raw RGB output formats.
> +
> +Required Properties:
> +- compatible: Must be "ovti,ov2659"
> +- reg: I2C slave address
> +- clocks: reference to the xvclk input clock.
> +- clock-names: should be "xvclk".
> +- link-frequencies: target pixel clock frequency.
> +
> +For further reading on port node refer to
> +Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +Example:
> +
> + i2c0@1c22000 {
> + ...
> + ...
> + ov2659@30 {
> + compatible = "ovti,ov2659";
> + reg = <0x30>;
> +
> + clocks = <&clk_ov2659 0>;
> + clock-names = "xvclk";
> +
> + port {
> + ov2659_0: endpoint {
> + remote-endpoint = <&vpfe_ep>;
> + link-frequencies = <7000>;
link-frequencies = /bits/ 64 <7000>;
> + };
> + };
> + };
> + ...
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index ddc5a8c..4006cc8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8910,6 +8910,16 @@ T: git
> git://linuxtv.org/mhadli/v4l-dvb-davinci_devices.git
> S: Maintained
> F: drivers/media/platform/am437x/
>
> +OV2659 OMNIVISION SENSOR DRIVER
> +M: Lad, Prabhakar
> +L: linux-me...@vger.kernel.org
> +W: http://linuxtv.org/
> +Q: http://patchwork.linuxtv.org/project/linux-media/list/
> +T: git git://linuxtv.org/mhadli/v4l-dvb-davinci_devices.git
> +S: Maintained
> +F: drivers/media/i2c/ov2659.c
> +F: include/media/ov2659.h
> +
> SIS 190 ETHERNET DRIVER
> M: Francois Romieu
> L: net...@vger.kernel.org
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index da58c9b..6f30ea7 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -466,6 +466,17 @@ config VIDEO_APTINA_PLL
> config VIDEO_SMIAPP_PLL
> tristate
>
> +config VIDEO_OV2659
> + tristate "OmniVision OV2659 sensor support"
> + depends on VIDEO_V4L2 && I2C
> + depends on MEDIA_CAMERA_SUPPORT
> + ---help---
> + This is a Video4Linux2 sensor-level driver for the OmniVision
> + OV2659 camera.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ov2659.
> +
> config VIDEO_OV7640
> tristate "OmniVision OV7640 sensor support"
> depends on I2C && VIDEO_V4L2
> diff --git a/drivers/media/i2c/Makefile b/drivers/media/i2c/Makefile
> index 98589001..f165fae 100644
> --- a/drivers/media/i2c/Makefile
> +++ b/drivers/media/i2c/Makefile
> @@ -77,3 +77,4 @@ obj-$(CONFIG_VIDEO_SMIAPP_PLL) += smiapp-pll.o
> obj-$(CONFIG_VIDEO_AK881X) += ak881x.o
> obj-$(CONFIG_VIDEO_IR_I2C) += ir-kbd-i2c.o
> obj-$(CONFIG_VIDEO_ML86V7667)+= ml86v7667.o
> +obj-$(CONFIG_VIDEO_OV2659) += ov2659.o
> diff --git a/drivers/media/i2c/ov2659.c b/drivers/media/i2c/ov2659.c
> new file mode 100644
> index 000.
Re: [PATCH v4] media: i2c: add support for omnivision's ov2659 sensor
Hi Prabhakar, On Sun, Mar 08, 2015 at 11:33:27AM +, Lad Prabhakar wrote: > From: Benoit Parrot > > this patch adds support for omnivision's ov2659 > sensor, the driver supports following features: > 1: Asynchronous probing > 2: DT support > 3: Media controller support > > Signed-off-by: Benoit Parrot > Signed-off-by: Lad, Prabhakar Now that DT support is included, could you document it as well? There's a single proprerty to document. -- Cheers, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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] media: i2c: add support for omnivision's ov2659 sensor
Hi Prabhakar,
On Sun, Mar 08, 2015 at 11:33:27AM +, Lad Prabhakar wrote:
...
> +static struct ov2659_platform_data *
> +ov2659_get_pdata(struct i2c_client *client)
> +{
> + struct ov2659_platform_data *pdata;
> + struct device_node *endpoint;
> + int ret;
> +
> + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node) {
> + dev_err(&client->dev, "ov2659_get_pdata: DT Node found\n");
> + return client->dev.platform_data;
> + }
> +
> + endpoint = of_graph_get_next_endpoint(client->dev.of_node, NULL);
> + if (!endpoint)
> + return NULL;
> +
> + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + goto done;
> +
> + ret = of_property_read_u32(endpoint, "link-frequencies",
> +&pdata->link_frequency);
This is actually documented as being a 64-bit array.
The smiapp wasn't even reading it from the endpoint node. Oh well...
--
Regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v12 10/19] DT: Add documentation for the mfd Maxim max77693
Hi Jacek, On Mon, Mar 09, 2015 at 01:19:32PM +0100, Jacek Anaszewski wrote: > Hi Sakari, > > Thanks for the review. > > On 03/09/2015 11:54 AM, Sakari Ailus wrote: > >Hi Jacek, > > > >On Wed, Mar 04, 2015 at 05:14:31PM +0100, Jacek Anaszewski wrote: > >>This patch adds device tree binding documentation for > >>the flash cell of the Maxim max77693 multifunctional device. > >> > >>Signed-off-by: Jacek Anaszewski > >>Signed-off-by: Andrzej Hajda > >>Acked-by: Kyungmin Park > >>Cc: Lee Jones > >>Cc: Chanwoo Choi > >>Cc: Bryan Wu > >>Cc: Richard Purdie > >>--- > >> Documentation/devicetree/bindings/mfd/max77693.txt | 61 > >> > >> 1 file changed, 61 insertions(+) > >> > >>diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt > >>b/Documentation/devicetree/bindings/mfd/max77693.txt > >>index 38e6440..ab8fbd5 100644 > >>--- a/Documentation/devicetree/bindings/mfd/max77693.txt > >>+++ b/Documentation/devicetree/bindings/mfd/max77693.txt > >>@@ -76,7 +76,53 @@ Optional properties: > >> Valid values: 430, 470, 480, 490 > >> Default: 430 > >> > >>+- led : the LED submodule device node > >>+ > >>+There are two LED outputs available - FLED1 and FLED2. Each of them can > >>+control a separate LED or they can be connected together to double > >>+the maximum current for a single connected LED. One LED is represented > >>+by one child node. > >>+ > >>+Required properties: > >>+- compatible : Must be "maxim,max77693-led". > >>+ > >>+Optional properties: > >>+- maxim,trigger-type : Flash trigger type. > >>+ Possible trigger types: > >>+ LEDS_TRIG_TYPE_EDGE (0) - Rising edge of the signal triggers > >>+ the flash, > >>+ LEDS_TRIG_TYPE_LEVEL (1) - Strobe pulse length controls duration > >>+ of the flash. > >>+- maxim,boost-mode : > >>+ In boost mode the device can produce up to 1.2A of total current > >>+ on both outputs. The maximum current on each output is reduced > >>+ to 625mA then. If not enabled explicitly, boost setting defaults to > >>+ LEDS_BOOST_FIXED in case both current sources are used. > >>+ Possible values: > >>+ LEDS_BOOST_OFF (0) - no boost, > >>+ LEDS_BOOST_ADAPTIVE (1) - adaptive mode, > >>+ LEDS_BOOST_FIXED (2) - fixed mode. > >>+- maxim,boost-mvout : Output voltage of the boost module in millivolts. > >>+- maxim,mvsys-min : Low input voltage level in millivolts. Flash is not > >>fired > >>+ if chip estimates that system voltage could drop below this level due > >>+ to flash power consumption. > >>+ > >>+Required properties of the LED child node: > >>+- label : see Documentation/devicetree/bindings/leds/common.txt > > > >According to ePAPR, label is "a human readable string describing a device". > >There's no requirement that this would be unique, for instance. If you have > >a camera flash LED, there's necessarily no meaningful label for it, as it > >doesn't really tell the user anything (vs. HDD activity LED, for instance). > > > >I think I'd make this optional. > > OK. > > >What comes to entity naming in Media controller, the label isn't enough. As > >we haven't yet fully agreed on how to name the entities in the future, I'd > >propose sticking to current practices: chip name (and optional numerical LED > >ID) followed by the I2C address. The name should be specified by the driver. > > > >Do you have other than I2C busses required by the current drivers? > > I have AAT1290 device driven through GPIOs. There was also other driver, > for a similar device, submitted few days ago to linux-leds list. The problem indeed is defining a stable and unique identifier for a device in a system. In context of your patchset, I think this mostly matters in the V4L2 flash API wrapper patch. GPIO controlled devices are little bit more troublesome, as GPIO numbers alone aren't necessarily stable, but depend on the probing order. Well, i2c controllers could also be registered dynamically. The same goes for PCI devices, too, for instance. Most i2c adapters have a static id, and PCI devices have a stable bus address (unless system configuration is modified by e.g. adding or removing OTHER devices). I wonder if this could be resolved on OF-based systems by adding a string property, say, device name, whenever where are more than one device of a kind in the system. The string could just contain a numeric value, say 0 or 1. Cc Hans and Laurent. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] v4l: mt9v032: Add OF support
Hi Sylwester,
On Mon, Mar 09, 2015 at 11:35:52AM +0100, Sylwester Nawrocki wrote:
> Hi Laurent,
>
> On 08/03/15 14:45, Laurent Pinchart wrote:
> > +++ b/Documentation/devicetree/bindings/media/i2c/mt9v032.txt
> > @@ -0,0 +1,41 @@
> > +* Aptina 1/3-Inch WVGA CMOS Digital Image Sensor
> > +
> > +The Aptina MT9V032 is a 1/3-inch CMOS active pixel digital image sensor
> > with
> > +an active array size of 752H x 480V. It is programmable through a simple
> > +two-wire serial interface.
> > +
> > +Required Properties:
> > +
> > +- compatible: value should be either one among the following
> > + (a) "aptina,mt9v032" for MT9V032 color sensor
> > + (b) "aptina,mt9v032m" for MT9V032 monochrome sensor
> > + (c) "aptina,mt9v034" for MT9V034 color sensor
> > + (d) "aptina,mt9v034m" for MT9V034 monochrome sensor
>
> It can't be determined at runtime whether the sensor is just monochromatic ?
> Al in all the color filter array is a physical property of the sensor, still
> the driver seems to be ignoring the "m" suffix. Hence I suspect the register
> interfaces for both color and monochromatic versions are compatible.
> I'm wondering whether using a boolean property to indicate the color filter
> array type would do as well.
>
> > +static struct mt9v032_platform_data *
> > +mt9v032_get_pdata(struct i2c_client *client)
> > +{
> > + struct mt9v032_platform_data *pdata;
> > + struct v4l2_of_endpoint endpoint;
> > + struct device_node *np;
> > + struct property *prop;
> > +
> > + if (!IS_ENABLED(CONFIG_OF) || !client->dev.of_node)
> > + return client->dev.platform_data;
> > +
> > + np = v4l2_of_get_next_endpoint(client->dev.of_node, NULL);
> > + if (!np)
> > + return NULL;
> > +
> > + if (v4l2_of_parse_endpoint(np, &endpoint) < 0)
> > + goto done;
> > +
> > + pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
> > + if (!pdata)
> > + goto done;
> > +
> > + prop = of_find_property(np, "link-freqs", NULL);
>
> I suspect you meant "link-frequencies" here ?
Yes. I wonder if it'd make sense to add this to struct v4l2_of_endpoint. I
can write a patch for that.
> > + if (prop) {
> > + size_t size = prop->length / 8;
> > + u64 *link_freqs;
> > +
> > + link_freqs = devm_kzalloc(&client->dev,
> > + size * sizeof(*link_freqs),
> > + GFP_KERNEL);
> > + if (!link_freqs)
> > + goto done;
> > +
> > + if (of_property_read_u64_array(np, "link-frequencies",
> > + link_freqs, size) < 0)
> > + goto done;
> > +
> > + pdata->link_freqs = link_freqs;
> > + pdata->link_def_freq = link_freqs[0];
> > + }
If you're interested in just a single value, you can use
of_property_read_u64().
> > + pdata->clk_pol = !!(endpoint.bus.parallel.flags &
> > + V4L2_MBUS_PCLK_SAMPLE_RISING);
> > +
> > +done:
> > + of_node_put(np);
> > + return pdata;
> > +}
>
> > @@ -1034,9 +1086,21 @@ static const struct i2c_device_id mt9v032_id[] = {
> > };
> > MODULE_DEVICE_TABLE(i2c, mt9v032_id);
> >
> > +#if IS_ENABLED(CONFIG_OF)
> > +static const struct of_device_id mt9v032_of_match[] = {
> > + { .compatible = "mt9v032" },
> > + { .compatible = "mt9v032m" },
> > + { .compatible = "mt9v034" },
> > + { .compatible = "mt9v034m" },
> > + { /* Sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, mt9v032_of_match);
> > +#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 devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v12 10/19] DT: Add documentation for the mfd Maxim max77693
Hi Jacek,
On Wed, Mar 04, 2015 at 05:14:31PM +0100, Jacek Anaszewski wrote:
> This patch adds device tree binding documentation for
> the flash cell of the Maxim max77693 multifunctional device.
>
> Signed-off-by: Jacek Anaszewski
> Signed-off-by: Andrzej Hajda
> Acked-by: Kyungmin Park
> Cc: Lee Jones
> Cc: Chanwoo Choi
> Cc: Bryan Wu
> Cc: Richard Purdie
> ---
> Documentation/devicetree/bindings/mfd/max77693.txt | 61
>
> 1 file changed, 61 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt
> b/Documentation/devicetree/bindings/mfd/max77693.txt
> index 38e6440..ab8fbd5 100644
> --- a/Documentation/devicetree/bindings/mfd/max77693.txt
> +++ b/Documentation/devicetree/bindings/mfd/max77693.txt
> @@ -76,7 +76,53 @@ Optional properties:
> Valid values: 430, 470, 480, 490
> Default: 430
>
> +- led : the LED submodule device node
> +
> +There are two LED outputs available - FLED1 and FLED2. Each of them can
> +control a separate LED or they can be connected together to double
> +the maximum current for a single connected LED. One LED is represented
> +by one child node.
> +
> +Required properties:
> +- compatible : Must be "maxim,max77693-led".
> +
> +Optional properties:
> +- maxim,trigger-type : Flash trigger type.
> + Possible trigger types:
> + LEDS_TRIG_TYPE_EDGE (0) - Rising edge of the signal triggers
> + the flash,
> + LEDS_TRIG_TYPE_LEVEL (1) - Strobe pulse length controls duration
> + of the flash.
> +- maxim,boost-mode :
> + In boost mode the device can produce up to 1.2A of total current
> + on both outputs. The maximum current on each output is reduced
> + to 625mA then. If not enabled explicitly, boost setting defaults to
> + LEDS_BOOST_FIXED in case both current sources are used.
> + Possible values:
> + LEDS_BOOST_OFF (0) - no boost,
> + LEDS_BOOST_ADAPTIVE (1) - adaptive mode,
> + LEDS_BOOST_FIXED (2) - fixed mode.
> +- maxim,boost-mvout : Output voltage of the boost module in millivolts.
> +- maxim,mvsys-min : Low input voltage level in millivolts. Flash is not fired
> + if chip estimates that system voltage could drop below this level due
> + to flash power consumption.
> +
> +Required properties of the LED child node:
> +- label : see Documentation/devicetree/bindings/leds/common.txt
According to ePAPR, label is "a human readable string describing a device".
There's no requirement that this would be unique, for instance. If you have
a camera flash LED, there's necessarily no meaningful label for it, as it
doesn't really tell the user anything (vs. HDD activity LED, for instance).
I think I'd make this optional.
What comes to entity naming in Media controller, the label isn't enough. As
we haven't yet fully agreed on how to name the entities in the future, I'd
propose sticking to current practices: chip name (and optional numerical LED
ID) followed by the I2C address. The name should be specified by the driver.
Do you have other than I2C busses required by the current drivers?
> +- led-sources : see Documentation/devicetree/bindings/leds/common.txt;
> + device current output identifiers: 0 - FLED1, 1 - FLED2
> +
> +Optional properties of the LED child node:
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> + Range: 15625 - 25
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> + Range: 15625 - 100
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> + Range: 62500 - 100
> +
> Example:
> +#include
> +
> max77693@66 {
> compatible = "maxim,max77693";
> reg = <0x66>;
> @@ -117,5 +163,20 @@ Example:
> maxim,thermal-regulation-celsius = <75>;
> maxim,battery-overcurrent-microamp = <300>;
> maxim,charge-input-threshold-microvolt = <430>;
> +
> + led {
> + compatible = "maxim,max77693-led";
> + maxim,trigger-type = ;
> + maxim,boost-mode = ;
> + maxim,boost-mvout = <5000>;
> + maxim,mvsys-min = <2400>;
> +
> + camera_flash: flash-led {
> + label = "max77693-flash1";
> + led-sources = <0>, <1>;
> + max-m
Re: [RFC 17/18] arm: dts: n950, n9: Add primary camera support
Hi Laurent,
On Sun, Mar 08, 2015 at 01:56:13AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Saturday 07 March 2015 23:41:14 Sakari Ailus wrote:
> > Add support for the primary camera of the Nokia N950 and N9.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > arch/arm/boot/dts/omap3-n9.dts | 39 +++
> > arch/arm/boot/dts/omap3-n950-n9.dtsi |4
> > arch/arm/boot/dts/omap3-n950.dts | 39 +++
> > 3 files changed, 78 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/omap3-n9.dts b/arch/arm/boot/dts/omap3-n9.dts
> > index 9938b5d..05f32ae 100644
> > --- a/arch/arm/boot/dts/omap3-n9.dts
> > +++ b/arch/arm/boot/dts/omap3-n9.dts
> > @@ -16,3 +16,42 @@
> > model = "Nokia N9";
> > compatible = "nokia,omap3-n9", "ti,omap36xx", "ti,omap3";
> > };
> > +
> > +&i2c2 {
> > + clock-frequency = <40>;
> > +
> > + smia_1: camera@10 {
> > + compatible = "nokia,smia";
> > + reg = <0x10>;
> > + /* No reset gpio */
> > + vana-supply = <&vaux3>;
> > + clocks = <&omap3_isp 0>;
> > + clock-frequency = <960>;
> > + nokia,nvm-size = <1024>; /* 16 * 64 */
>
> You could actually specify that as "<(16 * 64)>".
Will fix.
> > + link-frequencies = /bits/ 64 <19920 21000 49920>;
> > + port {
> > + smia_1_1: endpoint {
> > + clock-lanes = <0>;
> > + data-lanes = <1 2>;
> > + remote-endpoint = <&csi2a_ep>;
> > + };
> > + };
> > + };
> > +};
> > +
> > +&omap3_isp {
> > + vdd-csiphy1-supply = <&vaux2>;
> > + vdd-csiphy2-supply = <&vaux2>;
> > + ports {
> > + port@2 {
> > + reg = <2>;
> > + csi2a_ep: endpoint {
> > + remote-endpoint = <&smia_1_1>;
> > + clock-lanes = <2>;
> > + data-lanes = <1 3>;
> > + crc = <1>;
> > + lane-polarity = <1 1 1>;
> > + };
> > + };
> > + };
> > +};
> > diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi
> > b/arch/arm/boot/dts/omap3-n950-n9.dtsi index c41db94..51e5043 100644
> > --- a/arch/arm/boot/dts/omap3-n950-n9.dtsi
> > +++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
> > @@ -86,10 +86,6 @@
> > regulator-max-microvolt = <280>;
> > };
> >
> > -&i2c2 {
> > - clock-frequency = <40>;
> > -};
> > -
>
> What's the reason for moving this to the N9 and N950 DT files as you keep the
> same value in both ?
Just before submitting the patches I thought someone might ask. :-D
I'll fix that.
> > &i2c3 {
> > clock-frequency = <40>;
> > };
> > diff --git a/arch/arm/boot/dts/omap3-n950.dts
> > b/arch/arm/boot/dts/omap3-n950.dts index 261c558..2b2ed9c 100644
> > --- a/arch/arm/boot/dts/omap3-n950.dts
> > +++ b/arch/arm/boot/dts/omap3-n950.dts
> > @@ -16,3 +16,42 @@
> > model = "Nokia N950";
> > compatible = "nokia,omap3-n950", "ti,omap36xx", "ti,omap3";
> > };
> > +
> > +&i2c2 {
> > + clock-frequency = <40>;
> > +
> > + smia_1: camera@10 {
> > + compatible = "nokia,smia";
> > + reg = <0x10>;
> > + /* No reset gpio */
> > + vana-supply = <&vaux3>;
> > + clocks = <&omap3_isp 0>;
> > + clock-frequency = <960>;
> > + nokia,nvm-size = <1024>; /* 16 * 64 */
> > + link-frequencies = /bits/ 64 <21000 33360 39840>;
> > + port {
> > + smia_1_1: endpoint {
> > + clock-lanes = <0>;
> > + data-lanes = <1 2>;
> > + remote-endpoint = <&csi2a_ep>;
> > + };
> > + };
> > + };
> > +};
> > +
> > +&omap3_isp {
> > + vdd-csiphy1-supply = <&vaux2>;
> > + vdd-csiphy2-supply = <&vaux2>;
> > + ports {
> > + port@2 {
> > + reg = <2>;
> > + csi2a_ep: endpoint {
> > + remote-endpoint = <&smia_1_1>;
> > + clock-lanes = <2>;
> > + data-lanes = <3 1>;
> > + crc = <1>;
> > + lane-polarity = <1 1 1>;
> > + };
> > + };
> > + };
> > +};
>
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 12/18] dt: bindings: Add lane-polarity property to endpoint nodes
Hi Laurent, On Sun, Mar 08, 2015 at 01:46:02AM +0200, Laurent Pinchart wrote: > Hi Sakari, > > Thank you for the patch. > > (CC'ing Sylwester) > > On Saturday 07 March 2015 23:41:09 Sakari Ailus wrote: > > Add lane-polarity property to endpoint nodes. This essentially tells that > > the order of the differential signal wires is inverted. > > > > Signed-off-by: Sakari Ailus > > --- > > Documentation/devicetree/bindings/media/video-interfaces.txt |5 + > > 1 file changed, 5 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt > > b/Documentation/devicetree/bindings/media/video-interfaces.txt index > > 571b4c6..058d1e6 100644 > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt > > @@ -106,6 +106,11 @@ Optional endpoint properties > > - link-frequencies: Allowed data bus frequencies. For MIPI CSI-2, for > >instance, this is the actual frequency of the bus, not bits per clock per > > lane value. An array of 64-bit unsigned integers. > > +- lane-polarity: an array of polarities of the lanes starting from the > > clock > > + lane and followed by the data lanes in the same order as in data-lanes. > > + Valid values are 0 (normal) and 1 (inverted). > > Would it make sense to add #define's for this ? Good question. I don't really have too much of an opinion. I think I'd just use a number until someone else needs this. :-) > > The length of the array > > + should be the combined length of data-lanes and clock-lanes > > properties. > > + This property is valid for serial busses only. > > You should also document what happens when the property is omitted. Will add. -- Regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 10/18] omap3isp: Move the syscon register out of the ISP register maps
Hi Laurent,
On Sun, Mar 08, 2015 at 01:34:17AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> (CC'ing linux-omap and Tony)
Thanks.
> On Saturday 07 March 2015 23:41:07 Sakari Ailus wrote:
> > The syscon register isn't part of the ISP, use it through the syscom driver
> > regmap instead. The syscom block is considered to be from 343x on ISP
> > revision 2.0 whereas 15.0 is assumed to have 3630 syscon.
> >
> > Signed-off-by: Sakari Ailus
> > ---
> > arch/arm/boot/dts/omap3.dtsi|2 +-
> > arch/arm/mach-omap2/devices.c | 10 --
> > drivers/media/platform/omap3isp/isp.c | 19 +++
> > drivers/media/platform/omap3isp/isp.h | 19 +--
> > drivers/media/platform/omap3isp/ispcsiphy.c | 20 +---
>
> You might be asked to split the patch into two, let's see what Tony says.
>
> > 5 files changed, 42 insertions(+), 28 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
> > index 01b7111..fe0b293 100644
> > --- a/arch/arm/boot/dts/omap3.dtsi
> > +++ b/arch/arm/boot/dts/omap3.dtsi
> > @@ -183,7 +183,7 @@
> >
> > omap3_scm_general: tisyscon@48002270 {
> > compatible = "syscon";
> > - reg = <0x48002270 0x2f0>;
> > + reg = <0x48002270 0x2f4>;
> > };
> >
> > pbias_regulator: pbias_regulator {
> > diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
> > index 1afb50d..e945957 100644
> > --- a/arch/arm/mach-omap2/devices.c
> > +++ b/arch/arm/mach-omap2/devices.c
> > @@ -143,16 +143,6 @@ static struct resource omap3isp_resources[] = {
> > .flags = IORESOURCE_MEM,
> > },
> > {
> > - .start = OMAP343X_CTRL_BASE + OMAP343X_CONTROL_CSIRXFE,
> > - .end= OMAP343X_CTRL_BASE + OMAP343X_CONTROL_CSIRXFE
> > + 3,
> > - .flags = IORESOURCE_MEM,
> > - },
> > - {
> > - .start = OMAP343X_CTRL_BASE +
> > OMAP3630_CONTROL_CAMERA_PHY_CTRL,
> > - .end= OMAP343X_CTRL_BASE +
> > OMAP3630_CONTROL_CAMERA_PHY_CTRL + 3,
> > - .flags = IORESOURCE_MEM,
> > - },
> > - {
> > .start = 24 + OMAP_INTC_START,
> > .flags = IORESOURCE_IRQ,
> > }
> > diff --git a/drivers/media/platform/omap3isp/isp.c
> > b/drivers/media/platform/omap3isp/isp.c index 68d7edfc..4ff4bbd 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -51,6 +51,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -94,8 +95,9 @@ static const struct isp_res_mapping isp_res_maps[] = {
> >1 << OMAP3_ISP_IOMEM_RESZ |
> >1 << OMAP3_ISP_IOMEM_SBL |
> >1 << OMAP3_ISP_IOMEM_CSI2A_REGS1 |
> > - 1 << OMAP3_ISP_IOMEM_CSIPHY2 |
> > - 1 << OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE,
> > + 1 << OMAP3_ISP_IOMEM_CSIPHY2,
> > + .syscon_offset = 0xdc,
> > + .phy_type = ISP_PHY_TYPE_3430,
> > },
> > {
> > .isp_rev = ISP_REVISION_15_0,
> > @@ -112,8 +114,9 @@ static const struct isp_res_mapping isp_res_maps[] = {
> >1 << OMAP3_ISP_IOMEM_CSI2A_REGS2 |
> >1 << OMAP3_ISP_IOMEM_CSI2C_REGS1 |
> >1 << OMAP3_ISP_IOMEM_CSIPHY1 |
> > - 1 << OMAP3_ISP_IOMEM_CSI2C_REGS2 |
> > - 1 << OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL,
> > + 1 << OMAP3_ISP_IOMEM_CSI2C_REGS2,
> > + .syscon_offset = 0x2f0,
> > + .phy_type = ISP_PHY_TYPE_3630,
> > },
> > };
> >
> > @@ -2352,6 +2355,14 @@ static int isp_probe(struct platform_device *pdev)
> > }
> > }
> >
> > + isp->syscon = syscon_regmap_lookup_by_pdevname("syscon.0");
> > + isp->syscon_offset = isp_res_maps[m].syscon_offset;
> > + isp->phy_type = isp_res_maps[m].phy_type;
>
> You could move those two lines after the error check to keep the check closer
> to the source of error.
Ack.
> Apart from that,
>
> Acked-by: Laurent Pinchart
Thanks for the acks!
--
Kind regards,
Sakari Ailus
e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] media: i2c: add support for omnivision's ov2659 sensor
Hi Prabhakar, On Sat, Mar 07, 2015 at 03:21:40PM +, Lad Prabhakar wrote: > + pixel-clock-frequency = <7000>; As commented privately, could you use the link-frequencies property for this purpose? On parallel bus if often equals the pixel clock, but works much better on serial busses. It'd documented in video-interfaces.txt. -- Kind regards, Sakari Ailus e-mail: sakari.ai...@iki.fi XMPP: sai...@retiisi.org.uk -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 16/18] arm: dts: omap3: Add DT entries for OMAP 3
The resources the ISP needs are slightly different on 3[45]xx and 3[67]xx.
Especially the phy-type property is different.
Signed-off-by: Sakari Ailus
---
arch/arm/boot/dts/omap34xx.dtsi | 15 +++
arch/arm/boot/dts/omap36xx.dtsi | 15 +++
2 files changed, 30 insertions(+)
diff --git a/arch/arm/boot/dts/omap34xx.dtsi b/arch/arm/boot/dts/omap34xx.dtsi
index 3819c1e..4c034d0 100644
--- a/arch/arm/boot/dts/omap34xx.dtsi
+++ b/arch/arm/boot/dts/omap34xx.dtsi
@@ -37,6 +37,21 @@
pinctrl-single,register-width = <16>;
pinctrl-single,function-mask = <0xff1f>;
};
+
+ omap3_isp: omap3_isp@480bc000 {
+ compatible = "ti,omap3-isp";
+ reg = <0x480bc000 0x12fc
+ 0x480bd800 0x017c>;
+ interrupts = <24>;
+ iommus = <&mmu_isp>;
+ syscon = <&omap3_scm_general 0xdc>;
+ ti,phy-type = <0>;
+ #clock-cells = <1>;
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+ };
};
};
diff --git a/arch/arm/boot/dts/omap36xx.dtsi b/arch/arm/boot/dts/omap36xx.dtsi
index 541704a..31ac41c 100644
--- a/arch/arm/boot/dts/omap36xx.dtsi
+++ b/arch/arm/boot/dts/omap36xx.dtsi
@@ -69,6 +69,21 @@
pinctrl-single,register-width = <16>;
pinctrl-single,function-mask = <0xff1f>;
};
+
+ omap3_isp: omap3_isp@480bc000 {
+ compatible = "ti,omap3-isp";
+ reg = <0x480bc000 0x12fc
+ 0x480bd800 0x0600>;
+ interrupts = <24>;
+ iommus = <&mmu_isp>;
+ syscon = <&omap3_scm_general 0x2f0>;
+ ti,phy-type = <1>;
+ #clock-cells = <1>;
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+ };
};
};
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 12/18] dt: bindings: Add lane-polarity property to endpoint nodes
Add lane-polarity property to endpoint nodes. This essentially tells that the order of the differential signal wires is inverted. Signed-off-by: Sakari Ailus --- Documentation/devicetree/bindings/media/video-interfaces.txt |5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt index 571b4c6..058d1e6 100644 --- a/Documentation/devicetree/bindings/media/video-interfaces.txt +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt @@ -106,6 +106,11 @@ Optional endpoint properties - link-frequencies: Allowed data bus frequencies. For MIPI CSI-2, for instance, this is the actual frequency of the bus, not bits per clock per lane value. An array of 64-bit unsigned integers. +- lane-polarity: an array of polarities of the lanes starting from the clock + lane and followed by the data lanes in the same order as in data-lanes. + Valid values are 0 (normal) and 1 (inverted). The length of the array + should be the combined length of data-lanes and clock-lanes properties. + This property is valid for serial busses only. Example -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 08/18] omap3isp: Calculate vpclk_div for CSI-2
The video port clock is l3_ick divided by vpclk_div. This clock must be high
enough for the external pixel rate. The video port requires two clock cycles
to process a pixel.
Signed-off-by: Sakari Ailus
---
drivers/media/platform/omap3isp/ispcsi2.c |8 +++-
include/media/omap3isp.h |2 --
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/media/platform/omap3isp/ispcsi2.c
b/drivers/media/platform/omap3isp/ispcsi2.c
index 14d279d..97cdfeb 100644
--- a/drivers/media/platform/omap3isp/ispcsi2.c
+++ b/drivers/media/platform/omap3isp/ispcsi2.c
@@ -548,6 +548,7 @@ int omap3isp_csi2_reset(struct isp_csi2_device *csi2)
static int csi2_configure(struct isp_csi2_device *csi2)
{
+ struct isp_pipeline *pipe = to_isp_pipeline(&csi2->subdev.entity);
const struct isp_bus_cfg *buscfg;
struct isp_device *isp = csi2->isp;
struct isp_csi2_timing_cfg *timing = &csi2->timing[0];
@@ -570,7 +571,12 @@ static int csi2_configure(struct isp_csi2_device *csi2)
csi2->frame_skip = 0;
v4l2_subdev_call(sensor, sensor, g_skip_frames, &csi2->frame_skip);
- csi2->ctrl.vp_out_ctrl = buscfg->bus.csi2.vpclk_div;
+ csi2->ctrl.vp_out_ctrl =
+ clamp_t(unsigned int, pipe->l3_ick / pipe->external_rate - 1,
+ 1, 3);
+ dev_dbg(isp->dev, "%s: l3_ick %lu, external_rate %u, vp_out_ctrl %u\n",
+ __func__, pipe->l3_ick, pipe->external_rate,
+ csi2->ctrl.vp_out_ctrl);
csi2->ctrl.frame_mode = ISP_CSI2_FRAME_IMMEDIATE;
csi2->ctrl.ecc_enable = buscfg->bus.csi2.crc;
diff --git a/include/media/omap3isp.h b/include/media/omap3isp.h
index 39e0748..0f0c08b 100644
--- a/include/media/omap3isp.h
+++ b/include/media/omap3isp.h
@@ -129,11 +129,9 @@ struct isp_ccp2_cfg {
/**
* struct isp_csi2_cfg - CSI2 interface configuration
* @crc: Enable the cyclic redundancy check
- * @vpclk_div: Video port output clock control
*/
struct isp_csi2_cfg {
unsigned crc:1;
- unsigned vpclk_div:2;
struct isp_csiphy_lanes_cfg lanecfg;
};
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 04/18] omap3isp: DT support for clocks
From: Laurent Pinchart
Signed-off-by: Laurent Pinchart
---
drivers/media/platform/omap3isp/isp.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c
index a607f26..01356dd 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -279,9 +279,21 @@ static const struct clk_init_data isp_xclk_init_data = {
.num_parents = 1,
};
+static struct clk *isp_xclk_src_get(struct of_phandle_args *clkspec, void
*data)
+{
+ unsigned int idx = clkspec->args[0];
+ struct isp_device *isp = data;
+
+ if (idx >= ARRAY_SIZE(isp->xclks))
+ return ERR_PTR(-ENOENT);
+
+ return isp->xclks[idx].clk;
+}
+
static int isp_xclk_init(struct isp_device *isp)
{
struct isp_platform_data *pdata = isp->pdata;
+ struct device_node *np = isp->dev->of_node;
struct clk_init_data init;
unsigned int i;
@@ -312,6 +324,12 @@ static int isp_xclk_init(struct isp_device *isp)
if (IS_ERR(xclk->clk))
return PTR_ERR(xclk->clk);
+ /* When instantiated from DT we don't need to register clock
+* aliases.
+*/
+ if (np)
+ continue;
+
if (pdata->xclks[i].con_id == NULL &&
pdata->xclks[i].dev_id == NULL)
continue;
@@ -327,13 +345,20 @@ static int isp_xclk_init(struct isp_device *isp)
clkdev_add(xclk->lookup);
}
+ if (np)
+ of_clk_add_provider(np, isp_xclk_src_get, isp);
+
return 0;
}
static void isp_xclk_cleanup(struct isp_device *isp)
{
+ struct device_node *np = isp->dev->of_node;
unsigned int i;
+ if (np)
+ of_clk_del_provider(np);
+
for (i = 0; i < ARRAY_SIZE(isp->xclks); ++i) {
struct isp_xclk *xclk = &isp->xclks[i];
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 03/18] omap3isp: Separate external link creation from platform data parsing
Move the code which connects the external entity to an ISP entity into a
separate function. This disconnects it from parsing the platform data.
Signed-off-by: Sakari Ailus
---
drivers/media/platform/omap3isp/isp.c | 147 +
1 file changed, 74 insertions(+), 73 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c
index 4ab674d..a607f26 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1832,6 +1832,77 @@ isp_register_subdev_group(struct isp_device *isp,
return sensor;
}
+static int isp_link_entity(
+ struct isp_device *isp, struct media_entity *entity,
+ enum isp_interface_type interface)
+{
+ struct media_entity *input;
+ unsigned int flags;
+ unsigned int pad;
+ unsigned int i;
+
+ /* Connect the sensor to the correct interface module.
+* Parallel sensors are connected directly to the CCDC, while
+* serial sensors are connected to the CSI2a, CCP2b or CSI2c
+* receiver through CSIPHY1 or CSIPHY2.
+*/
+ switch (interface) {
+ case ISP_INTERFACE_PARALLEL:
+ input = &isp->isp_ccdc.subdev.entity;
+ pad = CCDC_PAD_SINK;
+ flags = 0;
+ break;
+
+ case ISP_INTERFACE_CSI2A_PHY2:
+ input = &isp->isp_csi2a.subdev.entity;
+ pad = CSI2_PAD_SINK;
+ flags = MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED;
+ break;
+
+ case ISP_INTERFACE_CCP2B_PHY1:
+ case ISP_INTERFACE_CCP2B_PHY2:
+ input = &isp->isp_ccp2.subdev.entity;
+ pad = CCP2_PAD_SINK;
+ flags = 0;
+ break;
+
+ case ISP_INTERFACE_CSI2C_PHY1:
+ input = &isp->isp_csi2c.subdev.entity;
+ pad = CSI2_PAD_SINK;
+ flags = MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED;
+ break;
+
+ default:
+ dev_err(isp->dev, "%s: invalid interface type %u\n", __func__,
+ interface);
+ return -EINVAL;
+ }
+
+ /*
+* Not all interfaces are available on all revisions of the
+* ISP. The sub-devices of those interfaces aren't initialised
+* in such a case. Check this by ensuring the num_pads is
+* non-zero.
+*/
+ if (!input->num_pads) {
+ dev_err(isp->dev, "%s: invalid input %u\n", entity->name,
+ interface);
+ return -EINVAL;
+ }
+
+ for (i = 0; i < entity->num_pads; i++) {
+ if (entity->pads[i].flags & MEDIA_PAD_FL_SOURCE)
+ break;
+ }
+ if (i == entity->num_pads) {
+ dev_err(isp->dev, "%s: no source pad in external entity\n",
+ __func__);
+ return -EINVAL;
+ }
+
+ return media_entity_create_link(entity, i, input, pad, flags);
+}
+
static int isp_register_entities(struct isp_device *isp)
{
struct isp_platform_data *pdata = isp->pdata;
@@ -1894,85 +1965,15 @@ static int isp_register_entities(struct isp_device *isp)
/* Register external entities */
for (subdevs = pdata->subdevs; subdevs && subdevs->subdevs; ++subdevs) {
- struct v4l2_subdev *sensor;
- struct media_entity *input;
- unsigned int flags;
- unsigned int pad;
- unsigned int i;
+ struct v4l2_subdev *sensor =
+ isp_register_subdev_group(isp, subdevs->subdevs);
- sensor = isp_register_subdev_group(isp, subdevs->subdevs);
if (sensor == NULL)
continue;
sensor->host_priv = subdevs;
- /* Connect the sensor to the correct interface module. Parallel
-* sensors are connected directly to the CCDC, while serial
-* sensors are connected to the CSI2a, CCP2b or CSI2c receiver
-* through CSIPHY1 or CSIPHY2.
-*/
- switch (subdevs->interface) {
- case ISP_INTERFACE_PARALLEL:
- input = &isp->isp_ccdc.subdev.entity;
- pad = CCDC_PAD_SINK;
- flags = 0;
- break;
-
- case ISP_INTERFACE_CSI2A_PHY2:
- input = &isp->isp_csi2a.subdev.entity;
- pad = CSI2_PAD_SINK;
- flags = MEDIA_LNK_FL_IMMUTABLE
- | MEDIA_LNK_FL_ENABLED;
- break;
-
- case ISP_INTERFACE_CCP2B_PHY1:
- case ISP_INTERFACE_CCP2B_PHY2:
-
[RFC 05/18] omap3isp: Platform data could be NULL
Only check for call platform data callback functions if there's platform
data. Also take care of a few other cases where the NULL pdata pointer could
have been accessed, and remove the check for NULL dev->platform_data
pointer.
Removing the check for NULL dev->platform_data isn't strictly needed by the
DT support but there's no harm from that either: the device now can be used
without sensors, for instance.
Signed-off-by: Sakari Ailus
---
drivers/media/platform/omap3isp/isp.c | 10 --
drivers/media/platform/omap3isp/ispvideo.c |6 +++---
2 files changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c
index 01356dd..b836bc8 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -330,8 +330,8 @@ static int isp_xclk_init(struct isp_device *isp)
if (np)
continue;
- if (pdata->xclks[i].con_id == NULL &&
- pdata->xclks[i].dev_id == NULL)
+ if (!pdata || (pdata->xclks[i].con_id == NULL &&
+ pdata->xclks[i].dev_id == NULL))
continue;
xclk->lookup = kzalloc(sizeof(*xclk->lookup), GFP_KERNEL);
@@ -1989,7 +1989,8 @@ static int isp_register_entities(struct isp_device *isp)
goto done;
/* Register external entities */
- for (subdevs = pdata->subdevs; subdevs && subdevs->subdevs; ++subdevs) {
+ for (subdevs = pdata ? pdata->subdevs : NULL;
+subdevs && subdevs->subdevs; ++subdevs) {
struct v4l2_subdev *sensor =
isp_register_subdev_group(isp, subdevs->subdevs);
@@ -2271,9 +2272,6 @@ static int isp_probe(struct platform_device *pdev)
int ret;
int i, m;
- if (pdata == NULL)
- return -EINVAL;
-
isp = devm_kzalloc(&pdev->dev, sizeof(*isp), GFP_KERNEL);
if (!isp) {
dev_err(&pdev->dev, "could not allocate memory\n");
diff --git a/drivers/media/platform/omap3isp/ispvideo.c
b/drivers/media/platform/omap3isp/ispvideo.c
index 3fe9047..d644164 100644
--- a/drivers/media/platform/omap3isp/ispvideo.c
+++ b/drivers/media/platform/omap3isp/ispvideo.c
@@ -1022,7 +1022,7 @@ isp_video_streamon(struct file *file, void *fh, enum
v4l2_buf_type type)
pipe->entities = 0;
- if (video->isp->pdata->set_constraints)
+ if (video->isp->pdata && video->isp->pdata->set_constraints)
video->isp->pdata->set_constraints(video->isp, true);
pipe->l3_ick = clk_get_rate(video->isp->clock[ISP_CLK_L3_ICK]);
pipe->max_rate = pipe->l3_ick;
@@ -1104,7 +1104,7 @@ err_set_stream:
err_check_format:
media_entity_pipeline_stop(&video->video.entity);
err_pipeline_start:
- if (video->isp->pdata->set_constraints)
+ if (video->isp->pdata && video->isp->pdata->set_constraints)
video->isp->pdata->set_constraints(video->isp, false);
/* The DMA queue must be emptied here, otherwise CCDC interrupts that
* will get triggered the next time the CCDC is powered up will try to
@@ -1165,7 +1165,7 @@ isp_video_streamoff(struct file *file, void *fh, enum
v4l2_buf_type type)
video->queue = NULL;
video->error = false;
- if (video->isp->pdata->set_constraints)
+ if (video->isp->pdata && video->isp->pdata->set_constraints)
video->isp->pdata->set_constraints(video->isp, false);
media_entity_pipeline_stop(&video->video.entity);
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 13/18] v4l: of: Read lane-polarity endpoint property
Add lane_polarity field to struct v4l2_of_bus_mipi_csi2 and write the
contents of the lane polarity property to it. The field tells the polarity
of the physical lanes starting from the first one. Any unused lanes are
ignored, i.e. only the polarity of the used lanes is specified.
Signed-off-by: Sakari Ailus
---
drivers/media/v4l2-core/v4l2-of.c | 21 -
include/media/v4l2-of.h |3 +++
2 files changed, 19 insertions(+), 5 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-of.c
b/drivers/media/v4l2-core/v4l2-of.c
index b4ed9a9..a7a855e 100644
--- a/drivers/media/v4l2-core/v4l2-of.c
+++ b/drivers/media/v4l2-core/v4l2-of.c
@@ -23,7 +23,6 @@ static void v4l2_of_parse_csi_bus(const struct device_node
*node,
struct v4l2_of_endpoint *endpoint)
{
struct v4l2_of_bus_mipi_csi2 *bus = &endpoint->bus.mipi_csi2;
- u32 data_lanes[ARRAY_SIZE(bus->data_lanes)];
struct property *prop;
bool have_clk_lane = false;
unsigned int flags = 0;
@@ -34,14 +33,26 @@ static void v4l2_of_parse_csi_bus(const struct device_node
*node,
const __be32 *lane = NULL;
int i;
- for (i = 0; i < ARRAY_SIZE(data_lanes); i++) {
- lane = of_prop_next_u32(prop, lane, &data_lanes[i]);
+ for (i = 0; i < ARRAY_SIZE(bus->data_lanes); i++) {
+ lane = of_prop_next_u32(prop, lane, &v);
if (!lane)
break;
+ bus->data_lanes[i] = v;
}
bus->num_data_lanes = i;
- while (i--)
- bus->data_lanes[i] = data_lanes[i];
+ }
+
+ prop = of_find_property(node, "lane-polarity", NULL);
+ if (prop) {
+ const __be32 *polarity = NULL;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(bus->lane_polarity); i++) {
+ polarity = of_prop_next_u32(prop, polarity, &v);
+ if (!polarity)
+ break;
+ bus->lane_polarity[i] = v;
+ }
}
if (!of_property_read_u32(node, "clock-lanes", &v)) {
diff --git a/include/media/v4l2-of.h b/include/media/v4l2-of.h
index 70fa7b7..a70eb52 100644
--- a/include/media/v4l2-of.h
+++ b/include/media/v4l2-of.h
@@ -29,12 +29,15 @@ struct device_node;
* @data_lanes: an array of physical data lane indexes
* @clock_lane: physical lane index of the clock lane
* @num_data_lanes: number of data lanes
+ * @lane_polarity: polarity of the lanes. The order is the same of
+ *the physical lanes.
*/
struct v4l2_of_bus_mipi_csi2 {
unsigned int flags;
unsigned char data_lanes[4];
unsigned char clock_lane;
unsigned short num_data_lanes;
+ bool lane_polarity[5];
};
/**
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 18/18] omap3isp: Deprecate platform data support
Print a warning when the driver is used with platform data. Existing
platform data user should move to DT now.
Signed-off-by: Sakari Ailus
---
drivers/media/platform/omap3isp/isp.c |2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c
index 2c9bc0d..e4a78bb 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2447,6 +2447,8 @@ static int isp_probe(struct platform_device *pdev)
isp->syscon = syscon_regmap_lookup_by_pdevname("syscon.0");
if (IS_ERR(isp->syscon))
return PTR_ERR(isp->syscon);
+ dev_warn(&pdev->dev,
+"Platform data support is deprecated! Please move to
DT now!\n");
}
isp->autoidle = autoidle;
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 15/18] omap3isp: Add support for the Device Tree
Add the ISP device to omap3 DT include file and add support to the driver to
use it.
Also obtain information on the external entities and the ISP configuration
related to them through the Device Tree in addition to the platform data.
Signed-off-by: Sakari Ailus
---
drivers/media/platform/omap3isp/isp.c | 206 +--
drivers/media/platform/omap3isp/isp.h | 11 ++
drivers/media/platform/omap3isp/ispcsiphy.c |7 +
3 files changed, 213 insertions(+), 11 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c
index 7804895..2c9bc0d 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -64,6 +64,7 @@
#include
#include
+#include
#include "isp.h"
#include "ispreg.h"
@@ -1991,6 +1992,14 @@ static int isp_register_entities(struct isp_device *isp)
if (ret < 0)
goto done;
+ /*
+* Device Tree --- the external of the sub-devices will be
+* registered later. Same goes for the sub-device node
+* registration.
+*/
+ if (isp->dev->of_node)
+ return 0;
+
/* Register external entities */
for (isp_subdev = pdata ? pdata->subdevs : NULL;
isp_subdev && isp_subdev->board_info; isp_subdev++) {
@@ -2016,8 +2025,10 @@ static int isp_register_entities(struct isp_device *isp)
ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
done:
- if (ret < 0)
+ if (ret < 0) {
isp_unregister_entities(isp);
+ v4l2_async_notifier_unregister(&isp->notifier);
+ }
return ret;
}
@@ -2232,6 +2243,7 @@ static int isp_remove(struct platform_device *pdev)
{
struct isp_device *isp = platform_get_drvdata(pdev);
+ v4l2_async_notifier_unregister(&isp->notifier);
isp_unregister_entities(isp);
isp_cleanup_modules(isp);
isp_xclk_cleanup(isp);
@@ -2243,6 +2255,151 @@ static int isp_remove(struct platform_device *pdev)
return 0;
}
+enum isp_of_phy {
+ ISP_OF_PHY_PARALLEL = 0,
+ ISP_OF_PHY_CSIPHY1,
+ ISP_OF_PHY_CSIPHY2,
+};
+
+static int isp_of_parse_node(struct device *dev, struct device_node *node,
+struct isp_async_subdev *isd)
+{
+ struct isp_bus_cfg *buscfg = &isd->bus;
+ struct v4l2_of_endpoint vep;
+ unsigned int i;
+
+ v4l2_of_parse_endpoint(node, &vep);
+
+ dev_dbg(dev, "interface %u\n", vep.base.port);
+
+ switch (vep.base.port) {
+ case ISP_OF_PHY_PARALLEL:
+ buscfg->interface = ISP_INTERFACE_PARALLEL;
+ buscfg->bus.parallel.data_lane_shift =
+ vep.bus.parallel.data_shift;
+ buscfg->bus.parallel.clk_pol =
+ !!(vep.bus.parallel.flags
+ & V4L2_MBUS_PCLK_SAMPLE_FALLING);
+ buscfg->bus.parallel.hs_pol =
+ !!(vep.bus.parallel.flags & V4L2_MBUS_VSYNC_ACTIVE_LOW);
+ buscfg->bus.parallel.vs_pol =
+ !!(vep.bus.parallel.flags & V4L2_MBUS_HSYNC_ACTIVE_LOW);
+ buscfg->bus.parallel.fld_pol =
+ !!(vep.bus.parallel.flags & V4L2_MBUS_FIELD_EVEN_LOW);
+ buscfg->bus.parallel.data_pol =
+ !!(vep.bus.parallel.flags & V4L2_MBUS_DATA_ACTIVE_LOW);
+ break;
+
+ case ISP_OF_PHY_CSIPHY1:
+ case ISP_OF_PHY_CSIPHY2:
+ /* FIXME: always assume CSI-2 for now. */
+ switch (vep.base.port) {
+ case ISP_OF_PHY_CSIPHY1:
+ buscfg->interface = ISP_INTERFACE_CSI2C_PHY1;
+ break;
+ case ISP_OF_PHY_CSIPHY2:
+ buscfg->interface = ISP_INTERFACE_CSI2A_PHY2;
+ break;
+ }
+ buscfg->bus.csi2.lanecfg.clk.pos = vep.bus.mipi_csi2.clock_lane;
+ buscfg->bus.csi2.lanecfg.clk.pol =
+ vep.bus.mipi_csi2.lane_polarity[0];
+ dev_dbg(dev, "clock lane polarity %u, pos %u\n",
+ buscfg->bus.csi2.lanecfg.clk.pol,
+ buscfg->bus.csi2.lanecfg.clk.pos);
+
+ for (i = 0; i < ISP_CSIPHY2_NUM_DATA_LANES; i++) {
+ buscfg->bus.csi2.lanecfg.data[i].pos =
+ vep.bus.mipi_csi2.data_lanes[i];
+ buscfg->bus.csi2.lanecfg.data[i].pol =
+ vep.bus.mipi_csi2.lane_polarity[i + 1];
+ dev_dbg(dev, "data lane %u polarity %u, pos %u\n", i,
+ buscfg->bus.csi2.lanecfg.data
[RFC 17/18] arm: dts: n950, n9: Add primary camera support
Add support for the primary camera of the Nokia N950 and N9.
Signed-off-by: Sakari Ailus
---
arch/arm/boot/dts/omap3-n9.dts | 39 ++
arch/arm/boot/dts/omap3-n950-n9.dtsi |4
arch/arm/boot/dts/omap3-n950.dts | 39 ++
3 files changed, 78 insertions(+), 4 deletions(-)
diff --git a/arch/arm/boot/dts/omap3-n9.dts b/arch/arm/boot/dts/omap3-n9.dts
index 9938b5d..05f32ae 100644
--- a/arch/arm/boot/dts/omap3-n9.dts
+++ b/arch/arm/boot/dts/omap3-n9.dts
@@ -16,3 +16,42 @@
model = "Nokia N9";
compatible = "nokia,omap3-n9", "ti,omap36xx", "ti,omap3";
};
+
+&i2c2 {
+ clock-frequency = <40>;
+
+ smia_1: camera@10 {
+ compatible = "nokia,smia";
+ reg = <0x10>;
+ /* No reset gpio */
+ vana-supply = <&vaux3>;
+ clocks = <&omap3_isp 0>;
+ clock-frequency = <960>;
+ nokia,nvm-size = <1024>; /* 16 * 64 */
+ link-frequencies = /bits/ 64 <19920 21000 49920>;
+ port {
+ smia_1_1: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2>;
+ remote-endpoint = <&csi2a_ep>;
+ };
+ };
+ };
+};
+
+&omap3_isp {
+ vdd-csiphy1-supply = <&vaux2>;
+ vdd-csiphy2-supply = <&vaux2>;
+ ports {
+ port@2 {
+ reg = <2>;
+ csi2a_ep: endpoint {
+ remote-endpoint = <&smia_1_1>;
+ clock-lanes = <2>;
+ data-lanes = <1 3>;
+ crc = <1>;
+ lane-polarity = <1 1 1>;
+ };
+ };
+ };
+};
diff --git a/arch/arm/boot/dts/omap3-n950-n9.dtsi
b/arch/arm/boot/dts/omap3-n950-n9.dtsi
index c41db94..51e5043 100644
--- a/arch/arm/boot/dts/omap3-n950-n9.dtsi
+++ b/arch/arm/boot/dts/omap3-n950-n9.dtsi
@@ -86,10 +86,6 @@
regulator-max-microvolt = <280>;
};
-&i2c2 {
- clock-frequency = <40>;
-};
-
&i2c3 {
clock-frequency = <40>;
};
diff --git a/arch/arm/boot/dts/omap3-n950.dts b/arch/arm/boot/dts/omap3-n950.dts
index 261c558..2b2ed9c 100644
--- a/arch/arm/boot/dts/omap3-n950.dts
+++ b/arch/arm/boot/dts/omap3-n950.dts
@@ -16,3 +16,42 @@
model = "Nokia N950";
compatible = "nokia,omap3-n950", "ti,omap36xx", "ti,omap3";
};
+
+&i2c2 {
+ clock-frequency = <40>;
+
+ smia_1: camera@10 {
+ compatible = "nokia,smia";
+ reg = <0x10>;
+ /* No reset gpio */
+ vana-supply = <&vaux3>;
+ clocks = <&omap3_isp 0>;
+ clock-frequency = <960>;
+ nokia,nvm-size = <1024>; /* 16 * 64 */
+ link-frequencies = /bits/ 64 <21000 33360 39840>;
+ port {
+ smia_1_1: endpoint {
+ clock-lanes = <0>;
+ data-lanes = <1 2>;
+ remote-endpoint = <&csi2a_ep>;
+ };
+ };
+ };
+};
+
+&omap3_isp {
+ vdd-csiphy1-supply = <&vaux2>;
+ vdd-csiphy2-supply = <&vaux2>;
+ ports {
+ port@2 {
+ reg = <2>;
+ csi2a_ep: endpoint {
+ remote-endpoint = <&smia_1_1>;
+ clock-lanes = <2>;
+ data-lanes = <3 1>;
+ crc = <1>;
+ lane-polarity = <1 1 1>;
+ };
+ };
+ };
+};
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 02/18] omap3isp: Avoid a BUG_ON() in media_entity_create_link()
If an uninitialised v4l2_subdev struct was passed to
media_entity_create_link(), one of the BUG_ON()'s in the function will be
hit since media_entity.num_pads will be zero. Avoid this by checking whether
the num_pads field is non-zero for the interface.
Signed-off-by: Sakari Ailus
---
drivers/media/platform/omap3isp/isp.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c
index fb193b6..4ab674d 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1946,6 +1946,19 @@ static int isp_register_entities(struct isp_device *isp)
goto done;
}
+ /*
+* Not all interfaces are available on all revisions
+* of the ISP. The sub-devices of those interfaces
+* aren't initialised in such a case. Check this by
+* ensuring the num_pads is non-zero.
+*/
+ if (!input->num_pads) {
+ dev_err(isp->dev, "%s: invalid input %u\n",
+ entity->name, subdevs->interface);
+ ret = -EINVAL;
+ goto done;
+ }
+
for (i = 0; i < sensor->entity.num_pads; i++) {
if (sensor->entity.pads[i].flags & MEDIA_PAD_FL_SOURCE)
break;
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 11/18] omap3isp: Replace many MMIO regions by two
The omap3isp MMIO register block is contiguous in the MMIO register space
apart from the fact that the ISP IOMMU register block is in the middle of
the area. Ioremap it at two occasions, and keep the rest of the layout of
the register space internal to the omap3isp driver.
Signed-off-by: Sakari Ailus
---
arch/arm/mach-omap2/devices.c | 66 +--
arch/arm/mach-omap2/omap34xx.h| 36 +--
drivers/media/platform/omap3isp/isp.c | 113 +
drivers/media/platform/omap3isp/isp.h |4 +-
4 files changed, 66 insertions(+), 153 deletions(-)
diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index e945957..990338f 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -74,72 +74,12 @@ omap_postcore_initcall(omap3_l3_init);
static struct resource omap3isp_resources[] = {
{
.start = OMAP3430_ISP_BASE,
- .end= OMAP3430_ISP_END,
+ .end= OMAP3430_ISP_BASE + 0x12fc,
.flags = IORESOURCE_MEM,
},
{
- .start = OMAP3430_ISP_CCP2_BASE,
- .end= OMAP3430_ISP_CCP2_END,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP3430_ISP_CCDC_BASE,
- .end= OMAP3430_ISP_CCDC_END,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP3430_ISP_HIST_BASE,
- .end= OMAP3430_ISP_HIST_END,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP3430_ISP_H3A_BASE,
- .end= OMAP3430_ISP_H3A_END,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP3430_ISP_PREV_BASE,
- .end= OMAP3430_ISP_PREV_END,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP3430_ISP_RESZ_BASE,
- .end= OMAP3430_ISP_RESZ_END,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP3430_ISP_SBL_BASE,
- .end= OMAP3430_ISP_SBL_END,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP3430_ISP_CSI2A_REGS1_BASE,
- .end= OMAP3430_ISP_CSI2A_REGS1_END,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP3430_ISP_CSIPHY2_BASE,
- .end= OMAP3430_ISP_CSIPHY2_END,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP3630_ISP_CSI2A_REGS2_BASE,
- .end= OMAP3630_ISP_CSI2A_REGS2_END,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP3630_ISP_CSI2C_REGS1_BASE,
- .end= OMAP3630_ISP_CSI2C_REGS1_END,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP3630_ISP_CSIPHY1_BASE,
- .end= OMAP3630_ISP_CSIPHY1_END,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP3630_ISP_CSI2C_REGS2_BASE,
- .end= OMAP3630_ISP_CSI2C_REGS2_END,
+ .start = OMAP3430_ISP_BASE2,
+ .end= OMAP3430_ISP_BASE2 + 0x0600,
.flags = IORESOURCE_MEM,
},
{
diff --git a/arch/arm/mach-omap2/omap34xx.h b/arch/arm/mach-omap2/omap34xx.h
index c0d1b4b..ed0024d 100644
--- a/arch/arm/mach-omap2/omap34xx.h
+++ b/arch/arm/mach-omap2/omap34xx.h
@@ -46,39 +46,9 @@
#define OMAP34XX_IC_BASE 0x4820
-#define OMAP3430_ISP_BASE (L4_34XX_BASE + 0xBC000)
-#define OMAP3430_ISP_CBUFF_BASE(OMAP3430_ISP_BASE + 0x0100)
-#define OMAP3430_ISP_CCP2_BASE (OMAP3430_ISP_BASE + 0x0400)
-#define OMAP3430_ISP_CCDC_BASE (OMAP3430_ISP_BASE + 0x0600)
-#define OMAP3430_ISP_HIST_BASE (OMAP3430_ISP_BASE + 0x0A00)
-#define OMAP3430_ISP_H3A_BASE (OMAP3430_ISP_BASE + 0x0C00)
-#define OMAP3430_ISP_PREV_BASE (OMAP3430_ISP_BASE + 0x0E00)
-#define OMAP3430_ISP_RESZ_BASE (OMAP3430_ISP_BASE + 0x1000)
-#define OMAP3430_ISP_SBL_BASE (OMAP3430_ISP_BASE + 0x1200)
-#define OMAP3430_ISP_MMU_BASE (OMAP3430_ISP_BASE + 0x1400)
-#define OMAP3430_ISP_CSI2A_REGS1_BASE (OMAP3430_ISP_BASE + 0x1800)
-#define OMAP3430_ISP_CSIPHY2_BASE (OMAP3430_ISP_BASE + 0x1970)
-#define OMAP3630_ISP_CSI2A_REGS2_BASE (OMAP3430_ISP_BASE + 0x19C0)
-#define OMAP3630_ISP_CSI2C_REGS1_BASE (OMAP3430_ISP_BASE + 0x1C00)
-#define OMAP3630_ISP_CSIPHY1_BASE
[RFC 07/18] omap3isp: Rename regulators to better suit the Device Tree
Rename VDD_CSIPHY1 as vdd-csiphy1 and VDD_CSIPHY2 as vdd-csiphy2. Signed-off-by: Sakari Ailus --- drivers/media/platform/omap3isp/isp.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c index 1b5c6df..c045318 100644 --- a/drivers/media/platform/omap3isp/isp.c +++ b/drivers/media/platform/omap3isp/isp.c @@ -2292,8 +2292,8 @@ static int isp_probe(struct platform_device *pdev) platform_set_drvdata(pdev, isp); /* Regulators */ - isp->isp_csiphy1.vdd = devm_regulator_get(&pdev->dev, "VDD_CSIPHY1"); - isp->isp_csiphy2.vdd = devm_regulator_get(&pdev->dev, "VDD_CSIPHY2"); + isp->isp_csiphy1.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy1"); + isp->isp_csiphy2.vdd = devm_regulator_get(&pdev->dev, "vdd-csiphy2"); /* Clocks * -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 06/18] omap3isp: Refactor device configuration structs for Device Tree
Make omap3isp configuration data structures more suitable for consumption by
the DT by separating the I2C bus information of all the sub-devices in a
group and the ISP bus information from each other. The ISP bus information
is made a pointer instead of being directly embedded in the struct.
In the case of the DT only the sensor specific information on the ISP bus
configuration is retained. The structs are renamed to reflect that.
After this change the structs needed to describe device configuration can be
allocated and accessed separately without those needed only in the case of
platform data. The platform data related structs can be later removed once
the support for platform data can be removed.
Signed-off-by: Sakari Ailus
Cc: Mike Rapoport
Cc: Igor Grinberg
---
arch/arm/mach-omap2/board-cm-t35.c | 57 +++---
drivers/media/platform/omap3isp/isp.c | 86 +--
drivers/media/platform/omap3isp/isp.h |2 +-
drivers/media/platform/omap3isp/ispccdc.c | 26
drivers/media/platform/omap3isp/ispccp2.c | 22 +++
drivers/media/platform/omap3isp/ispcsi2.c |8 +--
drivers/media/platform/omap3isp/ispcsiphy.c | 21 ---
include/media/omap3isp.h| 34 +--
8 files changed, 119 insertions(+), 137 deletions(-)
diff --git a/arch/arm/mach-omap2/board-cm-t35.c
b/arch/arm/mach-omap2/board-cm-t35.c
index 91738a1..b5dfbc1 100644
--- a/arch/arm/mach-omap2/board-cm-t35.c
+++ b/arch/arm/mach-omap2/board-cm-t35.c
@@ -492,51 +492,36 @@ static struct twl4030_platform_data cm_t35_twldata = {
#include
#include "devices.h"
-static struct i2c_board_info cm_t35_isp_i2c_boardinfo[] = {
+static struct isp_platform_subdev cm_t35_isp_subdevs[] = {
{
- I2C_BOARD_INFO("mt9t001", 0x5d),
- },
- {
- I2C_BOARD_INFO("tvp5150", 0x5c),
- },
-};
-
-static struct isp_subdev_i2c_board_info cm_t35_isp_primary_subdevs[] = {
- {
- .board_info = &cm_t35_isp_i2c_boardinfo[0],
- .i2c_adapter_id = 3,
- },
- { NULL, 0, },
-};
-
-static struct isp_subdev_i2c_board_info cm_t35_isp_secondary_subdevs[] = {
- {
- .board_info = &cm_t35_isp_i2c_boardinfo[1],
+ .board_info = &(struct i2c_board_info){
+ I2C_BOARD_INFO("mt9t001", 0x5d)
+ },
.i2c_adapter_id = 3,
- },
- { NULL, 0, },
-};
-
-static struct isp_v4l2_subdevs_group cm_t35_isp_subdevs[] = {
- {
- .subdevs = cm_t35_isp_primary_subdevs,
- .interface = ISP_INTERFACE_PARALLEL,
- .bus = {
- .parallel = {
- .clk_pol = 1,
+ .bus = &(struct isp_bus_cfg){
+ .interface = ISP_INTERFACE_PARALLEL,
+ .bus = {
+ .parallel = {
+ .clk_pol = 1,
+ },
},
},
},
{
- .subdevs = cm_t35_isp_secondary_subdevs,
- .interface = ISP_INTERFACE_PARALLEL,
- .bus = {
- .parallel = {
- .clk_pol = 0,
+ .board_info = &(struct i2c_board_info){
+ I2C_BOARD_INFO("tvp5150", 0x5c),
+ },
+ .i2c_adapter_id = 3,
+ .bus = &(struct isp_bus_cfg){
+ .interface = ISP_INTERFACE_PARALLEL,
+ .bus = {
+ .parallel = {
+ .clk_pol = 0,
+ },
},
},
},
- { NULL, 0, },
+ { 0 },
};
static struct isp_platform_data cm_t35_isp_pdata = {
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c
index b836bc8..1b5c6df 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -447,7 +447,7 @@ static void isp_core_init(struct isp_device *isp, int idle)
*/
void omap3isp_configure_bridge(struct isp_device *isp,
enum ccdc_input_entity input,
- const struct isp_parallel_platform_data *pdata,
+ const struct isp_parallel_cfg *parcfg,
unsigned int shift, unsigned int bridge)
{
u32 ispctrl_val;
@@ -462,8 +462,8 @@ void omap3isp_configure_bridge(struct isp_device *isp,
switch (input) {
case CCDC_INPUT_PARALLEL:
ispctrl_val |= ISPCTRL_PAR_SER_CLK_SEL_PARALLEL;
- ispctrl_val |= pdata->clk_pol << ISPCTRL_PAR_CLK_POL_SHIFT;
-
[RFC 10/18] omap3isp: Move the syscon register out of the ISP register maps
The syscon register isn't part of the ISP, use it through the syscom driver
regmap instead. The syscom block is considered to be from 343x on ISP
revision 2.0 whereas 15.0 is assumed to have 3630 syscon.
Signed-off-by: Sakari Ailus
---
arch/arm/boot/dts/omap3.dtsi|2 +-
arch/arm/mach-omap2/devices.c | 10 --
drivers/media/platform/omap3isp/isp.c | 19 +++
drivers/media/platform/omap3isp/isp.h | 19 +--
drivers/media/platform/omap3isp/ispcsiphy.c | 20 +---
5 files changed, 42 insertions(+), 28 deletions(-)
diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
index 01b7111..fe0b293 100644
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -183,7 +183,7 @@
omap3_scm_general: tisyscon@48002270 {
compatible = "syscon";
- reg = <0x48002270 0x2f0>;
+ reg = <0x48002270 0x2f4>;
};
pbias_regulator: pbias_regulator {
diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index 1afb50d..e945957 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -143,16 +143,6 @@ static struct resource omap3isp_resources[] = {
.flags = IORESOURCE_MEM,
},
{
- .start = OMAP343X_CTRL_BASE + OMAP343X_CONTROL_CSIRXFE,
- .end= OMAP343X_CTRL_BASE + OMAP343X_CONTROL_CSIRXFE
+ 3,
- .flags = IORESOURCE_MEM,
- },
- {
- .start = OMAP343X_CTRL_BASE +
OMAP3630_CONTROL_CAMERA_PHY_CTRL,
- .end= OMAP343X_CTRL_BASE +
OMAP3630_CONTROL_CAMERA_PHY_CTRL + 3,
- .flags = IORESOURCE_MEM,
- },
- {
.start = 24 + OMAP_INTC_START,
.flags = IORESOURCE_IRQ,
}
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c
index 68d7edfc..4ff4bbd 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -51,6 +51,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -94,8 +95,9 @@ static const struct isp_res_mapping isp_res_maps[] = {
1 << OMAP3_ISP_IOMEM_RESZ |
1 << OMAP3_ISP_IOMEM_SBL |
1 << OMAP3_ISP_IOMEM_CSI2A_REGS1 |
- 1 << OMAP3_ISP_IOMEM_CSIPHY2 |
- 1 << OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE,
+ 1 << OMAP3_ISP_IOMEM_CSIPHY2,
+ .syscon_offset = 0xdc,
+ .phy_type = ISP_PHY_TYPE_3430,
},
{
.isp_rev = ISP_REVISION_15_0,
@@ -112,8 +114,9 @@ static const struct isp_res_mapping isp_res_maps[] = {
1 << OMAP3_ISP_IOMEM_CSI2A_REGS2 |
1 << OMAP3_ISP_IOMEM_CSI2C_REGS1 |
1 << OMAP3_ISP_IOMEM_CSIPHY1 |
- 1 << OMAP3_ISP_IOMEM_CSI2C_REGS2 |
- 1 << OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL,
+ 1 << OMAP3_ISP_IOMEM_CSI2C_REGS2,
+ .syscon_offset = 0x2f0,
+ .phy_type = ISP_PHY_TYPE_3630,
},
};
@@ -2352,6 +2355,14 @@ static int isp_probe(struct platform_device *pdev)
}
}
+ isp->syscon = syscon_regmap_lookup_by_pdevname("syscon.0");
+ isp->syscon_offset = isp_res_maps[m].syscon_offset;
+ isp->phy_type = isp_res_maps[m].phy_type;
+ if (IS_ERR(isp->syscon)) {
+ ret = PTR_ERR(isp->syscon);
+ goto error_isp;
+ }
+
/* IOMMU */
ret = isp_attach_iommu(isp);
if (ret < 0) {
diff --git a/drivers/media/platform/omap3isp/isp.h
b/drivers/media/platform/omap3isp/isp.h
index 9535524..03d2129 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -59,8 +59,6 @@ enum isp_mem_resources {
OMAP3_ISP_IOMEM_CSI2C_REGS1,
OMAP3_ISP_IOMEM_CSIPHY1,
OMAP3_ISP_IOMEM_CSI2C_REGS2,
- OMAP3_ISP_IOMEM_343X_CONTROL_CSIRXFE,
- OMAP3_ISP_IOMEM_3630_CONTROL_CAMERA_PHY_CTRL,
OMAP3_ISP_IOMEM_LAST
};
@@ -93,14 +91,25 @@ enum isp_subclk_resource {
/* ISP2P: OMAP 36xx */
#define ISP_REVISION_15_0 0xF0
+#define ISP_PHY_TYPE_3430 0
+#define ISP_PHY_TYPE_3630 1
+
+struct regmap;
+
/*
* struct isp_res_mapping - Map ISP io resources to ISP revision.
* @isp_rev: ISP_REVISION_x_x
* @map: bitmap for enum isp_mem_resources
+ * @syscon_offset: offset of the syscon register for 343x / 3630
+ * (CONTROL_C
[RFC 14/18] dt: bindings: Add bindings for omap3isp
Signed-off-by: Sakari Ailus
---
.../devicetree/bindings/media/ti,omap3isp.txt | 64
MAINTAINERS|1 +
2 files changed, 65 insertions(+)
create mode 100644 Documentation/devicetree/bindings/media/ti,omap3isp.txt
diff --git a/Documentation/devicetree/bindings/media/ti,omap3isp.txt
b/Documentation/devicetree/bindings/media/ti,omap3isp.txt
new file mode 100644
index 000..2059524
--- /dev/null
+++ b/Documentation/devicetree/bindings/media/ti,omap3isp.txt
@@ -0,0 +1,64 @@
+OMAP 3 ISP Device Tree bindings
+===
+
+More documentation on these bindings is available in
+video-interfaces.txt in the same directory.
+
+Required properties
+===
+
+compatible : "ti,omap3-isp"
+reg: a set of two register block physical addresses and
+ lengths
+interrupts : the interrupt number
+iommus : phandle of the IOMMU
+syscon : syscon phandle and register offset
+ti,phy-type: 0 -- 3430; 1 -- 3630
+#clock-cells : Must be 1 --- the ISP provides two external clocks,
+ cam_xclka and cam_xclkb, at indices 0 and 1,
+ respectively. Please find more information on common
+ clock bindings in ../clock/clock-bindings.txt.
+
+Port nodes (optional)
+-
+
+reg: The interface:
+ 0 - parallel (CCDC)
+ 1 - CSIPHY1 -- CSI2C / CCP2B on 3630;
+ CSI1 -- CSIb on 3430
+ 2 - CSIPHY2 -- CSI2A / CCP2B on 3630;
+ CSI2 -- CSIa on 3430
+
+Optional properties
+===
+
+vdd-csiphy1-supply : voltage supply of the CSI-2 PHY 1
+vdd-csiphy2-supply : voltage supply of the CSI-2 PHY 2
+
+Endpoint nodes
+--
+
+lane-polarity : lane polarity (required on CSI-2)
+ 0 -- not inverted; 1 -- inverted
+data-lanes : an array of data lanes from 1 to 3. The length can
+ be either 1 or 2. (required CSI-2)
+clock-lanes: the clock lane (from 1 to 3). (required on CSI-2)
+
+
+Example
+===
+
+ omap3_isp: omap3_isp@480bc000 {
+ compatible = "ti,omap3-isp";
+ reg = <0x480bc000 0x12fc
+ 0x480bd800 0x0600>;
+ interrupts = <24>;
+ iommus = <&mmu_isp>;
+ syscon = <&omap3_scm_general 0x2f0>;
+ ti,phy-type = <1>;
+ #clock-cells = <1>;
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index ddc5a8c..cdeef39 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7079,6 +7079,7 @@ L:linux-me...@vger.kernel.org
S: Maintained
F: drivers/media/platform/omap3isp/
F: drivers/staging/media/omap4iss/
+F: Documentation/devicetree/bindings/media/ti,omap3isp.txt
OMAP USB SUPPORT
M: Felipe Balbi
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 01/18] omap3isp: Fix error handling in probe
The mutex was not destroyed correctly if dma_coerce_mask_and_coherent() failed for some reason. Signed-off-by: Sakari Ailus --- drivers/media/platform/omap3isp/isp.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c index deca809..fb193b6 100644 --- a/drivers/media/platform/omap3isp/isp.c +++ b/drivers/media/platform/omap3isp/isp.c @@ -2252,7 +2252,7 @@ static int isp_probe(struct platform_device *pdev) ret = dma_coerce_mask_and_coherent(isp->dev, DMA_BIT_MASK(32)); if (ret) - return ret; + goto error; platform_set_drvdata(pdev, isp); -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 00/18] Device tree support for omap3isp, N9[50] primary camera
Hi folks, I've had this patchset hanging around for a long, long time, and now it's time to send it out to linux-media. For OMAP 3, first there are a few patches for random and tiny bugfixes and then preparation for DT support (including compile tested rework of cm-t35 board code). The lane-polarity DT binding is added to video-interfaces.txt for this, I believe, is hardly specific to the OMAP 3 ISP. The same property is read by the V4L2 OF parser as well. ISP DT support is added for both OMAP 34xx and 36xx, but it remains tested on 3630 only. The primary camera support is added for both N950 and N9, the latter of which is tested. The differences are quite small between the two in both of the cases, but still testing definitely wouldn't hurt. The secondary camera support is still lacking primarily due to the I2C address conflict: both of the cameras have the same I2C address. It'd also require CCP2 bus support, which is relatively trivial to add. Effects of Philipp Zabel's patch "of: Decrement refcount of previous endpoint in of_graph_get_next_endpoint" aren't yet taken into account in this series. I'll address this in the next set if changes (small I presume) are needed. -- Kind regards, Sakari -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC 09/18] omap3isp: Replace mmio_base_phys array with the histogram block base
Only the histogram sub-block driver uses the physical address. Do not store
it for other sub-blocks.
Signed-off-by: Sakari Ailus
---
drivers/media/platform/omap3isp/isp.c |3 ++-
drivers/media/platform/omap3isp/isp.h |6 +++---
drivers/media/platform/omap3isp/isphist.c |2 +-
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/media/platform/omap3isp/isp.c
b/drivers/media/platform/omap3isp/isp.c
index c045318..68d7edfc 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2247,7 +2247,8 @@ static int isp_map_mem_resource(struct platform_device
*pdev,
if (IS_ERR(isp->mmio_base[res]))
return PTR_ERR(isp->mmio_base[res]);
- isp->mmio_base_phys[res] = mem->start;
+ if (res == OMAP3_ISP_IOMEM_HIST)
+ isp->mmio_hist_base_phys = mem->start;
return 0;
}
diff --git a/drivers/media/platform/omap3isp/isp.h
b/drivers/media/platform/omap3isp/isp.h
index b932a6f..9535524 100644
--- a/drivers/media/platform/omap3isp/isp.h
+++ b/drivers/media/platform/omap3isp/isp.h
@@ -138,8 +138,8 @@ struct isp_xclk {
* @irq_num: Currently used IRQ number.
* @mmio_base: Array with kernel base addresses for ioremapped ISP register
* regions.
- * @mmio_base_phys: Array with physical L4 bus addresses for ISP register
- * regions.
+ * @mmio_hist_base_phys: Physical L4 bus address for ISP hist block register
+ * region.
* @mapping: IOMMU mapping
* @stat_lock: Spinlock for handling statistics
* @isp_mutex: Mutex for serializing requests to ISP.
@@ -175,7 +175,7 @@ struct isp_device {
unsigned int irq_num;
void __iomem *mmio_base[OMAP3_ISP_IOMEM_LAST];
- unsigned long mmio_base_phys[OMAP3_ISP_IOMEM_LAST];
+ unsigned long mmio_hist_base_phys;
struct dma_iommu_mapping *mapping;
diff --git a/drivers/media/platform/omap3isp/isphist.c
b/drivers/media/platform/omap3isp/isphist.c
index ce822c3..3bb9c4f 100644
--- a/drivers/media/platform/omap3isp/isphist.c
+++ b/drivers/media/platform/omap3isp/isphist.c
@@ -70,7 +70,7 @@ static void hist_dma_config(struct ispstat *hist)
hist->dma_config.sync_mode = OMAP_DMA_SYNC_ELEMENT;
hist->dma_config.frame_count = 1;
hist->dma_config.src_amode = OMAP_DMA_AMODE_CONSTANT;
- hist->dma_config.src_start = isp->mmio_base_phys[OMAP3_ISP_IOMEM_HIST]
+ hist->dma_config.src_start = isp->mmio_hist_base_phys
+ ISPHIST_DATA;
hist->dma_config.dst_amode = OMAP_DMA_AMODE_POST_INC;
hist->dma_config.src_or_dst_synch = OMAP_DMA_SRC_SYNC;
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
