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

[Heiko Schocher]

Hello Pali,

On 05.04.22 16:10, Pali Rohár wrote:
> 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 at 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.

>From my side this is full okay to add here a new compatibility string
to differ between the two chips, and to see in DTS immediately which
chip is on the board. Also later if the driver really supports features
the other chip does not have, you do not need to change DTS anymore.

I would love to see this patch first in linux. Do you plan to sent
similiar change to linux?

And not forget, please add a documentation for the compatible string
in u-boot:/doc/device-tree-bindings/

Thanks!

bye,
Heiko
> 
>> 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
>>

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de