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. In addition, bindings documentation states clock property is mandatory. I tend to dislike any solution that keeps the existing flow (and do not break compatibility). Nothing that comes to my mind would be as simple as this logical flow correction. In addition, I checked the driver history and it looks like the minor bugs we found in corrent implementation (see previous patch about NULL pointer dereference) have been there for several years. In case backwards compatibility is a real issue, I could rewrite the patch so that an explicit external clock source property could be used instead. If so, I kindly ask you to suggest an acceptable property name. > > I kindly ask your feedback, I may adjust the patch according to your > > suggestions. I could also follow up with another patch on > > documentation, containing the following (related) issues: > > > > - adi,int-clock-output-enable is undocumented > > - adi,clock-xtal is undocumented > > - regulator name avdd and its description is quite misleading, since > > this is unrelated to the AVdd pin (#20) of AD7192; it is used instead > > as reference voltage (REFIN1 on #15/#16 or REFIN2 on #7/#8). See > > int_vref_mv variable within driver implementation. > > > > Don't think the above text belongs to this commit message. That said, > it would be great if you could follow up with a couple of patches to > document the undocumented properties. As for the regular name, I think > it's not so trivial to change it's name because there could be already > users using the property like this. So, I guess we have two ways: > > 1) Just add some description in the property to state what's this > regulator really is about. > 2) This one is more complex but might be the right one... Deprecrate > the current property (you can mark a property as deprecated in the > bindings) and add the new one with a proper name. Then, you need to > change the driver accordingly keeping in mind that it must still work > with the old, deprecrated property. > > Was lazy to ckeck but I'm assuming the bindings are already in yaml... I can confirm bindings are already documented in yaml. I will try to submit a patch with the missing documentation and the proposed solution 2 in the next days. > > > Signed-off-by: Fabrizio Lamarque <fl.scratchpad@xxxxxxxxx> > > --- linux/drivers/iio/adc/ad7192.c 2023-03-13 19:32:42.646239506 > > Anyways, for this patch and with a proper fixes tag (and with the > unrelated text removed from the commit message): > > Reviewed-by: nuno.sa@xxxxxxxxxx > >
