| Message ID | 4E400280.7070100@samsung.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Hi Sylwester, On Monday 08 August 2011 17:36:32 Sylwester Nawrocki wrote: > Hi everyone, > > Nowadays many of the V4L2 camera device drivers heavily depend on the board > code to set up voltage supplies, clocks, and some control signals, like > 'reset' and 'standby' signals for the sensors. Those things are often > being done by means of the driver specific platform data callbacks. > > There has been recently quite a lot effort on ARM towards migration to the > device tree. Unfortunately the custom platform data callbacks effectively > prevent the boards to be booted and configured through the device tree > bindings. > > The following is usually handled in the board files: > > 1) sensor/frontend power supply > 2) sensor's master clock (provided by the host device) > 3) reset and standby signals (GPIO) > 4) other signals applied by the host processor to the sensor device, e.g. > I2C level shifter enable, etc. > > For 1), the regulator API should possibly be used. It should be applicable > for most, if not all cases. > 3) and 4) are a bit hard as there might be huge differences across boards > as how many GPIOs are used, what are the required delays between changes > of their states, etc. Still we could try to find a common description of > the behaviour and pass such information to the drivers. > > For 2) I'd like to propose adding a callback to struct v4l2_device, for > instance as in the below patch. The host driver would implement such an > operation and the sensor subdev driver would use it in its s_power op. What about using a struct clk object ? There has been lots of work in the ARM tree to make struct clk generic. I'm not sure if all patches have been pushed to mainline yet, but I think that's the direction we should follow. > If there is more than one output clock at the host, to distinguish which > clock applies to a given sensor the host driver could be passed the > assignment information in it's platform data. > > AFAICS in the omap3isp case the clock control is done through the board > code. I wonder what prevents making direct calls between the drivers, > as the sensor subdevs call a board code callback there, which in turn only > calls into the omap3isp driver. > What am I missing ? We go through board code to remove dependencies between drivers. My goal is to implement this through struct clk, but I haven't had time to work on that yet. > In order to support fully DT based builds it would be desired to change > that method so the board specific callbacks in platform data structures > are avoided. > > > diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h > index d61febf..08b6699 100644 > --- a/include/media/v4l2-device.h > +++ b/include/media/v4l2-device.h > @@ -36,6 +36,15 @@ > > struct v4l2_ctrl_handler; > > +struct v4l2_device_ops { > + /* notify callback called by some sub-devices */ > + void (*notify)(struct v4l2_subdev *sd, > + unsigned int notification, void *arg); > + /* clock control callback */ > + void (*set_clock)(struct v4l2_subdev *sd, > + u_long *frequency, bool enable); > +}; > + > struct v4l2_device { > /* dev->driver_data points to this struct. > Note: dev might be NULL if there is no parent device > @@ -51,9 +60,8 @@ struct v4l2_device { > spinlock_t lock; > /* unique device name, by default the driver name + bus ID */ > char name[V4L2_DEVICE_NAME_SIZE]; > - /* notify callback called by some sub-devices. */ > - void (*notify)(struct v4l2_subdev *sd, > - unsigned int notification, void *arg); > + /* ops for sub-devices */ > + struct v4l2_device_ops ops; > /* The control handler. May be NULL. */ > struct v4l2_ctrl_handler *ctrl_handler; > /* Device's priority state */
Hi Laurent On Mon, 8 Aug 2011, Laurent Pinchart wrote: > Hi Sylwester, > > On Monday 08 August 2011 17:36:32 Sylwester Nawrocki wrote: > > Hi everyone, > > > > Nowadays many of the V4L2 camera device drivers heavily depend on the board > > code to set up voltage supplies, clocks, and some control signals, like > > 'reset' and 'standby' signals for the sensors. Those things are often > > being done by means of the driver specific platform data callbacks. > > > > There has been recently quite a lot effort on ARM towards migration to the > > device tree. Unfortunately the custom platform data callbacks effectively > > prevent the boards to be booted and configured through the device tree > > bindings. > > > > The following is usually handled in the board files: > > > > 1) sensor/frontend power supply > > 2) sensor's master clock (provided by the host device) > > 3) reset and standby signals (GPIO) > > 4) other signals applied by the host processor to the sensor device, e.g. > > I2C level shifter enable, etc. > > > > For 1), the regulator API should possibly be used. It should be applicable > > for most, if not all cases. > > 3) and 4) are a bit hard as there might be huge differences across boards > > as how many GPIOs are used, what are the required delays between changes > > of their states, etc. Still we could try to find a common description of > > the behaviour and pass such information to the drivers. > > > > For 2) I'd like to propose adding a callback to struct v4l2_device, for > > instance as in the below patch. The host driver would implement such an > > operation and the sensor subdev driver would use it in its s_power op. > > What about using a struct clk object ? There has been lots of work in the ARM > tree to make struct clk generic. I'm not sure if all patches have been pushed > to mainline yet, but I think that's the direction we should follow. > > > If there is more than one output clock at the host, to distinguish which > > clock applies to a given sensor the host driver could be passed the > > assignment information in it's platform data. > > > > AFAICS in the omap3isp case the clock control is done through the board > > code. I wonder what prevents making direct calls between the drivers, > > as the sensor subdevs call a board code callback there, which in turn only > > calls into the omap3isp driver. > > What am I missing ? > > We go through board code to remove dependencies between drivers. My goal is to > implement this through struct clk, but I haven't had time to work on that yet. Is this now a different topic from what I have proposed in mbus configuration RFC initially or have you changed your opinion? Thanks Guennadi > > In order to support fully DT based builds it would be desired to change > > that method so the board specific callbacks in platform data structures > > are avoided. > > > > > > diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h > > index d61febf..08b6699 100644 > > --- a/include/media/v4l2-device.h > > +++ b/include/media/v4l2-device.h > > @@ -36,6 +36,15 @@ > > > > struct v4l2_ctrl_handler; > > > > +struct v4l2_device_ops { > > + /* notify callback called by some sub-devices */ > > + void (*notify)(struct v4l2_subdev *sd, > > + unsigned int notification, void *arg); > > + /* clock control callback */ > > + void (*set_clock)(struct v4l2_subdev *sd, > > + u_long *frequency, bool enable); > > +}; > > + > > struct v4l2_device { > > /* dev->driver_data points to this struct. > > Note: dev might be NULL if there is no parent device > > @@ -51,9 +60,8 @@ struct v4l2_device { > > spinlock_t lock; > > /* unique device name, by default the driver name + bus ID */ > > char name[V4L2_DEVICE_NAME_SIZE]; > > - /* notify callback called by some sub-devices. */ > > - void (*notify)(struct v4l2_subdev *sd, > > - unsigned int notification, void *arg); > > + /* ops for sub-devices */ > > + struct v4l2_device_ops ops; > > /* The control handler. May be NULL. */ > > struct v4l2_ctrl_handler *ctrl_handler; > > /* Device's priority state */ > > -- > Regards, > > Laurent Pinchart > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, On Monday 08 August 2011 19:29:13 Guennadi Liakhovetski wrote: > On Mon, 8 Aug 2011, Laurent Pinchart wrote: > > On Monday 08 August 2011 17:36:32 Sylwester Nawrocki wrote: > > > Hi everyone, > > > > > > Nowadays many of the V4L2 camera device drivers heavily depend on the > > > board code to set up voltage supplies, clocks, and some control > > > signals, like 'reset' and 'standby' signals for the sensors. Those > > > things are often being done by means of the driver specific platform > > > data callbacks. > > > > > > There has been recently quite a lot effort on ARM towards migration to > > > the device tree. Unfortunately the custom platform data callbacks > > > effectively prevent the boards to be booted and configured through the > > > device tree bindings. > > > > > > The following is usually handled in the board files: > > > > > > 1) sensor/frontend power supply > > > 2) sensor's master clock (provided by the host device) > > > 3) reset and standby signals (GPIO) > > > 4) other signals applied by the host processor to the sensor device, > > > e.g. > > > > > > I2C level shifter enable, etc. > > > > > > For 1), the regulator API should possibly be used. It should be > > > applicable for most, if not all cases. > > > 3) and 4) are a bit hard as there might be huge differences across > > > boards as how many GPIOs are used, what are the required delays > > > between changes of their states, etc. Still we could try to find a > > > common description of the behaviour and pass such information to the > > > drivers. > > > > > > For 2) I'd like to propose adding a callback to struct v4l2_device, for > > > instance as in the below patch. The host driver would implement such an > > > operation and the sensor subdev driver would use it in its s_power op. > > > > What about using a struct clk object ? There has been lots of work in the > > ARM tree to make struct clk generic. I'm not sure if all patches have > > been pushed to mainline yet, but I think that's the direction we should > > follow. > > > > > If there is more than one output clock at the host, to distinguish > > > which clock applies to a given sensor the host driver could be passed > > > the assignment information in it's platform data. > > > > > > AFAICS in the omap3isp case the clock control is done through the board > > > code. I wonder what prevents making direct calls between the drivers, > > > as the sensor subdevs call a board code callback there, which in turn > > > only calls into the omap3isp driver. > > > What am I missing ? > > > > We go through board code to remove dependencies between drivers. My goal > > is to implement this through struct clk, but I haven't had time to work > > on that yet. > > Is this now a different topic from what I have proposed in mbus > configuration RFC initially or have you changed your opinion? It's a different topic. Your mbus configuration RFC was dealing with the bus clock, while this is dealing with the sensor (or other subdev) input clock.
Hi Laurent, On 08/08/2011 05:44 PM, Laurent Pinchart wrote: > Hi Sylwester, > > On Monday 08 August 2011 17:36:32 Sylwester Nawrocki wrote: >> Hi everyone, >> >> Nowadays many of the V4L2 camera device drivers heavily depend on the board >> code to set up voltage supplies, clocks, and some control signals, like >> 'reset' and 'standby' signals for the sensors. Those things are often >> being done by means of the driver specific platform data callbacks. >> >> There has been recently quite a lot effort on ARM towards migration to the >> device tree. Unfortunately the custom platform data callbacks effectively >> prevent the boards to be booted and configured through the device tree >> bindings. >> >> The following is usually handled in the board files: >> >> 1) sensor/frontend power supply >> 2) sensor's master clock (provided by the host device) >> 3) reset and standby signals (GPIO) >> 4) other signals applied by the host processor to the sensor device, e.g. >> I2C level shifter enable, etc. >> >> For 1), the regulator API should possibly be used. It should be applicable >> for most, if not all cases. >> 3) and 4) are a bit hard as there might be huge differences across boards >> as how many GPIOs are used, what are the required delays between changes >> of their states, etc. Still we could try to find a common description of >> the behaviour and pass such information to the drivers. >> >> For 2) I'd like to propose adding a callback to struct v4l2_device, for >> instance as in the below patch. The host driver would implement such an >> operation and the sensor subdev driver would use it in its s_power op. > > What about using a struct clk object ? There has been lots of work in the ARM > tree to make struct clk generic. I'm not sure if all patches have been pushed > to mainline yet, but I think that's the direction we should follow. But is the 'struct clk' tried to be unified across all archs, not only ARM ? I'm afraid it's not the case. By "using a struct clk object" do you also mean implementing some/all ops of this object by the driver which exports it ? I suppose we can't rely only on the clock controller functionality exposed through the clock API. Some devices may need to be brought to an active state before the clock can be used outside. Some may have internal frequency dividers which need to be handled in addition to the clock path in the clock controller. For instance, on Exynos4 the FIMC devices belong to a power domain that needs to be enabled so the clock is not blocked, and this is done through the runtime PM calls. Normally the host device driver runtime resumes the device when /dev/video* is opened. But we might want to use the clock before it happens, when only a /dev/v4l-subdev* is opened, to play with the sensor device only. In this situation the host device needs to be runtime resumed first. Thus the driver would need to (re)implement the clock ops to also handle the details which are not covered by the clock controller driver. I also wonder how could we support the boards which choose to use some extra external oscillator to provide clock to the sensors, rather than the one derived from the host. Thanks,
Hi Sylwester, On Tuesday 09 August 2011 10:10:40 Sylwester Nawrocki wrote: > On 08/08/2011 05:44 PM, Laurent Pinchart wrote: > > On Monday 08 August 2011 17:36:32 Sylwester Nawrocki wrote: > >> Hi everyone, > >> > >> Nowadays many of the V4L2 camera device drivers heavily depend on the > >> board code to set up voltage supplies, clocks, and some control > >> signals, like 'reset' and 'standby' signals for the sensors. Those > >> things are often being done by means of the driver specific platform > >> data callbacks. > >> > >> There has been recently quite a lot effort on ARM towards migration to > >> the device tree. Unfortunately the custom platform data callbacks > >> effectively prevent the boards to be booted and configured through the > >> device tree bindings. > >> > >> The following is usually handled in the board files: > >> > >> 1) sensor/frontend power supply > >> 2) sensor's master clock (provided by the host device) > >> 3) reset and standby signals (GPIO) > >> 4) other signals applied by the host processor to the sensor device, > >> e.g. > >> > >> I2C level shifter enable, etc. > >> > >> For 1), the regulator API should possibly be used. It should be > >> applicable for most, if not all cases. > >> 3) and 4) are a bit hard as there might be huge differences across > >> boards as how many GPIOs are used, what are the required delays between > >> changes of their states, etc. Still we could try to find a common > >> description of the behaviour and pass such information to the drivers. > >> > >> For 2) I'd like to propose adding a callback to struct v4l2_device, for > >> instance as in the below patch. The host driver would implement such an > >> operation and the sensor subdev driver would use it in its s_power op. > > > > What about using a struct clk object ? There has been lots of work in the > > ARM tree to make struct clk generic. I'm not sure if all patches have > > been pushed to mainline yet, but I think that's the direction we should > > follow. > > But is the 'struct clk' tried to be unified across all archs, not only ARM > ? I'm afraid it's not the case. If the goals haven't changed since https://lkml.org/lkml/2011/5/20/85, the new struct clk will be unified across all architectures. > By "using a struct clk object" do you also mean implementing some/all ops > of this object by the driver which exports it ? That's correct. > I suppose we can't rely only on the clock controller functionality exposed > through the clock API. > > Some devices may need to be brought to an active state before the clock can > be used outside. Some may have internal frequency dividers which need to be > handled in addition to the clock path in the clock controller. > > For instance, on Exynos4 the FIMC devices belong to a power domain that > needs to be enabled so the clock is not blocked, and this is done through > the runtime PM calls. > > Normally the host device driver runtime resumes the device when /dev/video* > is opened. But we might want to use the clock before it happens, when only > a /dev/v4l-subdev* is opened, to play with the sensor device only. In this > situation the host device needs to be runtime resumed first. > > Thus the driver would need to (re)implement the clock ops to also handle > the details which are not covered by the clock controller driver. The subdev driver would call clk_get(), which would end up being implemented by the driver for whatever hardware block provides the clock. The driver would then runtime_pm_resume() the hardware to start the clock. > I also wonder how could we support the boards which choose to use some > extra external oscillator to provide clock to the sensors, rather than the > one derived from the host. In that case the clock is always running. I'm not sure if we should create a dummy clk object, or just pass a NULL clock and a fixed frequency to the sensor driver.
Hi Laurent, On 08/09/2011 01:49 PM, Laurent Pinchart wrote: ... >>>> The following is usually handled in the board files: >>>> >>>> 1) sensor/frontend power supply >>>> 2) sensor's master clock (provided by the host device) ... >>>> For 2) I'd like to propose adding a callback to struct v4l2_device, for >>>> instance as in the below patch. The host driver would implement such an >>>> operation and the sensor subdev driver would use it in its s_power op. >>> >>> What about using a struct clk object ? There has been lots of work in the >>> ARM tree to make struct clk generic. I'm not sure if all patches have >>> been pushed to mainline yet, but I think that's the direction we should >>> follow. >> >> But is the 'struct clk' tried to be unified across all archs, not only ARM >> ? I'm afraid it's not the case. > > If the goals haven't changed since https://lkml.org/lkml/2011/5/20/85, the new > struct clk will be unified across all architectures. OK, it sounds good then. Except the uncertainty of when actually the agreement is achieved and the platforms are finally converted. Looks like there might still be much time needed auntil the clk API based approach is applicable. However I'm not denying we should be adopting it. > >> By "using a struct clk object" do you also mean implementing some/all ops >> of this object by the driver which exports it ? > > That's correct. > >> I suppose we can't rely only on the clock controller functionality exposed >> through the clock API. >> >> Some devices may need to be brought to an active state before the clock can >> be used outside. Some may have internal frequency dividers which need to be >> handled in addition to the clock path in the clock controller. >> >> For instance, on Exynos4 the FIMC devices belong to a power domain that >> needs to be enabled so the clock is not blocked, and this is done through >> the runtime PM calls. >> >> Normally the host device driver runtime resumes the device when /dev/video* >> is opened. But we might want to use the clock before it happens, when only >> a /dev/v4l-subdev* is opened, to play with the sensor device only. In this >> situation the host device needs to be runtime resumed first. >> >> Thus the driver would need to (re)implement the clock ops to also handle >> the details which are not covered by the clock controller driver. > > The subdev driver would call clk_get(), which would end up being implemented > by the driver for whatever hardware block provides the clock. The driver would > then runtime_pm_resume() the hardware to start the clock. Yup, makes sense. > >> I also wonder how could we support the boards which choose to use some >> extra external oscillator to provide clock to the sensors, rather than the >> one derived from the host. > > In that case the clock is always running. I'm not sure if we should create a > dummy clk object, or just pass a NULL clock and a fixed frequency to the > sensor driver. Good point, a dummy (or not really) clock might be a good idea. Especially that it might need to be created anyway. I think, what we need, is a way to describe which clock goes where in terms of DT bindings. Of course the clock names are not now being passed in DT. -- Regards, Sylwester Nawrocki -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/media/v4l2-device.h b/include/media/v4l2-device.h index d61febf..08b6699 100644 --- a/include/media/v4l2-device.h +++ b/include/media/v4l2-device.h @@ -36,6 +36,15 @@ struct v4l2_ctrl_handler; +struct v4l2_device_ops { + /* notify callback called by some sub-devices */ + void (*notify)(struct v4l2_subdev *sd, + unsigned int notification, void *arg); + /* clock control callback */ + void (*set_clock)(struct v4l2_subdev *sd, + u_long *frequency, bool enable); +}; + struct v4l2_device { /* dev->driver_data points to this struct. Note: dev might be NULL if there is no parent device @@ -51,9 +60,8 @@ struct v4l2_device { spinlock_t lock; /* unique device name, by default the driver name + bus ID */ char name[V4L2_DEVICE_NAME_SIZE]; - /* notify callback called by some sub-devices. */ - void (*notify)(struct v4l2_subdev *sd, - unsigned int notification, void *arg); + /* ops for sub-devices */ + struct v4l2_device_ops ops; /* The control handler. May be NULL. */ struct v4l2_ctrl_handler *ctrl_handler; /* Device's priority state */