On Tue, 4 Apr 2023 09:20:24 +0200 Fabrizio Lamarque <fl.scratchpad@xxxxxxxxx> wrote: > On Sat, Apr 1, 2023 at 4:21 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > 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. > > The bug was introduced in June 2021 from c9ec2cb328e3 iio: adc: > ad7192: use devm_clk_get_optional() for mclk, where there is a clearly > mistaken logic inversion. > > Link: https://lore.kernel.org/all/20210513120752.90074-10-aardelean@xxxxxxxxxxx/#r > > If you agree, I can send a revised patch set (perhaps within a new thread), > with the additional clarifications on clock usage in bindings documentation, > and one third patch for undocumented bindings. Sounds good to me. > > There is one last issue we are investigating, we have a work-around but > we have not found the root cause yet. I will post about it later, in > the hope the > patch set would include all the changes needed to have the driver operating > as expected. Sounds interesting :( > > Thank you once again for this review. NP. Thanks for sending your fixes upstream. Jonathan > > Fabrizio Lamarque > > > > > > > > > > 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á > > > > > >
