linux-kernel - Re: [PATCH v3 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20140723155016.GM23210@lee--X1>
Date:	Wed, 23 Jul 2014 16:50:16 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Peter Griffin <peter.griffin@...aro.org>
Cc:	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	maxime.coquelin@...com, patrice.chotard@...com,
	srinivas.kandagatla@...il.com, devicetree@...r.kernel.org,
	balbi@...com, linux-usb@...r.kernel.org,
	linux-omap@...r.kernel.org, peppe.cavallaro@...com
Subject: Re: [PATCH v3 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3
 HC

On Wed, 23 Jul 2014, Peter Griffin wrote:

> This patch adds the ST glue logic to manage the DWC3 HC
> on STiH407 SoC family. It manages the powerdown signal,
> and configures the internal glue logic and syscfg registers.
> 
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@...com>
> Signed-off-by: Peter Griffin <peter.griffin@...aro.org>
> ---
>  drivers/usb/dwc3/Kconfig   |   9 ++
>  drivers/usb/dwc3/Makefile  |   1 +
>  drivers/usb/dwc3/dwc3-st.c | 338 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 348 insertions(+)
>  create mode 100644 drivers/usb/dwc3/dwc3-st.c

[...]

> +/*
> + * For all fields in USB2_VBUS_MNGMNT_SEL1
> + * 2’b00 : Override value from Reg 0x30 is selected
> + * 2’b01 : utmiotg_<signal_name> from usb3_top is selected
> + * 2’b10 : pipew_<signal_name> from PIPEW instance is selected
> + * 2’b11 : value is 1'b0
> + */
> +#define REG30	0x0
> +#define UTMIOTG	0x1
> +#define PIPEW	0x2
> +#define ZERO	0x3

Possible register values are usually prefixed with something
descriptive which identifies them.

USB2_VBUS_ looks appropriate here.

[...]

> +/**
> + * struct st_dwc3 - st-dwc3 driver private structure
> + * @dwc3:		platform device pointer
> + * @dev:		device pointer
> + * @glue_base		ioaddr for the glue registers
> + * @regmap		regmap pointer for getting syscfg
> + * @syscfg_reg_off	usb syscfg control offset
> + * @dr_mode		drd static host/device config
> + * @rstc_pwrdn		rest controller for powerdown signal
> + * @rstc_rst		reset controller for softreset signal

Some of these have ':', some of them don't.  I suggest you standardise
to 'all do'.

> + *

Superflous line in comment.

> + */
> +

Superflous '\n'.

Take a look how you did the function headers below.

[...]

> +static int st_dwc3_drd_init(struct st_dwc3 *dwc3_data)
> +{
> +	u32 val;
> +	int err;
> +
> +	err = regmap_read(dwc3_data->regmap, dwc3_data->syscfg_reg_off, &val);
> +	if (err)
> +		return err;
> +
> +	switch (dwc3_data->dr_mode) {
> +	case USB_DR_MODE_PERIPHERAL:
> +		val |= USB_SET_PORT_DEVICE;
> +		dev_dbg(dwc3_data->dev, "Configuring as Device\n");
> +		break;
> +
> +	case USB_DR_MODE_HOST:
> +		val &= USB_HOST_DEFAULT_MASK;
> +		dev_dbg(dwc3_data->dev, "Configuring as Host\n");
> +		break;
> +
> +	default:
> +		dev_err(dwc3_data->dev, "Unsupported mode of operation %d\n"
> +			, dwc3_data->dr_mode);

',' should be on the line above.

> +		return -EINVAL;
> +	}
> +
> +	return regmap_write(dwc3_data->regmap, dwc3_data->syscfg_reg_off, val);
> +}

All of this stuff is pretty minor.

Once fixed apply my Ack on the next revision:

Acked-by: Lee Jones <lee.jones@...aro.org>

-- 
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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/