Messages in this thread | | | Date | Sat, 6 Mar 2021 20:14:46 +0100 | From | Jonathan Neuschäfer <> | Subject | Re: [PATCH -next] mfd: ntxec: Support for EC in Tolino Shine 2 HD |
| |
Hi,
(Cc'ing Mark Brown because of the regmap related questions)
On Sat, Mar 06, 2021 at 07:13:14PM +0100, Andreas Kemnade wrote: > Add the version of the EC in the Tolino Shine 2 HD > to the supported versions. It seems not to have an RTC > and does not ack data written to it. > The vendor kernel happily ignores write errors, using > I2C via userspace i2c-set also shows the error. > So add a quirk to ignore that error. > > PWM can be successfully configured despite of that error.
I'm curious, is this one of the variants with two PWM channels (for configurable color temperature)?
> > Signed-off-by: Andreas Kemnade <andreas@kemnade.info> > --- > drivers/mfd/ntxec.c | 57 ++++++++++++++++++++++++++++++++++++--- > include/linux/mfd/ntxec.h | 1 + > 2 files changed, 55 insertions(+), 3 deletions(-) > > diff --git a/drivers/mfd/ntxec.c b/drivers/mfd/ntxec.c > index 957de2b03529..e7fe570127af 100644 > --- a/drivers/mfd/ntxec.c > +++ b/drivers/mfd/ntxec.c > @@ -96,6 +96,36 @@ static struct notifier_block ntxec_restart_handler = { > .priority = 128, > }; > > +static int regmap_ignore_write(void *context, > + unsigned int reg, unsigned int val) > + > +{ > + struct regmap *regmap = context; > + > + regmap_write(regmap, reg, val); > + > + return 0; > +} > + > +static int regmap_wrap_read(void *context, unsigned int reg, > + unsigned int *val) > +{ > + struct regmap *regmap = context; > + > + return regmap_read(regmap, reg, val); > +} > + > +/* some firmware versions do not ack written data, add a wrapper */ > +static const struct regmap_config regmap_config_noack = { > + .name = "ntxec_noack", > + .reg_bits = 8, > + .val_bits = 16, > + .cache_type = REGCACHE_NONE, > + .val_format_endian = REGMAP_ENDIAN_BIG, > + .reg_write = regmap_ignore_write, > + .reg_read = regmap_wrap_read
Is the read wrapper necessary? It seems to me from reading regmap.h that leaving .reg_read set to NULL should do the right thing, but I'm not sure.
> +}; > + > static const struct regmap_config regmap_config = { > .name = "ntxec", > .reg_bits = 8, > @@ -109,10 +139,15 @@ static const struct mfd_cell ntxec_subdevices[] = { > { .name = "ntxec-pwm" }, > }; > > +static const struct mfd_cell ntxec_subdev_pwm[] = { > + { .name = "ntxec-pwm" }, > +};
ntxec_subdevices vs. ntxec_subdev_pwm seems slightly inconsistent in naming. ntxec_subdevices_pwm would be a wrong plural, but IMHO slightly better because of consistency. Maybe rename ntxec_subdevices to ntxec_subdev?
> + > static int ntxec_probe(struct i2c_client *client) > { > struct ntxec *ec; > unsigned int version; > + bool has_rtc; > int res; > > ec = devm_kmalloc(&client->dev, sizeof(*ec), GFP_KERNEL); > @@ -137,6 +172,15 @@ static int ntxec_probe(struct i2c_client *client) > /* Bail out if we encounter an unknown firmware version */ > switch (version) { > case NTXEC_VERSION_KOBO_AURA: > + has_rtc = true; > + break; > + case NTXEC_VERSION_TOLINO_SHINE2: > + has_rtc = false; > + ec->regmap = devm_regmap_init(ec->dev, NULL, > + ec->regmap, > + ®map_config_noack);
Ah— A custom regmap stacked on top of the old regmap… I think this deserves a comment.
> + if (IS_ERR(ec->regmap)) > + return PTR_ERR(ec->regmap); > break; > default: > dev_err(ec->dev, > @@ -155,7 +199,6 @@ static int ntxec_probe(struct i2c_client *client) > */ > res = regmap_write(ec->regmap, NTXEC_REG_POWERKEEP, > NTXEC_POWERKEEP_VALUE); > - if (res < 0) > return res;
This deletion looks like a mistake.
> > if (poweroff_restart_client) > @@ -181,8 +224,16 @@ static int ntxec_probe(struct i2c_client *client) > > i2c_set_clientdata(client, ec); > > - res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE, ntxec_subdevices, > - ARRAY_SIZE(ntxec_subdevices), NULL, 0, NULL); > + if (has_rtc) > + res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE, > + ntxec_subdevices, > + ARRAY_SIZE(ntxec_subdevices), > + NULL, 0, NULL); > + else > + res = devm_mfd_add_devices(ec->dev, PLATFORM_DEVID_NONE, > + ntxec_subdev_pwm, > + ARRAY_SIZE(ntxec_subdev_pwm), > + NULL, 0, NULL);
At some point, it will probably be simpler to have
struct mfd_cell *subdev = ntxec_subdevices; size_t subdev_size = ARRAY_SIZE(ntxec_subdevices);
on top of the probe function and override them in the switch statement, but at this point I think it doesn't matter, and either way is fine.
> if (res) > dev_err(ec->dev, "Failed to add subdevices: %d\n", res); > > diff --git a/include/linux/mfd/ntxec.h b/include/linux/mfd/ntxec.h > index 361204d125f1..26ab3b8eb612 100644 > --- a/include/linux/mfd/ntxec.h > +++ b/include/linux/mfd/ntxec.h > @@ -33,5 +33,6 @@ static inline __be16 ntxec_reg8(u8 value) > > /* Known firmware versions */ > #define NTXEC_VERSION_KOBO_AURA 0xd726 /* found in Kobo Aura */ > +#define NTXEC_VERSION_TOLINO_SHINE2 0xf110 /* found in Tolino Shine 2 HD */ > > #endif > -- > 2.29.2 >
Thanks for your patch, Jonathan [unhandled content-type:application/pgp-signature] | |