Re: [PATCH 3/8] pinctrl: msm: Add ability for drivers to supply a reserved GPIO list
On Fri, 07 Jun 2019, Bjorn Andersson wrote:
> On Wed 05 Jun 04:42 PDT 2019, Lee Jones wrote:
>
> > When booting MSM based platforms with Device Tree or some ACPI
> > implementations, it is possible to provide a list of reserved pins
> > via the 'gpio-reserved-ranges' and 'gpios' properties respectively.
> > However some ACPI tables are not populated with this information,
> > thus it has to come from a knowledgable device driver instead.
> >
> > Here we provide the MSM common driver with additional support to
> > parse this informtion and correctly populate the widely used
> > 'valid_mask'.
> >
> > Signed-off-by: Lee Jones
> > ---
> > drivers/pinctrl/qcom/pinctrl-msm.c | 18 ++
> > drivers/pinctrl/qcom/pinctrl-msm.h | 1 +
> > 2 files changed, 19 insertions(+)
> >
> > diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c
> > b/drivers/pinctrl/qcom/pinctrl-msm.c
> > index ee8119879c4c..3ac740b36508 100644
> > --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> > +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> > @@ -607,8 +607,23 @@ static int msm_gpio_init_valid_mask(struct gpio_chip
> > *chip)
> > int ret;
> > unsigned int len, i;
> > unsigned int max_gpios = pctrl->soc->ngpios;
> > + const int *reserved = pctrl->soc->reserved_gpios;
> > u16 *tmp;
> >
> > + /* Driver provided reserved list overrides DT and ACPI */
> > + if (reserved) {
> > + bitmap_fill(chip->valid_mask, max_gpios);
> > + for (i = 0; reserved[i] >= 0; i++) {
> > + if (i >= max_gpios || reserved[i] >= max_gpios) {
>
> reserved is a list of GPIOs to reserve, I don't see a reason to check
> if that list is longer than the number of GPIOs (i.e. the first half of
> the condition).
>
> It wouldn't make sense to be, but there's no logical issue with it and I
> had to read the conditional a few extra times to be sure what was going
> on.
If nothing else, it's an early hard stop in case someone forgot to
terminate the reserved array.
> Apart from that you have my
>
> Reviewed-by: Bjorn Andersson
Thanks Bjorn.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 3/8] pinctrl: msm: Add ability for drivers to supply a reserved GPIO list
On Fri, 07 Jun 2019, Bjorn Andersson wrote:
> On Fri 07 Jun 16:02 PDT 2019, Linus Walleij wrote:
>
> > On Wed, Jun 5, 2019 at 1:43 PM Lee Jones wrote:
> >
> > > When booting MSM based platforms with Device Tree or some ACPI
> > > implementations, it is possible to provide a list of reserved pins
> > > via the 'gpio-reserved-ranges' and 'gpios' properties respectively.
> > > However some ACPI tables are not populated with this information,
> > > thus it has to come from a knowledgable device driver instead.
> > >
> > > Here we provide the MSM common driver with additional support to
> > > parse this informtion and correctly populate the widely used
> > > 'valid_mask'.
> > >
> > > Signed-off-by: Lee Jones
> >
> > Exactly how we should use of the API, so if Björn can supply an
> > ACK to patches 3 and 4 I'm happy to apply them.
> >
> > Björn?
> >
>
> I'm waiting for a version that does not specify the reserved_gpios for
> struct msm_pinctrl_soc_data sdm845_pinctrl {}, as this would override
> the ability of getting these from DT.
>
> I haven't seen such revision yet, will review it once I find it.
Just testing it now. It should be on the list by the time you start.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 3/8] pinctrl: msm: Add ability for drivers to supply a reserved GPIO list
On Fri 07 Jun 16:02 PDT 2019, Linus Walleij wrote:
> On Wed, Jun 5, 2019 at 1:43 PM Lee Jones wrote:
>
> > When booting MSM based platforms with Device Tree or some ACPI
> > implementations, it is possible to provide a list of reserved pins
> > via the 'gpio-reserved-ranges' and 'gpios' properties respectively.
> > However some ACPI tables are not populated with this information,
> > thus it has to come from a knowledgable device driver instead.
> >
> > Here we provide the MSM common driver with additional support to
> > parse this informtion and correctly populate the widely used
> > 'valid_mask'.
> >
> > Signed-off-by: Lee Jones
>
> Exactly how we should use of the API, so if Björn can supply an
> ACK to patches 3 and 4 I'm happy to apply them.
>
> Björn?
>
I'm waiting for a version that does not specify the reserved_gpios for
struct msm_pinctrl_soc_data sdm845_pinctrl {}, as this would override
the ability of getting these from DT.
I haven't seen such revision yet, will review it once I find it.
Regards,
Bjorn
Re: [PATCH 3/8] pinctrl: msm: Add ability for drivers to supply a reserved GPIO list
On Wed 05 Jun 04:42 PDT 2019, Lee Jones wrote:
> When booting MSM based platforms with Device Tree or some ACPI
> implementations, it is possible to provide a list of reserved pins
> via the 'gpio-reserved-ranges' and 'gpios' properties respectively.
> However some ACPI tables are not populated with this information,
> thus it has to come from a knowledgable device driver instead.
>
> Here we provide the MSM common driver with additional support to
> parse this informtion and correctly populate the widely used
> 'valid_mask'.
>
> Signed-off-by: Lee Jones
> ---
> drivers/pinctrl/qcom/pinctrl-msm.c | 18 ++
> drivers/pinctrl/qcom/pinctrl-msm.h | 1 +
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c
> b/drivers/pinctrl/qcom/pinctrl-msm.c
> index ee8119879c4c..3ac740b36508 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.c
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.c
> @@ -607,8 +607,23 @@ static int msm_gpio_init_valid_mask(struct gpio_chip
> *chip)
> int ret;
> unsigned int len, i;
> unsigned int max_gpios = pctrl->soc->ngpios;
> + const int *reserved = pctrl->soc->reserved_gpios;
> u16 *tmp;
>
> + /* Driver provided reserved list overrides DT and ACPI */
> + if (reserved) {
> + bitmap_fill(chip->valid_mask, max_gpios);
> + for (i = 0; reserved[i] >= 0; i++) {
> + if (i >= max_gpios || reserved[i] >= max_gpios) {
reserved is a list of GPIOs to reserve, I don't see a reason to check
if that list is longer than the number of GPIOs (i.e. the first half of
the condition).
It wouldn't make sense to be, but there's no logical issue with it and I
had to read the conditional a few extra times to be sure what was going
on.
Apart from that you have my
Reviewed-by: Bjorn Andersson
Regards,
Bjorn
> + dev_err(pctrl->dev, "invalid list of reserved
> GPIOs\n");
> + return -EINVAL;
> + }
> + clear_bit(reserved[i], chip->valid_mask);
> + }
> +
> + return 0;
> + }
> +
> /* The number of GPIOs in the ACPI tables */
> len = ret = device_property_read_u16_array(pctrl->dev, "gpios", NULL,
> 0);
> @@ -964,6 +979,9 @@ static void msm_gpio_irq_handler(struct irq_desc *desc)
>
> static bool msm_gpio_needs_valid_mask(struct msm_pinctrl *pctrl)
> {
> + if (pctrl->soc->reserved_gpios)
> + return true;
> +
> return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
> }
>
> diff --git a/drivers/pinctrl/qcom/pinctrl-msm.h
> b/drivers/pinctrl/qcom/pinctrl-msm.h
> index c12048e54a6f..23b93ae92269 100644
> --- a/drivers/pinctrl/qcom/pinctrl-msm.h
> +++ b/drivers/pinctrl/qcom/pinctrl-msm.h
> @@ -121,6 +121,7 @@ struct msm_pinctrl_soc_data {
> bool pull_no_keeper;
> const char *const *tiles;
> unsigned int ntiles;
> + const int *reserved_gpios;
> };
>
> extern const struct dev_pm_ops msm_pinctrl_dev_pm_ops;
> --
> 2.17.1
>
Re: [PATCH 3/8] pinctrl: msm: Add ability for drivers to supply a reserved GPIO list
On Wed, Jun 5, 2019 at 1:43 PM Lee Jones wrote: > When booting MSM based platforms with Device Tree or some ACPI > implementations, it is possible to provide a list of reserved pins > via the 'gpio-reserved-ranges' and 'gpios' properties respectively. > However some ACPI tables are not populated with this information, > thus it has to come from a knowledgable device driver instead. > > Here we provide the MSM common driver with additional support to > parse this informtion and correctly populate the widely used > 'valid_mask'. > > Signed-off-by: Lee Jones Exactly how we should use of the API, so if Björn can supply an ACK to patches 3 and 4 I'm happy to apply them. Björn? Yours, Linus Walleij
