Hi Michael On Wed, 19 Apr 2023 at 12:25, Michael Riesch <michael.riesch@xxxxxxxxxxxxxx> wrote: > > Hi Sakari, > > On 4/19/23 11:01, Sakari Ailus wrote: > > Hi Michael, > > > > On Mon, Apr 17, 2023 at 02:38:20PM +0200, Michael Riesch wrote: > >> Hi Sakari, > >> > >> On 4/12/23 17:12, Sakari Ailus wrote: > >>> Hi Dave, Michael, > >>> > >>> On Wed, Apr 12, 2023 at 02:55:56PM +0100, Dave Stevenson wrote: > >>>>>> If the ranges aren't updated, where should that out-of-range lens > >>>>>> movement leave the lens? > >>>>> > >>>>> This is up to the hardware controller, but I would guess it typically > >>>>> stops one step before disaster. Wherever that may be, the error > >>>>> condition and the current position can be read out via this new STATUS > >>>>> control. > >>>>> > >>>>> Does this sound good so far? > >>>> > >>>> Sounds reasonable, but I'm not the gatekeeper (that would be Sakari or > >>>> Laurent), and I'm just expressing my views based on the lenses I've > >>>> encountered. > >>>> All of my lenses have a single drive for focus, a single drive for > >>>> zoom, and where there are multiple elements they are all connected > >>>> mechanically. Your setup sounds far more complex and is likely to need > >>>> a more extensive driver, but it'd be nice to not unnecessarily > >>>> overcomplicate the interface. > >>> > >>> Could we also have a driver that uses these new controls? > >> > >> If you are referring to the driver for our custom lens controller, then > >> I have to say that it is under development and simply not ready for > >> release yet. Also, the decision has not yet been made whether or not > >> this will be an open-source driver. > >> > >> A different approach could be the adaptation of the vimc-lens driver, > >> which currently only supports FOCUS_ABSOLUTE. But this would raise > >> several implementation questions and at least for me this would be a > >> nontrivial task. > >> > >> Is it required to have a driver for this interface (in the sense that > >> the patches cannot be accepted otherwise)? > > > > That has been traditionally required, and a virtual driver isn't usually > > considered enough. There are at least two reasons for this. The first one > > being that if the driver isn't reviewable and targetting upstream it may be > > difficult to figure out whether the interface changes are the right ones > > for that driver. This is perhaps a lesser concern here. Secondly, there is > > also unwillingness to add interface elements that might never be supported > > by the kernel itself --- this is effectively just dead code. > > > > Also cc Hans and Laurent. > > I understand your concerns. Cc: Alexander and Dieter > > We aim to be an open-source friendly company. If you are OK with us > submitting a driver that targets very custom hardware that is only > available in integrated form in our products (and not, for instance, > available for sale as a standalone device), then we are prepared to > submit the driver sources for consideration for inclusion in mainline > Linux. Would this be acceptable? My plan with the motor drive is far simpler with a Pi RP2040 microcontroller on I2C running an ADC for the potentiometers, PWM for motor control, and a PID loop driving it. The MCU code will be open source. It is a spare time project rather than work, so I can't guarantee timescales, but I'll see if I can find some time to progress it. (It could all be driven from the kernel with ADC and PWM, but I don't see such a driver being accepted. Offloading it to an MCU seems to be the easier option). > As I already stated above, it will take us some time to prepare > everything in a form that is suitable for submission. Now should I > submit the next iteration(s) of the series at hand as RFC or as regular > patch series? > > >>> The controls themselves appear reasonable to me as well. I guess there are > >>> changes to be made based on the discussion? > >> > >> I'd summarize that whether or not the status controls are compound > >> controls of the type V4L2_CTRL_TYPE_LENS_STATUS is the open question. > >> > >> As a potential follow-up question I recently asked myself if the struct > >> v4l2_ctrl_lens_status should contain trailing reserved bytes for future > >> extension (no idea, though, what this could be). > >> > >> Alternatively, we could come up with "V4L2_CID_FOCUS_CURRENT (integer)" > >> for the current position and "V4L2_CID_FOCUS_STATUS (bitmask)" (and add > >> further controls when they are needed. Here, we lose atomicity but maybe > >> this can be ignored. One could assume that all relevant controls are > >> read out with a single ioctl which provides at least some level of > >> atomicity. VIDIOC_G_EXT_CTRLS should allow you to read multiple controls in one ioctl call. It would be multiple calls to your g_volatile_ctrl handler, so potentially multiple I2C calls to your lens controller. There is the option of cluster controls, but I think that is largely only applicable for setting controls rather than reading them. Dave > > There might be something that could be done in the control framework to > > address this. But it's not something that can be expected to happen soon. > > > > I'd perhaps keep them separate, not to make it a compound control just for > > the access reason. But I certainly don't have a strong opinion about it. > > After some further considerations, and following Dave's and your > comments, I'll keep them separate. > > Discussion to be continued with v2. > > Best regards, > Michael > > > > >> > >> Any comments and/or recommendations to this open question would be much > >> appreciated. > >> > >> Other review comments will be incorporated in the next iteration of this > >> series as well, but they are quite straightforward. > >
