| Message ID | 20190605070507.11417-8-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tc358767 driver improvements | expand |
On 05.06.2019 09:04, Andrey Smirnov wrote: > Simplify AUX data write by dropping index arithmetic and shifting and > replacing it with a call to a helper function that does three things: > > 1. Copies user-provided data into a write buffer > 2. Optionally fixes the endianness of the write buffer (not needed > on LE hosts) > 3. Transfers contenst of the write buffer to up to 4 32-bit > registers on the chip > > Note that separate data endianness fix: > > tmp = (tmp << 8) | buf[i]; > > that was reserved for DP_AUX_I2C_WRITE looks really strange, since it > will place data differently depending on the passed user-data > size. E.g. for a write of 1 byte, data transferred to the chip would > look like: > > [byte0] [dummy1] [dummy2] [dummy3] > > whereas for a write of 4 bytes we'd get: > > [byte3] [byte2] [byte1] [byte0] > > Since there's no indication in the datasheet that I2C write buffer > should be treated differently than AUX write buffer and no comment in > the original code explaining why it was done this way, that special > I2C write buffer transformation was dropped in this patch. > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > Cc: Archit Taneja <architt@codeaurora.org> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> > Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com> > Cc: Philipp Zabel <p.zabel@pengutronix.de> > Cc: Cory Tusar <cory.tusar@zii.aero> > Cc: Chris Healy <cphealy@gmail.com> > Cc: Lucas Stach <l.stach@pengutronix.de> > Cc: dri-devel@lists.freedesktop.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/gpu/drm/bridge/tc358767.c | 59 +++++++++++++++++++------------ > 1 file changed, 37 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > index da47d81e7109..260fbcd0271e 100644 > --- a/drivers/gpu/drm/bridge/tc358767.c > +++ b/drivers/gpu/drm/bridge/tc358767.c > @@ -321,6 +321,32 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply) > return 0; > } > > +static int tc_aux_write_data(struct tc_data *tc, const void *data, > + size_t size) > +{ > + u32 auxwdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)] = { 0 }; > + int ret, i, count = DIV_ROUND_UP(size, sizeof(u32)); > + > + memcpy(auxwdata, data, size); > + > + for (i = 0; i < count; i++) { > + /* > + * Our regmap is configured as LE > + * for register data, so we need > + * undo any byte swapping that will > + * happened to preserve original > + * byte order. > + */ > + cpu_to_le32s(&auxwdata[i]); > + } > + > + ret = regmap_bulk_write(tc->regmap, DP0_AUXWDATA(0), auxwdata, count); > + if (ret) > + return ret; > + > + return size; > +} As prevoiusly maybe regmap_raw_write. Beside this: Reviewed-by: Andrzej Hajda <a.hajda@samsung.com> -- Regards Andrzej > + > static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size) > { > u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)]; > @@ -350,9 +376,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux, > struct tc_data *tc = aux_to_tc(aux); > size_t size = min_t(size_t, 8, msg->size); > u8 request = msg->request & ~DP_AUX_I2C_MOT; > - u8 *buf = msg->buffer; > - u32 tmp = 0; > - int i = 0; > int ret; > > if (size == 0) > @@ -362,25 +385,17 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux, > if (ret) > return ret; > > - if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) { > - /* Store data */ > - while (i < size) { > - if (request == DP_AUX_NATIVE_WRITE) > - tmp = tmp | (buf[i] << (8 * (i & 0x3))); > - else > - tmp = (tmp << 8) | buf[i]; > - i++; > - if (((i % 4) == 0) || (i == size)) { > - ret = regmap_write(tc->regmap, > - DP0_AUXWDATA((i - 1) >> 2), > - tmp); > - if (ret) > - return ret; > - tmp = 0; > - } > - } > - } else if (request != DP_AUX_I2C_READ && > - request != DP_AUX_NATIVE_READ) { > + switch (request) { > + case DP_AUX_NATIVE_READ: > + case DP_AUX_I2C_READ: > + break; > + case DP_AUX_NATIVE_WRITE: > + case DP_AUX_I2C_WRITE: > + ret = tc_aux_write_data(tc, msg->buffer, size); > + if (ret < 0) > + return ret; > + break; > + default: > return -EINVAL; > } >
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index da47d81e7109..260fbcd0271e 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -321,6 +321,32 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply) return 0; } +static int tc_aux_write_data(struct tc_data *tc, const void *data, + size_t size) +{ + u32 auxwdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)] = { 0 }; + int ret, i, count = DIV_ROUND_UP(size, sizeof(u32)); + + memcpy(auxwdata, data, size); + + for (i = 0; i < count; i++) { + /* + * Our regmap is configured as LE + * for register data, so we need + * undo any byte swapping that will + * happened to preserve original + * byte order. + */ + cpu_to_le32s(&auxwdata[i]); + } + + ret = regmap_bulk_write(tc->regmap, DP0_AUXWDATA(0), auxwdata, count); + if (ret) + return ret; + + return size; +} + static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size) { u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)]; @@ -350,9 +376,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux, struct tc_data *tc = aux_to_tc(aux); size_t size = min_t(size_t, 8, msg->size); u8 request = msg->request & ~DP_AUX_I2C_MOT; - u8 *buf = msg->buffer; - u32 tmp = 0; - int i = 0; int ret; if (size == 0) @@ -362,25 +385,17 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux, if (ret) return ret; - if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) { - /* Store data */ - while (i < size) { - if (request == DP_AUX_NATIVE_WRITE) - tmp = tmp | (buf[i] << (8 * (i & 0x3))); - else - tmp = (tmp << 8) | buf[i]; - i++; - if (((i % 4) == 0) || (i == size)) { - ret = regmap_write(tc->regmap, - DP0_AUXWDATA((i - 1) >> 2), - tmp); - if (ret) - return ret; - tmp = 0; - } - } - } else if (request != DP_AUX_I2C_READ && - request != DP_AUX_NATIVE_READ) { + switch (request) { + case DP_AUX_NATIVE_READ: + case DP_AUX_I2C_READ: + break; + case DP_AUX_NATIVE_WRITE: + case DP_AUX_I2C_WRITE: + ret = tc_aux_write_data(tc, msg->buffer, size); + if (ret < 0) + return ret; + break; + default: return -EINVAL; }
Simplify AUX data write by dropping index arithmetic and shifting and replacing it with a call to a helper function that does three things: 1. Copies user-provided data into a write buffer 2. Optionally fixes the endianness of the write buffer (not needed on LE hosts) 3. Transfers contenst of the write buffer to up to 4 32-bit registers on the chip Note that separate data endianness fix: tmp = (tmp << 8) | buf[i]; that was reserved for DP_AUX_I2C_WRITE looks really strange, since it will place data differently depending on the passed user-data size. E.g. for a write of 1 byte, data transferred to the chip would look like: [byte0] [dummy1] [dummy2] [dummy3] whereas for a write of 4 bytes we'd get: [byte3] [byte2] [byte1] [byte0] Since there's no indication in the datasheet that I2C write buffer should be treated differently than AUX write buffer and no comment in the original code explaining why it was done this way, that special I2C write buffer transformation was dropped in this patch. Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> Cc: Archit Taneja <architt@codeaurora.org> Cc: Andrzej Hajda <a.hajda@samsung.com> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com> Cc: Philipp Zabel <p.zabel@pengutronix.de> Cc: Cory Tusar <cory.tusar@zii.aero> Cc: Chris Healy <cphealy@gmail.com> Cc: Lucas Stach <l.stach@pengutronix.de> Cc: dri-devel@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org --- drivers/gpu/drm/bridge/tc358767.c | 59 +++++++++++++++++++------------ 1 file changed, 37 insertions(+), 22 deletions(-)