Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU

["Marek Behún" <kabel@kernel.org>]

Hello Andy,

On Tue, 19 Sep 2023 21:27:04 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Tue, Sep 19, 2023 at 05:16:38PM +0200, Marek Behún wrote:
> > On Tue, 19 Sep 2023 15:29:08 +0300
> > Andy Shevchenko <andy@kernel.org> wrote:  
> > > On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:  
> 
> ...
> 
> > > >  obj-$(CONFIG_GOLDFISH)		+= goldfish/
> > > >  obj-$(CONFIG_CHROME_PLATFORMS)	+= chrome/
> > > >  obj-$(CONFIG_SURFACE_PLATFORMS)	+= surface/
> > > > +obj-$(CONFIG_CZNIC_PLATFORMS)	+= cznic/    
> > > 
> > > Why not ordered (to some extent) here (as you did in the other file)?  
> > 
> > Where should I put it? The other entries are not ordered, see
> >   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/Makefile?h=v6.6-rc2  
> 
> With the visible context above, at least after chrome already much better, no?

OK

... 
> > > Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...  
> > 
> > i2c.h comes from turris-omnia-mcu.h. The others I will fix. Does the
> > the .c file also include all those headers if it includes a local
> > header already?  
> 
> For generic ones, yes.

OK

> > > > +	if (ret < 0)    
> > > 
> > > Can it return positive value? What would be the meaning of it?  
> > 
> > Originally I had it return 0 on success, but recently I've had a
> > discussion about this same thing with LED subsystem maintainer about
> > some LED patches, Lee Jones, who requested the _read function to return
> > number of bytes read on success. See at
> >   https://lore.kernel.org/linux-leds/20230821122644.GJ1380343@google.com/
> > where he says:
> >   Read and write APIs, even abstracted ones such as these generally
> >   return the number of bytes successfully read and written respectively.
> >   If you are going to normalise, then please do so against this
> >   standard.
> > I do not agree with him, since this is a local command accessor which
> > only succeeds if all the bytes are read/written as requested. But I
> > adjusted my code as per his request since he is the maintainer.  
> 
> This is strange. For example, regmap APIs never returns amount of data written
> or read. I think it's solely depends on the API. It might be useful for i²c
> APIs, in case you can do something about it. but if you have wrappers on top
> of that already (meaning not using directly the i2c_*() calls, I dunno the
> positive return is anyhow useful.

I shall change it back then.

...

> > > > +	int status;    
> > > 
> > > My gosh, it's a _third_ name for the same!  
> > 
> > The variable holds value of the status register, which is needed at
> > another point in this function. I thought naming it just "ret" would
> > cause confusions... Do you really propose to rename it such?  
> 
> It's not bad per se, just put yourself on the place of somebody who would like
> to understand the semantics of all these variables. Maybe you can add a comment
> on top explaining that (in short)?

OK

> > > > +	for (int i = 0; i < ARRAY_SIZE(features); ++i) {    
> > > 
> > > Why signed?  
> > 
> > Because it is shorter and makes no difference and there are tons of
> >   for (int i = 0; i < ARRAY_SIZE(x); i++)
> > in the kernel already. But if you really want, I can change it. Please
> > let me know.  
> 
> It just makes harder to get the code, because of possible unneeded questions
> like "if it's a signed type, maybe it's on purpose?"

OK

> > > Why pre-increment?  
> > 
> > Again, it makes no difference, is widely used in the kernel and I
> > personally prefer it. But I will change it if you want.  
> 
> Pre-increment usually also a sign of something special about the iterator
> variable. In non-special cases we use post-increment. And if you count
> the use of each you will see the difference.

OK

...

> > > > +	if (likely(ret == ARRAY_SIZE(msgs)))    
> > > 
> > > Why likely()? Please, justify.  
> > 
> > Becuase it is unlikely the I2C transaction will fail. In most cases, it
> > does not.  
> 
> Yes, but why likely() is needed? So, i.o.w. what's the benefit in _this_ case?

Compiler optimization (one branch avoided). But I guess this isn't a
hot path, since I2C is insanely slow anyway. OK, I shall remove the
likely() usage.

> ...
> 
> > > > +enum commands_e {    
> > > 
> > > Are you sure the name is unique enough / properly namespaced?
> > > Same Q to all enums.  
> > 
> > I can change it. I wanted it to be compatible with the header in the
> > firmware repository.  
> 
> Are these being generated, or also statically written in that repo?
> If the latter, I think better to use more unique naming schema in
> the kernel.

OK, I shall add some prefix.

Marek