RE: [RFC] drm/bridge/sii902x: Fix EDID readback

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Linus,

> Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback
>
> Hi Fabrizio,
>
> thanks for your patch!

Thank you for your feedback!

>
> On Wed, Oct 31, 2018 at 1:58 PM Fabrizio Castro
> <fabrizio.castro@xxxxxxxxxxxxxx> wrote:
>
> > While adding SiI9022A support to the iwg23s board it came up
> > that when the HDMI transmitter is in pass through mode the
> > device is not compliant with the I2C specification anymore,
> > as it requires a far bigger tbuf due to a delay the HDMI
> > transmitter is adding when relaying the STOP condition on the
> > monitor i2c side of things. When not providing an appropriate
> > delay after the STOP condition the i2c bus would get stuck.
> > Also, any other traffic on the bus while talking to the monitor
> > may cause the transaction to fail or even cause issues with the
> > i2c bus as well.
> >
> > I2c-gates seemed to reach consent as a possible way to address
> > these issues, and as such this patch is implementing a solution
> > based on that. Since others are clearly relying on the current
> > implementation of the driver, this patch won't require any DT
> > changes.
> >
> > Since we don't want any interference during the DDC Bus
> > Request/Grant procedure and while talking to the monitor, we have
> > to use the adapter locking primitives rather than the i2c-mux
> > locking primitives, but in order to achieve that I had to get
> > rid of regmap.
>
> Why did you have to remove regmap? It is perfectly possible
> to override any reading/writing operations locally if you don't
> want to use the regmap i2c variants.
>
> The code in your patch does not make it evident to me exactly
> where the problem is with regmap, also I bet the regmap
> maintainer would love to hear about it so we can attempt to
> fix it there instead of locally in this driver.
>
> AFAICT there is some locked vs unlocked business and I
> strongly hope there is some way to simply pass that down
> into whatever functions regmap end up calling so we don't
> have to change all call sites.

Yeah, there is some issue with locking here, and that's coming down
from the fact that when we call into drm_get_edid, we implicitly call
i2c_transfer which ultimately locks the i2c adapter, and then calls
into our sii902x_i2c_bypass_select, which in turn calls into the regmap
functions and therefore we try and lock the same i2c adapter again,
driving the system into a deadlock.
Having the option of using "unlocked" flavours of reads and writes
is what we need here, but looking at drivers/base/regmap/regmap-i2c.c
I couldn't find anything suitable for my case, maybe Mark could advise
on this one? I am sure I overlooked something here, is there a better
way to address this?

>
> (So please include Mark Brown on CC in future iterations.)
>
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@xxxxxxxxxxxxxx>
> > ---
> > Dear All,
> >
> > this is a follow up to:
> > https://www.mail-archive.com/linux-renesas-soc@xxxxxxxxxxxxxxx/msg32072.html
> >
> > Comments very welcome!
>
> At the very least I think the patch needs to be split in two,
> one dealing with the locking business and one dealing with
> the buffer size. As it looks now it is very hard to review.

The change with the buffer size comes down from sii902x_bulk_write implementation,
which btw is the only function that doesn't resembles its regmap equivalent, in terms
of parameters.

>
> >
> > Thanks,
> > Fab
> >
> >  drivers/gpu/drm/bridge/sii902x.c | 404 ++++++++++++++++++++++++++-------------
> >  1 file changed, 274 insertions(+), 130 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> > index e59a135..137a05a 100644
> > --- a/drivers/gpu/drm/bridge/sii902x.c
> > +++ b/drivers/gpu/drm/bridge/sii902x.c
> > @@ -21,9 +21,9 @@
> >   */
> >
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/i2c-mux.h>
> >  #include <linux/i2c.h>
> >  #include <linux/module.h>
> > -#include <linux/regmap.h>
> >
> >  #include <drm/drmP.h>
> >  #include <drm/drm_atomic_helper.h>
> > @@ -82,12 +82,130 @@
> >
> >  struct sii902x {
> >         struct i2c_client *i2c;
> > -       struct regmap *regmap;
> >         struct drm_bridge bridge;
> >         struct drm_connector connector;
> >         struct gpio_desc *reset_gpio;
> > +       struct i2c_mux_core *i2cmux;
> >  };
> >
> > +static int sii902x_transfer_read(struct i2c_client *i2c, u8 reg, u8 *val,
> > +                                u16 len, bool locked)
> > +{
> > +       struct i2c_msg xfer[] = {
> > +               {
> > +                       .addr = i2c->addr,
> > +                       .flags = 0,
> > +                       .len = 1,
> > +                       .buf = &reg,
> > +               }, {
> > +                       .addr = i2c->addr,
> > +                       .flags = I2C_M_RD,
> > +                       .len = len,
> > +                       .buf = val,
> > +               }
> > +       };
> > +       unsigned char xfers = ARRAY_SIZE(xfer);
> > +       int ret, retries = 5;
> > +
> > +       do {
> > +               if (locked)
> > +                       ret = i2c_transfer(i2c->adapter, xfer, xfers);
> > +               else
> > +                       ret = __i2c_transfer(i2c->adapter, xfer, xfers);
> > +               if (ret < 0)
> > +                       return ret;
> > +       } while (ret != xfers && --retries);
> > +       return ret == xfers ? 0 : -1;
> > +}
> > +
> > +static int sii902x_bulk_read(struct i2c_client *i2c, u8 reg, u8 *val, u16 len)
> > +{
> > +       return sii902x_transfer_read(i2c, reg, val, len, true);
> > +}
> > +
> > +static int __sii902x_bulk_read(struct i2c_client *i2c, u8 reg, u8 *val, u16 len)
> > +{
> > +       return sii902x_transfer_read(i2c, reg, val, len, false);
> > +}
>
> I'm not a fan of __functions because it is syntactically
> ambigous what that means.
>
> Example set_bit() vs __set_bit() - is it obvious from context
> that the latter is non-atomic? No.
>
> Give these functions names that indicate exactly what they
> do and why, please.
>
> *_locked and *_unlocked variants?

I agree, will do that

>
> > +static int sii902x_read(struct i2c_client *i2c, u8 reg, u8 *val)
> > +{
> > +       return sii902x_bulk_read(i2c, reg, val, 1);
> > +}
> > +
> > +static int __sii902x_read(struct i2c_client *i2c, u8 reg, u8 *val)
> > +{
> > +       return __sii902x_bulk_read(i2c, reg, val, 1);
> > +}
>
> Dito
>
> > +static int sii902x_transfer_write(struct i2c_client *i2c, u8 *val,
> > +                                u16 len, bool locked)
> > +{
> > +       struct i2c_msg xfer = {
> > +               .addr = i2c->addr,
> > +               .flags = 0,
> > +               .len = len,
> > +               .buf = val,
> > +       };
> > +       int ret, retries = 5;
> > +
> > +       do {
> > +               if (locked)
> > +                       ret = i2c_transfer(i2c->adapter, &xfer, 1);
> > +               else
> > +                       ret = __i2c_transfer(i2c->adapter, &xfer, 1);
>
> This locked variable seems to be the magic dust so
> please document it thorughly.

Will do

>
> > +               if (ret < 0)
> > +                       return ret;
> > +       } while (!ret && --retries);
> > +       return !ret ? -1 : 0;
> > +}
> > +
> > +static int sii902x_bulk_write(struct i2c_client *i2c, u8 *val, u16 len)
> > +{
> > +       return sii902x_transfer_write(i2c, val, len, true);
> > +}
> > +
> > +static int sii902x_write(struct i2c_client *i2c, u8 reg, u8 val)
> > +{
> > +       u8 data[2] = {reg, val};
> > +
> > +       return sii902x_transfer_write(i2c, data, 2, true);
> > +}
> > +
> > +static int __sii902x_write(struct i2c_client *i2c, u8 reg, u8 val)
> > +{
> > +       u8 data[2] = {reg, val};
> > +
> > +       return sii902x_transfer_write(i2c, data, 2, false);
> > +}
> > +
> > +static int sii902x_update_bits(struct i2c_client *i2c, u8 reg, u8 mask, u8 val)
> > +{
> > +       int ret;
> > +       u8 status;
> > +
> > +       ret = sii902x_read(i2c, reg, &status);
> > +       if (ret)
> > +               return ret;
> > +       status &= ~mask;
> > +       status |= val & mask;
> > +       return sii902x_write(i2c, reg, status);
> > +}
> > +
> > +static int __sii902x_update_bits(struct i2c_client *i2c, u8 reg, u8 mask,
> > +                                u8 val)
> > +{
> > +       int ret;
> > +       u8 status;
> > +
> > +       ret = __sii902x_read(i2c, reg, &status);
> > +       if (ret)
> > +               return ret;
> > +       status &= ~mask;
> > +       status |= val & mask;
> > +       return __sii902x_write(i2c, reg, status);
> > +}
>
> Dito.
>
> So now all read, write and rmw functions are duplicated.
>
> >  static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> >  {
> >         return container_of(bridge, struct sii902x, bridge);
> > @@ -115,9 +233,9 @@ static enum drm_connector_status
> >  sii902x_connector_detect(struct drm_connector *connector, bool force)
> >  {
> >         struct sii902x *sii902x = connector_to_sii902x(connector);
> > -       unsigned int status;
> > +       u8 status;
> >
> > -       regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status);
> > +       sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status);
> >
> >         return (status & SII902X_PLUGGED_STATUS) ?
> >                connector_status_connected : connector_status_disconnected;
> > @@ -135,41 +253,11 @@ static const struct drm_connector_funcs sii902x_connector_funcs = {
> >  static int sii902x_get_modes(struct drm_connector *connector)
> >  {
> >         struct sii902x *sii902x = connector_to_sii902x(connector);
> > -       struct regmap *regmap = sii902x->regmap;
> >         u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > -       struct device *dev = &sii902x->i2c->dev;
> > -       unsigned long timeout;
> > -       unsigned int retries;
> > -       unsigned int status;
> >         struct edid *edid;
> > -       int num = 0;
> > -       int ret;
> > -
> > -       ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> > -                                SII902X_SYS_CTRL_DDC_BUS_REQ,
> > -                                SII902X_SYS_CTRL_DDC_BUS_REQ);
> > -       if (ret)
> > -               return ret;
> > +       int num = 0, ret;
> >
> > -       timeout = jiffies +
> > -                 msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> > -       do {
> > -               ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> > -               if (ret)
> > -                       return ret;
> > -       } while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> > -                time_before(jiffies, timeout));
> > -
> > -       if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> > -               dev_err(dev, "failed to acquire the i2c bus\n");
> > -               return -ETIMEDOUT;
> > -       }
> > -
> > -       ret = regmap_write(regmap, SII902X_SYS_CTRL_DATA, status);
> > -       if (ret)
> > -               return ret;
> > -
> > -       edid = drm_get_edid(connector, sii902x->i2c->adapter);
> > +       edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
> >         drm_connector_update_edid_property(connector, edid);
> >         if (edid) {
> >                 num = drm_add_edid_modes(connector, edid);
> > @@ -181,42 +269,6 @@ static int sii902x_get_modes(struct drm_connector *connector)
> >         if (ret)
> >                 return ret;
> >
> > -       /*
> > -        * Sometimes the I2C bus can stall after failure to use the
> > -        * EDID channel. Retry a few times to see if things clear
> > -        * up, else continue anyway.
> > -        */
> > -       retries = 5;
> > -       do {
> > -               ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA,
> > -                                 &status);
> > -               retries--;
> > -       } while (ret && retries);
> > -       if (ret)
> > -               dev_err(dev, "failed to read status (%d)\n", ret);
> > -
> > -       ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> > -                                SII902X_SYS_CTRL_DDC_BUS_REQ |
> > -                                SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> > -       if (ret)
> > -               return ret;
> > -
> > -       timeout = jiffies +
> > -                 msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> > -       do {
> > -               ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> > -               if (ret)
> > -                       return ret;
> > -       } while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> > -                          SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> > -                time_before(jiffies, timeout));
> > -
> > -       if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> > -                     SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> > -               dev_err(dev, "failed to release the i2c bus\n");
> > -               return -ETIMEDOUT;
> > -       }
> > -
> >         return num;
> >  }
> >
> > @@ -237,20 +289,20 @@ static void sii902x_bridge_disable(struct drm_bridge *bridge)
> >  {
> >         struct sii902x *sii902x = bridge_to_sii902x(bridge);
> >
> > -       regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
> > -                          SII902X_SYS_CTRL_PWR_DWN,
> > -                          SII902X_SYS_CTRL_PWR_DWN);
> > +       sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +                           SII902X_SYS_CTRL_PWR_DWN,
> > +                           SII902X_SYS_CTRL_PWR_DWN);
> >  }
> >
> >  static void sii902x_bridge_enable(struct drm_bridge *bridge)
> >  {
> >         struct sii902x *sii902x = bridge_to_sii902x(bridge);
> >
> > -       regmap_update_bits(sii902x->regmap, SII902X_PWR_STATE_CTRL,
> > -                          SII902X_AVI_POWER_STATE_MSK,
> > -                          SII902X_AVI_POWER_STATE_D(0));
> > -       regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA,
> > -                          SII902X_SYS_CTRL_PWR_DWN, 0);
> > +       sii902x_update_bits(sii902x->i2c, SII902X_PWR_STATE_CTRL,
> > +                           SII902X_AVI_POWER_STATE_MSK,
> > +                           SII902X_AVI_POWER_STATE_D(0));
> > +       sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +                           SII902X_SYS_CTRL_PWR_DWN, 0);
> >  }
> >
> >  static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
> > @@ -258,25 +310,25 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
> >                                     struct drm_display_mode *adj)
> >  {
> >         struct sii902x *sii902x = bridge_to_sii902x(bridge);
> > -       struct regmap *regmap = sii902x->regmap;
> > -       u8 buf[HDMI_INFOFRAME_SIZE(AVI)];
> > +       u8 buf[HDMI_INFOFRAME_SIZE(AVI) + 1];
>
> Maybe this stuff should be a separate change?

This change is due to the fact that sii902x_bulk_write takes an array of u8, with the first element
being the register address, this makes sii902x_bulk_write not exactly the same as regmap_bulk_write.
Mark may point out something better regmap related, so maybe there is no even need for this.

Again, thank you very much for your comments, I am not going to send a v2 out straight away, I would
like to hear at least from Wolfram and Peter about the way I have implement the i2c-gate as it is a bit
unconventional, and then it would be great to have Mark's opinion on the locking bit.

Fab

>
> >         struct hdmi_avi_infoframe frame;
> >         int ret;
> >
> > -       buf[0] = adj->clock;
> > -       buf[1] = adj->clock >> 8;
> > -       buf[2] = adj->vrefresh;
> > -       buf[3] = 0x00;
> > -       buf[4] = adj->hdisplay;
> > -       buf[5] = adj->hdisplay >> 8;
> > -       buf[6] = adj->vdisplay;
> > -       buf[7] = adj->vdisplay >> 8;
> > -       buf[8] = SII902X_TPI_CLK_RATIO_1X | SII902X_TPI_AVI_PIXEL_REP_NONE |
> > +       buf[0] = SII902X_TPI_VIDEO_DATA;
> > +       buf[1] = adj->clock;
> > +       buf[2] = adj->clock >> 8;
> > +       buf[3] = adj->vrefresh;
> > +       buf[4] = 0x00;
> > +       buf[5] = adj->hdisplay;
> > +       buf[6] = adj->hdisplay >> 8;
> > +       buf[7] = adj->vdisplay;
> > +       buf[8] = adj->vdisplay >> 8;
> > +       buf[9] = SII902X_TPI_CLK_RATIO_1X | SII902X_TPI_AVI_PIXEL_REP_NONE |
> >                  SII902X_TPI_AVI_PIXEL_REP_BUS_24BIT;
> > -       buf[9] = SII902X_TPI_AVI_INPUT_RANGE_AUTO |
> > -                SII902X_TPI_AVI_INPUT_COLORSPACE_RGB;
> > +       buf[10] = SII902X_TPI_AVI_INPUT_RANGE_AUTO |
> > +                 SII902X_TPI_AVI_INPUT_COLORSPACE_RGB;
> >
> > -       ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10);
> > +       ret = sii902x_bulk_write(sii902x->i2c, buf, 11);
> >         if (ret)
> >                 return;
> >
> > @@ -286,16 +338,17 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
> >                 return;
> >         }
> >
> > -       ret = hdmi_avi_infoframe_pack(&frame, buf, sizeof(buf));
> > +       ret = hdmi_avi_infoframe_pack(&frame, buf + 1, sizeof(buf) - 1);
> >         if (ret < 0) {
> >                 DRM_ERROR("failed to pack AVI infoframe: %d\n", ret);
> >                 return;
> >         }
> >
> > +       buf[0] = SII902X_TPI_AVI_INFOFRAME;
> >         /* Do not send the infoframe header, but keep the CRC field. */
> > -       regmap_bulk_write(regmap, SII902X_TPI_AVI_INFOFRAME,
> > -                         buf + HDMI_INFOFRAME_HEADER_SIZE - 1,
> > -                         HDMI_AVI_INFOFRAME_SIZE + 1);
> > +       sii902x_bulk_write(sii902x->i2c,
> > +                          buf + HDMI_INFOFRAME_HEADER_SIZE,
> > +                          HDMI_AVI_INFOFRAME_SIZE + 1);
> >  }
> >
> >  static int sii902x_bridge_attach(struct drm_bridge *bridge)
> > @@ -336,29 +389,13 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = {
> >         .enable = sii902x_bridge_enable,
> >  };
> >
> > -static const struct regmap_range sii902x_volatile_ranges[] = {
> > -       { .range_min = 0, .range_max = 0xff },
> > -};
> > -
> > -static const struct regmap_access_table sii902x_volatile_table = {
> > -       .yes_ranges = sii902x_volatile_ranges,
> > -       .n_yes_ranges = ARRAY_SIZE(sii902x_volatile_ranges),
> > -};
> > -
> > -static const struct regmap_config sii902x_regmap_config = {
> > -       .reg_bits = 8,
> > -       .val_bits = 8,
> > -       .volatile_table = &sii902x_volatile_table,
> > -       .cache_type = REGCACHE_NONE,
> > -};
> > -
> >  static irqreturn_t sii902x_interrupt(int irq, void *data)
> >  {
> >         struct sii902x *sii902x = data;
> > -       unsigned int status = 0;
> > +       u8 status = 0;
> >
> > -       regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status);
> > -       regmap_write(sii902x->regmap, SII902X_INT_STATUS, status);
> > +       sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status);
> > +       sii902x_write(sii902x->i2c, SII902X_INT_STATUS, status);
> >
> >         if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev)
> >                 drm_helper_hpd_irq_event(sii902x->bridge.dev);
> > @@ -366,23 +403,111 @@ static irqreturn_t sii902x_interrupt(int irq, void *data)
> >         return IRQ_HANDLED;
> >  }
> >
> > +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
> > +{
> > +       struct sii902x *sii902x = i2c_mux_priv(mux);
> > +       struct device *dev = &sii902x->i2c->dev;
> > +       unsigned long timeout;
> > +       u8 status;
> > +       int ret;
> > +
> > +       ret = __sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +                                   SII902X_SYS_CTRL_DDC_BUS_REQ,
> > +                                   SII902X_SYS_CTRL_DDC_BUS_REQ);
> > +
> > +       timeout = jiffies +
> > +                 msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> > +       do {
> > +               ret = __sii902x_read(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +                                    &status);
> > +               if (ret)
> > +                       return ret;
> > +       } while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> > +                time_before(jiffies, timeout));
> > +
> > +       if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> > +               dev_err(dev, "Failed to acquire the i2c bus\n");
> > +               return -ETIMEDOUT;
> > +       }
> > +
> > +       ret = __sii902x_write(sii902x->i2c, SII902X_SYS_CTRL_DATA, status);
> > +       if (ret)
> > +               return ret;
> > +       return 0;
> > +}
> > +
> > +static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
> > +{
> > +       struct sii902x *sii902x = i2c_mux_priv(mux);
> > +       struct device *dev = &sii902x->i2c->dev;
> > +       unsigned long timeout;
> > +       unsigned int retries;
> > +       u8 status;
> > +       int ret;
> > +
> > +       udelay(30);
> > +
> > +       /*
> > +        * Sometimes the I2C bus can stall after failure to use the
> > +        * EDID channel. Retry a few times to see if things clear
> > +        * up, else continue anyway.
> > +        */
> > +       retries = 5;
> > +       do {
> > +               ret = __sii902x_read(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +                                    &status);
> > +               retries--;
> > +       } while (ret && retries);
> > +       if (ret) {
> > +               dev_err(dev, "failed to read status (%d)\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       ret = __sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +                                   SII902X_SYS_CTRL_DDC_BUS_REQ |
> > +                                   SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> > +       if (ret)
> > +               return ret;
> > +
> > +       timeout = jiffies +
> > +                 msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> > +       do {
> > +               ret = __sii902x_read(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +                                    &status);
> > +               if (ret)
> > +                       return ret;
> > +       } while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> > +                          SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> > +                time_before(jiffies, timeout));
> > +
> > +       if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> > +                     SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> > +               dev_err(dev, "failed to release the i2c bus\n");
> > +               return -ETIMEDOUT;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int sii902x_probe(struct i2c_client *client,
> >                          const struct i2c_device_id *id)
> >  {
> >         struct device *dev = &client->dev;
> > -       unsigned int status = 0;
> >         struct sii902x *sii902x;
> > -       u8 chipid[4];
> > +       u8 chipid[4], status = 0;
> >         int ret;
> >
> > +       ret = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
> > +       if (!ret) {
> > +               dev_err(dev, "I2C adapter not suitable\n");
> > +               return -EIO;
> > +       }
> > +
> >         sii902x = devm_kzalloc(dev, sizeof(*sii902x), GFP_KERNEL);
> >         if (!sii902x)
> >                 return -ENOMEM;
> >
> >         sii902x->i2c = client;
> > -       sii902x->regmap = devm_regmap_init_i2c(client, &sii902x_regmap_config);
> > -       if (IS_ERR(sii902x->regmap))
> > -               return PTR_ERR(sii902x->regmap);
> >
> >         sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> >                                                       GPIOD_OUT_LOW);
> > @@ -394,14 +519,14 @@ static int sii902x_probe(struct i2c_client *client,
> >
> >         sii902x_reset(sii902x);
> >
> > -       ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0);
> > +       ret = sii902x_write(sii902x->i2c, SII902X_REG_TPI_RQB, 0x0);
> >         if (ret)
> >                 return ret;
> >
> > -       ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0),
> > -                              &chipid, 4);
> > +       ret = sii902x_bulk_read(sii902x->i2c, SII902X_REG_CHIPID(0),
> > +                               chipid, 4);
> >         if (ret) {
> > -               dev_err(dev, "regmap_read failed %d\n", ret);
> > +               dev_err(dev, "Can't read chipid (error = %d)\n", ret);
> >                 return ret;
> >         }
> >
> > @@ -412,12 +537,12 @@ static int sii902x_probe(struct i2c_client *client,
> >         }
> >
> >         /* Clear all pending interrupts */
> > -       regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status);
> > -       regmap_write(sii902x->regmap, SII902X_INT_STATUS, status);
> > +       sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status);
> > +       sii902x_write(sii902x->i2c, SII902X_INT_STATUS, status);
> >
> >         if (client->irq > 0) {
> > -               regmap_write(sii902x->regmap, SII902X_INT_ENABLE,
> > -                            SII902X_HOTPLUG_EVENT);
> > +               sii902x_write(sii902x->i2c, SII902X_INT_ENABLE,
> > +                             SII902X_HOTPLUG_EVENT);
> >
> >                 ret = devm_request_threaded_irq(dev, client->irq, NULL,
> >                                                 sii902x_interrupt,
> > @@ -433,6 +558,22 @@ static int sii902x_probe(struct i2c_client *client,
> >
> >         i2c_set_clientdata(client, sii902x);
> >
> > +       sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
> > +                                       1, 0, I2C_MUX_GATE,
> > +                                       sii902x_i2c_bypass_select,
> > +                                       sii902x_i2c_bypass_deselect);
> > +       if (!sii902x->i2cmux) {
> > +               dev_err(dev, "failed to allocate I2C mux\n");
> > +               return -ENOMEM;
> > +       }
> > +
> > +       sii902x->i2cmux->priv = sii902x;
> > +       ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
> > +       if (ret) {
> > +               dev_err(dev, "Couldn't add i2c mux adapter\n");
> > +               return ret;
> > +       }
> > +
> >         return 0;
> >  }
> >
> > @@ -441,6 +582,9 @@ static int sii902x_remove(struct i2c_client *client)
> >  {
> >         struct sii902x *sii902x = i2c_get_clientdata(client);
> >
> > +       if (sii902x->i2cmux)
> > +               i2c_mux_del_adapters(sii902x->i2cmux);
> > +
> >         drm_bridge_remove(&sii902x->bridge);
> >
> >         return 0;
>
> Yours,
> Linus Walleij



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux