Re: [PATCH 2/2] bootcount: Add driver model I2C driver

[Philip Oberfichtner <pro@denx.de>]

Hi Simon,

maybe you can give me a hint on how to implement this cleanly in driver
model?

To sum it up, I'd like to have a phandle pointing to *any* I2C device,
not knowing which UCLASS actually fits. Then the device and the parent
bus should be probed/prepared such that dm_i2c_read() can be used.

Any ideas on this?

Best regards,
Philip


--
=====================================================================
DENX Software Engineering GmbH,        Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-22  Fax: +49-8142-66989-80   Email: pro@denx.de
=====================================================================

On Fri, Oct 13, 2023 at 02:58:04PM +0200, Philip Oberfichtner wrote:
> Hello Heiko,
> 
> On Fri, Oct 13, 2023 at 01:28:47PM +0200, Heiko Schocher wrote:
> > [...]
> > > 
> > > 	bootcount {
> > > 		compatible = "u-boot,bootcount-i2c";
> > > 		i2c-bus = <&i2c1>;
> > > 		address = <0x52>;
> > 
> > Hmm.. do we really need this here with DTS. Why not using a phandle
> > to a real i2c device? Something like this for example:
> > 
> > 		i2cbcdev = &i2c_rtc;
> > 
> > with
> > 
> > &i2c1 {
> >         i2c_rtc: rtc@68 {
> > [...]
> > 
> > and so there is no need for knowing the bus and address ...
> > 
> 
> Yeah I agree that would be much better, but ...
> 
> > [...]
> > 
> > when you use a phandle, you can replace the part from reading "offset"
> > with this:
> > 
> > 	uclass_get_device_by_phandle(UCLASS_I2C, dev, "i2cbcdev", &priv->bcdev);
> > 
> > of course plus error checking...
> 
> This does not work, UCLASS_I2C is used for the *bus above* the device we
> actually want.
> 
> Next thing I thougt about: Why not use the UCLASS_I2C_GENERIC. But this
> expects a "i2c-chip" compatible string, which our rtc or whatever device
> obviously does not have.
> 
> I think using UCLASS_I2C_GENERIC might indeed be the best approach.
> Maybe using something like 'device_bind_driver()'. But again, this
> expects the parent device to be there already as far as I can tell.
> 
> Any suggestions?
> 
> Best regards,
> Philip
> 
> 
> > 
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static const struct bootcount_ops bootcount_i2c_ops = {
> > > +	.get = bootcount_i2c_get,
> > > +	.set = bootcount_i2c_set,
> > > +};
> > > +
> > > +static const struct udevice_id bootcount_i2c_ids[] = {
> > > +	{ .compatible = "u-boot,bootcount-i2c" },
> > > +	{ }
> > > +};
> > > +
> > > +U_BOOT_DRIVER(bootcount_i2c) = {
> > > +	.name		= "bootcount-i2c",
> > > +	.id		= UCLASS_BOOTCOUNT,
> > > +	.priv_auto	= sizeof(struct bootcount_i2c_priv),
> > > +	.probe		= bootcount_i2c_probe,
> > > +	.of_match	= bootcount_i2c_ids,
> > > +	.ops		= &bootcount_i2c_ops,
> > > +};
> > > 
> > 
> > bye,
> > Heiko
> > -- 
> > DENX Software Engineering GmbH,      Managing Director: Erika Unter
> > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> > Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de