From: Andrey Skvortsov <andrej.skvortzov@gmail.com>
To: "Sakari Ailus" <sakari.ailus@linux.intel.com>,
"Mauro Carvalho Chehab" <mchehab@kernel.org>,
"Rob Herring" <robh+dt@kernel.org>,
"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
"Conor Dooley" <conor+dt@kernel.org>,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, "Ondřej Jirman" <megi@xff.cz>,
"Pavel Machek" <pavel@ucw.cz>,
"Arnaud Ferraris" <arnaud.ferraris@collabora.com>
Subject: Re: [PATCH v2 2/2] media: gc2145: implement basic dvp bus support
Date: Sat, 4 May 2024 19:30:47 +0300 [thread overview]
Message-ID: <ZjZit7sDmXJXDg56@skv.local> (raw)
In-Reply-To: <20240426163826.GA3095395@gnbcxd0016.gnb.st.com>
Hi Alain,
thanks for the review.
On 24-04-26 18:38, Alain Volmat wrote:
> Hi Andrey,
>
> > --- a/drivers/media/i2c/gc2145.c
> > +++ b/drivers/media/i2c/gc2145.c
> > @@ -39,6 +39,10 @@
> > #define GC2145_REG_ANALOG_MODE1 CCI_REG8(0x17)
> > #define GC2145_REG_OUTPUT_FMT CCI_REG8(0x84)
> > #define GC2145_REG_SYNC_MODE CCI_REG8(0x86)
> > +#define GC2145_SYNC_MODE_VSYNC_POL BIT(0)
> > +#define GC2145_SYNC_MODE_HSYNC_POL BIT(1)
> > +#define GC2145_SYNC_MODE_OPCLK_POL BIT(2)
> > +#define GC2145_SYNC_MODE_OPCLK_GATE BIT(3)
>
> OPCLK_GATE added but never used.
All near bits were described. I decided to put description for this bit
as well, so all bits 0-5 are described in source code.
>
> > #define GC2145_SYNC_MODE_COL_SWITCH BIT(4)
> > #define GC2145_SYNC_MODE_ROW_SWITCH BIT(5)
> > #define GC2145_REG_BYPASS_MODE CCI_REG8(0x89)
> > @@ -53,6 +57,12 @@
> > #define GC2145_REG_GLOBAL_GAIN CCI_REG8(0xb0)
> > #define GC2145_REG_CHIP_ID CCI_REG16(0xf0)
> > #define GC2145_REG_PAD_IO CCI_REG8(0xf2)
> > +#define GC2145_REG_PLL_MODE1 CCI_REG8(0xf7)
> > +#define GC2145_REG_PLL_MODE2 CCI_REG8(0xf8)
> > +#define GC2145_REG_CM_MODE CCI_REG8(0xf9)
> > +#define GC2145_REG_CLK_DIV_MODE CCI_REG8(0xfa)
> > +#define GC2145_REG_ANALOG_PWC CCI_REG8(0xfc)
>
> All 5 define added but never used, those settings are part of the cci_sequences
> table. Maybe either keep the define and update the tables or drop the
> new define ?
> > +#define GC2145_REG_PAD_IO CCI_REG8(0xf2)
>
> Was already defined in the existing code, see above.
Thanks, I'll drop all unneeded defines in v3.
> > @@ -773,6 +784,38 @@ static int gc2145_set_pad_format(struct v4l2_subdev *sd,
> > return 0;
> > }
> >
> > +static int gc2145_config_dvp_mode(struct gc2145 *gc2145,
> > + const struct gc2145_format *gc2145_format)
>
> alignment ?
Thanks.
> > +{
> > + int ret = 0;
> > + u64 sync_mode;
> > + int flags;
> > +
> > + flags = gc2145->ep.bus.parallel.flags;
>
> int flags = gc2145->ep.bus.parallel.flags;
> ?
Fix in v3.
> > @@ -1244,36 +1292,57 @@ static int gc2145_check_hwcfg(struct device *dev)
> > return -EINVAL;
> > }
> >
> > - ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &ep_cfg);
> > +
>
> no new line here.
ok.
> > + gc2145->ep.bus_type = V4L2_MBUS_CSI2_DPHY;
> > + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &gc2145->ep);
> > + if (ret) {
> > + gc2145->ep.bus_type = V4L2_MBUS_PARALLEL;
> > + ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &gc2145->ep);
> > + }
> > fwnode_handle_put(endpoint);
> > - if (ret)
> > + if (ret) {
> > + dev_err(dev, "could not parse endpoint\n");
> > 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;
> > }
> >
> > - /* 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");
> > + 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;
> > + }
>
> any reason for change of the if order ? before num_data_lanes was
> checked first, then nr_of_link_frequencies, then 3 link_frequencies.
No reason. I'll change order like it was before in v3.
> > +
> > + /* 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) {
>
> alignment
Thanks.
>
> > + dev_err(dev, "Invalid link-frequencies provided\n");
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > + break;
> > +
>
> no newline here
ok
--
Best regards,
Andrey Skvortsov
prev parent reply other threads:[~2024-05-04 16:30 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-22 6:22 [PATCH v2 0/2] media: gc2145: add basic dvp bus support Andrey Skvortsov
2024-02-22 6:22 ` [PATCH v2 1/2] dt-bindings: media: i2c: add galaxycore,gc2145 DVP " Andrey Skvortsov
2024-02-22 6:22 ` [PATCH v2 2/2] media: gc2145: implement basic dvp " Andrey Skvortsov
2024-04-26 16:38 ` Alain Volmat
2024-05-04 16:30 ` Andrey Skvortsov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZjZit7sDmXJXDg56@skv.local \
--to=andrej.skvortzov@gmail.com \
--cc=arnaud.ferraris@collabora.com \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=megi@xff.cz \
--cc=pavel@ucw.cz \
--cc=robh+dt@kernel.org \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).