Re: [RFC PATCH 1/2] arm: rockchip-rk3568-evb: add hardware id detection

[Michael Riesch <michael.riesch@wolfvision.net>]

Hello Ahmad,

On 6/30/21 12:17 PM, Ahmad Fatoum wrote:
> Hello Michael,
> 
> Trent already commented on the device tree overlay part.
> This commit here seems applicable regardless,
> so in case you want to resend it for upstream inclusion,
> some comments are inline.

Thanks for your feedback. OK, I'll separate this part and resend a
non-RFC patch (not sure when, though).

> On 29.06.21 14:31, Michael Riesch wrote:
>> Signed-off-by: Michael Riesch <michael.riesch@wolfvision.net>
> 
> A short commit message here would be nice, e.g.:
> 
> The rk3568 EVB uses a voltage divider to ... etc.
> 
>> ---
>>  arch/arm/boards/rockchip-rk3568-evb/board.c | 36 +++++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>
>> diff --git a/arch/arm/boards/rockchip-rk3568-evb/board.c b/arch/arm/boards/rockchip-rk3568-evb/board.c
>> index 57c24ed3c..ee8e1b979 100644
>> --- a/arch/arm/boards/rockchip-rk3568-evb/board.c
>> +++ b/arch/arm/boards/rockchip-rk3568-evb/board.c
>> @@ -2,6 +2,7 @@
>>  #include <common.h>
>>  #include <init.h>
>>  #include <mach/bbu.h>
>> +#include <aiodev.h>
>>  #include <bootsource.h>
>>  
>>  static int rk3568_evb_probe(struct device_d *dev)
>> @@ -34,3 +35,38 @@ static struct driver_d rk3568_evb_board_driver = {
>>  	.of_compatible = rk3568_evb_of_match,
>>  };
>>  coredevice_platform_driver(rk3568_evb_board_driver);
>> +
>> +static int rk3568_evb_detect_version(void)
>> +{
> 
> Once more 64-bit rockchip boards are added, they would all execute this
> initcall if this board is enabled. For this reason, you need a compatible
> check here.
> 
>> +	int ret = 0;
>> +	int evb_hw_id = 0;
> 
> Nitpick: Initializing variables with values that are never read risks introducing
> silent bugs once the code that uses it later on changes. If you leave
> it uninitialized you get a compiler warning if it's referenced without
> initialization.
> 
>> +	int evb_hw_id_voltage = 1800;
> 
> I am not sure it's a good idea to report the EVB is v1 when the
> driver isn't enabled.

This default is based on the assumption that the EVB1 device tree works
for all variants and does not require any overlays. The other variants
do require a specific overlay. This assumption was somewhat valid in the
scope of this RFC patch series but cannot be applied in general...

> 
>> +	struct aiochannel *evb_hw_id_chan;
>> +
>> +	evb_hw_id_chan = aiochannel_by_name("aiodev0.in_value1_mV");
>> +	if (!IS_ERR(evb_hw_id_chan))
> 
> In the error case it makes no sense to continue here, you should early exit.
> 
>> +		ret = aiochannel_get_value(evb_hw_id_chan, &evb_hw_id_voltage);
>> +	if (ret || IS_ERR(evb_hw_id_chan))
>> +		pr_warn("couldn't retrieve hardware ID");
> 
> early exit
> 
>> +
>> +	if (evb_hw_id_voltage > 1650) {
>> +		evb_hw_id = 1;
>> +	} else if (evb_hw_id_voltage > 1350) {
>> +		evb_hw_id = 2;
>> +	} else if (evb_hw_id_voltage > 1050) {
>> +		evb_hw_id = 3;
>> +	} else if (evb_hw_id_voltage > 750) {
>> +		evb_hw_id = 4;
>> +	} else if (evb_hw_id_voltage > 450) {
>> +		evb_hw_id = 5;
>> +	} else if (evb_hw_id_voltage > 150) {
>> +		evb_hw_id = 6;
>> +	} else {
>> +		evb_hw_id = 7;
>> +	}
>> +
>> +	pr_info("Detected RK3568 EVB%d\n", evb_hw_id);
> 
> You could populate a variable, e.g. global.board.revision
> here. That would allow using this info in scripts as Trent has described.

... so what should happen if the board variant cannot be detected?
Should the variable be empty? Not set at all? Set to "unknown"?

>> +
>> +	return 0;
>> +}
>> +late_initcall(rk3568_evb_detect_version);
> 
> Optimally, you would call the function from the board driver probe above.
> At postcore initcall, you wouldn't find the ADC yet, but if you
> return EPROBE_DEFER on error from the probe, the probe will be retried
> later on at which time it will succeed.
> 
> With the deep probe patches Sascha just merged, it is possible to directly
> probe dependencies instead of retrying later and hoping it was probed
> in-between. It's still not enabled for the EVB here though and it also
> needs some code added to aiodev, so for now you could go the EPROBE_DEFER
> route (or add a compatible check).

I think I'll stick to the late_initcall with compatible check as this
seems to be the most straightforward solution right now.

Regards,
Michael

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox