Messages in this thread | | | Date | Sat, 6 Mar 2021 21:19:38 +0100 | From | Jonathan Neuschäfer <> | Subject | Re: [PATCH -next] mfd: ntxec: Support for EC in Tolino Shine 2 HD |
| |
On Sat, Mar 06, 2021 at 08:42:19PM +0100, Andreas Kemnade wrote: > On Sat, 6 Mar 2021 20:14:46 +0100 > Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: [...] > > > +/* 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.
Ok, with the understanding that there will be no read functionality without the .reg_read callback, I'm fine leaving it in, even when the particular drivers in existence don't use it.
> 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.
Dropping REGMAP_ENDIAN_BIG sounds reasonable, because this regmap doesn't go to a byte-wise bus.
> > > + 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.
Ok, please add a short comment here or extend the one above the repmap config struct, to save interested readers a bit of trouble. I guess "add a wrapper" goes in right direction, but it didn't make the stacking obvious to me when I first read it.
Thanks, Jonathan [unhandled content-type:application/pgp-signature] | |