[PATCH v5 1/2] drm/bridge: Add sii902x driver
On Tue, 7 Jun 2016 08:28:18 + Meng Yi wrote: > Hi Boris, > > > Changes in v5: > > - drop the best_encoder() implementation > > > > Why best_encoder() droped? It's not needed anymore? Nope, not after this series [1]. [1]http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/432869.html -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
[PATCH v5 1/2] drm/bridge: Add sii902x driver
Hi Boris, > Changes in v5: > - drop the best_encoder() implementation > Why best_encoder() droped? It's not needed anymore? Regards, Meng
[PATCH v5 1/2] drm/bridge: Add sii902x driver
Hi Meng, On Tue, 7 Jun 2016 02:40:56 + Meng Yi wrote: > Hi Boris, > > Sorry for reply late, I was on PTO. And another PTO on June 9~11, 2016.UTC+8 > > > > > > > + gpiod_set_value(sii902x->reset_gpio, 1); > > > > + > > > > + msleep(100); > > > Ouch that is some juicy number. Can we get a comment with > > > reasoning/origin of it ? > > > > As already explained to Maxime, I just don't know why this is needed, simply > > because I don't have access to the datasheet and I just based my > > implementation on another driver. > > I can add a comment stating that this was extracted from another > > implementation, but with no explanation on why this is needed. > > > > Meng, do you have any information about startup-time, or something like > > that? > > > > I had checked the datasheet of sii9022a, which said the Min of TRESET is 100 > μs, and Max is not specified. Oh, thanks a lot. treset-min is exactly what I needed. Regards, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
[PATCH v5 1/2] drm/bridge: Add sii902x driver
Hi Boris, Sorry for reply late, I was on PTO. And another PTO on June 9~11, 2016.UTC+8 > > > > + gpiod_set_value(sii902x->reset_gpio, 1); > > > + > > > + msleep(100); > > Ouch that is some juicy number. Can we get a comment with > > reasoning/origin of it ? > > As already explained to Maxime, I just don't know why this is needed, simply > because I don't have access to the datasheet and I just based my > implementation on another driver. > I can add a comment stating that this was extracted from another > implementation, but with no explanation on why this is needed. > > Meng, do you have any information about startup-time, or something like that? > I had checked the datasheet of sii9022a, which said the Min of TRESET is 100 μs, and Max is not specified. Regards, Meng
[PATCH v5 1/2] drm/bridge: Add sii902x driver
On 3 June 2016 at 11:02, Boris Brezillon
wrote:
> Hi Emil,
>
> On Fri, 3 Jun 2016 10:38:49 +0100
> Emil Velikov wrote:
>
>> Hi Boris.
>>
>> On 2 June 2016 at 16:00, Boris Brezillon
>> wrote:
>>
>> > +static void sii902x_reset(struct sii902x *sii902x)
>> > +{
>> > + if (!sii902x->reset_gpio)
>> > + return;
>> > +
>> This is wrong (reset_gpio is err_ptr) although we can/should nuke it
>> all together. See below for reasoning.
>>
>> > + gpiod_set_value(sii902x->reset_gpio, 1);
>> > +
>> > + msleep(100);
>> Ouch that is some juicy number. Can we get a comment with
>> reasoning/origin of it ?
>
> As already explained to Maxime, I just don't know why this is needed,
> simply because I don't have access to the datasheet and I just based my
> implementation on another driver.
> I can add a comment stating that this was extracted from another
> implementation, but with no explanation on why this is needed.
>
Considering we don't hear from Meng, that sounds perfectly reasonable imho.
> Meng, do you have any information about startup-time, or something like
> that?
>
>>
>> ...
>>
>> > +static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
>> > + struct drm_display_mode *mode,
>> > + struct drm_display_mode *adj)
>> > +{
>> > + u8 buf[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];
>> HDMI_INFOFRAME_SIZE(AVI) seems shorter/easier to head imho.
>
> Yep.
>
>>
>> > + struct sii902x *sii902x = bridge_to_sii902x(bridge);
>> > + struct regmap *regmap = sii902x->regmap;
>> > + 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] = SIL902X_TPI_CLK_RATIO_1X | SIL902X_TPI_AVI_PIXEL_REP_NONE
>> > |
>> > +SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT;
>> > + buf[9] = SIL902X_TPI_AVI_INPUT_RANGE_AUTO |
>> > +SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB;
>> > +
>> Since all of the contents are cleared in hdmi_avi_infoframe_pack, move
>> the above into const video_data[] ?
>
> Something like
>
> const video_data[] = {
> adj->clock,
> adj->clock >> 8,
> ...
> };
>
> So we would have 2 buffers on the stack? Is this really useful?
>
IMHO It makes things substantially clearer, but if you/others
disagree, feel free to ignore.
>>
>> > + ret = regmap_bulk_write(regmap, SIL902X_TPI_VIDEO_DATA, buf, 10);
>> ... and use ARRAY_SIZE(video_data) over the hardcoded 10 ?
>>
>> ...
>>
>> > +static int sii902x_bridge_attach(struct drm_bridge *bridge)
>> > +{
>> > + struct sii902x *sii902x = bridge_to_sii902x(bridge);
>> > + struct drm_device *drm = bridge->dev;
>> > + int ret;
>> > +
>> > + drm_connector_helper_add(>connector,
>> > +_connector_helper_funcs);
>> > +
>> > + if (!drm_core_check_feature(drm, DRIVER_ATOMIC)) {
>> > + dev_err(>i2c->dev,
>> > + "sii902x driver is only compatible with DRM
>> > devices supporting atomic updates");
>> > + return -ENOTSUPP;
>> > + }
>> > +
>> > + ret = drm_connector_init(drm, >connector,
>> > +_connector_funcs,
>> > +DRM_MODE_CONNECTOR_HDMIA);
>> Side note: seems like most places in DRM do not check the return value
>> (~80 vs ~20). I wonder how badly/likely are things to explode.
>
> Yep. I tend to always check return code, but if you say it's useless
> (and error-prone) I can remove it.
>
Not sure which one is the correct thing to do. My gut goes "keep it as
is for the moment". I only pointed it out since DRM subsystem was
quite inconsistent about it.
>>
>> ...
>>
>> > +static int sii902x_probe(struct i2c_client *client,
>> > +const struct i2c_device_id *id)
>> > +{
>> ...
>>
>> > +
>> > + sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
>> > + GPIOD_OUT_LOW);
>> > + if (IS_ERR(sii902x->reset_gpio))
>> > + dev_warn(dev, "Failed to retrieve/request reset gpio:
>> > %ld\n",
>> > +PTR_ERR(sii902x->reset_gpio));
>> > +
>> Documentation says "Required" not optional. The above should be
>> updated and one should error out if missing, right ?
>
> Actually I was asked to make it optional, just forgot to update the
> documentation. This being said, devm_gpiod_get_optional() returns NULL
> if the property is not defined in the DT and an error code if the error
> comes from the GPIO layer, so I should just switch back to dev_err()
> and return
[PATCH v5 1/2] drm/bridge: Add sii902x driver
Am Freitag, den 03.06.2016, 10:38 +0100 schrieb Emil Velikov:
> Hi Boris.
[...]
>
> > +
> > + if (client->irq > 0) {
> I was always confused which is the correct way to check this >= 0 vs >
> 0. DRM has both :-\
> Do you have any suggestions, should be 'mass convert' DRM to use only
> one of the two ?
>
IRQ 0 is "no assigned irq", so invalid. Checking for > 0 is the right
thing to do.
Regards,
LUcas
[PATCH v5 1/2] drm/bridge: Add sii902x driver
Hi,
On 03/06/2016 at 10:38:49 +0100, Emil Velikov wrote :
> On 2 June 2016 at 16:00, Boris Brezillon
> wrote:
> > +
> > + if (client->irq > 0) {
> I was always confused which is the correct way to check this >= 0 vs >
> 0. DRM has both :-\
> Do you have any suggestions, should be 'mass convert' DRM to use only
> one of the two ?
>
0 is an invalid irq number for any i2c devices since
dab472eb931bc2916fa779e56deccd0ec319cf5b.
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[PATCH v5 1/2] drm/bridge: Add sii902x driver
Hi Emil,
On Fri, 3 Jun 2016 10:38:49 +0100
Emil Velikov wrote:
> Hi Boris.
>
> On 2 June 2016 at 16:00, Boris Brezillon
> wrote:
>
> > +static void sii902x_reset(struct sii902x *sii902x)
> > +{
> > + if (!sii902x->reset_gpio)
> > + return;
> > +
> This is wrong (reset_gpio is err_ptr) although we can/should nuke it
> all together. See below for reasoning.
>
> > + gpiod_set_value(sii902x->reset_gpio, 1);
> > +
> > + msleep(100);
> Ouch that is some juicy number. Can we get a comment with
> reasoning/origin of it ?
As already explained to Maxime, I just don't know why this is needed,
simply because I don't have access to the datasheet and I just based my
implementation on another driver.
I can add a comment stating that this was extracted from another
implementation, but with no explanation on why this is needed.
Meng, do you have any information about startup-time, or something like
that?
>
> ...
>
> > +static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
> > + struct drm_display_mode *mode,
> > + struct drm_display_mode *adj)
> > +{
> > + u8 buf[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];
> HDMI_INFOFRAME_SIZE(AVI) seems shorter/easier to head imho.
Yep.
>
> > + struct sii902x *sii902x = bridge_to_sii902x(bridge);
> > + struct regmap *regmap = sii902x->regmap;
> > + 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] = SIL902X_TPI_CLK_RATIO_1X | SIL902X_TPI_AVI_PIXEL_REP_NONE |
> > +SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT;
> > + buf[9] = SIL902X_TPI_AVI_INPUT_RANGE_AUTO |
> > +SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB;
> > +
> Since all of the contents are cleared in hdmi_avi_infoframe_pack, move
> the above into const video_data[] ?
Something like
const video_data[] = {
adj->clock,
adj->clock >> 8,
...
};
So we would have 2 buffers on the stack? Is this really useful?
>
> > + ret = regmap_bulk_write(regmap, SIL902X_TPI_VIDEO_DATA, buf, 10);
> ... and use ARRAY_SIZE(video_data) over the hardcoded 10 ?
>
> ...
>
> > +static int sii902x_bridge_attach(struct drm_bridge *bridge)
> > +{
> > + struct sii902x *sii902x = bridge_to_sii902x(bridge);
> > + struct drm_device *drm = bridge->dev;
> > + int ret;
> > +
> > + drm_connector_helper_add(>connector,
> > +_connector_helper_funcs);
> > +
> > + if (!drm_core_check_feature(drm, DRIVER_ATOMIC)) {
> > + dev_err(>i2c->dev,
> > + "sii902x driver is only compatible with DRM devices
> > supporting atomic updates");
> > + return -ENOTSUPP;
> > + }
> > +
> > + ret = drm_connector_init(drm, >connector,
> > +_connector_funcs,
> > +DRM_MODE_CONNECTOR_HDMIA);
> Side note: seems like most places in DRM do not check the return value
> (~80 vs ~20). I wonder how badly/likely are things to explode.
Yep. I tend to always check return code, but if you say it's useless
(and error-prone) I can remove it.
>
> ...
>
> > +static int sii902x_probe(struct i2c_client *client,
> > +const struct i2c_device_id *id)
> > +{
> ...
>
> > +
> > + sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> > + GPIOD_OUT_LOW);
> > + if (IS_ERR(sii902x->reset_gpio))
> > + dev_warn(dev, "Failed to retrieve/request reset gpio:
> > %ld\n",
> > +PTR_ERR(sii902x->reset_gpio));
> > +
> Documentation says "Required" not optional. The above should be
> updated and one should error out if missing, right ?
Actually I was asked to make it optional, just forgot to update the
documentation. This being said, devm_gpiod_get_optional() returns NULL
if the property is not defined in the DT and an error code if the error
comes from the GPIO layer, so I should just switch back to dev_err()
and return the error code here.
This would make the test in sii902x_reset() valid again.
>
> ...
>
> > +
> > + if (client->irq > 0) {
> I was always confused which is the correct way to check this >= 0 vs >
> 0. DRM has both :-\
> Do you have any suggestions, should be 'mass convert' DRM to use only
> one of the two ?
Not sure 0 is a valid irq number anymore, so I don't think it's really
important, but I can change it if you want.
Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel
[PATCH v5 1/2] drm/bridge: Add sii902x driver
Hi Boris.
On 2 June 2016 at 16:00, Boris Brezillon
wrote:
> +static void sii902x_reset(struct sii902x *sii902x)
> +{
> + if (!sii902x->reset_gpio)
> + return;
> +
This is wrong (reset_gpio is err_ptr) although we can/should nuke it
all together. See below for reasoning.
> + gpiod_set_value(sii902x->reset_gpio, 1);
> +
> + msleep(100);
Ouch that is some juicy number. Can we get a comment with
reasoning/origin of it ?
...
> +static void sii902x_bridge_mode_set(struct drm_bridge *bridge,
> + struct drm_display_mode *mode,
> + struct drm_display_mode *adj)
> +{
> + u8 buf[HDMI_INFOFRAME_HEADER_SIZE + HDMI_AVI_INFOFRAME_SIZE];
HDMI_INFOFRAME_SIZE(AVI) seems shorter/easier to head imho.
> + struct sii902x *sii902x = bridge_to_sii902x(bridge);
> + struct regmap *regmap = sii902x->regmap;
> + 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] = SIL902X_TPI_CLK_RATIO_1X | SIL902X_TPI_AVI_PIXEL_REP_NONE |
> +SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT;
> + buf[9] = SIL902X_TPI_AVI_INPUT_RANGE_AUTO |
> +SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB;
> +
Since all of the contents are cleared in hdmi_avi_infoframe_pack, move
the above into const video_data[] ?
> + ret = regmap_bulk_write(regmap, SIL902X_TPI_VIDEO_DATA, buf, 10);
... and use ARRAY_SIZE(video_data) over the hardcoded 10 ?
...
> +static int sii902x_bridge_attach(struct drm_bridge *bridge)
> +{
> + struct sii902x *sii902x = bridge_to_sii902x(bridge);
> + struct drm_device *drm = bridge->dev;
> + int ret;
> +
> + drm_connector_helper_add(>connector,
> +_connector_helper_funcs);
> +
> + if (!drm_core_check_feature(drm, DRIVER_ATOMIC)) {
> + dev_err(>i2c->dev,
> + "sii902x driver is only compatible with DRM devices
> supporting atomic updates");
> + return -ENOTSUPP;
> + }
> +
> + ret = drm_connector_init(drm, >connector,
> +_connector_funcs,
> +DRM_MODE_CONNECTOR_HDMIA);
Side note: seems like most places in DRM do not check the return value
(~80 vs ~20). I wonder how badly/likely are things to explode.
...
> +static int sii902x_probe(struct i2c_client *client,
> +const struct i2c_device_id *id)
> +{
...
> +
> + sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset",
> + GPIOD_OUT_LOW);
> + if (IS_ERR(sii902x->reset_gpio))
> + dev_warn(dev, "Failed to retrieve/request reset gpio: %ld\n",
> +PTR_ERR(sii902x->reset_gpio));
> +
Documentation says "Required" not optional. The above should be
updated and one should error out if missing, right ?
...
> +
> + if (client->irq > 0) {
I was always confused which is the correct way to check this >= 0 vs >
0. DRM has both :-\
Do you have any suggestions, should be 'mass convert' DRM to use only
one of the two ?
Regards,
Emil
[PATCH v5 1/2] drm/bridge: Add sii902x driver
Hi Enric,
On Thu, 2 Jun 2016 23:47:23 +0200
Enric Balletbo Serra wrote:
> > +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;
> > + unsigned int status;
> > + struct edid *edid;
> > + int num = 0;
> > + int ret;
> > + int i;
>
> Is the i variable really needed? (see my comments below)
>
> > +
> > + ret = regmap_update_bits(regmap, SIL902X_SYS_CTRL_DATA,
> > +SIL902X_SYS_CTRL_DDC_BUS_REQ,
> > +SIL902X_SYS_CTRL_DDC_BUS_REQ);
> > + if (ret)
> > + return ret;
> > +
> > + i = 0;
>
> You assign i to 0
>
> > + do {
> > + ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, );
> > + if (ret)
> > + return ret;
> > + i++;
>
> And you increment i, for what?
Oops, this is a leftover from when I was debugging the implementation.
>
> > + } while (!(status & SIL902X_SYS_CTRL_DDC_BUS_GRTD));
> > +
> > + ret = regmap_write(regmap, SIL902X_SYS_CTRL_DATA, status);
> > + if (ret)
> > + return ret;
> > +
> > + edid = drm_get_edid(connector, sii902x->i2c->adapter);
> > + drm_mode_connector_update_edid_property(connector, edid);
> > + if (edid) {
> > + num += drm_add_edid_modes(connector, edid);
>
> This is always 0 + the returned value, so you can do:
> num = drm_add_edid_modes(connector, edid);
> It's more clear for me.
Sure.
>
> > + kfree(edid);
> > + }
> > +
> > + ret = drm_display_info_set_bus_formats(>display_info,
> > + _format, 1);
> > + if (ret)
> > + return ret;
> > +
> > + regmap_read(regmap, SIL902X_SYS_CTRL_DATA, );
> > + if (ret)
> > + return ret;
> > +
> > + ret = regmap_update_bits(regmap, SIL902X_SYS_CTRL_DATA,
> > +SIL902X_SYS_CTRL_DDC_BUS_REQ |
> > +SIL902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> > + if (ret)
> > + return ret;
> > +
> > + i = 0;
>
> Again, you can remove the i variable, here and the i++ from the loop below
>
> > + do {
> > + ret = regmap_read(regmap, SIL902X_SYS_CTRL_DATA, );
> > + if (ret)
> > + return ret;
> > + i++;
> > + } while (status & (SIL902X_SYS_CTRL_DDC_BUS_REQ |
> > + SIL902X_SYS_CTRL_DDC_BUS_GRTD));
> > +
> > + return num;
> > +}
[...]
> > +static void sii902x_bridge_nop(struct drm_bridge *bridge)
> > +{
> > +}
> > +
>
> You can remove this dummy callback function now.
>
> > +static const struct drm_bridge_funcs sii902x_bridge_funcs = {
> > + .attach = sii902x_bridge_attach,
> > + .mode_set = sii902x_bridge_mode_set,
> > + .disable = sii902x_bridge_disable,
> > + .post_disable = sii902x_bridge_nop,
> > + .pre_enable = sii902x_bridge_nop,
>
> Remove .pre_enable
I guess ->{pre,post}_enable() were mandatory when I started the
development of this driver. I'll drop both.
>
> > + .enable = sii902x_bridge_enable,
> > +};
> > +
[...]
> > +
> > +#ifdef CONFIG_OF
>
> You already depend on OF in Kconfig so this will always be evaluated.
Indeed.
>
> > +static const struct of_device_id sii902x_dt_ids[] = {
> > + { .compatible = "sil,sii9022", },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, sii902x_dt_ids);
> > +#endif
> > +
> > +static const struct i2c_device_id sii902x_i2c_ids[] = {
> > + { "sii9022", 0 },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(i2c, sii902x_i2c_ids);
> > +
> > +static struct i2c_driver sii902x_driver = {
> > + .probe = sii902x_probe,
> > + .remove = sii902x_remove,
> > + .driver = {
> > + .name = "sii902x",
> > + .of_match_table = of_match_ptr(sii902x_dt_ids),
>
> You already depend on OF in Kconfig so you don't need of_match_ptr()
> here, of_match_ptr(x) will always evaluate to x.
I'll directly pass sii902x_dt_ids.
Thanks for your review.
Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
[PATCH v5 1/2] drm/bridge: Add sii902x driver
Hi Boris, A few comments below ... 2016-06-02 17:00 GMT+02:00 Boris Brezillon : > Add basic support for the sii902x RGB -> HDMI bridge. > This driver does not support audio output yet. > > Signed-off-by: Boris Brezillon > Tested-by: Nicolas Ferre > --- > Hello, > > This patch is only adding basic support for the sii9022 chip. > As stated in the commit log, there's no audio support, but the > driver also hardcodes a lot of things (like the RGB input format to > use). > > Best Regards, > > Boris > > Changes in v5: > - drop the best_encoder() implementation > > Changes in v4: > - make reset GPIO optional > - only support attaching to DRM devices supporting atomic updates > > Changes in v3: > - fix get_modes() implementation to avoid turning the screen in power > save mode > - rename the driver (sil902x -> sii902x) > > Changes in v2: > - fix errors reported by the kbuild robot > > fixup! drm: bridge: Add sii902x driver > --- > drivers/gpu/drm/bridge/Kconfig | 8 + > drivers/gpu/drm/bridge/Makefile | 1 + > drivers/gpu/drm/bridge/sii902x.c | 469 > +++ > 3 files changed, 478 insertions(+) > create mode 100644 drivers/gpu/drm/bridge/sii902x.c > > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > index 8f7423f..a1419214 100644 > --- a/drivers/gpu/drm/bridge/Kconfig > +++ b/drivers/gpu/drm/bridge/Kconfig > @@ -50,6 +50,14 @@ config DRM_PARADE_PS8622 > ---help--- > Parade eDP-LVDS bridge chip driver. > > +config DRM_SII902X > + tristate "Silicon Image sii902x RGB/HDMI bridge" > + depends on OF > + select DRM_KMS_HELPER > + select REGMAP_I2C > + ---help--- > + Silicon Image sii902x bridge chip driver. > + > source "drivers/gpu/drm/bridge/analogix/Kconfig" > > endmenu > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 96b13b3..bfec9f8 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -5,4 +5,5 @@ obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o > obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o > obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o > obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o > +obj-$(CONFIG_DRM_SII902X) += sii902x.o > obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ > diff --git a/drivers/gpu/drm/bridge/sii902x.c > b/drivers/gpu/drm/bridge/sii902x.c > new file mode 100644 > index 000..0912d2f > --- /dev/null > +++ b/drivers/gpu/drm/bridge/sii902x.c > @@ -0,0 +1,469 @@ > +/* > + * Copyright (C) 2014 Atmel > + * Bo Shen > + * > + * Authors: Bo Shen > + * Boris Brezillon > + * Wu, Songjun > + * > + * > + * Copyright (C) 2010-2011 Freescale Semiconductor, Inc. All Rights Reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define SIL902X_TPI_VIDEO_DATA 0x0 > + > +#define SIL902X_TPI_PIXEL_REPETITION 0x8 > +#define SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT BIT(5) > +#define SIL902X_TPI_AVI_PIXEL_REP_RISING_EDGE BIT(4) > +#define SIL902X_TPI_AVI_PIXEL_REP_4X 3 > +#define SIL902X_TPI_AVI_PIXEL_REP_2X 1 > +#define SIL902X_TPI_AVI_PIXEL_REP_NONE 0 > +#define SIL902X_TPI_CLK_RATIO_HALF (0 << 6) > +#define SIL902X_TPI_CLK_RATIO_1X (1 << 6) > +#define SIL902X_TPI_CLK_RATIO_2X (2 << 6) > +#define SIL902X_TPI_CLK_RATIO_4X (3 << 6) > + > +#define SIL902X_TPI_AVI_IN_FORMAT 0x9 > +#define SIL902X_TPI_AVI_INPUT_BITMODE_12BITBIT(7) > +#define SIL902X_TPI_AVI_INPUT_DITHER BIT(6) > +#define SIL902X_TPI_AVI_INPUT_RANGE_LIMITED(2 << 2) > +#define SIL902X_TPI_AVI_INPUT_RANGE_FULL (1 << 2) > +#define SIL902X_TPI_AVI_INPUT_RANGE_AUTO (0 << 2) > +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_BLACK (3 << 0) > +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_YUV422(2 << 0) > +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_YUV444(1 << 0) > +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB (0 << 0) > + > +#define SIL902X_TPI_AVI_INFOFRAME 0x0c > + > +#define SIL902X_SYS_CTRL_DATA 0x1a > +#define SIL902X_SYS_CTRL_PWR_DWN BIT(4) > +#define SIL902X_SYS_CTRL_AV_MUTE BIT(3) >
[PATCH v5 1/2] drm/bridge: Add sii902x driver
Add basic support for the sii902x RGB -> HDMI bridge. This driver does not support audio output yet. Signed-off-by: Boris Brezillon Tested-by: Nicolas Ferre --- Hello, This patch is only adding basic support for the sii9022 chip. As stated in the commit log, there's no audio support, but the driver also hardcodes a lot of things (like the RGB input format to use). Best Regards, Boris Changes in v5: - drop the best_encoder() implementation Changes in v4: - make reset GPIO optional - only support attaching to DRM devices supporting atomic updates Changes in v3: - fix get_modes() implementation to avoid turning the screen in power save mode - rename the driver (sil902x -> sii902x) Changes in v2: - fix errors reported by the kbuild robot fixup! drm: bridge: Add sii902x driver --- drivers/gpu/drm/bridge/Kconfig | 8 + drivers/gpu/drm/bridge/Makefile | 1 + drivers/gpu/drm/bridge/sii902x.c | 469 +++ 3 files changed, 478 insertions(+) create mode 100644 drivers/gpu/drm/bridge/sii902x.c diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 8f7423f..a1419214 100644 --- a/drivers/gpu/drm/bridge/Kconfig +++ b/drivers/gpu/drm/bridge/Kconfig @@ -50,6 +50,14 @@ config DRM_PARADE_PS8622 ---help--- Parade eDP-LVDS bridge chip driver. +config DRM_SII902X + tristate "Silicon Image sii902x RGB/HDMI bridge" + depends on OF + select DRM_KMS_HELPER + select REGMAP_I2C + ---help--- + Silicon Image sii902x bridge chip driver. + source "drivers/gpu/drm/bridge/analogix/Kconfig" endmenu diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile index 96b13b3..bfec9f8 100644 --- a/drivers/gpu/drm/bridge/Makefile +++ b/drivers/gpu/drm/bridge/Makefile @@ -5,4 +5,5 @@ obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o +obj-$(CONFIG_DRM_SII902X) += sii902x.o obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c new file mode 100644 index 000..0912d2f --- /dev/null +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -0,0 +1,469 @@ +/* + * Copyright (C) 2014 Atmel + * Bo Shen + * + * Authors: Bo Shen + * Boris Brezillon + * Wu, Songjun + * + * + * Copyright (C) 2010-2011 Freescale Semiconductor, Inc. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +#define SIL902X_TPI_VIDEO_DATA 0x0 + +#define SIL902X_TPI_PIXEL_REPETITION 0x8 +#define SIL902X_TPI_AVI_PIXEL_REP_BUS_24BIT BIT(5) +#define SIL902X_TPI_AVI_PIXEL_REP_RISING_EDGE BIT(4) +#define SIL902X_TPI_AVI_PIXEL_REP_4X 3 +#define SIL902X_TPI_AVI_PIXEL_REP_2X 1 +#define SIL902X_TPI_AVI_PIXEL_REP_NONE 0 +#define SIL902X_TPI_CLK_RATIO_HALF (0 << 6) +#define SIL902X_TPI_CLK_RATIO_1X (1 << 6) +#define SIL902X_TPI_CLK_RATIO_2X (2 << 6) +#define SIL902X_TPI_CLK_RATIO_4X (3 << 6) + +#define SIL902X_TPI_AVI_IN_FORMAT 0x9 +#define SIL902X_TPI_AVI_INPUT_BITMODE_12BITBIT(7) +#define SIL902X_TPI_AVI_INPUT_DITHER BIT(6) +#define SIL902X_TPI_AVI_INPUT_RANGE_LIMITED(2 << 2) +#define SIL902X_TPI_AVI_INPUT_RANGE_FULL (1 << 2) +#define SIL902X_TPI_AVI_INPUT_RANGE_AUTO (0 << 2) +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_BLACK (3 << 0) +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_YUV422(2 << 0) +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_YUV444(1 << 0) +#define SIL902X_TPI_AVI_INPUT_COLORSPACE_RGB (0 << 0) + +#define SIL902X_TPI_AVI_INFOFRAME 0x0c + +#define SIL902X_SYS_CTRL_DATA 0x1a +#define SIL902X_SYS_CTRL_PWR_DWN BIT(4) +#define SIL902X_SYS_CTRL_AV_MUTE BIT(3) +#define SIL902X_SYS_CTRL_DDC_BUS_REQ BIT(2) +#define SIL902X_SYS_CTRL_DDC_BUS_GRTD BIT(1) +#define SIL902X_SYS_CTRL_OUTPUT_MODE BIT(0) +#define SIL902X_SYS_CTRL_OUTPUT_HDMI 1 +#define SIL902X_SYS_CTRL_OUTPUT_DVI0 + +#define SIL902X_REG_CHIPID(n) (0x1b + (n)) + +#define SIL902X_PWR_STATE_CTRL
