Re: [PATCH v5 2/3] memory: omap-gpmc: add support for wait pin polarity

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Krzysztof,

On Tue, 2022-09-20 at 13:23 +0200, Krzysztof Kozlowski wrote:
> On 20/09/2022 12:12, Niedermayr, BENEDIKT wrote:
> > > I commented exactly below the line which I question. I don't question
> > > other lines. So let me be a bit more specific:
> > > 
> > > Why do you need
> > > "p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT"
> > > ? Can you write a scenario where this is useful?
> > > 
> > Ok. I think I got you now. Sorry I'm relatively new to OSS contributions, so please be patient with me...
> > 
> > If I remove that part of the if clause, then an error message would be printed in case "p->wait_pin_polarity == WAITPINPOLARITY_DEFAULT".
> 
> Exactly this will happen. As expected. This value cannot appear in DTS,
> therefore I would expect error message.
> 
> Now you allow such value in DTS which is not the same as your bindings.
> 
And now I completely got it...
With this implementation it's even possible to set WAITPINPOLARITY_DEFAULT in the DT...

Ok, changing this will lead to an error message if the "gpmc,wait-pin-polarity" is not set in DT. Means the DT property is more orless not an optional
property anymore.
If one defines the wait-pin without defining the polarity the driver probes successfully but and error message is printed.
Is this an acceptable solution for you?


> 
> > But this is a not an error case. WAITPINPOLARITY_DEFAULT is a valid value, is assigned right before the if clause as an init value(not extracted from
> > DT),
> > and leads to not touching the GPMC_CONFIG register in gpmc_cs_program_settings().
> > So in gpmc_cs_program_settings() if:
> >     p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH -> Issue a write to the GPMC_CONFIG register
> >     p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW  -> Issua a write to the GPMC_CONFIG register
> >     p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT     -> Do not touch the GPMC_CONFIG register
> > 
> > We want to preserve the reset value of the GPMC_CONFIG register in case the DT does not use the "gpmc,wait-pin-polarity" property. Otherwise
> > we might break platforms which rely on these reset values. 
> 
> Best regards,
> Krzysztof
Cheers,
benedikt





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux