Messages in this thread Patch in this message | | | Date | Sat, 6 Mar 2021 20:42:19 +0100 | From | Andreas Kemnade <> | Subject | Re: [PATCH -next] mfd: ntxec: Support for EC in Tolino Shine 2 HD |
| |
On Sat, 6 Mar 2021 20:14:46 +0100 Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> 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)? > No. Tolino Shine 3 and Kobo Clara HD have such things. There you have a /sys/class/backlight/backlight_{cold,warm}.
> > > > 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. > well if we want to read from it, there need to be some function for it. But if... I do not see anything worth to read besides of version. I think we can leave ouf val_format_endian because a lot of stuff is bypassed if no bus is set in regmap_init(). There is e.g. a goto skip_format_initialization.
> > +}; > > + > > 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? > yes, I will change it.
> > + > > 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. > Yes, devm_regmap_init_i2c() sets a different set of callbacks depending on circumstances. Seems to be the easiest way to avoid duplicating too much code. I really hope that there are not so much devices requiring such a dirty stuff that adding such thing to the generic regmap code would be justified.
> > + 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. > Oops, sorry.
> > > > 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. > Yes, that might be a good idea.
Regards, Andreas
| |