Re: [PATCH v7 1/2] drm/bridge: Add sii902x driver

[Boris Brezillon <boris.brezillon@free-electrons.com>]

Hi Archit,

On Thu, 9 Jun 2016 12:15:00 +0530
Archit Taneja <architt@codeaurora.org> wrote:

> Hi Boris,
> 
> On 6/8/2016 6:00 PM, Boris Brezillon wrote:
> > Add basic support for the sii902x RGB -> HDMI bridge.
> > This driver does not support audio output yet.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Tested-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> > ---
> > Hi Archit,
> >
> > I recently learned you were the drm/bridge maintainer, and I realize
> > you were not in Cc of the previous versions.
> >
> > Once you have some time, could you review this series, and let me know
> > what's the procedure to get it merged (send a PR to Dave, or let you
> > take it into your tree).  
> 
> I'm still in the process of preparing my account, and figuring out
> how to not mess up sending pull requests, etc. If it's done fast
> enough, I'll prepare a tree and start collecting stuff. It it's getting
> too late, Daniel/Dave/Thierry can pull it for this merge window.

Ok, cool!

> 
> Some comments below.
> 
> >
> > Thanks,
> >
> > Boris
> >
> > Changes in v6:
> > - use HDMI_INFOFRAME_SIZE(AVI)
> > - fix reset_gpio initialization
> > - reduce the reset time based on Ming feedback
> >
> > Changes in v5:
> > - drop the best_encoder() implementation
> >
> > Changes in v4:
> > - make reset GPIO optional
> > - only support attaching to DRM devices supporting atomic updates
> >
> > Changes in v3:
> > - fix get_modes() implementation to avoid turning the screen in power
> >    save mode
> > - rename the driver (sil902x -> sii902x)
> >
> > Changes in v2:
> > - fix errors reported by the kbuild robot
> >
> > fixup! drm: bridge: Add sii902x driver
> > ---
> >   drivers/gpu/drm/bridge/Kconfig   |   8 +
> >   drivers/gpu/drm/bridge/Makefile  |   1 +
> >   drivers/gpu/drm/bridge/sii902x.c | 459 +++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 468 insertions(+)
> >   create mode 100644 drivers/gpu/drm/bridge/sii902x.c
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 8f7423f..a1419214 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -50,6 +50,14 @@ config DRM_PARADE_PS8622
> >   	---help---
> >   	  Parade eDP-LVDS bridge chip driver.
> >
> > +config DRM_SII902X
> > +	tristate "Silicon Image sii902x RGB/HDMI bridge"
> > +	depends on OF
> > +	select DRM_KMS_HELPER
> > +	select REGMAP_I2C
> > +	---help---
> > +	  Silicon Image sii902x bridge chip driver.
> > +
> >   source "drivers/gpu/drm/bridge/analogix/Kconfig"
> >
> >   endmenu
> > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> > index 96b13b3..bfec9f8 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -5,4 +5,5 @@ obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
> >   obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
> >   obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
> >   obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> > +obj-$(CONFIG_DRM_SII902X) += sii902x.o
> >   obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> > new file mode 100644
> > index 0000000..d46bf98
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/sii902x.c
> > @@ -0,0 +1,459 @@
> > +/*
> > + * Copyright (C) 2014 Atmel
> > + *		      Bo Shen <voice.shen@atmel.com>  
> 
> Maybe a newer copyright here?

Sure.

> 
> > + *
> > + * Authors:	      Bo Shen <voice.shen@atmel.com>
> > + *		      Boris Brezillon <boris.brezillon@free-electrons.com>
> > + *		      Wu, Songjun <Songjun.Wu@atmel.com>
> > + *
> > + *
> > + * Copyright (C) 2010-2011 Freescale Semiconductor, Inc. All Rights Reserved.
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * 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.
> > + */
> > +
> > +#include <linux/component.h>  
> 
> This doesn't seem to be needed.
> 
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_gpio.h>  
> 
> Same for the two above.
> 
> > +#include <linux/regmap.h>
> > +
> > +#include <drm/drmP.h>
> > +#include <drm/drm_atomic.h>  
> 
> drm_atomic.h isn't needed either.
> 
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_crtc_helper.h>
> > +#include <drm/drm_edid.h>
> > +#include <drm/drm_encoder_slave.h>  
> 
> This one above too.

Will remove all the unneeded inclusions.

> 
> > +
> > +#define SIL902X_TPI_VIDEO_DATA			0x0
> > +
> > +#define SIL902X_TPI_PIXEL_REPETITION		0x8
> > +#define SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT     BIT(5)
> > +#define SIL902X_TPI_AVI_PIXEL_REP_RISING_EDGE   BIT(4)
> > +#define SIL902X_TPI_AVI_PIXEL_REP_4X		3
> > +#define SIL902X_TPI_AVI_PIXEL_REP_2X		1
> > +#define SIL902X_TPI_AVI_PIXEL_REP_NONE		0
> > +#define SIL902X_TPI_CLK_RATIO_HALF		(0 << 6)
> > +#define SIL902X_TPI_CLK_RATIO_1X		(1 << 6)
> > +#define SIL902X_TPI_CLK_RATIO_2X		(2 << 6)
> > +#define SIL902X_TPI_CLK_RATIO_4X		(3 << 6)
> > +
> > +#define SIL902X_TPI_AVI_IN_FORMAT		0x9
> > +#define SIL902X_TPI_AVI_INPUT_BITMODE_12BIT	BIT(7)
> > +#define SIL902X_TPI_AVI_INPUT_DITHER		BIT(6)
> > +#define SIL902X_TPI_AVI_INPUT_RANGE_LIMITED	(2 << 2)
> > +#define SIL902X_TPI_AVI_INPUT_RANGE_FULL	(1 << 2)
> > +#define SIL902X_TPI_AVI_INPUT_RANGE_AUTO	(0 << 2)
> > +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_BLACK	(3 << 0)
> > +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_YUV422	(2 << 0)
> > +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_YUV444	(1 << 0)
> > +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB	(0 << 0)
> > +
> > +#define SIL902X_TPI_AVI_INFOFRAME		0x0c
> > +
> > +#define SIL902X_SYS_CTRL_DATA			0x1a
> > +#define SIL902X_SYS_CTRL_PWR_DWN		BIT(4)
> > +#define SIL902X_SYS_CTRL_AV_MUTE		BIT(3)
> > +#define SIL902X_SYS_CTRL_DDC_BUS_REQ		BIT(2)
> > +#define SIL902X_SYS_CTRL_DDC_BUS_GRTD		BIT(1)
> > +#define SIL902X_SYS_CTRL_OUTPUT_MODE		BIT(0)
> > +#define SIL902X_SYS_CTRL_OUTPUT_HDMI		1
> > +#define SIL902X_SYS_CTRL_OUTPUT_DVI		0
> > +
> > +#define SIL902X_REG_CHIPID(n)			(0x1b + (n))
> > +
> > +#define SIL902X_PWR_STATE_CTRL			0x1e
> > +#define SIL902X_AVI_POWER_STATE_MSK		GENMASK(1, 0)
> > +#define SIL902X_AVI_POWER_STATE_D(l)		((l) & SIL902X_AVI_POWER_STATE_MSK)
> > +
> > +#define SI902X_INT_ENABLE			0x3c
> > +#define SI902X_INT_STATUS			0x3d
> > +#define SI902X_HOTPLUG_EVENT			BIT(0)
> > +#define SI902X_PLUGGED_STATUS			BIT(2)
> > +
> > +#define SIL902X_REG_TPI_RQB			0xc7  
> 
> I suppose the registers could be renamed to SII902x_... too?

Yep, and I'll also do the following change: s/SIL/SII/

> 
> > +
> > +struct sii902x {
> > +	struct i2c_client *i2c;
> > +	struct regmap *regmap;
> > +	struct drm_bridge bridge;
> > +	struct drm_connector connector;
> > +	struct gpio_desc *reset_gpio;
> > +	struct work_struct hotplug_work;  
> 
> hotplug_work doesn't seem to be used in the driver.

Indeed.

> 
> > +};
> > +

[...]

> > +
> > +static const struct drm_connector_funcs sii902x_connector_funcs = {
> > +	.dpms = drm_atomic_helper_connector_dpms,
> > +	.detect = sii902x_connector_detect,
> > +	.fill_modes = drm_helper_probe_single_connector_modes,
> > +	.destroy = sii902x_connector_destroy,  
> 
> You can pass the drm_connector_cleanup helper directly instead of
> creating a destroy op.

Yes, I'll pass 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 int sii902x_get_modes(struct drm_connector *connector)
> > +{
> > +	struct sii902x *sii902x = connector_to_sii902x(connector);
> > +	struct regmap *regmap = sii902x->regmap;
> > +	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > +	unsigned int status;
> > +	struct edid *edid;
> > +	int num = 0;
> > +	int ret;
> > +
> > +	ret = regmap_update_bits(regmap, SIL902X_SYS_CTRL_DATA,
> > +				 SIL902X_SYS_CTRL_DDC_BUS_REQ,
> > +				 SIL902X_SYS_CTRL_DDC_BUS_REQ);
> > +	if (ret)
> > +		return ret;
> > +
> > +	do {
> > +		ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
> > +		if (ret)
> > +			return ret;
> > +	} while (!(status & SIL902X_SYS_CTRL_DDC_BUS_GRTD));  
> 
> Can this result into an infinite while loop? Maybe we should timeout
> after a few tries? You can ignore this comment if it's something
> that's highly unprobable.

No, I guess it's better to have a timeout.

> 
> > +
> > +	ret = regmap_write(regmap, SIL902X_SYS_CTRL_DATA, status);
> > +	if (ret)
> > +		return ret;
> > +
> > +	edid = drm_get_edid(connector, sii902x->i2c->adapter);
> > +	drm_mode_connector_update_edid_property(connector, edid);
> > +	if (edid) {
> > +		num = drm_add_edid_modes(connector, edid);
> > +		kfree(edid);
> > +	}  
> 
> Shouldn't we ideally call drm_mode_connector_update_edid_property once
> we know edid is non-NULL?

Hm, drm_mode_connector_update_edid_property() seems to test edid value,
and I guess you still want to drop the previous edid information even
if the new one is NULL.

> 
> > +
> > +	ret = drm_display_info_set_bus_formats(&connector->display_info,
> > +					       &bus_format, 1);
> > +	if (ret)
> > +		return ret;
> > +
> > +	regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_update_bits(regmap, SIL902X_SYS_CTRL_DATA,
> > +				 SIL902X_SYS_CTRL_DDC_BUS_REQ |
> > +				 SIL902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> > +	if (ret)
> > +		return ret;
> > +
> > +	do {
> > +		ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, &status);
> > +		if (ret)
> > +			return ret;
> > +	} while (status & (SIL902X_SYS_CTRL_DDC_BUS_REQ |
> > +			   SIL902X_SYS_CTRL_DDC_BUS_GRTD));  
> 
> Same thing about the infinite loop here.
> 
> > +
> > +	return num;
> > +}
> > +

[...]

> > +			 const struct i2c_device_id *id)
> > +{
> > +	struct device *dev = &client->dev;
> > +	unsigned int status = 0;
> > +	struct sii902x *sii902x;
> > +	u8 chipid[4];
> > +	int ret;
> > +
> > +	sii902x = devm_kzalloc(dev, sizeof(*sii902x), GFP_KERNEL);
> > +	if (!sii902x)
> > +		return -ENOMEM;
> > +
> > +	sii902x->i2c = client;
> > +	sii902x->regmap = devm_regmap_init_i2c(client, &sii902x_regmap_config);
> > +	if (IS_ERR(sii902x->regmap))
> > +		return PTR_ERR(sii902x->regmap);
> > +
> > +	sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > +						      GPIOD_OUT_LOW);
> > +	if (IS_ERR(sii902x->reset_gpio)) {
> > +		dev_err(dev, "Failed to retrieve/request reset gpio: %ld\n",
> > +			PTR_ERR(sii902x->reset_gpio));
> > +		return PTR_ERR(sii902x->reset_gpio);
> > +	}
> > +
> > +	sii902x_reset(sii902x);
> > +
> > +	ret = regmap_write(sii902x->regmap, SIL902X_REG_TPI_RQB, 0x0);
> > +	if (ret)
> > +		return ret;  
> 
> Is there a reason why we're checking for the return values of regmap
> funcs in a couple of the functions but not all? I've seen it in other
> drivers too, just wondering..

Well, I guess we should check all regmap accesses everywhere,
especially when the regmap is accessed over an external bus, but people
usually assume that regmap accesses will never fail :-/.

The places where I'm not checking it are those called from 'void func()'
functions, where checking the return code would be useless.

> 
> > +
> > +	ret = regmap_bulk_read(sii902x->regmap, SIL902X_REG_CHIPID(0),
> > +			       &chipid, 4);
> > +	if (ret) {
> > +		dev_err(dev, "regmap_read failed %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	if (chipid[0] != 0xb0) {
> > +		dev_err(dev, "Invalid chipid: %02x (expecting 0xb0)\n",
> > +			chipid[0]);
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Clear all pending interrupts */
> > +	regmap_read(sii902x->regmap, SI902X_INT_STATUS, &status);
> > +	regmap_write(sii902x->regmap, SI902X_INT_STATUS, status);
> > +
> > +	if (client->irq > 0) {
> > +		regmap_write(sii902x->regmap, SI902X_INT_ENABLE,
> > +			     SI902X_HOTPLUG_EVENT);
> > +
> > +		ret = devm_request_threaded_irq(dev, client->irq, NULL,
> > +						sii902x_interrupt,
> > +						IRQF_ONESHOT, dev_name(dev),
> > +						sii902x);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	sii902x->bridge.funcs = &sii902x_bridge_funcs;
> > +	sii902x->bridge.of_node = dev->of_node;
> > +	ret = drm_bridge_add(&sii902x->bridge);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to add drm_bridge\n");
> > +		return ret;
> > +	}
> > +
> > +	i2c_set_clientdata(client, sii902x);
> > +
> > +	return 0;
> > +}
> > +
> > +static int sii902x_remove(struct i2c_client *client)
> > +
> > +{
> > +	struct sii902x *sii902x = i2c_get_clientdata(client);
> > +
> > +	drm_bridge_remove(&sii902x->bridge);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id sii902x_dt_ids[] = {
> > +	{ .compatible = "sil,sii9022", },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(of, sii902x_dt_ids);
> > +
> > +static const struct i2c_device_id sii902x_i2c_ids[] = {
> > +	{ "sii9022", 0 },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, sii902x_i2c_ids);
> > +
> > +static struct i2c_driver sii902x_driver = {
> > +	.probe = sii902x_probe,
> > +	.remove = sii902x_remove,
> > +	.driver = {
> > +		.name = "sii902x",
> > +		.of_match_table = sii902x_dt_ids,
> > +	},
> > +	.id_table = sii902x_i2c_ids,
> > +};
> > +module_i2c_driver(sii902x_driver);
> > +
> > +MODULE_AUTHOR("Boris Brezillon <boris.brezillon@free-electrons.com>");
> > +MODULE_DESCRIPTION("SIL902x RGB -> HDMI bridges");  
> 
> SII902x here.
> 
> Looks good apart from the minor comments.

Thanks for the review.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel