linux-kernel - Re: [PATCH -next] mfd: ntxec: Support for EC in Tolino Shine 2 HD

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YEPj2ss13w5p+R4C@latitude>
Date:   Sat, 6 Mar 2021 21:19:38 +0100
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     Andreas Kemnade <andreas@...nade.info>
Cc:     Jonathan Neuschäfer <j.neuschaefer@....net>,
        lee.jones@...aro.org, linux-kernel@...r.kernel.org,
        Mark Brown <broonie@...nel.org>
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@....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,
> > > +					      &regmap_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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)