From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergei Shtylyov Date: Wed, 10 Apr 2013 18:51:45 +0000 Subject: Re: [PATCH v3 7/9] rcar-phy: add platform data Message-Id: <5165B4C1.80203@cogentembedded.com> List-Id: References: <201304100237.50334.sergei.shtylyov@cogentembedded.com> In-Reply-To: <201304100237.50334.sergei.shtylyov@cogentembedded.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-sh@vger.kernel.org On 04/10/2013 10:40 PM, Felipe Balbi wrote: > >>>>>> Currently the driver hard-codes USBPCTRL0 register to 0. It is wrong since this >>>>>> register contains board-specific USB ports configuration and so its value should >>>>>> be somehow passed via the platform data. Add file with >>>>>> the USBPCTRL0 bit #define's and 'struct rcar_phy_platform_data' containing the >>>>>> value to be set by the driver to that register. >>>>>> Signed-off-by: Sergei Shtylyov >>>>>> Acked-by: Kuninori Morimoto >>>>>> Acked-by: Simon Horman >>>>>> --- >>>>>> Changes since version 2: >>>>>> - added #include ; >>>>>> - added ACKs from Simon Horman and Kuninori Morimoto. >>>>>> include/linux/usb/rcar-phy.h | 40 ++++++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 40 insertions(+) >>>>>> Index: renesas/include/linux/usb/rcar-phy.h >>>>>> =================================>>>>>> --- /dev/null >>>>>> +++ renesas/include/linux/usb/rcar-phy.h >>>>>> @@ -0,0 +1,40 @@ >>>>>> +/* >>>>>> + * Copyright (C) 2013 Renesas Solutions Corp. >>>>>> + * Copyright (C) 2013 Cogent Embedded, Inc. >>>>>> + * >>>>>> + * 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. >>>>>> + */ >>>>>> + >>>>>> +#ifndef __RCAR_PHY_H >>>>>> +#define __RCAR_PHY_H >>>>>> + >>>>>> +#include >>>>>> +#include >>>>>> + >>>>>> +/* USBPCTRL0 register bits */ >>>>>> +#define USBPCTRL0_OVC2 BIT(10) /* Switches the OVC input pin for port 2: */ >>>>>> + /* 1: USB_OVC2, 0: OVC2 */ >>>>>> +#define USBPCTRL0_OVC1_VBUS1 BIT(9) /* Switches the OVC input pin for port 1: */ >>>>>> + /* 1: USB_OVC1, 0: OVC1/VBUS1 */ >>>>>> +#define USBPCTRL0_OVC0 BIT(8) /* Switches the OVC input pin for port 0: */ >>>>>> + /* 1: USB_OVC0 pin, 0: OVC0 */ >>>>>> +#define USBPCTRL0_OVC2_ACT BIT(6) /* Host mode: OVC2 polarity: */ >>>>>> + /* 1: active-high, 0: active-low */ >>>>>> + /* Function mode: be sure to set to 1 */ >>>>>> +#define USBPCTRL0_PENC BIT(4) /* Function mode: output level of PENC1 pin: */ >>>>>> + /* 1: high, 0: low */ >>>>>> +#define USBPCTRL0_OVC0_ACT BIT(3) /* Host mode: OVC0 polarity: */ >>>>>> + /* 1: active-high, 0: active-low */ >>>>>> +#define USBPCTRL0_OVC1_ACT BIT(1) /* Host mode: OVC1 polarity: */ >>>>>> + /* 1: active-high, 0: active-low */ >>>>>> + /* Function mode: be sure to set to 1 */ >>>>>> +#define USBPCTRL0_PORT1 BIT(0) /* Selects port 1 mode: */ >>>>>> + /* 1: function, 0: host */ >>>>>> + >>>>>> +struct rcar_phy_platform_data { >>>>>> + u32 usbpctrl0; /* USBPCTRL0 register value */ >>>>>> +}; >>>>> looks really wrong to me to pass register contents via pdata. You should >>>>> pass flags which would aid the driver into figuring out how to program >>>>> that register. >>>> That was my first thought (and implementation) but this didn't >>>> look good either (and even worse with the device tree approach) as we >>>> couldn't come up with the clear names for the bitfields. Also, not >>>> all these bits are present in R8A7778 support for which I'm adding in >>>> the later patchset. >>>> Besides, IMO this little differs from having a flags field with >>>> the flags bits #define'd beforehand. Or did you mean that I should >>>> have surely used *bool* bitfields? >>> How about: >> Er, I was asking you about the platform data only, not the DT >> representation yet. :-) > easy(-ish) to translate, just needs more fields in your structure. That's clear , about more fields. :-) > >>> rcar { >>> compatible ... >>> reg... >>> >>> /* switch OVC for all three ports */ >>> renesas,rcar-ovc-port-config = <2 "high", >>> 1 "low", >>> 0 "high" >; >> Hm, 'dtc' allows mixed type properties now? >> Also, we need to describe the multiplexing of the OVCn pins (5V >> or 3.3V input), >> not only the active level. > fair enough, it can now be pre-processed so you can have defines, then > you can: > > #define PORT_HIGH 1 > #define PORT_LOW 0 > > #define MUX_ABC foo > #define MUX_XYZ bar > #define MUX_MNO baz > > ...-port-config = <2 PORT_HIGH MUX_ABC, > 1 PORT_LOW MUX_XYZ, > 0 PORT_HIGH MUX_MNO>; Ah, didn't know about that (although have seen some named entities like these in the device tree excerpts. OK, it's getting clearer now... >>> Would this work for you ? >> I should try... All this surely looks more complex than we would >> hope... > passing register contents will hurt you in the future if some other > device comes up with more bits It's already there: R8A7779 vs R8A7778. An it will hurt anyway, as I'll have to add the new fields for the new bits... > or a slightly different layout Don't think that'll be the case. Though you never know... > and stuff like that. > WBR, Sergei