On Wed, 24 Jun 2015, Hans de Goede wrote:
> On 24-06-15 13:23, Lee Jones wrote:
> >>Add a cell for the usb power_supply part of the axp20x PMICs.
> >
> >Why are you duplicating the subject line?
>
> Heh, because some maintainers insist that the main-body part of the
> commit message must stand by itself, without needing the subject to be
> understandable...
Some Maintainers are crazy. ;)
> >>Note that this cell is only for the usb power_supply part and not the
> >>ac-power / battery-charger / rtc-backup-bat-charger bits.
> >>
> >>Depending on the board each of those must be enabled / disabled separately
> >>in devicetree as most boards do not use all 4. So in dt each one needs its
> >>own child-node of the axp20x node. Another reason for using separate child
> >>nodes for each is so that other devicetree nodes can have a power-supply
> >>property with a phandle referencing a node representing a single
> >>power-supply.
> >>
> >>The decision to use a separate devicetree node for each is reflected on
> >>the kernel side by each getting its own mfd-cell / platform_device and
> >>platform-driver.
> >
> >You don't really need to say any of this, as this is the 'norm'.
>
> I agree it should be the norm, but I'm not sure if it actually is, while
> working on this I've seen several drivers which instantiate multiple
> power-supply class devices from a single mfd-cell. And this is what
> Bruno's original patches did, so I would prefer to keep this
Up to you, but it's really not required. No one else says this stuff.
> >What you didn't mention however, is that you're taking the opportunity
> >to fix some formatting issues and that there are no functional changes
> >in these lines.
>
> That is the end result of your request to change the indentation to
> avoid line-wrapping :)
Fine, you can add my Suggested-by too then [don't actually do that],
but regardless of where the idea came from you should still tell us
what the patch does in the commit log. I was looking for functional
differences within those formatting changes. If I wanted to be really
religious about it I would [rightly] say "don't mix functional changes
with clean-ups", but instead I'll politely ask for a quick mention in
the commit log when you re-submit. Aren't I nice?! ;)
> >>Cc: Bruno Prémont <bonbons@xxxxxxxxxxxxxxxxx>
> >>Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >>---
> >>Changes in v2:
> >>-Use DEFINE_RES_IRQ_NAMED
> >>-Change indentation of axp20x_cells initializers to avoid line wrapping
> >>---
> >> drivers/mfd/axp20x.c | 20 ++++++++++++++++----
> >> 1 file changed, 16 insertions(+), 4 deletions(-)
> >
> >Patch looks okay however:
> >
> >Acked-by: Lee Jones <lee.jones@xxxxxxxxxx>
>
> Thanks.
>
> Regards,
>
> Hans
>
>
> >
> >>diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> >>index f9a3c2d..ca4a604 100644
> >>--- a/drivers/mfd/axp20x.c
> >>+++ b/drivers/mfd/axp20x.c
> >>@@ -113,6 +113,13 @@ static struct resource axp20x_pek_resources[] = {
> >> },
> >> };
> >>
> >>+static struct resource axp20x_usb_power_supply_resources[] = {
> >>+ DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_PLUGIN, "VBUS_PLUGIN"),
> >>+ DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_REMOVAL, "VBUS_REMOVAL"),
> >>+ DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_VALID, "VBUS_VALID"),
> >>+ DEFINE_RES_IRQ_NAMED(AXP20X_IRQ_VBUS_NOT_VALID, "VBUS_NOT_VALID"),
> >>+};
> >>+
> >> static struct resource axp22x_pek_resources[] = {
> >> {
> >> .name = "PEK_DBR",
> >>@@ -363,11 +370,16 @@ static const struct regmap_irq_chip axp288_regmap_irq_chip = {
> >>
> >> static struct mfd_cell axp20x_cells[] = {
> >> {
> >>- .name = "axp20x-pek",
> >>- .num_resources = ARRAY_SIZE(axp20x_pek_resources),
> >>- .resources = axp20x_pek_resources,
> >>+ .name = "axp20x-pek",
> >>+ .num_resources = ARRAY_SIZE(axp20x_pek_resources),
> >>+ .resources = axp20x_pek_resources,
> >> }, {
> >>- .name = "axp20x-regulator",
> >>+ .name = "axp20x-regulator",
> >>+ }, {
> >>+ .name = "axp20x-usb-power-supply",
> >>+ .of_compatible = "x-powers,axp202-usb-power-supply",
> >>+ .num_resources = ARRAY_SIZE(axp20x_usb_power_supply_resources),
> >>+ .resources = axp20x_usb_power_supply_resources,
> >> },
> >> };
> >>
> >
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
