On Thu, 30 Mar 2023 09:53:29 +0200 Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > On Wed, 2023-03-29 at 23:23 +0200, Fabrizio Lamarque wrote: > > On Wed, Mar 29, 2023 at 5:15 PM Nuno Sá <noname.nuno@xxxxxxxxx> > > wrote: > > > > > > On Mon, 2023-03-27 at 22:21 +0200, Fabrizio Lamarque wrote: > > > > Allow the use of external clock when mclk clock is defined. > > > > When defining a mclk clock source in device tree with adi,clock- > > > > xtal > > > > property, the external crystal oscillator is not turned on. > > > > Without the change, the driver always uses the internal clock > > > > even > > > > when mclk clock is defined. > > > > > > > > Current implementation seems to contain a typo, since it expected > > > > st->mclk to be NULL within ad7192_of_clock_select() in order to > > > > select > > > > the external clock, but, if null, external clock cannot loaded > > > > correctly (out of bounds due to invalid mclk) in ad7192_probe(). > > > > > > > > I believe this patch follows the author's intended behavior. > > > > After applying this patch, the external oscillator is started as > > > > expected. > > > > > > > > > > Yes, looks like a valid fix... Just missing a Fixes tag. > > > > Thank you for having this patch reviewed. > > Here is a backwards compatibility note I believe you should be aware > > of. > > > > Without this patch, the DT node shall contain (any) clock property > > for the driver to be loaded properly. > > The clock frequency is ignored, as it is ignored adi,clock-xtal > > property. > > In case clock property is not set, the initialization always fails. > > There is no way to use an external clock source. > > As you already verified from source code, this was not intentional. > > > > While the proposed patch should make the DT config load as intended, > > there is a possible side effect on existing implementations relying > > on > > internal clock source only, that would be deactivated when clock > > property > > is defined in DT. > > Hmm, I see. I would argue anyone relying on that should have just > stepped forward and fix the real issue as you did :). So, maybe there's > no one actually relying on this but that's a big __maybe__... Not > really sure how to handle this. I'm not sure we want to keep > compatibility with something that's clearly wrong but often we need to > "live" with past messes. > > Jonathan I guess your input will be valuable here :)? Rather odd if they have bindings that say there is a clock but 'rely' on the internal clock as opposed to the current bug meaning they aren't using the external clock (but everything would work fine if they were). So I'd expect that we will only see a problem if someone has another bug somewhere else that means that clock isn't working correctly. At that point they will almost certainly want to fix that bug. > > > In addition, bindings documentation states clock > > property is mandatory. > > > > This looks at least suspicious to me given the fact we have an internal > alternative. Clearly looks wrong to me... Agreed. I don't think it should be mandatory in the binding doc. Thanks, Jonathan > > - Nuno Sá > >
