The driver-provided list of reserved gpios normally overrides any valid
ranges provided by the firmware (device tree and ACPI). When the driver
defines dummy pingroups by itself, it should mark these as invalid but
should not prevent the firmware from specifying more reserved gpios. Let
pinctrl drivers indicate that the reserved gpios list complements instead
of overrides other lists from firmware.
Signed-off-by: Richard Acayan <[email protected]>
---
drivers/pinctrl/qcom/pinctrl-msm.c | 5 +++--
drivers/pinctrl/qcom/pinctrl-msm.h | 4 ++++
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
index a2abfe987ab1..cea1d2af8c88 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -687,9 +687,10 @@ static int msm_gpio_init_valid_mask(struct gpio_chip *gc,
const int *reserved = pctrl->soc->reserved_gpios;
u16 *tmp;
- /* Driver provided reserved list overrides DT and ACPI */
+ /* Driver provided reserved list overrides DT and ACPI by default */
if (reserved) {
- bitmap_fill(valid_mask, ngpios);
+ if (!pctrl->soc->complement_fw_gpio_ranges)
+ bitmap_fill(valid_mask, ngpios);
for (i = 0; reserved[i] >= 0; i++) {
if (i >= ngpios || reserved[i] >= ngpios) {
dev_err(pctrl->dev, "invalid list of reserved GPIOs\n");
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
index dd0d949f7a9e..734fe7b2a472 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.h
+++ b/drivers/pinctrl/qcom/pinctrl-msm.h
@@ -128,6 +128,9 @@ struct msm_gpio_wakeirq_map {
* function number for eGPIO and any time we see that function
* number used we'll treat it as a request to mux away from
* our TLMM towards another owner.
+ * @complement_fw_gpio_ranges: If true, the reserved gpios list from the
+ * driver will not override the reserved gpios
+ * list from the firmware.
*/
struct msm_pinctrl_soc_data {
const struct pinctrl_pin_desc *pins;
@@ -146,6 +149,7 @@ struct msm_pinctrl_soc_data {
bool wakeirq_dual_edge_errata;
unsigned int gpio_func;
unsigned int egpio_func;
+ bool complement_fw_gpio_ranges;
};
extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
--
2.37.3
On Sat, Oct 01, 2022 at 05:07:24PM -0400, Richard Acayan wrote:
> The driver-provided list of reserved gpios normally overrides any valid
> ranges provided by the firmware (device tree and ACPI). When the driver
> defines dummy pingroups by itself, it should mark these as invalid but
> should not prevent the firmware from specifying more reserved gpios. Let
> pinctrl drivers indicate that the reserved gpios list complements instead
> of overrides other lists from firmware.
>
> Signed-off-by: Richard Acayan <[email protected]>
> ---
> drivers/pinctrl/qcom/pinctrl-msm.c | 5 +++--
> drivers/pinctrl/qcom/pinctrl-msm.h | 4 ++++
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> index a2abfe987ab1..cea1d2af8c88 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -687,9 +687,10 @@ static int msm_gpio_init_valid_mask(struct gpio_chip *gc,
> const int *reserved = pctrl->soc->reserved_gpios;
> u16 *tmp;
>
> - /* Driver provided reserved list overrides DT and ACPI */
> + /* Driver provided reserved list overrides DT and ACPI by default */
> if (reserved) {
> - bitmap_fill(valid_mask, ngpios);
> + if (!pctrl->soc->complement_fw_gpio_ranges)
reserved_gpios is only defined for ACPI drivers and afaict there's
nothing in the ACPI path that would modify the valid_mask after the
bitmap is being filled in gpiochip_allocate_mask().
If that's the case it seems reasonable that we can just drop the
bitmap_fill() here. But perhaps I'm missing something?
Regards,
Bjorn
> + bitmap_fill(valid_mask, ngpios);
> for (i = 0; reserved[i] >= 0; i++) {
> if (i >= ngpios || reserved[i] >= ngpios) {
> dev_err(pctrl->dev, "invalid list of reserved GPIOs\n");
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> index dd0d949f7a9e..734fe7b2a472 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -128,6 +128,9 @@ struct msm_gpio_wakeirq_map {
> * function number for eGPIO and any time we see that function
> * number used we'll treat it as a request to mux away from
> * our TLMM towards another owner.
> + * @complement_fw_gpio_ranges: If true, the reserved gpios list from the
> + * driver will not override the reserved gpios
> + * list from the firmware.
> */
> struct msm_pinctrl_soc_data {
> const struct pinctrl_pin_desc *pins;
> @@ -146,6 +149,7 @@ struct msm_pinctrl_soc_data {
> bool wakeirq_dual_edge_errata;
> unsigned int gpio_func;
> unsigned int egpio_func;
> + bool complement_fw_gpio_ranges;
> };
>
> extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> --
> 2.37.3
>
> On Sat, Oct 01, 2022 at 05:07:24PM -0400, Richard Acayan wrote:
> > The driver-provided list of reserved gpios normally overrides any valid
> > ranges provided by the firmware (device tree and ACPI). When the driver
> > defines dummy pingroups by itself, it should mark these as invalid but
> > should not prevent the firmware from specifying more reserved gpios. Let
> > pinctrl drivers indicate that the reserved gpios list complements instead
> > of overrides other lists from firmware.
> >
> > Signed-off-by: Richard Acayan <[email protected]>
> > ---
> > drivers/pinctrl/qcom/pinctrl-msm.c | 5 +++--
> > drivers/pinctrl/qcom/pinctrl-msm.h | 4 ++++
> > 2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index a2abfe987ab1..cea1d2af8c88 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -687,9 +687,10 @@ static int msm_gpio_init_valid_mask(struct gpio_chip *gc,
> > const int *reserved = pctrl->soc->reserved_gpios;
> > u16 *tmp;
> >
> > - /* Driver provided reserved list overrides DT and ACPI */
> > + /* Driver provided reserved list overrides DT and ACPI by default */
> > if (reserved) {
> > - bitmap_fill(valid_mask, ngpios);
> > + if (!pctrl->soc->complement_fw_gpio_ranges)
>
> reserved_gpios is only defined for ACPI drivers and afaict there's
> nothing in the ACPI path that would modify the valid_mask after the
> bitmap is being filled in gpiochip_allocate_mask().
>
> If that's the case it seems reasonable that we can just drop the
> bitmap_fill() here. But perhaps I'm missing something?
This was me trying to be as uninvasive to existing drivers as I could.
I just assumed that it was important because it was there already and
the comment suggested that it was what the original developer wanted.
When looking at it a bit closer, the bitmap_fill() call in
gpiochip_alloc_valid_mask() wasn't present at the original commit that
added this line. I'm guessing this means it's safe to remove, but just
in case (for the next version):
Cc: Lee Jones <[email protected]>
>
> Regards,
> Bjorn
>
> > + bitmap_fill(valid_mask, ngpios);
> > for (i = 0; reserved[i] >= 0; i++) {
> > if (i >= ngpios || reserved[i] >= ngpios) {
> > dev_err(pctrl->dev, "invalid list of reserved GPIOs\n");
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h b/drivers/pinctrl/qcom/pinctrl-msm.h
> > index dd0d949f7a9e..734fe7b2a472 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> > @@ -128,6 +128,9 @@ struct msm_gpio_wakeirq_map {
> > * function number for eGPIO and any time we see that function
> > * number used we'll treat it as a request to mux away from
> > * our TLMM towards another owner.
> > + * @complement_fw_gpio_ranges: If true, the reserved gpios list from the
> > + * driver will not override the reserved gpios
> > + * list from the firmware.
> > */
> > struct msm_pinctrl_soc_data {
> > const struct pinctrl_pin_desc *pins;
> > @@ -146,6 +149,7 @@ struct msm_pinctrl_soc_data {
> > bool wakeirq_dual_edge_errata;
> > unsigned int gpio_func;
> > unsigned int egpio_func;
> > + bool complement_fw_gpio_ranges;
> > };
> >
> > extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> > --
> > 2.37.3
> >