Messages in this thread | | | Subject | Re: [RFC PATCH v11 6/9] media: tegra: Add Tegra210 Video input driver | From | Dmitry Osipenko <> | Date | Thu, 30 Apr 2020 22:47:47 +0300 |
| |
30.04.2020 22:32, Sowjanya Komatineni пишет: > > On 4/30/20 6:38 AM, Dmitry Osipenko wrote: >> 30.04.2020 01:00, Sowjanya Komatineni пишет: >>> +/** >>> + * struct tegra_csi_ops - Tegra CSI operations >>> + * >>> + * @csi_streaming: programs csi hardware to enable or disable >>> streaming. >>> + * @csi_err_recover: csi hardware block recovery in case of any >>> capture errors >>> + * due to missing source stream or due to improper csi input >>> from >>> + * the external source. >>> + */ >>> +struct tegra_csi_ops { >>> + int (*csi_streaming)(struct tegra_csi_channel *csi_chan, u8 >>> pg_mode, >>> + int enable); >> What about to split csi_streaming() into csi_start_streaming() / >> csi_stop_streaming()? >> >> This will make tegra_csi_ops to be consistent with the tegra_ve_ops. A >> separated start/stop operations are somewhat more natural to have in >> general. > > vi ops is for vb2_ops which has separate start/stop so has seperate > start/stop for vi ops. > > csi is subdev and csi ops is for v4l2_subdev_ops which as s_stream > callback only. > > So, created single stream function for csi to match same as subdev_ops.
It will be nicer to have separate ops for CSI, regardless of the subdev_ops. It should be okay to have a single-combined ops if CSI start/stop was trivial, but it's not the case here.
For example, the pm_runtime_put() shouldn't be invoked if stream's stopping fails. The stopping can't fail for the current code, but this could change in the future.
You could make csi_streaming to return void, telling explicitly that this code can't fail. Then the combined OPS should be okay to have.
| |