Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
From:   "Matyas, Daniel" <Daniel.Matyas@analog.com>
To:     Guenter Roeck <linux@roeck-us.net>
CC:     Jean Delvare <jdelvare@suse.com>, Jonathan Corbet <corbet@lwn.net>,
        "linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v5 2/4] hwmon: max31827: Add support for max31828 and
 max31829
Thread-Topic: [PATCH v5 2/4] hwmon: max31827: Add support for max31828 and
 max31829
Thread-Index: AQHaCBrux3jCd9w9Ok6JeCc2kSWWE7BdFUuAgABLXpCAADv+kA==
Date:   Fri, 27 Oct 2023 13:07:04 +0000
Message-ID: <PH0PR03MB6771212410349459B2865BE889DCA@PH0PR03MB6771.namprd03.prod.outlook.com>
References: <20231026144405.546822-1-daniel.matyas@analog.com>
 <20231026144405.546822-2-daniel.matyas@analog.com>
 <fe99431e-3245-484c-bf26-928048500ec6@roeck-us.net>
 <PH0PR03MB67712B3C063B794442F6D58A89DCA@PH0PR03MB6771.namprd03.prod.outlook.com>
In-Reply-To: <PH0PR03MB67712B3C063B794442F6D58A89DCA@PH0PR03MB6771.namprd03.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?lnmDeQSqSiHy9CNuKUlJ+wPfLigIFw/jctGY5v8itSZLPgKx3mDzr57rvYHk?=
 =?us-ascii?Q?Rv91R/3ujK+ILdUt7br1+sWDmd7BL2+kNUYWtLjjHr2AIBL/lS61yw7SqkME?=
 =?us-ascii?Q?BZTFBfNSICksOQP5P4yZ5tfmJKKaJUcguzX8cv7qOsdpftmWfRln5I28RMJ9?=
 =?us-ascii?Q?HMdKqSAcjqAsLF0HAEnWRtmYjo0BYd1rzLLp3DNiZpTl6o1B7uenLq8du06L?=
 =?us-ascii?Q?IAowiHIfYxcJQiNIF4OXOgpZdvtTtFy024+8i5Y9knh+C4vNw1dGumIGawa6?=
 =?us-ascii?Q?zlrbBtibGJANnffUkZI6sUMTWr8legom2d/SiND8/5gBj09ThEWnJODZ373i?=
 =?us-ascii?Q?dk+wgGPu3dVj2p1rpvCDPXsTGXnEEMvIBSn15X2w1mIyOzJYEXu1HiDkNvwL?=
 =?us-ascii?Q?V4Ow3vgTIT8IxN9BZ6VPA+AIFFpyP+Pxrry/YwzGmzhO+ljeH+t0pU87oMrw?=
 =?us-ascii?Q?S4mCFMbEGkg6MoXkr21hokFf4CsbyNMHuMDhhKMIEWT8lScZ3XQ8flh1S7dv?=
 =?us-ascii?Q?Nsb2V92BgJj/4k2dDWqRiH5Q7uo6lkdf4WYt9vgXzwdAQAaqVI+vwPK12bJ2?=
 =?us-ascii?Q?9LVhPQVd1gASjfxvjLAPeqw4fCo57Vbk63Hc8w+6c9B4TuWMHrBF0fXZCY6/?=
 =?us-ascii?Q?GEeW34coPXDOHYcahpuOzBNACQeM8GbdkbviRJWfl39hf1Pbyww125tHxUwg?=
 =?us-ascii?Q?P7p6sGapGHS2CaW522Z4iV8/nE4YRiLCPlK2p2WFsWohcuhwFC+28GHETiRc?=
 =?us-ascii?Q?AJz8/wVreuD6/Vs4SvGskaMt+fn3Urxl4bhwFqUvVcJoPIv82IPlUUn9dPc6?=
 =?us-ascii?Q?NL8WFlDdTMdSgG5pFOeiVHIhh7bgOp1/kIX/R9nCSXES/qoiP/7IY/2kmakV?=
 =?us-ascii?Q?pOe7zkEOPPtM+sbpz+MxUulR4XtMNWpBOedZq+8cRsKXRnTM/ZZamjBa8dPR?=
 =?us-ascii?Q?FZV3mxOsdAn5ilOlIMySC1Y6UTqp3xILjxCZQl2Z2Xi0Ilv9DhUfPGCUIiM7?=
 =?us-ascii?Q?XK5qk6VZRwWUPoGU/ArL/K39mMB7z5JEDMBFJ29krTq/x7tt9JbBegFNNoBj?=
 =?us-ascii?Q?nrU3w7u2XxfxsNf7+pPV4JuM6XNuEu1uy6aR5nw9oaHx31v8tA0sfOGmAiuq?=
 =?us-ascii?Q?5pkbC/5n0xdgEHCC18GrcbNQin3G31LS23rMRXh7x9Q3HzrhnlnBqKQMwm5o?=
 =?us-ascii?Q?erio/NJdmYeR1MYKl+vYCBwDy5MI1Eq9vSksu58PfZA50zfPc3gAxKk/OsVv?=
 =?us-ascii?Q?3g7PRrW5T+4FVBKJhlQipvHCjHsyREQhEuSZQb+7u4i2L+6WYT4/pjpMCyHF?=
 =?us-ascii?Q?boRIhwssBTOPD80/N7DiQolrfdEhdnA+aJkhj503xfYOb74K/kUMslutemNf?=
 =?us-ascii?Q?s+iU+O69v9PvYXPHQPYIatfOPj5kjyYIMVoboQehBLtwdjlWpCs7NOYarpYI?=
 =?us-ascii?Q?2bS9Ma4awMSF/laIX+u58NILRnhcNuShQoztPFpAPGOdAMRNPiI87hwr5hXS?=
 =?us-ascii?Q?7boI9mgodSri7fn6jFDvwK5a8WOcAQ5wpn3OZwWEdDsP9f8eTXXcUivKMjGP?=
 =?us-ascii?Q?C/v9PZepcLeElTVJFw+c3w78HT6tUcw1LlrV4sdC?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: analog.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PH0PR03MB6771.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 9c616343-359d-471e-b6a2-08dbd6ed9dc6
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Oct 2023 13:07:04.5576
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: eaa689b4-8f87-40e0-9c6f-7228de4d754a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: mMsQ9cdfhjVGnY2alP288C+xqISTed3bk8BbGKfgJQnhC3Gw0G9cTOlGG4utucGau+25WCdI/8OnWFClXwEKUUS1UCFBZVZ+WQsEqTU0w74=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR03MB6732
X-Proofpoint-GUID: Yxn1EZfvFJrnlKxRUp_o_bxLNyR0g01g
X-Proofpoint-ORIG-GUID: Yxn1EZfvFJrnlKxRUp_o_bxLNyR0g01g
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-10-27_11,2023-10-27_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 suspectscore=0
 impostorscore=0 bulkscore=0 lowpriorityscore=0 malwarescore=0 spamscore=0
 mlxlogscore=999 adultscore=0 priorityscore=1501 phishscore=0 clxscore=1015
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.19.0-2310240000
 definitions=main-2310270113
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Fri, 27 Oct 2023 06:07:34 -0700 (PDT)


-----Original Message-----
From: Matyas, Daniel=20
Sent: Friday, October 27, 2023 4:01 PM
To: Guenter Roeck <linux@roeck-us.net>
Cc: no To-header on input <;>; Jean Delvare <jdelvare@suse.com>; Jonathan C=
orbet <corbet@lwn.net>; linux-hwmon@vger.kernel.org; linux-doc@vger.kernel.=
org; linux-kernel@vger.kernel.org
Subject: RE: [PATCH v5 2/4] hwmon: max31827: Add support for max31828 and m=
ax31829


> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Friday, October 27, 2023 8:02 AM
> To: Matyas, Daniel <Daniel.Matyas@analog.com>
> Cc: no To-header on input <;>; Jean Delvare <jdelvare@suse.com>;=20
> Jonathan Corbet <corbet@lwn.net>; linux-hwmon@vger.kernel.org; linux-=20
> doc@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v5 2/4] hwmon: max31827: Add support for
> max31828 and max31829
>=20
> [External]
>=20
> On Thu, Oct 26, 2023 at 05:44:02PM +0300, Daniel Matyas wrote:
> > When adi,flt-q and/or adi,alrm-pol are not mentioned, the default=20
> > configuration is loaded.
> >
> That isn't really a complete patch description.
> It should include (even if repeated) that support for additional chips=20
> is added.
>=20
> > Signed-off-by: Daniel Matyas <daniel.matyas@analog.com>
> > ---
> >
> > v4 -> v5: Passed i2c_client to init_client(), because I needed it to=20
> > retrieve device id.
> > Used a simple if to load default configuration. No more switch.
> >
> > v3 -> v4: No change.
> >
> > v3: Added patch.
> >
> >  drivers/hwmon/max31827.c | 51
> > +++++++++++++++++++++++++++++++---------
> >  1 file changed, 40 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/hwmon/max31827.c b/drivers/hwmon/max31827.c
> index
> > 7976d668ffd4..446232fa1710 100644
> > --- a/drivers/hwmon/max31827.c
> > +++ b/drivers/hwmon/max31827.c
> > @@ -39,10 +39,15 @@
> >
> >  #define MAX31827_12_BIT_CNV_TIME	140
> >
> > +#define MAX31827_ALRM_POL_HIGH	0x1
> > +#define MAX31827_FLT_Q_4	0x2
> > +
> >  #define MAX31827_16_BIT_TO_M_DGR(x)	(sign_extend32(x, 15) *
> 1000 / 16)
> >  #define MAX31827_M_DGR_TO_16_BIT(x)	(((x) << 4) / 1000)
> >  #define MAX31827_DEVICE_ENABLE(x)	((x) ? 0xA : 0x0)
> >
> > +enum chips { max31827, max31828, max31829 };
> > +
> >  enum max31827_cnv {
> >  	MAX31827_CNV_1_DIV_64_HZ =3D 1,
> >  	MAX31827_CNV_1_DIV_32_HZ,
> > @@ -373,12 +378,22 @@ static int max31827_write(struct device *dev,
> enum hwmon_sensor_types type,
> >  	return -EOPNOTSUPP;
> >  }
> >
> > +static const struct i2c_device_id max31827_i2c_ids[] =3D {
> > +	{ "max31827", max31827 },
> > +	{ "max31828", max31828 },
> > +	{ "max31829", max31829 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
> > +
> >  static int max31827_init_client(struct max31827_state *st,
> > -				struct device *dev)
> > +				struct i2c_client *client)
> >  {
> > +	struct device *dev =3D &client->dev;
> >  	struct fwnode_handle *fwnode;
> >  	unsigned int res =3D 0;
> >  	u32 data, lsb_idx;
> > +	enum chips type;
> >  	bool prop;
> >  	int ret;
> >
> > @@ -395,13 +410,20 @@ static int max31827_init_client(struct
> max31827_state *st,
> >  	prop =3D fwnode_property_read_bool(fwnode, "adi,timeout-
> enable");
> >  	res |=3D
> FIELD_PREP(MAX31827_CONFIGURATION_TIMEOUT_MASK, !prop);
> >
> > +	if (dev->of_node)
> > +		type =3D (enum chips)of_device_get_match_data(dev);
> > +	else
> > +		type =3D i2c_match_id(max31827_i2c_ids, client)-
> >driver_data;
> > +
>=20
> This should be something like
>=20
> 	type =3D (enum chips)(uintptr_t)device_get_match_data(dev);
>=20
> though that requires that the enum values start with 1. This avoids=20
> having to pass client to the function and is more generic.
>=20
> >  	if (fwnode_property_present(fwnode, "adi,alarm-pol")) {
> >  		ret =3D fwnode_property_read_u32(fwnode, "adi,alarm-
> pol", &data);
> >  		if (ret)
> >  			return ret;
> >
> >  		res |=3D
> FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
> > -	}
> > +	} else if (type =3D=3D max31829)
>=20
> Not sure why checkpatch ignores this (maybe because of 'else if'), but=20
> the else path should also be in {}.
>=20
> But why is this only for max31829 ? I mean, sure, the default for that=20
> chip is different, but that doesn't mean the other chips have that bit=20
> set. There is no guarantee that the chip is in its default state when=20
> the driver is loaded.
>=20
> > +		res |=3D
> FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
> > +				  MAX31827_ALRM_POL_HIGH);
> >
> >  	if (fwnode_property_present(fwnode, "adi,fault-q")) {
> >  		ret =3D fwnode_property_read_u32(fwnode, "adi,fault-q",
> &data); @@
> > -418,7 +440,9 @@ static int max31827_init_client(struct
> max31827_state *st,
> >  		}
> >
> >  		res |=3D
> FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK, lsb_idx);
> > -	}
> > +	} else if ((type =3D=3D max31828) || (type =3D=3D max31829))
>=20
> I _really_ dislike the notion of excessive ( ). Also, {} for the else bra=
nch.
>=20
> I also don't understand why that would be chip specific. I don't see=20
> anything along that line in the datasheet.
>=20
> Ah, wait ... I guess that is supposed to reflect the chip default.
> I don't see why the chip default makes a difference - a well defined=20
> default must be set either way. Again, there is no guarantee that the=20
> chip is in its default state when the driver is loaded.

The well defined default was set in v4, but I deleted it, because the defau=
lt value in hex for max31827 and max31828 alarm polarity, and max31827 faul=
t queue is 0x0. I had 2 #defines for these values, but you said:
" Since MAX31827_ALRM_POL_LOW is 0, this code doesn't really do anything an=
d just pollutes the code. "

So, I thought I should remove it altogether, since res is set to 0 in the b=
eginning and the default value of these chips (i.e. 0) is implicitly set.

>=20
> Also, why are the default values added in this patch and not in the=20
> previous patch ?
>

In v4 these default values were set in the previous patch.
=20
> > +		res |=3D
> FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
> > +				  MAX31827_FLT_Q_4);
> >
> >  	return regmap_write(st->regmap,
> MAX31827_CONFIGURATION_REG, res);  }
> > @@ -464,7 +488,7 @@ static int max31827_probe(struct i2c_client
> *client)
> >  		return dev_err_probe(dev, PTR_ERR(st->regmap),
> >  				     "Failed to allocate regmap.\n");
> >
> > -	err =3D max31827_init_client(st, dev);
> > +	err =3D max31827_init_client(st, client);
> >  	if (err)
> >  		return err;
> >
> > @@ -475,14 +499,19 @@ static int max31827_probe(struct i2c_client
> *client)
> >  	return PTR_ERR_OR_ZERO(hwmon_dev);  }
> >
> > -static const struct i2c_device_id max31827_i2c_ids[] =3D {
> > -	{ "max31827", 0 },
> > -	{ }
> > -};
> > -MODULE_DEVICE_TABLE(i2c, max31827_i2c_ids);
> > -
> >  static const struct of_device_id max31827_of_match[] =3D {
> > -	{ .compatible =3D "adi,max31827" },
> > +	{
> > +		.compatible =3D "adi,max31827",
> > +		.data =3D (void *)max31827
> > +	},
> > +	{
> > +		.compatible =3D "adi,max31828",
> > +		.data =3D (void *)max31828
> > +	},
> > +	{
> > +		.compatible =3D "adi,max31829",
> > +		.data =3D (void *)max31829
> > +	},
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(of, max31827_of_match);