Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
Hello again after easter,
I have looked little bit more at sn65* driver and its application to
have better background.
I miss only info what panel do you have, how it is enabled/power controlled.
W dniu 01.04.2021 o 16:57, Doug Anderson pisze:
> Hi,
>
> On Thu, Apr 1, 2021 at 4:12 AM Andrzej Hajda wrote:
>>
>> W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
>>> Hi,
>>>
>>> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda wrote:
>>>> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
>>>>> eDP panels won't provide their EDID unless they're powered on. Let's
>>>>> chain a power-on before we read the EDID. This roughly matches what
>>>>> was done in 'parade-ps8640.c'.
>>>>>
>>>>> NOTE: The old code attempted to call pm_runtime_get_sync() before
>>>>> reading the EDID. While that was enough to power the bridge chip on,
>>>>> it wasn't enough to talk to the panel for two reasons:
>>>>> 1. Since we never ran the bridge chip's pre-enable then we never set
>>>>> the bit to ignore HPD. This meant the bridge chip didn't even _try_
>>>>> to go out on the bus and communicate with the panel.
>>>>> 2. Even if we fixed things to ignore HPD, the EDID still wouldn't read
>>>>> if the panel wasn't on.
>>>>>
>>>>> One thing that's a bit odd here is taking advantage of the EDID that
>>>>> the core might have cached for us. See the patch ("drm/edid: Use the
>>>>> cached EDID in drm_get_edid() if eDP"). We manage to get at the cache
>>>>> by:
>>>>> - Instantly failing aux transfers if we're not powered.
>>>>> - If the first read of the EDID fails we try again after powering.
>>>>>
>>>>> Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC")
>>>>> Signed-off-by: Douglas Anderson
>>>>> ---
>>>>> Depending on what people think of the other patches in this series,
>>>>> some of this could change.
>>>>> - If everyone loves the "runtime PM" in the panel driver then we
>>>>> could, in theory, put the pre-enable chaining straight in the "aux
>>>>> transfer" function.
>>>>> - If everyone hates the EDID cache moving to the core then we can
>>>>> avoid some of the awkward flow of things and keep the EDID cache in
>>>>> the sn65dsi86 driver.
>>>> I wonder if this shouldn't be solved in the core - ie caller of
>>>> get_modes callback should be responsible for powering up the pipeline,
>>>> otherwise we need to repeat this stuff in every bridge/panel driver.
>>>>
>>>> Any thoughts?
>>> Yeah, I did look at this a little bit. Presumably it would only make
>>> sense to do it for eDP connections since:
>>>
>>> a) The concept of reading an EDID doesn't make sense for things like MIPI.
>> I guess you mean MIPI DSI
> Yes, sorry! I'll try to be more clear.
>
>
>> and yes I agree, more generally it usually(!)
>> doesn't make sense for any setup with fixed display panel.
>>
>> On the other hand there are DSI/HDMI or DSI/DP adapters which usually
>> have EDID reading logic.
>>
>> And the concept makes sense for most connectors accepting external
>> displays: HDMI, DP, MHL, VGA...
> So, actually, IMO the concept doesn't make sense for anything with an
> external connector. Here's the logic for a handful of connectors:
>
> 1. MIPI DSI: there is no EDID so this doesn't make sense.
>
> 2. An external connector (HDMI, DP, etc): the display that's plugged
> in is externally powered so doesn't need us to power it up to read the
> EDID. By definition, when the HPD signal is asserted then it's OK to
> read the EDID and we don't even know if a display is plugged in until
> HPD is asserted. Thus no special power sequencing is needed to read
> the EDID. (Yes, we need to make sure that the eDP controller itself
> is powered, but that doesn't seem like it's the core's business).
Not true IMO, even if external device is powered on, you must enable
EDID-reader logic.
I guess it is not uncommon to have different power states for EDID
reading and bridge/panel pre-enablement (especially in embedded world).
In fact there are setups where EDID-reader is totally different device
than the bridge itself, and these devices should be
powered/enabled/operational only for time of EDID reading.
>
> 3. eDP: this is where it matters. This is because:
>
&g
Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
W dniu 31.03.2021 o 16:48, Doug Anderson pisze:
> Hi,
>
> On Wed, Mar 31, 2021 at 4:08 AM Andrzej Hajda wrote:
>>
>> W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
>>> eDP panels won't provide their EDID unless they're powered on. Let's
>>> chain a power-on before we read the EDID. This roughly matches what
>>> was done in 'parade-ps8640.c'.
>>>
>>> NOTE: The old code attempted to call pm_runtime_get_sync() before
>>> reading the EDID. While that was enough to power the bridge chip on,
>>> it wasn't enough to talk to the panel for two reasons:
>>> 1. Since we never ran the bridge chip's pre-enable then we never set
>>> the bit to ignore HPD. This meant the bridge chip didn't even _try_
>>> to go out on the bus and communicate with the panel.
>>> 2. Even if we fixed things to ignore HPD, the EDID still wouldn't read
>>> if the panel wasn't on.
>>>
>>> One thing that's a bit odd here is taking advantage of the EDID that
>>> the core might have cached for us. See the patch ("drm/edid: Use the
>>> cached EDID in drm_get_edid() if eDP"). We manage to get at the cache
>>> by:
>>> - Instantly failing aux transfers if we're not powered.
>>> - If the first read of the EDID fails we try again after powering.
>>>
>>> Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC")
>>> Signed-off-by: Douglas Anderson
>>> ---
>>> Depending on what people think of the other patches in this series,
>>> some of this could change.
>>> - If everyone loves the "runtime PM" in the panel driver then we
>>> could, in theory, put the pre-enable chaining straight in the "aux
>>> transfer" function.
>>> - If everyone hates the EDID cache moving to the core then we can
>>> avoid some of the awkward flow of things and keep the EDID cache in
>>> the sn65dsi86 driver.
>>
>> I wonder if this shouldn't be solved in the core - ie caller of
>> get_modes callback should be responsible for powering up the pipeline,
>> otherwise we need to repeat this stuff in every bridge/panel driver.
>>
>> Any thoughts?
> Yeah, I did look at this a little bit. Presumably it would only make
> sense to do it for eDP connections since:
>
> a) The concept of reading an EDID doesn't make sense for things like MIPI.
I guess you mean MIPI DSI, and yes I agree, more generally it usually(!)
doesn't make sense for any setup with fixed display panel.
On the other hand there are DSI/HDMI or DSI/DP adapters which usually
have EDID reading logic.
And the concept makes sense for most connectors accepting external
displays: HDMI, DP, MHL, VGA...
>
> b) For something with an external connector (DP and HDMI) you don't
> even know they're inserted unless the EDID is ready to read (these
> devices are, essentially, always powered).
Usually there are two elements which are not the same:
1. HotPlug signal/wire.
2. EDID reading logic.
The logic responsible for reading EDID needs to be enabled only for time
required for EDID reading :) So it's power state often must be
controlled explicitly by the bridge driver. So even if in many cases
pre_enable powers on the logic for EDID reading it does not make it the
rule, so I must step back from my claim that it is up to caller :)
>
> So I started off trying to do this in the core for eDP, but then it
> wasn't completely clear how to write this code in a way that was super
> generic. Specifically:
>
> 1. I don't think it's a 100% guarantee that everything is powered on
> in pre-enable and powered off in post-disable. In this bridge chip
> it's true, but maybe not every eDP driver? Would you want me to just
> assume this, or add a flag?
Ok, pre_enable should power on the chip, but for performing
initialization of video transport layer. Assumption it will power on
EDID logic is incorrect, so my claim seems wrong, but also this patch
looks incorrect :)
In general only device containing EDID logic knows how to power it up.
Since I do not know your particular case I can propose few possible ways
to investigate:
- call bridge.next->get_modes - you leave responsibility for powering up
to the downstream device.
- ddc driver on i2c request should power up the panel - seems also correct,
Regards
Andrzej
>
> 2. It wasn't totally clear to me which state to use for telling if the
> bridge had already been pre-enabled so I could avoid double-calling
> it. I could dig more if need be but I spent a bit of time looking and
> was coming up empty. If you have advice I'd appreciate it, though.
>
> 3. It wasn't clear to me if I should
Re: [PATCH v2 11/14] drm/bridge: ti-sn65dsi86: Power things properly for reading the EDID
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> eDP panels won't provide their EDID unless they're powered on. Let's
> chain a power-on before we read the EDID. This roughly matches what
> was done in 'parade-ps8640.c'.
>
> NOTE: The old code attempted to call pm_runtime_get_sync() before
> reading the EDID. While that was enough to power the bridge chip on,
> it wasn't enough to talk to the panel for two reasons:
> 1. Since we never ran the bridge chip's pre-enable then we never set
> the bit to ignore HPD. This meant the bridge chip didn't even _try_
> to go out on the bus and communicate with the panel.
> 2. Even if we fixed things to ignore HPD, the EDID still wouldn't read
> if the panel wasn't on.
>
> One thing that's a bit odd here is taking advantage of the EDID that
> the core might have cached for us. See the patch ("drm/edid: Use the
> cached EDID in drm_get_edid() if eDP"). We manage to get at the cache
> by:
> - Instantly failing aux transfers if we're not powered.
> - If the first read of the EDID fails we try again after powering.
>
> Fixes: 58074b08c04a ("drm/bridge: ti-sn65dsi86: Read EDID blob over DDC")
> Signed-off-by: Douglas Anderson
> ---
> Depending on what people think of the other patches in this series,
> some of this could change.
> - If everyone loves the "runtime PM" in the panel driver then we
>could, in theory, put the pre-enable chaining straight in the "aux
>transfer" function.
> - If everyone hates the EDID cache moving to the core then we can
>avoid some of the awkward flow of things and keep the EDID cache in
>the sn65dsi86 driver.
I wonder if this shouldn't be solved in the core - ie caller of
get_modes callback should be responsible for powering up the pipeline,
otherwise we need to repeat this stuff in every bridge/panel driver.
Any thoughts?
Regards
Andrzej
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 39 +--
> 1 file changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c0398daaa4a6..673c9f1c2d8e 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -128,6 +128,7 @@
>* @dp_lanes: Count of dp_lanes we're using.
>* @ln_assign:Value to program to the LN_ASSIGN register.
>* @ln_polrs: Value for the 4-bit LN_POLRS field of SN_ENH_FRAME_REG.
> + * @pre_enabled: If true then pre_enable() has run.
>*
>* @gchip:If we expose our GPIOs, this is used.
>* @gchip_output: A cache of whether we've set GPIOs to output. This
> @@ -155,6 +156,7 @@ struct ti_sn_bridge {
> int dp_lanes;
> u8 ln_assign;
> u8 ln_polrs;
> + boolpre_enabled;
>
> #if defined(CONFIG_OF_GPIO)
> struct gpio_chipgchip;
> @@ -268,11 +270,33 @@ static int ti_sn_bridge_connector_get_modes(struct
> drm_connector *connector)
> {
> struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> struct edid *edid;
> + bool was_enabled;
> int num = 0;
>
> - pm_runtime_get_sync(pdata->dev);
> + /*
> + * Try to get the EDID first without anything special. There are
> + * three things that could happen with this call.
> + * a) It might just return from its cache.
> + * b) It might try to initiate an AUX transfer which might work.
> + * c) It might try to initiate an AUX transfer which might fail because
> + *we're not powered up.
> + *
> + * If we get a failure we'll assume case c) and try again. NOTE: we
> + * don't want to power up every time because that's slow and we don't
> + * have visibility into whether the data has already been cached.
> + */
> edid = drm_get_edid(connector, >aux.ddc);
> - pm_runtime_put(pdata->dev);
> + if (!edid) {
> + was_enabled = pdata->pre_enabled;
> +
> + if (!was_enabled)
> + drm_bridge_chain_pre_enable(>bridge);
> +
> + edid = drm_get_edid(connector, >aux.ddc);
> +
> + if (!was_enabled)
> + drm_bridge_chain_post_disable(>bridge);
> + }
>
> if (edid) {
> if (drm_edid_is_valid(edid))
> @@ -852,12 +876,16 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge
> *bridge)
> HPD_DISABLE);
>
> drm_panel_prepare(pdata->panel);
> +
> + pdata->pre_enabled = true;
> }
>
> static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> {
> struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>
> + pdata->pre_enabled = false;
> +
> drm_panel_unprepare(pdata->panel);
>
> clk_disable_unprepare(pdata->refclk);
> @@ -891,6 +919,13 @@ static ssize_t
Re: [PATCH v2 10/14] drm/bridge: ti-sn65dsi86: Stop caching the EDID ourselves
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> Now that we have the patch ("drm/edid: Use the cached EDID in
> drm_get_edid() if eDP") we no longer need to maintain our own
> cache. Drop this code.
>
> Signed-off-by: Douglas Anderson
Reviewed-by: Andrzej Hajda
Regards
Andrzej
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 +-
> 1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 9577ebd58c4c..c0398daaa4a6 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -121,7 +121,6 @@
>* @debugfs: Used for managing our debugfs.
>* @host_node:Remote DSI node.
>* @dsi: Our MIPI DSI source.
> - * @edid: Detected EDID of eDP panel.
>* @refclk: Our reference clock.
>* @panel:Our panel.
>* @enable_gpio: The GPIO we toggle to enable the bridge.
> @@ -147,7 +146,6 @@ struct ti_sn_bridge {
> struct drm_bridge bridge;
> struct drm_connectorconnector;
> struct dentry *debugfs;
> - struct edid *edid;
> struct device_node *host_node;
> struct mipi_dsi_device *dsi;
> struct clk *refclk;
> @@ -269,17 +267,17 @@ connector_to_ti_sn_bridge(struct drm_connector
> *connector)
> static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> {
> struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> - struct edid *edid = pdata->edid;
> - int num;
> + struct edid *edid;
> + int num = 0;
>
> - if (!edid) {
> - pm_runtime_get_sync(pdata->dev);
> - edid = pdata->edid = drm_get_edid(connector, >aux.ddc);
> - pm_runtime_put(pdata->dev);
> - }
> + pm_runtime_get_sync(pdata->dev);
> + edid = drm_get_edid(connector, >aux.ddc);
> + pm_runtime_put(pdata->dev);
>
> - if (edid && drm_edid_is_valid(edid)) {
> - num = drm_add_edid_modes(connector, edid);
> + if (edid) {
> + if (drm_edid_is_valid(edid))
> + num = drm_add_edid_modes(connector, edid);
> + kfree(edid);
> if (num)
> return num;
> }
> @@ -1308,8 +1306,6 @@ static int ti_sn_bridge_remove(struct i2c_client
> *client)
> if (!pdata)
> return -EINVAL;
>
> - kfree(pdata->edid);
> -
> ti_sn_debugfs_remove(pdata);
>
> drm_bridge_remove(>bridge);
Re: [PATCH v2 08/14] drm/bridge: ti-sn65dsi86: Remove extra call: drm_connector_update_edid_property()
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> As of commit 5186421cbfe2 ("drm: Introduce epoch counter to
> drm_connector") the drm_get_edid() function calls
> drm_connector_update_edid_property() for us. There's no reason for us
> to call it again.
>
> Signed-off-by: Douglas Anderson
Reviewed-by: Andrzej Hajda
Regards
Andrzej
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 ---
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index a0a00dd1187c..9577ebd58c4c 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -270,7 +270,7 @@ static int ti_sn_bridge_connector_get_modes(struct
> drm_connector *connector)
> {
> struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> struct edid *edid = pdata->edid;
> - int num, ret;
> + int num;
>
> if (!edid) {
> pm_runtime_get_sync(pdata->dev);
> @@ -279,12 +279,9 @@ static int ti_sn_bridge_connector_get_modes(struct
> drm_connector *connector)
> }
>
> if (edid && drm_edid_is_valid(edid)) {
> - ret = drm_connector_update_edid_property(connector, edid);
> - if (!ret) {
> - num = drm_add_edid_modes(connector, edid);
> - if (num)
> - return num;
> - }
> + num = drm_add_edid_modes(connector, edid);
> + if (num)
> + return num;
> }
>
> return drm_panel_get_modes(pdata->panel, connector);
Re: [PATCH v2 07/14] drm/bridge: ti-sn65dsi86: Get rid of the useless detect() function
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> If we just leave the detect() function as NULL then the upper layers
> assume we're always connected. There's no reason for a stub.
>
> Signed-off-by: Douglas Anderson
Reviewed-by: Andrzej Hajda
Regards
Andrzej
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 12
> 1 file changed, 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 50a52af8e39f..a0a00dd1187c 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -306,20 +306,8 @@ static struct drm_connector_helper_funcs
> ti_sn_bridge_connector_helper_funcs = {
> .mode_valid = ti_sn_bridge_connector_mode_valid,
> };
>
> -static enum drm_connector_status
> -ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force)
> -{
> - /**
> - * TODO: Currently if drm_panel is present, then always
> - * return the status as connected. Need to add support to detect
> - * device state for hot pluggable scenarios.
> - */
> - return connector_status_connected;
> -}
> -
> static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
> .fill_modes = drm_helper_probe_single_connector_modes,
> - .detect = ti_sn_bridge_connector_detect,
> .destroy = drm_connector_cleanup,
> .reset = drm_atomic_helper_connector_reset,
> .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
Re: [PATCH v2 06/14] drm/bridge: ti-sn65dsi86: Move drm_panel_unprepare() to post_disable()
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> We prepared the panel in pre_enable() so we should unprepare it in
> post_disable() to match.
>
> This becomes important once we start using pre_enable() and
> post_disable() to make sure things are powered on (and then off again)
> when reading the EDID.
>
> Signed-off-by: Douglas Anderson
Reviewed-by: Andrzej Hajda
Regards
Andrzej
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index e8e523b3a16b..50a52af8e39f 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -460,8 +460,6 @@ static void ti_sn_bridge_disable(struct drm_bridge
> *bridge)
> regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0);
> /* disable DP PLL */
> regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
> -
> - drm_panel_unprepare(pdata->panel);
> }
>
> static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata)
> @@ -877,6 +875,8 @@ static void ti_sn_bridge_post_disable(struct drm_bridge
> *bridge)
> {
> struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>
> + drm_panel_unprepare(pdata->panel);
> +
> clk_disable_unprepare(pdata->refclk);
>
> pm_runtime_put_sync(pdata->dev);
Re: [PATCH v2 05/14] drm/bridge: ti-sn65dsi86: Move MIPI detach() / unregister() to detach()
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> The register() / attach() for MIPI happen in the bridge's
> attach(). That means that the inverse belongs in the bridge's
> detach().
As I commented in previous patch, it would be better to fix mipi/bridge
registration order in host and this driver.
Have you considered this?
Regards
Andrzej
>
> Signed-off-by: Douglas Anderson
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 +--
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index c006678c9921..e8e523b3a16b 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -437,7 +437,15 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>
> static void ti_sn_bridge_detach(struct drm_bridge *bridge)
> {
> - drm_dp_aux_unregister(_to_ti_sn_bridge(bridge)->aux);
> + struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> +
> + if (pdata->dsi) {
> + mipi_dsi_detach(pdata->dsi);
> + mipi_dsi_device_unregister(pdata->dsi);
> + }
> +
> + drm_dp_aux_unregister(>aux);
> }
>
> static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> @@ -1315,11 +1323,6 @@ static int ti_sn_bridge_remove(struct i2c_client
> *client)
> if (!pdata)
> return -EINVAL;
>
> - if (pdata->dsi) {
> - mipi_dsi_detach(pdata->dsi);
> - mipi_dsi_device_unregister(pdata->dsi);
> - }
> -
> kfree(pdata->edid);
>
> ti_sn_debugfs_remove(pdata);
Re: [PATCH v2 04/14] drm/bridge: ti-sn65dsi86: Reorder remove()
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> Let's make the remove() function strictly the reverse of the probe()
> function so it's easier to reason about.
>
> NOTES:
> - The MIPI calls probably belong in detach() but will be moved in a
>separate patch.
The mipi is incorrectly handled already - mipi devices are searched
after bridge registration - it should be reverse, there is comment in
the driver that it is due to some dsi hosts, maybe it would be better to
fix it there instead of conserve this bad design.
> - The cached EDID freeing isn't actually part of probe but needs to be
>in remove to avoid orphaning memory until better handling of the
>EDID happens.
> This patch was created by code inspection and should move us closer to
> a proper remove.
>
> Signed-off-by: Douglas Anderson
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 76f43af6735d..c006678c9921 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1315,20 +1315,21 @@ static int ti_sn_bridge_remove(struct i2c_client
> *client)
> if (!pdata)
> return -EINVAL;
>
> - kfree(pdata->edid);
> - ti_sn_debugfs_remove(pdata);
> -
> - of_node_put(pdata->host_node);
> -
> - pm_runtime_disable(pdata->dev);
> -
> if (pdata->dsi) {
> mipi_dsi_detach(pdata->dsi);
> mipi_dsi_device_unregister(pdata->dsi);
> }
>
> + kfree(pdata->edid);
> +
> + ti_sn_debugfs_remove(pdata);
> +
> drm_bridge_remove(>bridge);
>
> + pm_runtime_disable(pdata->dev);
> +
> + of_node_put(pdata->host_node);
> +
Looks good.
Reviewed-by: Andrzej Hajda
Regards
Andrzej
> return 0;
> }
>
Re: [PATCH v2 03/14] drm/bridge: ti-sn65dsi86: Remove incorrectly tagged kerneldoc comment
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze: > A random comment inside a function had "/**" in front of it. That > doesn't make sense. Remove. > > Signed-off-by: Douglas Anderson Reviewed-by: Andrzej Hajda Regards Andrzej > --- > > (no changes since v1) > > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > index 96fe8f2c0ea9..76f43af6735d 100644 > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c > @@ -788,7 +788,7 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge) > /* set dsi clk frequency value */ > ti_sn_bridge_set_dsi_rate(pdata); > > - /** > + /* >* The SN65DSI86 only supports ASSR Display Authentication method and >* this method is enabled by default. An eDP panel must support this >* authentication method. We need to enable this method in the eDP panel
Re: [PATCH v2 02/14] drm/bridge: ti-sn65dsi86: Simplify refclk handling
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> The clock framework makes it simple to deal with an optional clock.
> You can call clk_get_optional() and if the clock isn't specified it'll
> just return NULL without complaint. It's valid to pass NULL to
> enable/disable/prepare/unprepare. Let's make use of this to simplify
> things a tiny bit.
>
> Signed-off-by: Douglas Anderson
> Reviewed-by: Robert Foss
> Reviewed-by: Bjorn Andersson
> Reviewed-by: Stephen Boyd
> Reviewed-by: Laurent Pinchart
Reviewed-by: Andrzej Hajda
Regards
Andrzej
> ---
>
> Changes in v2:
> - Removed 2nd paragraph in commit message.
>
> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 11 +++
> 1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index 88df4dd0f39d..96fe8f2c0ea9 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -1275,14 +1275,9 @@ static int ti_sn_bridge_probe(struct i2c_client
> *client,
> return ret;
> }
>
> - pdata->refclk = devm_clk_get(pdata->dev, "refclk");
> - if (IS_ERR(pdata->refclk)) {
> - ret = PTR_ERR(pdata->refclk);
> - if (ret == -EPROBE_DEFER)
> - return ret;
> - DRM_DEBUG_KMS("refclk not found\n");
> - pdata->refclk = NULL;
> - }
> + pdata->refclk = devm_clk_get_optional(pdata->dev, "refclk");
> + if (IS_ERR(pdata->refclk))
> + return PTR_ERR(pdata->refclk);
>
> ret = ti_sn_bridge_parse_dsi_host(pdata);
> if (ret)
Re: [PATCH v2 01/14] drm/bridge: Fix the stop condition of drm_bridge_chain_pre_enable()
Hi Douglas,
W dniu 30.03.2021 o 04:53, Douglas Anderson pisze:
> The drm_bridge_chain_pre_enable() is not the proper opposite of
> drm_bridge_chain_post_disable(). It continues along the chain to
> _before_ the starting bridge. Let's fix that.
>
> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> Signed-off-by: Douglas Anderson
> ---
>
> (no changes since v1)
>
> drivers/gpu/drm/drm_bridge.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 64f0effb52ac..044acd07c153 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -522,6 +522,9 @@ void drm_bridge_chain_pre_enable(struct drm_bridge
> *bridge)
> list_for_each_entry_reverse(iter, >bridge_chain, chain_node) {
> if (iter->funcs->pre_enable)
> iter->funcs->pre_enable(iter);
> +
> + if (iter == bridge)
> + break;
Looking at the bridge chaining code always makes me sick :) but beside
this the change looks correct, and follows
drm_atomic_bridge_chain_pre_enable.
Reviewed-by: Andrzej Hajda
Regards
Andrzej
> }
> }
> EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
Re: [PATCH v1] MAINTAINERS: Update Maintainers of DRM Bridge Drivers
W dniu 24.03.2021 o 11:20, Robert Foss pisze: > Add myself as co-maintainer of DRM Bridge Drivers. Repository > commit access has already been granted. > > https://protect2.fireeye.com/v1/url?k=c3508e7b-9ccbb771-c3510534-0cc47a31384a-ef2b7fbec8aa658e=1=46fd05b7-d9d9-4737-99cd-cd44e40a7bc7=https%3A%2F%2Fgitlab.freedesktop.org%2Ffreedesktop%2Ffreedesktop%2F-%2Fissues%2F338 > > Cc: Neil Armstrong > Cc: Laurent Pinchart > Cc: Jonas Karlman > Cc: Andrzej Hajda > Cc: Jernej Škrabec > Cc: Daniel Vetter > Signed-off-by: Robert Foss Great. Acked-by: Andrzej Hajda Regards Andrzej > --- > MAINTAINERS | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/MAINTAINERS b/MAINTAINERS > index 4b705ba51c54..16ace8f58649 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -5902,6 +5902,7 @@ F: drivers/gpu/drm/atmel-hlcdc/ > DRM DRIVERS FOR BRIDGE CHIPS > M: Andrzej Hajda > M: Neil Armstrong > +M: Robert Foss > R: Laurent Pinchart > R: Jonas Karlman > R: Jernej Skrabec
Re: [PATCH 2/2] driver core: add helper for deferred probe reason setting
W dniu 18.03.2021 o 08:39, Ahmad Fatoum pisze:
> We now have three places within the same file doing the same operation
> of freeing this pointer and setting it anew. A helper make this
> arguably easier to read, so add one.
>
> Signed-off-by: Ahmad Fatoum
Reviewed-by: Andrzej Hajda
Regards
Andrzej
> ---
> drivers/base/dd.c | 17 +++--
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index e2cf3b29123e..4201baa1cc13 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -69,6 +69,12 @@ static char async_probe_drv_names[ASYNC_DRV_NAMES_MAX_LEN];
>*/
> static bool defer_all_probes;
>
> +static void __device_set_deferred_probe_reason(const struct device *dev,
> char *reason)
> +{
> + kfree(dev->p->deferred_probe_reason);
> + dev->p->deferred_probe_reason = reason;
> +}
> +
> /*
>* deferred_probe_work_func() - Retry probing devices in the active list.
>*/
> @@ -97,8 +103,7 @@ static void deferred_probe_work_func(struct work_struct
> *work)
>
> get_device(dev);
>
> - kfree(dev->p->deferred_probe_reason);
> - dev->p->deferred_probe_reason = NULL;
> + __device_set_deferred_probe_reason(dev, NULL);
>
> /*
>* Drop the mutex while probing each device; the probe path may
> @@ -140,8 +145,7 @@ void driver_deferred_probe_del(struct device *dev)
> if (!list_empty(>p->deferred_probe)) {
> dev_dbg(dev, "Removed from deferred list\n");
> list_del_init(>p->deferred_probe);
> - kfree(dev->p->deferred_probe_reason);
> - dev->p->deferred_probe_reason = NULL;
> + __device_set_deferred_probe_reason(dev, NULL);
> }
> mutex_unlock(_probe_mutex);
> }
> @@ -220,11 +224,12 @@ void device_unblock_probing(void)
> void device_set_deferred_probe_reason(const struct device *dev, struct
> va_format *vaf)
> {
> const char *drv = dev_driver_string(dev);
> + char *reason;
>
> mutex_lock(_probe_mutex);
>
> - kfree(dev->p->deferred_probe_reason);
> - dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv,
> vaf);
> + reason = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf);
> + __device_set_deferred_probe_reason(dev, reason);
>
> mutex_unlock(_probe_mutex);
> }
Re: [PATCH v2 1/2] driver core: clear deferred probe reason on probe retry
Hi Ahmad,
W dniu 19.03.2021 o 12:04, Ahmad Fatoum pisze:
> When retrying a deferred probe, any old defer reason string should be
> discarded. Otherwise, if the probe is deferred again at a different spot,
> but without setting a message, the now incorrect probe reason will remain.
>
> This was observed with the i.MX I2C driver, which ultimately failed
> to probe due to lack of the GPIO driver. The probe defer for GPIO
> doesn't record a message, but a previous probe defer to clock_get did.
> This had the effect that /sys/kernel/debug/devices_deferred listed
> a misleading probe deferral reason.
>
> Cc: sta...@kernel.org
> Fixes: d090b70ede02 ("driver core: add deferring probe reason to
> devices_deferred property")
> Reviewed-by: Andy Shevchenko
> Signed-off-by: Ahmad Fatoum
Reviewed-by: Andrzej Hajda
Regards
Andrzej
> ---
> v1 -> v2:
> - reworded commit message (Andy)
> - collected Andy's Reviewed-by
> ---
> drivers/base/dd.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 9179825ff646..e2cf3b29123e 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -97,6 +97,9 @@ static void deferred_probe_work_func(struct work_struct
> *work)
>
> get_device(dev);
>
> + kfree(dev->p->deferred_probe_reason);
> + dev->p->deferred_probe_reason = NULL;
> +
> /*
>* Drop the mutex while probing each device; the probe path may
>* manipulate the deferred list
Re: [PATCH] drm/dsi: Add _NO_ to MIPI_DSI_* flags disabling features
Hi Nicolas,
W dniu 22.02.2021 o 06:31, Nicolas Boichat pisze:
> On Mon, Feb 22, 2021 at 3:08 AM Laurent Pinchart
> wrote:
>> Hi Nicolas,
>>
>> Thank you for the patch.
>>
>> On Thu, Feb 11, 2021 at 11:33:55AM +0800, Nicolas Boichat wrote:
>>> Many of the DSI flags have names opposite to their actual effects,
>>> e.g. MIPI_DSI_MODE_EOT_PACKET means that EoT packets will actually
>>> be disabled. Fix this by including _NO_ in the flag names, e.g.
>>> MIPI_DSI_MODE_NO_EOT_PACKET.
>>>
>>> Signed-off-by: Nicolas Boichat
>> This looks good to me, it increases readability.
>>
>> Reviewed-by: Laurent Pinchart
>>
>> Please however see the end of the mail for a comment.
Reviewed-by: Andrzej Hajda
And comment at the end.
>>
>>> ---
>>> I considered adding _DISABLE_ instead, but that'd make the
>>> flag names a big too long.
>>>
>>> Generated with:
>>> flag=MIPI_DSI_MODE_VIDEO_HFP; git grep $flag | cut -f1 -d':' | \
>>>xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_VIDEO_NO_HFP/" {}
>>> flag=MIPI_DSI_MODE_VIDEO_HBP; git grep $flag | cut -f1 -d':' | \
>>>xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_VIDEO_NO_HBP/" {}
>>> flag=MIPI_DSI_MODE_VIDEO_HSA; git grep $flag | cut -f1 -d':' | \
>>>xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_VIDEO_NO_HSA/" {}
>>> flag=MIPI_DSI_MODE_EOT_PACKET; git grep $flag | cut -f1 -d':' | \
>>>xargs -I{} sed -i -e "s/$flag/MIPI_DSI_MODE_NO_EOT_PACKET/" {}
>>> (then minor format changes)
>> Ever tried coccinelle ? :-)
> Fun project for next time ,-)
>
>>> drivers/gpu/drm/bridge/adv7511/adv7533.c | 2 +-
>>> drivers/gpu/drm/bridge/analogix/anx7625.c| 2 +-
>>> drivers/gpu/drm/bridge/cdns-dsi.c| 4 ++--
>>> drivers/gpu/drm/bridge/tc358768.c| 2 +-
>>> drivers/gpu/drm/exynos/exynos_drm_dsi.c | 8
>>> drivers/gpu/drm/mcde/mcde_dsi.c | 2 +-
>>> drivers/gpu/drm/mediatek/mtk_dsi.c | 2 +-
>>> drivers/gpu/drm/msm/dsi/dsi_host.c | 8
>>> drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c | 2 +-
>>> drivers/gpu/drm/panel/panel-dsi-cm.c | 2 +-
>>> drivers/gpu/drm/panel/panel-elida-kd35t133.c | 2 +-
>>> drivers/gpu/drm/panel/panel-khadas-ts050.c | 2 +-
>>> drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c | 2 +-
>>> drivers/gpu/drm/panel/panel-leadtek-ltk500hd1829.c | 2 +-
>>> drivers/gpu/drm/panel/panel-novatek-nt35510.c| 2 +-
>>> drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c | 2 +-
>>> drivers/gpu/drm/panel/panel-samsung-s6d16d0.c| 2 +-
>>> drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c | 2 +-
>>> drivers/gpu/drm/panel/panel-samsung-s6e63m0-dsi.c| 2 +-
>>> drivers/gpu/drm/panel/panel-samsung-s6e8aa0.c| 4 ++--
>>> drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 2 +-
>>> drivers/gpu/drm/panel/panel-simple.c | 2 +-
>>> drivers/gpu/drm/panel/panel-sony-acx424akp.c | 2 +-
>>> drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c | 2 +-
>>> include/drm/drm_mipi_dsi.h | 8
>>> 25 files changed, 36 insertions(+), 36 deletions(-)
>>>
>>> []
>>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>>> index 360e6377e84b..ba91cf22af51 100644
>>> --- a/include/drm/drm_mipi_dsi.h
>>> +++ b/include/drm/drm_mipi_dsi.h
>>> @@ -119,15 +119,15 @@ struct mipi_dsi_host
>>> *of_find_mipi_dsi_host_by_node(struct device_node *node);
>>> /* enable hsync-end packets in vsync-pulse and v-porch area */
>>> #define MIPI_DSI_MODE_VIDEO_HSE BIT(4)
>> We're mixing bits that enable a feature and bits that disable a feature.
>> Are these bits defined in the DSI spec, or internal to DRM ? In the
>> latter case, would it make sense to standardize on one "polarity" ? That
>> would be a more intrusive change in drivers though.
> Yes, that'd require auditing every single code path and reverse the
> logic as needed. I'm not volunteering for that ,-P (hopefully the
> current change is still an improvement).
>
> Hopefully real DSI experts can comment (Andrzej?), I think the default
> are sensible settings?
Hehe, "real DSI expert" :), ok I've read spec few time
Re: [PATCH] drm/bridge: anx7625: enable DSI EOTP
W dniu 04.02.2021 o 13:34, Nicolas Boichat pisze:
> On Thu, Feb 4, 2021 at 8:07 PM Robert Foss wrote:
>> Hi Xin,
>>
>> Thanks for the patch.
>>
>> On Thu, 28 Jan 2021 at 12:17, Xin Ji wrote:
>>> Enable DSI EOTP feature for fixing some panel screen constance
>>> shift issue.
>>> Removing MIPI flag MIPI_DSI_MODE_EOT_PACKET to enable DSI EOTP.
>> I don't think I quite understand how removing the
>> MIPI_DSI_MODE_EOT_PACKET flag will cause DSI EOTP to be enabled. Could
>> you extrapolate on this in the commit message?
> That confused me as well, but it turns out that's how the flag is defined:
> ```
> /* disable EoT packets in HS mode */
> #define MIPI_DSI_MODE_EOT_PACKET BIT(9)
> ```
> (https://protect2.fireeye.com/v1/url?k=5bd95ebd-044267fb-5bd8d5f2-0cc47a3003e8-ce9db8ea264d6901=1=900556dc-d199-4c18-9432-5c3465a98eae=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Flatest%2Fsource%2Finclude%2Fdrm%2Fdrm_mipi_dsi.h%23L129)
>
> I'm almost tempted to put together a mass patch to rename all of these
> flags...
Yes that would be good, many of these flags were just copy pasted from
some hw datasheet, without good analysis how to adapt them to the framework.
Regards
Andrzej
>
>>> Signed-off-by: Xin Ji
>>> ---
>>> drivers/gpu/drm/bridge/analogix/anx7625.c | 1 -
>>> 1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> b/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> index 65cc059..e31eeb1b 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
>>> @@ -1334,7 +1334,6 @@ static int anx7625_attach_dsi(struct anx7625_data
>>> *ctx)
>>> dsi->format = MIPI_DSI_FMT_RGB888;
>>> dsi->mode_flags = MIPI_DSI_MODE_VIDEO |
>>> MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
>>> - MIPI_DSI_MODE_EOT_PACKET|
>>> MIPI_DSI_MODE_VIDEO_HSE;
>>>
>>> if (mipi_dsi_attach(dsi) < 0) {
>>> --
>>> 2.7.4
>>>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://protect2.fireeye.com/v1/url?k=457f3f39-1ae4067f-457eb476-0cc47a3003e8-b702072da729d8c9=1=900556dc-d199-4c18-9432-5c3465a98eae=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>
Re: [PATCH v6] drm/bridge: add it6505 driver
Hi Allen, Sorry for long delay. W dniu 08.12.2020 o 11:58, allen pisze: > This adds support for the iTE IT6505. > This device can convert DPI signal to DP output. > > From: Allen Chen > Signed-off-by: Jitao Shi > Signed-off-by: Pi-Hsun Shih > Signed-off-by: Yilun Lin > Signed-off-by: Hermes Wu > Signed-off-by: Allen Chen > --- > drivers/gpu/drm/bridge/Kconfig |7 + > drivers/gpu/drm/bridge/Makefile |1 + > drivers/gpu/drm/bridge/ite-it6505.c | 3343 +++ > 3 files changed, 3351 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/ite-it6505.c > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index e4110d6ca7b3c..25d34d7196004 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -74,6 +74,13 @@ config DRM_LONTIUM_LT9611UXC > HDMI signals > Please say Y if you have such hardware. > > +config DRM_ITE_IT6505 > + tristate "ITE IT6505 DisplayPort bridge" > + depends on OF > + select DRM_KMS_HELPER > + help > + ITE IT6505 DisplayPort bridge chip driver. > + > config DRM_LVDS_CODEC > tristate "Transparent LVDS encoders and decoders support" > depends on OF > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 86e7acc76f8d6..2b2f8f0b5b0fa 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_CHRONTEL_CH7033) += chrontel-ch7033.o > obj-$(CONFIG_DRM_DISPLAY_CONNECTOR) += display-connector.o > obj-$(CONFIG_DRM_LONTIUM_LT9611) += lontium-lt9611.o > obj-$(CONFIG_DRM_LONTIUM_LT9611UXC) += lontium-lt9611uxc.o > +obj-$(CONFIG_DRM_ITE_IT6505) += ite-it6505.o Please keep alphabetic order. > obj-$(CONFIG_DRM_LVDS_CODEC) += lvds-codec.o > obj-$(CONFIG_DRM_MEGACHIPS_STDP_GE_B850V3_FW) += > megachips-stdp-ge-b850v3-fw.o > obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o > diff --git a/drivers/gpu/drm/bridge/ite-it6505.c > b/drivers/gpu/drm/bridge/ite-it6505.c > new file mode 100644 > index 0..5e76719a51a4a > --- /dev/null > +++ b/drivers/gpu/drm/bridge/ite-it6505.c > @@ -0,0 +1,3343 @@ > +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +/* > + * Copyright (c) 2020, The Linux Foundation. All rights reserved. > + */ > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#define REG_IC_VER 0x04 > + > +#define REG_RESET_CTRL 0x05 > +#define VIDEO_RESET BIT(0) > +#define AUDIO_RESET BIT(1) > +#define ALL_LOGIC_RESET BIT(2) > +#define AUX_RESET BIT(3) > +#define HDCP_RESET BIT(4) > + > +#define INT_STATUS_01 0x06 > +#define INT_MASK_01 0x09 > +#define INT_HPD_CHANGE BIT(0) > +#define INT_RECEIVE_HPD_IRQ BIT(1) > +#define INT_SCDT_CHANGE BIT(2) > +#define INT_HDCP_FAIL BIT(3) > +#define INT_HDCP_DONE BIT(4) > + > +#define INT_STATUS_02 0x07 > +#define INT_MASK_02 0x0A > +#define INT_AUX_CMD_FAIL BIT(0) > +#define INT_HDCP_KSV_CHECK BIT(1) > +#define INT_AUDIO_FIFO_ERROR BIT(2) > + > +#define INT_STATUS_03 0x08 > +#define INT_MASK_03 0x0B > +#define INT_LINK_TRAIN_FAIL BIT(4) > +#define INT_VID_FIFO_ERROR BIT(5) > +#define INT_IO_LATCH_FIFO_OVERFLOW BIT(7) > + > +#define REG_SYSTEM_STS 0x0D > +#define INT_STS BIT(0) > +#define HPD_STS BIT(1) > +#define VIDEO_STB BIT(2) > + > +#define REG_LINK_TRAIN_STS 0x0E > +#define LINK_STATE_CR BIT(2) > +#define LINK_STATE_EQ BIT(3) > +#define LINK_STATE_NORP BIT(4) > + > +#define REG_BANK_SEL 0x0F > +#define REG_CLK_CTRL0 0x10 > +#define M_PCLK_DELAY 0x03 > + > +#define REG_AUX_OPT 0x11 > +#define AUX_AUTO_RST BIT(0) > +#define AUX_FIX_FREQ BIT(3) > + > +#define REG_DATA_CTRL0 0x12 > +#define VIDEO_LATCH_EDGE BIT(4) > +#define ENABLE_PCLK_COUNTER BIT(7) > + > +#define REG_PCLK_COUNTER_VALUE 0x13 > + > +#define REG_501_FIFO_CTRL 0x15 > +#define RST_501_FIFO BIT(1) > + > +#define REG_TRAIN_CTRL0 0x16 > +#define FORCE_LBR BIT(0) > +#define LANE_COUNT_MASK 0x06 > +#define LANE_SWAP BIT(3) > +#define SPREAD_AMP_5 BIT(4) > +#define FORCE_CR_DONE BIT(5) > +#define FORCE_EQ_DONE BIT(6) > + > +#define REG_TRAIN_CTRL1 0x17 > +#define AUTO_TRAIN BIT(0) > +#define MANUAL_TRAIN BIT(1) > +#define FORCE_RETRAIN BIT(2) > + > +#define REG_AUX_CTRL 0x23 > +#define CLR_EDID_FIFO BIT(0) > +#define AUX_USER_MODE BIT(1) > +#define AUX_NO_SEGMENT_WR BIT(6) > +#define AUX_EN_FIFO_READ BIT(7) > + > +#define REG_AUX_ADR_0_7 0x24 > +#define REG_AUX_ADR_8_15 0x25 > +#define REG_AUX_ADR_16_19 0x26 > +#define REG_AUX_OUT_DATA0 0x27 > + > +#define REG_AUX_CMD_REQ 0x2B > +#define AUX_BUSY BIT(5) > + > +#define REG_AUX_DATA_0_7 0x2C > +#define REG_AUX_DATA_8_15 0x2D > +#define
Re: [PATCH RESEND] drm/bridge: tc358764: restore connector support
W dniu 04.10.2020 o 21:14, Sam Ravnborg pisze: > Hi Marek. > > On Wed, Sep 30, 2020 at 01:40:42PM +0200, Marek Szyprowski wrote: >> This patch restores DRM connector registration in the TC358764 bridge >> driver and restores usage of the old drm_panel_* API, thus allows dynamic >> panel registration. This fixes panel operation on Exynos5250-based >> Arndale board. >> >> This is equivalent to the revert of the following commits: >> 1644127f83bc "drm/bridge: tc358764: add drm_panel_bridge support" >> 385ca38da29c "drm/bridge: tc358764: drop drm_connector_(un)register" >> and removal of the calls to drm_panel_attach()/drm_panel_detach(), which >> were no-ops and has been removed in meanwhile. >> >> Signed-off-by: Marek Szyprowski >> Reviewed-by: Andrzej Hajda > Thanks for providing the revert so we can have this fixed in upstream. > So far I have had no time to dive deeper into what is going wrong but > and the revert is the right cause of action for now. > > I expect Andrzej to pick it up as he has already reviewed it. > > Sam Done Regards Andrzej
Re: [PATCH] drm/bridge: tc358764: restore connector support
W dniu 24.09.2020 o 10:31, Marek Szyprowski pisze:
> This patch restores DRM connector registration in the TC358764 bridge
> driver and restores usage of the old drm_panel_* API, thus allows dynamic
> panel registration. This fixes panel operation on Exynos5250-based
> Arndale board.
>
> This is equivalent to the revert of the following commits:
> 1644127f83bc "drm/bridge: tc358764: add drm_panel_bridge support"
> 385ca38da29c "drm/bridge: tc358764: drop drm_connector_(un)register"
> and removal of the calls to drm_panel_attach()/drm_panel_detach(), which
> were no-ops and has been removed in meanwhile.
>
> Signed-off-by: Marek Szyprowski
Reviewed-by: Andrzej Hajda
Regards
Andrzej
> ---
> As I've reported and Andrzej Hajda pointed, the reverted patches break
> operation of the panel on the Arndale board. Noone suggested how to fix
> the regression, I've decided to send a revert until a new solution is
> found.
>
> The issues with tc358764 might be automatically resolved once the Exynos
> DSI itself is converted to DRM bridge:
> https://patchwork.kernel.org/cover/11770683/
> but that approach has also its own issues so far.
>
> Best regards,
> Marek Szyprowski
> ---
> drivers/gpu/drm/bridge/tc358764.c | 107 +-
> 1 file changed, 92 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358764.c
> b/drivers/gpu/drm/bridge/tc358764.c
> index d89394bc5aa4..c1e35bdf9232 100644
> --- a/drivers/gpu/drm/bridge/tc358764.c
> +++ b/drivers/gpu/drm/bridge/tc358764.c
> @@ -153,9 +153,10 @@ static const char * const tc358764_supplies[] = {
> struct tc358764 {
> struct device *dev;
> struct drm_bridge bridge;
> + struct drm_connector connector;
> struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)];
> struct gpio_desc *gpio_reset;
> - struct drm_bridge *panel_bridge;
> + struct drm_panel *panel;
> int error;
> };
>
> @@ -209,6 +210,12 @@ static inline struct tc358764 *bridge_to_tc358764(struct
> drm_bridge *bridge)
> return container_of(bridge, struct tc358764, bridge);
> }
>
> +static inline
> +struct tc358764 *connector_to_tc358764(struct drm_connector *connector)
> +{
> + return container_of(connector, struct tc358764, connector);
> +}
> +
> static int tc358764_init(struct tc358764 *ctx)
> {
> u32 v = 0;
> @@ -271,11 +278,43 @@ static void tc358764_reset(struct tc358764 *ctx)
> usleep_range(1000, 2000);
> }
>
> +static int tc358764_get_modes(struct drm_connector *connector)
> +{
> + struct tc358764 *ctx = connector_to_tc358764(connector);
> +
> + return drm_panel_get_modes(ctx->panel, connector);
> +}
> +
> +static const
> +struct drm_connector_helper_funcs tc358764_connector_helper_funcs = {
> + .get_modes = tc358764_get_modes,
> +};
> +
> +static const struct drm_connector_funcs tc358764_connector_funcs = {
> + .fill_modes = drm_helper_probe_single_connector_modes,
> + .destroy = drm_connector_cleanup,
> + .reset = drm_atomic_helper_connector_reset,
> + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static void tc358764_disable(struct drm_bridge *bridge)
> +{
> + struct tc358764 *ctx = bridge_to_tc358764(bridge);
> + int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel);
> +
> + if (ret < 0)
> + dev_err(ctx->dev, "error disabling panel (%d)\n", ret);
> +}
> +
> static void tc358764_post_disable(struct drm_bridge *bridge)
> {
> struct tc358764 *ctx = bridge_to_tc358764(bridge);
> int ret;
>
> + ret = drm_panel_unprepare(ctx->panel);
> + if (ret < 0)
> + dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret);
> tc358764_reset(ctx);
> usleep_range(1, 15000);
> ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
> @@ -296,28 +335,71 @@ static void tc358764_pre_enable(struct drm_bridge
> *bridge)
> ret = tc358764_init(ctx);
> if (ret < 0)
> dev_err(ctx->dev, "error initializing bridge (%d)\n", ret);
> + ret = drm_panel_prepare(ctx->panel);
> + if (ret < 0)
> + dev_err(ctx->dev, "error preparing panel (%d)\n", ret);
> +}
> +
> +static void tc358764_enable(struct drm_bridge *bridge)
> +{
> + struct tc358764 *ctx = bridge_to_tc358764(bridge);
> + int ret = drm_panel_enable(ctx->panel);
> +
> + if (ret
Re: [RFT 09/10] arm64: dts: exynos: Correct port of USB-C connector node on Exynos5433 TM2
On 31.08.2020 14:50, Marek Szyprowski wrote:
> Hi Krzysztof,
>
> On 29.08.2020 16:25, Krzysztof Kozlowski wrote:
>> The USB-C connector bindings require port@0. Such port was already
>> described in DTS but outside of the connector itself. Put it into
>> proper place to fix dtbs_check warnings like:
>>
>> arch/arm64/boot/dts/exynos/exynos5433-tm2.dt.yaml: musb_connector:
>> ports: 'port@0' is a required property
>>
>> Signed-off-by: Krzysztof Kozlowski
> I'm not sure if topic should be about USB-C, I will call it simply USB
> connector node. TM2(e) uses Samsung's 11-pin micro USB 2.0 connector,
> which has nothing in common with USB Type-C.
>
> Anyway, this patch breaks DWC3 (tested in Device mode) driver operation,
> so something has to be somehow adjusted or fixed. Added CC Andrzej
> Hajda, who actually worked on this.
>
>> ---
>>
>> Not tested on HQ. Please kindly review and test.
>>
>> Best regards,
>> Krzysztof
>> ---
>>.../boot/dts/exynos/exynos5433-tm2-common.dtsi| 15 +++
>>1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
>> b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
>> index 6246cce2a15e..bab6c1addd5f 100644
>> --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
>> +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2-common.dtsi
>> @@ -871,6 +871,13 @@
>> #address-cells = <1>;
>> #size-cells = <0>;
>>
>> +port@0 {
>> +reg = <0>;
>> +muic_to_usb: endpoint {
>> +remote-endpoint =
>> <_to_muic>;
>> +};
>> +};
>> +
According to not-yet-yaml documentation of dt-bindings (patch 05/10):
> -Required nodes:
> -- any data bus to the connector should be modeled using the OF graph bindings
> - specified in bindings/graph.txt, unless the bus is between parent node and
> - the connector.
This is 'unless' case - muic is parent of the connector, so the port 0 is not
necessary.
>> port@3 {
>> reg = <3>;
>> musb_con_to_mhl: endpoint {
>> @@ -879,14 +886,6 @@
>> };
>> };
>> };
>> -
>> -ports {
>> -port {
>> -muic_to_usb: endpoint {
>> -remote-endpoint =
>> <_to_muic>;
>> -};
>> -};
And this port belongs to MUIC - it describes connection between USB-HOST
and MUIC, it has nothing to do with the connector, and is necessary.
Regards
Andrzej
>> -};
>> };
>>
>> regulators {
> Best regards
Re: [PATCH 2/2] drm/exynos: hdmi: Simplify with dev_err_probe()
On 26.08.2020 16:55, Krzysztof Kozlowski wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and also it prints the error value. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [PATCH 1/2] drm/exynos: dsi: Simplify with dev_err_probe()
On 26.08.2020 16:55, Krzysztof Kozlowski wrote: > Common pattern of handling deferred probe can be simplified with > dev_err_probe(). Less code and also it prints the error value. > > Signed-off-by: Krzysztof Kozlowski Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [PATCH v1] driver core: Annotate dev_err_probe() with __must_check
Hi Andy,
On 26.08.2020 12:44, Andy Shevchenko wrote:
> We have got already new users of this API which interpret it differently
> and miss the opportunity to optimize their code.
>
> In order to avoid similar cases in the future, annotate dev_err_probe()
> with __must_check.
There are many cases where __must_check can be annoying, for example:
ret = ...;
if (ret < 0) {
dev_err_probe(...);
goto cleanup;
}
Or (less frequently):
ptr = ...;
if (IS_ERR(ptr)) {
dev_err_probe(...);
return ptr;
}
Of course in both cases one can add workarounds, but I am not sure what
is better.
Regards
Andrzej
Re: [PATCH] driver core: Let dev_err_probe() use the symbolic error code
Hi Uwe,
On 11.08.2020 09:20, Uwe Kleine-König wrote:
> This makes the error message:
>
> error -EIO: ...
>
> instead of
>
> error -5: ...
>
> Signed-off-by: Uwe Kleine-König
> ---
> drivers/base/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index ac1046a382bc..33734d8831c7 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -4237,10 +4237,10 @@ int dev_err_probe(const struct device *dev, int err,
> const char *fmt, ...)
> vaf.va =
>
> if (err != -EPROBE_DEFER) {
> - dev_err(dev, "error %d: %pV", err, );
> + dev_err(dev, "error %pE: %pV", ERR_PTR(err), );
> } else {
> device_set_deferred_probe_reason(dev, );
> - dev_dbg(dev, "error %d: %pV", err, );
> + dev_dbg(dev, "error %pE: %pV", ERR_PTR(err), );
Apparently I have misread docs about %pE flag. Thanks for spotting this.
Reviewed-by: Andrzej Hajda
Regards
Andrzej
> }
>
> va_end(args);
Re: [PATCH v9 0/4] driver core: add probe error check helper
Hi Greg, Apparently the patchset has no more comments. Could you take the patches to your tree? At least 1st and 2nd. Regards Andrzej On 13.07.2020 16:43, Andrzej Hajda wrote: > Hi All, > > Thanks for comments. > > Changes since v8: > - fixed typo in function name, > - removed cocci script (added by mistake) > > Changes since v7: > - improved commit message > - added R-Bs > > Changes since v6: > - removed leftovers from old naming scheme in commit descritions, > - added R-Bs. > > Changes since v5: > - removed patch adding macro, dev_err_probe(dev, PTR_ERR(ptr), ...) should be > used instead, > - added dev_dbg logging in case of -EPROBE_DEFER, > - renamed functions and vars according to comments, > - extended docs, > - cosmetics. > > Original message (with small adjustments): > > Recently I took some time to re-check error handling in drivers probe code, > and I have noticed that number of incorrect resource acquisition error > handling > increased and there are no other propositions which can cure the situation. > > So I have decided to resend my old proposition of probe_err helper which > should > simplify resource acquisition error handling, it also extend it with adding > defer > probe reason to devices_deferred debugfs property, which should improve > debugging > experience for developers/testers. > > I have also added two patches showing usage and benefits of the helper. > > My dirty/ad-hoc cocci scripts shows that this helper can be used in at least > 2700 places > saving about 3500 lines of code. > > Regards > Andrzej > > > Andrzej Hajda (4): >driver core: add device probe log helper >driver core: add deferring probe reason to devices_deferred property >drm/bridge/sii8620: fix resource acquisition error handling >drm/bridge: lvds-codec: simplify error handling > > drivers/base/base.h | 3 ++ > drivers/base/core.c | 46 > drivers/base/dd.c| 23 +- > drivers/gpu/drm/bridge/lvds-codec.c | 10 ++ > drivers/gpu/drm/bridge/sil-sii8620.c | 21 ++--- > include/linux/device.h | 3 ++ > 6 files changed, 86 insertions(+), 20 deletions(-) >
Re: [PATCH] drm/bridge: sil_sii8620: initialize return of sii8620_readb
On 12.07.2020 17:24, t...@redhat.com wrote:
> From: Tom Rix
>
> clang static analysis flags this error
>
> sil-sii8620.c:184:2: warning: Undefined or garbage value
>returned to caller [core.uninitialized.UndefReturn]
> return ret;
> ^~
>
> sii8620_readb calls sii8620_read_buf.
> sii8620_read_buf can return without setting its output
> pararmeter 'ret'.
>
> So initialize ret.
>
> Fixes: ce6e153f414a ("drm/bridge: add Silicon Image SiI8620 driver")
>
> Signed-off-by: Tom Rix
> ---
> drivers/gpu/drm/bridge/sil-sii8620.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
> b/drivers/gpu/drm/bridge/sil-sii8620.c
> index 3540e4931383..da933d477e5f 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -178,7 +178,7 @@ static void sii8620_read_buf(struct sii8620 *ctx, u16
> addr, u8 *buf, int len)
>
> static u8 sii8620_readb(struct sii8620 *ctx, u16 addr)
> {
> - u8 ret;
> + u8 ret = 0;
In theory it shouldn't cause any harm, but this protections makes things
simpler.
Reviewed-by: Andrzej Hajda
Regards
Andrzej
>
> sii8620_read_buf(ctx, addr, , 1);
> return ret;
[PATCH v9 0/4] driver core: add probe error check helper
Hi All, Thanks for comments. Changes since v8: - fixed typo in function name, - removed cocci script (added by mistake) Changes since v7: - improved commit message - added R-Bs Changes since v6: - removed leftovers from old naming scheme in commit descritions, - added R-Bs. Changes since v5: - removed patch adding macro, dev_err_probe(dev, PTR_ERR(ptr), ...) should be used instead, - added dev_dbg logging in case of -EPROBE_DEFER, - renamed functions and vars according to comments, - extended docs, - cosmetics. Original message (with small adjustments): Recently I took some time to re-check error handling in drivers probe code, and I have noticed that number of incorrect resource acquisition error handling increased and there are no other propositions which can cure the situation. So I have decided to resend my old proposition of probe_err helper which should simplify resource acquisition error handling, it also extend it with adding defer probe reason to devices_deferred debugfs property, which should improve debugging experience for developers/testers. I have also added two patches showing usage and benefits of the helper. My dirty/ad-hoc cocci scripts shows that this helper can be used in at least 2700 places saving about 3500 lines of code. Regards Andrzej Andrzej Hajda (4): driver core: add device probe log helper driver core: add deferring probe reason to devices_deferred property drm/bridge/sii8620: fix resource acquisition error handling drm/bridge: lvds-codec: simplify error handling drivers/base/base.h | 3 ++ drivers/base/core.c | 46 drivers/base/dd.c| 23 +- drivers/gpu/drm/bridge/lvds-codec.c | 10 ++ drivers/gpu/drm/bridge/sil-sii8620.c | 21 ++--- include/linux/device.h | 3 ++ 6 files changed, 86 insertions(+), 20 deletions(-) -- 2.17.1
[PATCH v9 2/4] driver core: add deferring probe reason to devices_deferred property
/sys/kernel/debug/devices_deferred property contains list of deferred devices.
This list does not contain reason why the driver deferred probe, the patch
improves it.
The natural place to set the reason is dev_err_probe function introduced
recently, ie. if dev_err_probe will be called with -EPROBE_DEFER instead of
printk the message will be attached to a deferred device and printed when user
reads devices_deferred property.
Signed-off-by: Andrzej Hajda
Reviewed-by: Mark Brown
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Andy Shevchenko
Reviewed-by: Rafael J. Wysocki
---
v9:
- fixed typo in function name
v8:
- improved commit message
---
drivers/base/base.h | 3 +++
drivers/base/core.c | 8 ++--
drivers/base/dd.c | 23 ++-
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index 95c22c0f9036..c3562adf4789 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -93,6 +93,7 @@ struct device_private {
struct klist_node knode_class;
struct list_head deferred_probe;
struct device_driver *async_driver;
+ char *deferred_probe_reason;
struct device *device;
u8 dead:1;
};
@@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device
*dev,
extern void driver_detach(struct device_driver *drv);
extern int driver_probe_device(struct device_driver *drv, struct device *dev);
extern void driver_deferred_probe_del(struct device *dev);
+extern void device_set_deferred_probe_reason(const struct device *dev,
+struct va_format *vaf);
static inline int driver_match_device(struct device_driver *drv,
struct device *dev)
{
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 3a827c82933f..d04d19458795 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
* This helper implements common pattern present in probe functions for error
* checking: print debug or error message depending if the error value is
* -EPROBE_DEFER and propagate error upwards.
+ * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
+ * checked later by reading devices_deferred debugfs attribute.
* It replaces code sequence:
* if (err != -EPROBE_DEFER)
* dev_err(dev, ...);
@@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err,
const char *fmt, ...)
vaf.fmt = fmt;
vaf.va =
- if (err != -EPROBE_DEFER)
+ if (err != -EPROBE_DEFER) {
dev_err(dev, "error %d: %pV", err, );
- else
+ } else {
+ device_set_deferred_probe_reason(dev, );
dev_dbg(dev, "error %d: %pV", err, );
+ }
va_end(args);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9a1d940342ac..7555b31bdfdc 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "base.h"
#include "power/power.h"
@@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
if (!list_empty(>p->deferred_probe)) {
dev_dbg(dev, "Removed from deferred list\n");
list_del_init(>p->deferred_probe);
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = NULL;
}
mutex_unlock(_probe_mutex);
}
@@ -211,6 +214,23 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+/**
+ * device_set_deferred_probe_reason() - Set defer probe reason message for
device
+ * @dev: the pointer to the struct device
+ * @vaf: the pointer to va_format structure with message
+ */
+void device_set_deferred_probe_reason(const struct device *dev, struct
va_format *vaf)
+{
+ const char *drv = dev_driver_string(dev);
+
+ mutex_lock(_probe_mutex);
+
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv,
vaf);
+
+ mutex_unlock(_probe_mutex);
+}
+
/*
* deferred_devs_show() - Show the devices in the deferred probe pending list.
*/
@@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, void
*data)
mutex_lock(_probe_mutex);
list_for_each_entry(curr, _probe_pending_list, deferred_probe)
- seq_printf(s, "%s\n", dev_name(curr->device));
+ seq_printf(s, "%s\t%s", dev_name(curr->device),
+ curr->device->p->deferred_probe_reason ?: "\n");
mutex_unlock(_probe_mutex);
--
2.17.1
[PATCH v9 4/4] drm/bridge: lvds-codec: simplify error handling
Using dev_err_probe code has following advantages:
- shorter code,
- recorded defer probe reason for debugging,
- uniform error code logging.
Signed-off-by: Andrzej Hajda
Reviewed-by: Neil Armstrong
---
drivers/gpu/drm/bridge/lvds-codec.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c
b/drivers/gpu/drm/bridge/lvds-codec.c
index 24fb1befdfa2..f19d9f7a5db2 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -71,13 +71,9 @@ static int lvds_codec_probe(struct platform_device *pdev)
lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
GPIOD_OUT_HIGH);
- if (IS_ERR(lvds_codec->powerdown_gpio)) {
- int err = PTR_ERR(lvds_codec->powerdown_gpio);
-
- if (err != -EPROBE_DEFER)
- dev_err(dev, "powerdown GPIO failure: %d\n", err);
- return err;
- }
+ if (IS_ERR(lvds_codec->powerdown_gpio))
+ return dev_err_probe(dev, PTR_ERR(lvds_codec->powerdown_gpio),
+"powerdown GPIO failure\n");
/* Locate the panel DT node. */
panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
--
2.17.1
[PATCH v9 3/4] drm/bridge/sii8620: fix resource acquisition error handling
In case of error during resource acquisition driver should print error
message only in case it is not deferred probe, using dev_err_probe helper
solves the issue. Moreover it records defer probe reason for debugging.
Signed-off-by: Andrzej Hajda
Reviewed-by: Neil Armstrong
---
drivers/gpu/drm/bridge/sil-sii8620.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
b/drivers/gpu/drm/bridge/sil-sii8620.c
index 92acd336aa89..389c1f029774 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2299,10 +2299,9 @@ static int sii8620_probe(struct i2c_client *client,
INIT_LIST_HEAD(>mt_queue);
ctx->clk_xtal = devm_clk_get(dev, "xtal");
- if (IS_ERR(ctx->clk_xtal)) {
- dev_err(dev, "failed to get xtal clock from DT\n");
- return PTR_ERR(ctx->clk_xtal);
- }
+ if (IS_ERR(ctx->clk_xtal))
+ return dev_err_probe(dev, PTR_ERR(ctx->clk_xtal),
+"failed to get xtal clock from DT\n");
if (!client->irq) {
dev_err(dev, "no irq provided\n");
@@ -2313,16 +2312,14 @@ static int sii8620_probe(struct i2c_client *client,
sii8620_irq_thread,
IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
"sii8620", ctx);
- if (ret < 0) {
- dev_err(dev, "failed to install IRQ handler\n");
- return ret;
- }
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+"failed to install IRQ handler\n");
ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
- if (IS_ERR(ctx->gpio_reset)) {
- dev_err(dev, "failed to get reset gpio from DT\n");
- return PTR_ERR(ctx->gpio_reset);
- }
+ if (IS_ERR(ctx->gpio_reset))
+ return dev_err_probe(dev, PTR_ERR(ctx->gpio_reset),
+"failed to get reset gpio from DT\n");
ctx->supplies[0].supply = "cvcc10";
ctx->supplies[1].supply = "iovcc18";
--
2.17.1
[PATCH v9 1/4] driver core: add device probe log helper
During probe every time driver gets resource it should usually check for
error printk some message if it is not -EPROBE_DEFER and return the error.
This pattern is simple but requires adding few lines after any resource
acquisition code, as a result it is often omitted or implemented only
partially.
dev_err_probe helps to replace such code sequences with simple call,
so code:
if (err != -EPROBE_DEFER)
dev_err(dev, ...);
return err;
becomes:
return dev_err_probe(dev, err, ...);
Signed-off-by: Andrzej Hajda
Reviewed-by: Rafael J. Wysocki
Reviewed-by: Mark Brown
---
drivers/base/core.c| 42 ++
include/linux/device.h | 3 +++
2 files changed, 45 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 67d39a90b45c..3a827c82933f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3953,6 +3953,48 @@ define_dev_printk_level(_dev_info, KERN_INFO);
#endif
+/**
+ * dev_err_probe - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print debug or error message depending if the error value is
+ * -EPROBE_DEFER and propagate error upwards.
+ * It replaces code sequence:
+ * if (err != -EPROBE_DEFER)
+ * dev_err(dev, ...);
+ * else
+ * dev_dbg(dev, ...);
+ * return err;
+ * with
+ * return dev_err_probe(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ va_start(args, fmt);
+ vaf.fmt = fmt;
+ vaf.va =
+
+ if (err != -EPROBE_DEFER)
+ dev_err(dev, "error %d: %pV", err, );
+ else
+ dev_dbg(dev, "error %d: %pV", err, );
+
+ va_end(args);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(dev_err_probe);
+
static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
{
return fwnode && !IS_ERR(fwnode->secondary);
diff --git a/include/linux/device.h b/include/linux/device.h
index 15460a5ac024..6b2272ae9af8 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device
*supplier);
void device_links_supplier_sync_state_pause(void);
void device_links_supplier_sync_state_resume(void);
+extern __printf(3, 4)
+int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+
/* Create alias, so I can be autoloaded. */
#define MODULE_ALIAS_CHARDEV(major,minor) \
MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
--
2.17.1
[PATCH v8 3/5] drm/bridge/sii8620: fix resource acquisition error handling
In case of error during resource acquisition driver should print error
message only in case it is not deferred probe, using dev_err_probe helper
solves the issue. Moreover it records defer probe reason for debugging.
Signed-off-by: Andrzej Hajda
Reviewed-by: Neil Armstrong
---
drivers/gpu/drm/bridge/sil-sii8620.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
b/drivers/gpu/drm/bridge/sil-sii8620.c
index 92acd336aa89..389c1f029774 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2299,10 +2299,9 @@ static int sii8620_probe(struct i2c_client *client,
INIT_LIST_HEAD(>mt_queue);
ctx->clk_xtal = devm_clk_get(dev, "xtal");
- if (IS_ERR(ctx->clk_xtal)) {
- dev_err(dev, "failed to get xtal clock from DT\n");
- return PTR_ERR(ctx->clk_xtal);
- }
+ if (IS_ERR(ctx->clk_xtal))
+ return dev_err_probe(dev, PTR_ERR(ctx->clk_xtal),
+"failed to get xtal clock from DT\n");
if (!client->irq) {
dev_err(dev, "no irq provided\n");
@@ -2313,16 +2312,14 @@ static int sii8620_probe(struct i2c_client *client,
sii8620_irq_thread,
IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
"sii8620", ctx);
- if (ret < 0) {
- dev_err(dev, "failed to install IRQ handler\n");
- return ret;
- }
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+"failed to install IRQ handler\n");
ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
- if (IS_ERR(ctx->gpio_reset)) {
- dev_err(dev, "failed to get reset gpio from DT\n");
- return PTR_ERR(ctx->gpio_reset);
- }
+ if (IS_ERR(ctx->gpio_reset))
+ return dev_err_probe(dev, PTR_ERR(ctx->gpio_reset),
+"failed to get reset gpio from DT\n");
ctx->supplies[0].supply = "cvcc10";
ctx->supplies[1].supply = "iovcc18";
--
2.17.1
[PATCH v8 4/5] drm/bridge: lvds-codec: simplify error handling
Using dev_err_probe code has following advantages:
- shorter code,
- recorded defer probe reason for debugging,
- uniform error code logging.
Signed-off-by: Andrzej Hajda
Reviewed-by: Neil Armstrong
---
drivers/gpu/drm/bridge/lvds-codec.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c
b/drivers/gpu/drm/bridge/lvds-codec.c
index 24fb1befdfa2..f19d9f7a5db2 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -71,13 +71,9 @@ static int lvds_codec_probe(struct platform_device *pdev)
lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
GPIOD_OUT_HIGH);
- if (IS_ERR(lvds_codec->powerdown_gpio)) {
- int err = PTR_ERR(lvds_codec->powerdown_gpio);
-
- if (err != -EPROBE_DEFER)
- dev_err(dev, "powerdown GPIO failure: %d\n", err);
- return err;
- }
+ if (IS_ERR(lvds_codec->powerdown_gpio))
+ return dev_err_probe(dev, PTR_ERR(lvds_codec->powerdown_gpio),
+"powerdown GPIO failure\n");
/* Locate the panel DT node. */
panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
--
2.17.1
[PATCH v8 2/5] driver core: add deferring probe reason to devices_deferred property
/sys/kernel/debug/devices_deferred property contains list of deferred devices.
This list does not contain reason why the driver deferred probe, the patch
improves it.
The natural place to set the reason is dev_err_probe function introduced
recently, ie. if dev_err_probe will be called with -EPROBE_DEFER instead of
printk the message will be attached to a deferred device and printed when user
reads devices_deferred property.
Signed-off-by: Andrzej Hajda
Reviewed-by: Mark Brown
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Andy Shevchenko
Reviewed-by: Rafael J. Wysocki
---
v8:
- improved commit message
---
drivers/base/base.h | 3 +++
drivers/base/core.c | 8 ++--
drivers/base/dd.c | 23 ++-
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index 95c22c0f9036..6954fccab3d7 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -93,6 +93,7 @@ struct device_private {
struct klist_node knode_class;
struct list_head deferred_probe;
struct device_driver *async_driver;
+ char *deferred_probe_reason;
struct device *device;
u8 dead:1;
};
@@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device
*dev,
extern void driver_detach(struct device_driver *drv);
extern int driver_probe_device(struct device_driver *drv, struct device *dev);
extern void driver_deferred_probe_del(struct device *dev);
+extern void device_set_deferred_probe_reson(const struct device *dev,
+ struct va_format *vaf);
static inline int driver_match_device(struct device_driver *drv,
struct device *dev)
{
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 3a827c82933f..fee047f03681 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
* This helper implements common pattern present in probe functions for error
* checking: print debug or error message depending if the error value is
* -EPROBE_DEFER and propagate error upwards.
+ * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
+ * checked later by reading devices_deferred debugfs attribute.
* It replaces code sequence:
* if (err != -EPROBE_DEFER)
* dev_err(dev, ...);
@@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err,
const char *fmt, ...)
vaf.fmt = fmt;
vaf.va =
- if (err != -EPROBE_DEFER)
+ if (err != -EPROBE_DEFER) {
dev_err(dev, "error %d: %pV", err, );
- else
+ } else {
+ device_set_deferred_probe_reson(dev, );
dev_dbg(dev, "error %d: %pV", err, );
+ }
va_end(args);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9a1d940342ac..dd5683b61f74 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "base.h"
#include "power/power.h"
@@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
if (!list_empty(>p->deferred_probe)) {
dev_dbg(dev, "Removed from deferred list\n");
list_del_init(>p->deferred_probe);
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = NULL;
}
mutex_unlock(_probe_mutex);
}
@@ -211,6 +214,23 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+/**
+ * device_set_deferred_probe_reson() - Set defer probe reason message for
device
+ * @dev: the pointer to the struct device
+ * @vaf: the pointer to va_format structure with message
+ */
+void device_set_deferred_probe_reson(const struct device *dev, struct
va_format *vaf)
+{
+ const char *drv = dev_driver_string(dev);
+
+ mutex_lock(_probe_mutex);
+
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv,
vaf);
+
+ mutex_unlock(_probe_mutex);
+}
+
/*
* deferred_devs_show() - Show the devices in the deferred probe pending list.
*/
@@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, void
*data)
mutex_lock(_probe_mutex);
list_for_each_entry(curr, _probe_pending_list, deferred_probe)
- seq_printf(s, "%s\n", dev_name(curr->device));
+ seq_printf(s, "%s\t%s", dev_name(curr->device),
+ curr->device->p->deferred_probe_reason ?: "\n");
mutex_unlock(_probe_mutex);
--
2.17.1
[PATCH v8 5/5] coccinelle: add script looking for cases where probe__err can be used
This is AD-HOC script, it should nt be merged.
Signed-off-by: Andrzej Hajda
---
probe_err.cocci | 247
1 file changed, 247 insertions(+)
create mode 100644 probe_err.cocci
diff --git a/probe_err.cocci b/probe_err.cocci
new file mode 100644
index ..0ef9a9b4c9bc
--- /dev/null
+++ b/probe_err.cocci
@@ -0,0 +1,247 @@
+virtual context
+virtual patch
+
+@initialize:python@
+@@
+
+import re
+
+@@
+expression err, dev;
+constant char [] fmt;
+expression list args;
+@@
+
+- if (err != -EPROBE_DEFER) { dev_err(dev, fmt, args); }
++ dev_err_probe(dev, err, fmt, args);
+
+@@
+expression ptr, dev;
+constant char [] fmt;
+expression list args;
+@@
+
+- if (ptr != ERR_PTR(-EPROBE_DEFER)) { dev_err(dev, fmt, args); }
++ dev_err_probe(dev, PTR_ERR(ptr), fmt, args);
+
+@@
+expression e, dev;
+identifier err;
+identifier fget =~
"^(devm_)?(clk_get|gpiod_get|gpiod_get_optional|gpiod_get_index|gpiod_get_index_optional|regulator_get|regulator_get_optional|reset_control_get|reset_control_get_exclusive|reset_control_get_shared|phy_get|pinctrl_get|iio_channel_get|pwm_get)$";
+identifier flog =~ "^(dev_err|dev_warn|dev_info)$";
+expression list args;
+@@
+e = fget(...);
+if (IS_ERR(e)) {
+(
+ err = PTR_ERR(e);
+- flog(dev, args);
++ dev_err_probe(dev, err, args);
+|
+- flog(dev, args);
++ dev_err_probe(dev, PTR_ERR(e), args);
+)
+ ...
+}
+
+@@
+expression dev;
+identifier err;
+identifier fget =~
"^(devm_)?(request_irq|request_threaded_irq|regulator_bulk_get)$";
+identifier flog =~ "^(dev_err|dev_warn|dev_info)$";
+expression list args;
+@@
+err = fget(...);
+if ( \( err \| err < 0 \) ) {
+ ...
+- flog(dev, args);
++ dev_err_probe(dev, err, args);
+ ...
+}
+
+@catch_no_nl@
+expression dev, err;
+constant char [] fmt !~ "\\n$";
+@@
+dev_err_probe(dev, err, fmt, ...)
+
+@script:python add_nl depends on catch_no_nl@
+fmt << catch_no_nl.fmt;
+nfmt;
+@@
+print "add_nl " + fmt
+coccinelle.nfmt = fmt[:-1] + '\\n"';
+
+@fix_no_nl depends on catch_no_nl@
+constant char [] catch_no_nl.fmt;
+identifier add_nl.nfmt;
+@@
+- fmt
++ nfmt
+
+@catch_fmt@
+expression err, dev;
+expression fmt;
+position p;
+@@
+
+dev_err_probe@p(dev, err, fmt, ..., \( (int)err \| err \) )
+
+@script:python trim_fmt@
+fmt << catch_fmt.fmt;
+new_fmt;
+@@
+
+tmp = fmt
+tmp = re.sub('failed: irq request (IRQ: %d, error :%d)', 'irq request %d', tmp)
+tmp = re.sub('Error %l?[di] ', 'Error ', tmp)
+tmp = re.sub(' as irq = %dn', ', bad irqn', tmp)
+tmp = re.sub('[:,]? ?((ret|err|with|error)[ =]?)?%l?[di]\.?n', 'n',
tmp)
+tmp = re.sub(' ?\(((err|ret|error)\s*=?\s*)?%l?[diu]\)[!.]?n', 'n',
tmp)
+
+assert tmp != fmt, "cannot trim_fmt in: " + fmt
+print "trim_fmt " + fmt + " " + tmp
+coccinelle.new_fmt = tmp
+
+@fix_fmt@
+expression err, err1, dev;
+expression fmt;
+expression list l;
+identifier trim_fmt.new_fmt;
+position catch_fmt.p;
+@@
+
+- dev_err_probe@p(dev, err, fmt, l, err1)
++ dev_err_probe(dev, err, new_fmt, l)
+
+@err_ass1@
+identifier err;
+expression dev, ptr;
+expression list args;
+@@
+
+- err = PTR_ERR(ptr);
+- dev_err_probe(dev, err, args);
+- return ERR_PTR(err);
++ dev_err_probe(dev, PTR_ERR(ptr), args);
++ return ERR_CAST(ptr);
+
+@err_ass2@
+identifier err, f1, f2;
+expression dev, e;
+expression list args;
+@@
+- err = PTR_ERR(e);
+- dev_err_probe(dev, err, args);
+(
+|
+f1(...);
+|
+f1(...);
+f2(...);
+)
+- return err;
++ return dev_err_probe(dev, PTR_ERR(e), args);
+
+@@
+identifier err;
+expression dev, e;
+expression list args;
+@@
+
+- int err = e;
+- dev_err_probe(dev, err, args);
+- return err;
++ return dev_err_probe(dev, e, args);
+
+@@
+expression err, dev;
+expression list args;
+@@
+
+- dev_err_probe(dev, err, args);
+- return err;
++ return dev_err_probe(dev, err, args);
+
+@@
+expression err, dev, ptr;
+expression list args;
+@@
+
+- dev_err_probe(dev, PTR_ERR(ptr), args);
+err = PTR_ERR(ptr);
++ dev_err_probe(dev, err, args);
+
+@@
+expression e;
+expression list args;
+statement s, s1;
+@@
+
+// without s1 spatch generates extra empty line after s
+- if (e) { return dev_err_probe(args); } else s s1
++ if (e) return dev_err_probe(args); s s1
+
+@@
+expression e;
+expression list args;
+@@
+
+- if (e) { return dev_err_probe(args); }
++ if (e) return dev_err_probe(args);
+
+@@
+expression e, s, v;
+expression list args;
+@@
+
+- if (e == v) { s; } else { return dev_err_probe(args); }
++ if (e != v) return dev_err_probe(args); s;
+
+@err_ass3@
+identifier err;
+expression dev, ptr;
+expression list args;
+@@
+
+- err = PTR_ERR_OR_ZERO(ptr);
+- if (err) return dev_err_probe(dev, err, args);
++ if (IS_ERR(ptr)) return dev_err_probe(dev, PTR_ERR(ptr), args);
+
+@@
+expressi
[PATCH v8 1/5] driver core: add device probe log helper
During probe every time driver gets resource it should usually check for
error printk some message if it is not -EPROBE_DEFER and return the error.
This pattern is simple but requires adding few lines after any resource
acquisition code, as a result it is often omitted or implemented only
partially.
dev_err_probe helps to replace such code sequences with simple call,
so code:
if (err != -EPROBE_DEFER)
dev_err(dev, ...);
return err;
becomes:
return dev_err_probe(dev, err, ...);
Signed-off-by: Andrzej Hajda
Reviewed-by: Rafael J. Wysocki
Reviewed-by: Mark Brown
---
drivers/base/core.c| 42 ++
include/linux/device.h | 3 +++
2 files changed, 45 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 67d39a90b45c..3a827c82933f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3953,6 +3953,48 @@ define_dev_printk_level(_dev_info, KERN_INFO);
#endif
+/**
+ * dev_err_probe - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print debug or error message depending if the error value is
+ * -EPROBE_DEFER and propagate error upwards.
+ * It replaces code sequence:
+ * if (err != -EPROBE_DEFER)
+ * dev_err(dev, ...);
+ * else
+ * dev_dbg(dev, ...);
+ * return err;
+ * with
+ * return dev_err_probe(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ va_start(args, fmt);
+ vaf.fmt = fmt;
+ vaf.va =
+
+ if (err != -EPROBE_DEFER)
+ dev_err(dev, "error %d: %pV", err, );
+ else
+ dev_dbg(dev, "error %d: %pV", err, );
+
+ va_end(args);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(dev_err_probe);
+
static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
{
return fwnode && !IS_ERR(fwnode->secondary);
diff --git a/include/linux/device.h b/include/linux/device.h
index 15460a5ac024..6b2272ae9af8 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device
*supplier);
void device_links_supplier_sync_state_pause(void);
void device_links_supplier_sync_state_resume(void);
+extern __printf(3, 4)
+int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+
/* Create alias, so I can be autoloaded. */
#define MODULE_ALIAS_CHARDEV(major,minor) \
MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
--
2.17.1
[PATCH v8 0/5] driver core: add probe error check helper
Hi All, Thanks for comments. Changes since v7: - improved commit message - added R-Bs Changes since v6: - removed leftovers from old naming scheme in commit descritions, - added R-Bs. Changes since v5: - removed patch adding macro, dev_err_probe(dev, PTR_ERR(ptr), ...) should be used instead, - added dev_dbg logging in case of -EPROBE_DEFER, - renamed functions and vars according to comments, - extended docs, - cosmetics. Original message (with small adjustments): Recently I took some time to re-check error handling in drivers probe code, and I have noticed that number of incorrect resource acquisition error handling increased and there are no other propositions which can cure the situation. So I have decided to resend my old proposition of probe_err helper which should simplify resource acquisition error handling, it also extend it with adding defer probe reason to devices_deferred debugfs property, which should improve debugging experience for developers/testers. I have also added two patches showing usage and benefits of the helper. My dirty/ad-hoc cocci scripts shows that this helper can be used in at least 2700 places saving about 3500 lines of code. Regards Andrzej Andrzej Hajda (5): driver core: add device probe log helper driver core: add deferring probe reason to devices_deferred property drm/bridge/sii8620: fix resource acquisition error handling drm/bridge: lvds-codec: simplify error handling coccinelle: add script looking for cases where probe__err can be used drivers/base/base.h | 3 + drivers/base/core.c | 46 + drivers/base/dd.c| 23 ++- drivers/gpu/drm/bridge/lvds-codec.c | 10 +- drivers/gpu/drm/bridge/sil-sii8620.c | 21 +-- include/linux/device.h | 3 + probe_err.cocci | 247 +++ 7 files changed, 333 insertions(+), 20 deletions(-) create mode 100644 probe_err.cocci -- 2.17.1
Re: [PATCH v8 2/5] driver core: add deferring probe reason to devices_deferred property
On 10.07.2020 15:31, Greg Kroah-Hartman wrote: > On Thu, Jul 02, 2020 at 03:44:21PM +0200, Andrzej Hajda wrote: >> /sys/kernel/debug/devices_deferred property contains list of deferred >> devices. >> This list does not contain reason why the driver deferred probe, the patch >> improves it. >> The natural place to set the reason is dev_err_probe function introduced >> recently, ie. if dev_err_probe will be called with -EPROBE_DEFER instead of >> printk the message will be attached to a deferred device and printed when >> user >> reads devices_deferred property. >> >> Signed-off-by: Andrzej Hajda >> Reviewed-by: Mark Brown >> Reviewed-by: Javier Martinez Canillas >> Reviewed-by: Andy Shevchenko >> Reviewed-by: Rafael J. Wysocki >> --- >> v8: >> - improved commit message > I'm totally confused by this series. Can you resend the whole thing, > as a full series, not just random individual patches in the series > incremented? It's a pain to try to fish them all out as to which is the > "latest" with all of the needed reviewed by lines :( v7 is the latest except this one,which contains only commit message change. Anyway I will send v8 to make things simple. Regards Andrzej > > thanks, > > greg k-h > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://protect2.fireeye.com/v1/url?k=563dadd0-0bf16175-563c269f-0cc47a30d446-7237066d193b28b5=1=54779b9e-347e-4d0c-9845-da31d4cce7e4=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel >
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
On 07.07.2020 06:14, Dmitry Torokhov wrote:
> On Thu, Jul 02, 2020 at 08:57:55AM +0200, Andrzej Hajda wrote:
>> On 30.06.2020 20:00, Dmitry Torokhov wrote:
>>> On Tue, Jun 30, 2020 at 8:42 AM Andrzej Hajda wrote:
>>>> On 30.06.2020 10:59, Grygorii Strashko wrote:
>>>>> Hi
>>>>>
>>>>> On 29/06/2020 14:28, Andrzej Hajda wrote:
>>>>>> Hi Grygorii,
>>>>>>
>>>>>> (...)
>>>>>>
>>>>>>>> /*
>>>>>>>> * deferred_devs_show() - Show the devices in the deferred probe
>>>>>>>> pending list.
>>>>>>>> */
>>>>>>>> @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s,
>>>>>>>> void *data)
>>>>>>>> mutex_lock(_probe_mutex);
>>>>>>>>list_for_each_entry(curr, _probe_pending_list,
>>>>>>>> deferred_probe)
>>>>>>>> -seq_printf(s, "%s\n", dev_name(curr->device));
>>>>>>>> +seq_printf(s, "%s\t%s", dev_name(curr->device),
>>>>>>>> + curr->device->p->deferred_probe_reason ?: "\n");
>>>>>>>>mutex_unlock(_probe_mutex);
>>>>>>>>
>>>>>>> Sry, may be i missing smth, but shouldn't it be optional
>>>>>>> (CONFIG_DEBUG_FS is probably too generic).
>>>>>>>
>>>>>> I am not sure what exactly are you referring to, but this patch does not
>>>>>> add new property, it just extends functionality of existing one.
>>>>> Sry, needed to be more specific.
>>>>>
>>>>> You've added device_set_deferred_probe_reson(dev, );
>>>>> which expected to be used on every EPROBE_DEFER in dev_err_probe() in
>>>>> combination with
>>>>>
>>>>> + } else {
>>>>> + device_set_deferred_probe_reson(dev, );
>>>>> dev_dbg(dev, "error %d: %pV", err, );
>>>>>
>>>>> ^^ dev_dbg() does not add any runtime overhead during boot unless enabled
>>>>> + }
>>>>>
>>>>> But:
>>>>>
>>>>> +void device_set_deferred_probe_reson(const struct device *dev, struct
>>>>> va_format *vaf)
>>>>> +{
>>>>> + const char *drv = dev_driver_string(dev);
>>>>> +
>>>>> + mutex_lock(_probe_mutex);
>>>>> +
>>>>> + kfree(dev->p->deferred_probe_reason);
>>>>> + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s:
>>>>> %pV", drv, vaf);
>>>>> +
>>>>> + mutex_unlock(_probe_mutex);
>>>>> +}
>>>>>
>>>>> ^^ Adds locking, kfree() and kasprintf() for every deferred probe
>>>>> during boot and can't be disabled.
>>>>>
>>>>> Right?
>>>> Right, but usually the burden should be insignificant in comparison to
>>>> probe time, so I do not think it is worth optimizing.
>>> I do not think this is going to take. You are suggesting that we
>>> modify pretty much every driver to supply this deferral reason, and I
>>> doubt it will happen. Can we put this burden on providers that raise
>>> the deferral?
>>
>> I wouldn't say they raise the deferral, they just inform resource is not
>> yet available. Only device driver, and only in its probe function can
>> "raise the deferral".
> Well, this is a matter of perspective. If devm_gpiod_get() returns
> -EBUSY and this is returned to driver core, is it GPIO line signals that
> line is busy, or is it the driver applies its knowledge. I say that in
> majority of cases driver does not really get a say in this and simply
> has to pass whatever error condition that is signalled by providers up
> the stack.
>
> I would consider whenever a driver does not propagate -EPROBE_DEFER to
> the driver code a bug that needs fixing, because it should not degrade
> functionality and/or performance just because we have not figured out
> how to order probing properly and have to rely on deferrals.
>
>>
>>>I.e. majority of code are using devm API now, so we most
>>> likely know the device for which deferral is being raised. We can have
>>&
Re: [PATCH v7 2/2] display/drm/bridge: TC358775 DSI/LVDS driver
On 07.07.2020 10:25, Vinay Simha B N wrote: > Andrzej, > > Please suggest. > > In general it should be in the reverse-order RESX then STBY, But > As per the spec Datasheet Power off sequence is this Page 149, Section > Power Supply On and Off Sequence > > regulators > STBY > RESX > > https://www.google.com/url?sa=t=j==s=web==2ahUKEwiBmeWb2brqAhXO7HMBHZgaCTUQFjACegQIBxAB=https%3A%2F%2Fdownload.t-firefly.com%2Fproduct%2FRK3399%2FDocs%2FChip%2520Specifications%2FTC358774XBG_75XBG_V1%25204nm.pdf=AOvVaw2kBuPv8FaZBNynGWCQHEfc > > <https://protect2.fireeye.com/v1/url?k=fc81046b-a14dcfaf-fc808f24-0cc47a314e9a-260fe757996adc0f=1=c54ac15e-86b9-4dd9-b17e-762132071a25=https%3A%2F%2Fwww.google.com%2Furl%3Fsa%3Dt%26rct%3Dj%26q%3D%26esrc%3Ds%26source%3Dweb%26cd%3D%26ved%3D2ahUKEwiBmeWb2brqAhXO7HMBHZgaCTUQFjACegQIBxAB%26url%3Dhttps%253A%252F%252Fdownload.t-firefly.com%252Fproduct%252FRK3399%252FDocs%252FChip%252520Specifications%252FTC358774XBG_75XBG_V1%2525204nm.pdf%26usg%3DAOvVaw2kBuPv8FaZBNynGWCQHEfc> > I guess you misread the diagram, it should be read from left to right(not top-bottom), and you have power off sequence: RESX STDBY VDDC VDD_LVDS VDDIO > Regarding data-lanes > -data-lanes value does appear later from the mdp->dsi0 tree > -We need to pick dynamically data-lanes of the dsi set, based on this > we need to set in the bridge. > Otherwise we are already setting in dsi0 ports as <0 1 2 3> , again we > need to set it in the bridge tree. > - There is no helper function to get the data-lanes of the DSI The code asks for proper helper, but since there is no such I think it can stay as is. Regards Andrzej > > On Tue, Jul 7, 2020 at 12:15 PM Andrzej Hajda <mailto:a.ha...@samsung.com>> wrote: > > > On 04.07.2020 11:24, Vinay Simha BN wrote: > > This driver is tested with two panels individually with > Apq8016-IFC6309 board > > > > https://protect2.fireeye.com/url?k=fe87a8ec-a3e0ecca-fe8623a3-0cc47a31384a-ffbc547df1141490=1=https%3A%2F%2Fwww.inforcecomputing.com%2Fproducts%2Fsingle-board-computers-sbc%2Fqualcomm-snapdragon-410-inforce-6309-micro-sbc > > > > 1. 1366x768@60 auo,b101xtn01 data-mapping = "jeida-24" > > 2. 800x480@60 innolux,at070tn92 data-mapping = "vesa-24" > > > > - added SPDX identifier license > > - updated alphabetic order of headers > > - replaced u32 instead of uint32_t > > - magic number to macros for CLRSI and mux registers > > - mdelay to usleep_range > > - added bus_formats > > - removed drm_connector_status > > - regulator enable and disable with proper orders and delays > > as per the spec > > - devm_drm_panel_bridge_add method used instead of panel > > description modified > > - dual port implemented > > - panel->connector_type removed > > - ~vsdelay dynamic value set based on the > > calculation of dsi speed, output speed, blanking > > - help modified > > - display_timings naming local variables > > - check for bus_formats unsupported > > - error handling enpoint data-lanes > > - Kconfig proper indentation > > - GENMASK and FIELD_PREP used > > - bus_formats handeld in mode_valid > > - MODE_CLOCK_HIGH handled properly > > - len initialized > > - static function for mode_valid > > > > Signed-off-by: Vinay Simha BN <mailto:simha...@gmail.com>> > > --- > > v1: > > Initial version > > > > v2: > > * Andrzej Hajda review comments incorporated > > SPDX identifier > > development debug removed > > alphabetic order headers > > u32 instead of unit32_t > > magic numbers to macros for CLRSI and mux registers > > ignored return value > > > > * Laurent Pinchart review comments incorporated > > mdelay to usleep_range > > bus_formats added > > > > v3: > > * Andrzej Hajda review comments incorporated > > drm_connector_status removed > > u32 rev removed and local variabl is used > > regulator enable disable with proper orders and delays > > as per the spec > > devm_drm_panel_bridge_add method used instead of panel > > description modified > > dual port implemented > > > > v4: > > * Sam Ravnborg review comments incorporated > > panel->connector_type removed > > > > * Reported-by: kernel test robot <mailto
Re: [PATCH v7 08/36] drm: exynos: fix common struct sg_table related issues
On 07.07.2020 11:40, Andrzej Hajda wrote:
> On 19.06.2020 12:36, Marek Szyprowski wrote:
>> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
>> returns the number of the created entries in the DMA address space.
>> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
>> dma_unmap_sg must be called with the original number of the entries
>> passed to the dma_map_sg().
>>
>> struct sg_table is a common structure used for describing a non-contiguous
>> memory buffer, used commonly in the DRM and graphics subsystems. It
>> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
>> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
>> and DMA mapped pages (nents entry).
>>
>> It turned out that it was a common mistake to misuse nents and orig_nents
>> entries, calling DMA-mapping functions with a wrong number of entries or
>> ignoring the number of mapped entries returned by the dma_map_sg()
>> function.
>>
>> To avoid such issues, lets use a common dma-mapping wrappers operating
>> directly on the struct sg_table objects and use scatterlist page
>> iterators where possible. This, almost always, hides references to the
>> nents and orig_nents entries, making the code robust, easier to follow
>> and copy/paste safe.
>>
>> Signed-off-by: Marek Szyprowski
>
Just fixing my signature :)
Reviewed-by: Andrzej Hajda
Regards
Andrzej
Re: [PATCH v7 07/36] drm: exynos: use common helper for a scatterlist contiguity check
On 07.07.2020 11:35, Andrzej Hajda wrote: > Hi, > > On 19.06.2020 12:36, Marek Szyprowski wrote: >> Use common helper for checking the contiguity of the imported dma-buf. >> >> Signed-off-by: Marek Szyprowski Just fixing my signature :) Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [PATCH v7 01/36] drm: prime: add common helper to check scatterlist contiguity
On 07.07.2020 16:30, Andrzej Hajda wrote: > On 19.06.2020 12:36, Marek Szyprowski wrote: >> It is a common operation done by DRM drivers to check the contiguity >> of the DMA-mapped buffer described by a scatterlist in the >> sg_table object. Let's add a common helper for this operation. >> >> Signed-off-by: Marek Szyprowski >> --- Just fixing my signature :) Reviewed-by: Andrzej Hajda Regards Andrzej
Re: [PATCH v7 03/36] drm: core: fix common struct sg_table related issues
On 19.06.2020 12:36, Marek Szyprowski wrote:
> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
> returns the number of the created entries in the DMA address space.
> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
> dma_unmap_sg must be called with the original number of the entries
> passed to the dma_map_sg().
>
> struct sg_table is a common structure used for describing a non-contiguous
> memory buffer, used commonly in the DRM and graphics subsystems. It
> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
> and DMA mapped pages (nents entry).
>
> It turned out that it was a common mistake to misuse nents and orig_nents
> entries, calling DMA-mapping functions with a wrong number of entries or
> ignoring the number of mapped entries returned by the dma_map_sg()
> function.
>
> To avoid such issues, lets use a common dma-mapping wrappers operating
> directly on the struct sg_table objects and use scatterlist page
> iterators where possible. This, almost always, hides references to the
> nents and orig_nents entries, making the code robust, easier to follow
> and copy/paste safe.
>
> Signed-off-by: Marek Szyprowski
I guess whole patchset can go via drm-misc, after r-b/a-b.
Reviewed-by: Andrzej Hajda
Regards
Andrzej
> ---
> drivers/gpu/drm/drm_cache.c| 2 +-
> drivers/gpu/drm/drm_gem_shmem_helper.c | 14 +-
> drivers/gpu/drm/drm_prime.c| 11 ++-
> 3 files changed, 16 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_cache.c b/drivers/gpu/drm/drm_cache.c
> index 03e01b000f7a..0fe3c496002a 100644
> --- a/drivers/gpu/drm/drm_cache.c
> +++ b/drivers/gpu/drm/drm_cache.c
> @@ -127,7 +127,7 @@ drm_clflush_sg(struct sg_table *st)
> struct sg_page_iter sg_iter;
>
> mb(); /*CLFLUSH is ordered only by using memory barriers*/
> - for_each_sg_page(st->sgl, _iter, st->nents, 0)
> + for_each_sgtable_page(st, _iter, 0)
> drm_clflush_page(sg_page_iter_page(_iter));
> mb(); /*Make sure that all cache line entry is flushed*/
>
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c
> b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 4b7cfbac4daa..47d8211221f2 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -126,8 +126,8 @@ void drm_gem_shmem_free_object(struct drm_gem_object *obj)
> drm_prime_gem_destroy(obj, shmem->sgt);
> } else {
> if (shmem->sgt) {
> - dma_unmap_sg(obj->dev->dev, shmem->sgt->sgl,
> - shmem->sgt->nents, DMA_BIDIRECTIONAL);
> + dma_unmap_sgtable(obj->dev->dev, shmem->sgt,
> + DMA_BIDIRECTIONAL, 0);
> sg_free_table(shmem->sgt);
> kfree(shmem->sgt);
> }
> @@ -424,8 +424,7 @@ void drm_gem_shmem_purge_locked(struct drm_gem_object
> *obj)
>
> WARN_ON(!drm_gem_shmem_is_purgeable(shmem));
>
> - dma_unmap_sg(obj->dev->dev, shmem->sgt->sgl,
> - shmem->sgt->nents, DMA_BIDIRECTIONAL);
> + dma_unmap_sgtable(obj->dev->dev, shmem->sgt, DMA_BIDIRECTIONAL, 0);
> sg_free_table(shmem->sgt);
> kfree(shmem->sgt);
> shmem->sgt = NULL;
> @@ -697,12 +696,17 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct
> drm_gem_object *obj)
> goto err_put_pages;
> }
> /* Map the pages for use by the h/w. */
> - dma_map_sg(obj->dev->dev, sgt->sgl, sgt->nents, DMA_BIDIRECTIONAL);
> + ret = dma_map_sgtable(obj->dev->dev, sgt, DMA_BIDIRECTIONAL, 0);
> + if (ret)
> + goto err_free_sgt;
>
> shmem->sgt = sgt;
>
> return sgt;
>
> +err_free_sgt:
> + sg_free_table(sgt);
> + kfree(sgt);
> err_put_pages:
> drm_gem_shmem_put_pages(shmem);
> return ERR_PTR(ret);
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index b717e52e909e..d583d6545666 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -617,6 +617,7 @@ struct sg_table *drm_gem_map_dma_buf(struct
> dma_buf_attachment *attach,
> {
> struct drm_gem_object *obj = attach->dmabuf->priv;
> struct sg_table *sgt;
> + int ret;
>
> if (WARN_ON(dir == DM
Re: [PATCH v7 02/36] drm: prime: use sgtable iterators in drm_prime_sg_to_page_addr_arrays()
On 19.06.2020 12:36, Marek Szyprowski wrote:
> Replace the current hand-crafted code for extracting pages and DMA
> addresses from the given scatterlist by the much more robust
> code based on the generic scatterlist iterators and recently
> introduced sg_table-based wrappers. The resulting code is simple and
> easy to understand, so the comment describing the old code is no
> longer needed.
>
> Signed-off-by: Marek Szyprowski
Nice simplification.
Reviewed-by: Andrzej Hajda
Btw, I've wrongly re-configured my e-mail client, so my R-Bs for other
patches are little bit broken, I will resend them :)
Regards
Andrzej
> ---
> drivers/gpu/drm/drm_prime.c | 49 -
> 1 file changed, 15 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 226cd6ad3985..b717e52e909e 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -990,45 +990,26 @@ EXPORT_SYMBOL(drm_gem_prime_import);
> int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page
> **pages,
>dma_addr_t *addrs, int max_entries)
> {
> - unsigned count;
> - struct scatterlist *sg;
> - struct page *page;
> - u32 page_len, page_index;
> - dma_addr_t addr;
> - u32 dma_len, dma_index;
> -
> - /*
> - * Scatterlist elements contains both pages and DMA addresses, but
> - * one shoud not assume 1:1 relation between them. The sg->length is
> - * the size of the physical memory chunk described by the sg->page,
> - * while sg_dma_len(sg) is the size of the DMA (IO virtual) chunk
> - * described by the sg_dma_address(sg).
> - */
> - page_index = 0;
> - dma_index = 0;
> - for_each_sg(sgt->sgl, sg, sgt->nents, count) {
> - page_len = sg->length;
> - page = sg_page(sg);
> - dma_len = sg_dma_len(sg);
> - addr = sg_dma_address(sg);
> -
> - while (pages && page_len > 0) {
> - if (WARN_ON(page_index >= max_entries))
> + struct sg_dma_page_iter dma_iter;
> + struct sg_page_iter page_iter;
> + struct page **p = pages;
> + dma_addr_t *a = addrs;
> +
> + if (pages) {
> + for_each_sgtable_page(sgt, _iter, 0) {
> + if (p - pages >= max_entries)
> return -1;
> - pages[page_index] = page;
> - page++;
> - page_len -= PAGE_SIZE;
> - page_index++;
> + *p++ = sg_page_iter_page(_iter);
> }
> - while (addrs && dma_len > 0) {
> - if (WARN_ON(dma_index >= max_entries))
> + }
> + if (addrs) {
> + for_each_sgtable_dma_page(sgt, _iter, 0) {
> + if (a - addrs >= max_entries)
> return -1;
> - addrs[dma_index] = addr;
> - addr += PAGE_SIZE;
> - dma_len -= PAGE_SIZE;
> - dma_index++;
> + *a++ = sg_page_iter_dma_address(_iter);
> }
> }
> +
> return 0;
> }
> EXPORT_SYMBOL(drm_prime_sg_to_page_addr_arrays);
Re: [PATCH v7 01/36] drm: prime: add common helper to check scatterlist contiguity
On 19.06.2020 12:36, Marek Szyprowski wrote:
> It is a common operation done by DRM drivers to check the contiguity
> of the DMA-mapped buffer described by a scatterlist in the
> sg_table object. Let's add a common helper for this operation.
>
> Signed-off-by: Marek Szyprowski
> ---
> drivers/gpu/drm/drm_gem_cma_helper.c | 23 +++--
> drivers/gpu/drm/drm_prime.c | 31
> include/drm/drm_prime.h | 2 ++
> 3 files changed, 36 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
> b/drivers/gpu/drm/drm_gem_cma_helper.c
> index 06a5b9ee1fe0..41566a15dabd 100644
> --- a/drivers/gpu/drm/drm_gem_cma_helper.c
> +++ b/drivers/gpu/drm/drm_gem_cma_helper.c
> @@ -471,26 +471,9 @@ drm_gem_cma_prime_import_sg_table(struct drm_device *dev,
> {
> struct drm_gem_cma_object *cma_obj;
>
> - if (sgt->nents != 1) {
> - /* check if the entries in the sg_table are contiguous */
> - dma_addr_t next_addr = sg_dma_address(sgt->sgl);
> - struct scatterlist *s;
> - unsigned int i;
> -
> - for_each_sg(sgt->sgl, s, sgt->nents, i) {
> - /*
> - * sg_dma_address(s) is only valid for entries
> - * that have sg_dma_len(s) != 0
> - */
> - if (!sg_dma_len(s))
> - continue;
> -
> - if (sg_dma_address(s) != next_addr)
> - return ERR_PTR(-EINVAL);
> -
> - next_addr = sg_dma_address(s) + sg_dma_len(s);
> - }
> - }
> + /* check if the entries in the sg_table are contiguous */
> + if (drm_prime_get_contiguous_size(sgt) < attach->dmabuf->size)
> + return ERR_PTR(-EINVAL);
>
> /* Create a CMA GEM buffer. */
> cma_obj = __drm_gem_cma_create(dev, attach->dmabuf->size);
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index bbfc713bfdc3..226cd6ad3985 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -825,6 +825,37 @@ struct sg_table *drm_prime_pages_to_sg(struct page
> **pages, unsigned int nr_page
> }
> EXPORT_SYMBOL(drm_prime_pages_to_sg);
>
> +/**
> + * drm_prime_get_contiguous_size - returns the contiguous size of the buffer
> + * @sgt: sg_table describing the buffer to check
> + *
> + * This helper calculates the contiguous size in the DMA address space
> + * of the the buffer described by the provided sg_table.
> + *
> + * This is useful for implementing
> + * _gem_object_funcs.gem_prime_import_sg_table.
> + */
> +unsigned long drm_prime_get_contiguous_size(struct sg_table *sgt)
> +{
> + dma_addr_t expected = sg_dma_address(sgt->sgl);
> + struct scatterlist *sg;
> + unsigned long size = 0;
> + int i;
> +
> + for_each_sgtable_dma_sg(sgt, sg, i) {
> + unsigned int len = sg_dma_len(sg);
> +
> + if (!len)
> + break;
I wander if in some dark corners of the kernel 0-length buffers can be
in use :)
> + if (sg_dma_address(sg) != expected)
> + break;
> + expected += len;
> + size += len;
> + }
> + return size;
> +}
> +EXPORT_SYMBOL(drm_prime_get_contiguous_size);
> +
> /**
>* drm_gem_prime_export - helper library implementation of the export
> callback
>* @obj: GEM object to export
> diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h
> index 9af7422b44cf..47ef11614627 100644
> --- a/include/drm/drm_prime.h
> +++ b/include/drm/drm_prime.h
> @@ -92,6 +92,8 @@ struct sg_table *drm_prime_pages_to_sg(struct page **pages,
> unsigned int nr_page
> struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj,
>int flags);
>
> +unsigned long drm_prime_get_contiguous_size(struct sg_table *sgt);
> +
Reviewed-by
Regards
Andrzej
> /* helper functions for importing */
> struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
> struct dma_buf *dma_buf,
Re: [PATCH v7 08/36] drm: exynos: fix common struct sg_table related issues
On 19.06.2020 12:36, Marek Szyprowski wrote:
> The Documentation/DMA-API-HOWTO.txt states that the dma_map_sg() function
> returns the number of the created entries in the DMA address space.
> However the subsequent calls to the dma_sync_sg_for_{device,cpu}() and
> dma_unmap_sg must be called with the original number of the entries
> passed to the dma_map_sg().
>
> struct sg_table is a common structure used for describing a non-contiguous
> memory buffer, used commonly in the DRM and graphics subsystems. It
> consists of a scatterlist with memory pages and DMA addresses (sgl entry),
> as well as the number of scatterlist entries: CPU pages (orig_nents entry)
> and DMA mapped pages (nents entry).
>
> It turned out that it was a common mistake to misuse nents and orig_nents
> entries, calling DMA-mapping functions with a wrong number of entries or
> ignoring the number of mapped entries returned by the dma_map_sg()
> function.
>
> To avoid such issues, lets use a common dma-mapping wrappers operating
> directly on the struct sg_table objects and use scatterlist page
> iterators where possible. This, almost always, hides references to the
> nents and orig_nents entries, making the code robust, easier to follow
> and copy/paste safe.
>
> Signed-off-by: Marek Szyprowski
Reviewed-by
Regards
Andrzej
> ---
> drivers/gpu/drm/exynos/exynos_drm_g2d.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> index fcee33a43aca..7014a8cd971a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
> @@ -395,8 +395,8 @@ static void g2d_userptr_put_dma_addr(struct g2d_data *g2d,
> return;
>
> out:
> - dma_unmap_sg(to_dma_dev(g2d->drm_dev), g2d_userptr->sgt->sgl,
> - g2d_userptr->sgt->nents, DMA_BIDIRECTIONAL);
> + dma_unmap_sgtable(to_dma_dev(g2d->drm_dev), g2d_userptr->sgt,
> + DMA_BIDIRECTIONAL, 0);
>
> pages = frame_vector_pages(g2d_userptr->vec);
> if (!IS_ERR(pages)) {
> @@ -511,10 +511,10 @@ static dma_addr_t *g2d_userptr_get_dma_addr(struct
> g2d_data *g2d,
>
> g2d_userptr->sgt = sgt;
>
> - if (!dma_map_sg(to_dma_dev(g2d->drm_dev), sgt->sgl, sgt->nents,
> - DMA_BIDIRECTIONAL)) {
> + ret = dma_map_sgtable(to_dma_dev(g2d->drm_dev), sgt,
> + DMA_BIDIRECTIONAL, 0);
> + if (ret) {
> DRM_DEV_ERROR(g2d->dev, "failed to map sgt with dma region.\n");
> - ret = -ENOMEM;
> goto err_sg_free_table;
> }
>
Re: [PATCH v7 07/36] drm: exynos: use common helper for a scatterlist contiguity check
Hi,
On 19.06.2020 12:36, Marek Szyprowski wrote:
> Use common helper for checking the contiguity of the imported dma-buf.
>
> Signed-off-by: Marek Szyprowski
> ---
> drivers/gpu/drm/exynos/exynos_drm_gem.c | 23 +++
> 1 file changed, 3 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index efa476858db5..1716a023bca0 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -431,27 +431,10 @@ exynos_drm_gem_prime_import_sg_table(struct drm_device
> *dev,
> {
> struct exynos_drm_gem *exynos_gem;
>
> - if (sgt->nents < 1)
> + /* check if the entries in the sg_table are contiguous */
> + if (drm_prime_get_contiguous_size(sgt) < attach->dmabuf->size) {
> + DRM_ERROR("buffer chunks must be mapped contiguously");
> return ERR_PTR(-EINVAL);
> -
> - /*
> - * Check if the provided buffer has been mapped as contiguous
> - * into DMA address space.
> - */
> - if (sgt->nents > 1) {
> - dma_addr_t next_addr = sg_dma_address(sgt->sgl);
> - struct scatterlist *s;
> - unsigned int i;
> -
> - for_each_sg(sgt->sgl, s, sgt->nents, i) {
> - if (!sg_dma_len(s))
> - break;
> - if (sg_dma_address(s) != next_addr) {
> - DRM_ERROR("buffer chunks must be mapped
> contiguously");
> - return ERR_PTR(-EINVAL);
> - }
> - next_addr = sg_dma_address(s) + sg_dma_len(s);
> - }
> }
Reviewed-by
Regards
Andrzej
>
> exynos_gem = exynos_drm_gem_init(dev, attach->dmabuf->size);
Re: [PATCH v7 2/2] display/drm/bridge: TC358775 DSI/LVDS driver
On 04.07.2020 11:24, Vinay Simha BN wrote: > This driver is tested with two panels individually with Apq8016-IFC6309 board > https://protect2.fireeye.com/url?k=fe87a8ec-a3e0ecca-fe8623a3-0cc47a31384a-ffbc547df1141490=1=https%3A%2F%2Fwww.inforcecomputing.com%2Fproducts%2Fsingle-board-computers-sbc%2Fqualcomm-snapdragon-410-inforce-6309-micro-sbc > > 1. 1366x768@60 auo,b101xtn01 data-mapping = "jeida-24" > 2. 800x480@60 innolux,at070tn92 data-mapping = "vesa-24" > > - added SPDX identifier license > - updated alphabetic order of headers > - replaced u32 instead of uint32_t > - magic number to macros for CLRSI and mux registers > - mdelay to usleep_range > - added bus_formats > - removed drm_connector_status > - regulator enable and disable with proper orders and delays >as per the spec > - devm_drm_panel_bridge_add method used instead of panel >description modified > - dual port implemented > - panel->connector_type removed > - ~vsdelay dynamic value set based on the >calculation of dsi speed, output speed, blanking > - help modified > - display_timings naming local variables > - check for bus_formats unsupported > - error handling enpoint data-lanes > - Kconfig proper indentation > - GENMASK and FIELD_PREP used > - bus_formats handeld in mode_valid > - MODE_CLOCK_HIGH handled properly > - len initialized > - static function for mode_valid > > Signed-off-by: Vinay Simha BN > --- > v1: > Initial version > > v2: > * Andrzej Hajda review comments incorporated >SPDX identifier >development debug removed >alphabetic order headers >u32 instead of unit32_t >magic numbers to macros for CLRSI and mux registers >ignored return value > > * Laurent Pinchart review comments incorporated >mdelay to usleep_range >bus_formats added > > v3: > * Andrzej Hajda review comments incorporated >drm_connector_status removed >u32 rev removed and local variabl is used >regulator enable disable with proper orders and delays >as per the spec >devm_drm_panel_bridge_add method used instead of panel >description modified >dual port implemented > > v4: > * Sam Ravnborg review comments incorporated >panel->connector_type removed > > * Reported-by: kernel test robot >parse_dt to static function >removed the if (endpoint), since data-lanes has to be >present for dsi dts ports > > v5: >~vsdelay dynamic value set based on the >calculation of dsi speed, output speed, blanking > > v6: > * Sam Ravnborg review comments incorporated >help modified >display_timings naming local variables >check for bus_formats unsupported >error handling enpoint data-lanes > > v7: > * Sam Ravnborg review comments incorporated >Kconfig proper indentation >GENMASK and FIELD_PREP used >bus_formats handeld in mode_valid >MODE_CLOCK_HIGH handled properly > > * Reported-by: kernel test robot >len initialized >static function for mode_valid > --- > drivers/gpu/drm/bridge/Kconfig| 10 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/tc358775.c | 757 ++ > 3 files changed, 768 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/tc358775.c > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 43271c21d3fc..25c3097c4003 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -181,6 +181,16 @@ config DRM_TOSHIBA_TC358768 > help > Toshiba TC358768AXBG/TC358778XBG DSI bridge chip driver. > > +config DRM_TOSHIBA_TC358775 > + tristate "Toshiba TC358775 DSI/LVDS bridge" > + depends on OF > + select DRM_KMS_HELPER > + select REGMAP_I2C > + select DRM_PANEL > + select DRM_MIPI_DSI > + help > + Toshiba TC358775 DSI/LVDS bridge chip driver. > + > config DRM_TI_TFP410 > tristate "TI TFP410 DVI/HDMI bridge" > depends on OF > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index d63d4b7e4347..23c770b3bfe4 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o > obj-$(CONFIG_DRM_TOSHIBA_TC358764) += tc358764.o > obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o > obj-$(CONFIG_DRM_TOSHIBA_TC358768) += tc358768.o > +obj-$(CONFIG_DRM_TOSHIBA_TC358775) += tc358775.o > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o > o
[PATCH v8 2/5] driver core: add deferring probe reason to devices_deferred property
/sys/kernel/debug/devices_deferred property contains list of deferred devices.
This list does not contain reason why the driver deferred probe, the patch
improves it.
The natural place to set the reason is dev_err_probe function introduced
recently, ie. if dev_err_probe will be called with -EPROBE_DEFER instead of
printk the message will be attached to a deferred device and printed when user
reads devices_deferred property.
Signed-off-by: Andrzej Hajda
Reviewed-by: Mark Brown
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Andy Shevchenko
Reviewed-by: Rafael J. Wysocki
---
v8:
- improved commit message
---
drivers/base/base.h | 3 +++
drivers/base/core.c | 8 ++--
drivers/base/dd.c | 23 ++-
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index 95c22c0f9036..6954fccab3d7 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -93,6 +93,7 @@ struct device_private {
struct klist_node knode_class;
struct list_head deferred_probe;
struct device_driver *async_driver;
+ char *deferred_probe_reason;
struct device *device;
u8 dead:1;
};
@@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device
*dev,
extern void driver_detach(struct device_driver *drv);
extern int driver_probe_device(struct device_driver *drv, struct device *dev);
extern void driver_deferred_probe_del(struct device *dev);
+extern void device_set_deferred_probe_reson(const struct device *dev,
+ struct va_format *vaf);
static inline int driver_match_device(struct device_driver *drv,
struct device *dev)
{
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 3a827c82933f..fee047f03681 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
* This helper implements common pattern present in probe functions for error
* checking: print debug or error message depending if the error value is
* -EPROBE_DEFER and propagate error upwards.
+ * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
+ * checked later by reading devices_deferred debugfs attribute.
* It replaces code sequence:
* if (err != -EPROBE_DEFER)
* dev_err(dev, ...);
@@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err,
const char *fmt, ...)
vaf.fmt = fmt;
vaf.va =
- if (err != -EPROBE_DEFER)
+ if (err != -EPROBE_DEFER) {
dev_err(dev, "error %d: %pV", err, );
- else
+ } else {
+ device_set_deferred_probe_reson(dev, );
dev_dbg(dev, "error %d: %pV", err, );
+ }
va_end(args);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9a1d940342ac..dd5683b61f74 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "base.h"
#include "power/power.h"
@@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
if (!list_empty(>p->deferred_probe)) {
dev_dbg(dev, "Removed from deferred list\n");
list_del_init(>p->deferred_probe);
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = NULL;
}
mutex_unlock(_probe_mutex);
}
@@ -211,6 +214,23 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+/**
+ * device_set_deferred_probe_reson() - Set defer probe reason message for
device
+ * @dev: the pointer to the struct device
+ * @vaf: the pointer to va_format structure with message
+ */
+void device_set_deferred_probe_reson(const struct device *dev, struct
va_format *vaf)
+{
+ const char *drv = dev_driver_string(dev);
+
+ mutex_lock(_probe_mutex);
+
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv,
vaf);
+
+ mutex_unlock(_probe_mutex);
+}
+
/*
* deferred_devs_show() - Show the devices in the deferred probe pending list.
*/
@@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, void
*data)
mutex_lock(_probe_mutex);
list_for_each_entry(curr, _probe_pending_list, deferred_probe)
- seq_printf(s, "%s\n", dev_name(curr->device));
+ seq_printf(s, "%s\t%s", dev_name(curr->device),
+ curr->device->p->deferred_probe_reason ?: "\n");
mutex_unlock(_probe_mutex);
--
2.17.1
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
On 30.06.2020 20:00, Dmitry Torokhov wrote:
> On Tue, Jun 30, 2020 at 8:42 AM Andrzej Hajda wrote:
>>
>> On 30.06.2020 10:59, Grygorii Strashko wrote:
>>> Hi
>>>
>>> On 29/06/2020 14:28, Andrzej Hajda wrote:
>>>> Hi Grygorii,
>>>>
>>>> (...)
>>>>
>>>>>> /*
>>>>>> * deferred_devs_show() - Show the devices in the deferred probe
>>>>>> pending list.
>>>>>> */
>>>>>> @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s,
>>>>>> void *data)
>>>>>> mutex_lock(_probe_mutex);
>>>>>> list_for_each_entry(curr, _probe_pending_list,
>>>>>> deferred_probe)
>>>>>> -seq_printf(s, "%s\n", dev_name(curr->device));
>>>>>> +seq_printf(s, "%s\t%s", dev_name(curr->device),
>>>>>> + curr->device->p->deferred_probe_reason ?: "\n");
>>>>>> mutex_unlock(_probe_mutex);
>>>>>>
>>>>> Sry, may be i missing smth, but shouldn't it be optional
>>>>> (CONFIG_DEBUG_FS is probably too generic).
>>>>>
>>>> I am not sure what exactly are you referring to, but this patch does not
>>>> add new property, it just extends functionality of existing one.
>>> Sry, needed to be more specific.
>>>
>>> You've added device_set_deferred_probe_reson(dev, );
>>> which expected to be used on every EPROBE_DEFER in dev_err_probe() in
>>> combination with
>>>
>>> + } else {
>>> + device_set_deferred_probe_reson(dev, );
>>> dev_dbg(dev, "error %d: %pV", err, );
>>>
>>> ^^ dev_dbg() does not add any runtime overhead during boot unless enabled
>>> + }
>>>
>>> But:
>>>
>>> +void device_set_deferred_probe_reson(const struct device *dev, struct
>>> va_format *vaf)
>>> +{
>>> + const char *drv = dev_driver_string(dev);
>>> +
>>> + mutex_lock(_probe_mutex);
>>> +
>>> + kfree(dev->p->deferred_probe_reason);
>>> + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s:
>>> %pV", drv, vaf);
>>> +
>>> + mutex_unlock(_probe_mutex);
>>> +}
>>>
>>> ^^ Adds locking, kfree() and kasprintf() for every deferred probe
>>> during boot and can't be disabled.
>>>
>>> Right?
>>
>> Right, but usually the burden should be insignificant in comparison to
>> probe time, so I do not think it is worth optimizing.
> I do not think this is going to take. You are suggesting that we
> modify pretty much every driver to supply this deferral reason, and I
> doubt it will happen. Can we put this burden on providers that raise
> the deferral?
I wouldn't say they raise the deferral, they just inform resource is not
yet available. Only device driver, and only in its probe function can
"raise the deferral".
> I.e. majority of code are using devm API now, so we most
> likely know the device for which deferral is being raised. We can have
> a list of deferral reasons and their devices and when in device code
> once probe is done we could try reconciling it with the deferred
> devicelist, and this would mean you only need to implement this in
> gpiolib, regulator core, clocks core, etc.
This patchset tries to solve simple issue - replace multiple lines of
code present in multiple probe functions (additionally fixing lot of
them) with single call and then enhance it little bit, nothing more.
What you are proposing is blurry at the moment for me, provider does not
know if consumer want to defer, or will continue working without
missing resource, moreover some consumers can acquire resources after
probe - again no probe deferral. Even if it will be done (it can be, for
example by creating probe version of all resource get functions), it
will require much more changes but finally it will look like:
res = devm_get_resource_from_probe()
if (IS_ERR(res))
return PTR_ERR(res);
vs:
res = devm_get_resource(...)
if (IS_ERR(res))
return dev_err_probe(dev, PTR_ERR(res), ...);
Regards
Andrzej
>
> Thanks.
>
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
On 30.06.2020 10:59, Grygorii Strashko wrote:
> Hi
>
> On 29/06/2020 14:28, Andrzej Hajda wrote:
>> Hi Grygorii,
>>
>> (...)
>>
>>>> /*
>>>> * deferred_devs_show() - Show the devices in the deferred probe
>>>> pending list.
>>>> */
>>>> @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s,
>>>> void *data)
>>>> mutex_lock(_probe_mutex);
>>>> list_for_each_entry(curr, _probe_pending_list,
>>>> deferred_probe)
>>>> - seq_printf(s, "%s\n", dev_name(curr->device));
>>>> + seq_printf(s, "%s\t%s", dev_name(curr->device),
>>>> + curr->device->p->deferred_probe_reason ?: "\n");
>>>> mutex_unlock(_probe_mutex);
>>>>
>>>
>>> Sry, may be i missing smth, but shouldn't it be optional
>>> (CONFIG_DEBUG_FS is probably too generic).
>>>
>>
>> I am not sure what exactly are you referring to, but this patch does not
>> add new property, it just extends functionality of existing one.
>
> Sry, needed to be more specific.
>
> You've added device_set_deferred_probe_reson(dev, );
> which expected to be used on every EPROBE_DEFER in dev_err_probe() in
> combination with
>
> + } else {
> + device_set_deferred_probe_reson(dev, );
> dev_dbg(dev, "error %d: %pV", err, );
>
> ^^ dev_dbg() does not add any runtime overhead during boot unless enabled
> + }
>
> But:
>
> +void device_set_deferred_probe_reson(const struct device *dev, struct
> va_format *vaf)
> +{
> + const char *drv = dev_driver_string(dev);
> +
> + mutex_lock(_probe_mutex);
> +
> + kfree(dev->p->deferred_probe_reason);
> + dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s:
> %pV", drv, vaf);
> +
> + mutex_unlock(_probe_mutex);
> +}
>
> ^^ Adds locking, kfree() and kasprintf() for every deferred probe
> during boot and can't be disabled.
>
> Right?
Right, but usually the burden should be insignificant in comparison to
probe time, so I do not think it is worth optimizing.
Regards
Andrzej
>
>
Re: [PATCH v7 2/4] driver core: add deferring probe reason to devices_deferred property
On 29.06.2020 18:36, Andy Shevchenko wrote:
> On Mon, Jun 29, 2020 at 2:22 PM Andrzej Hajda wrote:
>> /sys/kernel/debug/devices_deferred property contains list of deferred
>> devices.
>> This list does not contain reason why the driver deferred probe, the patch
>> improves it.
>> The natural place to set the reason is dev_err_probe function introduced
>> recently,
>> ie. if dev_err_probe will be called with -EPROBE_DEFER instead of printk the
>> message
>> will be attached to deferred device and printed when user read
>> devices_deferred
> to a deferred
>
> reads
OK, thx.
>
>> property.
> ...
>
>> @@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err,
>> const char *fmt, ...)
>> vaf.fmt = fmt;
>> vaf.va =
>>
>> - if (err != -EPROBE_DEFER)
>> + if (err != -EPROBE_DEFER) {
> Why not positive conditional? (Okay, I'm fine with either in this case)
I put more natural branch 1st.
>
>> dev_err(dev, "error %d: %pV", err, );
>> - else
>> + } else {
>> + device_set_deferred_probe_reson(dev, );
>> dev_dbg(dev, "error %d: %pV", err, );
>> + }
> To reduce churn, you may move {} addition to the first patch.
But then I need to explain why it is there :)
>
> ...
>
>> list_for_each_entry(curr, _probe_pending_list,
>> deferred_probe)
>> - seq_printf(s, "%s\n", dev_name(curr->device));
>> + seq_printf(s, "%s\t%s", dev_name(curr->device),
>> + curr->device->p->deferred_probe_reason ?: "\n");
> Hmm... "\t" will be dangling in the latter case
Hmm, I followed your advice [1] :)
[1]:
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1787370.html
Regards
Andrzej
>
Re: [PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
Hi Grygorii, (...) >> /* >> * deferred_devs_show() - Show the devices in the deferred probe >> pending list. >> */ >> @@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, >> void *data) >> mutex_lock(_probe_mutex); >> list_for_each_entry(curr, _probe_pending_list, >> deferred_probe) >> - seq_printf(s, "%s\n", dev_name(curr->device)); >> + seq_printf(s, "%s\t%s", dev_name(curr->device), >> + curr->device->p->deferred_probe_reason ?: "\n"); >> mutex_unlock(_probe_mutex); >> > > Sry, may be i missing smth, but shouldn't it be optional > (CONFIG_DEBUG_FS is probably too generic). > I am not sure what exactly are you referring to, but this patch does not add new property, it just extends functionality of existing one. Regards Andrzej
[PATCH v7 2/4] driver core: add deferring probe reason to devices_deferred property
/sys/kernel/debug/devices_deferred property contains list of deferred devices.
This list does not contain reason why the driver deferred probe, the patch
improves it.
The natural place to set the reason is dev_err_probe function introduced
recently,
ie. if dev_err_probe will be called with -EPROBE_DEFER instead of printk the
message
will be attached to deferred device and printed when user read devices_deferred
property.
Signed-off-by: Andrzej Hajda
Reviewed-by: Mark Brown
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Andy Shevchenko
Reviewed-by: Rafael J. Wysocki
---
drivers/base/base.h | 3 +++
drivers/base/core.c | 8 ++--
drivers/base/dd.c | 23 ++-
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index 95c22c0f9036..6954fccab3d7 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -93,6 +93,7 @@ struct device_private {
struct klist_node knode_class;
struct list_head deferred_probe;
struct device_driver *async_driver;
+ char *deferred_probe_reason;
struct device *device;
u8 dead:1;
};
@@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device
*dev,
extern void driver_detach(struct device_driver *drv);
extern int driver_probe_device(struct device_driver *drv, struct device *dev);
extern void driver_deferred_probe_del(struct device *dev);
+extern void device_set_deferred_probe_reson(const struct device *dev,
+ struct va_format *vaf);
static inline int driver_match_device(struct device_driver *drv,
struct device *dev)
{
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 3a827c82933f..fee047f03681 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
* This helper implements common pattern present in probe functions for error
* checking: print debug or error message depending if the error value is
* -EPROBE_DEFER and propagate error upwards.
+ * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
+ * checked later by reading devices_deferred debugfs attribute.
* It replaces code sequence:
* if (err != -EPROBE_DEFER)
* dev_err(dev, ...);
@@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err,
const char *fmt, ...)
vaf.fmt = fmt;
vaf.va =
- if (err != -EPROBE_DEFER)
+ if (err != -EPROBE_DEFER) {
dev_err(dev, "error %d: %pV", err, );
- else
+ } else {
+ device_set_deferred_probe_reson(dev, );
dev_dbg(dev, "error %d: %pV", err, );
+ }
va_end(args);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9a1d940342ac..dd5683b61f74 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "base.h"
#include "power/power.h"
@@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
if (!list_empty(>p->deferred_probe)) {
dev_dbg(dev, "Removed from deferred list\n");
list_del_init(>p->deferred_probe);
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = NULL;
}
mutex_unlock(_probe_mutex);
}
@@ -211,6 +214,23 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+/**
+ * device_set_deferred_probe_reson() - Set defer probe reason message for
device
+ * @dev: the pointer to the struct device
+ * @vaf: the pointer to va_format structure with message
+ */
+void device_set_deferred_probe_reson(const struct device *dev, struct
va_format *vaf)
+{
+ const char *drv = dev_driver_string(dev);
+
+ mutex_lock(_probe_mutex);
+
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv,
vaf);
+
+ mutex_unlock(_probe_mutex);
+}
+
/*
* deferred_devs_show() - Show the devices in the deferred probe pending list.
*/
@@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, void
*data)
mutex_lock(_probe_mutex);
list_for_each_entry(curr, _probe_pending_list, deferred_probe)
- seq_printf(s, "%s\n", dev_name(curr->device));
+ seq_printf(s, "%s\t%s", dev_name(curr->device),
+ curr->device->p->deferred_probe_reason ?: "\n");
mutex_unlock(_probe_mutex);
--
2.17.1
[PATCH v7 0/4] driver core: add probe error check helper
Hi All, Thanks for comments. Changes since v6: - removed leftovers from old naming scheme in commit descritions, - added R-Bs. Changes since v5: - removed patch adding macro, dev_err_probe(dev, PTR_ERR(ptr), ...) should be used instead, - added dev_dbg logging in case of -EPROBE_DEFER, - renamed functions and vars according to comments, - extended docs, - cosmetics. Original message (with small adjustments): Recently I took some time to re-check error handling in drivers probe code, and I have noticed that number of incorrect resource acquisition error handling increased and there are no other propositions which can cure the situation. So I have decided to resend my old proposition of probe_err helper which should simplify resource acquisition error handling, it also extend it with adding defer probe reason to devices_deferred debugfs property, which should improve debugging experience for developers/testers. I have also added two patches showing usage and benefits of the helper. My dirty/ad-hoc cocci scripts shows that this helper can be used in at least 2700 places saving about 3500 lines of code. Regards Andrzej Andrzej Hajda (4): driver core: add device probe log helper driver core: add deferring probe reason to devices_deferred property drm/bridge/sii8620: fix resource acquisition error handling drm/bridge: lvds-codec: simplify error handling drivers/base/base.h | 3 ++ drivers/base/core.c | 46 drivers/base/dd.c| 23 +- drivers/gpu/drm/bridge/lvds-codec.c | 10 ++ drivers/gpu/drm/bridge/sil-sii8620.c | 21 ++--- include/linux/device.h | 3 ++ 6 files changed, 86 insertions(+), 20 deletions(-) -- 2.17.1
[PATCH v7 3/4] drm/bridge/sii8620: fix resource acquisition error handling
In case of error during resource acquisition driver should print error
message only in case it is not deferred probe, using dev_err_probe helper
solves the issue. Moreover it records defer probe reason for debugging.
Signed-off-by: Andrzej Hajda
Reviewed-by: Neil Armstrong
---
drivers/gpu/drm/bridge/sil-sii8620.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
b/drivers/gpu/drm/bridge/sil-sii8620.c
index 92acd336aa89..389c1f029774 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2299,10 +2299,9 @@ static int sii8620_probe(struct i2c_client *client,
INIT_LIST_HEAD(>mt_queue);
ctx->clk_xtal = devm_clk_get(dev, "xtal");
- if (IS_ERR(ctx->clk_xtal)) {
- dev_err(dev, "failed to get xtal clock from DT\n");
- return PTR_ERR(ctx->clk_xtal);
- }
+ if (IS_ERR(ctx->clk_xtal))
+ return dev_err_probe(dev, PTR_ERR(ctx->clk_xtal),
+"failed to get xtal clock from DT\n");
if (!client->irq) {
dev_err(dev, "no irq provided\n");
@@ -2313,16 +2312,14 @@ static int sii8620_probe(struct i2c_client *client,
sii8620_irq_thread,
IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
"sii8620", ctx);
- if (ret < 0) {
- dev_err(dev, "failed to install IRQ handler\n");
- return ret;
- }
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+"failed to install IRQ handler\n");
ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
- if (IS_ERR(ctx->gpio_reset)) {
- dev_err(dev, "failed to get reset gpio from DT\n");
- return PTR_ERR(ctx->gpio_reset);
- }
+ if (IS_ERR(ctx->gpio_reset))
+ return dev_err_probe(dev, PTR_ERR(ctx->gpio_reset),
+"failed to get reset gpio from DT\n");
ctx->supplies[0].supply = "cvcc10";
ctx->supplies[1].supply = "iovcc18";
--
2.17.1
[PATCH v7 4/4] drm/bridge: lvds-codec: simplify error handling
Using dev_err_probe code has following advantages:
- shorter code,
- recorded defer probe reason for debugging,
- uniform error code logging.
Signed-off-by: Andrzej Hajda
Reviewed-by: Neil Armstrong
---
drivers/gpu/drm/bridge/lvds-codec.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c
b/drivers/gpu/drm/bridge/lvds-codec.c
index 24fb1befdfa2..f19d9f7a5db2 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -71,13 +71,9 @@ static int lvds_codec_probe(struct platform_device *pdev)
lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
GPIOD_OUT_HIGH);
- if (IS_ERR(lvds_codec->powerdown_gpio)) {
- int err = PTR_ERR(lvds_codec->powerdown_gpio);
-
- if (err != -EPROBE_DEFER)
- dev_err(dev, "powerdown GPIO failure: %d\n", err);
- return err;
- }
+ if (IS_ERR(lvds_codec->powerdown_gpio))
+ return dev_err_probe(dev, PTR_ERR(lvds_codec->powerdown_gpio),
+"powerdown GPIO failure\n");
/* Locate the panel DT node. */
panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
--
2.17.1
[PATCH v7 1/4] driver core: add device probe log helper
During probe every time driver gets resource it should usually check for
error printk some message if it is not -EPROBE_DEFER and return the error.
This pattern is simple but requires adding few lines after any resource
acquisition code, as a result it is often omitted or implemented only
partially.
dev_err_probe helps to replace such code sequences with simple call,
so code:
if (err != -EPROBE_DEFER)
dev_err(dev, ...);
return err;
becomes:
return dev_err_probe(dev, err, ...);
Signed-off-by: Andrzej Hajda
Reviewed-by: Rafael J. Wysocki
---
drivers/base/core.c| 42 ++
include/linux/device.h | 3 +++
2 files changed, 45 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 67d39a90b45c..3a827c82933f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3953,6 +3953,48 @@ define_dev_printk_level(_dev_info, KERN_INFO);
#endif
+/**
+ * dev_err_probe - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print debug or error message depending if the error value is
+ * -EPROBE_DEFER and propagate error upwards.
+ * It replaces code sequence:
+ * if (err != -EPROBE_DEFER)
+ * dev_err(dev, ...);
+ * else
+ * dev_dbg(dev, ...);
+ * return err;
+ * with
+ * return dev_err_probe(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ va_start(args, fmt);
+ vaf.fmt = fmt;
+ vaf.va =
+
+ if (err != -EPROBE_DEFER)
+ dev_err(dev, "error %d: %pV", err, );
+ else
+ dev_dbg(dev, "error %d: %pV", err, );
+
+ va_end(args);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(dev_err_probe);
+
static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
{
return fwnode && !IS_ERR(fwnode->secondary);
diff --git a/include/linux/device.h b/include/linux/device.h
index 15460a5ac024..6b2272ae9af8 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device
*supplier);
void device_links_supplier_sync_state_pause(void);
void device_links_supplier_sync_state_resume(void);
+extern __printf(3, 4)
+int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+
/* Create alias, so I can be autoloaded. */
#define MODULE_ALIAS_CHARDEV(major,minor) \
MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
--
2.17.1
[PATCH v6 3/4] drm/bridge/sii8620: fix resource acquisition error handling
In case of error during resource acquisition driver should print error
message only in case it is not deferred probe, using dev_err_probe helper
solves the issue. Moreover it records defer probe reason for debugging.
Signed-off-by: Andrzej Hajda
---
drivers/gpu/drm/bridge/sil-sii8620.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
b/drivers/gpu/drm/bridge/sil-sii8620.c
index 92acd336aa89..389c1f029774 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2299,10 +2299,9 @@ static int sii8620_probe(struct i2c_client *client,
INIT_LIST_HEAD(>mt_queue);
ctx->clk_xtal = devm_clk_get(dev, "xtal");
- if (IS_ERR(ctx->clk_xtal)) {
- dev_err(dev, "failed to get xtal clock from DT\n");
- return PTR_ERR(ctx->clk_xtal);
- }
+ if (IS_ERR(ctx->clk_xtal))
+ return dev_err_probe(dev, PTR_ERR(ctx->clk_xtal),
+"failed to get xtal clock from DT\n");
if (!client->irq) {
dev_err(dev, "no irq provided\n");
@@ -2313,16 +2312,14 @@ static int sii8620_probe(struct i2c_client *client,
sii8620_irq_thread,
IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
"sii8620", ctx);
- if (ret < 0) {
- dev_err(dev, "failed to install IRQ handler\n");
- return ret;
- }
+ if (ret < 0)
+ return dev_err_probe(dev, ret,
+"failed to install IRQ handler\n");
ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
- if (IS_ERR(ctx->gpio_reset)) {
- dev_err(dev, "failed to get reset gpio from DT\n");
- return PTR_ERR(ctx->gpio_reset);
- }
+ if (IS_ERR(ctx->gpio_reset))
+ return dev_err_probe(dev, PTR_ERR(ctx->gpio_reset),
+"failed to get reset gpio from DT\n");
ctx->supplies[0].supply = "cvcc10";
ctx->supplies[1].supply = "iovcc18";
--
2.17.1
[PATCH v6 2/4] driver core: add deferring probe reason to devices_deferred property
/sys/kernel/debug/devices_deferred property contains list of deferred devices.
This list does not contain reason why the driver deferred probe, the patch
improves it.
The natural place to set the reason is probe_err function introduced recently,
ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
will be attached to deferred device and printed when user read devices_deferred
property.
Signed-off-by: Andrzej Hajda
Reviewed-by: Mark Brown
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Andy Shevchenko
---
drivers/base/base.h | 3 +++
drivers/base/core.c | 8 ++--
drivers/base/dd.c | 23 ++-
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index 95c22c0f9036..6954fccab3d7 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -93,6 +93,7 @@ struct device_private {
struct klist_node knode_class;
struct list_head deferred_probe;
struct device_driver *async_driver;
+ char *deferred_probe_reason;
struct device *device;
u8 dead:1;
};
@@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device
*dev,
extern void driver_detach(struct device_driver *drv);
extern int driver_probe_device(struct device_driver *drv, struct device *dev);
extern void driver_deferred_probe_del(struct device *dev);
+extern void device_set_deferred_probe_reson(const struct device *dev,
+ struct va_format *vaf);
static inline int driver_match_device(struct device_driver *drv,
struct device *dev)
{
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 3a827c82933f..fee047f03681 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3963,6 +3963,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
* This helper implements common pattern present in probe functions for error
* checking: print debug or error message depending if the error value is
* -EPROBE_DEFER and propagate error upwards.
+ * In case of -EPROBE_DEFER it sets also defer probe reason, which can be
+ * checked later by reading devices_deferred debugfs attribute.
* It replaces code sequence:
* if (err != -EPROBE_DEFER)
* dev_err(dev, ...);
@@ -3984,10 +3986,12 @@ int dev_err_probe(const struct device *dev, int err,
const char *fmt, ...)
vaf.fmt = fmt;
vaf.va =
- if (err != -EPROBE_DEFER)
+ if (err != -EPROBE_DEFER) {
dev_err(dev, "error %d: %pV", err, );
- else
+ } else {
+ device_set_deferred_probe_reson(dev, );
dev_dbg(dev, "error %d: %pV", err, );
+ }
va_end(args);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9a1d940342ac..dd5683b61f74 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "base.h"
#include "power/power.h"
@@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
if (!list_empty(>p->deferred_probe)) {
dev_dbg(dev, "Removed from deferred list\n");
list_del_init(>p->deferred_probe);
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = NULL;
}
mutex_unlock(_probe_mutex);
}
@@ -211,6 +214,23 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+/**
+ * device_set_deferred_probe_reson() - Set defer probe reason message for
device
+ * @dev: the pointer to the struct device
+ * @vaf: the pointer to va_format structure with message
+ */
+void device_set_deferred_probe_reson(const struct device *dev, struct
va_format *vaf)
+{
+ const char *drv = dev_driver_string(dev);
+
+ mutex_lock(_probe_mutex);
+
+ kfree(dev->p->deferred_probe_reason);
+ dev->p->deferred_probe_reason = kasprintf(GFP_KERNEL, "%s: %pV", drv,
vaf);
+
+ mutex_unlock(_probe_mutex);
+}
+
/*
* deferred_devs_show() - Show the devices in the deferred probe pending list.
*/
@@ -221,7 +241,8 @@ static int deferred_devs_show(struct seq_file *s, void
*data)
mutex_lock(_probe_mutex);
list_for_each_entry(curr, _probe_pending_list, deferred_probe)
- seq_printf(s, "%s\n", dev_name(curr->device));
+ seq_printf(s, "%s\t%s", dev_name(curr->device),
+ curr->device->p->deferred_probe_reason ?: "\n");
mutex_unlock(_probe_mutex);
--
2.17.1
[PATCH v6 4/4] drm/bridge: lvds-codec: simplify error handling
Using dev_err_probe code has following advantages:
- shorter code,
- recorded defer probe reason for debugging,
- uniform error code logging.
Signed-off-by: Andrzej Hajda
---
drivers/gpu/drm/bridge/lvds-codec.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c
b/drivers/gpu/drm/bridge/lvds-codec.c
index 24fb1befdfa2..f19d9f7a5db2 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -71,13 +71,9 @@ static int lvds_codec_probe(struct platform_device *pdev)
lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
GPIOD_OUT_HIGH);
- if (IS_ERR(lvds_codec->powerdown_gpio)) {
- int err = PTR_ERR(lvds_codec->powerdown_gpio);
-
- if (err != -EPROBE_DEFER)
- dev_err(dev, "powerdown GPIO failure: %d\n", err);
- return err;
- }
+ if (IS_ERR(lvds_codec->powerdown_gpio))
+ return dev_err_probe(dev, PTR_ERR(lvds_codec->powerdown_gpio),
+"powerdown GPIO failure\n");
/* Locate the panel DT node. */
panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
--
2.17.1
[PATCH v6 0/4] driver core: add probe error check helper
Hi All, Thanks for multiple comments. Changes since v5: - removed patch adding macro, dev_err_probe(dev, PTR_ERR(ptr), ...) should be used instead, - added dev_dbg logging in case of -EPROBE_DEFER, - renamed functions and vars according to comments, - extended docs, - cosmetics. Original message (with small adjustments): Recently I took some time to re-check error handling in drivers probe code, and I have noticed that number of incorrect resource acquisition error handling increased and there are no other propositions which can cure the situation. So I have decided to resend my old proposition of probe_err helper which should simplify resource acquisition error handling, it also extend it with adding defer probe reason to devices_deferred debugfs property, which should improve debugging experience for developers/testers. I have also added two patches showing usage and benefits of the helper. My dirty/ad-hoc cocci scripts shows that this helper can be used in at least 2700 places saving about 3500 lines of code. Regards Andrzej Andrzej Hajda (4): driver core: add device probe log helper driver core: add deferring probe reason to devices_deferred property drm/bridge/sii8620: fix resource acquisition error handling drm/bridge: lvds-codec: simplify error handling drivers/base/base.h | 3 ++ drivers/base/core.c | 46 drivers/base/dd.c| 23 +- drivers/gpu/drm/bridge/lvds-codec.c | 10 ++ drivers/gpu/drm/bridge/sil-sii8620.c | 21 ++--- include/linux/device.h | 3 ++ 6 files changed, 86 insertions(+), 20 deletions(-) -- 2.17.1
[PATCH v6 1/4] driver core: add device probe log helper
During probe every time driver gets resource it should usually check for
error printk some message if it is not -EPROBE_DEFER and return the error.
This pattern is simple but requires adding few lines after any resource
acquisition code, as a result it is often omitted or implemented only
partially.
dev_err_probe helps to replace such code sequences with simple call,
so code:
if (err != -EPROBE_DEFER)
dev_err(dev, ...);
return err;
becomes:
return probe_err(dev, err, ...);
Signed-off-by: Andrzej Hajda
---
drivers/base/core.c| 42 ++
include/linux/device.h | 3 +++
2 files changed, 45 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 67d39a90b45c..3a827c82933f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3953,6 +3953,48 @@ define_dev_printk_level(_dev_info, KERN_INFO);
#endif
+/**
+ * dev_err_probe - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print debug or error message depending if the error value is
+ * -EPROBE_DEFER and propagate error upwards.
+ * It replaces code sequence:
+ * if (err != -EPROBE_DEFER)
+ * dev_err(dev, ...);
+ * else
+ * dev_dbg(dev, ...);
+ * return err;
+ * with
+ * return dev_err_probe(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ va_start(args, fmt);
+ vaf.fmt = fmt;
+ vaf.va =
+
+ if (err != -EPROBE_DEFER)
+ dev_err(dev, "error %d: %pV", err, );
+ else
+ dev_dbg(dev, "error %d: %pV", err, );
+
+ va_end(args);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(dev_err_probe);
+
static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
{
return fwnode && !IS_ERR(fwnode->secondary);
diff --git a/include/linux/device.h b/include/linux/device.h
index 15460a5ac024..6b2272ae9af8 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device
*supplier);
void device_links_supplier_sync_state_pause(void);
void device_links_supplier_sync_state_resume(void);
+extern __printf(3, 4)
+int dev_err_probe(const struct device *dev, int err, const char *fmt, ...);
+
/* Create alias, so I can be autoloaded. */
#define MODULE_ALIAS_CHARDEV(major,minor) \
MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
--
2.17.1
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On 25.06.2020 10:41, Andy Shevchenko wrote: > On Wed, Jun 24, 2020 at 10:40 PM Andrzej Hajda wrote: >> On 24.06.2020 17:16, Robin Murphy wrote: > ... > >> I have proposed such thing in my previous iteration[1], except it was >> macro because of variadic arguments. > You may have a function with variadic arguments. Macros are beasts and > make in some cases more harm than help. What harm it can do in this particular case? With macro we have simple straightforward one-liner, with quite good type-checking. Maybe I am wrong, but I suspect creation of variadic function would require much more coding. Regards Andrzej
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On 24.06.2020 17:16, Robin Murphy wrote: > On 2020-06-24 16:04, Mark Brown wrote: >> On Wed, Jun 24, 2020 at 03:25:33PM +0100, Robin Murphy wrote: >> >>> And yeah, anyone who pipes up suggesting that places where an >>> ERR_PTR value >>> could be passed to probe_err() could simply refactor IS_ERR() checks >>> with >>> more uses of the god-awful PTR_ERR_OR_ZERO() obfuscator gets a long >>> stare of >>> disapproval... >> >> We could also have a probe_err_ptr() or something that took an ERR_PTR() >> instead if there really were an issue with explicitly doing this. > > Yeah, for all my lyrical objection, a static inline _ptr_err() > helper to wrap _err() with sensible type checking might actually > be an OK compromise if people really feel strongly for having that > utility. I have proposed such thing in my previous iteration[1], except it was macro because of variadic arguments. With current version we save 8 chars and hacky macro, with the old version we save only 4 chars and more clear construct - less tempting solution for me. Personally I prefer the current version - it does not seems to me more dangerous than all these PTR_ERR, IS_ERR,ERR_PTR helpers, but can prevent expression split across multiple lines due to 80char limit. Probably the simplest solution is to drop this patch, I will do it then. [1]: https://lwn.net/ml/linux-kernel/20181220102247.4911-4-a.ha...@samsung.com/ Regards Andrzej > > (and then we can debate whether it should also convert NULL to -ENOMEM > and !IS_ERR to 0... :D) > > Robin. > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://protect2.fireeye.com/url?k=074420c0-5ada8e5a-0745ab8f-0cc47a336fae-bba8bb4caf96e14d=1=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel > > >
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On 24.06.2020 14:30, Greg Kroah-Hartman wrote:
> On Wed, Jun 24, 2020 at 01:41:25PM +0200, Andrzej Hajda wrote:
>> Many resource acquisition functions return error value encapsulated in
>> pointer instead of integer value. To simplify coding we can use macro
>> which will accept both types of error.
>> With this patch user can use:
>> probe_err(dev, ptr, ...)
>> instead of:
>> probe_err(dev, PTR_ERR(ptr), ...)
>> Without loosing old functionality:
>> probe_err(dev, err, ...)
>>
>> Signed-off-by: Andrzej Hajda
>> ---
>> drivers/base/core.c| 25 ++---
>> include/linux/device.h | 25 -
>> 2 files changed, 26 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 2a96954d5460..df283c62d9c0 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>
>> #endif
>>
>> -/**
>> - * probe_err - probe error check and log helper
>> - * @dev: the pointer to the struct device
>> - * @err: error value to test
>> - * @fmt: printf-style format string
>> - * @...: arguments as specified in the format string
>> - *
>> - * This helper implements common pattern present in probe functions for
>> error
>> - * checking: print message if the error is not -EPROBE_DEFER and propagate
>> it.
>> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> - * later by reading devices_deferred debugfs attribute.
>> - * It replaces code sequence:
>> - * if (err != -EPROBE_DEFER)
>> - * dev_err(dev, ...);
>> - * return err;
>> - * with
>> - * return probe_err(dev, err, ...);
>> - *
>> - * Returns @err.
>> - *
>> - */
>> -int probe_err(const struct device *dev, int err, const char *fmt, ...)
>> +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
>> {
>> struct va_format vaf;
>> va_list args;
>> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const
>> char *fmt, ...)
>>
>> return err;
>> }
>> -EXPORT_SYMBOL_GPL(probe_err);
>> +EXPORT_SYMBOL_GPL(__probe_err);
>>
>> static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>> {
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 40a90d9bf799..22d3c3d4f461 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
>> void device_links_supplier_sync_state_resume(void);
>>
>> extern __printf(3, 4)
>> -int probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +
>> +/**
>> + * probe_err - probe error check and log helper
>> + * @dev: the pointer to the struct device
>> + * @err: error value to test, can be integer or pointer type
>> + * @fmt: printf-style format string
>> + * @...: arguments as specified in the format string
>> + *
>> + * This helper implements common pattern present in probe functions for
>> error
>> + * checking: print message if the error is not -EPROBE_DEFER and propagate
>> it.
>> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>> + * It replaces code sequence:
>> + * if (err != -EPROBE_DEFER)
>> + * dev_err(dev, ...);
>> + * return err;
>> + * with
>> + * return probe_err(dev, err, ...);
>> + *
>> + * Returns @err.
>> + *
>> + */
>> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
> Shouldn't that be "unsigned long" instead of "long"? That's what we put
> pointers in last I looked...
Unless we know this is error inside pointer, in such case we follow
practice from PTR_ERR function.
Regards
Andrzej
>
> thanks,
>
> greg k-h
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://protect2.fireeye.com/url?k=75712e41-28bf2f92-7570a50e-000babff317b-a5a76e98e30aecc2=1=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On 24.06.2020 14:14, Rafael J. Wysocki wrote:
> On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda wrote:
>> Many resource acquisition functions return error value encapsulated in
>> pointer instead of integer value. To simplify coding we can use macro
>> which will accept both types of error.
>> With this patch user can use:
>> probe_err(dev, ptr, ...)
>> instead of:
>> probe_err(dev, PTR_ERR(ptr), ...)
>> Without loosing old functionality:
>> probe_err(dev, err, ...)
>>
>> Signed-off-by: Andrzej Hajda
> The separation of this change from patch [1/5] looks kind of artificial to me.
>
> You are introducing a new function anyway, so why not to make it what
> you want right away?
Two reasons:
1.This patch is my recent idea, I didn't want to mix it with already
reviewed code.
2. This patch could be treated hacky by some devs due to macro
definition and type-casting.
If both patches are OK I can merge them of course into one.
Regards
Andrzej
>
>> ---
>> drivers/base/core.c| 25 ++---
>> include/linux/device.h | 25 -
>> 2 files changed, 26 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 2a96954d5460..df283c62d9c0 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>
>> #endif
>>
>> -/**
>> - * probe_err - probe error check and log helper
>> - * @dev: the pointer to the struct device
>> - * @err: error value to test
>> - * @fmt: printf-style format string
>> - * @...: arguments as specified in the format string
>> - *
>> - * This helper implements common pattern present in probe functions for
>> error
>> - * checking: print message if the error is not -EPROBE_DEFER and propagate
>> it.
>> - * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> - * later by reading devices_deferred debugfs attribute.
>> - * It replaces code sequence:
>> - * if (err != -EPROBE_DEFER)
>> - * dev_err(dev, ...);
>> - * return err;
>> - * with
>> - * return probe_err(dev, err, ...);
>> - *
>> - * Returns @err.
>> - *
>> - */
>> -int probe_err(const struct device *dev, int err, const char *fmt, ...)
>> +int __probe_err(const struct device *dev, int err, const char *fmt, ...)
>> {
>> struct va_format vaf;
>> va_list args;
>> @@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const
>> char *fmt, ...)
>>
>> return err;
>> }
>> -EXPORT_SYMBOL_GPL(probe_err);
>> +EXPORT_SYMBOL_GPL(__probe_err);
>>
>> static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
>> {
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 40a90d9bf799..22d3c3d4f461 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
>> void device_links_supplier_sync_state_resume(void);
>>
>> extern __printf(3, 4)
>> -int probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +int __probe_err(const struct device *dev, int err, const char *fmt, ...);
>> +
>> +/**
>> + * probe_err - probe error check and log helper
>> + * @dev: the pointer to the struct device
>> + * @err: error value to test, can be integer or pointer type
>> + * @fmt: printf-style format string
>> + * @...: arguments as specified in the format string
>> + *
>> + * This helper implements common pattern present in probe functions for
>> error
>> + * checking: print message if the error is not -EPROBE_DEFER and propagate
>> it.
>> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>> + * It replaces code sequence:
>> + * if (err != -EPROBE_DEFER)
>> + * dev_err(dev, ...);
>> + * return err;
>> + * with
>> + * return probe_err(dev, err, ...);
>> + *
>> + * Returns @err.
>> + *
>> + */
>> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
>>
>> /* Create alias, so I can be autoloaded. */
>> #define MODULE_ALIAS_CHARDEV(major,minor) \
>> --
>> 2.17.1
>>
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://protect2.fireeye.com/url?k=fe383567-a3a29cc4-fe39be28-002590f5b904-1faeb9cf68e31587=1=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>
Re: [RESEND PATCH v5 1/5] driver core: add probe_err log helper
On 24.06.2020 15:23, Laurent Pinchart wrote:
> On Wed, Jun 24, 2020 at 02:31:40PM +0200, Greg Kroah-Hartman wrote:
>> On Wed, Jun 24, 2020 at 01:41:23PM +0200, Andrzej Hajda wrote:
>>> During probe every time driver gets resource it should usually check for
>>> error
>>> printk some message if it is not -EPROBE_DEFER and return the error. This
>>> pattern is simple but requires adding few lines after any resource
>>> acquisition
>>> code, as a result it is often omited or implemented only partially.
>>> probe_err helps to replace such code sequences with simple call, so code:
>>> if (err != -EPROBE_DEFER)
>>> dev_err(dev, ...);
>>> return err;
>>> becomes:
>>> return probe_err(dev, err, ...);
>>>
>>> Signed-off-by: Andrzej Hajda
>>> Reviewed-by: Javier Martinez Canillas
>>> Reviewed-by: Mark Brown
>>> Reviewed-by: Andy Shevchenko
>>> ---
>>> drivers/base/core.c| 39 +++
>>> include/linux/device.h | 3 +++
>>> 2 files changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>> index 67d39a90b45c..ee9da66bff1b 100644
>>> --- a/drivers/base/core.c
>>> +++ b/drivers/base/core.c
>>> @@ -3953,6 +3953,45 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>>
>>> #endif
>>>
>>> +/**
>>> + * probe_err - probe error check and log helper
>>> + * @dev: the pointer to the struct device
>>> + * @err: error value to test
>>> + * @fmt: printf-style format string
>>> + * @...: arguments as specified in the format string
>>> + *
>>> + * This helper implements common pattern present in probe functions for
>>> error
>>> + * checking: print message if the error is not -EPROBE_DEFER and propagate
>>> it.
>>> + * It replaces code sequence:
>>> + * if (err != -EPROBE_DEFER)
>>> + * dev_err(dev, ...);
>>> + * return err;
>>> + * with
>>> + * return probe_err(dev, err, ...);
>>> + *
>>> + * Returns @err.
>>> + *
>>> + */
>>> +int probe_err(const struct device *dev, int err, const char *fmt, ...)
>>> +{
>>> + struct va_format vaf;
>>> + va_list args;
>>> +
>>> + if (err == -EPROBE_DEFER)
>>> + return err;
>>> +
>>> + va_start(args, fmt);
>>> + vaf.fmt = fmt;
>>> + vaf.va =
>>> +
>>> + dev_err(dev, "error %d: %pV", err, );
>>> +
>>> + va_end(args);
>>> +
>>> + return err;
>>> +}
>>> +EXPORT_SYMBOL_GPL(probe_err);
>> Please be specific in global symbols, how about "driver_probe_error()"?
> Or dev_err_probe() to match the existing dev_* functions ?
OK.
Regards
Andrzej
>
>> And merge the other patch into this one, as Raphael said, otherwise this
>> just looks odd to add something and then fix it up later.
Re: [RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code
On 24.06.2020 15:33, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Wed, Jun 24, 2020 at 01:41:27PM +0200, Andrzej Hajda wrote:
>> Using probe_err code has following advantages:
>> - shorter code,
>> - recorded defer probe reason for debugging,
>> - uniform error code logging.
>>
>> Signed-off-by: Andrzej Hajda
>> ---
>> drivers/gpu/drm/bridge/lvds-codec.c | 9 ++---
>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/lvds-codec.c
>> b/drivers/gpu/drm/bridge/lvds-codec.c
>> index 24fb1befdfa2..c76fa0239e39 100644
>> --- a/drivers/gpu/drm/bridge/lvds-codec.c
>> +++ b/drivers/gpu/drm/bridge/lvds-codec.c
>> @@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev)
>> lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
>> lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
>> GPIOD_OUT_HIGH);
>> -if (IS_ERR(lvds_codec->powerdown_gpio)) {
>> -int err = PTR_ERR(lvds_codec->powerdown_gpio);
>> -
>> -if (err != -EPROBE_DEFER)
>> -dev_err(dev, "powerdown GPIO failure: %d\n", err);
>> -return err;
>> -}
>> +if (IS_ERR(lvds_codec->powerdown_gpio))
>> +return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown
>> GPIO failure\n");
> Line wrap please.
I hoped that with latest checkpatch line length limit increase from 80
to 100 it is acceptable :) but apparently not [1].
[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
>
> It bothers me that the common pattern of writing the error code at the
> end of the message isn't possible anymore. Maybe I'll get used to it,
> but it removes some flexibility.
Yes, but it gives uniformity :) and now with %pe printk format it
changes anyway.
Regards
Andrzej
>
>>
>> /* Locate the panel DT node. */
>> panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property
On 24.06.2020 14:11, Rafael J. Wysocki wrote:
> On Wed, Jun 24, 2020 at 1:41 PM Andrzej Hajda wrote:
>> /sys/kernel/debug/devices_deferred property contains list of deferred
>> devices.
>> This list does not contain reason why the driver deferred probe, the patch
>> improves it.
>> The natural place to set the reason is probe_err function introduced
>> recently,
>> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the
>> message
>> will be attached to deferred device and printed when user read
>> devices_deferred
>> property.
>>
>> Signed-off-by: Andrzej Hajda
>> Reviewed-by: Mark Brown
>> Reviewed-by: Javier Martinez Canillas
>> Reviewed-by: Andy Shevchenko
>> ---
>> drivers/base/base.h | 3 +++
>> drivers/base/core.c | 10 ++
>> drivers/base/dd.c | 21 -
>> 3 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/base/base.h b/drivers/base/base.h
>> index 95c22c0f9036..93ef1c2f4c1f 100644
>> --- a/drivers/base/base.h
>> +++ b/drivers/base/base.h
>> @@ -93,6 +93,7 @@ struct device_private {
>> struct klist_node knode_class;
>> struct list_head deferred_probe;
>> struct device_driver *async_driver;
>> + char *deferred_probe_msg;
> What about calling this deferred_probe_reason?
>
>> struct device *device;
>> u8 dead:1;
>> };
>> @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device
>> *dev,
>> extern void driver_detach(struct device_driver *drv);
>> extern int driver_probe_device(struct device_driver *drv, struct device
>> *dev);
>> extern void driver_deferred_probe_del(struct device *dev);
>> +extern void __deferred_probe_set_msg(const struct device *dev,
>> +struct va_format *vaf);
> I'd call this device_set_deferred_probe_reson() to follow the naming
> convention for the function names in this header file.
>
>> static inline int driver_match_device(struct device_driver *drv,
>>struct device *dev)
>> {
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index ee9da66bff1b..2a96954d5460 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3962,6 +3962,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>*
>>* This helper implements common pattern present in probe functions for
>> error
>>* checking: print message if the error is not -EPROBE_DEFER and propagate
>> it.
>> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>>* It replaces code sequence:
>>* if (err != -EPROBE_DEFER)
>>* dev_err(dev, ...);
>> @@ -3977,14 +3979,14 @@ int probe_err(const struct device *dev, int err,
>> const char *fmt, ...)
>> struct va_format vaf;
>> va_list args;
>>
>> - if (err == -EPROBE_DEFER)
>> - return err;
>> -
>> va_start(args, fmt);
>> vaf.fmt = fmt;
>> vaf.va =
>>
>> - dev_err(dev, "error %d: %pV", err, );
>> + if (err == -EPROBE_DEFER)
>> + __deferred_probe_set_msg(dev, );
>> + else
>> + dev_err(dev, "error %d: %pV", err, );
>>
>> va_end(args);
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 9a1d940342ac..f44d26454b6a 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -27,6 +27,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "base.h"
>> #include "power/power.h"
>> @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
>> if (!list_empty(>p->deferred_probe)) {
>> dev_dbg(dev, "Removed from deferred list\n");
>> list_del_init(>p->deferred_probe);
>> + kfree(dev->p->deferred_probe_msg);
>> + dev->p->deferred_probe_msg = NULL;
>> }
>> mutex_unlock(_probe_mutex);
>> }
>> @@ -211,6 +214,21 @@ void device_unblock_probing(void)
>> driver_deferred_probe_trigger();
>> }
>>
>> +/*
>> + * __deferred_probe_set_msg() - Set defer probe reason message for device
> I'd change this
Re: [RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property
On 24.06.2020 14:34, Greg Kroah-Hartman wrote:
> On Wed, Jun 24, 2020 at 01:41:24PM +0200, Andrzej Hajda wrote:
>> /sys/kernel/debug/devices_deferred property contains list of deferred
>> devices.
>> This list does not contain reason why the driver deferred probe, the patch
>> improves it.
>> The natural place to set the reason is probe_err function introduced
>> recently,
>> ie. if probe_err will be called with -EPROBE_DEFER instead of printk the
>> message
>> will be attached to deferred device and printed when user read
>> devices_deferred
>> property.
>>
>> Signed-off-by: Andrzej Hajda
>> Reviewed-by: Mark Brown
>> Reviewed-by: Javier Martinez Canillas
>> Reviewed-by: Andy Shevchenko
>> ---
>> drivers/base/base.h | 3 +++
>> drivers/base/core.c | 10 ++
>> drivers/base/dd.c | 21 -
>> 3 files changed, 29 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/base/base.h b/drivers/base/base.h
>> index 95c22c0f9036..93ef1c2f4c1f 100644
>> --- a/drivers/base/base.h
>> +++ b/drivers/base/base.h
>> @@ -93,6 +93,7 @@ struct device_private {
>> struct klist_node knode_class;
>> struct list_head deferred_probe;
>> struct device_driver *async_driver;
>> +char *deferred_probe_msg;
>> struct device *device;
>> u8 dead:1;
>> };
>> @@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device
>> *dev,
>> extern void driver_detach(struct device_driver *drv);
>> extern int driver_probe_device(struct device_driver *drv, struct device
>> *dev);
>> extern void driver_deferred_probe_del(struct device *dev);
>> +extern void __deferred_probe_set_msg(const struct device *dev,
>> + struct va_format *vaf);
>> static inline int driver_match_device(struct device_driver *drv,
>>struct device *dev)
>> {
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index ee9da66bff1b..2a96954d5460 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -3962,6 +3962,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
>>*
>>* This helper implements common pattern present in probe functions for
>> error
>>* checking: print message if the error is not -EPROBE_DEFER and propagate
>> it.
>> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
>> + * later by reading devices_deferred debugfs attribute.
>>* It replaces code sequence:
>>* if (err != -EPROBE_DEFER)
>>* dev_err(dev, ...);
>> @@ -3977,14 +3979,14 @@ int probe_err(const struct device *dev, int err,
>> const char *fmt, ...)
>> struct va_format vaf;
>> va_list args;
>>
>> -if (err == -EPROBE_DEFER)
>> -return err;
>> -
>> va_start(args, fmt);
>> vaf.fmt = fmt;
>> vaf.va =
>>
>> -dev_err(dev, "error %d: %pV", err, );
>> +if (err == -EPROBE_DEFER)
>> +__deferred_probe_set_msg(dev, );
>> +else
>> +dev_err(dev, "error %d: %pV", err, );
>>
>> va_end(args);
>>
>> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
>> index 9a1d940342ac..f44d26454b6a 100644
>> --- a/drivers/base/dd.c
>> +++ b/drivers/base/dd.c
>> @@ -27,6 +27,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "base.h"
>> #include "power/power.h"
>> @@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
>> if (!list_empty(>p->deferred_probe)) {
>> dev_dbg(dev, "Removed from deferred list\n");
>> list_del_init(>p->deferred_probe);
>> +kfree(dev->p->deferred_probe_msg);
>> +dev->p->deferred_probe_msg = NULL;
>> }
>> mutex_unlock(_probe_mutex);
>> }
>> @@ -211,6 +214,21 @@ void device_unblock_probing(void)
>> driver_deferred_probe_trigger();
>> }
>>
>> +/*
>> + * __deferred_probe_set_msg() - Set defer probe reason message for device
>> + */
>> +void __deferred_probe_set_msg(const struct device *dev, struct va_format
>> *vaf)
>> +{
>> +const char *drv = dev_driver_string(dev);
>> +
>> +mutex_lock(_probe_mutex);
>> +
>> +kfree(dev->p->deferred_probe_msg);
>> +dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf);
> What about the device name? Don't you also want that?
deferred_devs_show prints it already, deferred_probe_msg is appended if
not null.
Regards
Andrzej
>
> You want the same format that __dev_printk() outputs, please use that
> to be consistant with all other messages that drivers are spitting out.
>
> thanks,
>
> greg k-h
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://protect2.fireeye.com/url?k=28daee95-7508f5cd-28db65da-0cc47a31c8b4-b3e8d1affcda9c08=1=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Fdri-devel
>
Re: [RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
On 24.06.2020 14:53, Andy Shevchenko wrote: > On Wed, Jun 24, 2020 at 2:41 PM Andrzej Hajda wrote: >> Many resource acquisition functions return error value encapsulated in >> pointer instead of integer value. To simplify coding we can use macro >> which will accept both types of error. >> With this patch user can use: >> probe_err(dev, ptr, ...) >> instead of: >> probe_err(dev, PTR_ERR(ptr), ...) >> Without loosing old functionality: >> probe_err(dev, err, ...) > ... > >> + * This helper implements common pattern present in probe functions for >> error >> + * checking: print message if the error is not -EPROBE_DEFER and propagate >> it. >> + * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked >> + * later by reading devices_deferred debugfs attribute. >> + * It replaces code sequence: >> + * if (err != -EPROBE_DEFER) >> + * dev_err(dev, ...); > Btw, we have now %pe. Can you consider to use it? OK, I haven't noticed it finally appeared. > >> + * return err; >> + * with >> + * return probe_err(dev, err, ...); >> + * >> + * Returns @err. >> + * >> + */ >> +#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args) > Can't we use PTR_ERR() here? Nope, I want to accept both types: int and pointer. Regards Andrzej >
[RESEND PATCH v5 1/5] driver core: add probe_err log helper
During probe every time driver gets resource it should usually check for error
printk some message if it is not -EPROBE_DEFER and return the error. This
pattern is simple but requires adding few lines after any resource acquisition
code, as a result it is often omited or implemented only partially.
probe_err helps to replace such code sequences with simple call, so code:
if (err != -EPROBE_DEFER)
dev_err(dev, ...);
return err;
becomes:
return probe_err(dev, err, ...);
Signed-off-by: Andrzej Hajda
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Mark Brown
Reviewed-by: Andy Shevchenko
---
drivers/base/core.c| 39 +++
include/linux/device.h | 3 +++
2 files changed, 42 insertions(+)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 67d39a90b45c..ee9da66bff1b 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3953,6 +3953,45 @@ define_dev_printk_level(_dev_info, KERN_INFO);
#endif
+/**
+ * probe_err - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * It replaces code sequence:
+ * if (err != -EPROBE_DEFER)
+ * dev_err(dev, ...);
+ * return err;
+ * with
+ * return probe_err(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+int probe_err(const struct device *dev, int err, const char *fmt, ...)
+{
+ struct va_format vaf;
+ va_list args;
+
+ if (err == -EPROBE_DEFER)
+ return err;
+
+ va_start(args, fmt);
+ vaf.fmt = fmt;
+ vaf.va =
+
+ dev_err(dev, "error %d: %pV", err, );
+
+ va_end(args);
+
+ return err;
+}
+EXPORT_SYMBOL_GPL(probe_err);
+
static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
{
return fwnode && !IS_ERR(fwnode->secondary);
diff --git a/include/linux/device.h b/include/linux/device.h
index 15460a5ac024..40a90d9bf799 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -964,6 +964,9 @@ void device_link_remove(void *consumer, struct device
*supplier);
void device_links_supplier_sync_state_pause(void);
void device_links_supplier_sync_state_resume(void);
+extern __printf(3, 4)
+int probe_err(const struct device *dev, int err, const char *fmt, ...);
+
/* Create alias, so I can be autoloaded. */
#define MODULE_ALIAS_CHARDEV(major,minor) \
MODULE_ALIAS("char-major-" __stringify(major) "-" __stringify(minor))
--
2.17.1
[RESEND PATCH v5 5/5] drm/bridge: lvds-codec: simplify error handling code
Using probe_err code has following advantages:
- shorter code,
- recorded defer probe reason for debugging,
- uniform error code logging.
Signed-off-by: Andrzej Hajda
---
drivers/gpu/drm/bridge/lvds-codec.c | 9 ++---
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c
b/drivers/gpu/drm/bridge/lvds-codec.c
index 24fb1befdfa2..c76fa0239e39 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -71,13 +71,8 @@ static int lvds_codec_probe(struct platform_device *pdev)
lvds_codec->connector_type = (uintptr_t)of_device_get_match_data(dev);
lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
GPIOD_OUT_HIGH);
- if (IS_ERR(lvds_codec->powerdown_gpio)) {
- int err = PTR_ERR(lvds_codec->powerdown_gpio);
-
- if (err != -EPROBE_DEFER)
- dev_err(dev, "powerdown GPIO failure: %d\n", err);
- return err;
- }
+ if (IS_ERR(lvds_codec->powerdown_gpio))
+ return probe_err(dev, lvds_codec->powerdown_gpio, "powerdown
GPIO failure\n");
/* Locate the panel DT node. */
panel_node = of_graph_get_remote_node(dev->of_node, 1, 0);
--
2.17.1
[RESEND PATCH v5 4/5] drm/bridge/sii8620: fix resource acquisition error handling
In case of error during resource acquisition driver should print error
message only in case it is not deferred probe, using probe_err helper
solves the issue. Moreover it records defer probe reason for debugging.
Signed-off-by: Andrzej Hajda
---
drivers/gpu/drm/bridge/sil-sii8620.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c
b/drivers/gpu/drm/bridge/sil-sii8620.c
index 92acd336aa89..2f825b2d0098 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -2299,10 +2299,8 @@ static int sii8620_probe(struct i2c_client *client,
INIT_LIST_HEAD(>mt_queue);
ctx->clk_xtal = devm_clk_get(dev, "xtal");
- if (IS_ERR(ctx->clk_xtal)) {
- dev_err(dev, "failed to get xtal clock from DT\n");
- return PTR_ERR(ctx->clk_xtal);
- }
+ if (IS_ERR(ctx->clk_xtal))
+ return probe_err(dev, ctx->clk_xtal, "failed to get xtal clock
from DT\n");
if (!client->irq) {
dev_err(dev, "no irq provided\n");
@@ -2313,16 +2311,12 @@ static int sii8620_probe(struct i2c_client *client,
sii8620_irq_thread,
IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
"sii8620", ctx);
- if (ret < 0) {
- dev_err(dev, "failed to install IRQ handler\n");
- return ret;
- }
+ if (ret < 0)
+ return probe_err(dev, ret, "failed to install IRQ handler\n");
ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
- if (IS_ERR(ctx->gpio_reset)) {
- dev_err(dev, "failed to get reset gpio from DT\n");
- return PTR_ERR(ctx->gpio_reset);
- }
+ if (IS_ERR(ctx->gpio_reset))
+ return probe_err(dev, ctx->gpio_reset, "failed to get reset
gpio from DT\n");
ctx->supplies[0].supply = "cvcc10";
ctx->supplies[1].supply = "iovcc18";
--
2.17.1
[RESEND PATCH v5 3/5] drivers core: allow probe_err accept integer and pointer types
Many resource acquisition functions return error value encapsulated in
pointer instead of integer value. To simplify coding we can use macro
which will accept both types of error.
With this patch user can use:
probe_err(dev, ptr, ...)
instead of:
probe_err(dev, PTR_ERR(ptr), ...)
Without loosing old functionality:
probe_err(dev, err, ...)
Signed-off-by: Andrzej Hajda
---
drivers/base/core.c| 25 ++---
include/linux/device.h | 25 -
2 files changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2a96954d5460..df283c62d9c0 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3953,28 +3953,7 @@ define_dev_printk_level(_dev_info, KERN_INFO);
#endif
-/**
- * probe_err - probe error check and log helper
- * @dev: the pointer to the struct device
- * @err: error value to test
- * @fmt: printf-style format string
- * @...: arguments as specified in the format string
- *
- * This helper implements common pattern present in probe functions for error
- * checking: print message if the error is not -EPROBE_DEFER and propagate it.
- * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
- * later by reading devices_deferred debugfs attribute.
- * It replaces code sequence:
- * if (err != -EPROBE_DEFER)
- * dev_err(dev, ...);
- * return err;
- * with
- * return probe_err(dev, err, ...);
- *
- * Returns @err.
- *
- */
-int probe_err(const struct device *dev, int err, const char *fmt, ...)
+int __probe_err(const struct device *dev, int err, const char *fmt, ...)
{
struct va_format vaf;
va_list args;
@@ -3992,7 +3971,7 @@ int probe_err(const struct device *dev, int err, const
char *fmt, ...)
return err;
}
-EXPORT_SYMBOL_GPL(probe_err);
+EXPORT_SYMBOL_GPL(__probe_err);
static inline bool fwnode_is_primary(struct fwnode_handle *fwnode)
{
diff --git a/include/linux/device.h b/include/linux/device.h
index 40a90d9bf799..22d3c3d4f461 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -965,7 +965,30 @@ void device_links_supplier_sync_state_pause(void);
void device_links_supplier_sync_state_resume(void);
extern __printf(3, 4)
-int probe_err(const struct device *dev, int err, const char *fmt, ...);
+int __probe_err(const struct device *dev, int err, const char *fmt, ...);
+
+/**
+ * probe_err - probe error check and log helper
+ * @dev: the pointer to the struct device
+ * @err: error value to test, can be integer or pointer type
+ * @fmt: printf-style format string
+ * @...: arguments as specified in the format string
+ *
+ * This helper implements common pattern present in probe functions for error
+ * checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
+ * later by reading devices_deferred debugfs attribute.
+ * It replaces code sequence:
+ * if (err != -EPROBE_DEFER)
+ * dev_err(dev, ...);
+ * return err;
+ * with
+ * return probe_err(dev, err, ...);
+ *
+ * Returns @err.
+ *
+ */
+#define probe_err(dev, err, args...) __probe_err(dev, (long)(err), args)
/* Create alias, so I can be autoloaded. */
#define MODULE_ALIAS_CHARDEV(major,minor) \
--
2.17.1
[RESEND PATCH v5 2/5] driver core: add deferring probe reason to devices_deferred property
/sys/kernel/debug/devices_deferred property contains list of deferred devices.
This list does not contain reason why the driver deferred probe, the patch
improves it.
The natural place to set the reason is probe_err function introduced recently,
ie. if probe_err will be called with -EPROBE_DEFER instead of printk the message
will be attached to deferred device and printed when user read devices_deferred
property.
Signed-off-by: Andrzej Hajda
Reviewed-by: Mark Brown
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Andy Shevchenko
---
drivers/base/base.h | 3 +++
drivers/base/core.c | 10 ++
drivers/base/dd.c | 21 -
3 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index 95c22c0f9036..93ef1c2f4c1f 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -93,6 +93,7 @@ struct device_private {
struct klist_node knode_class;
struct list_head deferred_probe;
struct device_driver *async_driver;
+ char *deferred_probe_msg;
struct device *device;
u8 dead:1;
};
@@ -134,6 +135,8 @@ extern void device_release_driver_internal(struct device
*dev,
extern void driver_detach(struct device_driver *drv);
extern int driver_probe_device(struct device_driver *drv, struct device *dev);
extern void driver_deferred_probe_del(struct device *dev);
+extern void __deferred_probe_set_msg(const struct device *dev,
+struct va_format *vaf);
static inline int driver_match_device(struct device_driver *drv,
struct device *dev)
{
diff --git a/drivers/base/core.c b/drivers/base/core.c
index ee9da66bff1b..2a96954d5460 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -3962,6 +3962,8 @@ define_dev_printk_level(_dev_info, KERN_INFO);
*
* This helper implements common pattern present in probe functions for error
* checking: print message if the error is not -EPROBE_DEFER and propagate it.
+ * In case of -EPROBE_DEFER it sets defer probe reason, which can be checked
+ * later by reading devices_deferred debugfs attribute.
* It replaces code sequence:
* if (err != -EPROBE_DEFER)
* dev_err(dev, ...);
@@ -3977,14 +3979,14 @@ int probe_err(const struct device *dev, int err, const
char *fmt, ...)
struct va_format vaf;
va_list args;
- if (err == -EPROBE_DEFER)
- return err;
-
va_start(args, fmt);
vaf.fmt = fmt;
vaf.va =
- dev_err(dev, "error %d: %pV", err, );
+ if (err == -EPROBE_DEFER)
+ __deferred_probe_set_msg(dev, );
+ else
+ dev_err(dev, "error %d: %pV", err, );
va_end(args);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9a1d940342ac..f44d26454b6a 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include "base.h"
#include "power/power.h"
@@ -136,6 +137,8 @@ void driver_deferred_probe_del(struct device *dev)
if (!list_empty(>p->deferred_probe)) {
dev_dbg(dev, "Removed from deferred list\n");
list_del_init(>p->deferred_probe);
+ kfree(dev->p->deferred_probe_msg);
+ dev->p->deferred_probe_msg = NULL;
}
mutex_unlock(_probe_mutex);
}
@@ -211,6 +214,21 @@ void device_unblock_probing(void)
driver_deferred_probe_trigger();
}
+/*
+ * __deferred_probe_set_msg() - Set defer probe reason message for device
+ */
+void __deferred_probe_set_msg(const struct device *dev, struct va_format *vaf)
+{
+ const char *drv = dev_driver_string(dev);
+
+ mutex_lock(_probe_mutex);
+
+ kfree(dev->p->deferred_probe_msg);
+ dev->p->deferred_probe_msg = kasprintf(GFP_KERNEL, "%s: %pV", drv, vaf);
+
+ mutex_unlock(_probe_mutex);
+}
+
/*
* deferred_devs_show() - Show the devices in the deferred probe pending list.
*/
@@ -221,7 +239,8 @@ static int deferred_devs_show(struct seq_file *s, void
*data)
mutex_lock(_probe_mutex);
list_for_each_entry(curr, _probe_pending_list, deferred_probe)
- seq_printf(s, "%s\n", dev_name(curr->device));
+ seq_printf(s, "%s\t%s", dev_name(curr->device),
+ curr->device->p->deferred_probe_msg ?: "\n");
mutex_unlock(_probe_mutex);
--
2.17.1
[RESEND PATCH v5 0/5] driver core: add probe error check helper
Hi All, Recently I took some time to re-check error handling in drivers probe code, and I have noticed that number of incorrect resource acquisition error handling increased and there are no other propositions which can cure the situation. So I have decided to resend my old proposition of probe_err helper which should simplify resource acquisition error handling, it also extend it with adding defer probe reason to devices_deferred debugfs property, which should improve debugging experience for developers/testers. In v5 I have also attached patch adding macro to replace quite frequent calls: probe_err(dev, PTR_ERR(ptr), ...) with probe_err(dev, ptr, ...) I have also added two patches showing usage and benefits of the helper. My dirty/ad-hoc cocci scripts shows that this helper can be used in at least 2700 places saving about 3500 lines of code. Regards Andrzej Andrzej Hajda (5): driver core: add probe_err log helper driver core: add deferring probe reason to devices_deferred property drivers core: allow probe_err accept integer and pointer types drm/bridge/sii8620: fix resource acquisition error handling drm/bridge: lvds-codec: simplify error handling code drivers/base/base.h | 3 +++ drivers/base/core.c | 20 drivers/base/dd.c| 21 - drivers/gpu/drm/bridge/lvds-codec.c | 9 ++--- drivers/gpu/drm/bridge/sil-sii8620.c | 18 ++ include/linux/device.h | 26 ++ 6 files changed, 77 insertions(+), 20 deletions(-) -- 2.17.1
Re: [PATCH v3] driver core: Break infinite loop when deferred probe can't be satisfied
On 09.06.2020 14:10, Marco Felsch wrote:
> On 20-06-09 11:27, Andrzej Hajda wrote:
>> On 09.06.2020 08:45, Marco Felsch wrote:
>>> On 20-06-08 13:11, Andrzej Hajda wrote:
>>>> On 08.06.2020 11:17, Marco Felsch wrote:
>>>>> On 20-03-26 18:31, Andy Shevchenko wrote:
>>>>>> On Thu, Mar 26, 2020 at 03:01:22PM +, Grant Likely wrote:
>>>>>>> On 25/03/2020 12:51, Andy Shevchenko wrote:
>>>>>>>> On Tue, Mar 24, 2020 at 08:29:01PM -0700, Saravana Kannan wrote:
>>>>>>>>> On Tue, Mar 24, 2020 at 5:38 AM Andy Shevchenko
>>>>>>>>> wrote:
>>>>>>>>>> Consider the following scenario.
>>>>>>>>>>
>>>>>>>>>> The main driver of USB OTG controller (dwc3-pci), which has the
>>>>>>>>>> following
>>>>>>>>>> functional dependencies on certain platform:
>>>>>>>>>> - ULPI (tusb1210)
>>>>>>>>>> - extcon (tested with extcon-intel-mrfld)
>>>>>>>>>>
>>>>>>>>>> Note, that first driver, tusb1210, is available at the moment of
>>>>>>>>>> dwc3-pci probing, while extcon-intel-mrfld is built as a module and
>>>>>>>>>> won't appear till user space does something about it.
>>>>>>>>>>
>>>>>>>>>> This is depicted by kernel configuration excerpt:
>>>>>>>>>>
>>>>>>>>>> CONFIG_PHY_TUSB1210=y
>>>>>>>>>> CONFIG_USB_DWC3=y
>>>>>>>>>> CONFIG_USB_DWC3_ULPI=y
>>>>>>>>>> CONFIG_USB_DWC3_DUAL_ROLE=y
>>>>>>>>>> CONFIG_USB_DWC3_PCI=y
>>>>>>>>>> CONFIG_EXTCON_INTEL_MRFLD=m
>>>>>>>>>>
>>>>>>>>>> In the Buildroot environment the modules are probed by alphabetical
>>>>>>>>>> ordering
>>>>>>>>>> of their modaliases. The latter comes to the case when USB OTG
>>>>>>>>>> driver will be
>>>>>>>>>> probed first followed by extcon one.
>>>>>>>>>>
>>>>>>>>>> So, if the platform anticipates extcon device to be appeared, in the
>>>>>>>>>> above case
>>>>>>>>>> we will get deferred probe of USB OTG, because of ordering.
>>>>>>>>>>
>>>>>>>>>> Since current implementation, done by the commit 58b116bce136
>>>>>>>>>> ("drivercore:
>>>>>>>>>> deferral race condition fix") counts the amount of triggered
>>>>>>>>>> deferred probe,
>>>>>>>>>> we never advance the situation -- the change makes it to be an
>>>>>>>>>> infinite loop.
>>>>>>>>> Hi Andy,
>>>>>>>>>
>>>>>>>>> I'm trying to understand this sequence of steps. Sorry if the
>>>>>>>>> questions
>>>>>>>>> are stupid -- I'm not very familiar with USB/PCI stuff.
>>>>>>>> Thank you for looking into this. My answer below.
>>>>>>>>
>>>>>>>> As a first thing I would like to tell that there is another example of
>>>>>>>> bad
>>>>>>>> behaviour of deferred probe with no relation to USB. The proposed
>>>>>>>> change also
>>>>>>>> fixes that one (however, less possible to find in real life).
>>>>>>>>
>>>>>>>>>> ---8<---8<---
>>>>>>>>>>
>>>>>>>>>> [ 22.187127] driver_deferred_probe_trigger <<< 1
>>>>>>>>>>
>>>>>>>>>> ...here is the late initcall triggers deferred probe...
>>>>>>>>>>
>>>>>>>>>> [ 22.191725] platform dwc3.0.auto: deferred_probe_work_func in
>>>>>>>>>> deferred list
>>>>>>>>>>
>>>>>>>>>> ...dwc3.0.auto is the only device in the deferred list...
>>>>>>>>> Ok,
Re: [PATCH v3] driver core: Break infinite loop when deferred probe can't be satisfied
On 09.06.2020 08:45, Marco Felsch wrote:
> On 20-06-08 13:11, Andrzej Hajda wrote:
>> On 08.06.2020 11:17, Marco Felsch wrote:
>>> On 20-03-26 18:31, Andy Shevchenko wrote:
>>>> On Thu, Mar 26, 2020 at 03:01:22PM +, Grant Likely wrote:
>>>>> On 25/03/2020 12:51, Andy Shevchenko wrote:
>>>>>> On Tue, Mar 24, 2020 at 08:29:01PM -0700, Saravana Kannan wrote:
>>>>>>> On Tue, Mar 24, 2020 at 5:38 AM Andy Shevchenko
>>>>>>> wrote:
>>>>>>>> Consider the following scenario.
>>>>>>>>
>>>>>>>> The main driver of USB OTG controller (dwc3-pci), which has the
>>>>>>>> following
>>>>>>>> functional dependencies on certain platform:
>>>>>>>> - ULPI (tusb1210)
>>>>>>>> - extcon (tested with extcon-intel-mrfld)
>>>>>>>>
>>>>>>>> Note, that first driver, tusb1210, is available at the moment of
>>>>>>>> dwc3-pci probing, while extcon-intel-mrfld is built as a module and
>>>>>>>> won't appear till user space does something about it.
>>>>>>>>
>>>>>>>> This is depicted by kernel configuration excerpt:
>>>>>>>>
>>>>>>>>CONFIG_PHY_TUSB1210=y
>>>>>>>>CONFIG_USB_DWC3=y
>>>>>>>>CONFIG_USB_DWC3_ULPI=y
>>>>>>>>CONFIG_USB_DWC3_DUAL_ROLE=y
>>>>>>>>CONFIG_USB_DWC3_PCI=y
>>>>>>>>CONFIG_EXTCON_INTEL_MRFLD=m
>>>>>>>>
>>>>>>>> In the Buildroot environment the modules are probed by alphabetical
>>>>>>>> ordering
>>>>>>>> of their modaliases. The latter comes to the case when USB OTG driver
>>>>>>>> will be
>>>>>>>> probed first followed by extcon one.
>>>>>>>>
>>>>>>>> So, if the platform anticipates extcon device to be appeared, in the
>>>>>>>> above case
>>>>>>>> we will get deferred probe of USB OTG, because of ordering.
>>>>>>>>
>>>>>>>> Since current implementation, done by the commit 58b116bce136
>>>>>>>> ("drivercore:
>>>>>>>> deferral race condition fix") counts the amount of triggered deferred
>>>>>>>> probe,
>>>>>>>> we never advance the situation -- the change makes it to be an
>>>>>>>> infinite loop.
>>>>>>> Hi Andy,
>>>>>>>
>>>>>>> I'm trying to understand this sequence of steps. Sorry if the questions
>>>>>>> are stupid -- I'm not very familiar with USB/PCI stuff.
>>>>>> Thank you for looking into this. My answer below.
>>>>>>
>>>>>> As a first thing I would like to tell that there is another example of
>>>>>> bad
>>>>>> behaviour of deferred probe with no relation to USB. The proposed change
>>>>>> also
>>>>>> fixes that one (however, less possible to find in real life).
>>>>>>
>>>>>>>> ---8<---8<---
>>>>>>>>
>>>>>>>> [ 22.187127] driver_deferred_probe_trigger <<< 1
>>>>>>>>
>>>>>>>> ...here is the late initcall triggers deferred probe...
>>>>>>>>
>>>>>>>> [ 22.191725] platform dwc3.0.auto: deferred_probe_work_func in
>>>>>>>> deferred list
>>>>>>>>
>>>>>>>> ...dwc3.0.auto is the only device in the deferred list...
>>>>>>> Ok, dwc3.0.auto is the only unprobed device at this point?
>>>>>> Correct.
>>>>>>
>>>>>>>> [ 22.198727] platform dwc3.0.auto: deferred_probe_work_func 1 <<<
>>>>>>>> counter 1
>>>>>>>>
>>>>>>>> ...the counter before mutex is unlocked is kept the same...
>>>>>>>>
>>>>>>>> [ 22.205663] platform dwc3.0.auto: Retrying from deferred list
>>>>>>>>
>>>>>>>> ...mutes has been unlocked, we try to re-probe the drive
Re: [PATCH v3] driver core: Break infinite loop when deferred probe can't be satisfied
On 08.06.2020 11:17, Marco Felsch wrote:
> On 20-03-26 18:31, Andy Shevchenko wrote:
>> On Thu, Mar 26, 2020 at 03:01:22PM +, Grant Likely wrote:
>>> On 25/03/2020 12:51, Andy Shevchenko wrote:
On Tue, Mar 24, 2020 at 08:29:01PM -0700, Saravana Kannan wrote:
> On Tue, Mar 24, 2020 at 5:38 AM Andy Shevchenko
> wrote:
>> Consider the following scenario.
>>
>> The main driver of USB OTG controller (dwc3-pci), which has the following
>> functional dependencies on certain platform:
>> - ULPI (tusb1210)
>> - extcon (tested with extcon-intel-mrfld)
>>
>> Note, that first driver, tusb1210, is available at the moment of
>> dwc3-pci probing, while extcon-intel-mrfld is built as a module and
>> won't appear till user space does something about it.
>>
>> This is depicted by kernel configuration excerpt:
>>
>> CONFIG_PHY_TUSB1210=y
>> CONFIG_USB_DWC3=y
>> CONFIG_USB_DWC3_ULPI=y
>> CONFIG_USB_DWC3_DUAL_ROLE=y
>> CONFIG_USB_DWC3_PCI=y
>> CONFIG_EXTCON_INTEL_MRFLD=m
>>
>> In the Buildroot environment the modules are probed by alphabetical
>> ordering
>> of their modaliases. The latter comes to the case when USB OTG driver
>> will be
>> probed first followed by extcon one.
>>
>> So, if the platform anticipates extcon device to be appeared, in the
>> above case
>> we will get deferred probe of USB OTG, because of ordering.
>>
>> Since current implementation, done by the commit 58b116bce136
>> ("drivercore:
>> deferral race condition fix") counts the amount of triggered deferred
>> probe,
>> we never advance the situation -- the change makes it to be an infinite
>> loop.
> Hi Andy,
>
> I'm trying to understand this sequence of steps. Sorry if the questions
> are stupid -- I'm not very familiar with USB/PCI stuff.
Thank you for looking into this. My answer below.
As a first thing I would like to tell that there is another example of bad
behaviour of deferred probe with no relation to USB. The proposed change
also
fixes that one (however, less possible to find in real life).
>> ---8<---8<---
>>
>> [ 22.187127] driver_deferred_probe_trigger <<< 1
>>
>> ...here is the late initcall triggers deferred probe...
>>
>> [ 22.191725] platform dwc3.0.auto: deferred_probe_work_func in
>> deferred list
>>
>> ...dwc3.0.auto is the only device in the deferred list...
> Ok, dwc3.0.auto is the only unprobed device at this point?
Correct.
>> [ 22.198727] platform dwc3.0.auto: deferred_probe_work_func 1 <<<
>> counter 1
>>
>> ...the counter before mutex is unlocked is kept the same...
>>
>> [ 22.205663] platform dwc3.0.auto: Retrying from deferred list
>>
>> ...mutes has been unlocked, we try to re-probe the driver...
>>
>> [ 22.211487] bus: 'platform': driver_probe_device: matched device
>> dwc3.0.auto with driver dwc3
>> [ 22.220060] bus: 'platform': really_probe: probing driver dwc3 with
>> device dwc3.0.auto
>> [ 22.238735] bus: 'ulpi': driver_probe_device: matched device
>> dwc3.0.auto.ulpi with driver tusb1210
>> [ 22.247743] bus: 'ulpi': really_probe: probing driver tusb1210 with
>> device dwc3.0.auto.ulpi
>> [ 22.256292] driver: 'tusb1210': driver_bound: bound to device
>> 'dwc3.0.auto.ulpi'
>> [ 22.263723] driver_deferred_probe_trigger <<< 2
>>
>> ...the dwc3.0.auto probes ULPI, we got successful bound and bumped
>> counter...
>>
>> [ 22.268304] bus: 'ulpi': really_probe: bound device dwc3.0.auto.ulpi
>> to driver tusb1210
> So where did this dwc3.0.auto.ulpi come from?
> Looks like the device is created by dwc3_probe() through this call flow:
> dwc3_probe() -> dwc3_core_init() -> dwc3_core_ulpi_init() ->
> dwc3_ulpi_init() -> ulpi_register_interface() -> ulpi_register()
Correct.
>> [ 22.276697] platform dwc3.0.auto: Driver dwc3 requests probe deferral
> Can you please point me to which code patch actually caused the probe
> deferral?
Sure, it's in drd.c.
if (device_property_read_string(dev, "linux,extcon-name", ) == 0) {
edev = extcon_get_extcon_dev(name);
if (!edev)
return ERR_PTR(-EPROBE_DEFER);
return edev;
}
>> ...but extcon driver is still missing...
>>
>> [ 22.283174] platform dwc3.0.auto: Added to deferred list
>> [ 22.288513] platform dwc3.0.auto: driver_deferred_probe_add_trigger
>> local counter: 1 new counter 2
> I'm not fully aware of all the USB implications, but if extcon is
> needed, why can't that check be done before we add and probe the ulpi
> device? That'll avoid this whole "fake" probing and avoid the counter
> increase. And avoid the need for this patch that's
Re: [PATCH 05/11] drm/bridge: analogix-anx78xx: correct value of TX_P0
On 16.09.2019 14:02, Brian Masney wrote: > On Mon, Sep 16, 2019 at 01:32:58PM +0200, Enric Balletbo i Serra wrote: >> Hi, >> >> On 16/9/19 12:49, Laurent Pinchart wrote: >>> Hi Brian, >>> >>> On Mon, Sep 16, 2019 at 06:36:14AM -0400, Brian Masney wrote: >>>> On Mon, Sep 16, 2019 at 12:02:09PM +0200, Andrzej Hajda wrote: >>>>> On 15.08.2019 02:48, Brian Masney wrote: >>>>>> When attempting to configure this driver on a Nexus 5 phone (msm8974), >>>>>> setting up the dummy i2c bus for TX_P0 would fail due to an -EBUSY >>>>>> error. The downstream MSM kernel sources [1] shows that the proper value >>>>>> for TX_P0 is 0x78, not 0x70, so correct the value to allow device >>>>>> probing to succeed. >>>>>> >>>>>> [1] >>>>>> https://github.com/AICP/kernel_lge_hammerhead/blob/n7.1/drivers/video/slimport/slimport_tx_reg.h >>>>>> >>>>>> Signed-off-by: Brian Masney >>>>>> --- >>>>>> drivers/gpu/drm/bridge/analogix-anx78xx.h | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.h >>>>>> b/drivers/gpu/drm/bridge/analogix-anx78xx.h >>>>>> index 25e063bcecbc..bc511fc605c9 100644 >>>>>> --- a/drivers/gpu/drm/bridge/analogix-anx78xx.h >>>>>> +++ b/drivers/gpu/drm/bridge/analogix-anx78xx.h >>>>>> @@ -6,7 +6,7 @@ >>>>>> #ifndef __ANX78xx_H >>>>>> #define __ANX78xx_H >>>>>> >>>>>> -#define TX_P0 0x70 >>>>>> +#define TX_P0 0x78 >>>>> >>>>> This bothers me little. There are no upstream users, grepping android >>>>> sources suggests that both values can be used [1][2] (grep for "#define >>>>> TX_P0"), moreover there is code suggesting both values can be valid [3]. >>>>> >>>>> Could you verify datasheet which i2c slave addresses are valid for this >>>>> chip, if both I guess this patch should be reworked. >>>>> >>>>> >>>>> [1]: >>>>> https://android.googlesource.com/kernel/msm/+/android-msm-flo-3.4-jb-mr2/drivers/misc/slimport_anx7808/slimport_tx_reg.h >>>>> >>>>> [2]: >>>>> https://github.com/AndroidGX/SimpleGX-MM-6.0_H815_20d/blob/master/drivers/video/slimport/anx7812/slimport7812_tx_reg.h >>>>> >>>>> [3]: >>>>> https://github.com/commaai/android_kernel_leeco_msm8996/blob/master/drivers/video/msm/mdss/dp/slimport_custom_declare.h#L73 >>>> This address is 0x78 on my Nexus 5. Given [3] above it looks like we >>>> need to support both addresses. What do you think about moving these >>>> addresses into device tree? >>> Assuming that the device supports different addresses (I can't validate >>> that as I don't have access to the datasheet), and different addresses >>> need to be used on different systems, then the address to be used needs >>> to be provided by the firmware (DT in this case). Two options are >>> possible, either specifying the address explicitly in the device's DT >>> node, or specifying free addresses (in the form of a white list or black >>> list) and allocating an address from that pool. The latter has been >>> discussed in a BoF at the Linux Plumbers Conference last week, >>> https://linuxplumbersconf.org/event/4/contributions/542/. >>> >>>> The downstream and upstream kernel sources divide these addresses by two >>>> to get the i2c address. Here's the code in upstream: >>>> >>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L1353 >>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/bridge/analogix-anx78xx.c#L41 >>>> >>>> I'm not sure why the actual i2c address isn't used in this code. >> The ANX7802/12/14/16 has a slave I2C bus that provides the interface to >> access >> or control the chip from the AP. The I2C slave addresses used to control the >> ANX7802/12/14/16 are 70h, 72h, 7Ah, 7Eh and 80h. Every address allows you to >> access to different registers of the chip and AFAICS is not configurable. >> >> I don't think these addresses should be configured via DT but for the driver >> its
Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic
On 12.09.2019 16:18, Matt Redfearn wrote: > > On 12/09/2019 14:21, Andrzej Hajda wrote: >> On 12.09.2019 04:38, John Stultz wrote: >>> On Wed, Sep 4, 2019 at 3:26 AM Andrzej Hajda wrote: >>>> On 03.09.2019 18:18, John Stultz wrote: >>>>> On Mon, Sep 2, 2019 at 6:22 AM Andrzej Hajda wrote: >>>>>> On 30.08.2019 19:00, Rob Clark wrote: >>>>>>> On Thu, Aug 29, 2019 at 11:52 PM Andrzej Hajda >>>>>>> wrote: >>>>>>>> Of course it seems you have different opinion what is the right thing >>>>>>>> in >>>>>>>> this case, so if you convince us that your approach is better one can >>>>>>>> revert the patch. >>>>>>> I guess my strongest / most immediate opinion is to not break other >>>>>>> existing adv75xx bridge users. >>>>>> It is pity that breakage happened, and next time we should be more >>>>>> strict about testing other platforms, before patch acceptance. >>>>>> >>>>>> But reverting it now will break also platform which depend on it. >>>>> I'm really of no opinion of which approach is better here, but I will >>>>> say that when a patch breaks previously working boards, that's a >>>>> regression and justifying that some other board is now enabled that >>>>> would be broken by the revert (of a patch that is not yet upstream) >>>>> isn't really a strong argument. >>>>> >>>>> I'm happy to work with folks to try to fixup the kirin driver if this >>>>> patch really is the right approach, but we need someone to do the same >>>>> for the db410c, and I don't think its fair to just dump that work onto >>>>> folks under the threat of the board breaking. >>>> These drivers should be fixed anyway - assumption that >>>> drm_bridge/drm_panel will be registered before the bus it is attached to >>>> is just incorrect. >>>> >>>> So instead of reverting, fixing and then re-applying the patch I have >>>> gently proposed shorter path. If you prefer long path we can try to go >>>> this way. >>>> >>>> Matt, is the pure revert OK for you or is it possible to prepare some >>>> workaround allowing cooperation with both approaches? >>> Rob/Andrzej: What's the call here? >>> >>> Should I resubmit the kirin fix for the adv7511 regression here? >>> Or do we revert the adv7511 patch? I believe db410c still needs a fix. >>> >>> I'd just like to keep the HiKey board from breaking, so let me know so >>> I can get the fix submitted if needed. >> >> Since there is no response from Matt, we can perform revert, since there >> are no other ideas. I will apply it tomorrow, if there are no objections. > Hi, > > Sorry - yeah I think reverting is probably best at this point to avoid > breaking in-tree platforms. > It's a shame though that there is a built-in incompatibility within the > tree between drivers. Quite common in development - some issues becomes visible after some time. > The way that the generic Synopsys DSI host driver > probes is currently incompatible with the ADV7533 (hence the patch), > other DSI host drivers are now compatible with the ADV7533 but break > when we change it to probe in a similar way to panel drivers. 1. The behavior should be consistent between all hosts/device drivers. 2. DSI controlled devices can expose drm objects (drm_bridge/drm_panel) only when they can probe, ie DSI bus they sit on must be created 1st. 1 and 2 enforces policy that all host drivers should 1st create control bus (DSI in this case) then look for higher level objects (drm_bridge/drm_panel). As a consequence all bridges and panels should 1st gather the resources they depends on, and then they can expose higher level objects. > >> And for the future: I guess it is not possible to make adv work with old >> and new approach, but simple workaround for adv could be added later: >> >> if (source is msm or kirin) >> >> do_the_old_way >> >> else >> >> do_correctly. > Maybe this would work, but how do we know that the list "msm or kirin" > is exhaustive to cope with all platforms? By checking dts/config files. > It seems to me the built in > incompatibility between DSI hosts needs to be resolved really rather > than trying to work around it in the ADV7533 driver (and any other DSI > bus device that falls into this trap). If you know how, please describe. Atm the only real solution I see is explicit workaround to allow new adv7511, then fixing controllers, together with workaround-removal. OK, it could be possible to change msm, kirin and adv in one patch, however I am not sure if this is the best solution. Regards Andrzej > > Anyway, my platform is out of tree so better to revert my patch and keep > the in-tree platforms working. > > Thanks everyone. > Matt > >> >> And remove it after fixing both dsi masters. >> >> >> Regards >> >> Andrzej >> >> >>> thanks >>> -john >>> >>>
Re: [PATCH 2/3] video: fbdev: mmp: add COMPILE_TEST support
On 27.06.2019 16:07, Bartlomiej Zolnierkiewicz wrote: > Add COMPILE_TEST support to mmp display subsystem for better compile > testing coverage. > > Signed-off-by: Bartlomiej Zolnierkiewicz Reviewed-by: Andrzej Hajda -- Regards Andrzej > --- > drivers/video/fbdev/mmp/Kconfig|2 +- > drivers/video/fbdev/mmp/hw/Kconfig |3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > Index: b/drivers/video/fbdev/mmp/Kconfig > === > --- a/drivers/video/fbdev/mmp/Kconfig > +++ b/drivers/video/fbdev/mmp/Kconfig > @@ -1,7 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0-only > menuconfig MMP_DISP > tristate "Marvell MMP Display Subsystem support" > - depends on CPU_PXA910 || CPU_MMP2 > + depends on CPU_PXA910 || CPU_MMP2 || COMPILE_TEST > help > Marvell Display Subsystem support. > > Index: b/drivers/video/fbdev/mmp/hw/Kconfig > === > --- a/drivers/video/fbdev/mmp/hw/Kconfig > +++ b/drivers/video/fbdev/mmp/hw/Kconfig > @@ -1,7 +1,8 @@ > # SPDX-License-Identifier: GPL-2.0-only > config MMP_DISP_CONTROLLER > bool "mmp display controller hw support" > - depends on CPU_PXA910 || CPU_MMP2 > + depends on HAVE_CLK && HAS_IOMEM > + depends on CPU_PXA910 || CPU_MMP2 || COMPILE_TEST > help > Marvell MMP display hw controller support > this controller is used on Marvell PXA910 and > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 00/12] treewide: Fix GENMASK misuses
Hi Joe, On 10.07.2019 07:04, Joe Perches wrote: > These GENMASK uses are inverted argument order and the > actual masks produced are incorrect. Fix them. > > Add checkpatch tests to help avoid more misuses too. > > Joe Perches (12): > checkpatch: Add GENMASK tests > clocksource/drivers/npcm: Fix misuse of GENMASK macro > drm: aspeed_gfx: Fix misuse of GENMASK macro > iio: adc: max9611: Fix misuse of GENMASK macro > irqchip/gic-v3-its: Fix misuse of GENMASK macro > mmc: meson-mx-sdio: Fix misuse of GENMASK macro > net: ethernet: mediatek: Fix misuses of GENMASK macro > net: stmmac: Fix misuses of GENMASK macro > rtw88: Fix misuse of GENMASK macro > phy: amlogic: G12A: Fix misuse of GENMASK macro > staging: media: cedrus: Fix misuse of GENMASK macro > ASoC: wcd9335: Fix misuse of GENMASK macro > > drivers/clocksource/timer-npcm7xx.c | 2 +- > drivers/gpu/drm/aspeed/aspeed_gfx.h | 2 +- > drivers/iio/adc/max9611.c | 2 +- > drivers/irqchip/irq-gic-v3-its.c | 2 +- > drivers/mmc/host/meson-mx-sdio.c | 2 +- > drivers/net/ethernet/mediatek/mtk_eth_soc.h | 2 +- > drivers/net/ethernet/mediatek/mtk_sgmii.c | 2 +- > drivers/net/ethernet/stmicro/stmmac/descs.h | 2 +- > drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 4 ++-- > drivers/net/wireless/realtek/rtw88/rtw8822b.c | 2 +- > drivers/phy/amlogic/phy-meson-g12a-usb2.c | 2 +- > drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 +- > scripts/checkpatch.pl | 15 +++ > sound/soc/codecs/wcd-clsh-v2.c| 2 +- > 14 files changed, 29 insertions(+), 14 deletions(-) > After adding following compile time check: -- diff --git a/Makefile b/Makefile index 5102b2bbd224..ac4ea5f443a9 100644 --- a/Makefile +++ b/Makefile @@ -457,7 +457,7 @@ KBUILD_AFLAGS := -D__ASSEMBLY__ -fno-PIE KBUILD_CFLAGS := -Wall -Wundef -Werror=strict-prototypes -Wno-trigraphs \ -fno-strict-aliasing -fno-common -fshort-wchar -fno-PIE \ -Werror=implicit-function-declaration -Werror=implicit-int \ - -Wno-format-security \ + -Wno-format-security -Werror=div-by-zero \ -std=gnu89 KBUILD_CPPFLAGS := -D__KERNEL__ KBUILD_AFLAGS_KERNEL := diff --git a/include/linux/bits.h b/include/linux/bits.h index 669d69441a62..61d74b103055 100644 --- a/include/linux/bits.h +++ b/include/linux/bits.h @@ -19,11 +19,11 @@ * GENMASK_ULL(39, 21) gives us the 64bit vector 0x00e0. */ #define GENMASK(h, l) \ - (((~UL(0)) - (UL(1) << (l)) + 1) & \ + (((~UL(0)) - (UL(1) << (l)) + 1 + 0/((h) >= (l))) & \ (~UL(0) >> (BITS_PER_LONG - 1 - (h #define GENMASK_ULL(h, l) \ - (((~ULL(0)) - (ULL(1) << (l)) + 1) & \ + (((~ULL(0)) - (ULL(1) << (l)) + 1 + 0/((h) >= (l))) & \ (~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h #endif /* __LINUX_BITS_H */ --- I was able to detect one more GENMASK misue (AARCH64, allyesconfig): CC drivers/phy/rockchip/phy-rockchip-inno-hdmi.o In file included from ../include/linux/bitops.h:5:0, from ../include/linux/kernel.h:12, from ../include/linux/clk.h:13, from ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:9: ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c: In function ‘inno_hdmi_phy_rk3328_power_on’: ../include/linux/bits.h:22:37: error: division by zero [-Werror=div-by-zero] (((~UL(0)) - (UL(1) << (l)) + 1 + 0/((h) >= (l))) & \ ^ ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:24:42: note: in expansion of macro ‘GENMASK’ #define UPDATE(x, h, l) (((x) << (l)) & GENMASK((h), (l))) ^~~ ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:201:50: note: in expansion of macro ‘UPDATE’ #define RK3328_TERM_RESISTOR_CALIB_SPEED_7_0(x) UPDATE(x, 7, 9) ^~ ../drivers/phy/rockchip/phy-rockchip-inno-hdmi.c:1046:26: note: in expansion of macro ‘RK3328_TERM_RESISTOR_CALIB_SPEED_7_0’ inno_write(inno, 0xc6, RK3328_TERM_RESISTOR_CALIB_SPEED_7_0(v)); Of course I do not advise to add the check as is to Kernel - it is undefined behavior area AFAIK. Regards Andrzej
Re: [PATCH 6/6] media: i2c: Convert to new i2c device probe()
On 10.07.2019 23:51, Kieran Bingham wrote:
> The I2C core framework provides a simplified probe framework from commit
> b8a1a4cd5a98 ("i2c: Provide a temporary .probe_new() call-back type").
>
> These drivers do not utilise the i2c_device_id table in the probe, so we
> can easily convert them to utilise the simplfied i2c driver
> registration.
>
> Signed-off-by: Kieran Bingham
If needed, for S5K5BAF:
Acked-by: Andrzej Hajda
--
Regards
Andrzej
> ---
> drivers/media/i2c/adv7343.c | 5 ++---
> drivers/media/i2c/imx274.c | 5 ++---
> drivers/media/i2c/max2175.c | 5 ++---
> drivers/media/i2c/mt9m001.c | 5 ++---
> drivers/media/i2c/mt9m111.c | 5 ++---
> drivers/media/i2c/ov2640.c | 5 ++---
> drivers/media/i2c/ov2659.c | 5 ++---
> drivers/media/i2c/ov5640.c | 5 ++---
> drivers/media/i2c/ov5645.c | 5 ++---
> drivers/media/i2c/ov5647.c | 5 ++---
> drivers/media/i2c/ov772x.c | 5 ++---
> drivers/media/i2c/ov7740.c | 5 ++---
> drivers/media/i2c/ov9650.c | 5 ++---
> drivers/media/i2c/s5k5baf.c | 5 ++---
> drivers/media/i2c/s5k6a3.c | 5 ++---
> drivers/media/i2c/tc358743.c | 5 ++---
> drivers/media/i2c/ths8200.c | 5 ++---
> drivers/media/i2c/tvp5150.c | 5 ++---
> drivers/media/i2c/tvp7002.c | 4 ++--
> 19 files changed, 38 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/media/i2c/adv7343.c b/drivers/media/i2c/adv7343.c
> index 4a441ee99dd8..63e94dfcb5d3 100644
> --- a/drivers/media/i2c/adv7343.c
> +++ b/drivers/media/i2c/adv7343.c
> @@ -428,8 +428,7 @@ adv7343_get_pdata(struct i2c_client *client)
> return pdata;
> }
>
> -static int adv7343_probe(struct i2c_client *client,
> - const struct i2c_device_id *id)
> +static int adv7343_probe(struct i2c_client *client)
> {
> struct adv7343_state *state;
> int err;
> @@ -524,7 +523,7 @@ static struct i2c_driver adv7343_driver = {
> .of_match_table = of_match_ptr(adv7343_of_match),
> .name = "adv7343",
> },
> - .probe = adv7343_probe,
> + .probe_new = adv7343_probe,
> .remove = adv7343_remove,
> .id_table = adv7343_id,
> };
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index f3ff1af209f9..6011cec5e351 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -1821,8 +1821,7 @@ static const struct i2c_device_id imx274_id[] = {
> };
> MODULE_DEVICE_TABLE(i2c, imx274_id);
>
> -static int imx274_probe(struct i2c_client *client,
> - const struct i2c_device_id *id)
> +static int imx274_probe(struct i2c_client *client)
> {
> struct v4l2_subdev *sd;
> struct stimx274 *imx274;
> @@ -1984,7 +1983,7 @@ static struct i2c_driver imx274_i2c_driver = {
> .name = DRIVER_NAME,
> .of_match_table = imx274_of_id_table,
> },
> - .probe = imx274_probe,
> + .probe_new = imx274_probe,
> .remove = imx274_remove,
> .id_table = imx274_id,
> };
> diff --git a/drivers/media/i2c/max2175.c b/drivers/media/i2c/max2175.c
> index 7b226fadcdb8..19a3ceea3bc2 100644
> --- a/drivers/media/i2c/max2175.c
> +++ b/drivers/media/i2c/max2175.c
> @@ -1271,8 +1271,7 @@ static int max2175_refout_load_to_bits(struct
> i2c_client *client, u32 load,
> return 0;
> }
>
> -static int max2175_probe(struct i2c_client *client,
> - const struct i2c_device_id *id)
> +static int max2175_probe(struct i2c_client *client)
> {
> bool master = true, am_hiz = false;
> u32 refout_load, refout_bits = 0; /* REFOUT disabled */
> @@ -1433,7 +1432,7 @@ static struct i2c_driver max2175_driver = {
> .name = DRIVER_NAME,
> .of_match_table = max2175_of_ids,
> },
> - .probe = max2175_probe,
> + .probe_new = max2175_probe,
> .remove = max2175_remove,
> .id_table = max2175_id,
> };
> diff --git a/drivers/media/i2c/mt9m001.c b/drivers/media/i2c/mt9m001.c
> index 2df743cbe09d..5613072908ac 100644
> --- a/drivers/media/i2c/mt9m001.c
> +++ b/drivers/media/i2c/mt9m001.c
> @@ -726,8 +726,7 @@ static const struct v4l2_subdev_ops mt9m001_subdev_ops = {
> .pad= _subdev_pad_ops,
> };
>
> -static int mt9m001_probe(struct i2c_client *client,
> - const struct i2c_device_id *did)
> +static int mt9m001_probe(struct i2c_client *client)
> {
> struct mt9m001 *mt9m001;
> struct i2c_adapter *adapter = client->adapter;
> @@ -872,7 +871,7 @@ static struct i2c_driv
Re: [PATCH 4/6] media: i2c: s5c73m3: Convert to new i2c device probe()
On 10.07.2019 23:51, Kieran Bingham wrote:
> The I2C core framework provides a simplified probe framework from commit
> b8a1a4cd5a98 ("i2c: Provide a temporary .probe_new() call-back type").
>
> This driver does not utilise the i2c_device_id table in the probe, so we can
> easily convert it to utilise the simplfied i2c driver registration.
>
> Signed-off-by: Kieran Bingham
Acked-by: Andrzej Hajda
--
Regards
Andrzej
Re: [PATCH -next] drm/bridge: sii902x: Make sii902x_audio_digital_mute static
On 14.06.2019 17:36, YueHaibing wrote:
> Fix sparse warning:
>
> drivers/gpu/drm/bridge/sii902x.c:665:5: warning:
> symbol 'sii902x_audio_digital_mute' was not declared. Should it be static?
>
> Reported-by: Hulk Robot
> Signed-off-by: YueHaibing
> ---
> drivers/gpu/drm/bridge/sii902x.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/sii902x.c
> b/drivers/gpu/drm/bridge/sii902x.c
> index dd7aa46..c2f97e5 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -662,7 +662,8 @@ static void sii902x_audio_shutdown(struct device *dev,
> void *data)
> clk_disable_unprepare(sii902x->audio.mclk);
> }
>
> -int sii902x_audio_digital_mute(struct device *dev, void *data, bool enable)
> +static int sii902x_audio_digital_mute(struct device *dev,
> + void *data, bool enable)
> {
> struct sii902x *sii902x = dev_get_drvdata(dev);
>
Thanks,
Applied to drm-misc-next.
Regards
Andrzej
Re: [PATCH v7 1/2] drm/bridge: sil_sii8620: make remote control optional.
On 19.04.2019 10:19, Ronald Tschalär wrote: > commit d6abe6df706c (drm/bridge: sil_sii8620: do not have a dependency > of RC_CORE) changed the driver to select both RC_CORE and INPUT. > However, this causes problems with other drivers, in particular an input > driver that depends on MFD_INTEL_LPSS_PCI (to be added in a separate > commit): > > drivers/clk/Kconfig:9:error: recursive dependency detected! > drivers/clk/Kconfig:9:symbol COMMON_CLK is selected by > MFD_INTEL_LPSS > drivers/mfd/Kconfig:566: symbol MFD_INTEL_LPSS is selected by > MFD_INTEL_LPSS_PCI > drivers/mfd/Kconfig:580: symbol MFD_INTEL_LPSS_PCI is implied by > KEYBOARD_APPLESPI > drivers/input/keyboard/Kconfig:73:symbol KEYBOARD_APPLESPI depends on > INPUT > drivers/input/Kconfig:8: symbol INPUT is selected by DRM_SIL_SII8620 > drivers/gpu/drm/bridge/Kconfig:83:symbol DRM_SIL_SII8620 depends on > DRM_BRIDGE > drivers/gpu/drm/bridge/Kconfig:1: symbol DRM_BRIDGE is selected by > DRM_PL111 > drivers/gpu/drm/pl111/Kconfig:1: symbol DRM_PL111 depends on COMMON_CLK > > According to the docs and general consensus, select should only be used > for non user-visible symbols, but both RC_CORE and INPUT are > user-visible. Furthermore almost all other references to INPUT > throughout the kernel config are depends, not selects. For this reason > the first part of this change reverts commit d6abe6df706c. > > In order to address the original reason for commit d6abe6df706c, namely > that not all boards use the remote controller functionality and hence > should not need have to deal with RC_CORE, the second part of this > change now makes the remote control support in the driver optional and > contingent on RC_CORE being defined. And with this the hard dependency > on INPUT also goes away as that is only needed if RC_CORE is defined > (which in turn already depends on INPUT). > > CC: Inki Dae > CC: Andrzej Hajda > CC: Laurent Pinchart > CC: Dmitry Torokhov > Signed-off-by: Ronald Tschalär > Reviewed-by: Andrzej Hajda Apparently this patch was not queued to kernel yet. If there are no objections I will queue it via drm-misc-next tree tomorrow. Regards Andrzej
Re: [PATCH 2/4] drm/rockchip: Enable DRM InfoFrame support on RK3328 and RK3399
On 26.05.2019 23:20, Jonas Karlman wrote:
> This patch enables Dynamic Range and Mastering InfoFrame on RK3328 and RK3399.
>
> Cc: Sandy Huang
> Cc: Heiko Stuebner
> Signed-off-by: Jonas Karlman
Reviewed-by: Andrzej Hajda
--
Regards
Andrzej
> ---
> drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index 4cdc9f86c2e5..1f31f3726f04 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -405,6 +405,7 @@ static const struct dw_hdmi_plat_data
> rk3328_hdmi_drv_data = {
> .phy_ops = _hdmi_phy_ops,
> .phy_name = "inno_dw_hdmi_phy2",
> .phy_force_vendor = true,
> + .drm_infoframe = true,
> };
>
> static struct rockchip_hdmi_chip_data rk3399_chip_data = {
> @@ -419,6 +420,7 @@ static const struct dw_hdmi_plat_data
> rk3399_hdmi_drv_data = {
> .cur_ctr= rockchip_cur_ctr,
> .phy_config = rockchip_phy_config,
> .phy_data = _chip_data,
> + .drm_infoframe = true,
> };
>
> static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
Re: [PATCH v2 3/7] drm/bridge: extract some Analogix I2C DP common code
sg)
> {
> struct anx78xx *anx78xx = container_of(aux, struct anx78xx, aux);
> - u8 ctrl1 = msg->request;
> - u8 ctrl2 = SP_AUX_EN;
> - u8 *buffer = msg->buffer;
> - int err;
> -
> - /* The DP AUX transmit and receive buffer has 16 bytes. */
> - if (WARN_ON(msg->size > AUX_CH_BUFFER_SIZE))
> - return -E2BIG;
> -
> - /* Zero-sized messages specify address-only transactions. */
> - if (msg->size < 1)
> - ctrl2 |= SP_ADDR_ONLY;
> - else/* For non-zero-sized set the length field. */
> - ctrl1 |= (msg->size - 1) << SP_AUX_LENGTH_SHIFT;
> -
> - if ((msg->request & DP_AUX_I2C_READ) == 0) {
> - /* When WRITE | MOT write values to data buffer */
> - err = regmap_bulk_write(anx78xx->map[I2C_IDX_TX_P0],
> - SP_DP_BUF_DATA0_REG, buffer,
> - msg->size);
> - if (err)
> - return err;
> - }
> -
> - /* Write address and request */
> - err = anx78xx_aux_address(anx78xx, msg->address);
> - if (err)
> - return err;
> -
> - err = regmap_write(anx78xx->map[I2C_IDX_TX_P0], SP_DP_AUX_CH_CTRL1_REG,
> -ctrl1);
> - if (err)
> - return err;
> -
> - /* Start transaction */
> - err = regmap_update_bits(anx78xx->map[I2C_IDX_TX_P0],
> - SP_DP_AUX_CH_CTRL2_REG, SP_ADDR_ONLY |
> - SP_AUX_EN, ctrl2);
> - if (err)
> - return err;
> -
> - err = anx78xx_aux_wait(anx78xx);
> - if (err)
> - return err;
> -
> - msg->reply = DP_AUX_I2C_REPLY_ACK;
> -
> - if ((msg->size > 0) && (msg->request & DP_AUX_I2C_READ)) {
> - /* Read values from data buffer */
> - err = regmap_bulk_read(anx78xx->map[I2C_IDX_TX_P0],
> -SP_DP_BUF_DATA0_REG, buffer,
> -msg->size);
> - if (err)
> - return err;
> - }
> -
> - err = anx78xx_clear_bits(anx78xx->map[I2C_IDX_TX_P0],
> - SP_DP_AUX_CH_CTRL2_REG, SP_ADDR_ONLY);
> - if (err)
> - return err;
> -
> - return msg->size;
> + return anx_dp_aux_transfer(anx78xx->map[I2C_IDX_TX_P0], msg);
> }
>
> static int anx78xx_set_hpd(struct anx78xx *anx78xx)
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.c
> b/drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.c
> new file mode 100644
> index ..d6016f789d80
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/analogix/analogix-i2c-dptx.c
> @@ -0,0 +1,174 @@
> +/*
> + * Copyright(c) 2016, Analogix Semiconductor.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
Again spdx.
With that fixed:
Reviewed-by: Andrzej Hajda
--
Regards
Andrzej
> + *
> + * Based on anx7808 driver obtained from chromeos with copyright:
> + * Copyright(c) 2013, Google Inc.
> + *
> + */
> +#include
> +
> +#include
> +#include
> +#include
> +
> +#include "analogix-i2c-dptx.h"
> +
> +#define AUX_WAIT_TIMEOUT_MS 15
> +#define AUX_CH_BUFFER_SIZE 16
> +
> +static int anx_i2c_dp_clear_bits(struct regmap *map, u8 reg, u8 mask)
> +{
> + return regmap_update_bits(map, reg, mask, 0);
> +}
> +
> +static bool anx_dp_aux_op_finished(struct regmap *map_dptx)
> +{
> + unsigned int value;
> + int err;
> +
> + err = regmap_read(map_dptx, SP_DP_AUX_CH_CTRL2_REG, );
> + if (err < 0)
> + return false;
> +
> + return (value & SP_AUX_EN) == 0;
> +}
> +
> +static int anx_dp_aux_wait(struct regmap *map_dptx)
> +{
> + unsigned long timeout;
> + unsigned int status;
> + int err;
> +
> + timeout = jiffies + msecs_to_jiffies(AUX_WAIT_TIMEOUT_MS) + 1;
> +
> + while (!anx_dp_aux_op_finished(map_dptx)) {
> + if (time_after(jiffies, timeout)) {
> + if (!anx_dp_aux_op_finished(map_dptx)) {
> +
Re: [PATCH v4 10/15] drm/bridge: tc358767: Add support for address-only I2C transfers
On 07.06.2019 06:45, Andrey Smirnov wrote:
> Transfer size of zero means a request to do an address-only
> transfer. Since the HW support this, we probably shouldn't be just
> ignoring such requests. While at it allow DP_AUX_I2C_MOT flag to pass
> through, since it is supported by the HW as well.
>
> Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
--
Regards
Andrzej
> Cc: Andrzej Hajda
> Cc: Laurent Pinchart
> Cc: Tomi Valkeinen
> Cc: Andrey Gusakov
> Cc: Philipp Zabel
> Cc: Cory Tusar
> Cc: Chris Healy
> Cc: Lucas Stach
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/gpu/drm/bridge/tc358767.c | 30 +++---
> 1 file changed, 23 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c
> b/drivers/gpu/drm/bridge/tc358767.c
> index 7d0fbb12195b..4bb9b15e1324 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -145,6 +145,8 @@
>
> /* AUX channel */
> #define DP0_AUXCFG0 0x0660
> +#define DP0_AUXCFG0_BSIZEGENMASK(11, 8)
> +#define DP0_AUXCFG0_ADDR_ONLYBIT(4)
> #define DP0_AUXCFG1 0x0664
> #define AUX_RX_FILTER_EN BIT(16)
>
> @@ -327,6 +329,18 @@ static int tc_aux_read_data(struct tc_data *tc, void
> *data, size_t size)
> return size;
> }
>
> +static u32 tc_auxcfg0(struct drm_dp_aux_msg *msg, size_t size)
> +{
> + u32 auxcfg0 = msg->request;
> +
> + if (size)
> + auxcfg0 |= FIELD_PREP(DP0_AUXCFG0_BSIZE, size - 1);
> + else
> + auxcfg0 |= DP0_AUXCFG0_ADDR_ONLY;
> +
> + return auxcfg0;
> +}
> +
> static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
> struct drm_dp_aux_msg *msg)
> {
> @@ -336,9 +350,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
> u32 auxstatus;
> int ret;
>
> - if (size == 0)
> - return 0;
> -
> ret = tc_aux_wait_busy(tc, 100);
> if (ret)
> return ret;
> @@ -362,8 +373,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
> if (ret)
> return ret;
> /* Start transfer */
> - ret = regmap_write(tc->regmap, DP0_AUXCFG0,
> -((size - 1) << 8) | request);
> + ret = regmap_write(tc->regmap, DP0_AUXCFG0, tc_auxcfg0(msg, size));
> if (ret)
> return ret;
>
> @@ -377,8 +387,14 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
>
> if (auxstatus & AUX_TIMEOUT)
> return -ETIMEDOUT;
> -
> - size = FIELD_GET(AUX_BYTES, auxstatus);
> + /*
> + * For some reason address-only DP_AUX_I2C_WRITE (MOT), still
> + * reports 1 byte transferred in its status. To deal we that
> + * we ignore aux_bytes field if we know that this was an
> + * address-only transfer
> + */
> + if (size)
> + size = FIELD_GET(AUX_BYTES, auxstatus);
> msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
>
> switch (request) {
Re: [PATCH v4 09/15] drm/bridge: tc358767: Use reported AUX transfer size
On 07.06.2019 06:45, Andrey Smirnov wrote:
> Don't assume that requested data transfer size is the same as amount
> of data that was transferred. Change the code to get that information
> from DP0_AUXSTATUS instead.
>
> Since the check for AUX_BUSY in tc_aux_get_status() is pointless (it
> will always called after tc_aux_wait_busy()) and there's only one user
> of it, inline its code into tc_aux_transfer() instead of trying to
> accommodate the change above.
>
> Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
--
Regards
Andrzej
> Cc: Andrzej Hajda
> Cc: Laurent Pinchart
> Cc: Tomi Valkeinen
> Cc: Andrey Gusakov
> Cc: Philipp Zabel
> Cc: Cory Tusar
> Cc: Chris Healy
> Cc: Lucas Stach
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/gpu/drm/bridge/tc358767.c | 40 ++-
> 1 file changed, 12 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c
> b/drivers/gpu/drm/bridge/tc358767.c
> index 8b53dc8908d3..7d0fbb12195b 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -152,10 +152,10 @@
> #define DP0_AUXWDATA(i) (0x066c + (i) * 4)
> #define DP0_AUXRDATA(i) (0x067c + (i) * 4)
> #define DP0_AUXSTATUS0x068c
> -#define AUX_STATUS_MASK 0xf0
> -#define AUX_STATUS_SHIFT 4
> -#define AUX_TIMEOUT BIT(1)
> -#define AUX_BUSY BIT(0)
> +#define AUX_BYTESGENMASK(15, 8)
> +#define AUX_STATUS GENMASK(7, 4)
> +#define AUX_TIMEOUT BIT(1)
> +#define AUX_BUSY BIT(0)
> #define DP0_AUXI2CADR0x0698
>
> /* Link Training */
> @@ -298,29 +298,6 @@ static int tc_aux_wait_busy(struct tc_data *tc, unsigned
> int timeout_ms)
> 1000, 1000 * timeout_ms);
> }
>
> -static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
> -{
> - int ret;
> - u32 value;
> -
> - ret = regmap_read(tc->regmap, DP0_AUXSTATUS, );
> - if (ret < 0)
> - return ret;
> -
> - if (value & AUX_BUSY) {
> - dev_err(tc->dev, "aux busy!\n");
> - return -EBUSY;
> - }
> -
> - if (value & AUX_TIMEOUT) {
> - dev_err(tc->dev, "aux access timeout!\n");
> - return -ETIMEDOUT;
> - }
> -
> - *reply = (value & AUX_STATUS_MASK) >> AUX_STATUS_SHIFT;
> - return 0;
> -}
> -
> static int tc_aux_write_data(struct tc_data *tc, const void *data,
>size_t size)
> {
> @@ -356,6 +333,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
> struct tc_data *tc = aux_to_tc(aux);
> size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
> u8 request = msg->request & ~DP_AUX_I2C_MOT;
> + u32 auxstatus;
> int ret;
>
> if (size == 0)
> @@ -393,10 +371,16 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
> if (ret)
> return ret;
>
> - ret = tc_aux_get_status(tc, >reply);
> + ret = regmap_read(tc->regmap, DP0_AUXSTATUS, );
> if (ret)
> return ret;
>
> + if (auxstatus & AUX_TIMEOUT)
> + return -ETIMEDOUT;
> +
> + size = FIELD_GET(AUX_BYTES, auxstatus);
> + msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
> +
> switch (request) {
> case DP_AUX_NATIVE_READ:
> case DP_AUX_I2C_READ:
Re: [PATCH v3 06/15] drm/bridge: tc358767: Simplify AUX data read
On 05.06.2019 09:04, Andrey Smirnov wrote:
> Simplify AUX data read by removing index arithmetic and shifting with
> a helper functions that does three things:
>
> 1. Fetch data from up to 4 32-bit registers from the chip
> 2. Optionally fix data endianness (not needed on LE hosts)
> 3. Copy read data into user provided array.
>
> Signed-off-by: Andrey Smirnov
> Cc: Archit Taneja
> Cc: Andrzej Hajda
> Cc: Laurent Pinchart
> Cc: Tomi Valkeinen
> Cc: Andrey Gusakov
> Cc: Philipp Zabel
> Cc: Cory Tusar
> Cc: Chris Healy
> Cc: Lucas Stach
> Cc: dri-de...@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> ---
> drivers/gpu/drm/bridge/tc358767.c | 40 +--
> 1 file changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c
> b/drivers/gpu/drm/bridge/tc358767.c
> index e197ce0fb166..da47d81e7109 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -321,6 +321,29 @@ static int tc_aux_get_status(struct tc_data *tc, u8
> *reply)
> return 0;
> }
>
> +static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
> +{
> + u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
> + int ret, i, count = DIV_ROUND_UP(size, sizeof(u32));
> +
> + ret = regmap_bulk_read(tc->regmap, DP0_AUXRDATA(0), auxrdata, count);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < count; i++) {
> + /*
> + * Our regmap is configured as LE for register data,
> + * so we need undo any byte swapping that might have
> + * happened to preserve original byte order.
> + */
> + le32_to_cpus([i]);
> + }
> +
> + memcpy(data, auxrdata, size);
> +
> + return size;
> +}
> +
Hmm, cannot we just use regmap_raw_read?
Beside this:
Reviewed-by: Andrzej Hajda
--
Regards
Andrzej
> static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
> struct drm_dp_aux_msg *msg)
> {
> @@ -379,19 +402,10 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
> if (ret)
> return ret;
>
> - if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
> - /* Read data */
> - while (i < size) {
> - if ((i % 4) == 0) {
> - ret = regmap_read(tc->regmap,
> - DP0_AUXRDATA(i >> 2), );
> - if (ret)
> - return ret;
> - }
> - buf[i] = tmp & 0xff;
> - tmp = tmp >> 8;
> - i++;
> - }
> + switch (request) {
> + case DP_AUX_NATIVE_READ:
> + case DP_AUX_I2C_READ:
> + return tc_aux_read_data(tc, msg->buffer, size);
> }
>
> return size;
Re: [PATCH v3 04/15] drm/bridge: tc358767: Simplify tc_set_video_mode()
On 05.06.2019 09:04, Andrey Smirnov wrote: > Simplify tc_set_video_mode() by replacing explicit shifting using > macros from . No functional change intended. > > Signed-off-by: Andrey Smirnov > Cc: Archit Taneja > Cc: Andrzej Hajda > Cc: Laurent Pinchart > Cc: Tomi Valkeinen > Cc: Andrey Gusakov > Cc: Philipp Zabel > Cc: Cory Tusar > Cc: Chris Healy > Cc: Lucas Stach > Cc: dri-de...@lists.freedesktop.org > Cc: linux-kernel@vger.kernel.org Reviewed-by: Andrzej Hajda -- Regards Andrzej > --- > drivers/gpu/drm/bridge/tc358767.c | 106 ++ > 1 file changed, 78 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c > b/drivers/gpu/drm/bridge/tc358767.c > index 115cffc55a96..c0fc686ce5ec 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -24,6 +24,7 @@ > * GNU General Public License for more details. > */ > > +#include > #include > #include > #include > @@ -56,6 +57,7 @@ > > /* Video Path */ > #define VPCTRL0 0x0450 > +#define VSDELAY GENMASK(31, 20) > #define OPXLFMT_RGB666 (0 << 8) > #define OPXLFMT_RGB888 (1 << 8) > #define FRMSYNC_DISABLED (0 << 4) /* Video Timing Gen Disabled */ > @@ -63,9 +65,17 @@ > #define MSF_DISABLED (0 << 0) /* Magic Square FRC disabled */ > #define MSF_ENABLED (1 << 0) /* Magic Square FRC enabled */ > #define HTIM01 0x0454 > +#define HPW GENMASK(8, 0) > +#define HBPR GENMASK(24, 16) > #define HTIM02 0x0458 > +#define HDISPR GENMASK(10, 0) > +#define HFPR GENMASK(24, 16) > #define VTIM01 0x045c > +#define VSPR GENMASK(7, 0) > +#define VBPR GENMASK(23, 16) > #define VTIM02 0x0460 > +#define VFPR GENMASK(23, 16) > +#define VDISPR GENMASK(10, 0) > #define VFUEN0 0x0464 > #define VFUENBIT(0) /* Video Frame Timing > Upload */ > > @@ -108,14 +118,28 @@ > /* Main Channel */ > #define DP0_SECSAMPLE0x0640 > #define DP0_VIDSYNCDELAY 0x0644 > +#define VID_SYNC_DLY GENMASK(15, 0) > +#define THRESH_DLY GENMASK(31, 16) > + > #define DP0_TOTALVAL 0x0648 > +#define H_TOTAL GENMASK(15, 0) > +#define V_TOTAL GENMASK(31, 16) > #define DP0_STARTVAL 0x064c > +#define H_START GENMASK(15, 0) > +#define V_START GENMASK(31, 16) > #define DP0_ACTIVEVAL0x0650 > +#define H_ACTGENMASK(15, 0) > +#define V_ACTGENMASK(31, 16) > + > #define DP0_SYNCVAL 0x0654 > +#define VS_WIDTH GENMASK(30, 16) > +#define HS_WIDTH GENMASK(14, 0) > #define SYNCVAL_HS_POL_ACTIVE_LOW(1 << 15) > #define SYNCVAL_VS_POL_ACTIVE_LOW(1 << 31) > #define DP0_MISC 0x0658 > #define TU_SIZE_RECOMMENDED (63) /* LSCLK cycles per TU */ > +#define MAX_TU_SYMBOLGENMASK(28, 23) > +#define TU_SIZE GENMASK(21, 16) > #define BPC_6(0 << 5) > #define BPC_8(1 << 5) > > @@ -192,6 +216,12 @@ > > /* Test & Debug */ > #define TSTCTL 0x0a00 > +#define COLOR_R GENMASK(31, 24) > +#define COLOR_G GENMASK(23, 16) > +#define COLOR_B GENMASK(15, 8) > +#define ENI2CFILTER BIT(4) > +#define COLOR_BAR_MODE GENMASK(1, 0) > +#define COLOR_BAR_MODE_BARS 2 > #define PLL_DBG 0x0a04 > > static bool tc_test_pattern; > @@ -672,6 +702,7 @@ static int tc_set_video_mode(struct tc_data *tc, > int upper_margin = mode->vtotal - mode->vsync_end; > int lower_margin = mode->vsync_start - mode->vdisplay; > int vsync_len = mode->vsync_end - mode->vsync_start; > + u32 dp0_syncval; > > /* >* Recommended maximum number of symbols transferred in a transfer unit: > @@ -696,50 +727,69 @@ static int tc_set_video_mode(struct tc_data *tc, >* assume we do not need any delay when DPI is a source of >* sync signals >*/ > - tc_write(VPCTRL0, (0 << 20) /* VSDE
Re: [PATCH v2] drm/bridge: Remove duplicate header
On 16.05.2019 17:25, Sabyasachi Gupta wrote:
> Remove duplicate header which is included twice
>
> Signed-off-by: Sabyasachi Gupta
Queued to drm-misc-next.
Regards
Andrzej
> ---
> v2: rebased the code against drm -next and arranged the headers alphabetically
>
> drivers/gpu/drm/bridge/panel.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 38eeaf8..000ba7c 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -9,13 +9,12 @@
> */
>
> #include
> -#include
> #include
> #include
> #include
> #include
> -#include
> #include
> +#include
>
> struct panel_bridge {
> struct drm_bridge bridge;
Re: [PATCH] drm/bridge/synopsys: dsi: Don't blindly call post_disable
On 24.04.2019 16:22, Matt Redfearn wrote:
> The DRM documentation states that post_disable is an optional callback.
> As such an implementing device may not populate it. To avoid panicing
> the kernel by calling a NULL function pointer, we should NULL check it
> before blindy calling it.
>
> Signed-off-by: Matt Redfearn
> ---
>
> drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 38e88071363..0ee440216b8 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -805,7 +805,8 @@ static void dw_mipi_dsi_bridge_post_disable(struct
> drm_bridge *bridge)
>* This needs to be fixed in the drm_bridge framework and the API
>* needs to be updated to manage our own call chains...
>*/
> - dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);
> + if (dsi->panel_bridge->funcs->post_disable)
> + dsi->panel_bridge->funcs->post_disable(dsi->panel_bridge);
>
Why not drm_bridge_post_disable ?
Regards
Andrzej
> if (dsi->slave) {
> dw_mipi_dsi_disable(dsi->slave);
Re: [PATCH] drm/bridge: dw-hdmi: fix SCDC configuration for ddc-i2c-bus
On 21.04.2019 10:25, Jonas Karlman wrote:
> When ddc-i2c-bus property is used, a NULL pointer dereference is reported:
>
> [ 31.041669] Unable to handle kernel NULL pointer dereference at virtual
> address 0008
> [ 31.041671] pgd = 4d3c16f6
> [ 31.041673] [0008] *pgd=
> [ 31.041678] Internal error: Oops: 5 [#1] SMP ARM
>
> [ 31.041711] Hardware name: Rockchip (Device Tree)
> [ 31.041718] PC is at i2c_transfer+0x8/0xe4
> [ 31.041721] LR is at drm_scdc_read+0x54/0x84
> [ 31.041723] pc : []lr : []psr: 280f0013
> [ 31.041725] sp : edffdad0 ip : 5ccb5511 fp : 0058
> [ 31.041727] r10: 0780 r9 : edf91608 r8 : c11b0f48
> [ 31.041728] r7 : 0438 r6 : r5 : r4 :
> [ 31.041730] r3 : edffdae7 r2 : 0002 r1 : edffdaec r0 :
>
> [ 31.041908] [] (i2c_transfer) from []
> (drm_scdc_read+0x54/0x84)
> [ 31.041913] [] (drm_scdc_read) from []
> (drm_scdc_set_scrambling+0x30/0xbc)
> [ 31.041919] [] (drm_scdc_set_scrambling) from []
> (dw_hdmi_update_power+0x1440/0x1610)
> [ 31.041926] [] (dw_hdmi_update_power) from []
> (dw_hdmi_bridge_enable+0x2c/0x70)
> [ 31.041932] [] (dw_hdmi_bridge_enable) from []
> (drm_bridge_enable+0x24/0x34)
> [ 31.041938] [] (drm_bridge_enable) from []
> (drm_atomic_helper_commit_modeset_enables+0x114/0x220)
> [ 31.041943] [] (drm_atomic_helper_commit_modeset_enables) from
> [] (rockchip_atomic_helper_commit_tail_rpm+0x28/0x64)
>
> hdmi->i2c may not be set when ddc-i2c-bus property is used in device tree.
> Fix this by using hdmi->ddc as the i2c adapter when calling drm_scdc_*().
> Also report that SCDC is not supported when there is no DDC bus.
>
> Fixes: 264fce6cc2c1 ("drm/bridge: dw-hdmi: Add SCDC and TMDS Scrambling
> support")
> Signed-off-by: Jonas Karlman
Pushed to drm-misc-fixes.
Regards
Andrzej
