Re: [PATCH v5] extcon-axp288: Add axp288 extcon driver support

[Chanwoo Choi]

Hi Ram,

On Tue, Apr 28, 2015 at 8:20 PM, Pallala, Ramakrishna
<ramakrishna.pallala@xxxxxxxxx> wrote:
>> On Tue, 28 Apr 2015, Pallala, Ramakrishna wrote:
>>
>> > > > This patch adds the extcon support for AXP288 PMIC which has the
>> > > > BC1.2 charger detection capability. Additionally it also adds the
>> > > > USB mux switching support b/w SOC and PMIC based on GPIO control.
>> > > >
>> > > > Signed-off-by: Ramakrishna Pallala <ramakrishna.pallala@xxxxxxxxx>
>> > > > ---
>> > > >  drivers/extcon/Kconfig         |    7 +
>> > > >  drivers/extcon/Makefile        |    1 +
>> > > >  drivers/extcon/extcon-axp288.c |  399
>> > > ++++++++++++++++++++++++++++++++++++++++
>> > > >  include/linux/mfd/axp20x.h     |    9 +
>> > > >  4 files changed, 416 insertions(+)  create mode 100644
>> > > > drivers/extcon/extcon-axp288.c
>> > > >
>> > > > diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig index
>> > > > fdc0bf0..2305edc 100644
>> > > > --- a/drivers/extcon/Kconfig
>> > > > +++ b/drivers/extcon/Kconfig
>> > > > @@ -28,6 +28,13 @@ config EXTCON_ARIZONA
>> > > >           with Wolfson Arizona devices. These are audio CODECs with
>> > > >           advanced audio accessory detection support.
>> > > >
>> > > > +config EXTCON_AXP288
>> > > > +       tristate "X-Power AXP288 EXTCON support"
>> > > > +       depends on MFD_AXP20X && USB_PHY
>> > > > +       help
>> > > > +         Say Y here to enable support for USB peripheral detection
>> > > > +         and USB MUX switching by X-Power AXP288 PMIC.
>> > > > +
>> > > >  config EXTCON_GPIO
>> > > >         tristate "GPIO extcon support"
>> > > >         depends on GPIOLIB
>> > > > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile
>> > > > index
>> > > > 9204114..ba787d0 100644
>> > > > --- a/drivers/extcon/Makefile
>> > > > +++ b/drivers/extcon/Makefile
>> > > > @@ -5,6 +5,7 @@
>> > > >  obj-$(CONFIG_EXTCON)           += extcon.o
>> > > >  obj-$(CONFIG_EXTCON_ADC_JACK)  += extcon-adc-jack.o
>> > > >  obj-$(CONFIG_EXTCON_ARIZONA)   += extcon-arizona.o
>> > > > +obj-$(CONFIG_EXTCON_AXP288)    += extcon-axp288.o
>> > > >  obj-$(CONFIG_EXTCON_GPIO)      += extcon-gpio.o
>> > > >  obj-$(CONFIG_EXTCON_MAX14577)  += extcon-max14577.o
>> > > >  obj-$(CONFIG_EXTCON_MAX77693)  += extcon-max77693.o
>> > > > diff --git a/drivers/extcon/extcon-axp288.c
>> > > > b/drivers/extcon/extcon-axp288.c new file mode 100644 index
>> > > > 0000000..91d764c
>> > > > --- /dev/null
>> > > > +++ b/drivers/extcon/extcon-axp288.c
>> > > > @@ -0,0 +1,399 @@
>> > > > +/*
>> > > > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
>> > > > +driver
>> > > > + *
>> > > > + * Copyright (C) 2014 Intel Corporation
>> > > > + * Author: Ramakrishna Pallala <ramakrishna.pallala@xxxxxxxxx>
>> > > > + *
>> > > > + * This program is free software; you can redistribute it and/or
>> > > > +modify
>> > > > + * it under the terms of the GNU General Public License version 2
>> > > > +as
>> > > > + * published by the Free Software Foundation.
>> > > > + *
>> > > > + * This program is distributed in the hope that it will be
>> > > > +useful,
>> > > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> > > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> > > > + * GNU General Public License for more details.
>> > > > + */
>> > > > +
>> > > > +#include <linux/module.h>
>> > > > +#include <linux/kernel.h>
>> > > > +#include <linux/io.h>
>> > > > +#include <linux/slab.h>
>> > > > +#include <linux/interrupt.h>
>> > > > +#include <linux/platform_device.h> #include <linux/property.h>
>> > > > +#include <linux/usb/phy.h> #include <linux/notifier.h> #include
>> > > > +<linux/extcon.h> #include <linux/regmap.h> #include
>> > > > +<linux/gpio.h> #include <linux/gpio/consumer.h> #include
>> > > > +<linux/mfd/axp20x.h>
>> >
>> > [snip]
>> >
>> > > > +module_platform_driver(axp288_extcon_driver);
>> > > > +
>> > > > +MODULE_AUTHOR("Ramakrishna Pallala
>> > > > +<ramakrishna.pallala@xxxxxxxxx>");
>> > > > +MODULE_DESCRIPTION("X-Powers AXP288 extcon driver");
>> > > > +MODULE_LICENSE("GPL v2");
>> > > > diff --git a/include/linux/mfd/axp20x.h
>> > > > b/include/linux/mfd/axp20x.h index dfabd6d..81152e2 100644
>> > > > --- a/include/linux/mfd/axp20x.h
>> > > > +++ b/include/linux/mfd/axp20x.h
>> > > > @@ -275,4 +275,13 @@ struct axp20x_fg_pdata {
>> > > >         int thermistor_curve[MAX_THERM_CURVE_SIZE][2];
>> > > >  };
>> > > >
>> > > > +#define AXP288_EXTCON_CABLE_SDP                "SLOW-CHARGER"
>> > > > +#define AXP288_EXTCON_CABLE_CDP                "CHARGE-
>> > > DOWNSTREAM"
>> > > > +#define AXP288_EXTCON_CABLE_DCP                "FAST-CHARGER"
>> > >
>> > > #defines are meant to make things *easier* for humans to read. It is
>> > > my opinion that these negatively impact the readability of the code.
>> > These macros will be used in extcon and charger drivers. How can I make it
>> more readable?
>>
>> If the things you're defining are more readable than the defines themselves
>> you're doing the opposite of what I'd expect.
>>
>> How about:
>>
>>   AXP288_EXTCON_SLOW_CHARGER_CABLE
>>   AXP288_EXTCON_FAST_CHARGER_CABLE
>>   AXP288_EXTCON_CHARGE_DOWNSTREAM_CABLE
>
> Ok I will change the names similar to the above ones.

This cable names are used in only extcon driver.
I think that you don't need to define them on header file.
You better to move this definition to extcon driver.

Also, I'll review your patch on other mail.

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