Hi, On Sat, Dec 20, 2014 at 6:20 PM, Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx> >>>> 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@xxxxxxxxxx> >>>>> Acked-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxx> >>>>> Acked-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx> >>>>> --- >>>>> .../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? Hans has made a good argument, so I take back any doubts I raised. ChenYu -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
