| Message ID | 20221214125821.12489-1-paul@crapouillou.net |
|---|---|
| Headers | show |
| Series | drm: bridge: it66121: IT6610 support and cleanups | expand |
On Wed, 14 Dec 2022 at 13:58, Paul Cercueil <paul@crapouillou.net> wrote: > > Simplify the code of the driver by using > devm_regulator_bulk_get_enable(), which will handle powering up the > regulators, and disabling them on probe error or module removal. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/bridge/ite-it66121.c | 34 +++++++--------------------- > 1 file changed, 8 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > index 7476cfbf9585..a698eec8f250 100644 > --- a/drivers/gpu/drm/bridge/ite-it66121.c > +++ b/drivers/gpu/drm/bridge/ite-it66121.c > @@ -301,7 +301,6 @@ struct it66121_ctx { > struct device *dev; > struct gpio_desc *gpio_reset; > struct i2c_client *client; > - struct regulator_bulk_data supplies[3]; > u32 bus_width; > struct mutex lock; /* Protects fields below and device registers */ > struct hdmi_avi_infoframe hdmi_avi_infoframe; > @@ -342,16 +341,6 @@ static void it66121_hw_reset(struct it66121_ctx *ctx) > gpiod_set_value(ctx->gpio_reset, 0); > } > > -static inline int ite66121_power_on(struct it66121_ctx *ctx) > -{ > - return regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies); > -} > - > -static inline int ite66121_power_off(struct it66121_ctx *ctx) > -{ > - return regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies); > -} > - > static inline int it66121_preamble_ddc(struct it66121_ctx *ctx) > { > return regmap_write(ctx->regmap, IT66121_MASTER_SEL_REG, IT66121_MASTER_SEL_HOST); > @@ -1512,6 +1501,10 @@ static int it66121_audio_codec_init(struct it66121_ctx *ctx, struct device *dev) > return PTR_ERR_OR_ZERO(ctx->audio.pdev); > } > > +static const char * const it66121_supplies[] = { > + "vcn33", "vcn18", "vrf12" > +}; > + > static int it66121_probe(struct i2c_client *client) > { > u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; > @@ -1564,26 +1557,18 @@ static int it66121_probe(struct i2c_client *client) > i2c_set_clientdata(client, ctx); > mutex_init(&ctx->lock); > > - ctx->supplies[0].supply = "vcn33"; > - ctx->supplies[1].supply = "vcn18"; > - ctx->supplies[2].supply = "vrf12"; > - ret = devm_regulator_bulk_get(ctx->dev, 3, ctx->supplies); > + ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(it66121_supplies), > + it66121_supplies); > if (ret) { > - dev_err(ctx->dev, "regulator_bulk failed\n"); > + dev_err(dev, "Failed to enable power supplies\n"); > return ret; > } > > - ret = ite66121_power_on(ctx); > - if (ret) > - return ret; > - > it66121_hw_reset(ctx); > > ctx->regmap = devm_regmap_init_i2c(client, &it66121_regmap_config); > - if (IS_ERR(ctx->regmap)) { > - ite66121_power_off(ctx); > + if (IS_ERR(ctx->regmap)) > return PTR_ERR(ctx->regmap); > - } > > regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]); > regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]); > @@ -1596,7 +1581,6 @@ static int it66121_probe(struct i2c_client *client) > > if (vendor_ids[0] != IT66121_VENDOR_ID0 || vendor_ids[1] != IT66121_VENDOR_ID1 || > device_ids[0] != IT66121_DEVICE_ID0 || device_ids[1] != IT66121_DEVICE_ID1) { > - ite66121_power_off(ctx); > return -ENODEV; > } > > @@ -1609,7 +1593,6 @@ static int it66121_probe(struct i2c_client *client) > IRQF_ONESHOT, dev_name(dev), ctx); > if (ret < 0) { > dev_err(dev, "Failed to request irq %d:%d\n", client->irq, ret); > - ite66121_power_off(ctx); > return ret; > } > > @@ -1626,7 +1609,6 @@ static void it66121_remove(struct i2c_client *client) > { > struct it66121_ctx *ctx = i2c_get_clientdata(client); > > - ite66121_power_off(ctx); > drm_bridge_remove(&ctx->bridge); > mutex_destroy(&ctx->lock); > } > -- > 2.35.1 > Reviewed-by: Robert Foss <robert.foss@linaro.org>
On Wed, 14 Dec 2022 at 13:58, Paul Cercueil <paul@crapouillou.net> wrote: > > Use regmap_noinc_read() instead of reading the data from the DDC FIFO one > byte at a time. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/bridge/ite-it66121.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > index a698eec8f250..12222840df30 100644 > --- a/drivers/gpu/drm/bridge/ite-it66121.c > +++ b/drivers/gpu/drm/bridge/ite-it66121.c > @@ -589,13 +589,12 @@ static int it66121_get_edid_block(void *context, u8 *buf, > if (ret) > return ret; > > - do { > - ret = regmap_read(ctx->regmap, IT66121_DDC_RD_FIFO_REG, &val); > - if (ret) > - return ret; > - *(buf++) = val; > - cnt--; > - } while (cnt > 0); > + ret = regmap_noinc_read(ctx->regmap, IT66121_DDC_RD_FIFO_REG, > + buf, cnt); > + if (ret) > + return ret; > + > + buf += cnt; > } > > return 0; > -- > 2.35.1 > Reviewed-by: Robert Foss <robert.foss@linaro.org>
On Wed, 14 Dec 2022 at 13:58, Paul Cercueil <paul@crapouillou.net> wrote: > > Since all AVI infoframe registers are contiguous in the address space, > the AVI infoframe can be written in one go with regmap_bulk_write(). > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/bridge/ite-it66121.c | 27 +++++++-------------------- > 1 file changed, 7 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > index 12222840df30..0a4fdfd7af44 100644 > --- a/drivers/gpu/drm/bridge/ite-it66121.c > +++ b/drivers/gpu/drm/bridge/ite-it66121.c > @@ -773,24 +773,9 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge, > const struct drm_display_mode *mode, > const struct drm_display_mode *adjusted_mode) > { > - int ret, i; > u8 buf[HDMI_INFOFRAME_SIZE(AVI)]; > struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge); > - const u16 aviinfo_reg[HDMI_AVI_INFOFRAME_SIZE] = { > - IT66121_AVIINFO_DB1_REG, > - IT66121_AVIINFO_DB2_REG, > - IT66121_AVIINFO_DB3_REG, > - IT66121_AVIINFO_DB4_REG, > - IT66121_AVIINFO_DB5_REG, > - IT66121_AVIINFO_DB6_REG, > - IT66121_AVIINFO_DB7_REG, > - IT66121_AVIINFO_DB8_REG, > - IT66121_AVIINFO_DB9_REG, > - IT66121_AVIINFO_DB10_REG, > - IT66121_AVIINFO_DB11_REG, > - IT66121_AVIINFO_DB12_REG, > - IT66121_AVIINFO_DB13_REG > - }; > + int ret; > > mutex_lock(&ctx->lock); > > @@ -810,10 +795,12 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge, > } > > /* Write new AVI infoframe packet */ > - for (i = 0; i < HDMI_AVI_INFOFRAME_SIZE; i++) { > - if (regmap_write(ctx->regmap, aviinfo_reg[i], buf[i + HDMI_INFOFRAME_HEADER_SIZE])) > - goto unlock; > - } > + ret = regmap_bulk_write(ctx->regmap, IT66121_AVIINFO_DB1_REG, > + &buf[HDMI_INFOFRAME_HEADER_SIZE], > + HDMI_AVI_INFOFRAME_SIZE); > + if (ret) > + goto unlock; > + > if (regmap_write(ctx->regmap, IT66121_AVIINFO_CSUM_REG, buf[3])) > goto unlock; > > -- > 2.35.1 > Reviewed-by: Robert Foss <robert.foss@linaro.org>
On Wed, 14 Dec 2022 at 13:59, Paul Cercueil <paul@crapouillou.net> wrote: > > The function it66121_wait_ddc_ready() would previously read the status > register until "true", which means it never actually polled anything and > would just read the register once. > > Now, it will properly wait until the DDC hardware is ready or until it > reported an error. > > The 'busy' variable was also renamed to 'error' since these bits are set > on error and not when the DDC hardware is busy. > > Since the DDC ready function is now working properly, the msleep(20) can > be removed. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/bridge/ite-it66121.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > index 0a4fdfd7af44..bfb9c87a7019 100644 > --- a/drivers/gpu/drm/bridge/ite-it66121.c > +++ b/drivers/gpu/drm/bridge/ite-it66121.c > @@ -440,15 +440,17 @@ static int it66121_configure_afe(struct it66121_ctx *ctx, > static inline int it66121_wait_ddc_ready(struct it66121_ctx *ctx) > { > int ret, val; > - u32 busy = IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS | > - IT66121_DDC_STATUS_ARBI_LOSE; > + u32 error = IT66121_DDC_STATUS_NOACK | IT66121_DDC_STATUS_WAIT_BUS | > + IT66121_DDC_STATUS_ARBI_LOSE; > + u32 done = IT66121_DDC_STATUS_TX_DONE; > > - ret = regmap_read_poll_timeout(ctx->regmap, IT66121_DDC_STATUS_REG, val, true, > - IT66121_EDID_SLEEP_US, IT66121_EDID_TIMEOUT_US); > + ret = regmap_read_poll_timeout(ctx->regmap, IT66121_DDC_STATUS_REG, val, > + val & (error | done), IT66121_EDID_SLEEP_US, > + IT66121_EDID_TIMEOUT_US); > if (ret) > return ret; > > - if (val & busy) > + if (val & error) > return -EAGAIN; > > return 0; > @@ -582,9 +584,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, > offset += cnt; > remain -= cnt; > > - /* Per programming manual, sleep here before emptying the FIFO */ > - msleep(20); > - > ret = it66121_wait_ddc_ready(ctx); > if (ret) > return ret; > -- > 2.35.1 > Reviewed-by: Robert Foss <robert.foss@linaro.org>
On Wed, 14 Dec 2022 at 13:59, Paul Cercueil <paul@crapouillou.net> wrote: > > The DDC error IRQs will fire on the IT6610 every time the FIFO is empty, > which is not very helpful. To resolve this, we can simply disable them, > and handle DDC errors in it66121_wait_ddc_ready(). > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/bridge/ite-it66121.c | 49 ++++++---------------------- > 1 file changed, 10 insertions(+), 39 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > index bfb9c87a7019..06fa59ae5ffc 100644 > --- a/drivers/gpu/drm/bridge/ite-it66121.c > +++ b/drivers/gpu/drm/bridge/ite-it66121.c > @@ -515,16 +515,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, > offset = (block % 2) * len; > block = block / 2; > > - ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, &val); > - if (ret) > - return ret; > - > - if (val & IT66121_INT_STATUS1_DDC_BUSHANG) { > - ret = it66121_abort_ddc_ops(ctx); > - if (ret) > - return ret; > - } > - > ret = it66121_clear_ddc_fifo(ctx); > if (ret) > return ret; > @@ -545,16 +535,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, > if (ret) > return ret; > > - ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, &val); > - if (ret) > - return ret; > - > - if (val & IT66121_INT_STATUS1_DDC_BUSHANG) { > - ret = it66121_abort_ddc_ops(ctx); > - if (ret) > - return ret; > - } > - > ret = it66121_preamble_ddc(ctx); > if (ret) > return ret; > @@ -585,8 +565,10 @@ static int it66121_get_edid_block(void *context, u8 *buf, > remain -= cnt; > > ret = it66121_wait_ddc_ready(ctx); > - if (ret) > + if (ret) { > + it66121_abort_ddc_ops(ctx); > return ret; > + } > > ret = regmap_noinc_read(ctx->regmap, IT66121_DDC_RD_FIFO_REG, > buf, cnt); > @@ -671,11 +653,7 @@ static int it66121_bridge_attach(struct drm_bridge *bridge, > /* Per programming manual, sleep here for bridge to settle */ > msleep(50); > > - /* Start interrupts */ > - return regmap_write_bits(ctx->regmap, IT66121_INT_MASK1_REG, > - IT66121_INT_MASK1_DDC_NOACK | > - IT66121_INT_MASK1_DDC_FIFOERR | > - IT66121_INT_MASK1_DDC_BUSHANG, 0); > + return 0; > } > > static int it66121_set_mute(struct it66121_ctx *ctx, bool mute) > @@ -926,21 +904,14 @@ static irqreturn_t it66121_irq_threaded_handler(int irq, void *dev_id) > ret = regmap_read(ctx->regmap, IT66121_INT_STATUS1_REG, &val); > if (ret) { > dev_err(dev, "Cannot read STATUS1_REG %d\n", ret); > - } else { > - if (val & IT66121_INT_STATUS1_DDC_FIFOERR) > - it66121_clear_ddc_fifo(ctx); > - if (val & (IT66121_INT_STATUS1_DDC_BUSHANG | > - IT66121_INT_STATUS1_DDC_NOACK)) > - it66121_abort_ddc_ops(ctx); > - if (val & IT66121_INT_STATUS1_HPD_STATUS) { > - regmap_write_bits(ctx->regmap, IT66121_INT_CLR1_REG, > - IT66121_INT_CLR1_HPD, IT66121_INT_CLR1_HPD); > + } else if (val & IT66121_INT_STATUS1_HPD_STATUS) { > + regmap_write_bits(ctx->regmap, IT66121_INT_CLR1_REG, > + IT66121_INT_CLR1_HPD, IT66121_INT_CLR1_HPD); > > - status = it66121_is_hpd_detect(ctx) ? connector_status_connected > - : connector_status_disconnected; > + status = it66121_is_hpd_detect(ctx) ? connector_status_connected > + : connector_status_disconnected; > > - event = true; > - } > + event = true; > } > > regmap_write_bits(ctx->regmap, IT66121_SYS_STATUS_REG, > -- > 2.35.1 > Reviewed-by: Robert Foss <robert.foss@linaro.org>
On Wed, 14 Dec 2022 at 13:59, Paul Cercueil <paul@crapouillou.net> wrote: > > The DDC FIFO was cleared before the loop in it66121_get_edid_block(), > and at the beginning of each iteration; which means that it did not have > to be cleared before the loop. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/bridge/ite-it66121.c | 16 ---------------- > 1 file changed, 16 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > index 06fa59ae5ffc..5335d4abd7c5 100644 > --- a/drivers/gpu/drm/bridge/ite-it66121.c > +++ b/drivers/gpu/drm/bridge/ite-it66121.c > @@ -456,18 +456,6 @@ static inline int it66121_wait_ddc_ready(struct it66121_ctx *ctx) > return 0; > } > > -static int it66121_clear_ddc_fifo(struct it66121_ctx *ctx) > -{ > - int ret; > - > - ret = it66121_preamble_ddc(ctx); > - if (ret) > - return ret; > - > - return regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG, > - IT66121_DDC_COMMAND_FIFO_CLR); > -} > - > static int it66121_abort_ddc_ops(struct it66121_ctx *ctx) > { > int ret; > @@ -515,10 +503,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, > offset = (block % 2) * len; > block = block / 2; > > - ret = it66121_clear_ddc_fifo(ctx); > - if (ret) > - return ret; > - > while (remain > 0) { > cnt = (remain > IT66121_EDID_FIFO_SIZE) ? > IT66121_EDID_FIFO_SIZE : remain; > -- > 2.35.1 > Reviewed-by: Robert Foss <robert.foss@linaro.org>
On Wed, 14 Dec 2022 at 13:59, Paul Cercueil <paul@crapouillou.net> wrote: > > The DDC preamble and target address only need to be set once before > reading the EDID, even if multiple segments have to be read. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/bridge/ite-it66121.c | 28 ++++++++++++++++------------ > 1 file changed, 16 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > index 5335d4abd7c5..7972003d4776 100644 > --- a/drivers/gpu/drm/bridge/ite-it66121.c > +++ b/drivers/gpu/drm/bridge/ite-it66121.c > @@ -506,9 +506,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, > while (remain > 0) { > cnt = (remain > IT66121_EDID_FIFO_SIZE) ? > IT66121_EDID_FIFO_SIZE : remain; > - ret = it66121_preamble_ddc(ctx); > - if (ret) > - return ret; > > ret = regmap_write(ctx->regmap, IT66121_DDC_COMMAND_REG, > IT66121_DDC_COMMAND_FIFO_CLR); > @@ -519,15 +516,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, > if (ret) > return ret; > > - ret = it66121_preamble_ddc(ctx); > - if (ret) > - return ret; > - > - ret = regmap_write(ctx->regmap, IT66121_DDC_HEADER_REG, > - IT66121_DDC_HEADER_EDID); > - if (ret) > - return ret; > - > ret = regmap_write(ctx->regmap, IT66121_DDC_OFFSET_REG, offset); > if (ret) > return ret; > @@ -842,9 +830,25 @@ static struct edid *it66121_bridge_get_edid(struct drm_bridge *bridge, > { > struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge); > struct edid *edid; > + int ret; > > mutex_lock(&ctx->lock); > + ret = it66121_preamble_ddc(ctx); > + if (ret) { > + edid = ERR_PTR(ret); > + goto out_unlock; > + } > + > + ret = regmap_write(ctx->regmap, IT66121_DDC_HEADER_REG, > + IT66121_DDC_HEADER_EDID); > + if (ret) { > + edid = ERR_PTR(ret); > + goto out_unlock; > + } > + > edid = drm_do_get_edid(connector, it66121_get_edid_block, ctx); > + > +out_unlock: > mutex_unlock(&ctx->lock); > > return edid; > -- > 2.35.1 > Reviewed-by: Robert Foss <robert.foss@linaro.org>
On Wed, 14 Dec 2022 at 14:01, Paul Cercueil <paul@crapouillou.net> wrote: > > This will make it easier later to introduce support for new chips in > this driver. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/bridge/ite-it66121.c | 27 +++++++++++++++------------ > 1 file changed, 15 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > index 7972003d4776..43b027b85b8e 100644 > --- a/drivers/gpu/drm/bridge/ite-it66121.c > +++ b/drivers/gpu/drm/bridge/ite-it66121.c > @@ -35,10 +35,6 @@ > #define IT66121_DEVICE_ID0_REG 0x02 > #define IT66121_DEVICE_ID1_REG 0x03 > > -#define IT66121_VENDOR_ID0 0x54 > -#define IT66121_VENDOR_ID1 0x49 > -#define IT66121_DEVICE_ID0 0x12 > -#define IT66121_DEVICE_ID1 0x06 > #define IT66121_REVISION_MASK GENMASK(7, 4) > #define IT66121_DEVICE_ID1_MASK GENMASK(3, 0) > > @@ -286,13 +282,12 @@ > #define IT66121_AUD_SWL_16BIT 0x2 > #define IT66121_AUD_SWL_NOT_INDICATED 0x0 > > -#define IT66121_VENDOR_ID0 0x54 > -#define IT66121_VENDOR_ID1 0x49 > -#define IT66121_DEVICE_ID0 0x12 > -#define IT66121_DEVICE_ID1 0x06 > -#define IT66121_DEVICE_MASK 0x0F > #define IT66121_AFE_CLK_HIGH 80000 /* Khz */ > > +struct it66121_chip_info { > + u16 vid, pid; > +}; > + > struct it66121_ctx { > struct regmap *regmap; > struct drm_bridge bridge; > @@ -311,6 +306,7 @@ struct it66121_ctx { > u8 swl; > bool auto_cts; > } audio; > + const struct it66121_chip_info *info; > }; > > static const struct regmap_range_cfg it66121_regmap_banks[] = { > @@ -1451,6 +1447,7 @@ static const char * const it66121_supplies[] = { > > static int it66121_probe(struct i2c_client *client) > { > + const struct i2c_device_id *id = i2c_client_get_device_id(client); > u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 }; > struct device_node *ep; > int ret; > @@ -1472,6 +1469,7 @@ static int it66121_probe(struct i2c_client *client) > > ctx->dev = dev; > ctx->client = client; > + ctx->info = (const struct it66121_chip_info *) id->driver_data; > > of_property_read_u32(ep, "bus-width", &ctx->bus_width); > of_node_put(ep); > @@ -1523,8 +1521,8 @@ static int it66121_probe(struct i2c_client *client) > revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]); > device_ids[1] &= IT66121_DEVICE_ID1_MASK; > > - if (vendor_ids[0] != IT66121_VENDOR_ID0 || vendor_ids[1] != IT66121_VENDOR_ID1 || > - device_ids[0] != IT66121_DEVICE_ID0 || device_ids[1] != IT66121_DEVICE_ID1) { > + if ((vendor_ids[1] << 8 | vendor_ids[0]) != ctx->info->vid || > + (device_ids[1] << 8 | device_ids[0]) != ctx->info->pid) { > return -ENODEV; > } > > @@ -1563,8 +1561,13 @@ static const struct of_device_id it66121_dt_match[] = { > }; > MODULE_DEVICE_TABLE(of, it66121_dt_match); > > +static const struct it66121_chip_info it66121_chip_info = { > + .vid = 0x4954, > + .pid = 0x0612, > +}; > + > static const struct i2c_device_id it66121_id[] = { > - { "it66121", 0 }, > + { "it66121", (kernel_ulong_t) &it66121_chip_info }, > { } > }; > MODULE_DEVICE_TABLE(i2c, it66121_id); > -- > 2.35.1 > Reviewed-by: Robert Foss <robert.foss@linaro.org>
On Wed, 14 Dec 2022 at 14:01, Paul Cercueil <paul@crapouillou.net> wrote: > > Add support for the IT6610 HDMI encoder. > > The hardware is very similar, and therefore the driver did not require > too many changes. Some bits are only available on the IT66121, and > vice-versa. Also, the IT6610 requires specific polarities on the DE and > pixel lines. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > drivers/gpu/drm/bridge/ite-it66121.c | 108 +++++++++++++++++++++------ > 1 file changed, 86 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c > index 43b027b85b8e..b34860871627 100644 > --- a/drivers/gpu/drm/bridge/ite-it66121.c > +++ b/drivers/gpu/drm/bridge/ite-it66121.c > @@ -68,6 +68,7 @@ > #define IT66121_AFE_XP_ENO BIT(4) > #define IT66121_AFE_XP_RESETB BIT(3) > #define IT66121_AFE_XP_PWDI BIT(2) > +#define IT6610_AFE_XP_BYPASS BIT(0) > > #define IT66121_AFE_IP_REG 0x64 > #define IT66121_AFE_IP_GAINBIT BIT(7) > @@ -284,7 +285,13 @@ > > #define IT66121_AFE_CLK_HIGH 80000 /* Khz */ > > +enum chip_id { > + ID_IT6610, > + ID_IT66121, > +}; > + > struct it66121_chip_info { > + enum chip_id id; > u16 vid, pid; > }; > > @@ -391,16 +398,22 @@ static int it66121_configure_afe(struct it66121_ctx *ctx, > > ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG, > IT66121_AFE_IP_GAINBIT | > - IT66121_AFE_IP_ER0 | > - IT66121_AFE_IP_EC1, > + IT66121_AFE_IP_ER0, > IT66121_AFE_IP_GAINBIT); > if (ret) > return ret; > > - ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_EC1_REG, > - IT66121_AFE_XP_EC1_LOWCLK, 0x80); > - if (ret) > - return ret; > + if (ctx->info->id == ID_IT66121) { > + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG, > + IT66121_AFE_IP_EC1, 0); > + if (ret) > + return ret; > + > + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_EC1_REG, > + IT66121_AFE_XP_EC1_LOWCLK, 0x80); > + if (ret) > + return ret; > + } > } else { > ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG, > IT66121_AFE_XP_GAINBIT | > @@ -411,17 +424,24 @@ static int it66121_configure_afe(struct it66121_ctx *ctx, > > ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG, > IT66121_AFE_IP_GAINBIT | > - IT66121_AFE_IP_ER0 | > - IT66121_AFE_IP_EC1, IT66121_AFE_IP_ER0 | > - IT66121_AFE_IP_EC1); > + IT66121_AFE_IP_ER0, > + IT66121_AFE_IP_ER0); > if (ret) > return ret; > > - ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_EC1_REG, > - IT66121_AFE_XP_EC1_LOWCLK, > - IT66121_AFE_XP_EC1_LOWCLK); > - if (ret) > - return ret; > + if (ctx->info->id == ID_IT66121) { > + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_IP_REG, > + IT66121_AFE_IP_EC1, > + IT66121_AFE_IP_EC1); > + if (ret) > + return ret; > + > + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_EC1_REG, > + IT66121_AFE_XP_EC1_LOWCLK, > + IT66121_AFE_XP_EC1_LOWCLK); > + if (ret) > + return ret; > + } > } > > /* Clear reset flags */ > @@ -430,6 +450,14 @@ static int it66121_configure_afe(struct it66121_ctx *ctx, > if (ret) > return ret; > > + if (ctx->info->id == ID_IT6610) { > + ret = regmap_write_bits(ctx->regmap, IT66121_AFE_XP_REG, > + IT6610_AFE_XP_BYPASS, > + IT6610_AFE_XP_BYPASS); > + if (ret) > + return ret; > + } > + > return it66121_fire_afe(ctx); > } > > @@ -491,7 +519,6 @@ static int it66121_get_edid_block(void *context, u8 *buf, > unsigned int block, size_t len) > { > struct it66121_ctx *ctx = context; > - unsigned int val; > int remain = len; > int offset = 0; > int ret, cnt; > @@ -572,10 +599,12 @@ static int it66121_bridge_attach(struct drm_bridge *bridge, > if (ret) > return ret; > > - ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG, > - IT66121_CLK_BANK_PWROFF_RCLK, 0); > - if (ret) > - return ret; > + if (ctx->info->id == ID_IT66121) { > + ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG, > + IT66121_CLK_BANK_PWROFF_RCLK, 0); > + if (ret) > + return ret; > + } > > ret = regmap_write_bits(ctx->regmap, IT66121_INT_REG, > IT66121_INT_TX_CLK_OFF, 0); > @@ -713,6 +742,24 @@ static void it66121_bridge_disable(struct drm_bridge *bridge, > ctx->connector = NULL; > } > > +static int it66121_bridge_check(struct drm_bridge *bridge, > + struct drm_bridge_state *bridge_state, > + struct drm_crtc_state *crtc_state, > + struct drm_connector_state *conn_state) > +{ > + struct it66121_ctx *ctx = container_of(bridge, struct it66121_ctx, bridge); > + > + if (ctx->info->id == ID_IT6610) { > + /* The IT6610 only supports these settings */ > + bridge_state->input_bus_cfg.flags |= DRM_BUS_FLAG_DE_HIGH | > + DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE; > + bridge_state->input_bus_cfg.flags &= > + ~DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE; > + } > + > + return 0; > +} > + > static > void it66121_bridge_mode_set(struct drm_bridge *bridge, > const struct drm_display_mode *mode, > @@ -758,9 +805,12 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge, > if (regmap_write(ctx->regmap, IT66121_HDMI_MODE_REG, IT66121_HDMI_MODE_HDMI)) > goto unlock; > > - if (regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG, > - IT66121_CLK_BANK_PWROFF_TXCLK, IT66121_CLK_BANK_PWROFF_TXCLK)) > + if (ctx->info->id == ID_IT66121 && > + regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG, > + IT66121_CLK_BANK_PWROFF_TXCLK, > + IT66121_CLK_BANK_PWROFF_TXCLK)) { > goto unlock; > + } > > if (it66121_configure_input(ctx)) > goto unlock; > @@ -768,7 +818,11 @@ void it66121_bridge_mode_set(struct drm_bridge *bridge, > if (it66121_configure_afe(ctx, adjusted_mode)) > goto unlock; > > - regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG, IT66121_CLK_BANK_PWROFF_TXCLK, 0); > + if (ctx->info->id == ID_IT66121 && > + regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG, > + IT66121_CLK_BANK_PWROFF_TXCLK, 0)) { > + goto unlock; > + } > > unlock: > mutex_unlock(&ctx->lock); > @@ -859,6 +913,7 @@ static const struct drm_bridge_funcs it66121_bridge_funcs = { > .atomic_get_input_bus_fmts = it66121_bridge_atomic_get_input_bus_fmts, > .atomic_enable = it66121_bridge_enable, > .atomic_disable = it66121_bridge_disable, > + .atomic_check = it66121_bridge_check, > .mode_set = it66121_bridge_mode_set, > .mode_valid = it66121_bridge_mode_valid, > .detect = it66121_bridge_detect, > @@ -1557,17 +1612,26 @@ static void it66121_remove(struct i2c_client *client) > > static const struct of_device_id it66121_dt_match[] = { > { .compatible = "ite,it66121" }, > + { .compatible = "ite,it6610" }, > { } > }; > MODULE_DEVICE_TABLE(of, it66121_dt_match); > > static const struct it66121_chip_info it66121_chip_info = { > + .id = ID_IT66121, > .vid = 0x4954, > .pid = 0x0612, > }; > > +static const struct it66121_chip_info it6610_chip_info = { > + .id = ID_IT6610, > + .vid = 0xca00, > + .pid = 0x0611, > +}; > + > static const struct i2c_device_id it66121_id[] = { > { "it66121", (kernel_ulong_t) &it66121_chip_info }, > + { "it6610", (kernel_ulong_t) &it6610_chip_info }, > { } > }; > MODULE_DEVICE_TABLE(i2c, it66121_id); > -- > 2.35.1 > Reviewed-by: Robert Foss <robert.foss@linaro.org>
Hi, This patchset adds a few cleanups to the IT66121 HDMI chip driver, and most importantly adds support for the IT6610 chip. The driver was tested with both chips, but as I only own a HDMI monitor without speakers, HDMI audio may not be working on the IT6610. Cheers, -Paul Paul Cercueil (10): dt-bindings: display: bridge: it66121: Add compatible string for IT6610 drm: bridge: it66121: Use devm_regulator_bulk_get_enable() drm: bridge: it66121: Use regmap_noinc_read() drm: bridge: it66121: Write AVI infoframe with regmap_bulk_write() drm: bridge: it66121: Fix wait for DDC ready drm: bridge: it66121: Don't use DDC error IRQs drm: bridge: it66121: Don't clear DDC FIFO twice drm: bridge: it66121: Set DDC preamble only once before reading EDID drm: bridge: it66121: Move VID/PID to new it66121_chip_info structure drm: bridge: it66121: Add support for the IT6610 .../bindings/display/bridge/ite,it66121.yaml | 4 +- drivers/gpu/drm/bridge/ite-it66121.c | 315 +++++++++--------- 2 files changed, 157 insertions(+), 162 deletions(-)