Re: [PATCH v14 1/4] usb: gadget: Introduce the usb charger framework

[Felipe Balbi]

Hi,

Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
> +static ssize_t charger_state_show(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	struct usb_charger *uchger = dev_to_uchger(dev);
> +	int cnt;
> +
> +	switch (uchger->state) {
> +	case USB_CHARGER_PRESENT:
> +		cnt = sprintf(buf, "%s\n", "PRESENT");
> +		break;
> +	case USB_CHARGER_REMOVE:
> +		cnt = sprintf(buf, "%s\n", "REMOVE");
> +		break;
> +	default:
> +		cnt = sprintf(buf, "%s\n", "UNKNOWN");
> +		break;

are these the only states we need? Don't we need at least "CHARGING" and
"ERROR" or something like that? Maybe those are exposed elsewhere,
dunno.

> +static int __usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
> +					       enum usb_charger_type type,
> +					       unsigned int cur_limit)
> +{
> +	if (WARN(!uchger, "charger can not be NULL"))
> +		return -EINVAL;

IIRC, I mentioned that this should assume charger to be a valid
pointer. Look at this, for a second:

You check that you have a valid pointer here...

> +int usb_charger_set_cur_limit_by_type(struct usb_charger *uchger,
> +				      enum usb_charger_type type,
> +				      unsigned int cur_limit)
> +{
> +	int ret;
> +
> +	if (WARN(!uchger, "charger can not be NULL"))
> +		return -EINVAL;

... and here, before calling the other function. This is the only place
which should check for valid uchger.

> +	mutex_lock(&uchger->lock);
> +	ret = __usb_charger_set_cur_limit_by_type(uchger, type, cur_limit);
> +	mutex_unlock(&uchger->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_set_cur_limit_by_type);
> +
> +/*
> + * usb_charger_set_cur_limit() - Set the current limitation.
> + * @uchger - the usb charger instance.
> + * @cur_limit_set - the current limitation.
> + */
> +int usb_charger_set_cur_limit(struct usb_charger *uchger,
> +			      struct usb_charger_cur_limit *cur_limit_set)
> +{
> +	if (WARN(!uchger || !cur_limit_set, "charger or setting can't be NULL"))
> +		return -EINVAL;

I can see a pattern here. Not *all* errors need a splat. Sometimes a
simple pr_err() or dev_err() (when you have a valid dev pointer) are
enough.

> diff --git a/include/linux/usb/charger.h b/include/linux/usb/charger.h
> new file mode 100644
> index 0000000..d2e745e
> --- /dev/null
> +++ b/include/linux/usb/charger.h
> @@ -0,0 +1,164 @@
> +#ifndef __LINUX_USB_CHARGER_H__
> +#define __LINUX_USB_CHARGER_H__
> +
> +#include <uapi/linux/usb/ch9.h>
> +#include <uapi/linux/usb/charger.h>
> +
> +#define CHARGER_NAME_MAX 30
> +
> +/* Current limitation by charger type */
> +struct usb_charger_cur_limit {
> +	unsigned int sdp_cur_limit;
> +	unsigned int dcp_cur_limit;
> +	unsigned int cdp_cur_limit;
> +	unsigned int aca_cur_limit;
> +};
> +
> +struct usb_charger_nb {
> +	struct notifier_block	nb;
> +	struct usb_charger	*uchger;
> +};
> +

please add KernelDoc here. And mention the fields which aren't supposed
to be accessed directly but should rely on the accessor functions. At
least type and state prefer to be accessed by their respective
getter/setter methods.

> +struct usb_charger {
> +	char			name[CHARGER_NAME_MAX];
> +	struct list_head	list;
> +	struct raw_notifier_head	uchger_nh;
> +	/* protect the notifier head and charger */
> +	struct mutex		lock;
> +	int			id;
> +	enum usb_charger_type	type;
> +	enum usb_charger_state	state;
> +
> +	/* for supporting extcon usb gpio */
> +	struct extcon_dev	*extcon_dev;
> +	struct usb_charger_nb	extcon_nb;
> +
> +	/* for supporting usb gadget */
> +	struct usb_gadget	*gadget;
> +	enum usb_device_state	old_gadget_state;
> +
> +	/* for supporting power supply */
> +	struct power_supply	*psy;
> +
> +	/* user can get charger type by implementing this callback */
> +	enum usb_charger_type	(*get_charger_type)(struct usb_charger *);
> +	/*
> +	 * charger detection method can be implemented if you need to
> +	 * manually detect the charger type.
> +	 */
> +	enum usb_charger_type	(*charger_detect)(struct usb_charger *);
> +
> +	/* current limitation */
> +	struct usb_charger_cur_limit	cur_limit;
> +	/* to check if it is needed to change the SDP charger default current */
> +	unsigned int		sdp_default_cur_change;
> +};

-- 
balbi

Attachment: signature.asc
Description: PGP signature