Date: Mon, 03 Jun 2019 08:52:41 -0600 From: Ian Lepore <ian@freebsd.org> To: Andriy Gapon <avg@FreeBSD.org>, Niclas Zeising <zeising@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Cc: jmd@freebsd.org Subject: Re: svn commit: r348355 - head/sys/dev/iicbus Message-ID: <75cec0b83709f48bbd52e2444d7af17569093f60.camel@freebsd.org> In-Reply-To: <cda9c032-f0ab-7bf5-9d6f-c6167b1cf9ff@FreeBSD.org> References: <201905290908.x4T98L89066643@repo.freebsd.org> <c3f1c60b-24b2-6098-501a-8cb81ef66d57@freebsd.org> <def030c0-80a5-84ca-bb48-7009aa34e69c@FreeBSD.org> <d1128088420c6e52721fb5df2280ca73096bf5c0.camel@freebsd.org> <ac9ae4b6-4b89-1e5f-9116-dcf20fee7e85@freebsd.org> <cda9c032-f0ab-7bf5-9d6f-c6167b1cf9ff@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 2019-06-03 at 15:08 +0300, Andriy Gapon wrote: > On 03/06/2019 14:16, Niclas Zeising wrote: > > Hi! > > It seems like things broke after all, latest pkg build (on head- > > amd64) reports > > this: > > > > > > /wrkdirs/usr/ports/graphics/drm-legacy-kmod/work/drm-legacy- > > 12bd551/src/dev/drm2/i915/intel_iic.c:570:2: > > error: implicit declaration of function 'iicbus_set_nostop' is > > invalid in C99 > > [-Werror,-Wimplicit-function-declaration] > > iicbus_set_nostop(idev, true); > > ^ > > /wrkdirs/usr/ports/graphics/drm-legacy-kmod/work/drm-legacy- > > 12bd551/src/dev/drm2/i915/intel_iic.c:570:2: > > error: this function declaration is not a prototype [-Werror,- > > Wstrict-prototypes] > > 2 errors generated. > > > > Full log: > > > > http://beefy12.nyi.freebsd.org/data/head-amd64-default/p503023_s348376/logs/drm-legacy-kmod-g20190523.log > > Hi! Thank you for the report. > I am going to restore iicbus_set_nostop, but this time as a function > that > modifies iicbus softc (instead of an ivar accessor for the bus). > I am including a patch that I would like to commit. > > However, for the drm code to request the nostop mode correctly it > needs to be > fixed as well. > My proposed patch is here: > https://github.com/FreeBSDDesktop/drm-legacy/pull/9 > > Index: sys/dev/iicbus/iicbus.h > =================================================================== > --- sys/dev/iicbus/iicbus.h (revision 348529) > +++ sys/dev/iicbus/iicbus.h (working copy) > @@ -46,6 +46,8 @@ struct iicbus_softc > * 0 if no start condition succeeded */ > u_char strict; /* deny operations that violate the > * I2C protocol */ > + bool nostop; /* iicbus_transfer defaults to > repeated > + * start between messages */ > struct mtx lock; > u_int bus_freq; /* Configured bus Hz. */ > }; > @@ -77,6 +79,7 @@ IICBUS_ACCESSOR(addr, ADDR, > uint32_t) > > int iicbus_generic_intr(device_t dev, int event, char *buf); > void iicbus_init_frequency(device_t dev, u_int bus_freq); > +void iicbus_set_nostop(device_t dev, bool val); > > extern driver_t iicbus_driver; > extern devclass_t iicbus_devclass; > Index: sys/dev/iicbus/iiconf.c > =================================================================== > --- sys/dev/iicbus/iiconf.c (revision 348529) > +++ sys/dev/iicbus/iiconf.c (working copy) > @@ -383,6 +383,14 @@ iicbus_block_read(device_t bus, u_char slave, > char > return (error); > } > > +void > +iicbus_set_nostop(device_t bus, bool val) > +{ > + struct iicbus_softc *sc = device_get_softc(bus); > + > + sc->nostop = val; > +} > + > /* > * iicbus_transfer() > * > @@ -427,7 +435,8 @@ iicbus_transfer_gen(device_t dev, struct iic_msg > * > { > int i, error, lenread, lenwrote, nkid, rpstart, addr; > device_t *children, bus; > - bool started; > + struct iicbus_softc *sc; > + bool nostop, started; > > if ((error = device_get_children(dev, &children, &nkid)) != 0) > return (IIC_ERESOURCE); > @@ -438,6 +447,8 @@ iicbus_transfer_gen(device_t dev, struct iic_msg > * > bus = children[0]; > rpstart = 0; > free(children, M_TEMP); > + sc = device_get_softc(bus); > + nostop = sc->nostop; > started = false; > for (i = 0, error = 0; i < nmsgs && error == 0; i++) { > addr = msgs[i].slave; > @@ -465,11 +476,12 @@ iicbus_transfer_gen(device_t dev, struct > iic_msg * > if (error != 0) > break; > > - if (!(msgs[i].flags & IIC_M_NOSTOP)) { > + if ((msgs[i].flags & IIC_M_NOSTOP) != 0 || > + (nostop && i + 1 < nmsgs)) { > + rpstart = 1; /* Next message gets repeated > start */ > + } else { > rpstart = 0; > iicbus_stop(bus); > - } else { > - rpstart = 1; /* Next message gets repeated > start */ > } > } > if (error != 0 && started) > > Please don't. We still have a situation where nobody has shown a runtime failure at all. This build failure could be fixed by simply defining a do-nothing iicbus_set_nostop() function if a quick fix is needed. Putting this nostop concept into code that is shared by many drivers is an abomination. We have exactly one driver that needs this functionality, so the right fix is to implement it wholly within that one driver. I'll put together a diff for that. -- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?75cec0b83709f48bbd52e2444d7af17569093f60.camel>