Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 31 Jul 2023 18:05:31 +0200
From:   Miquel Raynal <miquel.raynal@bootlin.com>
To:     Chen-Yu Tsai <wenst@chromium.org>
Cc:     Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        Robert Marko <robert.marko@sartura.hr>,
        Luka Perkov <luka.perkov@sartura.hr>,
        Michael Walle <michael@walle.cc>,
        Randy Dunlap <rdunlap@infradead.org>
Subject: Re: [PATCH v6 3/3] nvmem: core: Expose cells through sysfs
Message-ID: <20230731180531.7c616b3b@xps-13>
In-Reply-To: <20230718102617.GA982837@google.com>
References: <20230717075147.43326-1-miquel.raynal@bootlin.com>
        <20230717075147.43326-4-miquel.raynal@bootlin.com>
        <20230718102617.GA982837@google.com>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Chen-Yu,

> >  static int nvmem_sysfs_setup_compat(struct nvmem_device *nvmem,
> > @@ -1006,6 +1101,12 @@ struct nvmem_device *nvmem_register(const struct=
 nvmem_config *config)
> >  	if (rval)
> >  		goto err_remove_cells;
> > =20
> > +#ifdef CONFIG_NVMEM_SYSFS
> > +	rval =3D nvmem_populate_sysfs_cells(nvmem);
> > +	if (rval)
> > +		goto err_remove_cells; =20
>=20
> This breaks nvmem / efuse devices with multiple cells that share the
> same name. Something like this in DT:
>=20
> 	efuse: efuse@11f10000 {
> 		compatible =3D "mediatek,mt8183-efuse",
> 			     "mediatek,efuse";
> 		reg =3D <0 0x11f10000 0 0x1000>;
> 		#address-cells =3D <1>;
> 		#size-cells =3D <1>;
> 		thermal_calibration: calib@180 {
> 			reg =3D <0x180 0xc>;
> 		};
>=20
> 		mipi_tx_calibration: calib@190 {
> 			reg =3D <0x190 0xc>;
> 		};
>=20
> 		svs_calibration: calib@580 {
> 			reg =3D <0x580 0x64>;
> 		};
> 	};
>=20
> creates three cells, all named DT, and sysfs will complain:
>=20
>     sysfs: cannot create duplicate filename '/devices/platform/soc/11f100=
00.efuse/nvmem1/cells/calib'
>     mediatek,efuse: probe of 11f10000.efuse failed with error -17
>=20
> This causes the MT8183-based Chromebooks to lose display capability,
> among other things.

Sorry for the breakage, I did not identify this case, but you're right
this is incorrectly handled currently.

> The problem lies in the nvmem DT parsing code, where the cell name is
> derived from the node name, without including the address portion.
> However I'm not sure we can change that, since it could be considered
> ABI?

I would be in favor suffixing the cell names anyway as they have not
been exposed yet to userspace at all (well, not more than a couple of
days in -next).

Thanks,
Miqu=C3=A8l