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: >>> 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. Bindings describe hardware, not current implementation. > Patch 2/3 fixes a commit that caused an unwanted logical inversion and > thus prevented the use of external clock/crystal. OK > 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 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). > >> >>> 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. > 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. Just use get_maintainers.pl. > > 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. I don't understand the question. Each change should be one logical change, but bindings are not related to the driver. > > 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. Yep... and this should stop you from sending it. > I will make sure these tests pass without any warning. The patch is corrupted. Try to apply it from mailing lists... Best regards, Krzysztof
