| Message ID | 1392814154-11284-5-git-send-email-klightspeed@killerwolves.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2014-02-19 at 22:49 +1000, klightspeed@killerwolves.net wrote: > Correct the vendor prefix for qnap, which should be the stock ticker > symbol qnapsz. No. This is an ABI change which appears to be gratuitous. Perhaps it's really necessary, and perhaps it's harmless because it won't *really* break compatibility with existing firmware because these boxes have *always* shipped with a DTB built into the kernel or something. In which case all I'm complaining about is the fact that you didn't properly address that in the commit comment and *explain* why it was OK...
> diff --git a/drivers/power/reset/qnap-poweroff.c b/drivers/power/reset/qnap-poweroff.c > index a75db7f..d350ece 100644 > --- a/drivers/power/reset/qnap-poweroff.c > +++ b/drivers/power/reset/qnap-poweroff.c > @@ -41,7 +41,7 @@ static const struct power_off_cfg synology_power_off_cfg = { > }; > > static const struct of_device_id qnap_power_off_of_match_table[] = { > - { .compatible = "qnap,power-off", > + { .compatible = "qnapsz,power-off", > .data = &qnap_power_off_cfg, > }, Hi Ben This causes issues for devices using old DT blobs. You need to keep the old entry as well as supporting the new vendor prefix. We need to consider, is it worth the hassle of changing the prefix, since it has been in use for a long time. I think using the stock ticker is a "rule of thumb" and not a strict requirement. Andrew
On 19/02/14 23:05, Andrew Lunn wrote: >> diff --git a/drivers/power/reset/qnap-poweroff.c b/drivers/power/reset/qnap-poweroff.c >> index a75db7f..d350ece 100644 >> --- a/drivers/power/reset/qnap-poweroff.c >> +++ b/drivers/power/reset/qnap-poweroff.c >> @@ -41,7 +41,7 @@ static const struct power_off_cfg synology_power_off_cfg = { >> }; >> >> static const struct of_device_id qnap_power_off_of_match_table[] = { >> - { .compatible = "qnap,power-off", >> + { .compatible = "qnapsz,power-off", >> .data = &qnap_power_off_cfg, >> }, > > Hi Ben > > This causes issues for devices using old DT blobs. You need to keep > the old entry as well as supporting the new vendor prefix. > > We need to consider, is it worth the hassle of changing the prefix, > since it has been in use for a long time. I think using the stock > ticker is a "rule of thumb" and not a strict requirement. So, in this case it would be best to change qnapsz back to qnap in patch #1, and completely remove this patch?
On Wed, Feb 19, 2014 at 01:14:16PM +0000, Ben Peddell wrote: > On 19/02/14 23:05, Andrew Lunn wrote: > >> diff --git a/drivers/power/reset/qnap-poweroff.c b/drivers/power/reset/qnap-poweroff.c > >> index a75db7f..d350ece 100644 > >> --- a/drivers/power/reset/qnap-poweroff.c > >> +++ b/drivers/power/reset/qnap-poweroff.c > >> @@ -41,7 +41,7 @@ static const struct power_off_cfg synology_power_off_cfg = { > >> }; > >> > >> static const struct of_device_id qnap_power_off_of_match_table[] = { > >> - { .compatible = "qnap,power-off", > >> + { .compatible = "qnapsz,power-off", > >> .data = &qnap_power_off_cfg, > >> }, > > > > Hi Ben > > > > This causes issues for devices using old DT blobs. You need to keep > > the old entry as well as supporting the new vendor prefix. > > > > We need to consider, is it worth the hassle of changing the prefix, > > since it has been in use for a long time. I think using the stock > > ticker is a "rule of thumb" and not a strict requirement. > > So, in this case it would be best to change qnapsz back to qnap in > patch #1, and completely remove this patch? If we already have users of the qnap prefix, yes please. Thanks, Mark.
diff --git a/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt b/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt index af25e77..aabda51 100644 --- a/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt +++ b/Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt @@ -10,7 +10,7 @@ Synology NAS devices use a similar scheme, but a different baud rate, 9600, and a different character, '1'. Required Properties: -- compatible: Should be "qnap,power-off" or "synology,power-off" +- compatible: Should be "qnapsz,power-off" or "synology,power-off" - reg: Address and length of the register set for UART1 - clocks: tclk clock diff --git a/arch/arm/boot/dts/kirkwood-ts219.dtsi b/arch/arm/boot/dts/kirkwood-ts219.dtsi index 911f3a8..0f0892a 100644 --- a/arch/arm/boot/dts/kirkwood-ts219.dtsi +++ b/arch/arm/boot/dts/kirkwood-ts219.dtsi @@ -1,6 +1,6 @@ / { model = "QNAP TS219 family"; - compatible = "qnap,ts219", "marvell,kirkwood"; + compatible = "qnapsz,ts219", "marvell,kirkwood"; memory { device_type = "memory"; @@ -46,7 +46,7 @@ pinctrl-names = "default"; }; poweroff@12100 { - compatible = "qnap,power-off"; + compatible = "qnapsz,power-off"; reg = <0x12000 0x100>; clocks = <&gate_clk 7>; }; diff --git a/drivers/power/reset/qnap-poweroff.c b/drivers/power/reset/qnap-poweroff.c index a75db7f..d350ece 100644 --- a/drivers/power/reset/qnap-poweroff.c +++ b/drivers/power/reset/qnap-poweroff.c @@ -41,7 +41,7 @@ static const struct power_off_cfg synology_power_off_cfg = { }; static const struct of_device_id qnap_power_off_of_match_table[] = { - { .compatible = "qnap,power-off", + { .compatible = "qnapsz,power-off", .data = &qnap_power_off_cfg, }, { .compatible = "synology,power-off",
Correct the vendor prefix for qnap, which should be the stock ticker symbol qnapsz. Signed-off-by: Ben Peddell <klightspeed@killerwolves.net> Cc: Anton Vorontsov <anton@enomsg.org> Cc: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> Cc: David Woodhouse <dwmw2@infradead.org> --- Documentation/devicetree/bindings/power_supply/qnap-poweroff.txt | 2 +- arch/arm/boot/dts/kirkwood-ts219.dtsi | 4 ++-- drivers/power/reset/qnap-poweroff.c | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-)