subject:"\[PATCH v3 2\/3\] drm\: bridge\: Add thc63lvd1024 LVDS decoder driver"

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

2018-03-15 Thread jacopo mondi

HI Andrezej,

On Thu, Mar 15, 2018 at 10:44:42AM +0100, Andrzej Hajda wrote:
> On 14.03.2018 11:09, jacopo mondi wrote:
> > Hi Andrzej,
> > thanks for review
> >
> > On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote:
> >> On 13.03.2018 15:30, Jacopo Mondi wrote:
> >>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> >>> output decoder.
> >> IMO converter suits here better, but it is just suggestion.
> >>
> >>> Signed-off-by: Jacopo Mondi 
> >>> ---
> >>>  drivers/gpu/drm/bridge/Kconfig|   7 +
> >>>  drivers/gpu/drm/bridge/Makefile   |   1 +
> >>>  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 
> >>> ++
> >>>  3 files changed, 249 insertions(+)
> >>>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
> >>>
> >>> diff --git a/drivers/gpu/drm/bridge/Kconfig 
> >>> b/drivers/gpu/drm/bridge/Kconfig
> >>> index 3b99d5a..facf6bd 100644
> >>> --- a/drivers/gpu/drm/bridge/Kconfig
> >>> +++ b/drivers/gpu/drm/bridge/Kconfig
> >>> @@ -92,6 +92,13 @@ config DRM_SII9234
> >>> It is an I2C driver, that detects connection of MHL bridge
> >>> and starts encapsulation of HDMI signal.
> >>>
> >>> +config DRM_THINE_THC63LVD1024
> >>> + tristate "Thine THC63LVD1024 LVDS decoder bridge"
> >>> + depends on OF
> >>> + select DRM_PANEL_BRIDGE
> >> You don't use PANEL_BRIDGE, it should be possible to drop the select.
> >>
> > Ack
> >
> >>> + ---help---
> >>> +   Thine THC63LVD1024 LVDS decoder bridge driver.
> >> Decoder to what?
> >> Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
> >> converter or bridge.
> > "LVDS to digital CMOS/TTL parallel data converter" as the manual
> > describes the chip?
>
> Should work, unless we want some consistency in interface names, we have
> already: parallel / DPI / RGB. I am little bit confused about relations
> between them so I do not want to enforce any.

config DRM_THINE_THC63LVD1024
tristate "Thine THC63LVD1024 LVDS decoder bridge"
depends on OF
---help---
  Thine THC63LVD1024 LVDS/parallel converter driver.

I guess this is consistent with other symbol descriptions I found

>

[snip]
>
> >>> +
> >>> +static int thc63_gpio_init(struct thc63_dev *thc63)
> >>> +{
> >>> + thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
> >>> +   GPIOD_OUT_LOW);
> >> Shouldn't be GPIOD_OUT_HIGH - bridge initially disabled?
> > No, the PWDN gpio is active low, it puts the chip in power down mode
> > when the physical line level is set to 0.
> >
> > That's another confusing thing, when requesting the GPIO, the actual
> > physical line value has to be provided, while when "set_value()" the
> > logical one is requested, iirc.
>
> I think it is opposite, only *raw* functions uses physical level. Other
> uses logical level.
>
> >
> > Being the GPIO defined as active low, in power enable we set it to
> > "logical inactive" == "physical 1", while during power disable it has
> > to be set to "logical active" == "physical 0"
> >
> > Not the right place to complain here, but imo that's not consistent.
> > Or have I misinterpreted the APIs? I cannot do much test, as in my setup
> > the PWDN pin is hardwired to +vcc (through a physical switch, not
> > controlled through a GPIO though).
>
> devm_gpiod_get_optional calls (indirectly) gpiod_configure_flags, which
> calls gpiod_direction_output which uses logical levels.

I clearly mis-interpreted the APIs.

I'll fix and I'm sending v4 out shortly.

Thanks
   j

>
> Regards
> Andrzej
>
> >
> > Thanks
> >j
> >
> >> Regards
> >> Andrzej
> >>> + if (IS_ERR(thc63->pwdn)) {
> >>> + dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
> >>> + return PTR_ERR(thc63->pwdn);
> >>> + }
> >>> +
> >>> + thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> >>> + if (IS_ERR(thc63->oe)) {
> >>> + dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
> >>> + return PTR_ERR(thc63->oe);
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int thc63_regulator_init(struct thc63_dev *thc63)
> >>> +{
> >>> + struct regulator **reg;
> >>> + int i;
> >>> +
> >>> + reg = >vccs[0];
> >>> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++, reg++) {
> >>> + *reg = devm_regulator_get_optional(thc63->dev,
> >>> +thc63_reg_names[i]);
> >>> + if (IS_ERR(*reg)) {
> >>> + if (PTR_ERR(*reg) == -EPROBE_DEFER)
> >>> + return -EPROBE_DEFER;
> >>> + *reg = NULL;
> >>> + }
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static int thc63_probe(struct platform_device *pdev)
> >>> +{
> >>> + struct thc63_dev *thc63;
> >>> + int ret;
> >>> +
> >>> + thc63 = devm_kzalloc(>dev, sizeof(*thc63), GFP_KERNEL);
> >>> + if (!thc63)
> >>> + return -ENOMEM;
> >>> +
> >>> + thc63->dev = >dev;
> >>> +

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

2018-03-15 Thread Andrzej Hajda

On 14.03.2018 11:09, jacopo mondi wrote:
> Hi Andrzej,
> thanks for review
>
> On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote:
>> On 13.03.2018 15:30, Jacopo Mondi wrote:
>>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
>>> output decoder.
>> IMO converter suits here better, but it is just suggestion.
>>
>>> Signed-off-by: Jacopo Mondi 
>>> ---
>>>  drivers/gpu/drm/bridge/Kconfig|   7 +
>>>  drivers/gpu/drm/bridge/Makefile   |   1 +
>>>  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 
>>> ++
>>>  3 files changed, 249 insertions(+)
>>>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>>> index 3b99d5a..facf6bd 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -92,6 +92,13 @@ config DRM_SII9234
>>>   It is an I2C driver, that detects connection of MHL bridge
>>>   and starts encapsulation of HDMI signal.
>>>
>>> +config DRM_THINE_THC63LVD1024
>>> +   tristate "Thine THC63LVD1024 LVDS decoder bridge"
>>> +   depends on OF
>>> +   select DRM_PANEL_BRIDGE
>> You don't use PANEL_BRIDGE, it should be possible to drop the select.
>>
> Ack
>
>>> +   ---help---
>>> + Thine THC63LVD1024 LVDS decoder bridge driver.
>> Decoder to what?
>> Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
>> converter or bridge.
> "LVDS to digital CMOS/TTL parallel data converter" as the manual
> describes the chip?

Should work, unless we want some consistency in interface names, we have
already: parallel / DPI / RGB. I am little bit confused about relations
between them so I do not want to enforce any.


>
>>> +
>>>  config DRM_TOSHIBA_TC358767
>>> tristate "Toshiba TC358767 eDP bridge"
>>> depends on OF
>>> diff --git a/drivers/gpu/drm/bridge/Makefile 
>>> b/drivers/gpu/drm/bridge/Makefile
>>> index 373eb28..fd90b16 100644
>>> --- a/drivers/gpu/drm/bridge/Makefile
>>> +++ b/drivers/gpu/drm/bridge/Makefile
>>> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>>>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>>>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>>>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
>>> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>>>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
>>> b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> new file mode 100644
>>> index 000..4b059c0
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> @@ -0,0 +1,241 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
>>> + *
>>> + * Copyright (C) 2018 Jacopo Mondi 
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +static const char * const thc63_reg_names[] = {
>>> +   "vcc", "lvcc", "pvcc", "cvcc", };
>>> +
>>> +struct thc63_dev {
>>> +   struct device *dev;
>>> +
>>> +   struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
>>> +
>>> +   struct gpio_desc *pwdn;
>>> +   struct gpio_desc *oe;
>>> +
>>> +   struct drm_bridge bridge;
>>> +   struct drm_bridge *next;
>>> +};
>>> +
>>> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
>>> +{
>>> +   return container_of(bridge, struct thc63_dev, bridge);
>>> +}
>>> +
>>> +static int thc63_attach(struct drm_bridge *bridge)
>>> +{
>>> +   struct thc63_dev *thc63 = to_thc63(bridge);
>>> +
>>> +   return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
>>> +}
>>> +
>>> +static void thc63_enable(struct drm_bridge *bridge)
>>> +{
>>> +   struct thc63_dev *thc63 = to_thc63(bridge);
>>> +   struct regulator *vcc;
>>> +   unsigned int i;
>>> +   int ret;
>>> +
>>> +   for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>> +   vcc = thc63->vccs[i];
>>> +   if (vcc) {
>>> +   ret = regulator_enable(vcc);
>>> +   if (ret)
>>> +   goto error_vcc_enable;
>>> +   }
>>> +   }
>>> +
>>> +   if (thc63->pwdn)
>>> +   gpiod_set_value(thc63->pwdn, 0);
>>> +
>>> +   if (thc63->oe)
>>> +   gpiod_set_value(thc63->oe, 1);
>>> +
>>> +   return;
>>> +
>>> +error_vcc_enable:
>>> +   dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
>>> +}
>>> +
>>> +static void thc63_disable(struct drm_bridge *bridge)
>>> +{
>>> +   struct thc63_dev *thc63 = to_thc63(bridge);
>>> +   struct regulator *vcc;
>>> +   unsigned int i;
>>> +   int ret;
>>> +
>>> +   for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>> +   vcc = thc63->vccs[i];
>>> +   if (vcc) {
>>> +   ret = regulator_disable(vcc);
>>> +   if (ret)
>>> +

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

2018-03-14 Thread Sergei Shtylyov

On 03/14/2018 09:04 PM, jacopo mondi wrote:

>>> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
>>> output decoder.
>>>
>>> Signed-off-by: Jacopo Mondi 
>> [...]
>>> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
>>> b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> new file mode 100644
>>> index 000..4b059c0
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
>>> @@ -0,0 +1,241 @@
[...]
>>> +static void thc63_enable(struct drm_bridge *bridge)
>>> +{
>>> +   struct thc63_dev *thc63 = to_thc63(bridge);
>>> +   struct regulator *vcc;
>>> +   unsigned int i;
>>> +   int ret;
>>> +
>>> +   for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
>>> +   vcc = thc63->vccs[i];
>>> +   if (vcc) {
>>> +   ret = regulator_enable(vcc);
>>> +   if (ret)
>>
>>You hardly need this variable, could do a call right in this *if*.
>>
>> [...]
>>> +error_vcc_enable:
>>> +   dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
>>> +}
>>> +
>>
>>Why not do this instead of *goto* before?
> 
> Well, goto breaks the loop, if I only print out the error message, the
> enable sequence will go on and enable the other regulators.

> I can print out and break, but I don't see that much benefit

   Sorry, I meant that you should *return* there instead of *goto*.

> One thing I could do instead, is not only print out the error message,
> but disable the already enabled regulators if one fails to start.

   Yeah, probably...

[...]
>>> +static int thc63_gpio_init(struct thc63_dev *thc63)
>>> +{
>>> +   thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
>>> + GPIOD_OUT_LOW);
>>> +   if (IS_ERR(thc63->pwdn)) {
>>> +   dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
>>
>>"pwdn-gpios" maybe?
>>
>>> +   return PTR_ERR(thc63->pwdn);
>>> +   }
>>> +
>>> +   thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
>>> +   if (IS_ERR(thc63->oe)) {
>>> +   dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
>>
>>"oe-gpios" maybe?
> 
> Are you referring to the error message?

   Yes, seems more clear what to look for this way, IMHO...

[...]

MBR, Sergei

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

2018-03-14 Thread jacopo mondi

Hi Sergei,
   thanks for review

On Wed, Mar 14, 2018 at 08:09:52PM +0300, Sergei Shtylyov wrote:
> On 03/13/2018 05:30 PM, Jacopo Mondi wrote:
>
> > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> > output decoder.
> >
> > Signed-off-by: Jacopo Mondi 
> [...]
> > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
> > b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > new file mode 100644
> > index 000..4b059c0
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > @@ -0,0 +1,241 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> > + *
> > + * Copyright (C) 2018 Jacopo Mondi 
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +static const char * const thc63_reg_names[] = {
> > +   "vcc", "lvcc", "pvcc", "cvcc", };
>
>Your bracing style is pretty strange -- neither here nor there. Please 
> place };
> on the next line...

Yeah, I had doubt about this.. The most common style I found around is

static const char * const foo[] = {
"bar",
"baz",
"...",
};

But seems really too many lines for a bunch of 4 character strings...

>
> [...]
> > +static void thc63_enable(struct drm_bridge *bridge)
> > +{
> > +   struct thc63_dev *thc63 = to_thc63(bridge);
> > +   struct regulator *vcc;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> > +   vcc = thc63->vccs[i];
> > +   if (vcc) {
> > +   ret = regulator_enable(vcc);
> > +   if (ret)
>
>You hardly need this variable, could do a call right in this *if*.
>
> [...]
> > +error_vcc_enable:
> > +   dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
> > +}
> > +
>
>Why not do this instead of *goto* before?

Well, goto breaks the loop, if I only print out the error message, the
enable sequence will go on and enable the other regulators.

I can print out and break, but I don't see that much benefit

One thing I could do instead, is not only print out the error message,
but disable the already enabled regulators if one fails to start.

>
> > +static void thc63_disable(struct drm_bridge *bridge)
> > +{
> > +   struct thc63_dev *thc63 = to_thc63(bridge);
> > +   struct regulator *vcc;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> > +   vcc = thc63->vccs[i];
> > +   if (vcc) {
> > +   ret = regulator_disable(vcc);
> > +   if (ret)
>
>Again, no need for 'ret' whatsoever...
>
> > +   goto error_vcc_disable;
> > +   }
> > +   }
> > +
> > +   if (thc63->pwdn)
> > +   gpiod_set_value(thc63->pwdn, 1);
> > +
> > +   if (thc63->oe)
> > +   gpiod_set_value(thc63->oe, 0);
> > +
> > +   return;
> > +
> > +error_vcc_disable:
> > +   dev_err(thc63->dev, "Failed to disable regulator %u\n", i);
>
>Again, why not do it instead of *goto*?

ditto

>
> [...]
> > +static int thc63_gpio_init(struct thc63_dev *thc63)
> > +{
> > +   thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
> > + GPIOD_OUT_LOW);
> > +   if (IS_ERR(thc63->pwdn)) {
> > +   dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");
>
>"pwdn-gpios" maybe?
>
> > +   return PTR_ERR(thc63->pwdn);
> > +   }
> > +
> > +   thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> > +   if (IS_ERR(thc63->oe)) {
> > +   dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");
>
>"oe-gpios" maybe?

Are you referring to the error message? I can change this, but again, I
see no standards around.

Thanks
   j

>
> [...]
>
> MBR, Sergei


signature.asc
Description: PGP signature

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

2018-03-14 Thread Sergei Shtylyov

On 03/13/2018 05:30 PM, Jacopo Mondi wrote:

> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output decoder.
> 
> Signed-off-by: Jacopo Mondi 
[...]
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
> b/drivers/gpu/drm/bridge/thc63lvd1024.c
> new file mode 100644
> index 000..4b059c0
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,241 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi 
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +static const char * const thc63_reg_names[] = {
> + "vcc", "lvcc", "pvcc", "cvcc", };

   Your bracing style is pretty strange -- neither here nor there. Please place 
};
on the next line...

[...]
> +static void thc63_enable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + struct regulator *vcc;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> + vcc = thc63->vccs[i];
> + if (vcc) {
> + ret = regulator_enable(vcc);
> + if (ret)

   You hardly need this variable, could do a call right in this *if*.

[...]
> +error_vcc_enable:
> + dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
> +}
> +

   Why not do this instead of *goto* before?
 
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + struct regulator *vcc;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> + vcc = thc63->vccs[i];
> + if (vcc) {
> + ret = regulator_disable(vcc);
> + if (ret)

   Again, no need for 'ret' whatsoever...

> + goto error_vcc_disable;
> + }
> + }
> +
> + if (thc63->pwdn)
> + gpiod_set_value(thc63->pwdn, 1);
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 0);
> +
> + return;
> +
> +error_vcc_disable:
> + dev_err(thc63->dev, "Failed to disable regulator %u\n", i);

   Again, why not do it instead of *goto*?

[...]
> +static int thc63_gpio_init(struct thc63_dev *thc63)
> +{
> + thc63->pwdn = devm_gpiod_get_optional(thc63->dev, "pwdn",
> +   GPIOD_OUT_LOW);
> + if (IS_ERR(thc63->pwdn)) {
> + dev_err(thc63->dev, "Unable to get GPIO \"pwdn\"\n");

   "pwdn-gpios" maybe?

> + return PTR_ERR(thc63->pwdn);
> + }
> +
> + thc63->oe = devm_gpiod_get_optional(thc63->dev, "oe", GPIOD_OUT_LOW);
> + if (IS_ERR(thc63->oe)) {
> + dev_err(thc63->dev, "Unable to get GPIO \"oe\"\n");

   "oe-gpios" maybe?

[...]

MBR, Sergei

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

2018-03-14 Thread jacopo mondi

Hi Andrzej,
thanks for review

On Wed, Mar 14, 2018 at 09:42:36AM +0100, Andrzej Hajda wrote:
> On 13.03.2018 15:30, Jacopo Mondi wrote:
> > Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> > output decoder.
>
> IMO converter suits here better, but it is just suggestion.
>
> >
> > Signed-off-by: Jacopo Mondi 
> > ---
> >  drivers/gpu/drm/bridge/Kconfig|   7 +
> >  drivers/gpu/drm/bridge/Makefile   |   1 +
> >  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 
> > ++
> >  3 files changed, 249 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 3b99d5a..facf6bd 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -92,6 +92,13 @@ config DRM_SII9234
> >   It is an I2C driver, that detects connection of MHL bridge
> >   and starts encapsulation of HDMI signal.
> >
> > +config DRM_THINE_THC63LVD1024
> > +   tristate "Thine THC63LVD1024 LVDS decoder bridge"
> > +   depends on OF
> > +   select DRM_PANEL_BRIDGE
>
> You don't use PANEL_BRIDGE, it should be possible to drop the select.
>

Ack

> > +   ---help---
> > + Thine THC63LVD1024 LVDS decoder bridge driver.
>
> Decoder to what?
> Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
> converter or bridge.

"LVDS to digital CMOS/TTL parallel data converter" as the manual
describes the chip?

>
> > +
> >  config DRM_TOSHIBA_TC358767
> > tristate "Toshiba TC358767 eDP bridge"
> > depends on OF
> > diff --git a/drivers/gpu/drm/bridge/Makefile 
> > b/drivers/gpu/drm/bridge/Makefile
> > index 373eb28..fd90b16 100644
> > --- a/drivers/gpu/drm/bridge/Makefile
> > +++ b/drivers/gpu/drm/bridge/Makefile
> > @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
> >  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
> >  obj-$(CONFIG_DRM_SII902X) += sii902x.o
> >  obj-$(CONFIG_DRM_SII9234) += sii9234.o
> > +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
> >  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
> >  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
> >  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> > diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
> > b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > new file mode 100644
> > index 000..4b059c0
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> > @@ -0,0 +1,241 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> > + *
> > + * Copyright (C) 2018 Jacopo Mondi 
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +
> > +static const char * const thc63_reg_names[] = {
> > +   "vcc", "lvcc", "pvcc", "cvcc", };
> > +
> > +struct thc63_dev {
> > +   struct device *dev;
> > +
> > +   struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
> > +
> > +   struct gpio_desc *pwdn;
> > +   struct gpio_desc *oe;
> > +
> > +   struct drm_bridge bridge;
> > +   struct drm_bridge *next;
> > +};
> > +
> > +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> > +{
> > +   return container_of(bridge, struct thc63_dev, bridge);
> > +}
> > +
> > +static int thc63_attach(struct drm_bridge *bridge)
> > +{
> > +   struct thc63_dev *thc63 = to_thc63(bridge);
> > +
> > +   return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
> > +}
> > +
> > +static void thc63_enable(struct drm_bridge *bridge)
> > +{
> > +   struct thc63_dev *thc63 = to_thc63(bridge);
> > +   struct regulator *vcc;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> > +   vcc = thc63->vccs[i];
> > +   if (vcc) {
> > +   ret = regulator_enable(vcc);
> > +   if (ret)
> > +   goto error_vcc_enable;
> > +   }
> > +   }
> > +
> > +   if (thc63->pwdn)
> > +   gpiod_set_value(thc63->pwdn, 0);
> > +
> > +   if (thc63->oe)
> > +   gpiod_set_value(thc63->oe, 1);
> > +
> > +   return;
> > +
> > +error_vcc_enable:
> > +   dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
> > +}
> > +
> > +static void thc63_disable(struct drm_bridge *bridge)
> > +{
> > +   struct thc63_dev *thc63 = to_thc63(bridge);
> > +   struct regulator *vcc;
> > +   unsigned int i;
> > +   int ret;
> > +
> > +   for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> > +   vcc = thc63->vccs[i];
> > +   if (vcc) {
> > +   ret = regulator_disable(vcc);
> > +   if (ret)
> > +   goto error_vcc_disable;
>
> I think in disable path you can report an error and continue - should be
> safer.
>

Ack

> > +   }
> > +   }
> > +
> > +   if (thc63->pwdn)
> > +   gpiod_set_value(thc63->pwdn, 1);
> > +
> > +

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

2018-03-14 Thread Andrzej Hajda

On 13.03.2018 15:30, Jacopo Mondi wrote:
> Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
> output decoder.

IMO converter suits here better, but it is just suggestion.

>
> Signed-off-by: Jacopo Mondi 
> ---
>  drivers/gpu/drm/bridge/Kconfig|   7 +
>  drivers/gpu/drm/bridge/Makefile   |   1 +
>  drivers/gpu/drm/bridge/thc63lvd1024.c | 241 
> ++
>  3 files changed, 249 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..facf6bd 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -92,6 +92,13 @@ config DRM_SII9234
> It is an I2C driver, that detects connection of MHL bridge
> and starts encapsulation of HDMI signal.
>  
> +config DRM_THINE_THC63LVD1024
> + tristate "Thine THC63LVD1024 LVDS decoder bridge"
> + depends on OF
> + select DRM_PANEL_BRIDGE

You don't use PANEL_BRIDGE, it should be possible to drop the select.

> + ---help---
> +   Thine THC63LVD1024 LVDS decoder bridge driver.

Decoder to what?
Maybe it would be better to say it is LVDS/parallel or LVDS/RGB
converter or bridge.

> +
>  config DRM_TOSHIBA_TC358767
>   tristate "Toshiba TC358767 eDP bridge"
>   depends on OF
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..fd90b16 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
>  obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
>  obj-$(CONFIG_DRM_SII902X) += sii902x.o
>  obj-$(CONFIG_DRM_SII9234) += sii9234.o
> +obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
>  obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
> diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
> b/drivers/gpu/drm/bridge/thc63lvd1024.c
> new file mode 100644
> index 000..4b059c0
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
> @@ -0,0 +1,241 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * THC63LVD1024 LVDS to parallel data DRM bridge driver.
> + *
> + * Copyright (C) 2018 Jacopo Mondi 
> + */
> +
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +
> +static const char * const thc63_reg_names[] = {
> + "vcc", "lvcc", "pvcc", "cvcc", };
> +
> +struct thc63_dev {
> + struct device *dev;
> +
> + struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
> +
> + struct gpio_desc *pwdn;
> + struct gpio_desc *oe;
> +
> + struct drm_bridge bridge;
> + struct drm_bridge *next;
> +};
> +
> +static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
> +{
> + return container_of(bridge, struct thc63_dev, bridge);
> +}
> +
> +static int thc63_attach(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> +
> + return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
> +}
> +
> +static void thc63_enable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + struct regulator *vcc;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> + vcc = thc63->vccs[i];
> + if (vcc) {
> + ret = regulator_enable(vcc);
> + if (ret)
> + goto error_vcc_enable;
> + }
> + }
> +
> + if (thc63->pwdn)
> + gpiod_set_value(thc63->pwdn, 0);
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 1);
> +
> + return;
> +
> +error_vcc_enable:
> + dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
> +}
> +
> +static void thc63_disable(struct drm_bridge *bridge)
> +{
> + struct thc63_dev *thc63 = to_thc63(bridge);
> + struct regulator *vcc;
> + unsigned int i;
> + int ret;
> +
> + for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
> + vcc = thc63->vccs[i];
> + if (vcc) {
> + ret = regulator_disable(vcc);
> + if (ret)
> + goto error_vcc_disable;

I think in disable path you can report an error and continue - should be
safer.

> + }
> + }
> +
> + if (thc63->pwdn)
> + gpiod_set_value(thc63->pwdn, 1);
> +
> + if (thc63->oe)
> + gpiod_set_value(thc63->oe, 0);

Usually disable uses reverse order. Ie first disable oe, then, pwdn,
then regulators, also in reverse order.
Looks more reasonable.

> +
> + return;
> +
> +error_vcc_disable:
> + dev_err(thc63->dev, "Failed to disable regulator %u\n", i);
> +}
> +
> +struct drm_bridge_funcs thc63_bridge_func = {
> + .attach =

[PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

2018-03-13 Thread Jacopo Mondi

Add DRM bridge driver for Thine THC63LVD1024 LVDS to digital parallel
output decoder.

Signed-off-by: Jacopo Mondi 
---
 drivers/gpu/drm/bridge/Kconfig|   7 +
 drivers/gpu/drm/bridge/Makefile   |   1 +
 drivers/gpu/drm/bridge/thc63lvd1024.c | 241 ++
 3 files changed, 249 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/thc63lvd1024.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3b99d5a..facf6bd 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -92,6 +92,13 @@ config DRM_SII9234
  It is an I2C driver, that detects connection of MHL bridge
  and starts encapsulation of HDMI signal.
 
+config DRM_THINE_THC63LVD1024
+   tristate "Thine THC63LVD1024 LVDS decoder bridge"
+   depends on OF
+   select DRM_PANEL_BRIDGE
+   ---help---
+ Thine THC63LVD1024 LVDS decoder bridge driver.
+
 config DRM_TOSHIBA_TC358767
tristate "Toshiba TC358767 eDP bridge"
depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 373eb28..fd90b16 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
 obj-$(CONFIG_DRM_SIL_SII8620) += sil-sii8620.o
 obj-$(CONFIG_DRM_SII902X) += sii902x.o
 obj-$(CONFIG_DRM_SII9234) += sii9234.o
+obj-$(CONFIG_DRM_THINE_THC63LVD1024) += thc63lvd1024.o
 obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
 obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
diff --git a/drivers/gpu/drm/bridge/thc63lvd1024.c 
b/drivers/gpu/drm/bridge/thc63lvd1024.c
new file mode 100644
index 000..4b059c0
--- /dev/null
+++ b/drivers/gpu/drm/bridge/thc63lvd1024.c
@@ -0,0 +1,241 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * THC63LVD1024 LVDS to parallel data DRM bridge driver.
+ *
+ * Copyright (C) 2018 Jacopo Mondi 
+ */
+
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+
+static const char * const thc63_reg_names[] = {
+   "vcc", "lvcc", "pvcc", "cvcc", };
+
+struct thc63_dev {
+   struct device *dev;
+
+   struct regulator *vccs[ARRAY_SIZE(thc63_reg_names)];
+
+   struct gpio_desc *pwdn;
+   struct gpio_desc *oe;
+
+   struct drm_bridge bridge;
+   struct drm_bridge *next;
+};
+
+static inline struct thc63_dev *to_thc63(struct drm_bridge *bridge)
+{
+   return container_of(bridge, struct thc63_dev, bridge);
+}
+
+static int thc63_attach(struct drm_bridge *bridge)
+{
+   struct thc63_dev *thc63 = to_thc63(bridge);
+
+   return drm_bridge_attach(bridge->encoder, thc63->next, bridge);
+}
+
+static void thc63_enable(struct drm_bridge *bridge)
+{
+   struct thc63_dev *thc63 = to_thc63(bridge);
+   struct regulator *vcc;
+   unsigned int i;
+   int ret;
+
+   for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
+   vcc = thc63->vccs[i];
+   if (vcc) {
+   ret = regulator_enable(vcc);
+   if (ret)
+   goto error_vcc_enable;
+   }
+   }
+
+   if (thc63->pwdn)
+   gpiod_set_value(thc63->pwdn, 0);
+
+   if (thc63->oe)
+   gpiod_set_value(thc63->oe, 1);
+
+   return;
+
+error_vcc_enable:
+   dev_err(thc63->dev, "Failed to enable regulator %u\n", i);
+}
+
+static void thc63_disable(struct drm_bridge *bridge)
+{
+   struct thc63_dev *thc63 = to_thc63(bridge);
+   struct regulator *vcc;
+   unsigned int i;
+   int ret;
+
+   for (i = 0; i < ARRAY_SIZE(thc63->vccs); i++) {
+   vcc = thc63->vccs[i];
+   if (vcc) {
+   ret = regulator_disable(vcc);
+   if (ret)
+   goto error_vcc_disable;
+   }
+   }
+
+   if (thc63->pwdn)
+   gpiod_set_value(thc63->pwdn, 1);
+
+   if (thc63->oe)
+   gpiod_set_value(thc63->oe, 0);
+
+   return;
+
+error_vcc_disable:
+   dev_err(thc63->dev, "Failed to disable regulator %u\n", i);
+}
+
+struct drm_bridge_funcs thc63_bridge_func = {
+   .attach = thc63_attach,
+   .enable = thc63_enable,
+   .disable = thc63_disable,
+};
+
+static int thc63_parse_dt(struct thc63_dev *thc63)
+{
+   struct device_node *thc63_out;
+   struct device_node *remote;
+   int ret = 0;
+
+   thc63_out = of_graph_get_endpoint_by_regs(thc63->dev->of_node, 2, -1);
+   if (!thc63_out) {
+   dev_err(thc63->dev, "Missing endpoint in Port@2\n");
+   return -ENODEV;
+   }
+
+   remote = of_graph_get_remote_port_parent(thc63_out);
+   if (!remote) {
+   dev_err(thc63->dev, "Endpoint in Port@2 unconnected\n");
+   ret = -ENODEV;
+   goto error_put_out_node;
+

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

Re: [PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

[PATCH v3 2/3] drm: bridge: Add thc63lvd1024 LVDS decoder driver

8 matches

Site Navigation

Mail list logo

Footer information