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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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.


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.

> +	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.

> +
> +	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).

Cheers,
Ahmad

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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