Re: [PATCH v9 1/2] mfd: bd71837: mfd driver for ROHM BD71837 PMIC

quoted

quoted

quoted

quoted

+++ b/include/linux/mfd/bd71837.h

@@ -0,0 +1,391 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */

I thought these were meant to use C++ (//) comments?

The checkpatch.pl did complain if I used // comment on SPDX line for
header file. OTOH for c-file it required // comments and complained
about /* */ - version. So I did as it suggested and used 

Well that's just dandy -- who comes up with this stuff?

Engineers I bet =)

Ones who do not suffer from OCD, clearly!

quoted

quoted

quoted

quoted

+/* Copyright (C) 2018 ROHM Semiconductors */
+
+/*
+ * ROHM BD71837MWV header file
+ */

If you prefix the mentions of (BD,bd)71837 with (ROHM,rohm) then this
comment becomes redundant and you can remove it.

I am sorry but I am not sure what you suggest here. Do you mean I should
add ROHM or rohm to all definitions here? I think that would make
definitions pretty long so I would certinly need to split more lines.
Such cange would also impact already applied patches. So maybe I
misinterpreted your comment? And in case I did not - can this prefixing of
types - be done as a separate patch set as it impacts to regulators and
clk too? (clk is not yet applied so that is easy to change though).

Any lines which are already long, you can justify as not having to do
this, however I think for the filenames and function names it would
be nice if they were prefixed.

Right. For file names this should be easily doable. And when the regmap
wrappers are removed there are no public functions left. And I think the
name of file containing the functions is sufficient for grouping
functions under "Rohm", right? 

That's fine.

quoted

Filenames are particularly important.  That way all of the Rohm
drivers will be grouped.  Unless you can be assured that all Rohm
devices will be prefixed by 'bd', then the point is slightly mooted,
however since 'bd' doesn't really correlate with 'rohm' it's still
difficult to assume that bd* drivers are associated with Rohm -- if
you catch my drift.

I guess I do catch it. And no, all ROHM stuff will definitely not be
prefixed with bd - which is the name of the power chip

I mean power IC series.

Now you're getting it! ;)

quoted

quoted

quoted

quoted

+struct bd71837_board {
+	struct regulator_init_data *init_data[BD71837_REGULATOR_CNT];
+	int	gpio_intr;
+};

Where is this populated?

Currently nowhere as I use device-tree for getting the regulator/irq
config. This is for architectures which do not use DT - but as I don't
have one for testing I did leave the depends_on OF. If it was populated
I would expect it to be done in some setup code.

Please don't add code for 'what ifs'.

Please remove it and add it back when you need it.

Allright. Although this will also break the regulator part then...

Well, it's broken anyway ...

quoted

quoted

quoted

quoted

+/*
+ * bd71837 sub-driver chip access routines
+ */
+

Please don't abstract APIs for no reason.

Use the regmap_* API directly instead.

Yes. This was already pointed out by Stephen Boyd. But again, as part of
the patches (reguators) were already applied using the wrappers - I asked
if I can remove these in separate patch set after getting this initial
version out.

That is one of the issues with applying related patches without each
of them being reviewed first.  Applying unsuitable code is not the
correct thing to do, sorry.

So I assume you are not Ok with adding the wrappers and removing them
with later set of patches? I'll do following workaround then:

No, I'm not okay with that at all. :|

1. Change MFD Kconfig option name => current regulator code will not be
compiled even if the MFD would be applied.
2. Change MFD according to this discussion (and break the compatibility
with applied regulator code)
3. Fix the regulator code with further patches to Mark
4. Fix the depends_on Kconfig option in regulator tree to match the new
one suggested here.

Does this sound reasonable?

That's how I would do it.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog