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 :)? > 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... - Nuno Sá >
