Re: [PATCH 1/2] power: reset: at91: add sama5d3 reset function

[Maxime Ripard <maxime.ripard@free-electrons.com>]

[-- Attachment #1: Type: text/plain, Size: 1850 bytes --]

Hi Josh,

On Mon, Jul 13, 2015 at 11:21:44AM +0800, Josh Wu wrote:
> On 7/11/2015 12:12 AM, Nicolas Ferre wrote:
> >Le 10/07/2015 14:31, Maxime Ripard a écrit :
> >>On Fri, Jul 10, 2015 at 02:09:07PM +0200, Alexandre Belloni wrote:
> >>>Hi,
> >>>
> >>>On 10/07/2015 at 15:56:52 +0800, Josh Wu wrote :
> >>>>I would agree with Maxime. Currently all latest chip reset function is
> >>>>compatible with the atmel,sama5d3-rstc.
> >>>>So check compatible string is enough for now.
> >>>>But of cause if we have other incompatible reset in future with new chip,
> >>>>the structure like you said is needed.
> >>>We managed to avoid using of_machine_is_compatible() in all the at91
> >>>drivers. I'd like to keep it that way. It was painful enough to remove
> >>>all those cpu_is_at91xxx calls.
> >>That's your call...
> >>
> >>>Also, using it is trying to match strings and will result in longer boot
> >>>times.
> >>Have you looked at the implementation of of_match_device? If that's
> >>really a concern to you, you should actually avoid it.
> >I agree: let's keep it simple and use of_match_device().
> 
> Ok. I will keep it as it is now:  use the (match->data != sama5d3_restart)
> for the condition.

I'm not just that's been an option in our discussion so far.

Nicolas said that he was agreeing with me, but at the same time said
the complete opposite of what I was arguing for, so I'm not really
sure what's really on his mind, but the two options that were
discussed were to remove that test, and either:

  - Use of_device_is_compatible to prevent the loop execution

  - define a structure with a flag to say whether you need the ram
    controller quirk or not, and test that flag.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]