linux-kernel - Re: [RESEND PATCH v4 4/4] drm: bridge: anx78xx: Add anx78xx driver support.

Open Source and information security mailing list archives

Message-ID: <CAFqH_503y=pAH7BsXxsRuCn-VutTobJ_H2QK_nmu8xAKZ-Xhmg@mail.gmail.com>
Date:	Mon, 2 May 2016 17:22:08 +0200
From:	Enric Balletbo Serra <eballetbo@...il.com>
To:	Thierry Reding <thierry.reding@...il.com>
Cc:	Enric Balletbo i Serra <enric.balletbo@...labora.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	drinkcat@...omium.org, Emil Velikov <emil.l.velikov@...il.com>,
	linux-kernel@...r.kernel.org,
	dri-devel <dri-devel@...ts.freedesktop.org>,
	Rob Herring <robh+dt@...nel.org>,
	Thierry Reding <treding@...dia.com>,
	Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [RESEND PATCH v4 4/4] drm: bridge: anx78xx: Add anx78xx driver support.

Hi Thierry,

2016-05-02 16:22 GMT+02:00 Thierry Reding <thierry.reding@...il.com>:
> On Mon, May 02, 2016 at 09:54:26AM +0200, Enric Balletbo i Serra wrote:
> [...]
>> diff --git a/drivers/gpu/drm/bridge/analogix-anx78xx.c b/drivers/gpu/drm/bridge/analogix-anx78xx.c
> [...]
>> +static int anx78xx_init_pdata(struct anx78xx *anx78xx)
>> +{
>> +     struct device *dev = &anx78xx->client->dev;
>> +     struct anx78xx_platform_data *pdata = &anx78xx->pdata;
>> +
>> +     /* 1.0V digital core power regulator (optional) */
>> +     pdata->dvdd10 = devm_regulator_get(dev, "dvdd10");
>> +     if (IS_ERR(pdata->dvdd10)) {
>> +             DRM_INFO("DVDD10 regulator not found\n");
>> +             pdata->dvdd10 = NULL;
>> +     }
>
> I'm almost sure that this isn't what you want. What if the regulator is
> hooked up but the bridge driver probes before the regulator. I think
> what you really want is to simply propagate the error code via:
>
>         return PTR_ERR(pdata->dvdd10);
>
> My understanding is that the regulator core will give you a dummy one if
> there's really nothing hooked up. I think you're also supposed to call
> regulator_get_optional() (or the devm_*() equivalent) if this is truly
> an optional supply. Given that it's the "core power" regulator I doubt
> that it's really optional; it's more likely that you may not be able to
> control it, and that it's therefore always on. In that case you're
> supposed to model it in DT as a fixed regulator that's always on.
>

Yes, I was thinking in the case you're are not able to control it and
I make it as optional, and yes, you have reason so It's fine with me
the above solution.

> This is fairly minor and it's really the only thing I could find, so no
> need to respin just for that. If you're fine with the above solution (to
> propagate the error code) I can make the change manually while applying.
>

It's up to you. Whichever you prefer.

Thanks,
 Enric

> Thierry
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives