Re: [PATCH v3 7/9] rcar-phy: add platform data

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

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 <linux/usb/rcar-phy.h> 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 <sergei.shtylyov@cogentembedded.com>
>>>>>> Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>>>>>> Acked-by: Simon Horman <horms+renesas@verge.net.au>
>>>>>> ---
>>>>>> Changes since version 2:
>>>>>> - added #include <linux/types.h>;
>>>>>> - 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 <linux/bitops.h>
>>>>>> +#include <linux/types.h>
>>>>>> +
>>>>>> +/* 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