Re: [PATCH] misc: atsha204a: Add support for atsha204 chip

["Pali Rohár" <pali@kernel.org>]

On Tuesday 05 April 2022 15:52:17 Stefan Roese wrote:
> On 4/5/22 15:28, Pali Rohár wrote:
> > On Tuesday 05 April 2022 15:14:52 Stefan Roese wrote:
> > > On 4/5/22 14:49, Pali Rohár wrote:
> > > > atsha204 chip is predecessor of atsha204a chip. Current U-Boot driver
> > > > atsha204a-i2c.c can use both atsha204 and atsha204a chips because it does
> > > > not call specific functions to just one of these chips.
> > > > 
> > > > So just add compatible string for atsha204.
> > > > 
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > ---
> > > >    drivers/misc/atsha204a-i2c.c | 1 +
> > > >    1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/misc/atsha204a-i2c.c b/drivers/misc/atsha204a-i2c.c
> > > > index 63fe541dade3..8b0055f99893 100644
> > > > --- a/drivers/misc/atsha204a-i2c.c
> > > > +++ b/drivers/misc/atsha204a-i2c.c
> > > > @@ -399,6 +399,7 @@ static int atsha204a_of_to_plat(struct udevice *dev)
> > > >    }
> > > >    static const struct udevice_id atsha204a_ids[] = {
> > > > +	{ .compatible = "atmel,atsha204" },
> > > >    	{ .compatible = "atmel,atsha204a" },
> > > >    	{ }
> > > >    };
> > > 
> > > Why do we need this new compatible here in the driver?
> > 
> > They are different chips,
> 
> Sure...
> 
> > so should have different compatible strings.
> 
> ... but is this really necessary and "best practice"? If the driver
> can handle both chips without any changes, why do you need the new
> compatible here?

Well, currently it can handle both of them.

But if driver is going to be extended to support e.g. SHA command
(Calculate a SHA256 digest) then this command should be issued only for
atsha204a. atsha204 does not support it.

Similarly, if other DTS-based system is going to implement that SHA
command, it would mean that U-Boot DTS file would not be compatible with
that other system.

Also it is a good idea to have DTS files and its compatible strings
universal and not u-boot specific. So it could be used also by other
projects (e.g. linux kernel).

And if we mix now two chips which are similar (and supports lot of
common operations) we would not be able in future to extend drivers in
backward compatible manner.

Just to note, I'm not going to implement atsha204a specific commands
(which are not available in atsha204; like SHA command) because I do not
need them (right now).

> Don't get me wrong. I'm not blocking this change, just want to be sure
> that it's really necessary.

In case U-Boot driver has compatible string something like
"atsha204-common" which could say that driver is using only functions
which are available in all chip family then there would not be need for
it. But if driver has chip specific name, I think the best is not to
mask one chip by another which does not have 1:1 SW API compatibility.

> Thanks,
> Stefan
> 
> > > A quick grep
> > > doesn't show this in any of the dts files, not in U-Boot and not in the
> > > Kernel.
> > 
> > Not yet. I'm preparing patches for a board which has atsha204 and will
> > use this u-boot driver.
> > 
> > > Just checking...
> > > 
> > > Thanks,
> > > Stefan
>