On Sun, 2018-12-02 at 15:34 -0800, Darren Hart wrote:
> On Fri, Nov 16, 2018 at 05:24:03PM +0100, Lubomir Rintel wrote:
> > The battery and the protocol are essentially the same as OLPC XO 1.5,
> > but the responses from the EC are LSB first.
> >
> > Signed-off-by: Lubomir Rintel <[email protected]>
> > Acked-by: Pavel Machek <[email protected]>
> >
> > ---
> > Changes since v1:
> > - s/s16 ecword_to_cpu/u16 ecword_to_cpu/
> > - s/u16 ec_byte/u16 ec_word/
> >
> > drivers/power/supply/olpc_battery.c | 23 ++++++++++++++++++-----
> > 1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
>
> ...
>
> > @@ -626,6 +635,10 @@ static int olpc_battery_probe(struct platform_device *pdev)
> > if (ecver > 0x44) {
> > /* XO 1 or 1.5 with a new EC firmware. */
> > data->new_proto = 1;
> > + } else if (of_find_compatible_node(NULL, NULL, "olpc,xo1.75-ec")) {
>
> This if/else blocks concerns me a bit, but I might just be missing some
> context.
>
> This tests both ecver as well as the OF compatible string, is this reliable? Do
> we know that for all xo1.75-ec compatible nodes the ecver will be <= 0x44? Or,
> is ecver undefined? If the latter, then perhaps this test should be performed
> first?
>
> if (of_find_compatible_node....x01.75-ec...)
> ...
> else if (ecver > 0x44)
> ...
> else
> ...
>
> And what happens when ecver == 0x44? We test for > and < but not ==, <=,
> or >= in this block

You're right, the conditionals are not correct. On XO 1.75 the
versioning is different (now at level 0x05) and uninteresting,
therefore the XO 1.75 check needs to go first.

On XO 1 and XO 1.75, we don't support < 0x44. 0x44 is okay, though uses
stays with an old protocol, and > 0x44 uses a new protocol.

Will follow up with a new version of the patch soon.

>
> > + /* XO 1.75 */
> > + data->new_proto = 1;
> > + data->little_endian = 1;
> > } else if (ecver < 0x44) {
> > /*
> > * We've seen a number of EC protocol changes; this driver
> > --
> > 2.19.1

Thanks
Lubo

2019-01-07 18:38:38

by Lubomir Rintel

[permalink] [raw]

Subject: Re: [PATCH v2 13/17] power: supply: olpc_battery: Use DT to get battery version

On Wed, 2018-12-05 at 21:54 +0100, Sebastian Reichel wrote:
> Hi,
>
> On Fri, Nov 16, 2018 at 05:23:59PM +0100, Lubomir Rintel wrote:
> > Avoid using the x86 OLPC platform specific call to get the board
> > version. It won't work on FDT-based ARM MMP2 platform.
> >
> > Signed-off-by: Lubomir Rintel <[email protected]>
> > Reviewed-by: Andy Shevchenko <[email protected]>
> > Acked-by: Pavel Machek <[email protected]>
> >
> > ---
>
> Reviewed-by: Sebastian Reichel <[email protected]>
>
> I assume this patch cannot be applied to power-supply without patch
> 12 having been merged to avoid breaking XO1.5 devices temporarily?

That is true. What's the correct way to deal with a situation like
this? Just hold the patch until the other changes get in, perhaps via
linux-platform-drivers-x86.git? Or maybe this can go in together via
that tree along with the other patches?

Thanks
Lubo

> -- Sebastian
>
> > Changes since v1:
> > - Sort the new include a bit higher
> >
> > drivers/power/supply/olpc_battery.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/power/supply/olpc_battery.c
> > b/drivers/power/supply/olpc_battery.c
> > index 5a97e42a3547..5323987d9284 100644
> > --- a/drivers/power/supply/olpc_battery.c
> > +++ b/drivers/power/supply/olpc_battery.c
> > @@ -14,6 +14,7 @@
> > #include <linux/types.h>
> > #include <linux/err.h>
> > #include <linux/device.h>
> > +#include <linux/of.h>
> > #include <linux/platform_device.h>
> > #include <linux/power_supply.h>
> > #include <linux/jiffies.h>
> > @@ -622,11 +623,13 @@ static int olpc_battery_probe(struct
> > platform_device *pdev)
> > olpc_ac = power_supply_register(&pdev->dev, &olpc_ac_desc,
> > NULL);
> > if (IS_ERR(olpc_ac))
> > return PTR_ERR(olpc_ac);
> > -
> > - if (olpc_board_at_least(olpc_board_pre(0xd0))) { /* XO-1.5 */
> > + if (of_property_match_string(pdev->dev.of_node, "compatible",
> > + "olpc,xo1.5-battery") >= 0) {
> > + /* XO-1.5 */
> > olpc_bat_desc.properties = olpc_xo15_bat_props;
> > olpc_bat_desc.num_properties =
> > ARRAY_SIZE(olpc_xo15_bat_props);
> > - } else { /* XO-1 */
> > + } else {
> > + /* XO-1 */
> > olpc_bat_desc.properties = olpc_xo1_bat_props;
> > olpc_bat_desc.num_properties =
> > ARRAY_SIZE(olpc_xo1_bat_props);
> > }
> > @@ -672,6 +675,7 @@ static int olpc_battery_remove(struct
> > platform_device *pdev)
> >
> > static const struct of_device_id olpc_battery_ids[] = {
> > { .compatible = "olpc,xo1-battery" },
> > + { .compatible = "olpc,xo1.5-battery" },
> > {}
> > };
> > MODULE_DEVICE_TABLE(of, olpc_battery_ids);
> > --
> > 2.19.1
> >