Hi,
On Wed, Oct 21, 2015 at 3:54 PM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
>
> On 21-10-15 07:59, Chen-Yu Tsai wrote:
>>
>> This claims and enables regulators listed in the simple framebuffer dt
>> node. This is needed so that regulators powering the display pipeline
>> and external hardware, described in the device node and known by the
>> kernel code, will remain properly enabled.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@xxxxxxxx>
>> ---
>> drivers/video/fbdev/simplefb.c | 122
>> ++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 121 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/video/fbdev/simplefb.c
>> b/drivers/video/fbdev/simplefb.c
>> index 52c5c7e63b52..c4ee10d83a70 100644
>> --- a/drivers/video/fbdev/simplefb.c
>> +++ b/drivers/video/fbdev/simplefb.c
>> @@ -28,7 +28,10 @@
>> #include <linux/platform_device.h>
>> #include <linux/clk.h>
>> #include <linux/clk-provider.h>
>> +#include <linux/of.h>
>> #include <linux/of_platform.h>
>> +#include <linux/parser.h>
>> +#include <linux/regulator/consumer.h>
>>
>> static struct fb_fix_screeninfo simplefb_fix = {
>> .id = "simple",
>> @@ -174,6 +177,10 @@ struct simplefb_par {
>> int clk_count;
>> struct clk **clks;
>> #endif
>> +#if defined CONFIG_OF && defined CONFIG_REGULATOR
>> + u32 regulator_count;
>> + struct regulator **regulators;
>> +#endif
>> };
>>
>> #if defined CONFIG_OF && defined CONFIG_COMMON_CLK
>> @@ -269,6 +276,112 @@ static int simplefb_clocks_init(struct simplefb_par
>> *par,
>> static void simplefb_clocks_destroy(struct simplefb_par *par) { }
>> #endif
>>
>> +#if defined CONFIG_OF && defined CONFIG_REGULATOR
>> +
>> +#define SUPPLY_SUFFIX "-supply"
>> +
>> +/*
>> + * Regulator handling code.
>> + *
>> + * Here we handle the num-supplies and vin*-supply properties of our
>> + * "simple-framebuffer" dt node. This is necessary so that we can make
>> sure
>> + * that any regulators needed by the display hardware that the bootloader
>> + * set up for us (and for which it provided a simplefb dt node), stay up,
>> + * for the life of the simplefb driver.
>> + *
>> + * When the driver unloads, we cleanly disable, and then release the
>> + * regulators.
>> + *
>> + * We only complain about errors here, no action is taken as the most
>> likely
>> + * error can only happen due to a mismatch between the bootloader which
>> set
>> + * up simplefb, and the regulator definitions in the device tree. Chances
>> are
>> + * that there are no adverse effects, and if there are, a clean teardown
>> of
>> + * the fb probe will not help us much either. So just complain and carry
>> on,
>> + * and hope that the user actually gets a working fb at the end of
>> things.
>> + */
>> +static int simplefb_regulators_init(struct simplefb_par *par,
>> + struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct property *prop;
>> + struct regulator *regulator;
>> + const char *p;
>> + int count = 0, i = 0, ret;
>> +
>> + if (dev_get_platdata(&pdev->dev) || !np)
>> + return 0;
>> +
>> + /* Count the number of regulator supplies */
>> + for_each_property_of_node(np, prop) {
>> + p = strstr(prop->name, SUPPLY_SUFFIX);
>> + if (p && p != prop->name)
>> + count++;
>> + }
>> +
>> + if (!count)
>> + return 0;
>> +
>> + par->regulators = devm_kcalloc(&pdev->dev, count,
>> + sizeof(struct regulator *),
>> GFP_KERNEL);
>> + if (!par->regulators)
>> + return -ENOMEM;
>> +
>> + /* Get all the regulators */
>> + for_each_property_of_node(np, prop) {
>> + char name[32]; /* 32 is max size of property name */
>> +
>> + p = strstr(prop->name, SUPPLY_SUFFIX);
>> + if (p && p != prop->name)
>> + continue;
>> +
>> + strlcpy(name, prop->name,
>> + strlen(prop->name) - sizeof(SUPPLY_SUFFIX) + 1);
>> + regulator = devm_regulator_get_optional(&pdev->dev, name);
>> + if (IS_ERR(regulator)) {
>> + if (PTR_ERR(regulator) == -EPROBE_DEFER)
>> + return -EPROBE_DEFER;
>> + dev_err(&pdev->dev, "regulator %s not found:
>> %ld\n",
>> + name, PTR_ERR(regulator));
>> + continue;
>> + }
>> + par->regulators[i++] = regulator;
>
>
> So you only fill slots when the regulator_get has succeeded
>
>> + }
>> + par->regulator_count = i;
>
>
> and regulator_count now is the amount of successfully gotten regulators
> (which may be different from count).
>
>> +
>> + /* Enable all the regulators */
>> + for (i = 0; i < par->regulator_count; i++) {
>> + if (par->regulators[i]) {
>
>
> That means that this "if" is not necessary, it will always be true.
Right. This is leftover code from the first version. I'll remove it.
>> + ret = regulator_enable(par->regulators[i]);
>> + if (ret) {
>> + dev_err(&pdev->dev,
>> + "failed to enable regulator %d:
>> %d\n",
>> + i, ret);
>> + devm_regulator_put(par->regulators[i]);
>> + par->regulators[i] = NULL;
Note here.
>> + }
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void simplefb_regulators_destroy(struct simplefb_par *par)
>> +{
>> + int i;
>> +
>> + if (!par->regulators)
>> + return;
>> +
>> + for (i = 0; i < par->regulator_count; i++)
>> + if (par->regulators[i])
>
>
> And idem for this if.
This is still needed, since if we fail to enable any regulator, we just
ignore it, call regulator_put() on it, and forget about it (set the entry
to NULL). See the noted place above.
>
> Other then that this patch looks good and is:
>
> Reviewed-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Thanks. I'll wait a bit before sending the next version, in case Mark has
some comments on the regulator usage.
Regards
ChenYu
>> + regulator_disable(par->regulators[i]);
>> +}
>> +#else
>> +static int simplefb_regulators_init(struct simplefb_par *par,
>> + struct platform_device *pdev) { return 0; }
>> +static void simplefb_regulators_destroy(struct simplefb_par *par) { }
>> +#endif
>> +
>> static int simplefb_probe(struct platform_device *pdev)
>> {
>> int ret;
>> @@ -340,6 +453,10 @@ static int simplefb_probe(struct platform_device
>> *pdev)
>> if (ret < 0)
>> goto error_unmap;
>>
>> + ret = simplefb_regulators_init(par, pdev);
>> + if (ret < 0)
>> + goto error_clocks;
>> +
>> dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to
>> 0x%p\n",
>> info->fix.smem_start, info->fix.smem_len,
>> info->screen_base);
>> @@ -351,13 +468,15 @@ static int simplefb_probe(struct platform_device
>> *pdev)
>> ret = register_framebuffer(info);
>> if (ret < 0) {
>> dev_err(&pdev->dev, "Unable to register simplefb: %d\n",
>> ret);
>> - goto error_clocks;
>> + goto error_regulators;
>> }
>>
>> dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
>>
>> return 0;
>>
>> +error_regulators:
>> + simplefb_regulators_destroy(par);
>> error_clocks:
>> simplefb_clocks_destroy(par);
>> error_unmap:
>> @@ -373,6 +492,7 @@ static int simplefb_remove(struct platform_device
>> *pdev)
>> struct simplefb_par *par = info->par;
>>
>> unregister_framebuffer(info);
>> + simplefb_regulators_destroy(par);
>> simplefb_clocks_destroy(par);
>> framebuffer_release(info);
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
