mail archive of the barebox mailing list
 help / color / mirror / Atom feed
From: Michael Riesch <michael.riesch@wolfvision.net>
To: Ahmad Fatoum <a.fatoum@pengutronix.de>, barebox@lists.infradead.org
Subject: Re: [RFC PATCH 1/2] arm: rockchip-rk3568-evb: add hardware id detection
Date: Thu, 1 Jul 2021 09:19:21 +0200	[thread overview]
Message-ID: <7f7e89e9-4941-490f-6171-f61e3a39ad73@wolfvision.net> (raw)
In-Reply-To: <d230f623-b7e8-faee-98c8-1526e2840425@pengutronix.de>

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


  reply	other threads:[~2021-07-01  7:21 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-29 12:31 [RFC PATCH 0/2] Proof of concept device tree overlays Michael Riesch
2021-06-29 12:31 ` [RFC PATCH 1/2] arm: rockchip-rk3568-evb: add hardware id detection Michael Riesch
2021-06-30 10:17   ` Ahmad Fatoum
2021-07-01  7:19     ` Michael Riesch [this message]
2021-07-03 20:49     ` Sascha Hauer
2021-06-29 12:31 ` [RFC PATCH 2/2] arm: rockchip-rk3568-evb: apply dt overlay for evb7 Michael Riesch
2021-06-29 21:58   ` Trent Piepho
2021-06-30  9:18     ` Michael Riesch

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7f7e89e9-4941-490f-6171-f61e3a39ad73@wolfvision.net \
    --to=michael.riesch@wolfvision.net \
    --cc=a.fatoum@pengutronix.de \
    --cc=barebox@lists.infradead.org \
    --subject='Re: [RFC PATCH 1/2] arm: rockchip-rk3568-evb: add hardware id detection' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox