| Message ID | 20231207052016.25954-1-zhi.mao@mediatek.com |
|---|---|
| Headers | show |
| Series | media: i2c: Add support for GC08A3 sensor | expand |
On 07/12/2023 06:20, Zhi Mao wrote: > This series adds YAML DT binding and V4L2 sub-device driver for Galaxycore's > GC08A3 8-megapixel 10-bit RAW CMOS 1/4" sensor, with an MIPI CSI-2 image data > interface and the I2C control bus. > > The driver is implemented with V4L2 framework. > - Async registered as a V4L2 sub-device. > - As the first component of camera system including Seninf, ISP pipeline. > - A media entity that provides one source pad in common. > - Used in camera features on ChromeOS application. > > Also this driver supports following features: > - manual exposure and analog gain control support > - vertical blanking control support > - test pattern support > - media controller support > - runtime PM support > - support resolution: 3264x2448@30fps, 1920x1080@60fps > > Previous versions of this patch-set can be found here: > v1: https://lore.kernel.org/linux-media/20231123115104.32094-1-zhi.mao@mediatek.com/ > > Changes of v2 mainly address comments from Krzysztof/Rob Herring&Conor Dooley. > Compared to v1: > - Fix some review comments What exactly fixed? This cannot be that vague! Best regards, Krzysztof
Hi Zhi Mao, I have reviewed your v1 before noticing the v2, sorry about that. Most of the comments I made on v1 are still applicable, so I'll skip v2 and review v3. On Thu, Dec 07, 2023 at 01:20:14PM +0800, Zhi Mao wrote: > This series adds YAML DT binding and V4L2 sub-device driver for Galaxycore's > GC08A3 8-megapixel 10-bit RAW CMOS 1/4" sensor, with an MIPI CSI-2 image data > interface and the I2C control bus. > > The driver is implemented with V4L2 framework. > - Async registered as a V4L2 sub-device. > - As the first component of camera system including Seninf, ISP pipeline. > - A media entity that provides one source pad in common. > - Used in camera features on ChromeOS application. > > Also this driver supports following features: > - manual exposure and analog gain control support > - vertical blanking control support > - test pattern support > - media controller support > - runtime PM support > - support resolution: 3264x2448@30fps, 1920x1080@60fps > > Previous versions of this patch-set can be found here: > v1: https://lore.kernel.org/linux-media/20231123115104.32094-1-zhi.mao@mediatek.com/ > > Changes of v2 mainly address comments from Krzysztof/Rob Herring&Conor Dooley. > Compared to v1: > - Fix some review comments > - Add reviewed-by for sensor driver > - Fix some build-error and warning message > > Thanks > > > Zhi Mao (2): > media: i2c: Add GC08A3 image sensor driver > media: dt-bindings: media: i2c: Document GC08A3 bindings > > .../bindings/media/i2c/galaxycore,gc08a3.yaml | 127 ++ > .../devicetree/bindings/vendor-prefixes.yaml | 2 + > drivers/media/i2c/Kconfig | 14 + > drivers/media/i2c/Makefile | 1 + > drivers/media/i2c/gc08a3.c | 1888 +++++++++++++++++ > 5 files changed, 2032 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/galaxycore,gc08a3.yaml > create mode 100644 drivers/media/i2c/gc08a3.c
On Thu, 2023-12-07 at 17:44 +0000, Conor Dooley wrote: > On Thu, Dec 07, 2023 at 01:30:35PM +0100, Krzysztof Kozlowski wrote: > > On 07/12/2023 12:34, Sakari Ailus wrote: > > > > + ret = gc08a3_parse_fwnode(dev); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + gc08a3 = devm_kzalloc(dev, sizeof(*gc08a3), > > > > GFP_KERNEL); > > > > + if (!gc08a3) > > > > + return -ENOMEM; > > > > + > > > > + gc08a3->dev = dev; > > > > + > > > > + gc08a3->xclk = devm_clk_get(dev, NULL); > > > > + if (IS_ERR(gc08a3->xclk)) > > > > + return dev_err_probe(dev, PTR_ERR(gc08a3- > > > > >xclk), > > > > + "failed to get > > > > xclk\n"); > > > > + > > > > + ret = clk_set_rate(gc08a3->xclk, > > > > GC08A3_DEFAULT_CLK_FREQ); > > > > > > Please see: > > > <URL: > > > https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#devicetree> > > > ;. > > > > > > Oh, that's cool it was documented! > > > > The canonical link would be: > > https://www.kernel.org/doc/html/latest/driver-api/media/camera-sensor.html#devicetree > > I believe this is that answer to the "why are these needed" that I > asked > on the previous version and never got a response to. Instead, they > were > just removed from the binding etc. About "assigned-clocks" & "assigned-clock-rates" in v1 patch, as they are not used in sensor driver, I have removed them in sensor dts- bindind file. And "clock-names" & "clock-frequency" are also the same, they will be removed in next version.
Hi Zhi, On Fri, Dec 08, 2023 at 02:07:36AM +0000, Zhi Mao (毛智) wrote: > On Thu, 2023-12-07 at 17:44 +0000, Conor Dooley wrote: > > On Thu, Dec 07, 2023 at 01:30:35PM +0100, Krzysztof Kozlowski wrote: > > > On 07/12/2023 12:34, Sakari Ailus wrote: > > > > > + ret = gc08a3_parse_fwnode(dev); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + gc08a3 = devm_kzalloc(dev, sizeof(*gc08a3), > > > > > GFP_KERNEL); > > > > > + if (!gc08a3) > > > > > + return -ENOMEM; > > > > > + > > > > > + gc08a3->dev = dev; > > > > > + > > > > > + gc08a3->xclk = devm_clk_get(dev, NULL); > > > > > + if (IS_ERR(gc08a3->xclk)) > > > > > + return dev_err_probe(dev, PTR_ERR(gc08a3- > > > > > >xclk), > > > > > + "failed to get > > > > > xclk\n"); > > > > > + > > > > > + ret = clk_set_rate(gc08a3->xclk, > > > > > GC08A3_DEFAULT_CLK_FREQ); > > > > > > > > Please see: > > > > <URL: > > > > https://hverkuil.home.xs4all.nl/spec/driver-api/camera-sensor.html#devicetree> > > > > ;. > > > > > > > > > Oh, that's cool it was documented! > > > > > > The canonical link would be: > > > > https://www.kernel.org/doc/html/latest/driver-api/media/camera-sensor.html#devicetree > > > > I believe this is that answer to the "why are these needed" that I > > asked > > on the previous version and never got a response to. Instead, they > > were > > just removed from the binding etc. > > About "assigned-clocks" & "assigned-clock-rates" in v1 patch, as they > are not used in sensor driver, I have removed them in sensor dts- > bindind file. And "clock-names" & "clock-frequency" are also the same, > they will be removed in next version. Ack. You should only need "clocks" there, indeed.