On Tue, Oct 16, 2018 at 8:50 PM Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > Hi Tomasz, > > On Fri, Oct 12, 2018 at 05:06:56PM +0900, Tomasz Figa wrote: > > On Fri, Oct 12, 2018 at 4:58 PM Sakari Ailus > > <sakari.ailus@xxxxxxxxxxxxxxx> wrote: > > > > > > Hi Tomasz, > > > > > > On Fri, Oct 12, 2018 at 01:51:10PM +0900, Tomasz Figa wrote: > > > > Hi Sakari, > > > > > > > > On Wed, Sep 26, 2018 at 11:38 AM <bingbu.cao@xxxxxxxxx> wrote: > > > > > > > > > > From: Bingbu Cao <bingbu.cao@xxxxxxxxx> > > > > > > > > > > Add a v4l2 sub-device driver for the Sony imx319 image sensor. > > > > > This is a camera sensor using the i2c bus for control and the > > > > > csi-2 bus for data. > > > > > > > > > > This driver supports following features: > > > > > - manual exposure and analog/digital gain control support > > > > > - vblank/hblank control support > > > > > - 4 test patterns control support > > > > > - vflip/hflip control support (will impact the output bayer order) > > > > > - support following resolutions: > > > > > - 3264x2448, 3280x2464 @ 30fps > > > > > - 1936x1096, 1920x1080 @ 60fps > > > > > - 1640x1232, 1640x922, 1296x736, 1280x720 @ 120fps > > > > > - support 4 bayer orders output (via change v/hflip) > > > > > - SRGGB10(default), SGRBG10, SGBRG10, SBGGR10 > > > > > > > > > > Cc: Tomasz Figa <tfiga@xxxxxxxxxxxx> > > > > > Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > > > > > Signed-off-by: Bingbu Cao <bingbu.cao@xxxxxxxxx> > > > > > Signed-off-by: Tianshu Qiu <tian.shu.qiu@xxxxxxxxx> > > > > > > > > > > --- > > > > > > > > > > This patch is based on sakari's media-tree git: > > > > > https://git.linuxtv.org/sailus/media_tree.git/log/?h=for-4.20-1 > > > > > > > > > > Changes from v5: > > > > > - add some comments for gain calculation > > > > > - use lock to protect the format > > > > > - fix some style issues > > > > > > > > > > Changes from v4 to v5: > > > > > - use single PLL for all internal clocks > > > > > - change link frequency to 482.4MHz > > > > > - adjust frame timing for 2x2 binning modes > > > > > and enlarge frame readout time > > > > > - get CSI-2 link frequencies and external clock > > > > > from firmware > > > > > > > > If I remember correctly, that was suggested by you. Why do we need to > > > > specify link frequency in firmware if it's fully configured by the > > > > driver, with the only external dependency being the external clock? > > > > > > The driver that's now in upstream supports, for now, a very limited set of > > > configurations from what the sensor supports. These are more or less > > > tailored to the particular system where it is being used right now (output > > > image size, external clock frequency, frame rates, link frequencies etc.). > > > > As a side note, they're tailored to exactly the system I mentioned, > > with different link frequency hardcoded in the firmware, coming from > > earlier stage of development. > > > > > If the same sensor is needed elsewhere (quite likely), the configuration > > > needed elsewhere is very likely to be different from what you're using now. > > > > > > The link frequency in particular is important as using a different link > > > frequency (which could be fine elsewhere) could cause EMI issues, e.g. > > > rendering your GPS receiver inoperable during the time the camera sensor is > > > streaming images. > > > > > > Should new configurations be added to this driver to support a different > > > system, the link frequencies used by those configurations may be > > > problematic to your system, and after a software update the driver could as > > > well use those new frequencies. That's a big no-no. > > > > > > > Okay, those are some valid points indeed, thanks for clarifying. > > > > > > > > > > We're having problems with firmware listing the link frequency from v4 > > > > and we can't easily change it anymore to report the new one. I feel > > > > like this dependency on the firmware here is unnecessary, as long as > > > > the external clock frequency matches. > > > > > > This is information you really need to know. > > > > > > A number of older drivers do not use the link frequency information from > > > the firmware but that comes with a risk. Really, it's better to change the > > > frequency now to something you can choose, rather than have it changed > > > later on to something someone else chose for you. > > > > I guess it means that we have to carry a local downstream patch that > > bypasses this check, since we cannot easily change the firmware > > anymore. > > Is there a possibility update the firmware, or carry an SSDT overlay as part > of the software? The options are laid out in > Documentation/acpi/ssdt-overlays.txt . Do remember to pay attention to the > revision field --- also in future Coreboot updates. > Generally we try to avoid updating the firmware in the field, but most of the time there is a reason to do it anyway, so that might eventually happen. Let me try to figure out. > > > > An alternative would be to make the driver try to select a frequency > > that matches what's in the firmware, but issue a warning and fall back > > to a default one if a matching is not found. It might be actually > > better than nothing for some early testing on new systems, since it > > wouldn't require firmware changes. > > You don't need firmware changes per se; see above. Allowing that will very, > very easily lead this being unaddressed during developement and changing > later on inadvertly. Still, requiring the user to create an SSDT overlay sounds like an overkill, for something that is not really a fatal error. We handle ACPI firmware bugs in many parts of the kernel by issuing a warning and using some reasonably safe fallback and I don't know why we couldn't do the same here. Best regards, Tomasz
