Hi Sakari,
On 24-02-20 10:55, Sakari Ailus wrote:
> Hi Andrey,
>
> On Sun, Feb 18, 2024 at 01:03:08AM +0300, Andrey Skvortsov wrote:
> > Tested on PinePhone with libcamera-based GNOME screenshot.
> >
> > Signed-off-by: Andrey Skvortsov <andrej.skvortzov@xxxxxxxxx>
> > ---
> > drivers/media/i2c/gc2145.c | 117 ++++++++++++++++++++++++++++---------
> > 1 file changed, 90 insertions(+), 27 deletions(-)
> >
> >
> > @@ -612,6 +623,11 @@ struct gc2145 {
> > const struct gc2145_mode *mode;
> > };
> >
> > +static inline bool gc2145_is_csi2(const struct gc2145 *gc2145)
> > +{
> > + return gc2145->ep.bus_type == V4L2_MBUS_CSI2_DPHY;
>
> This is used in a single place. Could you move this comparison there?
Yes, I'll do.
>
> > +}
> > +
> > }
> > @@ -924,6 +975,9 @@ static void gc2145_stop_streaming(struct gc2145 *gc2145)
> > GC2145_CSI2_MODE_EN | GC2145_CSI2_MODE_MIPI_EN, 0,
> > &ret);
> > cci_write(gc2145->regmap, GC2145_REG_PAGE_SELECT, 0x00, &ret);
> > +
> > + /* Disable dvp streaming */
> > + cci_write(gc2145->regmap, GC2145_REG_PAD_IO, 0x00, &ret);
> > if (ret)
> > dev_err(&client->dev, "%s failed to write regs\n", __func__);
> >
> > @@ -1233,9 +1287,8 @@ static int gc2145_init_controls(struct gc2145 *gc2145)
> > static int gc2145_check_hwcfg(struct device *dev)
> > {
> > struct fwnode_handle *endpoint;
> > - struct v4l2_fwnode_endpoint ep_cfg = {
> > - .bus_type = V4L2_MBUS_CSI2_DPHY
> > - };
>
> First try D-PHY and if that fails, then try PARALLEL.
>
> > + struct v4l2_subdev *sd = dev_get_drvdata(dev);
> > + struct gc2145 *gc2145 = to_gc2145(sd);
> > int ret;
> >
> > endpoint = fwnode_graph_get_next_endpoint(dev_fwnode(dev), NULL);
> > @@ -1244,36 +1297,46 @@ static int gc2145_check_hwcfg(struct device *dev)
> > return -EINVAL;
> > }
> >
> > - ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
> > + ret = v4l2_fwnode_endpoint_parse(endpoint, &gc2145->ep);
>
> You won't have any link frequencies available with this change.
Thanks for catching this up. I'll fix that.
>
> > fwnode_handle_put(endpoint);
> > if (ret)
> > return ret;
> >
> > - /* Check the number of MIPI CSI2 data lanes */
> > - if (ep_cfg.bus.mipi_csi2.num_data_lanes != 2) {
> > - dev_err(dev, "only 2 data lanes are currently supported\n");
> > - ret = -EINVAL;
> > - goto out;
> > - }
> > + switch (gc2145->ep.bus_type) {
> > + case V4L2_MBUS_CSI2_DPHY:
> > + /* Check the link frequency set in device tree */
> > + if (!gc2145->ep.nr_of_link_frequencies) {
> > + dev_err(dev, "link-frequencies property not found in DT\n");
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + /* Check the number of MIPI CSI2 data lanes */
> > + if (gc2145->ep.bus.mipi_csi2.num_data_lanes != 2) {
> > + dev_err(dev, "only 2 data lanes are currently supported\n");
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + if (gc2145->ep.nr_of_link_frequencies != 3 ||
> > + gc2145->ep.link_frequencies[0] != GC2145_640_480_LINKFREQ ||
> > + gc2145->ep.link_frequencies[1] != GC2145_1280_720_LINKFREQ ||
> > + gc2145->ep.link_frequencies[2] != GC2145_1600_1200_LINKFREQ) {
> > + dev_err(dev, "Invalid link-frequencies provided\n");
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > + break;
> >
> > - /* Check the link frequency set in device tree */
> > - if (!ep_cfg.nr_of_link_frequencies) {
> > - dev_err(dev, "link-frequency property not found in DT\n");
> > + case V4L2_MBUS_PARALLEL:
> > + break;
> > + default:
> > + dev_err(dev, "unsupported bus type %u\n",
> > + gc2145->ep.bus_type);
>
> Fits on the previous line.
I'll change this in v2.
--
Best regards,
Andrey Skvortsov
