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

[Daniel Vetter]

On Wed, Apr 05, 2017 at 02:08:52PM +0800, Shawn Guo wrote:
> 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."

Hm indeed, I guess I'm just bad at this review stuff :-)

> > 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.

Yeah, we can go with this one for now, but long-term might indeed be good
to switch both of them over to drm_do_get_edid, since that should fit
better. Sorry for all the confusion.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch