by Sebastian Reichel

[permalink] [raw]

Subject: Re: [PATCH 0/15] Add support for OLPC XO 1.75 Embedded Controller

Hi,

On Wed, Oct 10, 2018 at 07:22:45PM +0200, Lubomir Rintel wrote:
> Hi.
>
> This patchset adds support for the Embedded Controller on an OLPC XO
> 1.75 machine. OLPC XO 1.75 is a MMP2 based ARM laptop. It plugs into
> the existing OLPC platform infrastructure, currently used by the x86
> based models.
>
> The EC operates in SPI master mode, meaning the SOC is the SPI slave. It
> uses extra handshake signal to signal readiness of SOC to accept data
> from EC and to initiate a transaction if SOC wishes to submit data.
>
> The SPI slave support for MMP2 was submitted separately:
> https://lore.kernel.org/lkml/[email protected]/T/#t
>
> THe "power: supply: olpc_battery: correct the temperature" patch was
> already sent out separately, but I'm including it because the last
> commit of the set depends on it.
>
> Tested to work on an OLPC XO 1.75 and also tested not to break x86
> support with an OLPC XO 1 machine. I don't have a XO 1.5, but it's
> unlikely this breaks it when XO 1 works.
>
> Thanks in advance for reviews and feedback of any kind.

I reviewed the power-supply related patches (1, 10, 12-15) and think
they are fine apart from the things found by Andy Shevchenko.

-- Sebastian

Attachments:

(No filename) (1.23 kB)
signature.asc (849.00 B)
Download all attachments

2018-10-22 13:08:31

by Sebastian Reichel

[permalink] [raw]

Subject: Re: [PATCH 01/15] power: supply: olpc_battery: correct the temperature units

Hi,

On Fri, Oct 19, 2018 at 04:00:32PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 10, 2018 at 8:23 PM Lubomir Rintel <[email protected]> wrote:
> >
> > According to [1] and [2], the temperature values are in tenths of degree
> > Celsius. Exposing the Celsius value makes the battery appear on fire:
> >
> > $ upower -i /org/freedesktop/UPower/devices/battery_olpc_battery
> > ...
> > temperature: 236.9 degrees C
> >
> > Tested on OLPC XO-1 and OLPC XO-1.75 laptops.
>
> It's interesting that the very author of that code is not included in
> so-o long Cc list :)
> Cc: David.
>
> David, do you remember if and how you had tested temperature report of
> the battery on OLPC? I guess this kind of error would be appear immediately.

It depends on the way of testing. It's not so obvious when you just
do a `cat /sys/class/power_supply/.../temperature`, since you just
get the raw integer.

> OTOH it might be that power framework had changed requirements (which
> would be noticeable change).

As far as I know, power-supply has always used 1/10 ?C. People tend
to get units wrong all the time though (i.e. mV instead of uV). I'm
not surprised, that this sneaked into the kernel.

> If the latter is true, this patch misses Fixes tag. Actually in any
> case it misses it.

Yes, this should probably have Fixes: fb972873a767 ("[BATTERY] One Laptop
Per Child power/battery driver").

-- Sebastian

> > [1] include/linux/power_supply.h
> > [2] Documentation/power/power_supply_class.txt
> >
> > Cc: [email protected]
> > Signed-off-by: Lubomir Rintel <[email protected]>
> > ---
> > drivers/power/supply/olpc_battery.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/power/supply/olpc_battery.c b/drivers/power/supply/olpc_battery.c
> > index 6da79ae14860..5a97e42a3547 100644
> > --- a/drivers/power/supply/olpc_battery.c
> > +++ b/drivers/power/supply/olpc_battery.c
> > @@ -428,14 +428,14 @@ static int olpc_bat_get_property(struct power_supply *psy,
> > if (ret)
> > return ret;
> >
> > - val->intval = (s16)be16_to_cpu(ec_word) * 100 / 256;
> > + val->intval = (s16)be16_to_cpu(ec_word) * 10 / 256;
> > break;
> > case POWER_SUPPLY_PROP_TEMP_AMBIENT:
> > ret = olpc_ec_cmd(EC_AMB_TEMP, NULL, 0, (void *)&ec_word, 2);
> > if (ret)
> > return ret;
> >
> > - val->intval = (int)be16_to_cpu(ec_word) * 100 / 256;
> > + val->intval = (int)be16_to_cpu(ec_word) * 10 / 256;
> > break;
> > case POWER_SUPPLY_PROP_CHARGE_COUNTER:
> > ret = olpc_ec_cmd(EC_BAT_ACR, NULL, 0, (void *)&ec_word, 2);
> > --
> > 2.19.0
> >
>
>
> --
> With Best Regards,
> Andy Shevchenko

Attachments:

Hi!

> > > +#include <linux/delay.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/spinlock.h>
> > > +#include <linux/completion.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/ctype.h>
> > > +#include <linux/olpc-ec.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/reboot.h>
> > > +#include <linux/input.h>
> > > +#include <linux/kfifo.h>
> > > +#include <linux/module.h>
> > > +#include <linux/power_supply.h>
> >
> > Easy to maintain when it's sorted.

No / it depends.

> > > + channel = priv->rx_buf[0];
> > > + byte = priv->rx_buf[1];
> >
> > Maybe specific structures would fit better?
> >
> > Like
> >
> > struct olpc_ec_resp_hdr {
> > u8 channel;
> > u8 byte;
> > ...
> > }

Structures have padding and other nastyness...

> > > + pm_wakeup_event(priv->pwrbtn->dev.parent,
> > > 1000);
> >
> > Magic number.

Nothing wrong with magic numbers.

> > > + args[0] = mask & 0xff;
> > > + args[1] = (mask >> 8) & 0xff;
> >
> > ...mask >> 0;
> > ...mask >> 8;

No, please.

> > __maybe_unused instead of ugly #ifdef?
> >
> > > +{
> > > + struct platform_device *pdev = to_platform_device(dev);
> > > + struct olpc_xo175_ec *priv = platform_get_drvdata(pdev);
> >
> > dev_get_drvdata() or how is it called?
> >
> > > + unsigned char hintargs[5];
> >
> > struct olpc_ec_hint_cmd {
> > u8 ...
> > u32 ...
> > };
> >
> > ?

No, unless you want to break the code. Or add __attribute__ packed and
deal with endianness.

Just no.

> > > + static unsigned int suspend_count;
> >
> > u32 I suppose.

You know, there's semantic difference between unsigned int and
u32. And this sounds like candidate for unsigned int.

> > > + /* Enable all EC events while we're awake */
> > > + olpc_xo175_ec_set_event_mask(0xffff);
> >
> > #define EC_ALL_EVENTS GENMASK(15, 0)

Actually that's less readable. Just don't.

> > > +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> > > + { .compatible = "olpc,xo1.75-ec" },
> > > + { },
> >
> > No comma for terminators.

Comma is fine.
Pavel

--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (2.36 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2018-11-19 13:26:19

by Lubomir Rintel

[permalink] [raw]

Subject: Re: [PATCH 06/15] Platform: OLPC: Add XO-1.75 EC driver

Hi Pavel,

I've addressed some of Andy's concerns you're replying to below in a
follow-up version of the patch. To many points I'm generally
indifferent and prefer to lean towards making things easy for whoever
may be likely to deal with the code in past. It's not clear to me who
that might be, or how important either of you consider your remarks to
be. I'll be thankful if anyone makes this clearer to me.

(I hoped that you're in the Cc list; thought git send-mail will see
your address in the Acked-by tags and add you. I failed to notice that
it's not the case. I apologize. Here's the v2 posting: [1].)

[1] https://lore.kernel.org/lkml/[email protected]/

On Mon, 2018-11-19 at 11:40 +0100, Pavel Machek wrote:
> Hi!
>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/gpio/consumer.h>
> > > > +#include <linux/spinlock.h>
> > > > +#include <linux/completion.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/ctype.h>
> > > > +#include <linux/olpc-ec.h>
> > > > +#include <linux/spi/spi.h>
> > > > +#include <linux/reboot.h>
> > > > +#include <linux/input.h>
> > > > +#include <linux/kfifo.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/power_supply.h>
> > >
> > > Easy to maintain when it's sorted.
>
> No / it depends.

I've sorted it in the new files; following a rule of keeping things
sorted at the very least has an advantage of being unambiguous.

I'm not sorting things where they currently are not in order, since it
would obscure real changes.

> > > > + channel = priv->rx_buf[0];
> > > > + byte = priv->rx_buf[1];
> > >
> > > Maybe specific structures would fit better?
> > >
> > > Like
> > >
> > > struct olpc_ec_resp_hdr {
> > > u8 channel;
> > > u8 byte;
> > > ...
> > > }
>
> Structures have padding and other nastyness...

I've turned them into structs. It perhaps looks a bit better -- the
structure members serving as a documentation for the protocol.

> > > > + pm_wakeup_event(priv->pwrbtn-
> > > > >dev.parent,
> > > > 1000);
> > >
> > > Magic number.
>
> Nothing wrong with magic numbers.

Turned it into a define, but it's not like it's any clearer why that
particular value works...

I actually have little idea. I've copied this from the vendor's
original driver.

> > > > + args[0] = mask & 0xff;
> > > > + args[1] = (mask >> 8) & 0xff;
> > >
> > > ...mask >> 0;
> > > ...mask >> 8;
>
> No, please.

I've done this too, but I don't see much point either. Perhaps those
within the OCD spectrum may appreciate :)

> > > __maybe_unused instead of ugly #ifdef?
> > >
> > > > +{
> > > > + struct platform_device *pdev = to_platform_device(dev);
> > > > + struct olpc_xo175_ec *priv =
> > > > platform_get_drvdata(pdev);
> > >
> > > dev_get_drvdata() or how is it called?
> > >
> > > > + unsigned char hintargs[5];
> > >
> > > struct olpc_ec_hint_cmd {
> > > u8 ...
> > > u32 ...
> > > };
> > >
> > > ?
>
> No, unless you want to break the code. Or add __attribute__ packed
> and
> deal with endianness.
>
> Just no.

Turned it into a packed struct too for the same reasons as above.

The endianness of the hints argument doesn't actually matter, since the
EC firmware actually uses this in a debugging printf. It's not even
cleat what endianness did EC expect initially.

> > > > + static unsigned int suspend_count;
> > >
> > > u32 I suppose.
>
> You know, there's semantic difference between unsigned int and
> u32. And this sounds like candidate for unsigned int.
>
> > > > + /* Enable all EC events while we're awake */
> > > > + olpc_xo175_ec_set_event_mask(0xffff);
> > >
> > > #define EC_ALL_EVENTS GENMASK(15, 0)
>
> Actually that's less readable. Just don't.

Done this too, and, like the points above, I am indifferent too. Both
seem equal to me.

> > > > +static const struct of_device_id olpc_xo175_ec_of_match[] = {
> > > > + { .compatible = "olpc,xo1.75-ec" },
> > > > + { },
> > >
> > > No comma for terminators.
>
> Comma is fine.

Removed it. Perhaps the lack of a comma could be understood as an
indication that no further initializers shall follow.

> Pavel

Just in case I didn't stress that enough: I'm much more interested in
things working well than style issues that too often are just matter of
tast. I happily accept advice on those though.

Take care,
Lubo