linux-kernel - Re: [PATCH v2 05/10] pwm: ntxec: Add driver for PWM function in Netronix EC

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <20200910190957.GB3306@latitude>
Date:   Thu, 10 Sep 2020 21:09:57 +0200
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Neuschäfer <j.neuschaefer@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Lee Jones <lee.jones@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Thierry Reding <thierry.reding@...il.com>,
        Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Sam Ravnborg <sam@...nborg.org>,
        Linus Walleij <linus.walleij@...aro.org>,
        Heiko Stuebner <heiko.stuebner@...obroma-systems.com>,
        Stephan Gerhold <stephan@...hold.net>,
        Lubomir Rintel <lkundrak@...sk>,
        Mark Brown <broonie@...nel.org>, allen <allen.chen@....com.tw>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-pwm@...r.kernel.org" <linux-pwm@...r.kernel.org>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Josua Mayer <josua.mayer@....eu>,
        Andreas Kemnade <andreas@...nade.info>,
        Arnd Bergmann <arnd@...db.de>, Daniel Palmer <daniel@...f.com>
Subject: Re: [PATCH v2 05/10] pwm: ntxec: Add driver for PWM function in
 Netronix EC

On Sat, Sep 05, 2020 at 09:08:49PM +0300, Andy Shevchenko wrote:
> On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@....net>
> wrote:
> 
> > The Netronix EC provides a PWM output which is used for the backlight
> > on some ebook readers. This patches adds a driver for the PWM output.
> >
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@....net>
> > ---
[...]
> > +#include <linux/of_device.h>
> 
> 
> mod_devicetable.h

Okay

> > +/* Convert an 8-bit value into the correct format for writing into a
> > register */
> > +#define u8_to_reg(x) (((x) & 0xff) << 8)
> 
> 
> You spread this macro among the drivers w/o explanation what’s going on. I
> think there will be better approach.

Okay, I'll move it to ntxec.h and expand on the explanation. I think
what's missing is the following part:

  Some registers, such as the battery status register (0x41), are in
  big-endian, but others only have eight significant bits, which are in
  the first byte transmitted over I2C (the MSB of the big-endian value).
  This convenience macro/function converts an 8-bit value to 16-bit for
  use in the second kind of register.

> > +
> > +       res |= regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_HIGH,
> > u8_to_reg(period >> 8));
> > +       res |= regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_LOW,
> > u8_to_reg(period));
> > +       res |= regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_HIGH,
> > u8_to_reg(duty >> 8));
> > +       res |= regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_LOW,
> > u8_to_reg(duty));
> 
> 
> These funny res |= is unusual pattern for returned error codes. Moreover
> you are shadowing the real ones. Same go the rest drovers. Please fix by
> checking each separately.

Okay

> > +       platform_set_drvdata(pdev, pwm);
> > +
> > +       return (res < 0) ? -EIO : 0;
> 
> 
> What?!

That's an editing error, sorry.

> > +static const struct of_device_id ntxec_pwm_of_match[] = {
> > +       { .compatible = "netronix,ntxec-pwm" },
> >
> >
> 
> > +       { },
> 
> 
> No comma.

Okay


Thanks for the review,
Jonathan Neuschäfer

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