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