Hi Geert,
> Subject: Re: [PATCH v2 1/5] gpio: rcar: Add GPIO hole support
> On Thu, Aug 2, 2018 at 4:17 PM Biju Das <[email protected]> wrote:
> > GPIO hole is present in RZ/G1C SoC. Valid GPIO pins on bank3 are in
> > the range GP3_0 to GP3_16 and GP3_27 to GP3_29. The GPIO pins
> between
> > GP3_17 to GP3_26 are unused. Add support for handling unused GPIO's.
> >
> > Signed-off-by: Biju Das <[email protected]>
> > Reviewed-by: Fabrizio Castro <[email protected]>
> > u32 intmsk;
> > + u32 gpiomsk;
>
> I think you can do without adding this variable (see below).
OK.
> > };
> >
> > struct gpio_rcar_priv {
> > @@ -252,6 +253,9 @@ static int gpio_rcar_request(struct gpio_chip *chip,
> unsigned offset)
> > struct gpio_rcar_priv *p = gpiochip_get_data(chip);
> > int error;
> >
> > + if (!gpiochip_line_is_valid(chip, offset))
> > + return -EINVAL;
> > +
>
> Perhaps this check should be added to gpiod_request_commit(), so not all
> drivers (currently drivers/pinctrl/qcom/pinctrl-msm.c) have to check it
> themselves?
Yes I agree. Linus, what do you think?
> > error = pm_runtime_get_sync(&p->pdev->dev);
> > if (error < 0)
> > return error;
> > @@ -313,6 +317,9 @@ static void gpio_rcar_set_multiple(struct gpio_chip
> *chip, unsigned long *mask,
> > unsigned long flags;
> > u32 val, bankmask;
> >
> > + if (chip->valid_mask && (mask[0] & p->bank_info.gpiomsk))
> > + return;
> > +
>
> You can use chip->valid_mask[0] instead of p->bank_info.gpiomsk.
> However, instead of returning, I would just ignore any invalid bits set.
> Bits > chip->ngpio are already ignored below...
>
> > bankmask = mask[0] & GENMASK(chip->ngpio - 1, 0);
>
> ... so just add:
>
> if (chip->valid_mask)
> bankmask &= chip->valid_mask[0];
OK. Will do this.
> However, if you force gpiochip->need_valid_mask = true, you can just use
>
> bankmask = mask[0] & chip->valid_mask[0];
> unconditionally.
>
> > if (!bankmask)
> > return;
> > @@ -399,7 +406,8 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv
> *p, unsigned int *npins)
> > struct device_node *np = p->pdev->dev.of_node;
> > const struct gpio_rcar_info *info;
> > struct of_phandle_args args;
> > - int ret;
> > + int ret, len, i;
> > + u32 start, count;
> >
> > info = of_device_get_match_data(&p->pdev->dev);
> >
> > @@ -414,6 +422,22 @@ static int gpio_rcar_parse_dt(struct gpio_rcar_priv
> *p, unsigned int *npins)
> > *npins = RCAR_MAX_GPIO_PER_BANK;
> > }
> >
> > + p->bank_info.gpiomsk = 0;
> > + len = of_property_count_u32_elems(np, "gpio-reserved-ranges");
> > + if (len < 0 || len % 2 != 0)
> > + return 0;
> > +
> > + for (i = 0; i < len; i += 2) {
> > + of_property_read_u32_index(np, "gpio-reserved-ranges",
> > + i, &start);
> > + of_property_read_u32_index(np, "gpio-reserved-ranges",
> > + i + 1, &count);
> > + if (start >= *npins || start + count >= *npins)
> > + continue;
> > +
> > + p->bank_info.gpiomsk = GENMASK(start + count - 1, start);
> > + }
>
> of_gpiochip_init_valid_mask() already does the same, for chip->valid_mask.
Currently we are not setting "gpio_chip.of_node" variable in the driver.
So the below function in gpiochip_init_valid_mask() will return negative
value and won't allocate memory for valid_mask.
size = of_property_count_u32_elems(np, "gpio-reserved-ranges");
We need to either set " of_node" variable or parse "gpio-reserved-ranges"
and set " need_valid_mask=true ;"
so that gpiochip_init_valid_mask() will allocate the valid_mask.
> > +
> > return 0;
> > }
> >
> > @@ -471,6 +495,7 @@ static int gpio_rcar_probe(struct platform_device
> *pdev)
> > gpio_chip->owner = THIS_MODULE;
> > gpio_chip->base = -1;
> > gpio_chip->ngpio = npins;
> > + gpio_chip->need_valid_mask = p->bank_info.gpiomsk ? true :
> > + false;
>
> gpiochip_init_valid_mask() already sets gpio_chip->need_valid_mask to true
> if "gpio-reserved-ranges" is detected.
The plan is to set "of_node" variable in driver. So that
gpiochip_init_valid_mask() sets gpio_chip->need_valid_mask to true.
What is your opinion on this?
> However, setting it to true unconditionally allow to simplify
> gpio_rcar_set_multiple()
> and gpio_rcar_resume().
>
> >
> > irq_chip = &p->irq_chip;
> > irq_chip->name = name;
> > @@ -551,6 +576,9 @@ static int gpio_rcar_resume(struct device *dev)
> >
> > for (offset = 0; offset < p->gpio_chip.ngpio; offset++) {
> > mask = BIT(offset);
> > + if (p->gpio_chip.valid_mask && (mask &
> > p->bank_info.gpiomsk))
> > + continue;
> > +
>
> Just do "if (!gpiochip_line_is_valid(chip, offset)) continue;" at the top of
> the
> loop?
Ok. Will do this.
> However, if you force gpiochip->need_valid_mask = true, you can just
> replace the hand-coded for loop by for_each_set_bit().
>
> > /* I/O pin */
> > if (!(p->bank_info.iointsel & mask)) {
> > if (p->bank_info.inoutsel & mask)
Regards,
Biju
Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End,
Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered
No. 04586709.
