On Thu, Apr 13, 2023 at 4:21 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: > > On 13/04/2023 10:36, Fabrizio Lamarque wrote: > > Added undocumented properties: > > Use imperative. > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > > > > > - adi,clock-xtal > > - adi,int-clock-output-enable > > > > Removed clocks from required properties. > > Why? Current documentation does not follow existing source code implementation. Patch 2/3 fixes a commit that caused an unwanted logical inversion and thus prevented the use of external clock/crystal. The driver has been originally designed to operate with the internal clock when clocks property is omitted. 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. > > > 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. The option to change the regulator name or add a third regulator would have broken compatibility. Other ADI drivers already have the vref-supply property in place. Here again I partially left the reasons in the first thread, sorry. In any case I will remove this change on a revised patch set. I will leave the avdd-supply name but I'll change the description in documentation. > > Use subject prefixes matching the subsystem (which you can get for > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory > your patch is touching). > The single change on documentation will be prepended by dt-bindings: iio: ad7192: The invalid change I suggested intended to change avdd to vref name in the driver too. I misinterpreted the meaning of a single "logical change", sorry. > Please use scripts/get_maintainers.pl to get a list of necessary people > and lists to CC. It might happen, that command when run on an older > kernel, gives you outdated entries. Therefore please be sure you base > your patches on recent Linux kernel. > > You decided to ignore quite a lot of entries, but most important - also > lists, so it won't even be tested. The patch is indeed based on the latest version; however the driver maintainer does not work in ADI any more; the first time I sent a message I got the email bounced back. I see I omitted a necessary step though. You are right in saying that I did not follow carefully the instructions provided, but it was not deliberate. It's the first time I am trying to send back the changes. I appreciate the feedback and corrections; in the next patch set I will try to remedy everything you indicated. > > > --- > > .../bindings/iio/adc/adi,ad7192.yaml | 28 +++++++++++++++---- > > Bindings are always separate patches. > > Corrupted patch. > > Run checkpatch, test your patches with dt_binding_check. This really > needs a lot of work. 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. Unless I was wrong in doing copy/paste, the only feedback I got from the tests is a warning message telling that the changes to documentation should be isolated from source code changes. I will make sure these tests pass without any warning. Thank you and best regards, Fabrizio Lamarque
