| Message ID | 1531948342-23678-1-git-send-email-justinpopo6@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] gpio: brcmstb: Properly account for empty banks | expand |
Hi Justin, I got this same patch 3 times in a row in one day, not sure why. :-) On Wed, Jul 18, 2018 at 2:12 PM, <justinpopo6@gmail.com> wrote: > From: Justin Chen <justinpopo6@gmail.com> > > On some chips we have empty banks or a hole in the register space. > The driver assumes all banks are consecutive and the same size. It does assume both of these, but if this workaround works, then only the former is a problem, right? Consider omitting the mention of it being the same size if it's not relevant for the change. > To work around this, we create a fake bank where there is a hole > and keep it out of the linked list so it won't be called or initialized The idea seems OK, and it looks like this should already even be handled properly by brcmstb_gpio_irq_map(). I think the main issue is informational; the info line printed at the end of probe would give the appearance of the driver registering more GPIOs than it actually does. Please update that to be clearer about what is actually registered. Best regards, Gregory -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 6, 2018 at 6:18 PM, Gregory Fong <gregory.0xf0@gmail.com> wrote: > Hi Justin, > > I got this same patch 3 times in a row in one day, not sure why. :-) Good to hear from you Gregory. :) Sorry mail server issue hehe > > On Wed, Jul 18, 2018 at 2:12 PM, <justinpopo6@gmail.com> wrote: >> From: Justin Chen <justinpopo6@gmail.com> >> >> On some chips we have empty banks or a hole in the register space. >> The driver assumes all banks are consecutive and the same size. > > It does assume both of these, but if this workaround works, then only > the former is a problem, right? Consider omitting the mention of it > being the same size if it's not relevant for the change. Oh ok, I see what you are getting at here. I think I might just rephrase the commit message as a whole. Now that I am reading it again, it is a bit confusing. > >> To work around this, we create a fake bank where there is a hole >> and keep it out of the linked list so it won't be called or initialized > > The idea seems OK, and it looks like this should already even be > handled properly by brcmstb_gpio_irq_map(). I think the main issue is > informational; the info line printed at the end of probe would give > the appearance of the driver registering more GPIOs than it actually > does. Please update that to be clearer about what is actually > registered. Without the patch we actually fail at brcmstb_gpio_sanity_check_banks because of if (res_num_banks != num_banks) { dev_err(dev, "Mismatch in banks: res had %d, bank-widths had %d\n", res_num_banks, num_banks); return -EINVAL; } ... Ok I'll see what I can do. The information is already misleading because not all banks use all 32 gpios. Thanks, Justin > > Best regards, > Gregory -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Gregory, >> The idea seems OK, and it looks like this should already even be >> handled properly by brcmstb_gpio_irq_map(). I think the main issue is >> informational; the info line printed at the end of probe would give >> the appearance of the driver registering more GPIOs than it actually >> does. Please update that to be clearer about what is actually >> registered. Was looking more into this. We already have the information above at dev_dbg(dev, "bank=%d, base=%d, ngpio=%d, width=%d\n", bank->id, gc->base, gc->ngpio, bank->width); albeit only prints with dbg. Would removing the misleading information make sense here? - dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n", - num_banks, priv->gpio_base, gpio_base - 1); + dev_info(dev, "Registered %d banks\n", num_banks); Either that or something like this... "Registered X banks, (GPIO(s): 160-180, 192-208, 224-229 ...)" Which I feel is unnecessarily complex. Thanks, Justin On 08/07/2018 11:19 AM, Justin Chen wrote: > On Mon, Aug 6, 2018 at 6:18 PM, Gregory Fong <gregory.0xf0@gmail.com> wrote: >> Hi Justin, >> >> I got this same patch 3 times in a row in one day, not sure why. :-) > Good to hear from you Gregory. :) Sorry mail server issue hehe >> >> On Wed, Jul 18, 2018 at 2:12 PM, <justinpopo6@gmail.com> wrote: >>> From: Justin Chen <justinpopo6@gmail.com> >>> >>> On some chips we have empty banks or a hole in the register space. >>> The driver assumes all banks are consecutive and the same size. >> >> It does assume both of these, but if this workaround works, then only >> the former is a problem, right? Consider omitting the mention of it >> being the same size if it's not relevant for the change. > Oh ok, I see what you are getting at here. I think I might just rephrase the > commit message as a whole. Now that I am reading it again, it is a bit > confusing. >> >>> To work around this, we create a fake bank where there is a hole >>> and keep it out of the linked list so it won't be called or initialized >> >> The idea seems OK, and it looks like this should already even be >> handled properly by brcmstb_gpio_irq_map(). I think the main issue is >> informational; the info line printed at the end of probe would give >> the appearance of the driver registering more GPIOs than it actually >> does. Please update that to be clearer about what is actually >> registered. > Without the patch we actually fail at brcmstb_gpio_sanity_check_banks because of > if (res_num_banks != num_banks) { > dev_err(dev, "Mismatch in banks: res had %d, > bank-widths had %d\n", > res_num_banks, num_banks); > return -EINVAL; > } ... > Ok I'll see what I can do. The information is already misleading > because not all banks use all 32 gpios. > > Thanks, > Justin >> >> Best regards, >> Gregory -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c index 99247f6..9daeda7 100644 --- a/drivers/gpio/gpio-brcmstb.c +++ b/drivers/gpio/gpio-brcmstb.c @@ -603,6 +603,18 @@ static int brcmstb_gpio_probe(struct platform_device *pdev) struct brcmstb_gpio_bank *bank; struct gpio_chip *gc; + /* + * If bank_width is 0, then there is an empty bank in the + * register block. Special handling for this case. + */ + if (bank_width == 0) { + dev_dbg(dev, "Fake bank %d (GPIO(s): %d-%d)\n", + num_banks, gpio_base, gpio_base + MAX_GPIO_PER_BANK); + num_banks++; + gpio_base += MAX_GPIO_PER_BANK; + continue; + } + bank = devm_kzalloc(dev, sizeof(*bank), GFP_KERNEL); if (!bank) { err = -ENOMEM; @@ -611,7 +623,7 @@ static int brcmstb_gpio_probe(struct platform_device *pdev) bank->parent_priv = priv; bank->id = num_banks; - if (bank_width <= 0 || bank_width > MAX_GPIO_PER_BANK) { + if (bank_width < 0 || bank_width > MAX_GPIO_PER_BANK) { dev_err(dev, "Invalid bank width %d\n", bank_width); err = -EINVAL; goto fail;