Re: [PATCH v2 17/17] power: supply: olpc_battery: Add OLPC XO 1.75 support
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
> Acked-by: Pavel Machek
>
> ---
> 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_nodex01.75-ec...)
...
else if (ecver > 0x44)
...
else
...
And what happens when ecver == 0x44? We test for > and < but not ==, <=,
or >= in this block
> + /* 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
>
>
--
Darren Hart
VMware Open Source Technology Center
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 13/17] power: supply: olpc_battery: Use DT to get battery version
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
> Reviewed-by: Andy Shevchenko
> Acked-by: Pavel Machek
>
> ---
> 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -622,11 +623,13 @@ static int olpc_battery_probe(struct platform_device
> *pdev)
> olpc_ac = power_supply_register(>dev, _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" },
The code previously supported xo1.5 (it seems), and the commit message doesn't
mention changing the compatible string. Was this an intentional change? If so,
it should be mentioned in the commit message.
> {}
> };
> MODULE_DEVICE_TABLE(of, olpc_battery_ids);
> --
> 2.19.1
>
>
--
Darren Hart
VMware Open Source Technology Center
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 07/17] Platform: OLPC: Avoid a warning if the EC didn't register yet
On Fri, Nov 16, 2018 at 05:23:53PM +0100, Lubomir Rintel wrote: > Just return ENODEV, so that whoever attempted to use the EC call can I think you meant EPROBE_DEFER here, but the language in the commit message is a bit ambiguous here... > defer their work. > > Signed-off-by: Lubomir Rintel > Acked-by: Pavel Machek > > --- > Changes since v1: > - EPROBE_DEFER instead of ENODEV > > drivers/platform/olpc/olpc-ec.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/olpc/olpc-ec.c b/drivers/platform/olpc/olpc-ec.c > index 35a21c66cd0d..9ee993d5d54b 100644 > --- a/drivers/platform/olpc/olpc-ec.c > +++ b/drivers/platform/olpc/olpc-ec.c > @@ -116,8 +116,11 @@ int olpc_ec_cmd(u8 cmd, u8 *inbuf, size_t inlen, u8 > *outbuf, size_t outlen) > struct olpc_ec_priv *ec = ec_priv; > struct ec_cmd_desc desc; > > - /* Ensure a driver and ec hook have been registered */ > - if (WARN_ON(!ec_driver || !ec_driver->ec_cmd)) > + /* Driver not yet registered. */ > + if (!ec_driver) > + return -EPROBE_DEFER; > + > + if (WARN_ON(!ec_driver->ec_cmd)) > return -ENODEV; > > if (!ec) > -- > 2.19.1 > > -- Darren Hart VMware Open Source Technology Center ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 06/17] Platform: OLPC: Add XO-1.75 EC driver
On Fri, Nov 16, 2018 at 05:23:52PM +0100, Lubomir Rintel wrote: > It's based off the driver from the OLPC kernel sources. Somewhat > modernized and cleaned up, for better or worse. > > Modified to plug into the olpc-ec driver infrastructure (so that battery > interface and debugfs could be reused) and the SPI slave framework. > > Signed-off-by: Lubomir Rintel > Hi Lubomir, You asked for some tips on how to incorporate the changes in a patch series like this. Keep in mind that each patch should create an independent small functional change. This makes it easier to review the patch and verify that what you said you were going to do matches what the patch does. For example, if you separate out the style, whitespace, ordering of declarations into an initial first patch, then all that noise is removed when the reviewer goes to check to implementation of one of the features below. This is a large patch, and should most certainly be broken up into several smaller patches. Do cleanups first, followed by functional changes. This ordering ensure that when a "git blame" is used in the future to understand why a given line is what it is, the first hit will be a functional change, and not a cleanup. > --- > Changes since v1: > - Cosmetic style changes; whitespace, ordering of declarations and > #includes, remoted extra comas from sentinels Please make this a separate change, possibly more than one, depending on how many of these there are. This will reduce the size of the subsequent patches, making them easier to review. > - Count the terminating NUL in LOG_BUF_SIZE > - Make olpc_xo175_ec_is_valid_cmd() return -EINVAL instead of -1 > on error > - Spell keyboard/touchpad in full for CHAN_KEYBOARD/TOUCHPAD messages > - Use a #define for PM wakeup processing time > - Log a message on unknown event > - Escape logging payload with %*pE > - Replace an open-coded min() > - Correct an error code on short read > - replaced PM callback #ifdefs with __maybe_unusedm SET_RUNTIME_PM_OPS > and SET_NOIRQ_SYSTEM_SLEEP_PM_OPS > - dev_get_drvdata() instead of a round-trip through platform device > - s/unsigned char x/u8 x/ in olpc_xo175_ec_resume() > - Use GENMASK() instead of 0x for the event mask > - Replace cmd tx/resp rx buffers with structures > - Turned suspend hint arguments into a struct, and tidied up the comment Just from these comments, each of these could be a separate patch. You can group related things together, or those that change the same line or function for example. Order them with cleanups / non-functional-changes first, followed by functional changes. > > Basically all of the above is based on the review by Andy Shevchenko. Andy, what was your intent for Lubomir here? From the above, this looks like it should be several patches to me. Thanks, -- Darren Hart VMware Open Source Technology Center ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg
On Wed, Sep 12, 2018 at 05:08:52PM +0200, Arnd Bergmann wrote:
> The .ioctl and .compat_ioctl file operations have the same prototype so
> they can both point to the same function, which works great almost all
> the time when all the commands are compatible.
>
> One exception is the s390 architecture, where a compat pointer is only
> 31 bit wide, and converting it into a 64-bit pointer requires calling
> compat_ptr(). Most drivers here will ever run in s390, but since we now
> have a generic helper for it, it's easy enough to use it consistently.
>
> I double-checked all these drivers to ensure that all ioctl arguments
> are used as pointers or are ignored, but are not interpreted as integer
> values.
>
> Signed-off-by: Arnd Bergmann
> ---
...
> drivers/platform/x86/wmi.c | 2 +-
...
> static void link_event_work(struct work_struct *work)
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 04791ea5d97b..e4d0697e07d6 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -886,7 +886,7 @@ static const struct file_operations wmi_fops = {
> .read = wmi_char_read,
> .open = wmi_char_open,
> .unlocked_ioctl = wmi_ioctl,
> - .compat_ioctl = wmi_ioctl,
> + .compat_ioctl = generic_compat_ioctl_ptrarg,
> };
For platform/drivers/x86:
Acked-by: Darren Hart (VMware)
As for a longer term solution, would it be possible to init fops in such
a way that the compat_ioctl call defaults to generic_compat_ioctl_ptrarg
so we don't have to duplicate this boilerplate for every ioctl fops
structure?
--
Darren Hart
VMware Open Source Technology Center
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 11/20] power_supply: Change ownership from driver to core
] (seq_read) from [c00e53dc] (__vfs_read+0x18/0x4c) [ 55.979188] [c00e53dc] (__vfs_read) from [c00e548c] (vfs_read+0x7c/0x100) [ 55.986304] [c00e548c] (vfs_read) from [c00e5550] (SyS_read+0x40/0x8c) [ 55.993164] [c00e5550] (SyS_read) from [c000f1a0] (ret_fast_syscall+0x0/0x48) [ 56.000626] Code: bad PC value [ 56.011652] ---[ end trace 7b64343fbdae8ef1 ]--- Signed-off-by: Krzysztof Kozlowski k.kozlow...@samsung.com Reviewed-by: Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com [for the nvec part] Reviewed-by: Marc Dietrich marvi...@gmx.de --- arch/x86/platform/olpc/olpc-xo1-sci.c | 4 +- arch/x86/platform/olpc/olpc-xo15-sci.c| 4 +- drivers/acpi/ac.c | 32 +++--- drivers/acpi/battery.c| 55 +- drivers/acpi/sbs.c| 68 +++- drivers/hid/hid-input.c | 51 + drivers/hid/hid-sony.c| 43 drivers/hid/hid-wiimote-modules.c | 41 +++ drivers/hid/hid-wiimote.h | 3 +- drivers/hid/wacom.h | 8 +- drivers/hid/wacom_sys.c | 71 ++-- drivers/platform/x86/compal-laptop.c | 29 +++-- drivers/power/[...] | lots of changes drivers/staging/nvec/nvec_power.c | 29 +++-- include/linux/hid.h | 6 +- include/linux/mfd/abx500/ux500_chargalg.h | 11 +- include/linux/mfd/rt5033.h| 2 +- include/linux/mfd/wm8350/supply.h | 6 +- include/linux/power/charger-manager.h | 3 +- include/linux/power_supply.h | 39 --- 80 files changed, 2172 insertions(+), 1756 deletions(-) I would like to merge this via the power supply subsystem. Some of the files being patched are not under my maintainence, though. It would be nice if I get an Acked-By from their maintainers. As far as I can see this patch is currently missing feedback from: arch/x86/platform/olpc: x86 Maintainers drivers/acpi: Rafael J. Wysocki, Len Brown drivers/hid: Jiri Kosina drivers/platform/x86: Darren Hart For compal-laptop.c: Acked-by: Darren Hart dvh...@linux.intel.com -- Darren Hart Intel Open Source Technology Center ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 02/20] power_supply: Move run-time configuration to separate structure
On Fri, Jan 30, 2015 at 03:47:40PM +0100, Krzysztof Kozlowski wrote: Add new structure 'power_supply_config' for holding run-time initialization data like of_node, supplies and private driver data. The power_supply_register() function is changed so all power supply drivers need updating. When registering the power supply this new 'power_supply_config' should be used instead of directly initializing 'struct power_supply'. This allows changing the ownership of power_supply structure from driver to the power supply core in next patches. When a driver does not use of_node or supplies then it should use NULL as config. If driver uses of_node or supplies then it should allocate config on stack and initialize it with proper values. Signed-off-by: Krzysztof Kozlowski k.kozlow...@samsung.com Reviewed-by: Bartlomiej Zolnierkiewicz b.zolnier...@samsung.com For drivers/platform/x86/compal-laptop.c Reviewed-by: Darren Hart dvh...@linux.intel.com ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
