Message-ID: <6df731a5-9dc8-ae5b-dc14-d7ea5eb3890b@redhat.com>
Date:   Sun, 6 Aug 2017 16:52:32 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Wolfram Sang <wsa@...-dreams.de>,
        Sebastian Reichel <sre@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-i2c@...r.kernel.org, Liam Breck <liam@...workimprov.net>,
        Tony Lindgren <tony@...mide.com>, linux-pm@...r.kernel.org,
        devel@...verdev.osuosl.org
Subject: Re: [PATCH 10/18] staging: typec: fusb302: Add support for
 fcs,vbus-regulator-name device-property

Hi,

On 06-08-17 16:30, Guenter Roeck wrote:
> On 08/06/2017 05:35 AM, Hans de Goede wrote:
>> On devicetree platforms the fusb302 dt-node will have a vbus regulator
>> property with a phandle to the regulator.
>>
>> On ACPI platforms, there are no phandles and we need to get the vbus by a
>> system wide unique name. Add support for a new "fcs,vbus-regulator-name"
>> device-property which ACPI platform code can set to pass the name.
>>
>> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
>> ---
>>   drivers/staging/typec/fusb302/fusb302.c | 28 ++++++++++++++++++++++------
>>   1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
>> index e1e08f57af99..c3bcc5484ade 100644
>> --- a/drivers/staging/typec/fusb302/fusb302.c
>> +++ b/drivers/staging/typec/fusb302/fusb302.c
>> @@ -1722,6 +1722,28 @@ static int fusb302_probe(struct i2c_client *client,
>>               return -EPROBE_DEFER;
>>       }
>> +    /*
>> +     * Devicetree platforms should get vbus from their dt-node.
>> +     * On ACPI platforms, we need to get the vbus by a system wide unique
>> +     * name, which is set in a device prop by the platform code.
>> +     */
>> +    if (device_property_read_string(dev, "fcs,vbus-regulator-name",
>> +                    &name) == 0) {
> 
> Another property to be documented and approved.

Again this is for kernel internal use on non-dt platforms only, so documenting
it in the devicetree bindings is not necessary.
> Also, isn't there a better way to get regulator names for dt- and non-dt systems ?
> This would apply to every driver supporting both and using regulators, which seems
> awkward.

While working on this I noticed that it is possible to add a regulator_match
table entry when registering a regulator, but that requires describing this
in regulator_init_data. Which would mean passing regulator_init_data from the
place where it is instantiated to where it gets registered, which would
mean passing a pointer through a device-property, given that this is purely kernel
internal that is possible, but not really how device-props are supposed to be used.

Also since the regulator-core only adds the mapping when registering the
regulator, this means that if we try to get the regulator before it has been
registered; and there is another regulator with the rather generic "vbus"
name then that will be returned instead.

Basically regulators are practically almost unused on x86 systems. I had to
add CONFIG_REGULATOR=y to my .config which is based on the Fedora 26 kernel
.config, so it has pretty much everything under the sun enabled. So it seems
that we are covering new ground here.

An alternative would be to not use the regulator subsys for this at all, but
it does seem the logical thing to use and using get-by-name is no different
then what we've doing for setting the the "fusb302-typec-source" psy as supplier
for the charger psy class device registered by the bq24190_charger driver.

TL;DR: It seems that on x86, at least for existing devices where we cannot
control the ACPI tables that getting things by name is the thing to do.

>> +        /*
>> +         * Use regulator_get_optional so that we can detect if we need
>> +         * to defer the probe rather then getting the dummy-regulator.
>> +         */
> 
> Wouldn't this apply to dt systems as well ?

No because there will be a property named "vbus-supply" in the fusb302
node containing a phandle to the regulator, if the regulator to which the phandle
points has not been registered yet regulator_get will automatically return
-EPROBE_DEFER because there is a "vbus-supply" property, only if there is
no such property at all will it return a dummy regulator.

>> +        chip->vbus = devm_regulator_get_optional(dev, name);
>> +        if (IS_ERR(chip->vbus)) {
>> +            ret = PTR_ERR(chip->vbus);
>> +            return (ret == -ENODEV) ? -EPROBE_DEFER : ret;
>> +        }
>> +    } else {
>> +        chip->vbus = devm_regulator_get(dev, "vbus");
>> +        if (IS_ERR(chip->vbus))
>> +            return PTR_ERR(chip->vbus);
>> +    }
>> +
> 
> You might also want to explain why you moved this code.

Right, I did that because it may fail with -EPROBE_DEFER and
I wanted to do that before the register_psy. But as I just
explained the old code could do that too, so I properly should
just put the register_psy later.

Regards,

Hans



>>       ret = tcpm_register_psy(chip->dev, &chip->tcpc_dev,
>>                   "fusb302-typec-source");
>>       if (ret < 0)
>> @@ -1739,12 +1761,6 @@ static int fusb302_probe(struct i2c_client *client,
>>       INIT_DELAYED_WORK(&chip->bc_lvl_handler, fusb302_bc_lvl_handler_work);
>>       init_tcpc_dev(&chip->tcpc_dev);
>> -    chip->vbus = devm_regulator_get(chip->dev, "vbus");
>> -    if (IS_ERR(chip->vbus)) {
>> -        ret = PTR_ERR(chip->vbus);
>> -        goto destroy_workqueue;
>> -    }
>> -
>>       if (client->irq) {
>>           chip->gpio_int_n_irq = client->irq;
>>       } else {
>>
> 

Powered by blists - more mailing lists