Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface
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
Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface
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
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
Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface
On Mon, Apr 25, 2016 at 04:04:50PM +0800, Lu Baolu wrote:
> + ret = device_property_read_string(dev, "gpio-name", &gpio_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
Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface
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", &gpio_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
Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface
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
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
Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface
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
