| Message ID | 20171219023935.GA17456@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/18/17 8:39 PM, Stephen Boyd wrote: > Ah I missed that the u16 array can't be iterated through. Any > chance the ACPI tables can be changed to list pin ranges, like > <33 3>, <90 2>, to indicate that pins 33, 34, 35 and pins 90, 91 > are available? It's too late. Firmware is already shipping with the current layout. Unfortunately, there's no good peer review process for DSDs that don't have a DT equivalent. > That would allow us to put that into the core > pinctrl-msm.c file a little better and then only expose pins on > the gpiochip when call gpiochip_add_pin_range(). If we want to > support this in DT, I think we would have a DT property like > available-gpios = <33 3>, <90 2>, <100 34> that we can then > iterate through and add only these pins to the gpiochip. That's > better than a bitmap in DT and is still compressed somewhat. Keep in mind that all this ACPI junk is localized to pinctrl-qdf2xxx. pinctrl-msm does not define any new data structures, it just reuses the existing one. You can still define your DT properties any way you want in your client drivers. pinctrl-qdf2xxx is specific to the Centriq chips. > Without going all the way down into that path, here's my patch to > make your patch smaller, but perhaps we can just look for the > ACPI property or the DT property in the pinctrl-msm.c core and > then add pin ranges directly. Then this ACPI driver doesn't > really need to change besides for the ID update. We can expose > all the pins and offsets, etc. from the hardware driver but cut > out gpios in the core layer in a generic way. Ok, let me review this. I don't think there's any gain in moving the ACPI processing to pinctrl-msm, however.
On 12/18, Timur Tabi wrote: > On 12/18/17 8:39 PM, Stephen Boyd wrote: > >Ah I missed that the u16 array can't be iterated through. Any > >chance the ACPI tables can be changed to list pin ranges, like > ><33 3>, <90 2>, to indicate that pins 33, 34, 35 and pins 90, 91 > >are available? > > It's too late. Firmware is already shipping with the current > layout. Unfortunately, there's no good peer review process for DSDs > that don't have a DT equivalent. Alright! > > >That would allow us to put that into the core > >pinctrl-msm.c file a little better and then only expose pins on > >the gpiochip when call gpiochip_add_pin_range(). If we want to > >support this in DT, I think we would have a DT property like > >available-gpios = <33 3>, <90 2>, <100 34> that we can then > >iterate through and add only these pins to the gpiochip. That's > >better than a bitmap in DT and is still compressed somewhat. > > Keep in mind that all this ACPI junk is localized to > pinctrl-qdf2xxx. pinctrl-msm does not define any new data > structures, it just reuses the existing one. You can still define > your DT properties any way you want in your client drivers. > pinctrl-qdf2xxx is specific to the Centriq chips. Of course. > > >Without going all the way down into that path, here's my patch to > >make your patch smaller, but perhaps we can just look for the > >ACPI property or the DT property in the pinctrl-msm.c core and > >then add pin ranges directly. Then this ACPI driver doesn't > >really need to change besides for the ID update. We can expose > >all the pins and offsets, etc. from the hardware driver but cut > >out gpios in the core layer in a generic way. > > Ok, let me review this. I don't think there's any gain in moving > the ACPI processing to pinctrl-msm, however. > I will attempt to implement the DT part today. It may make the get_direction() revert irrelevant if the gpios aren't even exposed to gpiolib in the first place.
On 12/18/2017 08:39 PM, Stephen Boyd wrote: > + for (i = 0, j = 0; i < num_gpios; i++) { > pins[i].number = i; > - pins[i].name = names[i]; > + groups[i].pins = &pins[i].number; > + > + /* Only expose GPIOs that are available */ > + if (gpios && gpios[j] != i) > + continue; I don't know if I would say this is an improvement. For one thing, QCOM8001 systems are deprecated and don't really exist any more. At the time I originally wrote this patch, they were still in the wild, but they're all gone now. So it's no longer efficient to treat QCOM8001 as the default case. This means that the for-loop will iterate over the full range now, instead of the partial range that it does with my v10 patch. If I post another version of this patch, I'm just going to remove support for QCOM8001. If you want to avoid kmalloc'ing the GPIOs array, we can put it on the stack with a dynamic size, since it will be no more than MAX_GPIOS * 2 (i.e. 512) bytes in size. u16 gpios[avail_gpios]; It would be a little hackish since it needs to be defined at the beginning of a code block, so I would probably put into its own function, but I still fail to see what's wrong with using kmalloc to allocate that array for short-term use temporarily.
On 12/19, Timur Tabi wrote: > On 12/18/2017 08:39 PM, Stephen Boyd wrote: > >+ for (i = 0, j = 0; i < num_gpios; i++) { > > pins[i].number = i; > >- pins[i].name = names[i]; > >+ groups[i].pins = &pins[i].number; > >+ > >+ /* Only expose GPIOs that are available */ > >+ if (gpios && gpios[j] != i) > >+ continue; > > I don't know if I would say this is an improvement. For one thing, > QCOM8001 systems are deprecated and don't really exist any more. At > the time I originally wrote this patch, they were still in the wild, > but they're all gone now. So it's no longer efficient to treat > QCOM8001 as the default case. This means that the for-loop will > iterate over the full range now, instead of the partial range that > it does with my v10 patch. > > If I post another version of this patch, I'm just going to remove > support for QCOM8001. That sounds good too. The diff was really noisy because all the foo[i] became foo[gpio] which causes the diff to increase for no real purpose. My patch was rewriting that stuff so it doesn't come into the diff and we can concentrate on what's actually changing. We already iterate over the full range to fill in the two fields anyway, so I'm not sure what you're getting at with your for-loop comment. Seems like a micro-optimization on probe that probably isn't going to be noticed. > > If you want to avoid kmalloc'ing the GPIOs array, we can put it on > the stack with a dynamic size, since it will be no more than > MAX_GPIOS * 2 (i.e. 512) bytes in size. > > u16 gpios[avail_gpios]; > > It would be a little hackish since it needs to be defined at the > beginning of a code block, so I would probably put into its own > function, but I still fail to see what's wrong with using kmalloc to > allocate that array for short-term use temporarily. > Yeah I wouldn't do that. I'm not trying to avoid allocating the array anymore. Dynamically sized arrays on the stack are not a great idea in the kernel where we have limited stack sizes.
On 12/19/2017 02:30 PM, Stephen Boyd wrote: > Yeah I wouldn't do that. I'm not trying to avoid allocating the > array anymore. Dynamically sized arrays on the stack are not a > great idea in the kernel where we have limited stack sizes. So do you just want to see a v11 that drops support for QCOM8001?
On 12/18/2017 08:39 PM, Stephen Boyd wrote: > + if (gpios && gpios[j] != i) > + continue; ... > + j++; Doesn't this assume that the available GPIOs are listed in numerical order in the ACPI table? If so, then this patch won't work because that order is not guaranteed.
On 12/19, Timur Tabi wrote: > On 12/18/2017 08:39 PM, Stephen Boyd wrote: > >+ if (gpios && gpios[j] != i) > >+ continue; > ... > > + j++; > > Doesn't this assume that the available GPIOs are listed in numerical > order in the ACPI table? If so, then this patch won't work because > that order is not guaranteed. > Yes, I added a comment in the patch about that assumption. It's simple enough to sort the array in place with sort() though. I was looking at the gpiochip_add_pin_ranges() code to try and understand how to only expose pins that are supported as gpios into gpiolib, and then I looked at the history of these patches and wrote some code around pin ranges, got super confused and started thinking about adding gpiochips for each range (ugh), talked to Bjorn, and now I'm writing this mail. The approach of multiple ranges or chips looks like a dead-end that you've already gone down so let's not do that. The thing that I don't like about this patch is that we're modifying npins to indicate what gpios are available or not and then having a huge diff in this patch to do the 's/i/gpio/'. Ideally, everything would flow directly from the request callback and the SoC specific pinctrl driver would just tell the core code what all pins exist in hardware even if they're locked away and in use by something non-linux. That way, we don't have to rejig things in the SoC specific driver when the system configuration changes. I'm hoping we can do the valid mask part generically in the core pinctrl-msm.c file once so that things aren't spread around the SoC specific drivers and we solve ACPI and DT at the same time. We will want irq lines to be unallocated for things that aren't GPIOs, I'm not sure about ACPI and if it cares here, and we have a one to one mapping between irqs, GPIOs, and pinctrl pins with this hardware. Furthermore, we have the irq_valid_mask support in place already, so it seems that we can at least use the mask to be the one place where we indicate which pins are allowed to be used. And it seems like the simplest solution is to set the irq valid mask to be the GPIOs from the device property and then test that bitmask in the pinmux_ops structure's request callback so we cover both gpio (via the gpiochip_generic_request() -> pinmux_request_gpio() -> pin_request() path) and pinctrl (via the pin_request() path). Debugfs will need to test the mask too, but that should be simple. I believe you don't care about pin muxing, but it would make things work in both cases and will help if we want to limit access on platforms that use pin muxing. Let's resolve this by the end of this week. If this plan works we can have the revert patch for get_direction() and the pinctrl-msm.c patch to update the valid mask on gpiochip registration.
On 12/19/17 8:26 PM, Stephen Boyd wrote: > The thing that I don't like about this patch is that we're > modifying npins to indicate what gpios are available or not and > then having a huge diff in this patch to do the 's/i/gpio/'. Considering how small the driver is, I'm not sure if I'd say it's a "huge" diff. Frankly, I think this is a very elegant re-purposing of 'npins'. > Ideally, everything would flow directly from the request callback > and the SoC specific pinctrl driver would just tell the core code > what all pins exist in hardware even if they're locked away and > in use by something non-linux. So you want the request callback to propagated to the SOC driver? I guess that could work. > That way, we don't have to rejig > things in the SoC specific driver when the system configuration > changes. I'm hoping we can do the valid mask part generically in > the core pinctrl-msm.c file once so that things aren't spread > around the SoC specific drivers and we solve ACPI and DT at the > same time. Well, now I'm confused. First I thought you wanted to move the valid check into pinctrl-qdf2xxx, but now you say you want it done in pinctrl-msm, but isn't that what my patches are doing now? > We will want irq lines to be unallocated for things that aren't > GPIOs, I'm not sure about ACPI and if it cares here, and we have > a one to one mapping between irqs, GPIOs, and pinctrl pins with > this hardware. If the pin can't be requested, doesn't that take care of IRQ lines automatically? I don't touch the irq_valid_mask code. > Furthermore, we have the irq_valid_mask support in > place already, so it seems that we can at least use the mask to > be the one place where we indicate which pins are allowed to be > used. Well, I really didn't want to do that. Keep in mind that the root problem is getting pinctrl-qdf2xxx to be able to tell pinctrl-msm what pins are valid. That is the bulk of my code. I'm understanding you less and less the more I read. >And it seems like the simplest solution is to set the irq > valid mask to be the GPIOs from the device property and then test > that bitmask in the pinmux_ops structure's request callback so we > cover both gpio (via the gpiochip_generic_request() -> > pinmux_request_gpio() -> pin_request() path) and pinctrl (via the > pin_request() path). I do not understand that. To be honest, I think I already have the simplest solution, at least for ACPI. I don't really want to complicate my patches to support DT, since I don't really know what the DT-specific problems are. > Debugfs will need to test the mask too, but > that should be simple. I believe you don't care about pin muxing, > but it would make things work in both cases and will help if we > want to limit access on platforms that use pin muxing. I don't care about pin muxing, but my patches already take care of debugfs. > Let's resolve this by the end of this week. If this plan works we > can have the revert patch for get_direction() and the > pinctrl-msm.c patch to update the valid mask on gpiochip > registration. Frankly, I thought I had everything resolved already, and it sounds like you want me to start over from scratch anyway.
diff --git a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c index bb3ce5c3e18b..2ca2f40719b3 100644 --- a/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c +++ b/drivers/pinctrl/qcom/pinctrl-qdf2xxx.c @@ -38,45 +38,107 @@ static struct msm_pinctrl_soc_data qdf2xxx_pinctrl; /* maximum size of each gpio name (enough room for "gpioXXX" + null) */ #define NAME_SIZE 8 +enum { + QDF2XXX_V1, + QDF2XXX_V2, +}; + static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) { + const struct acpi_device_id *id; struct pinctrl_pin_desc *pins; struct msm_pingroup *groups; char (*names)[NAME_SIZE]; - unsigned int i; + unsigned int i, j; u32 num_gpios; + unsigned int avail_gpios; /* The number of GPIOs we support */ + u16 *gpios = NULL; /* An array of supported GPIOs */ int ret; /* Query the number of GPIOs from ACPI */ ret = device_property_read_u32(&pdev->dev, "num-gpios", &num_gpios); if (ret < 0) { - dev_warn(&pdev->dev, "missing num-gpios property\n"); + dev_err(&pdev->dev, "missing 'num-gpios' property\n"); return ret; } - if (!num_gpios || num_gpios > MAX_GPIOS) { - dev_warn(&pdev->dev, "invalid num-gpios property\n"); + dev_err(&pdev->dev, "invalid 'num-gpios' property\n"); return -ENODEV; } + /* + * The QCOM8001 HID contains only the number of GPIOs, and assumes + * that all of them are available. avail_gpios is the same as num_gpios. + * + * The QCOM8002 HID introduces the 'gpios' DSD, which lists + * specific GPIOs that the driver is allowed to access. + * + * The make the common code simpler, in both cases we create an + * array of GPIOs that are accessible. So for QCOM8001, that would + * be all of the GPIOs. + */ + id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev); + + if (id->driver_data == QDF2XXX_V1) { + avail_gpios = num_gpios; + } else { + /* The number of GPIOs in the approved list */ + ret = device_property_read_u16_array(&pdev->dev, "gpios", + NULL, 0); + if (ret < 0) { + dev_err(&pdev->dev, "missing 'gpios' property\n"); + return ret; + } + /* + * The number of available GPIOs should be non-zero, and no + * more than the total number of GPIOS. + */ + if (!ret || ret > num_gpios) { + dev_err(&pdev->dev, "invalid 'gpios' property\n"); + return -ENODEV; + } + avail_gpios = ret; + + gpios = devm_kmalloc_array(&pdev->dev, avail_gpios, + sizeof(gpios[0]), GFP_KERNEL); + if (!gpios) + return -ENOMEM; + + /* Assume the array of available GPIOs is sorted */ + ret = device_property_read_u16_array(&pdev->dev, "gpios", gpios, + avail_gpios); + if (ret < 0) { + dev_err(&pdev->dev, "could not read list of GPIOs\n"); + return ret; + } + } + pins = devm_kcalloc(&pdev->dev, num_gpios, sizeof(struct pinctrl_pin_desc), GFP_KERNEL); groups = devm_kcalloc(&pdev->dev, num_gpios, sizeof(struct msm_pingroup), GFP_KERNEL); - names = devm_kcalloc(&pdev->dev, num_gpios, NAME_SIZE, GFP_KERNEL); + names = devm_kcalloc(&pdev->dev, avail_gpios, NAME_SIZE, GFP_KERNEL); if (!pins || !groups || !names) return -ENOMEM; - for (i = 0; i < num_gpios; i++) { - snprintf(names[i], NAME_SIZE, "gpio%u", i); - + /* + * Initialize the array. GPIOs not listed in the 'gpios' bitmap + * still need a number, but nothing else. + */ + for (i = 0, j = 0; i < num_gpios; i++) { pins[i].number = i; - pins[i].name = names[i]; + groups[i].pins = &pins[i].number; + + /* Only expose GPIOs that are available */ + if (gpios && gpios[j] != i) + continue; groups[i].npins = 1; - groups[i].name = names[i]; - groups[i].pins = &pins[i].number; + snprintf(names[j], NAME_SIZE, "gpio%u", i); + pins[i].name = names[j]; + groups[i].name = names[j]; + j++; groups[i].ctl_reg = 0x10000 * i; groups[i].io_reg = 0x04 + 0x10000 * i; @@ -100,6 +162,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) groups[i].intr_detection_width = 2; } + devm_kfree(&pdev->dev, gpios); + qdf2xxx_pinctrl.pins = pins; qdf2xxx_pinctrl.groups = groups; qdf2xxx_pinctrl.npins = num_gpios; @@ -110,7 +174,8 @@ static int qdf2xxx_pinctrl_probe(struct platform_device *pdev) } static const struct acpi_device_id qdf2xxx_acpi_ids[] = { - {"QCOM8001"}, + {"QCOM8001", QDF2XXX_V1}, + {"QCOM8002", QDF2XXX_V2}, {}, }; MODULE_DEVICE_TABLE(acpi, qdf2xxx_acpi_ids);