linux-kernel - Re: [PATCH v2 2/2] drm/bridge: add support for lontium lt8713sx bridge driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aVDZ3GBUJW6zs3y6@hu-vishsain-blr.qualcomm.com>
Date: Sun, 28 Dec 2025 12:48:52 +0530
From: Vishnu Saini <vishnu.saini@....qualcomm.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Robert Foss <rfoss@...nel.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>, Tony <syyang@...tium.com>,
        dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, prahlad.valluru@....qualcomm.com,
        Prahlad Valluru <vvalluru@....qualcomm.com>
Subject: Re: [PATCH v2 2/2] drm/bridge: add support for lontium lt8713sx
 bridge driver

On Tue, Nov 18, 2025 at 08:37:22AM +0100, Krzysztof Kozlowski wrote:
> On 18/11/2025 05:37, Vishnu Saini wrote:
> > +static void lt8713sx_reset(struct lt8713sx *lt8713sx)
> > +{
> > +	pr_debug("reset bridge.\n");
> > +	gpiod_set_value_cansleep(lt8713sx->reset_gpio, 1);
> > +	msleep(20);
> > +
> > +	gpiod_set_value_cansleep(lt8713sx->reset_gpio, 0);
> > +	msleep(20);
> > +
> > +	gpiod_set_value_cansleep(lt8713sx->reset_gpio, 1);
> > +	msleep(20);
> > +	pr_debug("reset done.\n");
> 
> No, it is not done, because you kept the device in the reset. 1 is
> reset. Don't mix up line and logical signals.
done. 
> > +}
> > +
> > +static int lt8713sx_regulator_init(struct lt8713sx *lt8713sx)
> > +{
> > +	int ret;
> > +
> > +	lt8713sx->supplies[0].supply = "vdd";
> > +	lt8713sx->supplies[1].supply = "vcc";
> > +
> > +	ret = devm_regulator_bulk_get(lt8713sx->dev, 2, lt8713sx->supplies);
> > +	if (ret < 0)
> > +		return dev_err_probe(lt8713sx->dev, ret, "failed to get regulators\n");
> > +
> > +	ret = regulator_set_load(lt8713sx->supplies[0].consumer, 200000);
> > +	if (ret < 0)
> > +		return dev_err_probe(lt8713sx->dev, ret, "failed to set regulator load\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static int lt8713sx_regulator_enable(struct lt8713sx *lt8713sx)
> > +{
> > +	int ret;
> > +
> > +	ret = regulator_enable(lt8713sx->supplies[0].consumer);
> > +	if (ret < 0)
> > +		return dev_err_probe(lt8713sx->dev, ret, "failed to enable vdd regulator\n");
> > +
> > +	usleep_range(1000, 10000);
> > +
> > +	ret = regulator_enable(lt8713sx->supplies[1].consumer);
> > +	if (ret < 0) {
> > +		regulator_disable(lt8713sx->supplies[0].consumer);
> > +		return dev_err_probe(lt8713sx->dev, ret, "failed to enable vcc regulator\n");
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int lt8713sx_gpio_init(struct lt8713sx *lt8713sx)
> > +{
> > +	struct device *dev = lt8713sx->dev;
> > +
> > +	lt8713sx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(lt8713sx->reset_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(lt8713sx->reset_gpio),
> > +				     "failed to acquire reset gpio\n");
> > +
> > +	/* power enable gpio */
> > +	lt8713sx->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> > +	if (IS_ERR(lt8713sx->enable_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(lt8713sx->enable_gpio),
> > +				     "failed to acquire enable gpio\n");
> > +	return 0;
> > +}
> > +
> > +static ssize_t lt8713sx_firmware_store(struct device *dev,
> > +				       struct device_attribute *attr,
> > +				       const char *buf, size_t len)
> > +{
> > +	struct lt8713sx *lt8713sx = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	ret = lt8713sx_firmware_update(lt8713sx);
> > +	if (ret < 0)
> > +		return ret;
> > +	return len;
> > +}
> > +
> > +static DEVICE_ATTR_WO(lt8713sx_firmware);
> > +
> > +static struct attribute *lt8713sx_attrs[] = {
> > +	&dev_attr_lt8713sx_firmware.attr,
> > +	NULL,
> > +};
> > +
> > +static const struct attribute_group lt8713sx_attr_group = {
> > +	.attrs = lt8713sx_attrs,
> > +};
> > +
> > +static const struct attribute_group *lt8713sx_attr_groups[] = {
> > +	&lt8713sx_attr_group,
> > +	NULL,
> > +};
> > +
> > +static int lt8713sx_probe(struct i2c_client *client)
> > +{
> > +	struct lt8713sx *lt8713sx;
> > +	struct device *dev = &client->dev;
> > +	int ret;
> > +
> > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > +		return dev_err_probe(dev, -ENODEV, "device doesn't support I2C\n");
> > +
> > +	lt8713sx = devm_kzalloc(dev, sizeof(*lt8713sx), GFP_KERNEL);
> > +	if (!lt8713sx)
> > +		return dev_err_probe(dev, -ENOMEM, "failed to allocate lt8713sx struct\n");
> > +
> I did not ask for dev_err_probe here. Do you see such pattern anywhere?
> No, because there are never error messages on memory allocation (see
> coccinelle). Drop.
Dropped this dev_err_probe.
 
> Please run standard kernel tools for static analysis, like coccinelle,
> smatch and sparse, and fix reported warnings. Also please check for
> warnings when building with W=1 for gcc and clang. Most of these
> commands (checks or W=1 build) can build specific targets, like some
> directory, to narrow the scope to only your code. The code here looks
> like it needs a fix. Feel free to get in touch if the warning is not clear.

Run a couple of tools you mentioned and fixed the warnings in next patch.

> Best regards,
> Krzysztof