Re: [PATCH v2 2/2] hwmon: ltc4282: add support for the LTC4282 chip

From: "Nuno Sá" <noname.nuno@gmail.com>
To: Krzysztof Kozlowski <krzk@kernel.org>,
	kernel test robot <lkp@intel.com>,
	 Nuno Sa via B4 Relay <devnull+nuno.sa.analog.com@kernel.org>,
	linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
	 linux-doc@vger.kernel.org
Cc: oe-kbuild-all@lists.linux.dev, Jean Delvare <jdelvare@suse.com>,
	Guenter Roeck <linux@roeck-us.net>,
	Rob Herring <robh+dt@kernel.org>,
	Conor Dooley <conor+dt@kernel.org>,
	 Jonathan Corbet <corbet@lwn.net>,
	Bartosz Golaszewski <brgl@bgdev.pl>,
	Linus Walleij <linus.walleij@linaro.org>,
	Andy Shevchenko <andy@kernel.org>
Subject: Re: [PATCH v2 2/2] hwmon: ltc4282: add support for the LTC4282 chip
Date: Mon, 27 Nov 2023 09:44:31 +0100	[thread overview]
Message-ID: <5a4182806724fa96eab92e7224d11aec7aa39e20.camel@gmail.com> (raw)
In-Reply-To: <ac950d01-d9aa-4fb7-810d-b21335e4cc94@kernel.org>

On Mon, 2023-11-27 at 09:12 +0100, Krzysztof Kozlowski wrote:
> On 27/11/2023 09:10, Krzysztof Kozlowski wrote:
> > On 27/11/2023 08:53, Nuno Sá wrote:
> > > >    355  
> > > >  > 356  unsigned long ltc4282_recalc_rate(struct clk_hw *hw, unsigned long
> > > > parent)
> > > >    357  {
> > > >    358          struct ltc4282_state *st = container_of(hw, struct
> > > > ltc4282_state,
> > > >    359                                                  clk_hw);
> > > >    360          u32 clkdiv;
> > > >    361          int ret;
> > > >    362  
> > > >    363          ret = regmap_read(st->map, LTC4282_CLK_DIV, &clkdiv);
> > > >    364          if (ret)
> > > >    365                  return 0;
> > > >    366  
> > > >    367          clkdiv = FIELD_GET(LTC4282_CLKOUT_MASK, clkdiv);
> > > >    368          if (!clkdiv)
> > > >    369                  return 0;
> > > >    370          if (clkdiv == LTC4282_CLKOUT_INT)
> > > >    371                  return LTC4282_CLKOUT_SYSTEM;
> > > >    372  
> > > >    373          return LTC4282_CLKOUT_CNV;
> > > >    374  }
> > > >    375  
> > > > 
> > > 
> > > Arghh, I do need to see if I can add some test branch of my own to the test
> > > robot :/.
> > > Anyways, will wait for some more reviewing before sending v3 to address this.
> > 
> > It's easy to test your patches on your own machines...  Just build few
> > different configs.
> > 
> 
> Wait, this was not even unusual test, just standard compile, which means
> you did not do basic tests on your end. You must build your new driver
> with W=1, smatch, sparse and coccinelle before sending upstream.
> 
> Best regards,
> Krzysztof
> 

Well, I do agree that a sparse build would easily caught this and I'm mad with myself
because that much (together with checkpatch) I usually run (I confess that coccinelle
and smatch is something that I don't usually do. 

"you did not do basic tests on your end"

However I do think this is a bit unfair. I did got a lot of things to do in v2 with
major refactors on some of the features. So "basic test" to me is actually making
sure that all of these changes don't break the driver and the device still works as
expected.

Anyways, I don't want to find any execuses and start an argument. Will try to do
better in next iterations.

- Nuno Sá