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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
