linux-kernel - Re: [PATCH 2/3] thermal: sun50i: add thermal driver for h6

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190521142734.fvhdomqhuntpop52@flea>
Date:   Tue, 21 May 2019 16:27:34 +0200
From:   Maxime Ripard <maxime.ripard@...tlin.com>
To:     Frank Lee <tiny.windzz@...il.com>,
        Mark Rutland <mark.rutland@....com>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        catalin.marinas@....com, will.deacon@....com,
        bjorn.andersson@...aro.org,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        paulmck@...ux.ibm.com, stefan.wahren@...e.com,
        Linux PM <linux-pm@...r.kernel.org>,
        Chen-Yu Tsai <wens@...e.org>,
        Jagan Teki <jagan@...rulasolutions.com>,
        Andy Gross <andy.gross@...aro.org>, rui.zhang@...el.com,
        devicetree@...r.kernel.org, marc.w.gonzalez@...e.fr,
        Eduardo Valentin <edubezval@...il.com>,
        enric.balletbo@...labora.com, robh+dt@...nel.org,
        Jonathan.Cameron@...wei.com,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        olof@...om.net, David Miller <davem@...emloft.net>
Subject: Re: [PATCH 2/3] thermal: sun50i: add thermal driver for h6

On Tue, May 21, 2019 at 12:27:21PM +0200, Ondřej Jirman wrote:
> On Tue, May 21, 2019 at 10:05:15AM +0200, Maxime Ripard wrote:
> > On Sat, May 18, 2019 at 01:27:39AM +0800, Frank Lee wrote:
> > > On Fri, May 17, 2019 at 3:36 PM Maxime Ripard <maxime.ripard@...tlin.com> wrote:
> > > >
> > > > On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote:
> > > > > > > +struct sun50i_thermal_chip {
> > > > > > > +     int     sensor_num;
> > > > > > > +     int     offset;
> > > > > > > +     int     scale;
> > > > > > > +     int     ft_deviation;
> > > > > > > +     int     temp_calib_base;
> > > > > > > +     int     temp_data_base;
> > > > > > > +     int     (*enable)(struct tsens_device *tmdev);
> > > > > > > +     int     (*disable)(struct tsens_device *tmdev);
> > > > > > > +};
> > > > > >
> > > > > > I'm not super fond of having a lot of quirks that are not needed. If
> > > > > > we ever need those quirks when adding support for a new SoC, then
> > > > > > yeah, we should totally have some, but only when and if it's needed.
> > > > > >
> > > > > > Otherwise, the driver is more complicated for no particular reason.
> > > > >
> > > > > This is unavoidable because of the difference in soc.
> > > >
> > > > I know, but this isn't my point.
> > > >
> > > > My point is that at this time of the driver development, we don't know
> > > > what is going to be needed to support all of those SoCs.
> > > >
> > > > Some of the parameters you added might not be needed, some parameters
> > > > might be missing, we don't know. So let's keep it simple for now.
> > > >
> > > > > > > +static int tsens_probe(struct platform_device *pdev)
> > > > > > > +{
> > > > > > > +     struct tsens_device *tmdev;
> > > > > > > +     struct device *dev = &pdev->dev;
> > > > > > > +     int ret;
> > > > > > > +
> > > > > > > +     tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL);
> > > > > > > +     if (!tmdev)
> > > > > > > +             return -ENOMEM;
> > > > > > > +
> > > > > > > +     tmdev->dev = dev;
> > > > > > > +     tmdev->chip = of_device_get_match_data(&pdev->dev);
> > > > > > > +     if (!tmdev->chip)
> > > > > > > +             return -EINVAL;
> > > > > > > +
> > > > > > > +     ret = tsens_init(tmdev);
> > > > > > > +     if (ret)
> > > > > > > +             return ret;
> > > > > > > +
> > > > > > > +     ret = tsens_register(tmdev);
> > > > > > > +     if (ret)
> > > > > > > +             return ret;
> > > > > > > +
> > > > > > > +     ret = tmdev->chip->enable(tmdev);
> > > > > > > +     if (ret)
> > > > > > > +             return ret;
> > > > > > >
> > > > > > > +     platform_set_drvdata(pdev, tmdev);
> > > > > >
> > > > > > Your registration should be the very last thing you do. Otherwise, you
> > > > > > have a small window where the get_temp callback can be called, but the
> > > > > > driver will not be functional yet.
> > > > >
> > > > > No. Anyway, ths data qcquisition is ms level.
> > > >
> > > > That's kind of irrelevant. There's nothing preventing get_temp to be
> > > > called right away.
> > >
> > > As Ondřej said,
> > >
> > > Registration after enabling will lead to call tz update on non-registered tz
> > > from an interrupt handler.
> >
> > I'm probably missing something but you're not using the interrupts, so
> > how could an interrupt handler call it?
> >
> > Also, other drivers seem to be doing that just fine (mtk_thermal for
> > example), so surely there's a way?
>
> Last version is using the interrupts.
>
> Drivers do it in various ways. For example imx_thermal (and others like
> hisi_thermal) does it the suggested way. It enables interrupts after thermal
> zone registration, so that IRQ handler doesn't get invoked before the tzd is
> registered.

Enabling the interrupts after the registration makes sense, yes, but
filling the device private pointer with the private structure,
enabling the clocks, setting up the controller and so on can be done
before.

Maxime

--
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)