On Thu, Apr 13, 2023 at 6:35 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: > > On 13/04/2023 18:07, Fabrizio Lamarque wrote: > > On Thu, Apr 13, 2023 at 4:21 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: > >> > >> On 13/04/2023 10:36, Fabrizio Lamarque wrote: > > > > Current documentation does not follow existing source code implementation. > > Bindings describe hardware, not current implementation. OK, I'll keep in mind the perspective while describing the change. The bug corrected by 2/2 was found because the external crystal oscillator did not work and the internal, fixed clock was always used. Before the correction, the driver did not load correctly without the clocks property. At the same time, the specified frequency was ignored and the fixed, internal frequency was used instead. After the correction, I found the documentation telling that clock property is mandatory, while omitting it is the way to use the internal, fixed frequency, clock. The documentation did not explain how to choose between the internal and external oscillator, but the driver was already designed to implement the choice. > > The driver has been originally designed to operate with the internal > > clock when clocks property is omitted. > > Not really a reason to do it. Reason could be - hardware does not always > need clock input. I hope the change in perspective will be enough. The external clock is mandatory for some applications. The internal clock might be required for others. > > > > > I thought the reason is clear from patch 2, but, as Nuno Sá already > > suggested, I will describe the reasons in full again, each time I post > > a revised patch set, even if it is quite verbose. > > Your commit must answer to why you are doing it. What you are doing is > easily visible from the diff. Rephrase commit msg to explain it and add > proper rationale (hardware related, not driver). I am really just suggesting to align the documentation with the driver, since the driver operates the hardware as expected (after the two regressions fixes). Without this change, one should read the source code to understand which clock is used and when, which bindings have to be applied, and find that documentation mandates an (already) optional property. > > > > >> > >>> Renamed avdd-supply to vreg-supply, while keeping backward compatibility > >>> (deprecated avdd-supply). > >> > >> Why? > > > > From AD7192 datasheet, you may see AVDD pin/voltage has no > > relationship with VREF pin/voltage. > > avdd-supply name is misleading, since it is treated in code as AVDD > > pin and iio reference voltage instead. > > Then why removing AVDD? It is a supply, as you said, thus bindings > should describe it. I don't understand why AVDD is being deprecated. Please ignore the deprecation and the additional avdd-supply property. It will be removed from the next patch version. I won't be able to provide any patch here without breaking compatibility. BTW, avdd-supply has nothing to do with vref on the hardware. In the driver avdd-supply voltage is used as if it were the reference voltage (VREF pin). This change will be removed from this patch set, maybe the driver author could provide something acceptable. The idea to deprecate it has been suggested in the thread related to the previous patch version. > > > > I kindly ask you whether the entire (corrected) change on the > > documentation file only (without any change on the driver source code) > > could be accepted as a single patch. > > I don't understand the question. Each change should be one logical > change, but bindings are not related to the driver. The question came after this: On Thu, Apr 13, 2023 at 1:21 PM Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > > I kindly ask you to confirm if, as per your suggestion, I should send > > a v3 patch series with the proper "fixes" tag and this last one > > changed as follows: > > > > - No changes on driver side (keep avdd-supply instead of vref-supply) > > - Indicate in bindings documentation that avdd-supply is vref instead > > (with the "Phandle to reference voltage regulator") > > - Add dependencies to yaml bindings > > > Yeps, but note that for the bindings patch you are making distinct changes ( > adding missing properties and changing one) so someone might complain. But for > me, personally, they are simple enough that can go in one patch. Just properly > document it in the commit description. I really need to send a proper, complete and acceptable v3 patch set, or drop patch 3/3 from the set. Would you accept the change to adi,ad7192.yaml file alone with both the change in description of avdd-supply and the additional missing properties? Is "Phandle to reference voltage regulator" as a description for avdd-supply acceptable on your side? In case you feel the proposed adjustments are not enough or changes in documentation are unworthy, I feel it would be better to leave them to the driver maintainers. Otherwise, I'll do my best (according to my limited expertise, to the available time and the acceptable costs for the company I work for) to provide something acceptable. Thank you again. Best regards, Fabrizio Lamarque
