Re: [PATCH RFC] gpio: brcmstb: Properly account for empty banks

[Justin Chen <justin.chen@xxxxxxxxxxxx>]

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@xxxxxxxxx> 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@xxxxxxxxx> wrote:
>>> From: Justin Chen <justinpopo6@xxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html