Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node
On Thu, Jan 10, 2019 at 06:40:00PM +0100, Lubomir Rintel wrote: > The XO-1 and XO-1.5 batteries apparently differ in an ability to report > ambient temperature. Add a different compatible string to the 1.5 > battery. Hi Lubomir, Thanks for the recent Cc and pointer to here. In general, I have no problem with the net changes. I do have some concerns from a reviewability and change documentation perspective. You document your intent above, but you also made several other changes to get there which aren't documented, so when reviewing they stand out as "why is this here?". >From what I can tell, this change contains: 1) Cleanup of olpc_dt_interpret() calls to avoid splitting string literals (noted in the patch history, but not called out as an intentional change) 2) Refactoring of logic and breaking out the check for the compatible property into olpc_dt_compatible_match 3) Addition of "we're running very old firmware if this is missing" checks - I'm not sure how this relates to the stated problem and the intended fix. 4) The addition of the xo1.5 compatible property. All the other things makes it very difficult to determine if this patch does what you describe - and only what you describe. In general please: a) Cleanup code b) Refactor code c) Change functionality In that order - that way the new functionality is what show up when someone does a git blame on the file (rather than a cleanup patch, which isn't as useful in defect analysis). And always document in your commit messages the approach you take to fix the reported issue. Thanks, -- Darren Hart VMware Open Source Technology Center
Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node
[+cc Darren Hart, Andy Shevchenko] Hi Lubo, On Mon, Feb 11, 2019 at 12:37:19PM +0100, Lubomir Rintel wrote: > perhaps the message slipped through the cracks? I'm happy to do > whatever is needed to get the patch set into 5.1, but it seems I > need some help and clarifications. The following patches should be merged through the power-supply tree, which is maintained by me. This one is not for my subsystem, so either I need an Acked-by from the x86 people (=they are ok with me merging the patch through the power-supply tree) or they merge it into the x86 platform tree. If it is merged through the x86 tree I need a pull-request from them, so that I can merge the other patches. TLDR: This needs interaction from the x86 platform maintainers (i.e. Darren Hart and Andy Shevchenko). Looks like you did not Cc them? https://patchwork.kernel.org/patch/10756459/ -- Sebastian signature.asc Description: PGP signature
Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node
Hi Sebastian, perhaps the message slipped through the cracks? I'm happy to do whatever is needed to get the patch set into 5.1, but it seems I need some help and clarifications. Thank you, Lubo On Thu, 2019-01-31 at 13:26 +0100, Lubomir Rintel wrote: > Hi, > > On Wed, 2019-01-23 at 21:56 +0100, Sebastian Reichel wrote: > > Hi, > > > > On Thu, Jan 10, 2019 at 06:40:00PM +0100, Lubomir Rintel wrote: > > > The XO-1 and XO-1.5 batteries apparently differ in an ability to report > > > ambient temperature. Add a different compatible string to the 1.5 > > > battery. > > > > > > Signed-off-by: Lubomir Rintel > > > Acked-by: Pavel Machek > > > > > > --- > > > > I either need an Acked-by from the x86 platform maintainers, that I > > can queue this through power-supply or a pull request for an immutable > > branch (probably the better idea). > > I'm happy to prepare a branch that could be pulled from. In fact, > here's a branch with fixes for issues pointed out by the review that > could be pulled from: > > git pull https://github.com/hackerspace/olpc-xo175-linux > lr/olpc-xo175-battery-for-v5.1 > > What do really not understand is how does this help. This is probably > just my unfamiliarity with the process; but perhaps you could help me > get less unfamiliar. Would it somehow help with a potential (though > unlikely) conflict resolution? Would an Ack from x86 crowd serve as an > altenative way off making sure things in their tree won't conflict with > this one? > > > -- Sebastian > > Thank you > Lubo >
Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node
+ the platform maintainers. On Thu, Jan 31, 2019 at 01:26:16PM +0100, Lubomir Rintel wrote: > Hi, > > On Wed, 2019-01-23 at 21:56 +0100, Sebastian Reichel wrote: > > Hi, > > > > On Thu, Jan 10, 2019 at 06:40:00PM +0100, Lubomir Rintel wrote: > > > The XO-1 and XO-1.5 batteries apparently differ in an ability to report > > > ambient temperature. Add a different compatible string to the 1.5 > > > battery. > > > > > > Signed-off-by: Lubomir Rintel > > > Acked-by: Pavel Machek > > > > > > --- > > > > I either need an Acked-by from the x86 platform maintainers, that I > > can queue this through power-supply or a pull request for an immutable > > branch (probably the better idea). > > I'm happy to prepare a branch that could be pulled from. In fact, > here's a branch with fixes for issues pointed out by the review that > could be pulled from: > > git pull https://github.com/hackerspace/olpc-xo175-linux > lr/olpc-xo175-battery-for-v5.1 > > What do really not understand is how does this help. This is probably > just my unfamiliarity with the process; but perhaps you could help me > get less unfamiliar. Would it somehow help with a potential (though > unlikely) conflict resolution? Would an Ack from x86 crowd serve as an > altenative way off making sure things in their tree won't conflict with > this one? > > > -- Sebastian > > Thank you > Lubo > -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node
Hi, On Wed, 2019-01-23 at 21:56 +0100, Sebastian Reichel wrote: > Hi, > > On Thu, Jan 10, 2019 at 06:40:00PM +0100, Lubomir Rintel wrote: > > The XO-1 and XO-1.5 batteries apparently differ in an ability to report > > ambient temperature. Add a different compatible string to the 1.5 > > battery. > > > > Signed-off-by: Lubomir Rintel > > Acked-by: Pavel Machek > > > > --- > > I either need an Acked-by from the x86 platform maintainers, that I > can queue this through power-supply or a pull request for an immutable > branch (probably the better idea). I'm happy to prepare a branch that could be pulled from. In fact, here's a branch with fixes for issues pointed out by the review that could be pulled from: git pull https://github.com/hackerspace/olpc-xo175-linux lr/olpc-xo175-battery-for-v5.1 What do really not understand is how does this help. This is probably just my unfamiliarity with the process; but perhaps you could help me get less unfamiliar. Would it somehow help with a potential (though unlikely) conflict resolution? Would an Ack from x86 crowd serve as an altenative way off making sure things in their tree won't conflict with this one? > -- Sebastian Thank you Lubo
Re: [PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node
Hi, On Thu, Jan 10, 2019 at 06:40:00PM +0100, Lubomir Rintel wrote: > The XO-1 and XO-1.5 batteries apparently differ in an ability to report > ambient temperature. Add a different compatible string to the 1.5 > battery. > > Signed-off-by: Lubomir Rintel > Acked-by: Pavel Machek > > --- I either need an Acked-by from the x86 platform maintainers, that I can queue this through power-supply or a pull request for an immutable branch (probably the better idea). -- Sebastian signature.asc Description: PGP signature
[PATCH v5 2/7] x86, olpc: Use a correct version when making up a battery node
The XO-1 and XO-1.5 batteries apparently differ in an ability to report
ambient temperature. Add a different compatible string to the 1.5
battery.
Signed-off-by: Lubomir Rintel
Acked-by: Pavel Machek
---
Changes since v1:
- Avoid splitting string literals
arch/x86/platform/olpc/olpc_dt.c | 84 +---
1 file changed, 56 insertions(+), 28 deletions(-)
diff --git a/arch/x86/platform/olpc/olpc_dt.c b/arch/x86/platform/olpc/olpc_dt.c
index b4ab779f1d47..8557add82752 100644
--- a/arch/x86/platform/olpc/olpc_dt.c
+++ b/arch/x86/platform/olpc/olpc_dt.c
@@ -217,10 +217,28 @@ static u32 __init olpc_dt_get_board_revision(void)
return be32_to_cpu(rev);
}
-void __init olpc_dt_fixup(void)
+int olpc_dt_compatible_match(phandle node, const char *compat)
{
- int r;
char buf[64];
+ int plen;
+ char *p;
+ int len;
+
+ plen = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
+ if (plen <= 0)
+ return 0;
+
+ len = strlen(compat);
+ for (p = buf; p < buf + plen; p += strlen(p) + 1) {
+ if (strcmp(p, compat) == 0)
+ return 1;
+ }
+
+ return 0;
+}
+
+void __init olpc_dt_fixup(void)
+{
phandle node;
u32 board_rev;
@@ -228,41 +246,51 @@ void __init olpc_dt_fixup(void)
if (!node)
return;
- /*
-* If the battery node has a compatible property, we are running a new
-* enough firmware and don't have fixups to make.
-*/
- r = olpc_dt_getproperty(node, "compatible", buf, sizeof(buf));
- if (r > 0)
- return;
-
- pr_info("PROM DT: Old firmware detected, applying fixes\n");
-
- /* Add olpc,xo1-battery compatible marker to battery node */
- olpc_dt_interpret("\" /battery@0\" find-device"
- " \" olpc,xo1-battery\" +compatible"
- " device-end");
-
board_rev = olpc_dt_get_board_revision();
if (!board_rev)
return;
if (board_rev >= olpc_board_pre(0xd0)) {
+ if (olpc_dt_compatible_match(node, "olpc,xo1.5-battery"))
+ return;
+
+ /* Add olpc,xo1.5-battery compatible marker to battery node */
+ olpc_dt_interpret("\" /battery@0\" find-device");
+ olpc_dt_interpret(" \" olpc,xo1.5-battery\" +compatible");
+ olpc_dt_interpret("device-end");
+
+ /* We're running a very old firmware if this is missing. */
+ if (olpc_dt_compatible_match(node, "olpc,xo1-battery"))
+ return;
+
/* XO-1.5: add dcon device */
- olpc_dt_interpret("\" /pci/display@1\" find-device"
- " new-device"
- " \" dcon\" device-name \" olpc,xo1-dcon\" +compatible"
- " finish-device device-end");
+ olpc_dt_interpret("\" /pci/display@1\" find-device");
+ olpc_dt_interpret(" new-device");
+ olpc_dt_interpret("\" dcon\" device-name");
+ olpc_dt_interpret("\" olpc,xo1-dcon\" +compatible");
+ olpc_dt_interpret(" finish-device");
+ olpc_dt_interpret("device-end");
} else {
+ /* We're running a very old firmware if this is missing. */
+ if (olpc_dt_compatible_match(node, "olpc,xo1-battery"))
+ return;
+
/* XO-1: add dcon device, mark RTC as olpc,xo1-rtc */
- olpc_dt_interpret("\" /pci/display@1,1\" find-device"
- " new-device"
- " \" dcon\" device-name \" olpc,xo1-dcon\" +compatible"
- " finish-device device-end"
- " \" /rtc\" find-device"
- " \" olpc,xo1-rtc\" +compatible"
- " device-end");
+ olpc_dt_interpret("\" /pci/display@1,1\" find-device");
+ olpc_dt_interpret(" new-device");
+ olpc_dt_interpret("\" dcon\" device-name");
+ olpc_dt_interpret("\" olpc,xo1-dcon\" +compatible");
+ olpc_dt_interpret(" finish-device");
+ olpc_dt_interpret("device-end");
+ olpc_dt_interpret("\" /rtc\" find-device");
+ olpc_dt_interpret(" \" olpc,xo1-rtc\" +compatible");
+ olpc_dt_interpret("device-end");
}
+
+ /* Add olpc,xo1-battery compatible marker to battery node */
+ olpc_dt_interpret("\" /battery@0\" find-device");
+ olpc_dt_interpret(" \" olpc,xo1-battery\" +compatible");
+ olpc_dt_interpret("device-end");
}
void __init olpc_dt_build_devicetree(void)
--
2.20.1
