Re: [linux-sunxi] [PATCH v2 04/13] rc: sunxi-cir: Add support for an optional reset controller

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 19-01-15 15:10, Chen-Yu Tsai wrote:
> Hi,
>
> On Sat, Dec 20, 2014 at 6:20 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 19-12-14 19:17, Maxime Ripard wrote:
>>>
>>> Hi,
>>>
>>> On Thu, Dec 18, 2014 at 09:50:26AM +0100, Hans de Goede wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 18-12-14 03:48, Chen-Yu Tsai wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On Thu, Dec 18, 2014 at 1:18 AM, Hans de Goede <hdegoede@redhat.com>
>>>>> wrote:
>>>>>>
>>>>>> On sun6i the cir block is attached to the reset controller, add support
>>>>>> for de-asserting the reset if a reset controller is specified in dt.
>>>>>>
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> Acked-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>>>>>> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>>>>>> ---
>>>>>>    .../devicetree/bindings/media/sunxi-ir.txt         |  2 ++
>>>>>>    drivers/media/rc/sunxi-cir.c                       | 25
>>>>>> ++++++++++++++++++++--
>>>>>>    2 files changed, 25 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/media/sunxi-ir.txt
>>>>>> b/Documentation/devicetree/bindings/media/sunxi-ir.txt
>>>>>> index 23dd5ad..6b70b9b 100644
>>>>>> --- a/Documentation/devicetree/bindings/media/sunxi-ir.txt
>>>>>> +++ b/Documentation/devicetree/bindings/media/sunxi-ir.txt
>>>>>> @@ -10,6 +10,7 @@ Required properties:
>>>>>>
>>>>>>    Optional properties:
>>>>>>    - linux,rc-map-name : Remote control map name.
>>>>>> +- resets : phandle + reset specifier pair
>>>>>
>>>>>
>>>>> Should it be optional? Or should we use a sun6i compatible with
>>>>> a mandatory reset phandle? I mean, the driver/hardware is not
>>>>> going to work with the reset missing on sun6i.
>>>>>
>>>>> Seems we are doing it one way for some of our drivers, and
>>>>> the other (optional) way for more generic ones, like USB.
>>>>
>>>>
>>>> I do not believe that we should add a new compatible just because
>>>> the reset line of a block is hooked up differently. It is the
>>>> exact same ip-block. Only now the reset is not controlled
>>>> through the apb-gate, but controlled separately.
>>>
>>>
>>> He has a point though. Your driver might very well probe nicely and
>>> everything, but still wouldn't be functional at all because the reset
>>> line wouldn't have been specified in the DT.
>>
>>
>> Right, just like other drivers we've, see e.g.:
>>
>> Documentation/devicetree/bindings/mmc/sunxi-mmc.txt
>>
>> Which is dealing with this in the same way.
>>
>>> The easiest way to deal with that would be in the bindings doc to
>>> update it with a compatible for the A31, and mentionning that the
>>> reset property is mandatory there.
>>
>>
>> No the easiest way to deal with this is to expect people writing
>> the dts to know what they are doing, just like we do for a lot
>> of the other blocks in sun6i.
>>
>> Maybe put a generic note somewhere that sun6i has a reset controller,
>> and that for all the blocks with optional resets property it should
>> be considered mandatory on sun6i ?
>>
>> I'm sorry but I'm not going to make this change for the ir bindings
>> given that we've the same situation in a lot of other places.
>>
>> Consistency is important. Moreover I believe that having a sun6i
>> specific compatible string is just wrong, since it is the exact
>> same hardware block as on sun5i, just with its reset line routed
>> differently, just like e.g. the mmc controller, the uarts or the gmac
>> all of which also do not have a sun6i specific compatible to enforce
>> reset controller usage.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>> Note that the code itself might not change at all though. I'd just
>>> like to avoid any potential breaking of the DT bindings themselves. If
>>> we further want to refine the code, we can do that however we want.
>>>
>>> I have a slight preference for a clean error if reset is missing, but
>>> I won't get in the way just for that.
>
> Seems this patch and the following patch were overlooked after the
> discussion. Any chance we could get this in?

I'm a linux/media sub-maintainer, so I've already send a pull-req for
these 2 to the linux/media maintainer, iow this is taken care of :)

Regards,

Hans