Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200726111215.22361-1-konradybcio@gmail.com> <20200726111215.22361-10-konradybcio@gmail.com>
 <20200726112920.GA1286220@kroah.com>
In-Reply-To: <20200726112920.GA1286220@kroah.com>
From:   Konrad Dybcio <konradybcio@gmail.com>
Date:   Sun, 26 Jul 2020 13:40:46 +0200
Message-ID: <CAMS8qEV7jhbHqpXE2UOaXBVM5WbCThaGrcD3wiH9kf6h_K-qeA@mail.gmail.com>
Subject: Re: [PATCH 9/9] soc/qcom: Add REVID driver
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     Martin Botka <martin.botka1@gmail.com>,
        Rob Clark <robdclark@gmail.com>, Sean Paul <sean@poorly.run>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Rob Herring <robh+dt@kernel.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Vinod Koul <vkoul@kernel.org>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Felipe Balbi <balbi@kernel.org>,
        Jordan Crouse <jcrouse@codeaurora.org>,
        zhengbin <zhengbin13@huawei.com>,
        Jeffrey Hugo <jeffrey.l.hugo@gmail.com>,
        AngeloGioacchino Del Regno <kholk11@gmail.com>,
        Ben Dooks <ben.dooks@codethink.co.uk>,
        Krzysztof Wilczynski <kw@linux.com>,
        Harigovindan P <harigovi@codeaurora.org>,
        Brian Masney <masneyb@onstation.org>,
        Sam Ravnborg <sam@ravnborg.org>,
        Xiaozhe Shi <xiaozhes@codeaurora.org>,
        Manu Gautam <mgautam@codeaurora.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        "open list:DRM PANEL DRIVERS" <dri-devel@lists.freedesktop.org>,
        freedreno <freedreno@lists.freedesktop.org>,
        DTML <devicetree@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-usb@vger.kernel.org, linux-clk@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Greg, thanks for your review!

>Why do we need this noise in the kernel log?

I guess it could be left there as a debug print? Knowing your hardware
revision seems like a good, but yeah, not a necessary thing.

>You can drop the GPL boilerplate text and add a proper SPDX line at the
>top.

Seems I only did that in my other local tree.. whoops!

>Drivers should always use dev_err() and friends, as you have access to a
>struct device * always.  Please fix up the driver here to use that api
>instead, no pr_* should be needed at all.

Will do.

>Horrible global symbol name.  Who calls this?

Welcome to development on qcom platforms :D

>This is the last patch in
>the series, so if there is no user for this, please don't export it.

Other downstream drivers make use of it.. need to get this up first, sorry :V

>Why do you need a .h file in the include directory if only a single .c
>file needs it?  Just put that info in the .c file itself.

Again, other downstream drivers which some people and I intend to
bring to upstream standards use that to access the PMIC model/hw revision.

>But again, who uses this module?  If it's only good for a single line in
>the kernel log, that feels like a huge waste to me.

downstream-kernel-dir$ rg -l qpnp-revid.h | wc -l
25

So yeah, quite a bunch of other qcom-specific drivers.

I'll try to fix these and send a v2.

Regards
Konrad