{"thread":{"id":"31015","subject":"Re: [PATCH 0/7] Add support for Freescale's mc34708 to mc13xxx driver","startedAt":"2012-07-13T08:48:54Z","lastAt":"2012-07-13T19:39:34Z","messageCount":2,"participants":["Uwe Kleine-König","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"195000","messageId":"20120713084854.GD592@pengutronix.de","threadId":"31015","inReplyTo":"11987916.rOoNGcb1ly@dev1","subject":"Re: [PATCH 0/7] Add support for Freescale's mc34708 to mc13xxx driver","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2012-07-13T08:48:54Z","receivedAt":"2012-07-13T08:48:54Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hello,\n\n[I added git@vger.k.o to Cc: please strip the recipents accordingly if\nyou reply.]\n\nOn Fri, Jul 13, 2012 at 09:02:56AM +1000, Marc Reilly wrote:\n> Hi Uwe,\n> \n> > This series was tested on a Phytec pcm038 (mc13783 on spi) using\n> > traditional boot (i.e. not dt) and on a i.MX53 based machine (mc34708 on\n> > i2c) using dt boot.\n> > \n> > Philippe's patches are already in next, they are just included here for\n> > those who want to test the patches. The 'mfd/mc13xxx: drop modifying\n> > driver's id_table in probe' was already sent out yesterday and is\n> > included here because the last patch depends on it.\n> > \n> \n> For all patches (that don't already have it):\n> Acked-by: Marc Reilly <marc@cpdesign.com.au>\nThanks.\n\n> If for some reason you do a V2:\n> - In patch 6 moves line \"#define MC13XXX_NUMREGS 0x3f\" around, is this \n> necessary?\nIt doesn't move it around, that's only how it looks. I removed enum\nmc13xxx_id (above MC13XXX_NUMREGS) and added struct mc13xxx_variant\n(below MC13XXX_NUMREGS). Git choosed to use the closing brace of enum\nmc13xxx_id and struct mc13xxx_variant respectively as context (together\nwith the following empty line).\n(For the new readers, here is how git represented the relevant part:\n\n #include <linux/regmap.h>\n #include <linux/mfd/mc13xxx.h>\n\n-enum mc13xxx_id {\n-\tMC13XXX_ID_MC13783,\n-\tMC13XXX_ID_MC13892,\n-\tMC13XXX_ID_INVALID,\n+#define MC13XXX_NUMREGS 0x3f\n+\n+struct mc13xxx;\n+\n+struct mc13xxx_variant {\n+\tconst char *name;\n+\tvoid (*print_revision)(struct mc13xxx *mc13xxx, u32 revision);\n };\n\n-#define MC13XXX_NUMREGS 0x3f\n+extern struct mc13xxx_variant\n+\t\tmc13xxx_variant_mc13783,\n+\t\tmc13xxx_variant_mc13892;\n\n struct mc13xxx {\n \tstruct regmap *regmap;\n...\n)\n\nThe following would be an equivalent and (for humans) easier to review\npatch:\n\n #include <linux/regmap.h>\n #include <linux/mfd/mc13xxx.h>\n\n-enum mc13xxx_id {\n-\tMC13XXX_ID_MC13783,\n-\tMC13XXX_ID_MC13892,\n-\tMC13XXX_ID_INVALID,\n-};\n-\n #define MC13XXX_NUMREGS 0x3f\n\n+struct mc13xxx;\n+\n+struct mc13xxx_variant {\n+\tconst char *name;\n+\tvoid (*print_revision)(struct mc13xxx *mc13xxx, u32 revision);\n+};\n+\n+extern struct mc13xxx_variant\n+\t\tmc13xxx_variant_mc13783,\n+\t\tmc13xxx_variant_mc13892;\n+\n struct mc13xxx {\n \tstruct regmap *regmap;\n...\n\nBut as this touches 17 lines compared to only 15 using the way git\nchoosed to represent patch 6, git used the smaller representation. (I'm\nnot sure this is the correct reason, but at least it sounds sensible.)\nUsually this metric is sane, but here it's not. I don't know if you can\ndo anything about it? E.g. take the number of +, - and context blocks\ninto account. Then it would be 5 for the patch above vs. 7 for the\nway git did it.\nOr weight a context line containing\n\n\t#define MC13XXX_NUMREGS 0x3f\n\nmore than two lines one of which is empty and the other only contains a\n}?\n\n> - Patch 4 should come last, ie after \"add support for mc34708\"\nYeah, but it doesn't break bisectibility, and as the rtc patches go in via a\ndifferent tree (in fact akpm already took them) it doesn't matter much.\n\nBest regards and thanks for your feedback\nUwe\n\n-- \nPengutronix e.K.                           | Uwe Kleine-König            |\nIndustrial Linux Solutions                 | http://www.pengutronix.de/  |\n"},{"id":"195023","messageId":"7vpq7z8n7t.fsf@alter.siamese.dyndns.org","threadId":"31015","inReplyTo":"20120713084854.GD592@pengutronix.de","subject":"Re: [PATCH 0/7] Add support for Freescale's mc34708 to mc13xxx driver","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-13T19:39:34Z","receivedAt":"2012-07-13T19:39:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:\n\n> It doesn't move it around, that's only how it looks. I removed enum\n> mc13xxx_id (above MC13XXX_NUMREGS) and added struct mc13xxx_variant\n> (below MC13XXX_NUMREGS). Git choosed to use the closing brace of enum\n> mc13xxx_id and struct mc13xxx_variant respectively as context (together\n> with the following empty line).\n> (For the new readers, here is how git represented the relevant part:\n>\n>  #include <linux/regmap.h>\n>  #include <linux/mfd/mc13xxx.h>\n>\n> -enum mc13xxx_id {\n> -\tMC13XXX_ID_MC13783,\n> -\tMC13XXX_ID_MC13892,\n> -\tMC13XXX_ID_INVALID,\n> +#define MC13XXX_NUMREGS 0x3f\n> +\n> +struct mc13xxx;\n> +\n> +struct mc13xxx_variant {\n> +\tconst char *name;\n> +\tvoid (*print_revision)(struct mc13xxx *mc13xxx, u32 revision);\n>  };\n>\n> -#define MC13XXX_NUMREGS 0x3f\n> +extern struct mc13xxx_variant\n> +\t\tmc13xxx_variant_mc13783,\n> +\t\tmc13xxx_variant_mc13892;\n>\n>  struct mc13xxx {\n>  \tstruct regmap *regmap;\n> ...\n> )\n>\n> The following would be an equivalent and (for humans) easier to review\n> patch:\n>\n>  #include <linux/regmap.h>\n>  #include <linux/mfd/mc13xxx.h>\n>\n> -enum mc13xxx_id {\n> -\tMC13XXX_ID_MC13783,\n> -\tMC13XXX_ID_MC13892,\n> -\tMC13XXX_ID_INVALID,\n> -};\n> -\n>  #define MC13XXX_NUMREGS 0x3f\n>\n> +struct mc13xxx;\n> +\n> +struct mc13xxx_variant {\n> +\tconst char *name;\n> +\tvoid (*print_revision)(struct mc13xxx *mc13xxx, u32 revision);\n> +};\n> +\n> +extern struct mc13xxx_variant\n> +\t\tmc13xxx_variant_mc13783,\n> +\t\tmc13xxx_variant_mc13892;\n> +\n>  struct mc13xxx {\n>  \tstruct regmap *regmap;\n> ...\n>\n> But as this touches 17 lines compared to only 15 using the way git\n> choosed to represent patch 6, git used the smaller representation.\n\nYes.  Useful information bits per line count is the primary thing\nour default xdiff based output pays attention to (e.g. we coalesce\ntwo adjacent hunks that are one missing context line apart into one\nlarger hunk by removing the \"@@ linenum @@\" line from the beginning\nof the latter hunk for this reason).\n\n> Usually this metric is sane, but here it's not. I don't know if you can\n> do anything about it? E.g. take the number of +, - and context blocks\n> into account. Then it would be 5 for the patch above vs. 7 for the\n> way git did it.\n> Or weight a context line containing\n>\n> \t#define MC13XXX_NUMREGS 0x3f\n>\n> more than two lines one of which is empty and the other only contains a\n> }?\n\n\"GNU diff\" gives the same output as ours, and \"git diff --patience\"\ngives more redundant (it wasts lines by removing \"};\" and then later\nadding \"};\" back) output.  I think this is because \"patience\" pays\nmore attention to key off unique lines in the range (e.g. the line\n\"#define MC13XXX_NUMREGS 0x3f\" appears only once in the preimage and\nalso in the postimage, so it must pair with each other).\n"}]}