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

[Sean Paul <seanpaul@chromium.org>]

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 :-)

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.

Sean

> 
> Shawn
> 
> [1] https://patchwork.kernel.org/patch/9349195/
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel