Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Lu Baolu]

Hi,

On 04/29/2016 01:15 AM, Mark Brown wrote:
> On Thu, Apr 28, 2016 at 01:55:35PM +0800, Lu Baolu wrote:
>
>> How about below code?
>> +   gpiod = gpiod_get(dev, "vbus_en", GPIOD_ASIS);
>> +   if (IS_ERR(gpiod))
>> +   return PTR_ERR(gpiod);
>> +
>> +   config->gpio = desc_to_gpio(gpiod);
>> +   config->enable_high = device_property_read_bool(dev,
>> +   
>> "enable-active-high");
>> +   gpiod_put(gpiod);
>
> That looks reasonable, though if you use "gpio" as the name like DT you
> could keep the code the same which would be even better?  Not super
> crticial though.

Systems might implement the name mapping in BIOS by implementing _DSD.
The "gpio" looks a little generic. I'd like to use a function specific name.

I will send a refreshed patch later.

Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Thu, Apr 28, 2016 at 01:55:35PM +0800, Lu Baolu wrote:

> How about below code?

> +   gpiod = gpiod_get(dev, "vbus_en", GPIOD_ASIS);
> +   if (IS_ERR(gpiod))
> +   return PTR_ERR(gpiod);
> +
> +   config->gpio = desc_to_gpio(gpiod);
> +   config->enable_high = device_property_read_bool(dev,
> +   "enable-active-high");
> +   gpiod_put(gpiod);


That looks reasonable, though if you use "gpio" as the name like DT you
could keep the code the same which would be even better?  Not super
crticial though.


signature.asc
Description: PGP signature

Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Lu Baolu]

Hi,

On 04/27/2016 08:33 PM, Mark Brown wrote:
> On Wed, Apr 27, 2016 at 09:54:10AM +0800, Lu Baolu wrote:
>
>> Please refer to Documentation/acpi/gpio-properties.txt.
> That's not visibly what your driver is doing, that is also recommending
> using a static name which is what I'm asking for.

Yes, I agree that we should use a static name.

>
>>> Why does the device care?It's requesting the GPIO in
>>> its own context and it's only requesting one GPIO, with DT we're just
>>> always calling the GPIO "gpio" which works fine.
>> This driver is not bound to an ACPI device node directly. It's a child
>> of a mfd device, which is corresponding to a real ACPI device node.
> If it's the child of a MFD it's got an ACPI device, the ACPI device is
> the parent.It should use the parent device or the parent should map
> the GPIO through to the child as many other MFDs do, the whole concept
> of a MFD is a Linux internal implementation detail.

Yes. The mapping of GPIO is done in the parent. And the parent
passes the GPIO by setting ACPI companion to this device (done
in mfd internal). This driver is able to get the gpio descriptor with
a static name.

How about below code?

+   gpiod = gpiod_get(dev, "vbus_en", GPIOD_ASIS);
+   if (IS_ERR(gpiod))
+   return PTR_ERR(gpiod);
+
+   config->gpio = desc_to_gpio(gpiod);
+   config->enable_high = device_property_read_bool(dev,
+   "enable-active-high");
+   gpiod_put(gpiod);

Best regards,
Lu Baolu

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Wed, Apr 27, 2016 at 09:54:10AM +0800, Lu Baolu wrote:

> Please refer to Documentation/acpi/gpio-properties.txt.

That's not visibly what your driver is doing, that is also recommending
using a static name which is what I'm asking for.

> > Why does the device care?It's requesting the GPIO in
> > its own context and it's only requesting one GPIO, with DT we're just
> > always calling the GPIO "gpio" which works fine.

> This driver is not bound to an ACPI device node directly. It's a child
> of a mfd device, which is corresponding to a real ACPI device node.

If it's the child of a MFD it's got an ACPI device, the ACPI device is
the parent.  It should use the parent device or the parent should map
the GPIO through to the child as many other MFDs do, the whole concept
of a MFD is a Linux internal implementation detail.


signature.asc
Description: PGP signature

Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Lu Baolu]

Hi,

On 04/26/2016 06:23 PM, Mark Brown wrote:
> On Tue, Apr 26, 2016 at 10:24:56AM +0800, Lu Baolu wrote:
>
>> The GPIO name might be different in different use cases. For my case,
>> it is "vbus_en", but other cases should use the different name.
>> On ACPI compatible platforms, GPIO resources are reported via ACPI
>> tables and (devm_)gpiod_get() hides the APCI complexity and returns
>> the gpiod according to "gpio_name".
> That's labelling that you might want to do on the supplier side or at
> system level.

The labeling is done at firmware level (ACPI 5.1). It uses _DSD
configuration object to give names to GPIOs. There are systems
which don't contain _DSD. On those platforms, Linux kernel
could do this instead.

Please refer to Documentation/acpi/gpio-properties.txt.

> Why does the device care?It's requesting the GPIO in
> its own context and it's only requesting one GPIO, with DT we're just
> always calling the GPIO "gpio" which works fine.

This driver is not bound to an ACPI device node directly. It's a child
of a mfd device, which is corresponding to a real ACPI device node.

I agree with you that we should not retrieve gpio name from the
device provider. Driver should have the knowledge of the gpio name.
(Please correct me if I didn't understand your point right. :-) )


Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Tue, Apr 26, 2016 at 10:24:56AM +0800, Lu Baolu wrote:

> The GPIO name might be different in different use cases. For my case,
> it is "vbus_en", but other cases should use the different name.

> On ACPI compatible platforms, GPIO resources are reported via ACPI
> tables and (devm_)gpiod_get() hides the APCI complexity and returns
> the gpiod according to "gpio_name".

That's labelling that you might want to do on the supplier side or at
system level.  Why does the device care?  It's requesting the GPIO in
its own context and it's only requesting one GPIO, with DT we're just
always calling the GPIO "gpio" which works fine.


signature.asc
Description: PGP signature

Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Lu Baolu]

Hi Mark,

On 04/26/2016 01:30 AM, Mark Brown wrote:
> On Mon, Apr 25, 2016 at 04:04:50PM +0800, Lu Baolu wrote:
>
>> +ret = device_property_read_string(dev, "gpio-name", _name);
>> +if (!ret) {
>> +gpiod = gpiod_get(dev, gpio_name, GPIOD_ASIS);
>> +if (!IS_ERR(gpiod)) {
> This doesn't look like it's a standard ACPI binding for GPIOs, why are
> we using a property to get the GPIO noame?

The GPIO name might be different in different use cases. For my case,
it is "vbus_en", but other cases should use the different name.

On ACPI compatible platforms, GPIO resources are reported via ACPI
tables and (devm_)gpiod_get() hides the APCI complexity and returns
the gpiod according to "gpio_name".

Best regards,
Lu Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Mon, Apr 25, 2016 at 04:04:50PM +0800, Lu Baolu wrote:

> + ret = device_property_read_string(dev, "gpio-name", _name);
> + if (!ret) {
> + gpiod = gpiod_get(dev, gpio_name, GPIOD_ASIS);
> + if (!IS_ERR(gpiod)) {

This doesn't look like it's a standard ACPI binding for GPIOs, why are
we using a property to get the GPIO noame?


signature.asc
Description: PGP signature

[PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Lu Baolu]

Add support to retrieve fixed voltage configure information through
ACPI interface. This is needed for Intel Bay Trail devices, where a
GPIO is used to control the USB vbus.

Signed-off-by: Lu Baolu 
---
 drivers/regulator/fixed.c | 48 +++
 1 file changed, 48 insertions(+)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index e6f376f..4d0cc84 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -30,6 +30,9 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
 
 struct fixed_voltage_data {
struct regulator_desc desc;
@@ -104,6 +107,46 @@ of_get_fixed_voltage_config(struct device *dev,
return config;
 }
 
+/**
+ * acpi_get_fixed_voltage_config - extract fixed_voltage_config structure info
+ * @dev: device requesting for fixed_voltage_config
+ * @desc: regulator description
+ *
+ * Populates fixed_voltage_config structure by extracting data through ACPI
+ * interface, returns a pointer to the populated structure of NULL if memory
+ * alloc fails.
+ */
+static struct fixed_voltage_config *
+acpi_get_fixed_voltage_config(struct device *dev,
+ const struct regulator_desc *desc)
+{
+   struct fixed_voltage_config *config;
+   const char *supply_name, *gpio_name;
+   struct gpio_desc *gpiod;
+   int ret;
+
+   config = devm_kzalloc(dev, sizeof(*config), GFP_KERNEL);
+   if (!config)
+   return ERR_PTR(-ENOMEM);
+
+   ret = device_property_read_string(dev, "supply-name", _name);
+   if (!ret)
+   config->supply_name = supply_name;
+
+   ret = device_property_read_string(dev, "gpio-name", _name);
+   if (!ret) {
+   gpiod = gpiod_get(dev, gpio_name, GPIOD_ASIS);
+   if (!IS_ERR(gpiod)) {
+   config->gpio = desc_to_gpio(gpiod);
+   config->enable_high = device_property_read_bool(dev,
+   "enable-active-high");
+   gpiod_put(gpiod);
+   }
+   }
+
+   return config;
+}
+
 static struct regulator_ops fixed_voltage_ops = {
 };
 
@@ -124,6 +167,11 @@ static int reg_fixed_voltage_probe(struct platform_device 
*pdev)
 >desc);
if (IS_ERR(config))
return PTR_ERR(config);
+   } else if (ACPI_HANDLE(>dev)) {
+   config = acpi_get_fixed_voltage_config(>dev,
+  >desc);
+   if (IS_ERR(config))
+   return PTR_ERR(config);
} else {
config = dev_get_platdata(>dev);
}
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html