Re: [PATCH v4 02/10] drm/bridge: add common api for inno hdmi

[Krzysztof Kozlowski <krzk@kernel.org>]

On 21/05/2024 12:58, keith wrote:
> Add INNO common api so that it can be used by vendor
> drivers which implement vendor specific extensions to Innosilicon HDMI.
> 
> Signed-off-by: keith <keith.zhao@starfivetech.com>
> ---
>  MAINTAINERS                                   |   2 +

> +}
> +
> +static u32 inno_hdmi_i2c_func(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_algorithm inno_hdmi_algorithm = {
> +	.master_xfer	= inno_hdmi_i2c_xfer,
> +	.functionality	= inno_hdmi_i2c_func,
> +};
> +
> +static struct i2c_adapter *inno_hdmi_i2c_adapter(struct inno_hdmi *hdmi)
> +{
> +	struct i2c_adapter *adap;
> +	struct inno_hdmi_i2c *i2c;
> +	int ret;
> +
> +	i2c = devm_kzalloc(hdmi->dev, sizeof(*i2c), GFP_KERNEL);
> +	if (!i2c)
> +		return ERR_PTR(-ENOMEM);
> +
> +	mutex_init(&i2c->lock);
> +	init_completion(&i2c->cmp);
> +
> +	adap = &i2c->adap;
> +	adap->owner = THIS_MODULE;
> +	adap->dev.parent = hdmi->dev;
> +	adap->dev.of_node = hdmi->dev->of_node;
> +	adap->algo = &inno_hdmi_algorithm;
> +	strscpy(adap->name, "Inno HDMI", sizeof(adap->name));
> +	i2c_set_adapdata(adap, hdmi);
> +
> +	ret = i2c_add_adapter(adap);
> +	if (ret) {
> +		dev_warn(hdmi->dev, "cannot add %s I2C adapter\n", adap->name);
> +		devm_kfree(hdmi->dev, i2c);

Hm, why freeing? That's a devm. You don't free other pieces in bind paths...

> +		return ERR_PTR(ret);
> +	}
> +
> +	hdmi->i2c = i2c;
> +
> +	DRM_DEV_INFO(hdmi->dev, "registered %s I2C bus driver\n", adap->name);

Please do not print simple success messages. Tracing tells you this and
driver should be quiet in typical cases.

You have also some weird tabs instead of spaces here and there... like
before 'encoder' arguments. This is needs some basic style check.


Best regards,
Krzysztof