Re: [PATCH 4/4] drm: zte: add VGA driver support

[Shawn Guo <shawnguo@kernel.org>]

On Tue, Apr 04, 2017 at 01:32:39PM -0400, Sean Paul wrote:
> On Tue, Apr 04, 2017 at 09:47:38PM +0800, Shawn Guo wrote:
> > On Mon, Apr 03, 2017 at 11:23:51AM +0200, Daniel Vetter wrote:
> > > > +static int zx_vga_ddc_register(struct zx_vga *vga)
> > > > +{
> > > > +	struct device *dev = vga->dev;
> > > > +	struct i2c_adapter *adap;
> > > > +	struct zx_vga_i2c *ddc;
> > > > +	int ret;
> > > > +
> > > > +	ddc = devm_kzalloc(dev, sizeof(*ddc), GFP_KERNEL);
> > > > +	if (!ddc)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	vga->ddc = ddc;
> > > > +	mutex_init(&ddc->lock);
> > > > +
> > > > +	adap = &ddc->adap;
> > > > +	adap->owner = THIS_MODULE;
> > > > +	adap->class = I2C_CLASS_DDC;
> > > > +	adap->dev.parent = dev;
> > > > +	adap->algo = &zx_vga_algorithm;
> > > > +	snprintf(adap->name, sizeof(adap->name), "zx vga i2c");
> > > > +
> > > > +	ret = i2c_add_adapter(adap);
> > > > +	if (ret) {
> > > > +		DRM_DEV_ERROR(dev, "failed to add I2C adapter: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	i2c_set_adapdata(adap, vga);
> > > 
> > > Since this really isn't a full-featured i2c adapter but a specialized
> > > "give me the edid" thing I think you should look into drm_do_get_edid and
> > > _not_ expose the i2c thing. That should simplify your code a lot, and
> > > avoid any kind of synchronization issues between userspace i2c access and
> > > your driver's access (which you atm don't handle at all).
> > 
> > I'm a bit confused here.  Could you give me some more details on how to
> > know it's a full-featured I2C bus or specialized "give me the edid" one?
> > My understanding is that the ZTE hardware for reading HDMI and VGA EDID
> > are different but pretty similar.  But in HDMI driver review [1], you
> > gave the opposite comment, asking to change to I2C adapter from
> > drm_do_get_edid().  I'm wondering how you see that the hardware in HDMI
> > is a full-featured I2C bus, while the one in VGA is a specialized bus?
> 
> That was my suggestion :-)

It was Daniel's too.  He commented as below under function
zx_hdmi_get_edid_block().

"It looks like the ZX_DDC register range implements a hw i2c engine (since
you specifiy port and offsets and everything). Please implement it as an
i2c_adapter driver and use the normal drm_get_edid function."

Shawn

> 
> At the time, it seemed like the hardware implemented a fully fledged i2c bus, so
> I suggested you made it generic. In retrospect, since it can only read from one
> address, the original implementation was fine.
> 
> Given the HDMI adapter is implemented the same way, I'm inclined to favor this
> version, but Daniel might disagree with me.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel