Re: [PATCH v1 2/2] This patch adds a new DRM bridge driver for the Lontium LT9611C chip.

[Krzysztof Kozlowski <krzk@kernel.org>]

On 04/09/2025 12:48, 杨孙运 wrote:
>>> +
>>> +static void lt9611c_cleanup_resources(struct lt9611c *lt9611c)
>>> +{
>>> +     struct device *dev = lt9611c->dev;
>>> +
>>> +     if (lt9611c->work_inited) {
>>> +             cancel_work_sync(&lt9611c->work);
>>> +             lt9611c->work_inited = false;
>>> +             dev_err(dev, "work cancelled\n");
>>
>> Why???
>>
> ?? I don't understand.

You need to explain why that line - printing error - should be here. And
focus on "WHY" part.

> 
>>> +     }
>>> +
>>> +     if (lt9611c->bridge_added) {
>>> +             drm_bridge_remove(&lt9611c->bridge);
>>> +             lt9611c->bridge_added = false;
>>> +             dev_err(dev, "DRM bridge removed\n");
>>> +     }
>>> +
>>> +     if (lt9611c->regulators_enabled) {
>>> +             regulator_bulk_disable(ARRAY_SIZE(lt9611c->supplies), lt9611c->supplies);
>>> +             lt9611c->regulators_enabled = false;
>>> +             dev_err(dev, "regulators disabled\n");
>>> +     }
>>> +
>>> +     if (lt9611c->audio_pdev)
>>> +             lt9611c_audio_exit(lt9611c);
>>> +
>>> +     if (lt9611c->fw) {
>>
>> You definitely don't need firmware when the bridge is up and running.
>>
> The previous text has already described the working logic of the firmware.
> 
>>> +             release_firmware(lt9611c->fw);
>>> +             lt9611c->fw = NULL;
>>> +             dev_err(dev, "firmware released\n");
>>> +     }
>>> +
>>> +     if (lt9611c->dsi0_node) {
>>> +             of_node_put(lt9611c->dsi0_node);
>>> +             lt9611c->dsi0_node = NULL;
>>> +             dev_err(dev, "dsi0 node released\n");

Your driver is way, way to noisy.

Please read coding style - what does it say about driver being silent?


Best regards,
Krzysztof