On Fri, Nov 30, 2018 at 11:29:13PM +0000, Matthew Starr wrote:
> The implementation for setting the over current polarity has always been
> the over-current-active-high property. The problem with this is there
> is no way to enable over current active low polarity since the default
> state of the register bit that controls the over current polarity is a 0
> which means active high. This value never actually did anything since
> active high is already the default state. Also it is not used in any
> device tree source in the kernel. The default register bit state was
> verified by checking the i.MX6Q and i.MX7Dual reference manuals.
>
> The change replaces the over-current-active-high property with the
> over-current-active-low property. Without this property the over
> current polarity will be active high by default like it always has been.
> With the property the over current polarity will be active low.
I'd prefer to do the following instead: Keep over-current-active-high in
the binding and encourage users to specify (exactly) one of them by
issuing a warning if there is none and keep the bit as configured by the
bootloader (or by reset if the bootloader didn't touch it).
> Signed-off-by: Matthew Starr <mstarr@xxxxxxxxxxxxx>
> ---
> .../devicetree/bindings/usb/ci-hdrc-usb2.txt | 4 ++--
> drivers/usb/chipidea/ci_hdrc_imx.c | 2 +-
> drivers/usb/chipidea/usbmisc_imx.c | 12 ++++++++----
> 3 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> index 529e51879fb2..3dee58037839 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> @@ -87,8 +87,8 @@ i.mx specific properties
> - fsl,usbmisc: phandler of non-core register device, with one
> argument that indicate usb controller index
> - disable-over-current: disable over current detect
> -- over-current-active-high: over current signal polarity is high active,
> - typically over current signal polarity is low active.
> +- over-current-active-low: over current signal polarity is low active,
> + the default signal polarity is high active.
> - external-vbus-divider: enables off-chip resistor divider for Vbus
>
> Example:
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 09b37c0d075d..f7069544fb42 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -135,7 +135,7 @@ static struct imx_usbmisc_data *usbmisc_get_init_data(struct device *dev)
> if (of_find_property(np, "disable-over-current", NULL))
> data->disable_oc = 1;
>
> - if (of_find_property(np, "over-current-active-high", NULL))
> + if (of_find_property(np, "over-current-active-low", NULL))
> data->oc_polarity = 1;
>
> if (of_find_property(np, "external-vbus-divider", NULL))
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c b/drivers/usb/chipidea/usbmisc_imx.c
> index def80ff547e4..b39ba76b874f 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -341,10 +341,11 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data *data)
> if (data->disable_oc) {
> reg |= MX6_BM_OVER_CUR_DIS;
> } else if (data->oc_polarity == 1) {
> - /* High active */
> - reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
> - } else {
> + /* Low active */
> reg &= ~(MX6_BM_OVER_CUR_DIS);
> + reg |= MX6_BM_OVER_CUR_POLARITY;
> + } else {
> + reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
> }
> writel(reg, usbmisc->base + data->index * 4);
>
> @@ -445,7 +446,10 @@ static int usbmisc_imx7d_init(struct imx_usbmisc_data *data)
> if (data->disable_oc) {
> reg |= MX6_BM_OVER_CUR_DIS;
> } else if (data->oc_polarity == 1) {
> - /* High active */
> + /* Low active */
> + reg &= ~(MX6_BM_OVER_CUR_DIS);
> + reg |= MX6_BM_OVER_CUR_POLARITY;
> + else {
> reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
You break machines here that use active low signaling but don't use the
new property (yet).
Also it's not nice to inverse the sematic of oc_polarity. Before it was
at least intuitively right that = 1 means active high.
As I improved my patch while being offline and before I saw your patch,
I'm sending out my patch as a better alternative.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
