On Thu, Feb 14, 2019 at 04:28:33AM -0800, chandanu@xxxxxxxxxxxxxx wrote:
> On 2018-10-23 09:28, Sean Paul wrote:
> > On Wed, Oct 10, 2018 at 10:15:58AM -0700, Chandan Uddaraju wrote:
> > > Add the needed displayPort files to enable DP driver
> > > on msm target.
> > >
> > > "dp_display" module is the main module that calls into
> > > other sub-modules. "dp_drm" file represents the interface
> > > between DRM framework and DP driver.
> > >
> >
>
> Hello Sean,
> I have responded to all of your comments and queries. I have specified the
> comments addressed
> in V2 patch series and what I have missed that will be fixed in V3.
>
>
/snip
> > >
> > > +struct dss_io_data {
> > > + u32 len;
> > > + void __iomem *base;
> > > +};
> >
> > len isn't used anywhere, so this struct can go away. If the struct goes
> > away,
> > you can really simplify (or eliminate msm_dss_ioremap_byname())
> >
>
> We want to keep this since we use len for our internal unit level testing
> and in simulation mode.
>
That's not really a valid reason to keep unused code around. We can't reasonably
expect upstream developers to know which structs and unused fields are used for
Qualcomm's downstream testing, and afaict, those results aren't published so
there's no benefit to the upsteam community.
So you can either opensource the unit tests along with the driver, or keep the
downstream code in the downstream kernel.
/snip
> > > +
> > > +#define dp_catalog_get_priv(x) { \
> >
> > static inline struct dp_catalog_private *dp_catalog_get_priv(...)
> > {
> > ...
> > }
> >
> For inline, we have to use 3 different functions (one each for aux, ctrl and
> panel).
> Looking at "type-checking" w.r.t macro, container_of will through an error
> if its not valid pointer.
> Please let me know if you still want to change this macro to inline func.
>
Yeah, I think it's worth it to have the 3.
> > > + struct dp_catalog *dp_catalog; \
> > > + dp_catalog = container_of(x, struct dp_catalog, x); \
> > > + catalog = container_of(dp_catalog, struct dp_catalog_private, \
> > > + dp_catalog); \
> > > +}
/snip
> >
> > Then again, I'm guessing hitting the NULL case is likely impossible, so
> > you can
> > probably remove all of the machinery around checking !aux.
> >
>
> Removed most of the arg null checks except few APIs where
> unit-testing/simulator-mode is used.
>
There should only be null checks if null is a possibility. If you have
pathelogical unit tests injecting bogus values in your function, you should
probably just fix/delete those unit tests. If upstream doesn't have
visibility into your internal testing, we can't distinguish between
test-pleasing vs unused code.
> > > +static void dp_catalog_ctrl_send_phy_pattern(struct dp_catalog_ctrl
> > > *ctrl,
> > > + u32 pattern)
> > > +{
> > > + struct dp_catalog_private *catalog;
> > > + u32 value = 0x0;
> > > + void __iomem *base = NULL;
> > > +
> > > + if (!ctrl) {
> > > + pr_err("invalid input\n");
> > > + return;
> > > + }
> > > +
> > > + dp_catalog_get_priv(ctrl);
> > > +
> > > + base = catalog->io->dp_link.base;
> > > +
> > > + dp_write(base + DP_STATE_CTRL, 0x0);
> >
> > Do you really need to do this?
> >
> Yes, to make sure no other pattern is set/enabled in hardware before any new
> pattern is configured.
>
> > > +
> > > + switch (pattern) {
> > > + case DP_TEST_PHY_PATTERN_D10_2_NO_SCRAMBLING:
> > > + dp_write(base + DP_STATE_CTRL, 0x1);
> > > + break;
> > > + case DP_TEST_PHY_PATTERN_SYMBOL_ERR_MEASUREMENT_CNT:
> > > + value &= ~(1 << 16);
> > > + dp_write(base + DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value);
> > > + value |= 0xFC;
> > > + dp_write(base + DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value);
> > > + dp_write(base + DP_MAINLINK_LEVELS, 0x2);
> > > + dp_write(base + DP_STATE_CTRL, 0x10);
> > > + break;
> > > + case DP_TEST_PHY_PATTERN_PRBS7:
> > > + dp_write(base + DP_STATE_CTRL, 0x20);
> > > + break;
> > > + case DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN:
> > > + dp_write(base + DP_STATE_CTRL, 0x40);
> > > + /* 00111110000011111000001111100000 */
> > > + dp_write(base + DP_TEST_80BIT_CUSTOM_PATTERN_REG0, 0x3E0F83E0);
> > > + /* 00001111100000111110000011111000 */
> > > + dp_write(base + DP_TEST_80BIT_CUSTOM_PATTERN_REG1, 0x0F83E0F8);
> > > + /* 1111100000111110 */
> > > + dp_write(base + DP_TEST_80BIT_CUSTOM_PATTERN_REG2, 0x0000F83E);
> > > + break;
> > > + case DP_TEST_PHY_PATTERN_HBR2_CTS_EYE_PATTERN:
> > > + value = BIT(16);
> > > + dp_write(base + DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value);
> > > + value |= 0xFC;
> > > + dp_write(base + DP_HBR2_COMPLIANCE_SCRAMBLER_RESET, value);
> > > + dp_write(base + DP_MAINLINK_LEVELS, 0x2);
> > > + dp_write(base + DP_STATE_CTRL, 0x10);
> >
> > This code is the exact same as the case for
> > DP_TEST_PHY_PATTERN_SYMBOL_ERR_MEASUREMENT_CNT, aside from BIT(16) vs
> > ~BIT(16). Can you please combine these cases
> > and share code?
>
> Will fix this in V3.
>
> >
> > > + break;
> > > + default:
> > > + pr_debug("No valid test pattern requested: 0x%x\n", pattern);
> > > + return;
> > > + }
> >
> > All of the DP_STATE_CTRL values should be #defined, and you can move the
> > write
> > down here instead of duplicating it everywhere (with
> > DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN writing a 0).
> >
> All the DP_STATE_CTRL values are defines in V2.
>
> I am not clear on moving the write operations after the switch cases. Can
> you please provide
> some more details?
I think what I meant was that since all cases finish with a write to
DP_STATE_CTRL, you can move that write out of each case and put it at the
bottom. But I think I missed that DP_STATE_CTRL is set at the beginning of
DP_TEST_PHY_PATTERN_80_BIT_CUSTOM_PATTERN, so I don't think that'll work.
/snip
> > > +struct dp_ctrl_private {
> > > + struct dp_ctrl dp_ctrl;
> > > +
> > > + struct device *dev;
> > > + struct dp_aux *aux;
> > > + struct dp_panel *panel;
> > > + struct dp_link *link;
> > > + struct dp_power *power;
> > > + struct dp_parser *parser;
> > > + struct dp_catalog_ctrl *catalog;
> >
> > Once you remove struct dp_ctrl and call the functions directly, I
> > suspect you
> > can get rid of all of these.
> >
> Even though we call the functions directly, we still need variables present
> in each module.
> For example, we need to access link parameters, panel parameters etc
>
> Unnecessary stuff is removed but we still need other variables in the
> structure. All have been
> updated in V2.
Ok, I'll re-evaluate in v3
/snip
> > > +static void dp_ctrl_push_idle(struct dp_ctrl *dp_ctrl)
> > > +{
> > > + int const idle_pattern_completion_timeout_ms = 3 * HZ / 100;
> >
> > This should be #define'd outside of the functioninstead. Also, why the
> > '_ms_'
> > suffix? The units are in jiffies.
> >
> > I think something like
> >
> > #define IDLE_PATTERN_COMPLETION_TIMEOUT_JIFFIES (30 * HZ / 1000) /* 30
> > ms */
> >
> > would be clearer.
> >
> Fixed in V2.
>
> > > + struct dp_ctrl_private *ctrl;
> > > +
> > > + if (!dp_ctrl) {
> > > + pr_err("Invalid input data\n");
> > > + return;
> > > + }
> > > +
> > > + ctrl = container_of(dp_ctrl, struct dp_ctrl_private, dp_ctrl);
> > > +
> > > + reinit_completion(&ctrl->idle_comp);
> >
> > This function is called from a bunch of places, and idle_comp is also
> > used in a
> > bunch of places. Are you certain this is thread-safe if you have
> > concurrent
> > callers or users of idle_comp?
> >
> I don't think we need any synchronization logic for idle_comp since it is
> reinitialized only
> when the complete core is reset. Only place where it is needed is in
> push_idle which already has
> a mutex.
So here are the callsites for push_idle():
- dp_display_clean- called on extcon event thread, unlocked
- dp_display_pre_disable- called on drm thread, locked with modeset locks
- dp_ctrl_link_maintenance- called in hpd handler, unlocked
It _seems_ like there could be races here, and I don't know which mutex you're
referring to above. Mind walking me through it in more detail?
>
> > > + dp_ctrl_state_ctrl(ctrl, ST_PUSH_IDLE);
> > > +
> > > + if (!wait_for_completion_timeout(&ctrl->idle_comp,
> > > + idle_pattern_completion_timeout_ms))
> > > + pr_warn("PUSH_IDLE pattern timedout\n");
> >
> > Return an error?
> >
> Its not fatal error since we are going to reset the controller and we are
> going to recover from
> this timeout.
In that case, can you please downgrade the log level to debug or info?
/snip
> > > +
> > > + if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN)
> >
> > You should really protect sink_request with a lock, it's racy. I'm
> > trying
> > to catch the race conditions while I review, but I'd urge you to audit
> > the
> > driver for other places that need locking.
> >
> sink_request will be running in a single-threaded work queue.
> ("drm_dp_extcon" work queue).
> We might not need locking for sink_request.
>
It seems like sink_request could change in between reads in dp_ctrl_on?
dp_ctrl_on reads the value twice, and calls dp_ctrl_setup_main_link which also
reads the value.
/snip
> > > +static void dp_ctrl_set_clock_rate(struct dp_ctrl_private *ctrl,
> > > + char *name, u32 rate)
> > > +{
> > > + u32 num = ctrl->parser->mp[DP_CTRL_PM].num_clk;
> > > + struct dss_clk *cfg = ctrl->parser->mp[DP_CTRL_PM].clk_config;
> > > +
> > > + while (num && strcmp(cfg->clk_name, name)) {
> > > + num--;
> > > + cfg++;
> > > + }
> > > +
> > > + pr_debug("setting rate=%d on clk=%s\n", rate, name);
> > > +
> > > + if (num)
> > > + cfg->rate = rate;
> > > + else
> > > + pr_err("%s clock could not be set with rate %d\n", name, rate);
> > > +}
> >
> > Instead of doing all of this work, just store pointers to the link and
> > pixel
> > clock when you parse the dt. That way you can just access them directly
> > in
> > dp_ctrl_enable_mainlink_clocks.
> >
> These are generic implementation for DP_CORE and DP_CTRL clocks. This will
> help
> for any future upgrades where the only change will be done dtsi and maybe
> minor
> changes in Driver when we add/remove any clocks.
Just add new pointers and NULL checks in the future if/when this is needed.
/snip
> > > +
> > > + if (ctrl->link->sink_request & DP_TEST_LINK_PHY_TEST_PATTERN)
> >
> > This is racy, sink_request could change between the check above and
> > here.
> >
> Sink_request is accessed in single thread context.
>
Ah, so I mentioned that in the earlier review. I guess I'm not seeing what
prevents that single thread from modifying the value twice? This code will be
run in a different thread, so it really seems like there could be a race here.
/snip
> > > +static void dp_display_send_hpd_event(struct msm_dp *dp_display)
> > > +{
> > > + struct dp_display_private *dp;
> > > + struct drm_connector *connector;
> > > +
> > > + if (!dp_display) {
> > > + pr_err("invalid input\n");
> > > + return;
> > > + }
> > > +
> > > + dp = container_of(dp_display, struct dp_display_private,
> > > dp_display);
> > > + if (!dp) {
> > > + pr_err("invalid params\n");
> > > + return;
> > > + }
> > > +
> > > + connector = dp->dp_display.connector;
> > > + drm_helper_hpd_irq_event(connector->dev);
> > > +}
> > > +
> > > +static int dp_display_send_hpd_notification(struct
> > > dp_display_private *dp,
> > > + bool hpd)
> >
> > This function is called from a few different threads with no locking. It
> > seems
> > like there's good opportunity for is_connected and notification_comp to
> > get out
> > of sync.
> >
> > Additionally, is_connected is set to true before the function returns
> > (and
> > enable has been completed). So is that going to cause problems when we
> > call
> > detect/get_modes where is_connected == true and the hw is not up?
> >
> Will fix this in V3.
>
>
> > Perhaps I'm misreading this function, but it seems like the plan is to
> > block
> > until userspace has been notified and does a full modeset (ie: enable)?
> > If that
> > is the case, we usually don't want to have the hardware lit up until
> > it's
> > actually going to be used. In these cases, we'll wrap get_modes in
> > enable/disable (with proper locking), and wait for modeset to leave it
> > on.
> >
> When HPD goes high, we do dpcd reads to get the sink capabilities and then
> inform the framework so that "enable"
> gets called by drm framework.
>
I'm not sure I follow what you're saying. Is notification_comp meant to
ratelimit/debounce hpd notifications? I don't think it's needed, and if it is,
it would be interesting to know why.
> > > +{
> > > + if ((hpd && dp->dp_display.is_connected) ||
> > > + (!hpd && !dp->dp_display.is_connected)) {
> > > + pr_info("HPD already %s\n", (hpd ? "on" : "off"));
> > > + return 0;
> > > + }
> > > +
> > > + /* reset video pattern flag on disconnect */
> > > + if (!hpd)
> > > + dp->panel->video_test = false;
> > > +
> > > + dp->dp_display.is_connected = hpd;
> > > + reinit_completion(&dp->notification_comp);
> > > + dp_display_send_hpd_event(&dp->dp_display);
> > > +
> > > + if (!wait_for_completion_timeout(&dp->notification_comp, HZ * 2)) {
> > > + pr_warn("%s timeout\n", hpd ? "connect" : "disconnect");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + return 0;
> > > +}
/snip
> > > +/**
> > > + * dp_connector_get_modes - callback to add drm modes via
> > > drm_mode_probed_add()
> > > + * @connector: Pointer to drm connector structure
> > > + * Returns: Number of modes added
> > > + */
> > > +static int dp_connector_get_modes(struct drm_connector *connector)
> > > +{
> > > + int rc = 0;
> > > + struct msm_dp *dp;
> > > + struct dp_display_mode *dp_mode = NULL;
> > > + struct drm_display_mode *m, drm_mode;
> > > + struct dp_connector *dp_conn;
> > > +
> > > + if (!connector)
> > > + return 0;
> > > +
> > > + dp_conn = to_dp_connector(connector);
> > > + dp = dp_conn->dp_display;
> > > +
> > > + dp_mode = kzalloc(sizeof(*dp_mode), GFP_KERNEL);
> > > + if (!dp_mode)
> > > + return 0;
> >
> > When you get rid of dp_display_mode, please use the stack for the new
> > drm_display_mode
> >
> Fixed in V2.
>
> > > +
> > > + /* pluggable case assumes EDID is read when HPD */
> > > + if (dp->is_connected) {
> > > + rc = dp->get_modes(dp, dp_mode);
> >
> > Why is only one mode returned?
> >
> The get_modes() function might return one mode that is stored
> in dp_mode when compliance test is in progress. If not, the
> return value is equal to the total number of modes supported
> by the sink
Ah. I think this would be better to do this work in dp_panel_get_modes and
return 1 from there. Then you can remove all of the special handling from this
function.
>
> > > + if (!rc)
> >
> > This can also return -errno, you should check for that too.
> >
> Fixed in V2.
>
> > > + pr_err("failed to get DP sink modes, rc=%d\n", rc);
> >
> > Why continue if you know you failed?
> >
> > > +
> > > + if (dp_mode->timing.pixel_clk_khz) { /* valid DP mode */
> > > + memset(&drm_mode, 0x0, sizeof(drm_mode));
> > > + convert_to_drm_mode(dp_mode, &drm_mode);
> > > + m = drm_mode_duplicate(connector->dev, &drm_mode);
> > > + if (!m) {
> > > + pr_err("failed to add mode %ux%u\n",
> > > + drm_mode.hdisplay,
> > > + drm_mode.vdisplay);
> > > + kfree(dp_mode);
> > > + return 0;
> > > + }
> > > + m->width_mm = connector->display_info.width_mm;
> > > + m->height_mm = connector->display_info.height_mm;
> >
> > You shouldn't overwrite these.
> >
> Fixed in V2.
>
> > > + drm_mode_probed_add(connector, m);
> > > + }
> > > + } else {
> > > + pr_err("No sink connected\n");
> >
> > This shouldn't be an error
> >
> Fixed in V2.
>
> > > + }
> > > + kfree(dp_mode);
> > > +
> > > + return rc;
> > > +}
/snip
> > > + * @link: Display Port Driver data
> > > + *
> > > + * Parses the DPCD to check if an automated link is requested (Byte
> > > 0x201),
> > > + * and what type of link automation is being requested (Byte 0x218).
> > > + */
> > > +static int dp_link_parse_request(struct dp_link_private *link)
> > > +{
> > > + int ret = 0;
> > > + u8 bp;
> > > + u8 data;
> >
> > Only need 1 of bp/data
> >
> > > + int rlen;
> >
> > ssize_t
> >
> > > + u32 const param_len = 0x1;
> >
> > Not needed, use drm_dp_dpcd_readb()
> >
> > > +
> > > + /**
> > > + * Read the device service IRQ vector (Byte 0x201) to determine
> > > + * whether an automated link has been requested by the sink.
> > > + */
> > > + rlen = drm_dp_dpcd_read(link->aux->drm_aux,
> > > + DP_DEVICE_SERVICE_IRQ_VECTOR, &bp, param_len);
> > > + if (rlen < param_len) {
> > > + pr_err("aux read failed\n");
> >
> > Print error. This goes for other failure messages in this file.
> >
> Addressed all the comments above in this function in V2.
>
> > > + ret = -EINVAL;
> > > + goto end;
> > > + }
> > > +
> > > + data = bp;
> > > +
> > > + pr_debug("device service irq vector = 0x%x\n", data);
> > > +
> > > + if (!(data & DP_AUTOMATED_TEST_REQUEST)) {
> > > + pr_debug("no test requested\n");
> > > + return 0;
> > > + }
> > > +
> > > + /**
> > > + * Read the link request byte (Byte 0x218) to determine what type
> > > + * of automated link has been requested by the sink.
> > > + */
> > > + rlen = drm_dp_dpcd_read(link->aux->drm_aux, DP_TEST_REQUEST,
> > > + &bp, param_len);
> > > + if (rlen < param_len) {
> > > + pr_err("aux read failed\n");
> > > + ret = -EINVAL;
> > > + goto end;
> > > + }
> > > +
> > > + data = bp;
> > > +
> > > + if (!dp_link_is_test_supported(data)) {
> > > + pr_debug("link 0x%x not supported\n", data);
> > > + goto end;
> > > + }
> > > +
> > > + pr_debug("%s (0x%x) requested\n", dp_link_get_test_name(data),
> > > data);
> > > + link->request.test_requested = data;
> >
> > If test_requested is only used in the scope of this function, just use a
> > local
> > and pass it around. If test_requested is used outside the scope of this
> > function, you need locking.
>
> test_requested in only used in dp_link.c. But we use this in dp_debug for
> debugging. We also
> use this in unit testing. This is one of main reason we have it in dp_link
> structure.
>
Ok, same comment regarding unit testing as above. Please change this to a local
in the upstream version.
/snip
> > > + /**
> >
> > /*
> >
> Multi-line comments, we use "/**"
>
This will cause warnings in the htmldocs build. See
https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
/snip
> > > +static void dp_link_send_test_response(struct dp_link *dp_link)
> > > +{
> > > + struct dp_link_private *link = NULL;
> > > + u32 const response_len = 0x1;
> > > +
> > > + if (!dp_link) {
> > > + pr_err("invalid input\n");
> > > + return;
> > > + }
> > > +
> > > + link = container_of(dp_link, struct dp_link_private, dp_link);
> > > +
> > > + drm_dp_dpcd_write(link->aux->drm_aux, DP_TEST_RESPONSE,
> > > + &dp_link->test_response, response_len);
> >
> > I strongly suspect you should have locking protecting test_response.
> >
> Will cross-check whether we need locking and fix it in V3. Just curious, do
> you still think
> we need locking even though we handle this in a single thread?
AFAICT, this is called from the drm thread (via
dp_ctrl_on->dp_ctrl_send_phy_test_pattern), and extcon hpd thread?
/snip
> > > +static u8 get_link_status(const u8
> > > link_status[DP_LINK_STATUS_SIZE], int r)
> > > +{
> > > + return link_status[r - DP_LANE0_1_STATUS];
> >
> > I don't think you're really supposed to reach into the link_status like
> > this.
> > How about adding a new helper to check for DP_LINK_STATUS_UPDATED and
> > one for
> > DP_DOWNSTREAM_PORT_STATUS_CHANGED?
> >
> This function is similar to function in upstream drm_dp_helper.c file.
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3.18/drivers/gpu/drm/drm_dp_helper.c#259
>
Yeah, that's my point, it's something that's done internally to the helper.
Adding a couple helpers to drm_dp_helper will clean up this code and also
benefit others who need to do the same thing (I don't see anyone else checking
those bits yet).
> > > +}
/snip
> > > +static bool dp_link_is_ds_port_status_changed(struct
> > > dp_link_private *link)
> > > +{
> > > + if (get_link_status(link->link_status,
> > > DP_LANE_ALIGN_STATUS_UPDATED) &
> > > + DP_DOWNSTREAM_PORT_STATUS_CHANGED) /* port status changed */
> > > + return true;
> > > +
> > > + if (link->prev_sink_count != link->dp_link.sink_count.count)
> >
> > It seems like this is running in a separate thread from where
> > prev_sink_count is
> > set, potential race here?
> >
> I don't think we might need locking here. All the updates/checks related to
> prev_sink_count are done
> in dp_link_process_request() which will run in single threaded work queue.
>
Hmm, didn't realize it's only set/accessed in dp_link_process_request. Can you
change this to a local var and just pass it via arguments so 1- the code is more
clear, and 2- there's no chance it gets stomped by another thread.
/snip
> > > +static u32 dp_link_get_test_bits_depth(struct dp_link *dp_link, u32
> > > bpp)
> > > +{
> > > + u32 tbd;
> > > +
> > > + /*
> > > + * Few simplistic rules and assumptions made here:
> > > + * 1. Test bit depth is bit depth per color component
> > > + * 2. Assume 3 color components
> > > + */
> > > + switch (bpp) {
> > > + case 18:
> > > + tbd = DP_TEST_BIT_DEPTH_6;
> > > + break;
> > > + case 24:
> > > + tbd = DP_TEST_BIT_DEPTH_8;
> > > + break;
> > > + case 30:
> > > + tbd = DP_TEST_BIT_DEPTH_10;
> > > + break;
> > > + default:
> > > + tbd = DP_TEST_BIT_DEPTH_UNKNOWN;
> > > + break;
> > > + }
> > > +
> > > + if (tbd != DP_TEST_BIT_DEPTH_UNKNOWN)
> > > + tbd = (tbd >> DP_TEST_BIT_DEPTH_SHIFT);
> >
> > AFAICT tbd corresponds to a SoC register value, as opposed to the DPCD
> > value.
> > It's a happy coincidence that they're the same, however I think you
> > should use a
> > separate msm-specific #define instead of the drm_dp_helper values.
> >
> These are standard bpp values and will match to the DPCD values. For the
> same reason
> we are using existing standard macros.
Hmm, Ok. Mind adding a comment then? It's a bit confusing to mix the values.
/snip
> > > + rc = dp_panel_read_edid(dp_panel, connector);
> >
> > Why read the edid so early? Can't you wait until get_modes is called
> > (you'll
> > have to re-read anyways).
> >
> We won't do re-read again when get_modes is called.
>
You really should be, userspace should determine when/whether to limit it. With
that change, you can remove the read from here.
> > > + if (rc) {
> > > + pr_err("panel edid read failed, set failsafe mode\n");
> > > + return rc;
> > > + }
> > > +
> > > + if (panel->aux_cfg_update_done) {
> > > + pr_debug("read DPCD with updated AUX config\n");
> > > + dp_panel_read_dpcd(dp_panel);
> >
> > Check return value. Should you also check the rate/lane_count validity
> > again too?
> >
> Fixed in V2.
>
> > > + panel->aux_cfg_update_done = false;
> >
> > Should you have locking for aux_cfg_update_done?
> >
> panel_read_edid() and aux_cfg_update_done = false is done one after the
> other in this function. I don't think
> we locking for this.
Another good case of not needing to store this in a central place. Just make
this a local in dp_panel_read_sink_caps().
/snip
> > > + mp->vreg_config = devm_kzalloc(&parser->pdev->dev,
> > > + sizeof(struct dss_vreg) * mp->num_vreg, GFP_KERNEL);
> > > + if (!mp->vreg_config) {
> > > + rc = -ENOMEM;
> > > + goto error;
> > > + }
> > > +
> > > + for_each_child_of_node(supply_root_node, supply_node) {
> > > + const char *st = NULL;
> > > + /* vreg-name */
> > > + rc = of_property_read_string(supply_node,
> > > + "qcom,supply-name", &st);
> > > + if (rc) {
> > > + pr_err("error reading name. rc=%d\n",
> > > + rc);
> > > + goto error;
> > > + }
> > > + snprintf(mp->vreg_config[i].vreg_name,
> > > + ARRAY_SIZE((mp->vreg_config[i].vreg_name)), "%s", st);
> >
> > Instead of storing the name just to get a regulator reference later, why
> > don't
> > you just grab the reference now and avoid the string ops?
> >
> Vreg keep changing for each chip. This is more generic implementation that
> avoids doing changes to the code.
> Updating the dtsi-platform specific changes will have access to the vregs.
Changing the code is fun, string operations are not fun. In order to maximize
fun, can we please change these?
More seriously, trying to come up with generic solutions is super tricky and
more often than not, incorrect. The only thing we know for certain about
designing code is that it will eventually be obsolete :)
/snip
> > > +
> > > + if (dp_parser_check_prefix(core_clk, clk_name))
> > > + core_clk_count++;
> > > +
> > > + if (dp_parser_check_prefix(ctrl_clk, clk_name))
> > > + ctrl_clk_count++;
> >
> > Just use "core"/"ctrl" directly here.
> >
> > Have you considered just having 2 clock lists? That would avoid having
> > to do
> > this.
> >
> I didn't get what you meant by two clock lists. We currently have two clock
> list to store them locally.
> You mean having two clock lists in dtsi?
>
Yes, I believe that's what I meant.
/snip
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > > Forum,
> > > a Linux Foundation Collaborative Project
> > >
> > > _______________________________________________
> > > Freedreno mailing list
> > > Freedreno@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/freedreno
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
