linux-kernel - Re: [PATCHv2] power_supply: Add support for Richtek rt9455 battery charger

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 30 Apr 2015 16:53:56 +0900
From:	Krzysztof Kozlowski <k.kozlowski@...sung.com>
To:	Anda-Maria Nicolae <anda-maria.nicolae@...el.com>
Cc:	sre@...nel.org, dbaryshkov@...il.com, robh+dt@...nel.org,
	pawel.moll@....com, mark.rutland@....com,
	ijc+devicetree@...lion.org.uk, galak@...eaurora.org,
	dwmw2@...radead.org, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCHv2] power_supply: Add support for Richtek rt9455 battery charger

2015-04-30 5:13 GMT+09:00 Anda-Maria Nicolae <anda-maria.nicolae@...el.com>:
> Based on the datasheet found here:
> http://www.richtek.com/download_ds.jsp?p=RT9455
>
> Updates from the previous version:
> - replaced license GPLv2 with GPL
> - replaced vendor prefix rt with richtek
> - replaced uppercase properties names from devicetree with lowercase separated by dash properties names
> - replaced I/O read/write API with regmap_read/write and regmap_field_read/write
> - used kernel multiline comment
> - used DELAYED_WORK and scheduled the works in system_power_efficient_wq. It is high probability that the device is in suspend state while charging (i.e. the user leaves the device to charge during night) and it is needed that the scheduled works to be executed as planned.

I think when device is suspended (e.g. to RAM) no work will ever be
executed. The timers (for delayed work) will not wake up the device...
RTC could wake but this is different story. My proposal of deferred
timers is affected by idle CPU time. Are you sure that you want to
wake up from the system suspend?

> - replaced struct power_supply_desc rt9455_charger_desc with static const struct power_supply_desc rt9455_charger_desc
> - replaced if (IS_ERR_OR_NULL(info->charger)) with if (IS_ERR(info->charger))
> - replaced {"RTK9455", 0} with { "RTK9455", 0 }
> - removed .owner = THIS_MODULE
>
> Signed-off-by: Anda-Maria Nicolae <anda-maria.nicolae@...el.com>

You missed some of my comments. I mentioned wrong error paths around
put_usb_phy in probe. They do not seem to be fixed...

Just one hint - mostly new bindings are posted in separate patches at
the beginning of the patchset. I actually don't mind. but separating
them will probably give you higher chance of review from DT people.
This is also mentioned in:
Documentation/devicetree/bindings/submitting-patches.txt

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/