Patchwork [v5,2/7] x86, olpc: Use a correct version when making up a battery node

login
register
mail settings
Submitter Lubomir Rintel
Date Jan. 10, 2019, 5:40 p.m.
Message ID <20190110174005.1202564-3-lkundrak@v3.sk>
Download mbox | patch
Permalink /patch/697069/
State New
Headers show

Comments

Lubomir Rintel - Jan. 10, 2019, 5:40 p.m.
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 <lkundrak@v3.sk>
Acked-by: Pavel Machek <pavel@ucw.cz>

---
Changes since v1:
- Avoid splitting string literals

 arch/x86/platform/olpc/olpc_dt.c | 84 +++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 28 deletions(-)
Sebastian Reichel - Jan. 23, 2019, 8:56 p.m.
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 <lkundrak@v3.sk>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> ---

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
Lubomir Rintel - Jan. 31, 2019, 12:26 p.m.
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 <lkundrak@v3.sk>
> > Acked-by: Pavel Machek <pavel@ucw.cz>
> > 
> > ---
> 
> 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
Borislav Petkov - Jan. 31, 2019, 12:59 p.m.
+ 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 <lkundrak@v3.sk>
> > > Acked-by: Pavel Machek <pavel@ucw.cz>
> > > 
> > > ---
> > 
> > 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
>
Lubomir Rintel - Feb. 11, 2019, 11:37 a.m.
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 <lkundrak@v3.sk>
> > > Acked-by: Pavel Machek <pavel@ucw.cz>
> > > 
> > > ---
> > 
> > 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
>
Sebastian Reichel - Feb. 19, 2019, 11:15 p.m.
[+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
Darren Hart - March 7, 2019, 5:01 a.m.
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,

Patch

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)