| Message ID | 20200905133230.1014581-1-j.neuschaefer@gmx.net |
|---|---|
| Headers | show |
| Series | Netronix embedded controller driver for Kobo and Tolino ebook readers | expand |
On Sat, 05 Sep 2020, 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> > > --- > > > > v2: > > - Various grammar and style improvements, as suggested by Uwe Kleine-König, > > Lee Jones, and Alexandre Belloni > > - Switch to regmap > > - Prefix registers with NTXEC_REG_ > > - Add help text to the Kconfig option > > - Use the .apply callback instead of the old API > > - Add a #define for the time base (125ns) > > - Don't change device state in .probe; this avoids multiple problems > > - Rework division and overflow check logic to perform divisions in 32 bits > > - Avoid setting duty cycle to zero, to work around a hardware quirk > > --- > > drivers/pwm/Kconfig | 8 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-ntxec.c | 160 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 169 insertions(+) > > create mode 100644 drivers/pwm/pwm-ntxec.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index 7dbcf6973d335..7fd17c6cda95e 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -350,6 +350,14 @@ config PWM_MXS > > To compile this driver as a module, choose M here: the module > > will be called pwm-mxs. > > > > +config PWM_NTXEC > > + tristate "Netronix embedded controller PWM support" > > > > > > + depends on MFD_NTXEC && OF > > > I don’t see need to reduce test coverage and use of the driver by sticking > with OF. Actually it’s not used. If the device is only known to boot with OF, then it's pointless building it when !OF. If you want to increase test coverage enable COMPILE_TEST instead.
On Tue, Sep 8, 2020 at 11:14 AM Lee Jones <lee.jones@linaro.org> wrote: > On Sat, 05 Sep 2020, Andy Shevchenko wrote: > > On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > wrote: ... > > > +config PWM_NTXEC > > > + tristate "Netronix embedded controller PWM support" > > > > > > > > > > > + depends on MFD_NTXEC && OF > > > > > > I don’t see need to reduce test coverage and use of the driver by sticking > > with OF. Actually it’s not used. > > If the device is only known to boot with OF, then it's pointless > building it when !OF. No, it's not. As I pointed out the (compilation) test coverage is better. > If you want to increase test coverage enable > COMPILE_TEST instead. It is one way to achieve that, yes; depends on OF || COMPILE_TEST depends on MFD_NTXEC
On Tue, 08 Sep 2020, Andy Shevchenko wrote: > On Tue, Sep 8, 2020 at 11:14 AM Lee Jones <lee.jones@linaro.org> wrote: > > On Sat, 05 Sep 2020, Andy Shevchenko wrote: > > > On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > > wrote: > > ... > > > > > +config PWM_NTXEC > > > > + tristate "Netronix embedded controller PWM support" > > > > > > > > > > > > > > > > + depends on MFD_NTXEC && OF > > > > > > > > > I don’t see need to reduce test coverage and use of the driver by sticking > > > with OF. Actually it’s not used. > > > > If the device is only known to boot with OF, then it's pointless > > building it when !OF. > > No, it's not. As I pointed out the (compilation) test coverage is better. No, it's a waste of disk space. Why would you knowingly compile something you know you can't use? That's the whole point of COMPILE_TEST. Note that when you want real coverage and you use `allyesconfig` and/or `allmodconfig` then CONFIG_OF is also enabled on platforms which support it. > > If you want to increase test coverage enable > > COMPILE_TEST instead. > > It is one way to achieve that, yes; > > depends on OF || COMPILE_TEST > depends on MFD_NTXEC This is better.
On 05/09/2020 15:32:27+0200, Jonathan Neuschäfer wrote: > Some RTCs store the year as an 8-bit number relative to the year 2000. > This results in a maximum timestamp of 2255-12-31 23:59:59. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > --- > > v2: > - New patch > --- > include/linux/rtc.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/linux/rtc.h b/include/linux/rtc.h > index 22d1575e4991b..fcc086084a603 100644 > --- a/include/linux/rtc.h > +++ b/include/linux/rtc.h > @@ -154,6 +154,7 @@ struct rtc_device { > #define RTC_TIMESTAMP_END_2079 3471292799LL /* 2079-12-31 23:59:59 */ > #define RTC_TIMESTAMP_END_2099 4102444799LL /* 2099-12-31 23:59:59 */ > #define RTC_TIMESTAMP_END_2199 7258118399LL /* 2199-12-31 23:59:59 */ > +#define RTC_TIMESTAMP_END_2255 9025257599LL /* 2255-12-31 23:59:59 */ Honestly, I wouldn't bother adding that one unless you have examples of other RTCs endng at the same date, I'm fine having the value and comment directly in the probe function. > #define RTC_TIMESTAMP_END_9999 253402300799LL /* 9999-12-31 23:59:59 */ > > extern struct rtc_device *devm_rtc_device_register(struct device *dev, > -- > 2.28.0 >
On Sat, 05 Sep 2020, Jonathan Neuschäfer wrote: > The Netronix embedded controller is a microcontroller found in some > e-book readers designed by the ODM Netronix, Inc. It contains RTC, > battery monitoring, system power management, and PWM functionality. > > This driver implements register access and version detection. > > Third-party hardware documentation is available at: > > https://github.com/neuschaefer/linux/wiki/Netronix-MSP430-embedded-controller > > The EC supports interrupts, but the driver doesn't make use of them so > far. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > --- > > v2: > - Add a description of the device to the patch text > - Unify spelling as 'Netronix embedded controller'. > 'Netronix' is the proper name of the manufacturer, but 'embedded controller' > is just a label that I have assigned to the device. > - Switch to regmap, avoid regmap use in poweroff and reboot handlers. > Inspired by cf84dc0bb40f4 ("mfd: rn5t618: Make restart handler atomic safe") > - Use a list of known-working firmware versions instead of checking for a > known-incompatible version > - Prefix registers with NTXEC_REG_ > - Define register values as constants > - Various style cleanups as suggested by Lee Jones > - Don't align = signs in struct initializers [Uwe Kleine-König] > - Don't use dev_dbg for an error message > - Explain sleep in poweroff handler > - Remove (struct ntxec).client > - Switch to .probe_new in i2c driver > - Add .remove callback > - Make CONFIG_MFD_NTXEC a tristate option > --- > drivers/mfd/Kconfig | 7 ++ > drivers/mfd/Makefile | 1 + > drivers/mfd/ntxec.c | 217 ++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/ntxec.h | 24 +++++ > 4 files changed, 249 insertions(+) > create mode 100644 drivers/mfd/ntxec.c > create mode 100644 include/linux/mfd/ntxec.h > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index 33df0837ab415..bab999081f32d 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -978,6 +978,13 @@ config MFD_VIPERBOARD > You need to select the mfd cell drivers separately. > The drivers do not support all features the board exposes. > > +config MFD_NTXEC > + tristate "Netronix embedded controller" Nit: "Embedded Controller (EC)" > + depends on I2C && OF depends on (I2C && OF) || COMPILE_TEST > + help > + Say yes here if you want to support the embedded controller found in > + certain e-book readers designed by the ODM Netronix. > + > config MFD_RETU > tristate "Nokia Retu and Tahvo multi-function device" > select MFD_CORE > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index a60e5f835283e..236a8acd917a0 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -217,6 +217,7 @@ obj-$(CONFIG_MFD_INTEL_MSIC) += intel_msic.o > obj-$(CONFIG_MFD_INTEL_PMC_BXT) += intel_pmc_bxt.o > obj-$(CONFIG_MFD_PALMAS) += palmas.o > obj-$(CONFIG_MFD_VIPERBOARD) += viperboard.o > +obj-$(CONFIG_MFD_NTXEC) += ntxec.o > obj-$(CONFIG_MFD_RC5T583) += rc5t583.o rc5t583-irq.o > obj-$(CONFIG_MFD_RK808) += rk808.o > obj-$(CONFIG_MFD_RN5T618) += rn5t618.o > diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c > new file mode 100644 > index 0000000000000..49cc4fa35b9fe > --- /dev/null > +++ b/drivers/mfd/ntxec.c > @@ -0,0 +1,217 @@ > +// SPDX-License-Identifier: GPL-2.0-only Why 2 only? > +/* > + * The Netronix embedded controller is a microcontroller found in some > + * e-book readers designed by the ODM Netronix, Inc. It contains RTC, > + * battery monitoring, system power management, and PWM functionality. > + * > + * This driver implements register access, version detection, and system > + * power-off/reset. > + * > + * Copyright 2020 Jonathan Neuschäfer Email? > + */ > + > +#include <asm/unaligned.h> > +#include <linux/delay.h> > +#include <linux/errno.h> > +#include <linux/i2c.h> > +#include <linux/mfd/ntxec.h> > +#include <linux/module.h> > +#include <linux/of_platform.h> > +#include <linux/pm.h> > +#include <linux/reboot.h> > +#include <linux/regmap.h> > +#include <linux/types.h> > + > +#define NTXEC_REG_VERSION 0x00 > +#define NTXEC_REG_POWEROFF 0x50 > +#define NTXEC_REG_POWERKEEP 0x70 > +#define NTXEC_REG_RESET 0x90 > + > +#define NTXEC_POWEROFF_VALUE 0x0100 > +#define NTXEC_POWERKEEP_VALUE 0x0800 > +#define NTXEC_RESET_VALUE 0xff00 > + > +static struct i2c_client *poweroff_restart_client; > + > +static void ntxec_poweroff(void) > +{ > + int res; > + Remove this line please. > + u8 buf[] = { > + NTXEC_REG_POWEROFF, > + (NTXEC_POWEROFF_VALUE >> 8) & 0xff, > + NTXEC_POWEROFF_VALUE & 0xff, > + }; > + And this one. > + struct i2c_msg msgs[] = { > + { > + .addr = poweroff_restart_client->addr, > + .flags = 0, > + .len = sizeof(buf), > + .buf = buf > + } > + }; > + > + res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs)); > + And this one. > + if (res < 0) > + dev_alert(&poweroff_restart_client->dev, "Failed to power off (err = %d)\n", res); This looks way over 80 chars. Did you run checkpatch.pl? > + /* > + * The time from the register write until the host CPU is powered off > + * has been observed to be about 2.5 to 3 seconds. Sleep long enough to > + * safely avoid returning from the poweroff handler. > + */ > + msleep(5000); > +} > + > +static int ntxec_restart(struct notifier_block *nb, > + unsigned long action, void *data) > +{ > + int res; > + > + /* > + * NOTE: The lower half of the reset value is not sent, because sending > + * it causes an error > + */ > + u8 buf[] = { > + NTXEC_REG_RESET, > + (NTXEC_RESET_VALUE >> 8) & 0xff, > + }; > + > + struct i2c_msg msgs[] = { > + { > + .addr = poweroff_restart_client->addr, > + .flags = 0, > + .len = sizeof(buf), > + .buf = buf > + } > + }; > + > + res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs)); > + > + if (res < 0) > + dev_alert(&poweroff_restart_client->dev, "Failed to restart (err = %d)\n", res); > + > + return NOTIFY_DONE; > +} All as above for this function. > +static struct notifier_block ntxec_restart_handler = { > + .notifier_call = ntxec_restart, > + .priority = 128 > +}; > + > +static const struct regmap_config regmap_config = { > + .name = "ntxec", > + .reg_bits = 8, > + .val_bits = 16, > + .cache_type = REGCACHE_NONE, > + .val_format_endian = REGMAP_ENDIAN_BIG, > +}; > + > +static const struct ntxec_info ntxec_known_versions[] = { > + { 0xd726 }, /* found in Kobo Aura */ > +}; > + > +static const struct ntxec_info *ntxec_lookup_info(u16 version) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(ntxec_known_versions); i++) { > + const struct ntxec_info *info = &ntxec_known_versions[i]; > + > + if (info->version == version) > + return info; > + } > + > + return NULL; > +} Wait, what? This is over-engineered. Just have a look-up table (switch) of supported devices. > +static int ntxec_probe(struct i2c_client *client) > +{ > + struct ntxec *ec; > + unsigned int version; > + int res; > + > + ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL); > + if (!ec) > + return -ENOMEM; > + > + ec->dev = &client->dev; > + > + ec->regmap = devm_regmap_init_i2c(client, ®map_config); > + if (IS_ERR(ec->regmap)) { > + dev_err(ec->dev, "Failed to set up regmap for device\n"); > + return res; > + } > + > + /* Determine the firmware version */ > + res = regmap_read(ec->regmap, NTXEC_REG_VERSION, &version); > + if (res < 0) { > + dev_err(ec->dev, "Failed to read firmware version number\n"); > + return res; > + } > + dev_info(ec->dev, > + "Netronix embedded controller version %04x detected.\n", > + version); > + > + /* Bail out if we encounter an unknown firmware version */ > + ec->info = ntxec_lookup_info(version); > + if (!ec->info) > + return -EOPNOTSUPP; #define EOPNOTSUPP 95 /* Operation not supported on transport endpoint */ I think you want: #define ENODEV 19 /* No such device */ > + if (of_device_is_system_power_controller(ec->dev->of_node)) { > + /* > + * Set the 'powerkeep' bit. This is necessary on some boards > + * in order to keep the system running. > + */ > + res = regmap_write(ec->regmap, NTXEC_REG_POWERKEEP, > + NTXEC_POWERKEEP_VALUE); > + if (res < 0) > + return res; > + > + /* Install poweroff handler */ Don't think this comment is required. > + WARN_ON(poweroff_restart_client); > + poweroff_restart_client = client; > + if (pm_power_off) > + dev_err(ec->dev, "pm_power_off already assigned\n"); > + else > + pm_power_off = ntxec_poweroff; > + > + /* Install board reset handler */ Nor this. > + res = register_restart_handler(&ntxec_restart_handler); > + if (res) > + dev_err(ec->dev, > + "Failed to register restart handler: %d\n", res); > + } > + > + i2c_set_clientdata(client, ec); > + > + return devm_of_platform_populate(ec->dev); > +} > + > +static int ntxec_remove(struct i2c_client *i2c) Why do you call it 'client' in .probe, but 'i2c' in .remove? > +{ > + if (i2c == poweroff_restart_client) { > + poweroff_restart_client = NULL; > + pm_power_off = NULL; > + unregister_restart_handler(&ntxec_restart_handler); > + } > + > + return 0; > +} > + > +static const struct of_device_id of_ntxec_match_table[] = { > + { .compatible = "netronix,ntxec", }, > + {} > +}; > + > +static struct i2c_driver ntxec_driver = { > + .driver = { > + .name = "ntxec", > + .of_match_table = of_ntxec_match_table, > + }, > + .probe_new = ntxec_probe, > + .remove = ntxec_remove, > +}; > +module_i2c_driver(ntxec_driver); > diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h > new file mode 100644 > index 0000000000000..65e389af79da6 > --- /dev/null > +++ b/include/linux/mfd/ntxec.h > @@ -0,0 +1,24 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* > + * Copyright 2020 Jonathan Neuschäfer > + * > + * Register access and version information for the Netronix embedded > + * controller. > + */ > + > +#ifndef NTXEC_H > +#define NTXEC_H > + > +#include <linux/types.h> > + > +struct ntxec_info { > + u16 version; > +}; > + > +struct ntxec { > + struct device *dev; > + struct regmap *regmap; > + const struct ntxec_info *info; What is this used for? > +}; > + > +#endif
On Tue, Sep 08, 2020 at 02:29:34PM +0100, Lee Jones wrote: > On Sat, 05 Sep 2020, Jonathan Neuschäfer wrote: [...] > > +config MFD_NTXEC > > + tristate "Netronix embedded controller" > > Nit: "Embedded Controller (EC)" I intentionally left it lowercase, because the term 'embedded controller' is not used in code coming from Netronix. It's just a label that I found fitting to assign to the device. (In the vendor kernels it's either called 'msp430', named after the TI MSP430 microcontroller family, or 'uc', presumably for 'microcontroller') Adding '(EC)' seems somewhat useful. > > > + depends on I2C && OF > > depends on (I2C && OF) || COMPILE_TEST Okay > > +++ b/drivers/mfd/ntxec.c > > @@ -0,0 +1,217 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > Why 2 only? No particular reason. If you prefer 2-or-later, I'll change it. > > +/* > > + * The Netronix embedded controller is a microcontroller found in some > > + * e-book readers designed by the ODM Netronix, Inc. It contains RTC, > > + * battery monitoring, system power management, and PWM functionality. > > + * > > + * This driver implements register access, version detection, and system > > + * power-off/reset. > > + * > > + * Copyright 2020 Jonathan Neuschäfer > > Email? Alright, I'll add it > > +static void ntxec_poweroff(void) > > +{ > > + int res; > > + > > Remove this line please. > > > + u8 buf[] = { > > + NTXEC_REG_POWEROFF, > > + (NTXEC_POWEROFF_VALUE >> 8) & 0xff, > > + NTXEC_POWEROFF_VALUE & 0xff, > > + }; > > + > > And this one. > > > + struct i2c_msg msgs[] = { > > + { > > + .addr = poweroff_restart_client->addr, > > + .flags = 0, > > + .len = sizeof(buf), > > + .buf = buf > > + } > > + }; > > + > > + res = i2c_transfer(poweroff_restart_client->adapter, msgs, ARRAY_SIZE(msgs)); > > + > > And this one. Okay > > > + if (res < 0) > > + dev_alert(&poweroff_restart_client->dev, "Failed to power off (err = %d)\n", res); > > This looks way over 80 chars. Okay, I'll break it up. > Did you run checkpatch.pl? Yes, but it didn't complain about this line. - propabably because the line length threshold in checkpatch has recently been increased to 100. > > + /* > > + * The time from the register write until the host CPU is powered off > > + * has been observed to be about 2.5 to 3 seconds. Sleep long enough to > > + * safely avoid returning from the poweroff handler. > > + */ > > + msleep(5000); > > +} > > + > > +static int ntxec_restart(struct notifier_block *nb, > > + unsigned long action, void *data) [...] > > All as above for this function. Alright > > > +static struct notifier_block ntxec_restart_handler = { > > + .notifier_call = ntxec_restart, > > + .priority = 128 > > +}; > > + > > +static const struct regmap_config regmap_config = { > > + .name = "ntxec", > > + .reg_bits = 8, > > + .val_bits = 16, > > + .cache_type = REGCACHE_NONE, > > + .val_format_endian = REGMAP_ENDIAN_BIG, > > +}; > > + > > +static const struct ntxec_info ntxec_known_versions[] = { > > + { 0xd726 }, /* found in Kobo Aura */ > > +}; > > + > > +static const struct ntxec_info *ntxec_lookup_info(u16 version) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(ntxec_known_versions); i++) { > > + const struct ntxec_info *info = &ntxec_known_versions[i]; > > + > > + if (info->version == version) > > + return info; > > + } > > + > > + return NULL; > > +} > > Wait, what? This is over-engineered. I thought it would be useful when we want to attach additional information to specific versions.... okay, it is over-engineered. > Just have a look-up table (switch) of supported devices. Will do. > > > +static int ntxec_probe(struct i2c_client *client) > > +{ > > + struct ntxec *ec; > > + unsigned int version; > > + int res; > > + > > + ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL); > > + if (!ec) > > + return -ENOMEM; > > + > > + ec->dev = &client->dev; > > + > > + ec->regmap = devm_regmap_init_i2c(client, ®map_config); > > + if (IS_ERR(ec->regmap)) { > > + dev_err(ec->dev, "Failed to set up regmap for device\n"); > > + return res; > > + } > > + > > + /* Determine the firmware version */ > > + res = regmap_read(ec->regmap, NTXEC_REG_VERSION, &version); > > + if (res < 0) { > > + dev_err(ec->dev, "Failed to read firmware version number\n"); > > + return res; > > + } > > + dev_info(ec->dev, > > + "Netronix embedded controller version %04x detected.\n", > > + version); > > + > > + /* Bail out if we encounter an unknown firmware version */ > > + ec->info = ntxec_lookup_info(version); > > + if (!ec->info) > > + return -EOPNOTSUPP; > > #define EOPNOTSUPP 95 /* Operation not supported on transport endpoint */ > > I think you want: > > #define ENODEV 19 /* No such device */ Indeed, EOPNOTSUPP seems quite wrong here. I think I used ENOTSUPP at some earlier point but moved away from it because it's one of the internal error codes (≥512). ENODEV makes sense when I read it as "Not a device that this driver can deal with". > > > + if (of_device_is_system_power_controller(ec->dev->of_node)) { > > + /* > > + * Set the 'powerkeep' bit. This is necessary on some boards > > + * in order to keep the system running. > > + */ > > + res = regmap_write(ec->regmap, NTXEC_REG_POWERKEEP, > > + NTXEC_POWERKEEP_VALUE); > > + if (res < 0) > > + return res; > > + > > + /* Install poweroff handler */ > > Don't think this comment is required. > > > + WARN_ON(poweroff_restart_client); > > + poweroff_restart_client = client; > > + if (pm_power_off) > > + dev_err(ec->dev, "pm_power_off already assigned\n"); > > + else > > + pm_power_off = ntxec_poweroff; > > + > > + /* Install board reset handler */ > > Nor this. Alright, I'll remove them. > > + res = register_restart_handler(&ntxec_restart_handler); > > + if (res) > > + dev_err(ec->dev, > > + "Failed to register restart handler: %d\n", res); > > + } > > + > > + i2c_set_clientdata(client, ec); > > + > > + return devm_of_platform_populate(ec->dev); > > +} > > + > > +static int ntxec_remove(struct i2c_client *i2c) > > Why do you call it 'client' in .probe, but 'i2c' in .remove? No particular reason. I'll make them consistent. > > +struct ntxec_info { > > + u16 version; > > +}; > > + > > +struct ntxec { > > + struct device *dev; > > + struct regmap *regmap; > > > + const struct ntxec_info *info; > > What is this used for? At this point, nothing. I'll remove it. Thanks, Jonathan Neuschäfer
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
On Tue, Sep 08, 2020 at 10:35:38AM +0100, Lee Jones wrote: > On Tue, 08 Sep 2020, Andy Shevchenko wrote: > > > On Tue, Sep 8, 2020 at 11:14 AM Lee Jones <lee.jones@linaro.org> wrote: > > > On Sat, 05 Sep 2020, Andy Shevchenko wrote: > > > > On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net> [...] > > > > > + depends on MFD_NTXEC && OF > > > > > > > > > > > > I don’t see need to reduce test coverage and use of the driver by sticking > > > > with OF. Actually it’s not used. > > > > > > If the device is only known to boot with OF, then it's pointless > > > building it when !OF. > > > > No, it's not. As I pointed out the (compilation) test coverage is better. > > No, it's a waste of disk space. > > Why would you knowingly compile something you know you can't use? > > That's the whole point of COMPILE_TEST. > > Note that when you want real coverage and you use `allyesconfig` > and/or `allmodconfig` then CONFIG_OF is also enabled on platforms > which support it. > > > > If you want to increase test coverage enable > > > COMPILE_TEST instead. > > > > It is one way to achieve that, yes; > > > > depends on OF || COMPILE_TEST > > depends on MFD_NTXEC > > This is better. Ok, I'll go with this variant. Thanks
On Tue, Sep 08, 2020 at 11:39:46AM +0200, Alexandre Belloni wrote: > On 05/09/2020 15:32:27+0200, Jonathan Neuschäfer wrote: [...] > > #define RTC_TIMESTAMP_END_2079 3471292799LL /* 2079-12-31 23:59:59 */ > > #define RTC_TIMESTAMP_END_2099 4102444799LL /* 2099-12-31 23:59:59 */ > > #define RTC_TIMESTAMP_END_2199 7258118399LL /* 2199-12-31 23:59:59 */ > > +#define RTC_TIMESTAMP_END_2255 9025257599LL /* 2255-12-31 23:59:59 */ > > Honestly, I wouldn't bother adding that one unless you have examples of > other RTCs endng at the same date, I'm fine having the value and comment > directly in the probe function. Indeed, it's the only RTC I know with this end date. I'll fold it into the driver. Thanks, Jonathan Neuschäfer
On Sat, Sep 05, 2020 at 08:56:35PM +0300, Andy Shevchenko wrote: > On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net> > wrote: [...] > > +#include <linux/of_device.h> > > > No user for this. Perhaps you meant mod_devicetable.h? Yes > > +/* Convert an 8-bit value into the correct format for writing into a > > register */ > > +#define u8_to_reg(x) (((x) & 0xff) << 8) > > > This needs more explanation. Does register be16? Yes, the registers are treated as be16 in the base driver. I wrote a slightly longer explanation in response to your other review. > > +static int ntxec_set_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct ntxec_rtc *rtc = dev_get_drvdata(dev); > > + int res = 0; > > + > > + res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_YEAR, > > u8_to_reg(tm->tm_year - 100)); > > + res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MONTH, > > u8_to_reg(tm->tm_mon + 1)); > > + res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_DAY, > > u8_to_reg(tm->tm_mday)); > > + res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_HOUR, > > u8_to_reg(tm->tm_hour)); > > + res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_MINUTE, > > u8_to_reg(tm->tm_min)); > > + res |= regmap_write(rtc->ec->regmap, NTXEC_REG_WRITE_SECOND, > > u8_to_reg(tm->tm_sec)); > > + > > + return (res < 0) ? -EIO : 0; > > > Hmm... (I stumbled over res |= parts) I'll convert it to the (more verbose but also more correct) pattern of returning each error early. > > +static const struct of_device_id ntxec_rtc_of_match[] = { > > + { .compatible = "netronix,ntxec-rtc" }, > > + { }, > > > No need for comma in terminator line. Okay Thanks, Jonathan Neuschäfer