Messages in this thread | | | Date | Thu, 10 Sep 2020 21:09:57 +0200 | From | Jonathan Neuschäfer <> | 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@gmx.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@gmx.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 [unhandled content-type:application/pgp-signature] | |