* Re: [PATCH RESEND v10 3/4] PHY: add APM X-Gene SoC 15Gbps Multi-purpose PHY driver
From: Kishon Vijay Abraham I @ 2014-02-27 6:02 UTC (permalink / raw)
To: Loc Ho
Cc: devicetree@vger.kernel.org, Suman Tripathi, Arnd Bergmann,
Jon Masters, patches@apm.com, linux-kernel, Olof Johansson,
Don Dutile, Tejun Heo, Tuan Phan,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAPw-ZT=eLbxt3wPL7hrZFtxTP9vGxbRyBY2Yoh77pq9ypphCaw@mail.gmail.com>
On Thursday 27 February 2014 02:15 AM, Loc Ho wrote:
> Hi,
>
>>>
>>> +config PHY_XGENE
>>> + tristate "APM X-Gene 15Gbps PHY support"
>>> + depends on ARM64 || COMPILE_TEST
>>> + select GENERIC_PHY
>>
>>
>> depends on HAS_IOMEM and CONFIG_OF..
>
> I will make it depends as "HAS_IOMEM && OF && (ARM64 || COMPILE_TEST)
>
>>>
>>> +/* PLL Clock Macro Unit (CMU) CSR accessing from SDS indirectly */
>>> +#define CMU_REG0 0x00000
>>> +#define CMU_REG0_PLL_REF_SEL_MASK 0x00002000
>>> +#define CMU_REG0_PLL_REF_SEL_SET(dst, src) \
>>> + (((dst) & ~0x00002000) | (((u32)(src) << 0xd) & 0x00002000))
>>
>>
>> using decimals for shift value would be better. No strong feeling though.
>
> I will change to integer value.
>
>>> +/*
>>> + * For chip earlier than A3 version, enable this flag.
>>> + * To enable, pass boot argument phy_xgene.preA3Chip=1
>>> + */
>>> +static int preA3Chip;
>>> +MODULE_PARM_DESC(preA3Chip, "Enable pre-A3 chip support (1=enable 0=disable)");
>>> +module_param_named(preA3Chip, preA3Chip, int, 0444);
>>
>>
>> Do we need to have module param for this? I mean we can differentiate between
>> different chip versions in dt data only.
>
> This is only required for the short term. Once all the pre-A3 system
> are replaced, there isn't an need for this. For those who still has an
> pre-A3 silicon system, this would provide an short term solution for
> them. DT isn't quite correct here. This is an global thing. I guess I
> can OR all node. If it is still better to put in the DT, let me know
> and I will move it.
>
>>> +
>>> +static void sds_wr(void __iomem *csr_base, u32 indirect_cmd_reg,
>>> + u32 indirect_data_reg, u32 addr, u32 data)
>>> +{
>>> + u32 val;
>>> + u32 cmd;
>>> +
>>> + cmd = CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK;
>>> + cmd = CFG_IND_ADDR_SET(cmd, addr);
>>
>>
>> This looks hacky. If 'CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK' should be set then it should be part of the second argument. From the macro 'CFG_IND_ADDR_SET' the first argument should be more like the current value present in the register right? I feel the macro (CFG_IND_ADDR_SET) is not used in the way it is intended to.
>
> The macro XXX_SET is intended to update an field within the register.
> The update field is returned. The first assignment lines are setting
> another field. Those two lines can be written as:
>
> cmd = 0;
> cmd |= CFG_IND_WR_CMD_MASK; ==> Set the CMD bit
> cmd |= CFG_IND_CMD_DONE_MASK; ==> Set the DONE bit
> cmd = CFG_IND_ADDR_SET(cmd, addr); ===> Set the field ADDR
#define CFG_IND_ADDR_SET(dst, src) \
(((dst) & ~0x003ffff0) | (((u32)(src)<<4) & 0x003ffff0))
From this macro the first argument should be the present value in that
register. Here you reset the address bits and write the new address bits.
IMO the first argument should be the value in 'csr_base +
indirect_cmd_reg'. So it resets the address bits in 'csr_base +
indirect_cmd_reg' and write down the new address bits.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH RESEND v10 3/4] PHY: add APM X-Gene SoC 15Gbps Multi-purpose PHY driver
From: Loc Ho @ 2014-02-27 6:25 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: Tejun Heo, Olof Johansson, Arnd Bergmann,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Don Dutile, Jon Masters, patches-qTEPVZfXA3Y@public.gmane.org,
Tuan Phan, Suman Tripathi
In-Reply-To: <530ED4EF.1060000-l0cyMroinI0@public.gmane.org>
Hi,
>>>> +
>>>> +static void sds_wr(void __iomem *csr_base, u32 indirect_cmd_reg,
>>>> + u32 indirect_data_reg, u32 addr, u32 data)
>>>> +{
>>>> + u32 val;
>>>> + u32 cmd;
>>>> +
>>>> + cmd = CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK;
>>>> + cmd = CFG_IND_ADDR_SET(cmd, addr);
>>>
>>>
>>>
>>> This looks hacky. If 'CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK' should
>>> be set then it should be part of the second argument. From the macro
>>> 'CFG_IND_ADDR_SET' the first argument should be more like the current value
>>> present in the register right? I feel the macro (CFG_IND_ADDR_SET) is not
>>> used in the way it is intended to.
>>
>>
>> The macro XXX_SET is intended to update an field within the register.
>> The update field is returned. The first assignment lines are setting
>> another field. Those two lines can be written as:
>>
>> cmd = 0;
>> cmd |= CFG_IND_WR_CMD_MASK; ==> Set the CMD bit
>> cmd |= CFG_IND_CMD_DONE_MASK; ==> Set the DONE bit
>> cmd = CFG_IND_ADDR_SET(cmd, addr); ===> Set the field ADDR
>
>
> #define CFG_IND_ADDR_SET(dst, src) \
> (((dst) & ~0x003ffff0) | (((u32)(src)<<4) & 0x003ffff0))
>
> From this macro the first argument should be the present value in that
> register. Here you reset the address bits and write the new address bits.
Yes.. This is correct. I am clearing x number of bit and then set new value.
> IMO the first argument should be the value in 'csr_base + indirect_cmd_reg'.
> So it resets the address bits in 'csr_base + indirect_cmd_reg' and write
> down the new address bits.
Yes.. The above code does just that. In addition, I am also setting
the bits CFG_IND_WR_CMD_MASK and CFG_IND_CMD_DONE_MASK with the two
previous statement. Think of the code flow as follow:
val = readl(some void * address); /* read the register */
val = XXXX_SET(val, 0x1); /* set bit 0 - assuming XXXX set
bit 0 only */
val = YYYY_SET(val, 0x1); /* set bit 1 - assuming YYYY set
bit 1 only */
val = ZZZZ_SET(val, 0x5); /* set upper 16 bit of the
register to 0x5 - assuming ZZZZ set field of the upper 16 bits */
Instead writing the above, I am replacing the above 4 lines with these
two lines:
cmd = CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK;
cmd = CFG_IND_ADDR_SET(cmd, addr);
Is there clear?
-Loc
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH RESEND v10 3/4] PHY: add APM X-Gene SoC 15Gbps Multi-purpose PHY driver
From: Kishon Vijay Abraham I @ 2014-02-27 6:34 UTC (permalink / raw)
To: Loc Ho
Cc: devicetree@vger.kernel.org, Suman Tripathi, Arnd Bergmann,
Jon Masters, patches@apm.com, linux-kernel, Olof Johansson,
Don Dutile, Tejun Heo, Tuan Phan,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAPw-ZTnLYbUjQTm7QhJO5EMNqN5+dtWndSRUZn_bgMGvmNEu0Q@mail.gmail.com>
On Thursday 27 February 2014 11:55 AM, Loc Ho wrote:
> Hi,
>
>>>>> +
>>>>> +static void sds_wr(void __iomem *csr_base, u32 indirect_cmd_reg,
>>>>> + u32 indirect_data_reg, u32 addr, u32 data)
>>>>> +{
>>>>> + u32 val;
>>>>> + u32 cmd;
>>>>> +
>>>>> + cmd = CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK;
>>>>> + cmd = CFG_IND_ADDR_SET(cmd, addr);
>>>>
>>>>
>>>>
>>>> This looks hacky. If 'CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK' should
>>>> be set then it should be part of the second argument. From the macro
>>>> 'CFG_IND_ADDR_SET' the first argument should be more like the current value
>>>> present in the register right? I feel the macro (CFG_IND_ADDR_SET) is not
>>>> used in the way it is intended to.
>>>
>>>
>>> The macro XXX_SET is intended to update an field within the register.
>>> The update field is returned. The first assignment lines are setting
>>> another field. Those two lines can be written as:
>>>
>>> cmd = 0;
>>> cmd |= CFG_IND_WR_CMD_MASK; ==> Set the CMD bit
>>> cmd |= CFG_IND_CMD_DONE_MASK; ==> Set the DONE bit
>>> cmd = CFG_IND_ADDR_SET(cmd, addr); ===> Set the field ADDR
>>
>>
>> #define CFG_IND_ADDR_SET(dst, src) \
>> (((dst) & ~0x003ffff0) | (((u32)(src)<<4) & 0x003ffff0))
>>
>> From this macro the first argument should be the present value in that
>> register. Here you reset the address bits and write the new address bits.
>
> Yes.. This is correct. I am clearing x number of bit and then set new value.
>
>> IMO the first argument should be the value in 'csr_base + indirect_cmd_reg'.
>> So it resets the address bits in 'csr_base + indirect_cmd_reg' and write
>> down the new address bits.
>
> Yes.. The above code does just that. In addition, I am also setting
> the bits CFG_IND_WR_CMD_MASK and CFG_IND_CMD_DONE_MASK with the two
> previous statement. Think of the code flow as follow:
>
> val = readl(some void * address); /* read the register */
Where are you reading the register in your code (before CFG_IND_ADDR_SET)?
> val = XXXX_SET(val, 0x1); /* set bit 0 - assuming XXXX set
> bit 0 only */
If you want to set other bits (other than address) don't use
CFG_IND_ADDR_SET macro. That looks hacky to me.
> val = YYYY_SET(val, 0x1); /* set bit 1 - assuming YYYY set
> bit 1 only */
> val = ZZZZ_SET(val, 0x5); /* set upper 16 bit of the
> register to 0x5 - assuming ZZZZ set field of the upper 16 bits */
>
> Instead writing the above, I am replacing the above 4 lines with these
> two lines:
>
> cmd = CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK;
> cmd = CFG_IND_ADDR_SET(cmd, addr);
>
> Is there clear?
>
> -Loc
>
Cheers
Kishon
^ permalink raw reply
* Re: [PATCH RESEND v10 3/4] PHY: add APM X-Gene SoC 15Gbps Multi-purpose PHY driver
From: Loc Ho @ 2014-02-27 6:41 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: Tejun Heo, Olof Johansson, Arnd Bergmann,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Don Dutile, Jon Masters, patches-qTEPVZfXA3Y@public.gmane.org,
Tuan Phan, Suman Tripathi
In-Reply-To: <530EDC65.8060106-l0cyMroinI0@public.gmane.org>
Hi,
>>>>>> +
>>>>>> +static void sds_wr(void __iomem *csr_base, u32 indirect_cmd_reg,
>>>>>> + u32 indirect_data_reg, u32 addr, u32 data)
>>>>>> +{
>>>>>> + u32 val;
>>>>>> + u32 cmd;
>>>>>> +
>>>>>> + cmd = CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK;
>>>>>> + cmd = CFG_IND_ADDR_SET(cmd, addr);
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> This looks hacky. If 'CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK'
>>>>> should
>>>>> be set then it should be part of the second argument. From the macro
>>>>> 'CFG_IND_ADDR_SET' the first argument should be more like the current
>>>>> value
>>>>> present in the register right? I feel the macro (CFG_IND_ADDR_SET) is
>>>>> not
>>>>> used in the way it is intended to.
>>>>
>>>>
>>>>
>>>> The macro XXX_SET is intended to update an field within the register.
>>>> The update field is returned. The first assignment lines are setting
>>>> another field. Those two lines can be written as:
>>>>
>>>> cmd = 0;
>>>> cmd |= CFG_IND_WR_CMD_MASK; ==> Set the CMD bit
>>>> cmd |= CFG_IND_CMD_DONE_MASK; ==> Set the DONE bit
>>>> cmd = CFG_IND_ADDR_SET(cmd, addr); ===> Set the field ADDR
>>>
>>>
>>>
>>> #define CFG_IND_ADDR_SET(dst, src) \
>>> (((dst) & ~0x003ffff0) | (((u32)(src)<<4) & 0x003ffff0))
>>>
>>> From this macro the first argument should be the present value in that
>>> register. Here you reset the address bits and write the new address bits.
>>
>>
>> Yes.. This is correct. I am clearing x number of bit and then set new
>> value.
>>
>>> IMO the first argument should be the value in 'csr_base +
>>> indirect_cmd_reg'.
>>> So it resets the address bits in 'csr_base + indirect_cmd_reg' and write
>>> down the new address bits.
>>
>>
>> Yes.. The above code does just that. In addition, I am also setting
>> the bits CFG_IND_WR_CMD_MASK and CFG_IND_CMD_DONE_MASK with the two
>> previous statement. Think of the code flow as follow:
>>
>> val = readl(some void * address); /* read the register */
>
>
> Where are you reading the register in your code (before CFG_IND_ADDR_SET)?
I am not reading the register as I will be completely setting them.
This example is to show how these macro intended to be used.
>>
>> val = XXXX_SET(val, 0x1); /* set bit 0 - assuming XXXX set
>> bit 0 only */
>
> If you want to set other bits (other than address) don't use
> CFG_IND_ADDR_SET macro. That looks hacky to me.
I am not. This example is only to show how it can be used if there are
multiple fields to be set. I need to set three fields - two 1 bit
fields and one 19 bit fields. For the one bit field, I just use the
mask. For the 19 bit field, I am using the CFG_IND_ADDR_SET macro. Any
issue before I post an update version?
>
>> val = YYYY_SET(val, 0x1); /* set bit 1 - assuming YYYY set
>> bit 1 only */
>> val = ZZZZ_SET(val, 0x5); /* set upper 16 bit of the
>> register to 0x5 - assuming ZZZZ set field of the upper 16 bits */
>>
>> Instead writing the above, I am replacing the above 4 lines with these
>> two lines:
>>
>> cmd = CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK;
>> cmd = CFG_IND_ADDR_SET(cmd, addr);
>>
>> Is there clear?
>>
-Loc
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH RESEND v10 3/4] PHY: add APM X-Gene SoC 15Gbps Multi-purpose PHY driver
From: Kishon Vijay Abraham I @ 2014-02-27 6:42 UTC (permalink / raw)
To: Loc Ho
Cc: devicetree@vger.kernel.org, Suman Tripathi, Arnd Bergmann,
Jon Masters, patches@apm.com, linux-kernel, Olof Johansson,
Don Dutile, Tejun Heo, Tuan Phan,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <530EDC65.8060106@ti.com>
On Thursday 27 February 2014 12:04 PM, Kishon Vijay Abraham I wrote:
> On Thursday 27 February 2014 11:55 AM, Loc Ho wrote:
>> Hi,
>>
>>>>>> +
>>>>>> +static void sds_wr(void __iomem *csr_base, u32 indirect_cmd_reg,
>>>>>> + u32 indirect_data_reg, u32 addr, u32 data)
>>>>>> +{
>>>>>> + u32 val;
>>>>>> + u32 cmd;
>>>>>> +
>>>>>> + cmd = CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK;
>>>>>> + cmd = CFG_IND_ADDR_SET(cmd, addr);
>>>>>
>>>>>
>>>>>
>>>>> This looks hacky. If 'CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK'
>>>>> should
>>>>> be set then it should be part of the second argument. From the macro
>>>>> 'CFG_IND_ADDR_SET' the first argument should be more like the
>>>>> current value
>>>>> present in the register right? I feel the macro (CFG_IND_ADDR_SET)
>>>>> is not
>>>>> used in the way it is intended to.
>>>>
>>>>
>>>> The macro XXX_SET is intended to update an field within the register.
>>>> The update field is returned. The first assignment lines are setting
>>>> another field. Those two lines can be written as:
>>>>
>>>> cmd = 0;
>>>> cmd |= CFG_IND_WR_CMD_MASK; ==> Set the CMD bit
>>>> cmd |= CFG_IND_CMD_DONE_MASK; ==> Set the DONE bit
>>>> cmd = CFG_IND_ADDR_SET(cmd, addr); ===> Set the field ADDR
>>>
>>>
>>> #define CFG_IND_ADDR_SET(dst, src) \
>>> (((dst) & ~0x003ffff0) | (((u32)(src)<<4) &
>>> 0x003ffff0))
>>>
>>> From this macro the first argument should be the present value in that
>>> register. Here you reset the address bits and write the new address
>>> bits.
>>
>> Yes.. This is correct. I am clearing x number of bit and then set new
>> value.
>>
>>> IMO the first argument should be the value in 'csr_base +
>>> indirect_cmd_reg'.
>>> So it resets the address bits in 'csr_base + indirect_cmd_reg' and write
>>> down the new address bits.
>>
>> Yes.. The above code does just that. In addition, I am also setting
>> the bits CFG_IND_WR_CMD_MASK and CFG_IND_CMD_DONE_MASK with the two
>> previous statement. Think of the code flow as follow:
>>
>> val = readl(some void * address); /* read the register */
>
> Where are you reading the register in your code (before CFG_IND_ADDR_SET)?
>> val = XXXX_SET(val, 0x1); /* set bit 0 - assuming XXXX set
>> bit 0 only */
> If you want to set other bits (other than address) don't use
> CFG_IND_ADDR_SET macro. That looks hacky to me.
huh.. looked it again and I think only the readl is missing. If you can
add that, it should be fine.
How about something like this
val = readl(csr_base + indirect_cmd_reg);
val = CFG_IND_ADDR_SET(val, addr);
val |= CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK;
writel(val, csr_base + indirect_cmd_reg);
Cheers
Kishon
^ permalink raw reply
* Re: [PATCH RESEND v10 3/4] PHY: add APM X-Gene SoC 15Gbps Multi-purpose PHY driver
From: Kishon Vijay Abraham I @ 2014-02-27 6:47 UTC (permalink / raw)
To: Loc Ho
Cc: devicetree@vger.kernel.org, Suman Tripathi, Arnd Bergmann,
Jon Masters, patches@apm.com, linux-kernel, Olof Johansson,
Don Dutile, Tejun Heo, Tuan Phan,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <CAPw-ZTn=za14-x9fjnDfcY419i5nuFPqsQp_r-75zjyHf11E4w@mail.gmail.com>
On Thursday 27 February 2014 12:11 PM, Loc Ho wrote:
> Hi,
>
>>>>>>> +
>>>>>>> +static void sds_wr(void __iomem *csr_base, u32 indirect_cmd_reg,
>>>>>>> + u32 indirect_data_reg, u32 addr, u32 data)
>>>>>>> +{
>>>>>>> + u32 val;
>>>>>>> + u32 cmd;
>>>>>>> +
>>>>>>> + cmd = CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK;
>>>>>>> + cmd = CFG_IND_ADDR_SET(cmd, addr);
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> This looks hacky. If 'CFG_IND_WR_CMD_MASK | CFG_IND_CMD_DONE_MASK'
>>>>>> should
>>>>>> be set then it should be part of the second argument. From the macro
>>>>>> 'CFG_IND_ADDR_SET' the first argument should be more like the current
>>>>>> value
>>>>>> present in the register right? I feel the macro (CFG_IND_ADDR_SET) is
>>>>>> not
>>>>>> used in the way it is intended to.
>>>>>
>>>>>
>>>>>
>>>>> The macro XXX_SET is intended to update an field within the register.
>>>>> The update field is returned. The first assignment lines are setting
>>>>> another field. Those two lines can be written as:
>>>>>
>>>>> cmd = 0;
>>>>> cmd |= CFG_IND_WR_CMD_MASK; ==> Set the CMD bit
>>>>> cmd |= CFG_IND_CMD_DONE_MASK; ==> Set the DONE bit
>>>>> cmd = CFG_IND_ADDR_SET(cmd, addr); ===> Set the field ADDR
>>>>
>>>>
>>>>
>>>> #define CFG_IND_ADDR_SET(dst, src) \
>>>> (((dst) & ~0x003ffff0) | (((u32)(src)<<4) & 0x003ffff0))
>>>>
>>>> From this macro the first argument should be the present value in that
>>>> register. Here you reset the address bits and write the new address bits.
>>>
>>>
>>> Yes.. This is correct. I am clearing x number of bit and then set new
>>> value.
>>>
>>>> IMO the first argument should be the value in 'csr_base +
>>>> indirect_cmd_reg'.
>>>> So it resets the address bits in 'csr_base + indirect_cmd_reg' and write
>>>> down the new address bits.
>>>
>>>
>>> Yes.. The above code does just that. In addition, I am also setting
>>> the bits CFG_IND_WR_CMD_MASK and CFG_IND_CMD_DONE_MASK with the two
>>> previous statement. Think of the code flow as follow:
>>>
>>> val = readl(some void * address); /* read the register */
>>
>>
>> Where are you reading the register in your code (before CFG_IND_ADDR_SET)?
>
> I am not reading the register as I will be completely setting them.
Ok. Never-mind then. Sorry for the noise. You code is fine.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH] thermal: add generic IIO channel thermal sensor driver
From: Zhang Rui @ 2014-02-27 6:53 UTC (permalink / raw)
To: Courtney Cavin
Cc: eduardo.valentin, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, galak, rob, grant.likely, linux-pm, devicetree,
linux-doc, linux-kernel
In-Reply-To: <1391651007-29813-1-git-send-email-courtney.cavin@sonymobile.com>
On Wed, 2014-02-05 at 17:43 -0800, Courtney Cavin wrote:
> This driver is a generic method for using IIO ADC channels as thermal
> sensors.
>
> Signed-off-by: Courtney Cavin <courtney.cavin@sonymobile.com>
Eduardo,
what do you think of this patch?
thanks,
rui
> ---
> .../devicetree/bindings/thermal/iio-thermal.txt | 46 +++++
> drivers/thermal/Kconfig | 13 ++
> drivers/thermal/Makefile | 1 +
> drivers/thermal/iio_thermal.c | 207 +++++++++++++++++++++
> 4 files changed, 267 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/thermal/iio-thermal.txt
> create mode 100644 drivers/thermal/iio_thermal.c
>
> diff --git a/Documentation/devicetree/bindings/thermal/iio-thermal.txt b/Documentation/devicetree/bindings/thermal/iio-thermal.txt
> new file mode 100644
> index 0000000..3be11b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/iio-thermal.txt
> @@ -0,0 +1,46 @@
> +Generic IIO channel thermal sensor bindings
> +
> +compatible:
> + Usage: required
> + Type: string
> + Desc: compatible string, must be "iio-thermal"
> +
> +conversion-method:
> + Usage: required
> + Type: string
> + Desc: How to convert IIO voltage values to temperature, one of:
> + "interpolation" - interpolate between values in lookup table
> + "scalar" - use values as multiplier and divisor
> +
> +conversion-values:
> + Usage: required
> + Type: u32 array, 2-tuples
> + Desc: lookup table for conversion, for conversion-method:
> + "interpolation" - 2-tuples of < uV mK >; micro-volts to
> + milli-kelvin; table must ascend
> + "scalar" - single scalar 2-tuple as < M D >; where:
> + mK = uV * M / D
> +
> +io-channels:
> + Usage: required
> + Type: prop-encoded-array
> + Desc: See bindings/iio/iio-bindings.txt; must be a voltage channel
> +
> +Example:
> +
> +vadc: some_vadc {
> + compatible = "...";
> + #io-channel-cells = <1>;
> +};
> +
> +iio-thermal {
> + compatible = "iio-thermal";
> + io-channels = <&vadc 0>;
> + conversion-method = "interpolation";
> + conversion-values = <
> + 30000 398200
> + 385000 318200
> + 1738000 233200
> + >;
> +};
> +
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 35c0664..f83a8e8 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -114,6 +114,19 @@ config THERMAL_EMULATION
> because userland can easily disable the thermal policy by simply
> flooding this sysfs node with low temperature values.
>
> +config IIO_THERMAL
> + tristate "Temperature sensor driver for generic IIO channels"
> + depends on IIO
> + depends on THERMAL_OF
> + help
> + Support for generic IIO channels, such as ADCs. This driver allows
> + you to expose an IIO voltage channel as a thermal sensor. This is
> + implemented as a thermal sensor, not a thermal zone, and thus
> + requires DT defined thermal infrastructure in order to be useful.
> +
> + If you aren't sure that you need this support, or haven't configured
> + a thermal infrastructure in device tree, you should say 'N' here.
> +
> config IMX_THERMAL
> tristate "Temperature sensor driver for Freescale i.MX SoCs"
> depends on CPU_THERMAL
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 54e4ec9..0ee2c92 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -25,6 +25,7 @@ obj-y += samsung/
> obj-$(CONFIG_DOVE_THERMAL) += dove_thermal.o
> obj-$(CONFIG_DB8500_THERMAL) += db8500_thermal.o
> obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o
> +obj-$(CONFIG_IIO_THERMAL) += iio_thermal.o
> obj-$(CONFIG_IMX_THERMAL) += imx_thermal.o
> obj-$(CONFIG_DB8500_CPUFREQ_COOLING) += db8500_cpufreq_cooling.o
> obj-$(CONFIG_INTEL_POWERCLAMP) += intel_powerclamp.o
> diff --git a/drivers/thermal/iio_thermal.c b/drivers/thermal/iio_thermal.c
> new file mode 100644
> index 0000000..df21dbc
> --- /dev/null
> +++ b/drivers/thermal/iio_thermal.c
> @@ -0,0 +1,207 @@
> +/*
> + * An IIO channel based thermal sensor driver
> + *
> + * Copyright (C) 2014 Sony Mobile Communications, AB
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/thermal.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +
> +#define MILLIKELVIN_0C 273150
> +
> +struct iio_thermal_conv {
> + s32 *values;
> + int ntuple;
> + long (*convert)(const struct iio_thermal_conv *, int val);
> +};
> +
> +struct iio_thermal {
> + struct thermal_zone_device *tz;
> + struct iio_channel *chan;
> + struct iio_thermal_conv conv;
> +};
> +
> +static int iio_bsearch(const struct iio_thermal_conv *conv, s32 input)
> +{
> + int h = conv->ntuple - 1;
> + int l = 0;
> +
> + while (h - 1 > l) {
> + int m = (l + h) & ~1;
> + s32 v = conv->values[m];
> + if (v < input)
> + l = m >> 1;
> + else
> + h = m >> 1;
> + }
> +
> + return h;
> +}
> +
> +static long iio_thermal_interpolate(const struct iio_thermal_conv *conv, int v)
> +{
> + int n;
> +
> + n = iio_bsearch(conv, v);
> + if (n == 0 || n == conv->ntuple) {
> + BUG();
> + } else {
> + s32 *pts = &conv->values[(n-1)*2];
> + s32 sx = pts[3] - pts[1];
> + s32 sy = pts[2] - pts[0];
> + return sx * (v - pts[0]) / sy + pts[1];
> + }
> +}
> +
> +static long iio_thermal_scale(const struct iio_thermal_conv *conv, int v)
> +{
> + return div_s64((s64)v * conv->values[0], conv->values[1]);
> +}
> +
> +static int iio_thermal_get_temp(void *pdata, long *value)
> +{
> + struct iio_thermal *iio = pdata;
> + long temp;
> + int val;
> + int rc;
> +
> + rc = iio_read_channel_processed(iio->chan, &val);
> + if (rc)
> + return rc;
> +
> + temp = iio->conv.convert(&iio->conv, val);
> +
> + /* thermal core wants milli-celsius; we deal in milli-kelvin */
> + temp = temp - MILLIKELVIN_0C;
> +
> + /*
> + * Although it would appear that the temperature value here is a
> + * signed value for sensors, the underlying thermal zone core
> + * doesn't deal with negative temperature. Cut this value off at 0C.
> + */
> + *value = (temp < 0) ? 0 : temp;
> +
> + return 0;
> +}
> +
> +static int iio_thermal_probe(struct platform_device *pdev)
> +{
> + struct iio_thermal *iio;
> + const void *values;
> + const char *type;
> + int rc;
> +
> + dev_info(&pdev->dev, "registering IIO thermal device\n");
> + iio = devm_kzalloc(&pdev->dev, sizeof(*iio), GFP_KERNEL);
> + if (!iio)
> + return -ENOMEM;
> +
> + values = of_get_property(pdev->dev.of_node,
> + "conversion-values", &iio->conv.ntuple);
> + if (values == NULL || (iio->conv.ntuple % (sizeof(u32) * 2)) != 0) {
> + dev_err(&pdev->dev, "invalid/missing conversion values\n");
> + return -EINVAL;
> + }
> + iio->conv.ntuple /= sizeof(u32) * 2;
> + iio->conv.values = devm_kzalloc(&pdev->dev,
> + sizeof(u32) * 2 * iio->conv.ntuple, GFP_KERNEL);
> + if (!iio->conv.values)
> + return -ENOMEM;
> +
> + rc = of_property_read_u32_array(pdev->dev.of_node,
> + "conversion-values", (u32 *)iio->conv.values,
> + iio->conv.ntuple * 2);
> + if (rc) {
> + dev_err(&pdev->dev, "invalid/missing conversion values\n");
> + return -EINVAL;
> + }
> +
> + rc = of_property_read_string(pdev->dev.of_node,
> + "conversion-method", &type);
> + if (rc) {
> + dev_err(&pdev->dev, "invalid/missing conversion method\n");
> + return rc;
> + }
> + if (!strcmp(type, "interpolation") && iio->conv.ntuple > 1) {
> + if (iio->conv.values[0] > iio->conv.values[2]) {
> + dev_err(&pdev->dev,
> + "conversion values should ascend\n");
> + return rc;
> + }
> + iio->conv.convert = iio_thermal_interpolate;
> + } else if (!strcmp(type, "scalar") && iio->conv.ntuple == 1) {
> + iio->conv.convert = iio_thermal_scale;
> + } else {
> + dev_err(&pdev->dev, "invalid conversion method for values\n");
> + return rc;
> + }
> +
> + iio->chan = iio_channel_get(&pdev->dev, NULL);
> + if (IS_ERR(iio->chan)) {
> + dev_err(&pdev->dev, "invalid/missing iio channel\n");
> + return PTR_ERR(iio->chan);
> + }
> + if (iio->chan->channel->type != IIO_VOLTAGE) {
> + dev_err(&pdev->dev, "specified iio channel is not voltage\n");
> + iio_channel_release(iio->chan);
> + return -EINVAL;
> + }
> + platform_set_drvdata(pdev, iio);
> +
> + iio->tz = thermal_zone_of_sensor_register(&pdev->dev, 0, iio,
> + iio_thermal_get_temp, NULL);
> + if (IS_ERR(iio->tz)) {
> + dev_err(&pdev->dev, "failed to register thermal sensor\n");
> + iio_channel_release(iio->chan);
> + return PTR_ERR(iio->tz);
> + }
> + dev_info(&pdev->dev, "successfully registered IIO thermal sensor\n");
> +
> + return 0;
> +}
> +
> +static int iio_thermal_remove(struct platform_device *pdev)
> +{
> + struct iio_thermal *iio;
> +
> + iio = platform_get_drvdata(pdev);
> +
> + thermal_zone_of_sensor_unregister(&pdev->dev, iio->tz);
> + iio_channel_release(iio->chan);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id iio_thermal_match[] = {
> + { .compatible = "iio-thermal", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, iio_thermal_match);
> +
> +static struct platform_driver iio_thermal = {
> + .driver = {
> + .name = "iio-thermal",
> + .owner = THIS_MODULE,
> + .of_match_table = iio_thermal_match,
> + },
> + .probe = iio_thermal_probe,
> + .remove = iio_thermal_remove,
> +};
> +module_platform_driver(iio_thermal);
> +
> +MODULE_DESCRIPTION("Thermal driver for IIO ADCs");
> +MODULE_LICENSE("GPL v2");
^ permalink raw reply
* Re: [RFCv4 3/7] mfd: twl4030-madc: Cleanup driver
From: Lee Jones @ 2014-02-27 8:03 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Sebastian Reichel, Marek Belisko, Jonathan Cameron, Samuel Ortiz,
Lars-Peter Clausen, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Grant Likely,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1393444990-28140-4-git-send-email-sre-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
> Some style fixes in twl4030-madc driver.
>
> Reported-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Sebastian Reichel <sre-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
> ---
> drivers/mfd/twl4030-madc.c | 106 ++++++++++++++++++---------------------
> include/linux/i2c/twl4030-madc.h | 2 +-
> 2 files changed, 51 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/mfd/twl4030-madc.c b/drivers/mfd/twl4030-madc.c
Acked-by: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v4 3/3] Documentation: of: Document graph bindings
From: Tomi Valkeinen @ 2014-02-27 8:08 UTC (permalink / raw)
To: Philipp Zabel
Cc: Russell King - ARM Linux, Mauro Carvalho Chehab, Grant Likely,
Rob Herring, Sylwester Nawrocki, Laurent Pinchart, Kyungmin Park,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-media-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, Guennadi Liakhovetski
In-Reply-To: <1393429676.3248.110.camel-+qGW7pzALmz7o/J7KWpOmN53zsg1cpMQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2369 bytes --]
On 26/02/14 17:47, Philipp Zabel wrote:
> Ok, that looks compact enough. I still don't see the need to change make
> the remote-endpoint property required to achieve this, though. On the
> other hand, I wouldn't object to making it mandatory either.
Sure, having remote-endpoint as required doesn't achieve anything
particular as such. I just feel it's cleaner. If you have an endpoint,
it must point to somewhere. Maybe it makes the code a tiny bit simpler.
If we do already have users for this that do not have the
remote-endpoint, then we're stuck with having it as optional. If we
don't, I'd rather have it as mandatory.
In any case, it's not a very important thing either way.
>> Of course, it's up to the developer how his dts looks like. But to me it
>> makes sense to require the remote-endpoint property, as the endpoint, or
>> even the port, doesn't make much sense if there's nothing to connect to.
>
> Please let's not make it mandatory for a port node to contain an
> endpoint. For any device with multiple ports we can't use the simplified
> form above, and only adding the (correctly numbered) port in all the
> board device trees would be a pain.
That's true. I went with having the ports in the board file, for example
on omap3 the dss has two ports, and N900 board uses the second one:
&dss {
status = "ok";
pinctrl-names = "default";
pinctrl-0 = <&dss_sdi_pins>;
vdds_sdi-supply = <&vaux1>;
ports {
#address-cells = <1>;
#size-cells = <0>;
port@1 {
reg = <1>;
sdi_out: endpoint {
remote-endpoint = <&lcd_in>;
datapairs = <2>;
};
};
};
};
Here I guess I could have:
&dss {
status = "ok";
pinctrl-names = "default";
pinctrl-0 = <&dss_sdi_pins>;
vdds_sdi-supply = <&vaux1>;
};
&dss_sdi_port {
sdi_out: endpoint {
remote-endpoint = <&lcd_in>;
datapairs = <2>;
};
};
But I didn't like that as it splits the pincontrol and regulator supply
from the port/endpoint, which are functionally linked together.
Actually, somewhat aside the subject, I'd like to have the pinctrl and
maybe regulator supply also per endpoint, but I didn't see how that
would be possible with the current framework. If a board would need to
endpoints for the same port, most likely it would also need to different
sets of pinctrls.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [RESEND PATCH v11] video: backlight: gpio-backlight: Add DT support.
From: Lee Jones @ 2014-02-27 8:12 UTC (permalink / raw)
To: Denis Carikli
Cc: Bryan Wu, Jingoo Han, Eric Bénard, devicetree, linux-kernel
In-Reply-To: <1393437845-25564-1-git-send-email-denis@eukrea.com>
> Cc: Bryan Wu <cooloney@gmail.com>
> Cc: Jingoo Han <jg1.han@samsung.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Eric Bénard <eric@eukrea.com>
> Signed-off-by: Denis Carikli <denis@eukrea.com>
> ---
> ChangeLog v10->v11:
Wow, that's a lot of fix-ups.
> - Shrinked the Cc list.
It's okay to have a large CC list, but I'd do it manually rather than
adding a line for each in the commit log. This latter method is good
for one or two extra people you wish to add to an individual patch
when sending a patch-set.
> - Fixed the of_match_table warning.
>
> ChangeLog v9->v10:
> - Only the respective maintainers, or people who responded to the patch
> were kept in the Cc.
> - The unnecessary of_match_ptr in of_match_table was removed.
Why was it unnecessary?
> ChangeLog v8->v9:
> - Added Shawn Guo in the Cc list.
> - The default-brightness-level is now a boolean default-on property,
> the gpio is only touched if the gpio-backlight driver instance probes.
> - The code and documentation was updated accordingly.
>
> ChangeLog v7->v8:
> - The default-state was renamed to default-brightness-level.
> - default-brightness-level is now mandatory, like for backlight-pwm,
> That way we avoid having to handle the case where it's not set,
> which means that we would need not to set the gpio, but still
> report a brightness value for sysfs, when not all gpio controllers are
> able to read the gpio value.
> - switched the default-brightness-level to boolean values (0 or 1) instead
> of using strings ("on", "off", "keep").
> - The documentation was updated accordingly.
> - The example in the documentation now uses the dts gpio defines.
> - The "backlight: gpio_backlight: Use a default state enum." patch was then
> dropped, becuase it is not necessary anymore.
>
> ChangeLog v6->v7:
> - removed a compilation warning with the removal of the useless ret declaration.
>
> ChangeLog v5->v6:
> - The default state handling was reworked:
> - it's now called default-state, and looks like the gpio-leds default-state.
> - it now has a "keep" option, like for the gpio-leds.
> - that "keep" option is the default when the default-state property is not set.
> - The documentation was updated accordingly.
>
> ChangeLog v4->v5:
> - The default-brightness property now defaults to 0 in the driver.
> - def_value int becomes a bool.
> - The check for the gpio validity has been reworked.
> ---
> .../bindings/video/backlight/gpio-backlight.txt | 19 +++++++
> drivers/video/backlight/gpio_backlight.c | 60 +++++++++++++++++---
These should really be in a seperate patch.
> 2 files changed, 72 insertions(+), 7 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
>
> diff --git a/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> new file mode 100644
> index 0000000..a923193
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/gpio-backlight.txt
> @@ -0,0 +1,19 @@
> +gpio-backlight bindings
> +
> +Required properties:
> + - compatible: "gpio-backlight"
> + - gpios: describes the gpio that is used for enabling/disabling the backlight
> + (see GPIO binding[0] for more details).
The format of the [0] is confusing and it doesn't really add anything
here. If you wish to reverence an external binding document just write
out the path. Failing that [bindings]/gpio/gpio.txt also works.
> +Optional properties:
> + - default-on: enable the backlight at boot.
> +
> +[0]: Documentation/devicetree/bindings/gpio/gpio.txt
> +
> +Example:
> +
> + backlight {
> + compatible = "gpio-backlight";
> + gpios = <&gpio3 4 GPIO_ACTIVE_HIGH>;
> + default-on;
> + };
> diff --git a/drivers/video/backlight/gpio_backlight.c b/drivers/video/backlight/gpio_backlight.c
<snip>
> +static int gpio_backlight_probe_dt(struct platform_device *pdev,
> + struct gpio_backlight *gbl)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + enum of_gpio_flags gpio_flags;
> + bool default_on = false;
There's no need to initialise this.
> + gbl->fbdev = NULL;
This will already be NULL, as you used kzalloc().
> + default_on = of_property_read_bool(np, "default-on");
> +
> + gbl->def_value = default_on;
Why complicate matters?
gbl->def_value = of_property_read_bool(np, "default-on");
> - bl->props.brightness = pdata->def_value;
> + bl->props.brightness = gbl->def_value;
Do you use gbl->def_value again later? If not:
gbl->def_value = = of_property_read_bool(np, "default-on");
#ifdef CONFIG_OF ?
> +static struct of_device_id gpio_backlight_of_match[] = {
> + { .compatible = "gpio-backlight" },
> + { /* sentinel */ }
> +};
> +
#endif ?
> static struct platform_driver gpio_backlight_driver = {
> .driver = {
> .name = "gpio-backlight",
> .owner = THIS_MODULE,
> + .of_match_table = gpio_backlight_of_match,
of_match_ptr() ?
<snip>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [RFCv4 2/7] mfd: twl4030-madc: Add DT support and convert to IIO framework
From: Lee Jones @ 2014-02-27 8:23 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Sebastian Reichel, Marek Belisko, Jonathan Cameron, Samuel Ortiz,
Lars-Peter Clausen, Rob Herring, Pawel Moll, Mark Rutland,
Ian Campbell, Kumar Gala, Grant Likely,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1393444990-28140-3-git-send-email-sre-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
On Wed, 26 Feb 2014, Sebastian Reichel wrote:
> This converts twl4030-madc module to use the Industrial IO ADC
> framework and adds device tree support.
>
> Signed-off-by: Sebastian Reichel <sre-8fiUuRrzOP0dnm+yROfE0A@public.gmane.org>
> ---
> drivers/mfd/twl4030-madc.c | 127 +++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 118 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mfd/twl4030-madc.c b/drivers/mfd/twl4030-madc.c
> index 5458561..37cb3ad 100644
> --- a/drivers/mfd/twl4030-madc.c
> +++ b/drivers/mfd/twl4030-madc.c
<snip>
> - if (!pdata) {
> + if (!pdata && !np) {
> dev_err(&pdev->dev, "platform_data not available\n");
and Device Tree node ...
<snip>
> + iio_dev->channels = twl4030_madc_iio_channels;
> + iio_dev->num_channels = 16;
ARRAY_SIZE()
<snip>
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [RFC PATCH] [media]: of: move graph helpers from drivers/media/v4l2-core to drivers/of
From: Tomi Valkeinen @ 2014-02-27 8:36 UTC (permalink / raw)
To: Philipp Zabel, Grant Likely
Cc: Sascha Hauer, Rob Herring, Russell King - ARM Linux,
Mauro Carvalho Chehab, Rob Herring, Sylwester Nawrocki,
Laurent Pinchart, Kyungmin Park,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Philipp Zabel
In-Reply-To: <1393426129.3248.64.camel-+qGW7pzALmz7o/J7KWpOmN53zsg1cpMQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 895 bytes --]
On 26/02/14 16:48, Philipp Zabel wrote:
>> I would like the document to acknowledge the difference from the
>> phandle+args pattern used elsewhere and a description of when it would
>> be appropriate to use this instead of a simpler binding.
>
> Alright. The main point of this binding is that the devices may have
> multiple distinct ports that each can be connected to other devices.
The other main point with this binding are multiple endpoints per port.
So you can have, say, a display controller, with single port, which has
two endpoints going to two separate LCD panels.
In physical level that would usually mean that the same pins from the
display controller are connected to two panels. Most likely this would
mean that only one panel can be used at a time, possibly with different
settings (say, 16 RGB pins for one panel, 24 RGB pins for the other).
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH v4 8/8] devicetree: bindings: Document PM8921/8058 PMICs
From: Lee Jones @ 2014-02-27 8:38 UTC (permalink / raw)
To: Stephen Boyd
Cc: Samuel Ortiz, linux-kernel, linux-arm-msm, linux-arm-kernel,
devicetree
In-Reply-To: <1393441166-32692-9-git-send-email-sboyd@codeaurora.org>
> PM8921 and PM8058 are PMICs found paired with MSM8960 and MSM8660
> devices respectively. They contain subdevices such as keypads,
> RTC, regulators, clocks, etc.
>
> Cc: <devicetree@vger.kernel.org>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
> .../devicetree/bindings/mfd/qcom,pm8xxx.txt | 63 ++++++++++++++++++++++
> 1 file changed, 63 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/qcom,pm8xxx.txt
This patch has been on the MLs for some time now and hasn't received
any feedback from the DT guys.
Patch looks good (despite the lack IRQ defines), applied.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v2 3/6] spi: sh-msiof: Add support for R-Car H2 and M2
From: Geert Uytterhoeven @ 2014-02-27 8:39 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mark Brown, Takashi Yoshii, Magnus Damm, linux-spi, Linux-sh list,
linux-kernel@vger.kernel.org, Geert Uytterhoeven,
devicetree@vger.kernel.org
In-Reply-To: <38730952.JbtPf2mapG@avalon>
On Wed, Feb 26, 2014 at 11:16 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> diff --git a/Documentation/devicetree/bindings/spi/sh-msiof.txt
>> b/Documentation/devicetree/bindings/spi/sh-msiof.txt index
>> eae3c8c9300e..1f0cb33763a1 100644
>> --- a/Documentation/devicetree/bindings/spi/sh-msiof.txt
>> +++ b/Documentation/devicetree/bindings/spi/sh-msiof.txt
>> @@ -1,8 +1,13 @@
>> Renesas MSIOF spi controller
>>
>> Required properties:
>> -- compatible : "renesas,sh-msiof" for SuperH, or
>> +- compatible : "renesas,msiof-<soctype>" for SoCs,
>> + "renesas,sh-msiof" for SuperH, or
>> "renesas,sh-mobile-msiof" for SH Mobile series.
>> + Examples with soctypes are:
>> + "renesas,msiof-sh7724" (SH)
>
> Given that the driver doesn't handle the "renesas,msiof-sh7724" compatible
> string this might not be a good example. Furthermore SuperH doesn't have DT
> support. I would thus drop the "renesas,sh-msiof" compatible string from patch
> 1/6 and wouldn't mention sh7724 here. I very much doubt that someone would
> have developed DT support for SuperH on the side and shipped products that
> would be broken by this change :-)
Upon reading your comment again: do you suggest to also remove the plain
"renesas,sh-msiof"? That one was present before, since DT support was added
to the driver in
commit cf9c86efecf9510e62388fd174cf607671c59fa3
Author: Bastian Hecht <hechtb@gmail.com>
Date: Wed Dec 12 12:54:48 2012 +0100
spi/sh-msiof: Add device tree parsing to driver
This adds the capability to retrieve setup data from the device tree
node. The usage of platform data is still available.
Signed-off-by: Bastian Hecht <hechtb+renesas@gmail.com>
Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
So I prefer not to remove any pre-existing compatible values.
Do you agree?
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH] gpio: document polarity flag best practices
From: Linus Walleij @ 2014-02-27 9:15 UTC (permalink / raw)
To: Stephen Warren
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Laurent Pinchart, Alexandre Courbot, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, Stephen Warren
In-Reply-To: <1392835406-9380-1-git-send-email-swarren@wwwdotorg.org>
On Wed, Feb 19, 2014 at 7:43 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> From: Stephen Warren <swarren@nvidia.com>
>
> Document what we (Laurent and I, following a mailing list dicussion)
> believe are best practices for the polarity flag in a GPIO specifier.
>
> While touching the doc, I made a few minor editing changes to other
> areas.
>
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
Patch applied, sorry for taking some time to react and THANKS
to you guys for hashing this out, much appreciated!
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] phy: micrel: add of configuration for LED mode
From: Mark Rutland @ 2014-02-27 9:15 UTC (permalink / raw)
To: Ben Dooks
Cc: linux-kernel@lists.codethink.co.uk, devicetree@vger.kernel.org,
netdev@vger.kernel.org, f.fainelli@gmail.com
In-Reply-To: <1393415280-10227-1-git-send-email-ben.dooks@codethink.co.uk>
On Wed, Feb 26, 2014 at 11:48:00AM +0000, Ben Dooks wrote:
> Add support for the led-mode property for the following PHYs
> which have a single LED mode configuration value.
>
> KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and
> KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4
> to control the LED configuration.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
> Documentation/devicetree/bindings/net/micrel.txt | 18 +++++++++
> drivers/net/phy/micrel.c | 49 ++++++++++++++++++++++--
> 2 files changed, 63 insertions(+), 4 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/net/micrel.txt
>
> diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
> new file mode 100644
> index 0000000..98a3e61
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/micrel.txt
> @@ -0,0 +1,18 @@
> +Micrel PHY properties.
> +
> +These properties cover the base properties Micrel PHYs.
> +
> +Optional properties:
> +
> + - micrel,led-mode : LED mode value to set for PHYs with configurable LEDs.
> +
> + Configure the LED mode with single value. The list of PHYs and
> + the bits that are currently supported:
> +
> + KSZ8001: register 0x1e, bits 15..14
> + KSZ8041: register 0x1e, bits 15..14
> + KSZ8021: register 0x1f, bits 5..4
> + KSZ8031: register 0x1f, bits 5..4
> + KSZ8051: register 0x1f, bits 5..4
> +
> + See the respective PHY datasheet for the mode values.
What do these mean, roughly,, and why can the kernel not decide how to
cnofigure these?
In general we prefer to not place raw register values in the DT, and I'd
like to know why we'd have to here.
Cheers,
Mark.
^ permalink raw reply
* Re: [PATCHv2 14/16] ARM: OMAP3: hwmod data: cleanup data for IOMMUs
From: Florian Vaussard @ 2014-02-27 9:16 UTC (permalink / raw)
To: Suman Anna, Tony Lindgren
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
Laurent Pinchart, linux-omap-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
In-Reply-To: <530E2B67.9050300-l0cyMroinI0@public.gmane.org>
Hi,
On 02/26/2014 06:59 PM, Suman Anna wrote:
> Tony,
>
> On 02/26/2014 11:18 AM, Tony Lindgren wrote:
>> * Suman Anna <s-anna-l0cyMroinI0@public.gmane.org> [140213 10:19]:
>>> From: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
>>>
>>> The irq numbers, ocp address space and device attribute data
>>> have all been cleaned up for OMAP3 IOMMUs. All this data is
>>> populated via the corresponding dt node.
>>>
>>> Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
>>> Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
>>
>> This will need to wait until we've made omap3 to be DT only
>> as this will break idling of things for the legacy booting.
>>
>
> OK, will drop this and I will adjust Patch 9 to support the legacy boot
> for OMAP3 ISP.
>
BTW, Tony do you have a new window for omap3 to be DT onyl?
Regards,
Florian
^ permalink raw reply
* Re: [PATCH V6] gpio: New driver for LSI ZEVIO SoCs
From: Linus Walleij @ 2014-02-27 9:31 UTC (permalink / raw)
To: Fabian Vogt
Cc: linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-doc@vger.kernel.org, devicetree@vger.kernel.org,
Grant Likely, Pawel Moll, Rob Landley
In-Reply-To: <1393271698-15829-1-git-send-email-fabian@ritter-vogt.de>
On Mon, Feb 24, 2014 at 8:54 PM, Fabian Vogt <fabian@ritter-vogt.de> wrote:
> This driver supports the GPIO controller found in LSI ZEVIO SoCs.
> It has been successfully tested on a TI nspire CX calculator.
>
> Signed-off-by: Fabian Vogt <fabian@ritter-vogt.de>
I like this version. Patch applied.
Q:
> +config GPIO_ZEVIO
> + bool "LSI ZEVIO SoC memory mapped GPIOs"
> + depends on OF
So OF_GPIO is not needed for the systems using this driver?
DT just used for probing the driver?
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v5 06/11] drivers: of: initialize and assign reserved memory to newly created devices
From: Marek Szyprowski @ 2014-02-27 10:10 UTC (permalink / raw)
To: Grant Likely, linux-kernel, linux-arm-kernel, linaro-mm-sig,
devicetree, linux-doc
Cc: Benjamin Herrenschmidt, Arnd Bergmann, Michal Nazarewicz,
Tomasz Figa, Sascha Hauer, Laura Abbott, Rob Herring,
Olof Johansson, Pawel Moll, Mark Rutland, Stephen Warren,
Ian Campbell, Tomasz Figa, Kumar Gala, Nishanth Peethambaran,
Marc, Josh Cartwright, Catalin Marinas, Will Deacon,
Paul Mackerras
In-Reply-To: <20140226121445.54D2DC40A89@trevor.secretlab.ca>
Hello,
On 2014-02-26 13:14, Grant Likely wrote:
> On Fri, 21 Feb 2014 13:25:22 +0100, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > Use recently introduced of_reserved_mem_device_init() function to
> > automatically assign respective reserved memory region to the newly created
> > platform and amba device.
> >
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
> I'm wary on this patch. It hides the assignment of regions into the core
> code and I worry that it is the wrong level of abstraction. I would
> think that drivers should know that they need a reserved memory region
> and should be calling the API to obtain the region directly rather than
> doing it for them. The reason being is that there may be some situations
> where the common code isn't quite right and the driver needs to override
> the behaviour. If it is called automatically then the driver cannot do
> that.
>
> Is it really a big burden to have the driver call the reserved memory
> init function?
If a device requires very special handling of the reserved memory region,
it may simply provide its own reserved memory region driver which will do
the required early initialization.
If we assume that driver needs to initialize reserved region manually, then
why do we ever bother with adding support for custom reserved memory drivers
and assigning regions to a device node? We can simply stick with just a set
of reserved regions and tell drivers to use them.
In my opinion for most typical use cases a board designer will assign
'dma-shared-pool' driver to the given set of devices, which in turn ensures
that all memory allocations for dma purposes for those device will be
served from that region. It is really not a driver role to initialize it
in such case. Driver should focus on controlling hw operations, regardless
the way the hardware module has been integrated to the system.
I can perfectly imagine a generic driver which operates the same way in any
of the following cases (depends mainly on the hw version): 1) restricted
dma window, 2) iommu for all dma for the given device, 3) fully featured
memory master for dma for the given device (no restrictions), if the
respective kernel subsystems did the correct initialization and provide
their own methods for managing DMA operation. I already have a working
platform glue code for the above cases tested with Samsung multimedia
drivers. No changes to the drivers were required.
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH] phy: micrel: add of configuration for LED mode
From: Ben Dooks @ 2014-02-27 10:22 UTC (permalink / raw)
To: Florian Fainelli; +Cc: linux-kernel, devicetree@vger.kernel.org, netdev
In-Reply-To: <CAGVrzcbUShzdYgEgHJiG8gL11+1DANiJpAk3PL5=jKASqXAyGg@mail.gmail.com>
On 26/02/14 22:20, Florian Fainelli wrote:
> Hi,
>
> 2014-02-26 3:48 GMT-08:00 Ben Dooks <ben.dooks@codethink.co.uk>:
>
> [snip]
>
>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>> index 5a8993b..0c9e434 100644
>> --- a/drivers/net/phy/micrel.c
>> +++ b/drivers/net/phy/micrel.c
>> @@ -148,15 +148,52 @@ static int ks8737_config_intr(struct phy_device *phydev)
>> return rc < 0 ? rc : 0;
>> }
>>
>> +static int kszphy_setup_led(struct phy_device *phydev,
>> + unsigned int reg, unsigned int shift)
>> +{
>> +
>> + struct device *dev = &phydev->dev;
>> + struct device_node *of_node = dev->of_node;
>> + int rc, temp;
>> + u32 val;
>> +
>> + if (!of_node && dev->parent->of_node)
>> + of_node = dev->parent->of_node;
>> +
>> + if (of_property_read_u32(of_node, "micrel,led-mode", &val))
>> + return 0;
>
> This breaks non-OF configuration because of_read_property_read_u32()
> will return -ENOSYS, so you skip the LED register configuration
> entirely, is that intended?
Yes, it is only here for OF case. I should however check if
of_node is available for the !OF case.
>> +
>> + temp = phy_read(phydev, reg);
>> + if (temp < 0)
>> + return temp;
>> +
>> + temp &= 3 << shift;
>
> The compiler cannot verify that we are not overflowing, you might want
> to make sure that shift <= 14 (just in case)
Ok, should I add a WARN_ON(shift > 14) ?
>> + temp |= val << shift;
>> + rc = phy_write(phydev, reg, temp);
>> +
>> + return rc < 0 ? rc : 0;
>
> You could have;
>
> return phy_write(phydev, reg, temp);
Thanks, will change to doing that.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
^ permalink raw reply
* Re: [PATCH v2 RESEND] pwm: Add CLPS711X PWM support
From: Mark Rutland @ 2014-02-27 10:27 UTC (permalink / raw)
To: Alexander Shiyan
Cc: Arnd Bergmann, linux-pwm@vger.kernel.org, Thierry Reding,
devicetree@vger.kernel.org, Rob Herring, Pawel Moll, Ian Campbell,
Kumar Gala, grant.likely@linaro.org
In-Reply-To: <1393433446.816907298@f337.i.mail.ru>
On Wed, Feb 26, 2014 at 04:50:46PM +0000, Alexander Shiyan wrote:
> Hello.
>
> Среда, 26 февраля 2014, 16:00 UTC от Mark Rutland <mark.rutland@arm.com>:
> > On Tue, Feb 25, 2014 at 03:50:32PM +0000, Arnd Bergmann wrote:
> > > On Tuesday 25 February 2014 19:47:57 Alexander Shiyan wrote:
> > > > Вторник, 25 февраля 2014, 16:33 +01:00 от Arnd Bergmann <arnd@arndb.de>:
> > > > > On Tuesday 25 February 2014 19:27:47 Alexander Shiyan wrote:
> > > => >
> > > > > We really want to avoid wildcards in compatible strings. Can you call this
> > > > > "cirrus,cs89712-pwm" to match the first SoC that came with this hardware?
> > > > > Obviously if there was some chip before that (I'm not familiar with the
> > > > > model numbers), use that instead.
> > > > >
> > > > > You can either list all chips you know that have this in the driver,
> > > > > or you use "cirrus,cs89712-pwm" as the fallback, and use the name of
> > > > > the SoC you have as the more specific string.
> > > >
> > > > It seems that in this case we will need to modify the compatibility string
> > > > for other drivers that are already available in the kernel...
> > >
> > > Ah, right. I missed the binding for gpio and serial going in.
> > >
> > > DT maintainers, any suggestion on how we should proceed here?
> > >
> > > AFAICT, clps711x platform support is still work-in-progress, so we don't
> > > have any upstream users to worry about yet, but changing them is still
> > > going to be slightly messy.
> >
> > When allocating any new compatible strings, as Arnd says, we should
> > avoid wildcards as they're usually far too encompassing and end up
> > causing more trouble than they're worth.
> >
> > In this case how problematic are the wildcard strings?
> >
> > I assume we still have specific strings earlier in any compatible list
> > in any case? If not, and there are possible differences, that should be
> > fixed right away.
> >
> > We have a few options:
> >
> > a) Update the docs only.
> >
> > Note in the docs that "cirrus,clps711x-$UNIT" means anything
> > compatible with the $UNIT in the cs89712. This may be
> > counter-intuitive, and if a new clps711x platform comes out with an
> > incompatible $UNIT it should omit "cirrus,clps711x-$UNIT" from its
> > compatible list, but otherwise no harm done.
> >
> > b) Deprecate the existing string.
> >
> > Add "cirrus,cs89712-$UNIT" to the binding docs and driver. Mark
> > "cirrus,clps711x-$UNIT" as deprecated in the binding document.
> > Replace "cirrus,clps711x-$UNIT" with "cirrus,cs89712-$UNIT" in all
> > DTs.
> >
> > This will mean new DTBs won't work with an older kernel, but as
> > support is a work in progress that might not matter. Old DTBs will
> > continue to function.
> >
> > Iff you can guarantee that the old string is not possibly being used
> > by anyone, it can be dropped from the driver. If not it has to remain
> > (though can be commented to be deprecated), so that old DTBs
> > function. It's probably best to leave it there.
> >
> > c) Deprecate, maintaining (forwards) compatibility.
> >
> > As above, but rather than replacing "cirrus,clps711x-$UNIT" with
> > "cirrus,cs89712-$UNIT" in DTs, place "cirrus,cs89712-$UNIT" before
> > "cirrus,clps711x-$UNIT" in DTs. This allows new DTBs to work with
> > older kernels too. Depending on what level of support you have in
> > mainline currently this may or may not be an important consideration.
>
> If I understood correctly, in the variant "a", we change nothing.
> Ie compatibility string is a global to entire platform, and in case of any
> CPU differences, we simply add the additional compatibility string fully
> meets the CPU name, for example for Cirrus Logic EP7312 this will be
> like: "cirrus,ep7312-hw", "cirrus,clps711x-hw".
> Correct?
That depends. Option a still requires that we give a definite meaning to
"cirrus,clps711x-$UNIT" to mean compatible with the unit on the cs89712.
If a new unit is compatible with this, then it can have
"cirrus,clps711x-$UNIT" as a fallback in its compatible list.
If a new unit is not compatible and must be handled differently, then it
should not have "cirrus,clps711x-$UNIT" in its compatible list.
The naming you suggest for "cirrus,ep7312-$UNIT" seems sensible in
either case.
Cheers,
Mark.
^ permalink raw reply
* Re: [PATCH] phy: micrel: add of configuration for LED mode
From: Ben Dooks @ 2014-02-27 10:30 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-kernel@lists.codethink.co.uk, devicetree@vger.kernel.org,
netdev@vger.kernel.org, f.fainelli@gmail.com
In-Reply-To: <20140227091505.GA6945@e106331-lin.cambridge.arm.com>
On 27/02/14 09:15, Mark Rutland wrote:
> On Wed, Feb 26, 2014 at 11:48:00AM +0000, Ben Dooks wrote:
>> Add support for the led-mode property for the following PHYs
>> which have a single LED mode configuration value.
>>
>> KSZ8001 and KSZ8041 which both use register 0x1e bits 15,14 and
>> KSZ8021, KSZ8031 and KSZ8051 which use register 0x1f bits 5,4
>> to control the LED configuration.
>>
>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> ---
>> Documentation/devicetree/bindings/net/micrel.txt | 18 +++++++++
>> drivers/net/phy/micrel.c | 49 ++++++++++++++++++++++--
>> 2 files changed, 63 insertions(+), 4 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/net/micrel.txt
>>
>> diff --git a/Documentation/devicetree/bindings/net/micrel.txt b/Documentation/devicetree/bindings/net/micrel.txt
>> new file mode 100644
>> index 0000000..98a3e61
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/micrel.txt
>> @@ -0,0 +1,18 @@
>> +Micrel PHY properties.
>> +
>> +These properties cover the base properties Micrel PHYs.
>> +
>> +Optional properties:
>> +
>> + - micrel,led-mode : LED mode value to set for PHYs with configurable LEDs.
>> +
>> + Configure the LED mode with single value. The list of PHYs and
>> + the bits that are currently supported:
>> +
>> + KSZ8001: register 0x1e, bits 15..14
>> + KSZ8041: register 0x1e, bits 15..14
>> + KSZ8021: register 0x1f, bits 5..4
>> + KSZ8031: register 0x1f, bits 5..4
>> + KSZ8051: register 0x1f, bits 5..4
>> +
>> + See the respective PHY datasheet for the mode values.
>
> What do these mean, roughly,, and why can the kernel not decide how to
> cnofigure these?
Board specific, in the case of the Lager one of the LEDs is connected
to the ethernet mac block to indicate link, however the default mode
is not for just "Link" so we have to change it.
> In general we prefer to not place raw register values in the DT, and I'd
> like to know why we'd have to here.
I could copy out stuff from the data-sheet, but I was trying to avoid a
copy and paste job.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
^ permalink raw reply
* Re: [PATCH v2 3/6] spi: sh-msiof: Add support for R-Car H2 and M2
From: Laurent Pinchart @ 2014-02-27 10:41 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Mark Brown, Takashi Yoshii, Magnus Damm, linux-spi, Linux-sh list,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Geert Uytterhoeven,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAMuHMdU_ej9cE=qmRL2WqvEW+fNA_5bj7OC2=7B7ZR33FaFUTA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi Geert,
On Thursday 27 February 2014 09:39:54 Geert Uytterhoeven wrote:
> On Wed, Feb 26, 2014 at 11:16 PM, Laurent Pinchart wrote:
> >> diff --git a/Documentation/devicetree/bindings/spi/sh-msiof.txt
> >> b/Documentation/devicetree/bindings/spi/sh-msiof.txt index
> >> eae3c8c9300e..1f0cb33763a1 100644
> >> --- a/Documentation/devicetree/bindings/spi/sh-msiof.txt
> >> +++ b/Documentation/devicetree/bindings/spi/sh-msiof.txt
> >> @@ -1,8 +1,13 @@
> >>
> >> Renesas MSIOF spi controller
> >>
> >> Required properties:
> >> -- compatible : "renesas,sh-msiof" for SuperH, or
> >> +- compatible : "renesas,msiof-<soctype>" for SoCs,
> >> + "renesas,sh-msiof" for SuperH, or
> >> "renesas,sh-mobile-msiof" for SH Mobile series.
> >> + Examples with soctypes are:
> >> + "renesas,msiof-sh7724" (SH)
> >
> > Given that the driver doesn't handle the "renesas,msiof-sh7724" compatible
> > string this might not be a good example. Furthermore SuperH doesn't have
> > DT support. I would thus drop the "renesas,sh-msiof" compatible string
> > from patch 1/6 and wouldn't mention sh7724 here. I very much doubt that
> > someone would have developed DT support for SuperH on the side and
> > shipped products that would be broken by this change :-)
>
> Upon reading your comment again: do you suggest to also remove the plain
> "renesas,sh-msiof"? That one was present before, since DT support was added
> to the driver in
>
> commit cf9c86efecf9510e62388fd174cf607671c59fa3
> Author: Bastian Hecht <hechtb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date: Wed Dec 12 12:54:48 2012 +0100
>
> spi/sh-msiof: Add device tree parsing to driver
>
> This adds the capability to retrieve setup data from the device tree
> node. The usage of platform data is still available.
>
> Signed-off-by: Bastian Hecht <hechtb+renesas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>
> So I prefer not to remove any pre-existing compatible values.
> Do you agree?
I'd like to remove it (in a separate patch) if we can. The reason is that
keeping the DT ABI both forward- and backward-compatible is pretty painful
enough without having to care about compatibility strings that have no user.
I'd rather work on adding DT support for SuperH MSIOF later when we'll have a
platform we can test it on, instead of trying to guess now what the needs will
be, get users later and realize even later on that we made a mistake that we
can't fix because those users will have DT binaries in the wild. Every
unneeded bit of DT bindings that we keep in the kernel is one potential
problem for future binary compatibility.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v4 3/3] Documentation: of: Document graph bindings
From: Tomi Valkeinen @ 2014-02-27 10:41 UTC (permalink / raw)
To: Philipp Zabel
Cc: Russell King - ARM Linux, Mauro Carvalho Chehab, Grant Likely,
Rob Herring, Sylwester Nawrocki, Laurent Pinchart, Kyungmin Park,
linux-kernel, linux-media, devicetree, Guennadi Liakhovetski
In-Reply-To: <1393498356.4507.32.camel@paszta.hi.pengutronix.de>
[-- Attachment #1: Type: text/plain, Size: 3442 bytes --]
On 27/02/14 12:52, Philipp Zabel wrote:
> This is a bit verbose, and if your output port is on an encoder device
> with multiple inputs, the correct port number would become a bit
> unintuitive. For example, we'd have to use port@4 as the output encoder
> units that have a 4-port input multiplexer and port@1 for those that
> don't.
Hmm, sorry, I don't follow...
The port numbers should be fixed for a particular device. If the device
has 4 input ports, the output port would always be port@4, no matter how
many of the input ports are actually used.
I don't have anything against having the ports described in the SoC
dtsi. But I do think it may make it a bit unclear that the ports are
from the same device, and share things like pinmuxing. Both approaches
should work fine, afaics.
However, if, instead, we could have the pinmuxing and other relevant
information in the port or endpoint nodes, making the ports independent
of each other and of the device behind them, I argument above would
disappear.
Also, while I'm all for making the dts files clear, I do think the one
writing the dts still needs to go carefully through the binding docs.
Say, a device may need a gpio list with a bunch of gpios. The developer
just needs to read the docs and know that gpio #3 on the list is GPIO_XYZ.
So I don't see it as a major problem that the board developer needs to
know that port@1 on OMAP3's DSS is SDI output.
>> Here I guess I could have:
>>
>> &dss {
>> status = "ok";
>>
>> pinctrl-names = "default";
>> pinctrl-0 = <&dss_sdi_pins>;
>>
>> vdds_sdi-supply = <&vaux1>;
>> };
>
> What is supplied by this regulator. Is it the PHY?
Yes.
>> Actually, somewhat aside the subject, I'd like to have the pinctrl and
>> maybe regulator supply also per endpoint, but I didn't see how that
>> would be possible with the current framework. If a board would need to
>> endpoints for the same port, most likely it would also need to different
>> sets of pinctrls.
>
> I have a usecase for this the other way around. The i.MX6 DISP0 parallel
> display pads can be connected to two different display controllers via
> multiplexers in the pin control block.
>
> parallel-display {
> compatible = "fsl,imx-parallel-display";
> #address-cells = <1>;
> #size-cells = <0>;
>
> port@0 {
> endpoint {
> remote-endpoint = <&ipu1_di0>;
> };
> };
>
> port@1 {
> endpoint {
> remote-endpoint = <&ipu2_di0>;
> };
> };
>
> disp0: port@2 {
> endpoint {
> pinctrl-names = "0", "1";
> pinctrl-0 = <&pinctrl_disp0_ipu1>;
> pinctrl-1 = <&pinctrl_disp0_ipu2>;
> remote-endpoint = <&lcd_in>;
> };
> }
> };
>
> Here, depending on the active input port, the corresponding pin control
> on the output port could be set. This is probably quite driver specific,
> so I don't see yet how the framework should help with this. In any case,
> maybe this is a bit out of scope for the generic graph bindings.
Hmm, why wouldn't you have the pinctrl definitions in the ports 0 and 1,
then, if it's about selecting the active input pins?
I think the pinctrl framework could offer ways to have pinctrl
definitions anywhere in the DT structure. It'd be up to the driver to
point to the pinctrl in the DT, ask the framework to parse them, and
eventually enable/disable the pins.
But yes, it's a bit out of scope =).
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* Re: [PATCH v4 3/3] Documentation: of: Document graph bindings
From: Philipp Zabel @ 2014-02-27 10:52 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Russell King - ARM Linux, Mauro Carvalho Chehab, Grant Likely,
Rob Herring, Sylwester Nawrocki, Laurent Pinchart, Kyungmin Park,
linux-kernel, linux-media, devicetree, Guennadi Liakhovetski
In-Reply-To: <530EF294.7070801@ti.com>
Hi Tomi,
Am Donnerstag, den 27.02.2014, 10:08 +0200 schrieb Tomi Valkeinen:
> On 26/02/14 17:47, Philipp Zabel wrote:
> > Please let's not make it mandatory for a port node to contain an
> > endpoint. For any device with multiple ports we can't use the simplified
> > form above, and only adding the (correctly numbered) port in all the
> > board device trees would be a pain.
>
> That's true. I went with having the ports in the board file, for example
> on omap3 the dss has two ports, and N900 board uses the second one:
>
> &dss {
> status = "ok";
>
> pinctrl-names = "default";
> pinctrl-0 = <&dss_sdi_pins>;
>
> vdds_sdi-supply = <&vaux1>;
>
> ports {
> #address-cells = <1>;
> #size-cells = <0>;
>
> port@1 {
> reg = <1>;
>
> sdi_out: endpoint {
> remote-endpoint = <&lcd_in>;
> datapairs = <2>;
> };
> };
> };
> };
This is a bit verbose, and if your output port is on an encoder device
with multiple inputs, the correct port number would become a bit
unintuitive. For example, we'd have to use port@4 as the output encoder
units that have a 4-port input multiplexer and port@1 for those that
don't.
> Here I guess I could have:
>
> &dss {
> status = "ok";
>
> pinctrl-names = "default";
> pinctrl-0 = <&dss_sdi_pins>;
>
> vdds_sdi-supply = <&vaux1>;
> };
What is supplied by this regulator. Is it the PHY?
> &dss_sdi_port {
> sdi_out: endpoint {
> remote-endpoint = <&lcd_in>;
> datapairs = <2>;
> };
> };
>
> But I didn't like that as it splits the pincontrol and regulator supply
> from the port/endpoint, which are functionally linked together.
>
> Actually, somewhat aside the subject, I'd like to have the pinctrl and
> maybe regulator supply also per endpoint, but I didn't see how that
> would be possible with the current framework. If a board would need to
> endpoints for the same port, most likely it would also need to different
> sets of pinctrls.
I have a usecase for this the other way around. The i.MX6 DISP0 parallel
display pads can be connected to two different display controllers via
multiplexers in the pin control block.
parallel-display {
compatible = "fsl,imx-parallel-display";
#address-cells = <1>;
#size-cells = <0>;
port@0 {
endpoint {
remote-endpoint = <&ipu1_di0>;
};
};
port@1 {
endpoint {
remote-endpoint = <&ipu2_di0>;
};
};
disp0: port@2 {
endpoint {
pinctrl-names = "0", "1";
pinctrl-0 = <&pinctrl_disp0_ipu1>;
pinctrl-1 = <&pinctrl_disp0_ipu2>;
remote-endpoint = <&lcd_in>;
};
}
};
Here, depending on the active input port, the corresponding pin control
on the output port could be set. This is probably quite driver specific,
so I don't see yet how the framework should help with this. In any case,
maybe this is a bit out of scope for the generic graph bindings.
regards
Philipp
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox