linux-kernel - Re: [PATCH v2 07/13] media: i2c: imx214: Check number of lanes from device tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <76960282bb92e1827890712ed01e543803ceb992.camel@apitzsch.eu>
Date: Wed, 30 Oct 2024 22:17:27 +0100
From: André Apitzsch <git@...tzsch.eu>
To: Ricardo Ribalda Delgado <ribalda@...nel.org>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, Sakari Ailus	
 <sakari.ailus@...ux.intel.com>, ~postmarketos/upstreaming@...ts.sr.ht, 
	phone-devel@...r.kernel.org, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Dave Stevenson
 <dave.stevenson@...pberrypi.com>
Subject: Re: [PATCH v2 07/13] media: i2c: imx214: Check number of lanes from
 device tree

Hi Ricardo,

Am Mittwoch, dem 30.10.2024 um 12:38 +0100 schrieb Ricardo Ribalda
Delgado:
> On Mon, Oct 21, 2024 at 12:14 AM André Apitzsch via B4 Relay
> <devnull+git.apitzsch.eu@...nel.org> wrote:
> > 
> > From: André Apitzsch <git@...tzsch.eu>
> > 
> > The imx214 camera is capable of either two-lane or four-lane
> > operation.
> > 
> > Currently only the four-lane mode is supported, as proper pixel
> > rates
> > and link frequences for the two-lane mode are unknown.
> > 
> > Signed-off-by: André Apitzsch <git@...tzsch.eu>
> > ---
> >  drivers/media/i2c/imx214.c | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx214.c
> > b/drivers/media/i2c/imx214.c
> > index
> > 0c83149bcc3e3b833a087d26104eb7dfaafdf904..497baad616ad7374a92a3da2b
> > 7c1096b1d72a0c7 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -199,7 +199,6 @@ struct imx214 {
> > 
> >  /*From imx214_mode_tbls.h*/
> >  static const struct cci_reg_sequence mode_4096x2304[] = {
> > -       { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE },
> >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> >         { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH
> > },
> >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > @@ -272,7 +271,6 @@ static const struct cci_reg_sequence
> > mode_4096x2304[] = {
> >  };
> > 
> >  static const struct cci_reg_sequence mode_1920x1080[] = {
> > -       { IMX214_REG_CSI_LANE_MODE, IMX214_CSI_4_LANE_MODE },
> >         { IMX214_REG_HDR_MODE, IMX214_HDR_MODE_OFF },
> >         { IMX214_REG_HDR_RES_REDUCTION, IMX214_HDR_RES_REDU_THROUGH
> > },
> >         { IMX214_REG_EXPOSURE_RATIO, 1 },
> > @@ -791,6 +789,13 @@ static int imx214_start_streaming(struct
> > imx214 *imx214)
> >                 return ret;
> >         }
> > 
> > +       ret = cci_write(imx214->regmap, IMX214_REG_CSI_LANE_MODE,
> > +                       IMX214_CSI_4_LANE_MODE, NULL);
> > +       if (ret) {
> > +               dev_err(imx214->dev, "%s failed to configure
> > lanes\n", __func__);
> > +               return ret;
> > +       }
> > +
> >         ret = cci_multi_reg_write(imx214->regmap, imx214->cur_mode-
> > >reg_table,
> >                                   imx214->cur_mode->num_of_regs,
> > NULL);
> >         if (ret < 0) {
> > @@ -932,7 +937,7 @@ static int imx214_get_regulators(struct device
> > *dev, struct imx214 *imx214)
> >                                        imx214->supplies);
> >  }
> > 
> > -static int imx214_parse_fwnode(struct device *dev)
> > +static int imx214_parse_fwnode(struct device *dev, struct imx214
> > *imx214)
> We don't seem to use imx214 in the function. You probably do not want
> to add this change.
> >  {
> >         struct fwnode_handle *endpoint;
> >         struct v4l2_fwnode_endpoint bus_cfg = {
> > @@ -951,6 +956,13 @@ static int imx214_parse_fwnode(struct device
> > *dev)
> >                 goto done;
> >         }
> > 
> > +       /* Check the number of MIPI CSI2 data lanes */
> > +       if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > +               dev_err_probe(dev, -EINVAL,
> > +                             "only 4 data lanes are currently
> > supported\n");
> > +               goto done;
> > +       }
> > +
> >         for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> >                 if (bus_cfg.link_frequencies[i] ==
> > IMX214_DEFAULT_LINK_FREQ)
> >                         break;
> > @@ -975,14 +987,14 @@ static int imx214_probe(struct i2c_client
> > *client)
> >         struct imx214 *imx214;
> >         int ret;
> > 
> > -       ret = imx214_parse_fwnode(dev);
> > -       if (ret)
> > -               return ret;
> > -
> >         imx214 = devm_kzalloc(dev, sizeof(*imx214), GFP_KERNEL);
> >         if (!imx214)
> >                 return -ENOMEM;
> > 
> > +       ret = imx214_parse_fwnode(dev, imx214);
> > +       if (ret)
> > +               return ret;
> I  am not against changing the order... but the commit message does
> not mention it.
> 

I'm not sure how to argue why the order should be changed, now that the
imx214 argument is gone. I'll restore the original order. It can be
undone, when actually needed.

Best regards,
André

> > +
> >         imx214->dev = dev;
> > 
> >         imx214->xclk = devm_clk_get(dev, NULL);
> > 
> > --
> > 2.47.0
> > 
> >