From: Vinod Koul <vkoul@kernel.org>
To: Emil Velikov <emil.l.velikov@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@linux.ie>,
Rob Clark <robdclark@gmail.com>,
devicetree <devicetree@vger.kernel.org>,
Jernej Skrabec <jernej.skrabec@siol.net>,
Neil Armstrong <narmstrong@baylibre.com>,
linux-arm-msm <linux-arm-msm@vger.kernel.org>,
Jonas Karlman <jonas@kwiboo.se>,
"Linux-Kernel@Vger. Kernel. Org" <linux-kernel@vger.kernel.org>,
ML dri-devel <dri-devel@lists.freedesktop.org>,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Andrzej Hajda <a.hajda@samsung.com>,
Rob Herring <robh+dt@kernel.org>,
Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge
Date: Thu, 14 May 2020 12:36:50 +0530 [thread overview]
Message-ID: <20200514070650.GO14092@vkoul-mobl> (raw)
In-Reply-To: <CACvgo502+8YroB5QtnGYFeSu92s_vpe_M3cPFeuC77u9xpanXQ@mail.gmail.com>
Hello Emil,
Thanks for the comments.
On 13-05-20, 20:20, Emil Velikov wrote:
> Hi Vinod,
>
> Few high-level comments:
> - handful of functions always return 0 and the return value is never
> checked - switch to return void
Sure makes sense, will do
> - annotate all (nearly) arrays as static const
Will do
> - consistently use multi_reg_write - in some cases non-const array
> will be fine, overwriting a few entries as needed
Okay that makes sense
> - there is very partial comments about the registers/values - missing docs or?
yeah am not a big fan either, problem is documentation.
Well the spec I have doesn't have register names and few registers are
missing :( I have few name created but naming registers turned nasty
super quick.. Do let me know if you have suggestions, I will give it one
more shot though
> Personally I'm in favour of using symbolic names, instead of
> hex+comment. Considering how partial the comments are, current
> approach is perfectly fine.
>
> On Wed, 13 May 2020 at 11:06, Vinod Koul <vkoul@kernel.org> wrote:
> >
> > Lontium Lt9611 is a DSI to HDMI bridge which supports two DSI ports and
> > I2S port as an input and HDMI port as output
> >
> > Co-developed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
>
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/lt9611.c
>
> Please add a vendor prefix to the filename.
Okay
> > +struct lt9611_mode {
> > + u16 hdisplay;
> > + u16 vdisplay;
> > + u8 fps;
> We all enjoy the odd fps game, but let's use vrefresh here.
Sure will change
> > +static int lt9611_mipi_input_digital(struct lt9611 *lt9611,
> > + const struct drm_display_mode *mode)
> > +{
> > + regmap_write(lt9611->regmap, 0x8300, LT9611_4LANES);
> > +
> > + if (mode->hdisplay == 3840)
> > + regmap_write(lt9611->regmap, 0x830a, 0x03);
> > + else
> > + regmap_write(lt9611->regmap, 0x830a, 0x00);
> > +
> > + regmap_write(lt9611->regmap, 0x824f, 0x80);
> > + regmap_write(lt9611->regmap, 0x8250, 0x10);
> > + regmap_write(lt9611->regmap, 0x8302, 0x0a);
> > + regmap_write(lt9611->regmap, 0x8306, 0x0a);
> Create an (non-const) array, overwriting the [1] entry for 3840 mode?
So array is the recommendation, I dont have much liking for them but I
can see they would be useful here, so will change this and other
instances and we can use regmap_multi_reg_write() while taking care of
static const for non modified arrays
>
> > +
> > + return 0;
> Kill return type.
Yup, here and other places
> > + regmap_write(lt9611->regmap, 0x82e3, pclk >> 17); /* pclk[19:16] */
> > + regmap_write(lt9611->regmap, 0x82e4, pclk >> 9); /* pclk[15:8] */
> > + regmap_write(lt9611->regmap, 0x82e5, pclk >> 1); /* pclk[7:0] */
> Comment does not match the code.
> We're discarding the LSB, so we cannot realistically be writing
> pclk[7:0]. Similar applies for the other two.
Thanks for pointing, will fix it up
> > + /* v_act */
> > + ret = regmap_read(lt9611->regmap, 0x8282, &temp);
> > + if (ret)
> > + goto end;
> > +
> > + v_act = temp << 8;
> > + ret = regmap_read(lt9611->regmap, 0x8283, &temp);
> > + if (ret)
> > + goto end;
> > + v_act = v_act + temp;
> > +
> Having a helper for the above "result = read(x) << 8 | read(x+1)"
> would be great.
> This way one doesn't have to repeat the pattern 4-5 times.
will add
> > +static int lt9611_read_edid(struct lt9611 *lt9611)
> > +{
> > + unsigned int temp;
> > + int ret = 0;
> > + int i, j;
> > +
> > + memset(lt9611->edid_buf, 0, EDID_SEG_SIZE);
> How about:
> memset(lt9611->edid_buf, 0, sizeof(lt9611->edid_buf));
>
> Then again, do we need the memset()? We are allocating the memory with
> devm_kzalloc()
Yes but lt9611_read_edid() is called multiple times so would make sense
to memset it, will modify this to sizeof.
> > +
> > + regmap_write(lt9611->regmap, 0x8503, 0xc9);
> > +
> > + /* 0xA0 is EDID device address */
> > + regmap_write(lt9611->regmap, 0x8504, 0xa0);
> > + /* 0x00 is EDID offset address */
> > + regmap_write(lt9611->regmap, 0x8505, 0x00);
> > + /* length for read */
> > + regmap_write(lt9611->regmap, 0x8506, 0x20);
> Is this the same 32 as seen in the loops below? #define and use consistently?
Sure will use defines here and other places
> > + if (block > 1)
> > + return -EINVAL;
> > +
> > + if (block == 0) {
> > + /* always read 2 edid blocks once */
> Please mention why that's a good idea. From memory - there aren't many
> other drivers that do this.
Okay will find the reason for this and
>
> > + ret = lt9611_read_edid(lt9611);
> > + if (ret) {
> > + dev_err(lt9611->dev, "edid read failed\n");
> > + return ret;
> > + }
> > + }
> > +
> > + if (block % 2 == 0)
> > + memcpy(buf, lt9611->edid_buf, len);
> > + else
> > + memcpy(buf, lt9611->edid_buf + 128, len);
> The above can be written as:
> memcpy(buf, lt9611->edid_buf + (block * 128), len);
correct
> > + /* Attach secondary DSI, if specified */
> > + if (lt9611->dsi1_node) {
> > + lt9611->dsi1 = lt9611_attach_dsi(lt9611, lt9611->dsi1_node);
> > + if (IS_ERR(lt9611->dsi1)) {
> > + ret = PTR_ERR(lt9611->dsi1);
> > + goto err_unregister_dsi0;
> > + }
> > + }
> > +
> > + return 0;
> > +
> > +err_unregister_dsi0:
> Missing detach? If possible directly use lt9611_bridge_detach().
will update
> > +static int lt9611_read_device_rev(struct lt9611 *lt9611)
> > +{
> > + unsigned int rev;
> > + int ret;
> > +
> > + regmap_write(lt9611->regmap, 0x80ee, 0x01);
> > + ret = regmap_read(lt9611->regmap, 0x8002, &rev);
> > + if (ret)
> > + dev_err(lt9611->dev, "failed to read revision: %d\n", ret);
> > +
> The "failed" message will be followed by printing random kernel memory.
> Initialize rev to some dummy number or omit the dev_info.
Am printing the 'ret' error code here and not the uninitialized rev, so
I guess this one should be fine
> > + ret = lt9611_parse_dt(&client->dev, lt9611);
> > + if (ret) {
> > + dev_err(dev, "failed to parse device tree\n");
> > + return ret;
> > + }
> > +
> > + ret = lt9611_gpio_init(lt9611);
> > + if (ret < 0)
> Missing of_node_put() here and for the next few error paths.
Yes this should be replaced by jump to of_node_put below
> > +static const struct of_device_id lt9611_match_table[] = {
> > + {.compatible = "lontium,lt9611"},
> In the above two - add space after { and before }. Pretty sure
Correct, will fix this.
> ./scripts/checkpatch.pl will complain about those.
> Might want to double-check for other issues reported by said tool.
Somehow that was not the case :( I always run checkpatch.pl with
--strict option. I have 1 warn about 80 char limit for a error message
which I have ignored :)
--
~Vinod
next prev parent reply other threads:[~2020-05-14 7:07 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-13 10:05 [PATCH 0/3] Add LT9611 DSI to HDMI bridge Vinod Koul
2020-05-13 10:05 ` [PATCH 1/3] dt-bindings: vendor-prefixes: Add Lontium vendor prefix Vinod Koul
2020-05-28 1:34 ` Rob Herring
2020-05-13 10:05 ` [PATCH 2/3] dt-bindings: display: bridge: Add documentation for LT9611 Vinod Koul
2020-05-28 1:37 ` Rob Herring
2020-05-28 1:48 ` Laurent Pinchart
2020-06-04 7:18 ` Vinod Koul
2020-06-04 7:29 ` Laurent Pinchart
2020-05-13 10:05 ` [PATCH 3/3] drm/bridge: Introduce LT9611 DSI to HDMI bridge Vinod Koul
2020-05-13 16:14 ` kbuild test robot
2020-05-13 16:46 ` kbuild test robot
2020-05-13 19:20 ` Emil Velikov
2020-05-14 7:06 ` Vinod Koul [this message]
2020-05-28 1:52 ` Laurent Pinchart
2020-06-04 7:25 ` Vinod Koul
2020-06-04 7:38 ` Laurent Pinchart
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=20200514070650.GO14092@vkoul-mobl \
--to=vkoul@kernel.org \
--cc=Laurent.pinchart@ideasonboard.com \
--cc=a.hajda@samsung.com \
--cc=airlied@linux.ie \
--cc=bjorn.andersson@linaro.org \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=emil.l.velikov@gmail.com \
--cc=jernej.skrabec@siol.net \
--cc=jonas@kwiboo.se \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=narmstrong@baylibre.com \
--cc=robdclark@gmail.com \
--cc=robh+dt@kernel.org \
/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 NNTP newsgroup(s).