RE: [PATCH v2 2/2] extcon-axp288: Add axp288 extcon driver support

[Pallala, Ramakrishna]

Hi Choi,

> Hi Ramakrishna,
> 
> When I apply this patch for build test on extcon-next branch, conflict happen.
> you have to implement this patchset on latest extcon-next branch.
Ok I will create the patches on git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git

> Also, This patch must need more clean-up and use latest extcon helper API.
> When you implement extcon-axp288.c, I recommend you refer to merged
> extcon drivers.
Ok.

> > --- a/drivers/extcon/Kconfig
> > +++ b/drivers/extcon/Kconfig
> > @@ -93,4 +93,11 @@ config EXTCON_SM5502
> >  	  Silicon Mitus SM5502. The SM5502 is a USB port accessory
> >  	  detector and switch.
> >
> > +config EXTCON_AXP288
> > +	tristate "AXP288 EXTCON support"
> 
> I recommend to add manufactor name of AXP288 PMIC chip.
Ok.

> > +	depends on MFD_AXP20X && USB_PHY
> > +	help
> > +	  Say Y here to enable support for USB peripheral detection
> > +	  and USB MUX switching by AXP288 PMIC.
> > +
> 
> Need to relocate EXTCON_AXP288 configuration alpabetically.
Ok.

> >  endif # MULTISTATE_SWITCH
> > diff --git a/drivers/extcon/Makefile b/drivers/extcon/Makefile index
> > 0370b42..832ad79 100644
> > --- a/drivers/extcon/Makefile
> > +++ b/drivers/extcon/Makefile
> > @@ -12,3 +12,4 @@ obj-$(CONFIG_EXTCON_MAX8997)	+= extcon-
> max8997.o
> >  obj-$(CONFIG_EXTCON_PALMAS)	+= extcon-palmas.o
> >  obj-$(CONFIG_EXTCON_RT8973A)	+= extcon-rt8973a.o
> >  obj-$(CONFIG_EXTCON_SM5502)	+= extcon-sm5502.o
> > +obj-$(CONFIG_EXTCON_AXP288)	+= extcon-axp288.o
> 
> ditto.
Ok.

> > diff --git a/drivers/extcon/extcon-axp288.c
> > b/drivers/extcon/extcon-axp288.c new file mode 100644 index
> > 0000000..2e75d45
> > --- /dev/null
> > +++ b/drivers/extcon/extcon-axp288.c
> > @@ -0,0 +1,479 @@
> > +/*
> > + * extcon-axp288.c - X-Power AXP288 PMIC extcon cable detection
> > +driver
> > + *
> > + * Copyright (C) 2015 Intel Corporation
> > + * Ramakrishna Pallala <ramakrishna.pallala@xxxxxxxxx>
> 
> Need to add 'Author' in front of name.
Ok.

> > +#define AXP288_BC_DET_STAT_REG		0x2f
> > +#define DET_STAT_MASK			0xe0
> > +#define DET_STAT_OFFSET			5
> > +#define DET_STAT_SDP			0x1
> > +#define DET_STAT_CDP			0x2
> > +#define DET_STAT_DCP			0x3
> > +
> > +#define AXP288_PS_BOOT_REASON_REG	0x2
> > +
> > +#define AXP288_PWRSRC_IRQ_CFG_REG	0x40
> > +#define PWRSRC_IRQ_CFG_MASK		0x1c
> > +
> > +#define AXP288_BC12_IRQ_CFG_REG		0x45
> > +#define BC12_IRQ_CFG_MASK		0x2
> 
> I recommend to gather axp288 registers as following by using 'enum' keyword
> and then add the mask definition using the BIT(n) macro.
> 
> enum axp288_reg {
> 	AXP288_REG_PS_STAT_REG = 0x00,
> 	AXP288_REG_BC_GLOBAL_REG = 0x2c,
> 	AXP288_REG_BC_VBUS_CNTL_REG	= 0x2d,
> 	AXP288_REG_BC_USB_STAT_REG = 0x2e,
> 	AXP288_REG_BC_DET_STAT_REG = 0x2f,
> 	...
> 
> 	AXP288_REG_END
> };
> 
> Also, I think need more description about mask/offset definition.
> 
> > +
> > +#define AXP288_PWRSRC_INTR_NUM		4
> 
> Don't need this definition. You can add AXP288_
> 
> > +
> > +#define AXP288_DRV_NAME			"axp288_extcon"
> 
> Don't need this definition. You use drive name in structure
> axp288_extcon_driver.
Ok.

> 
> > +
> > +#define AXP288_EXTCON_CABLE_SDP		"Slow-charger"
> > +#define AXP288_EXTCON_CABLE_CDP		"Charge-downstream"
> > +#define AXP288_EXTCON_CABLE_DCP		"Fast-charger"
> 
> Use the capital letter for cable name instead of small letter.
> I'll change all cable names by using capital letter.
You mean like this... "Slow-charger" => "SLOW-CHARGER"?

> 
> > +
> > +#define EXTCON_GPIO_MUX_SEL_PMIC	0
> > +#define EXTCON_GPIO_MUX_SEL_SOC		1
> > +
> > +enum {
> 
> Need to add enum name to improve readability and catch the correct meaning
> of this definitions.
Ok.

> > +	VBUS_FALLING_IRQ = 0,
> > +	VBUS_RISING_IRQ,
> > +	MV_CHNG_IRQ,
> > +	BC_USB_CHNG_IRQ,
> 
> If AXP288_PWRSRC_INTR_NUM means the number of this interrupt, you can
> add AXP288_PWRSRC_INTR_NUM value in this enum list without separate
> defintion (#define AXP288_PWRSRC_INTR_NUM).
Ok.

> > +static const char *axp288_extcon_cables[] = {
> > +	AXP288_EXTCON_CABLE_SDP,
> > +	AXP288_EXTCON_CABLE_CDP,
> > +	AXP288_EXTCON_CABLE_DCP,
> > +	NULL,
> > +};
> > +
> > +struct axp288_extcon_info {
> > +	struct platform_device *pdev;
> 
> Define 'struct device *dev' instead of 'struct platform_device *pdev'.
> I think you can support all case by using 'dev' instance in extcon driver.
Ok I will check.

> > +	struct regmap *regmap;
> > +	struct regmap_irq_chip_data *regmap_irqc;
> > +	struct axp288_extcon_pdata *pdata;
> 
> Are you sure that pdata is necessary? The axp288_extcon_pdata includes only
> gpio.
Yes, as I have plans to include more info to pdata struct.

> You have to get the gpio value by using gpiod helper funciton as following:
> You can refer other extcon driver(extcon-usb-gpio.c)/
> 
> 	devm_gpiod_get(dev, "mux_cntl");
> 
I will check.

> > +	int irq[AXP288_PWRSRC_INTR_NUM];
> > +	struct extcon_dev *edev;
> > +	struct usb_phy *otg;
> > +	struct notifier_block extcon_nb;
> > +	struct extcon_specific_cable_nb cable;
> > +	bool is_sdp;
> 
> I can't find any 'if statemenet' using 'is_sdp' variable.
> Just this driver store the false or true to 'is_sdp'.
> Remove the 'is_sdp'.
I forgot to remove this. I will fix it now.

> 
> 
> > +	bool usb_id_short;
> 
> What is meaning of usb_id_short? You need to add the description.
> 
Ok.

> > +static char *pwr_up_down_info[] = {
> 
> Add 'axp288' prefix to array name.
> 
> > +	/* bit 0 */ "Last wake caused by user pressing the power button",
> > +	/* bit 2 */ "Last wake caused by a charger insertion",
> > +	/* bit 1 */ "Last wake caused by a battery insertion",
> > +	/* bit 3 */ "Last wake caused by SOC initiated global reset",
> > +	/* bit 4 */ "Last wake caused by cold reset",
> > +	/* bit 5 */ "Last shutdown caused by PMIC UVLO threshold",
> > +	/* bit 6 */ "Last shutdown caused by SOC initiated cold off",
> > +	/* bit 7 */ "Last shutdown caused by user pressing the power
> > +button",
> 
> I don't prefer to add following comment in front of each entry.
> 
> 	/* bit x */
> 
> > +	NULL,
> > +};
> 
> Need to add more detailed description why this array is necessary.
Ok.

> > +
> > +/*
> > + * Decode and log the given "reset source indicator"
> > + * register and then clear it.
> > + */
> > +static void axp288_extcon_log_rsi(struct axp288_extcon_info *info,
> > +				char **pwrsrc_rsi_info, int reg)
> > +{
> > +	char **rsi;
> 
> What is meaning of 'rsi'?
"reset source indicator"

> > +static int axp288_extcon_registration(struct axp288_extcon_info
> > +*info) {
> > +	int ret;
> > +
> > +	/* Register with extcon */
> > +	info->edev = devm_kzalloc(&info->pdev->dev,
> > +				sizeof(struct extcon_dev), GFP_KERNEL);
> 
> Use devm_extcon_dev_allocate() instead of devm_kzalloc()
> 
> > +	if (!info->edev)
> > +		return -ENOMEM;
> > +
> > +	info->edev->name = "extcon-axp288";
> 
> Don't need to assing extcon device name. extcon_dev_register() will use the
> device name as extcon device name.
> 
> > +	info->edev->supported_cable = axp288_extcon_cables;
> > +	ret = extcon_dev_register(info->edev);
> 
> Use devm_extcon_dev_register() instead of extcon_dev_register.
> 
> > +	if (ret)
> > +		dev_err(&info->pdev->dev, "extcon registration failed!!\n");
> > +
> > +	return ret;
> > +}
> 
> Don't need seprate this function. I prefer to execute devm_extcon_* function in
> probe funtion.
Ok.
 
> > +
> > +static inline bool is_usb_host_mode(struct extcon_dev *evdev) {
> > +	return !!evdev->state;
> > +}
> 
> The state of extcon_dev means the state of all supported cables.
> If you want to get the state of specific cable, you can use
> extcon_get_cable_state().
Ok.

> > +static int axp288_handle_extcon_event(struct notifier_block *nb,
> > +				   unsigned long event, void *param) {
> > +	struct axp288_extcon_info *info =
> > +	    container_of(nb, struct axp288_extcon_info, extcon_nb);
> > +	struct extcon_dev *edev = param;
> > +	int usb_host = is_usb_host_mode(edev);
> > +
> > +	dev_info(&info->pdev->dev,
> > +		"[extcon notification] evt:USB-Host val:%s\n",
> 
> I recommend to use the standard log mesasge and use dev_dbg instead of
> dev_info.
> I think following message is very specific log.
> 	[extcon notification] evt:USB-Host val:
> 
> > +		usb_host ? "Connected" : "Disconnected");
As this handler is specifically registered for usb-host cable I think we have specific message?
If you not agree, please suggest any standard log message.

> > +
> > +	/*
> > +	 * Set usb_id_short flag to avoid running charger detection logic
> > +	 * in case usb host.
> > +	 */
> > +	info->usb_id_short = usb_host;
> > +
> > +	/*
> > +	 * Connect the USB mux to SOC in case of usb host else connect
> > +	 * it to PMIC.
> > +	 */
> > +	if (info->pdata->gpio_mux_cntl != NULL) {
> 
> As I commented, need to remove pdata.
> 
> > +		dev_dbg(&info->pdev->dev,
> > +			"usb_id_short=%d\n", info->usb_id_short);
> 
> Ditto. Need to modify log message.
This message is redundant. I will remove it.

> > +static int axp288_init_gpio_mux_cntl(struct axp288_extcon_info *info)
> > +{
> > +	int ret;
> > +
> > +	ret = gpio_request(desc_to_gpio(info->pdata->gpio_mux_cntl),
> "USB_MUX");
> > +	if (ret < 0) {
> > +		dev_err(&info->pdev->dev,
> > +			"usb mux gpio request failed:gpio=%d\n",
> > +			desc_to_gpio(info->pdata->gpio_mux_cntl));
> 
> You can change the error message as following:
> 	"Failed to request the gpio"
> 
> > +		return ret;
> > +	}
> > +	gpiod_direction_output(info->pdata->gpio_mux_cntl,
> > +					EXTCON_GPIO_MUX_SEL_PMIC);
> > +
> > +	info->extcon_nb.notifier_call = axp288_handle_extcon_event;
> > +	ret = extcon_register_interest(&info->cable, NULL,
> > +				"USB-Host", &info->extcon_nb);
> 
> NAK.
> 
> Why do you register notifier_chain in this driver?
> 
> Only extcon provider driver can be included int drivers/extcon/ directory.
> extcon_register_interest() function should be used for extcon consumer driver.
> 
> > +	if (ret) {
> > +		dev_err(&info->pdev->dev, "failed to register extcon
> notifier\n");
> > +		return ret;
> > +	}
This driver acts as consumer for charger detection logic and also acts as consumer for OTG detection for mux switching.
And also to avoid false charger notification for Host mode case.

> > +static int axp288_extcon_probe(struct platform_device *pdev) {
> > +	struct axp288_extcon_info *info;
> > +	struct axp20x_dev *axp20x = dev_get_drvdata(pdev->dev.parent);
> > +	int ret, i, pirq;
> > +
> > +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> > +	if (!info)
> > +		return -ENOMEM;
> > +
> > +	info->pdev = pdev;
> > +	info->regmap = axp20x->regmap;
> > +	info->regmap_irqc = axp20x->regmap_irqc;
> 
> If axp288_extcon_info structure includes the axp20x_dev structure,
> info->regmap and info->regmap_irqc is not necessary.
Then I would need "axp20x_dev->" every time I use regmap...

> 
> > +	info->pdata = pdev->dev.platform_data;
> 
> I don't prefer to use platform_data. You have to get the platform data from
> devicetree by using OF helper function.
> 
My platform don't use device tree....it uses SFI or ACPI based approach...I cannot remove this at the moment..

> > +
> > +	if (!info->pdata) {
> > +		/* TODO: Try ACPI provided pdata via device properties */
> > +		if (!device_property_present(&pdev->dev,
> > +					"axp288_extcon_data\n"))
> > +			dev_err(&pdev->dev, "no platform data\n");
> > +		return -ENODEV;
> > +	}
> > +	platform_set_drvdata(pdev, info);
> > +
> > +	axp288_extcon_log_rsi(info, pwr_up_down_info,
> > +				AXP288_PS_BOOT_REASON_REG);
> 
> I need more detailed description.
This functions basically logs the system power up/down reason for debug purpose. Also there is user space daemon which parses these logs in my board.


> > +
> > +	/* Register extcon device */
> > +	ret = axp288_extcon_registration(info);
> > +	if (ret < 0)
> > +		goto extcon_reg_failed;
> 
> Remove the call of axp288_extcon_registration()
> 
Ok.

> > +
> > +	/* Get otg transceiver phy */
> > +	info->otg = usb_get_phy(USB_PHY_TYPE_USB2);
> > +	if (IS_ERR(info->otg)) {
> > +		dev_warn(&info->pdev->dev, "Failed to get otg
> transceiver!!\n");
> > +		ret = PTR_ERR(info->otg);
> > +		goto otg_reg_failed;
> > +	}
> > +
> > +	for (i = 0; i < AXP288_PWRSRC_INTR_NUM; i++) {
> > +		pirq = platform_get_irq(pdev, i);
> > +		info->irq[i] = regmap_irq_get_virq(info->regmap_irqc, pirq);
> > +		if (info->irq[i] < 0) {
> > +			dev_warn(&info->pdev->dev,
> > +				"regmap_irq get virq failed for IRQ %d: %d\n",
> > +				pirq, info->irq[i]);
> > +			info->irq[i] = -1;
> > +			goto intr_reg_failed;
> > +		}
> > +		ret = request_threaded_irq(info->irq[i],
> 
> Use devm_request_threaded_irq().
> 
> > +				NULL, axp288_extcon_isr,
> > +				IRQF_ONESHOT, AXP288_DRV_NAME, info);
> 
> You can use pdev->name instead of AXP288_DRV_NAME.
Ok.

> > +static int axp288_extcon_remove(struct platform_device *pdev) {
> > +	struct axp288_extcon_info *info = platform_get_drvdata(pdev);
> > +	int i;
> > +
> > +	for (i = 0; i < AXP288_PWRSRC_INTR_NUM; i++)
> > +		free_irq(info->irq[i], info);
> 
> Don't need to free interrupt if you use devm_request_threaded_irq().
Ok.


> > +	usb_put_phy(info->otg);
> > +	extcon_dev_unregister(info->edev);
> 
> Don't need to unregister extcon device if you use devm_extcon_*.
Ok.

Thanks,
Ram
--
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/