Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754598AbbKQSuh (ORCPT ); Tue, 17 Nov 2015 13:50:37 -0500 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:36288 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754487AbbKQSuf (ORCPT ); Tue, 17 Nov 2015 13:50:35 -0500 Date: Tue, 17 Nov 2015 18:50:30 +0000 From: Mark Brown To: James Ban Cc: LINUXKERNEL , Liam Girdwood , David Dajun Chen , Support Opensource Message-ID: <20151117185030.GD31303@sirena.org.uk> References: <201511170518.tAH5Ic1j043774@krsrvapps-01.diasemi.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="SSQrXjJAjSvYS5Wm" Content-Disposition: inline In-Reply-To: <201511170518.tAH5Ic1j043774@krsrvapps-01.diasemi.com> X-Cookie: Does the name Pavlov ring a bell? User-Agent: Mutt/1.5.24 (2015-08-30) X-SA-Exim-Connect-IP: 94.175.94.161 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [PATCH V1] regulator: pv88060: new regulator driver X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: Yes (on mezzanine.sirena.org.uk) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2243 Lines: 68 --SSQrXjJAjSvYS5Wm Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Nov 17, 2015 at 11:25:21AM +0900, James Ban wrote: A couple of minor points but overall this looks good: > + node = of_get_child_by_name(dev->of_node, "regulators"); > + if (!node) { > + dev_err(dev, "regulators node not found\n"); > + return ERR_PTR(-ENODEV); > + } > + > + num = of_regulator_match(dev, node, pv88060_matches, > + ARRAY_SIZE(pv88060_matches)); Don't manually parse the DT, use the core support and set of_match and regulators_node in the regulator_desc. > + if (i == 0) { > + pv88060_matches[i].init_data->constraints > + .valid_modes_mask > + |= REGULATOR_MODE_FAST > + | REGULATOR_MODE_NORMAL > + | REGULATOR_MODE_STANDBY; > + pv88060_matches[i].init_data->constraints.valid_ops_mask > + |= REGULATOR_CHANGE_MODE; > + } The driver should never modify the constraints that are passed in - the whole point with constraints is to allow the system to set up what is safe on that particular system. > + if (!chip->pdata) > + chip->pdata = pv88060_parse_regulators_dt(chip->dev); > + > + if (IS_ERR(chip->pdata)) { > + dev_err(chip->dev, "No regulators defined for the platform\n"); > + return PTR_ERR(chip->pdata); > + } This should not be an error, the driver should just instantiate all regulators the device has and at least let people read back the current state. --SSQrXjJAjSvYS5Wm Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWS3b2AAoJECTWi3JdVIfQ4JcH/il5dghN7S7GpLWimUrTy+Ll Umr5zk0pYSty908atQ8pL4B8gNVlgIN1jLbDuNQ33QdFdTdViTieLg7z0ZoOU4Bw xSkFPDQH3PZK2lxLthCIa7/eqE2cZLJrMQVknGYGfXgnUOYAeFuIz20j9RZbtWxq e1/dHs+UzH1rm7t12xyPFvLeYRvhWoVkyGM27Aozeg1EdSu6wp4LwP1yfdgm9Tmc ze6mNTScUnPqnaYOOmdALE19E2esWmwras1iQBaKN7q88k7KEQp2+AD2f4qPj/5D WgHTpAdubGrg3k/7phGAFRx1uuMLoNDu379Db8TdIYLbavGhPPgHMpq+P8CTenQ= =Xpq4 -----END PGP SIGNATURE----- --SSQrXjJAjSvYS5Wm-- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/