Re: [PATCH][WAS:bcmai,axi] bcma: add Broadcom specific AMBA bus driver

[RafaÅ MiÅecki]

2011/5/7 George Kashperko <george@xxxxxxxxxxx>:
>
>> 2011/5/7 George Kashperko <george@xxxxxxxxxxx>:
>> >
>> >> 2011/5/6 RafaÅ MiÅecki <zajec5@xxxxxxxxx>:
>> >> > 2011/5/6 Arnd Bergmann <arnd@xxxxxxxx>:
>> >> >>> +const char *bcma_device_name(u16 coreid)
>> >> >>> +{
>> >> >>> + Â Â switch (coreid) {
>> >> >>> + Â Â case BCMA_CORE_OOB_ROUTER:
>> >> >>> + Â Â Â Â Â Â return "OOB Router";
>> >> >>> + Â Â case BCMA_CORE_INVALID:
>> >> >>> + Â Â Â Â Â Â return "Invalid";
>> >> >>> + Â Â case BCMA_CORE_CHIPCOMMON:
>> >> >>> + Â Â Â Â Â Â return "ChipCommon";
>> >> >>> + Â Â case BCMA_CORE_ILINE20:
>> >> >>> + Â Â Â Â Â Â return "ILine 20";
>> >> >>
>> >> >> It's better to make that a data structure than a switch() statement,
>> >> >> both from readability and efficiency aspects.
>> >> >
>> >> > Well, maybe. We call it only once, at init time. In any case we're
>> >> > still waiting for Broadcom to clarify which cores are really used for
>> >> > BCMA.
>> >>
>> >> Arnd: did you have a look at defines at all?
>> >>
>> >> Most of the defines have values in range 0x800 â 0x837. Converting
>> >> this to array means loosing 0x800 u16 entries. We can not use 0x800
>> >> offset, because there are also some defined between 0x000 and 0x800:
>> >> #define BCMA_CORE_OOB_ROUTER Â Â Â Â Â 0x367 Â /* Out of band */
>> >> #define BCMA_CORE_INVALID Â Â Â Â Â Â Â0x700
>> >>
>> >> Oh and there is still:
>> >> #define BCMA_CORE_DEFAULT Â Â Â Â Â Â Â0xFFF
>> >> we could want to include. Then we would loose additional (0xFFF -
>> >> 0x837) u16 entries in array.
>> > What is the purpose for bcma_device_name in bus driver code ? Why not
>> > define const char *name in struct bcma_driver and let driver writers
>> > supply kernel with knowledge on new cores' names rather than hard type
>> > those into the bus code ?
>>
>> The purpose is ridiculously trivial. Print user-friendly names on
>> scanning. Why not do that?
> Output like
> Core 0: ChipCommon (id 0x800, rev 18, vendor 0x14e4)
> and
> Core 0: id 0x800, rev 18, vendor 0x14e4
> both give to 99% of linux systems' end-users exactly the same consistent
> information. Its more than enough for diagnostic purposes (I guess
> scanning code does outputs this for diagnostic purposes by those less
> than 1% of people who are aware wth is actually that ChipCommon is, not
> to be just user friendly?).

Really, what's wrong with that? Does it kill anyone's pet we print
this? We also do:
pr_err("Scanning failed because of wrong CID\n");
return -1;
While we could drop pr_err. Why to do this? Advanced used can always
check what -1 means.

I just like this. I find situations when it's useful, while I don't
see real negatives. I want to keep this. Can we leave this that way?
Unless someone finds some really bad effects of this?


> For user firendly output you still can keep the name of the core in
> dedicated driver.

It won't work in case of core with no driver (not supported), driver
not compiled (someone didn't know it's needed for his system) and it
won't print nicely all bus devices in one place.


>> Let's allow user understand what his bus contains without looking info
>> defines in .h.
>>
>>
>> > Also this will close the question Arend asked
>> > you regarding same core ids with different manufacturer ids.
>>
>> I don't know what was Arend's question. I asked but it was few minutes
>> ago. I guess he just wanted to point there can be other manufacturer's
>> cores.
>>
> I guess core id 0x800 by 0x04BF vendor and 0x800 by 0x043B vendor will
> both be reported as ChipCommon which most likely is wrong for second
> one. Btw, ChipCommon is 0x500 for 4706 and there will be more new core
> codes for new Broadcom devices. Don't think its right to build core
> names database into kernel while there will be at most few of them used
> on particular end system.

This is constructive, I'll fix this, thanks.

-- 
RafaÅ
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/