Re: [PATCH] misc: atsha204a: Add support for atsha204 chip
Hello Pali.
On 10.05.22 06:45, Heiko Schocher wrote:
> Hello Pali,
>
> On 22.04.22 05:59, Heiko Schocher wrote:
>> Hello Pali,
>>
>> On 21.04.22 11:40, Pali Rohár wrote:
>>> On Thursday 21 April 2022 06:11:11 Heiko Schocher wrote:
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
> ---
>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?
>>>
>>> Hello! We are not using Linux kernel driver for atsha cryptochips (I was
>>> told that it decrease lifetime) but I can send also similar change to
>>> Linux.
>>
>> See it, thanks!
>
> Reviewed-by: Heiko Schocher
>
> Will apply soon, as it is accepted in linux.
applied to u-boot-i2c master
Thanks!
bye,
Heiko
--
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: h...@denx.de
Re: [PATCH] misc: atsha204a: Add support for atsha204 chip
Hello Pali,
On 22.04.22 05:59, Heiko Schocher wrote:
> Hello Pali,
>
> On 21.04.22 11:40, Pali Rohár wrote:
>> On Thursday 21 April 2022 06:11:11 Heiko Schocher wrote:
>>> 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
---
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?
>>
>> Hello! We are not using Linux kernel driver for atsha cryptochips (I was
>> told that it decrease lifetime) but I can send also similar change to
>> Linux.
>
> See it, thanks!
Reviewed-by: Heiko Schocher
Will apply soon, as it is accepted in linux.
bye,
Heiko
--
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: h...@denx.de
Re: [PATCH] misc: atsha204a: Add support for atsha204 chip
On Friday 22 April 2022 05:59:28 Heiko Schocher wrote: > Hello Pali, > > On 21.04.22 11:40, Pali Rohár wrote: > > On Thursday 21 April 2022 06:11:11 Heiko Schocher wrote: > >> And not forget, please add a documentation for the compatible string > >> in u-boot:/doc/device-tree-bindings/ > > > > Currently I do not see any information about atsha in > > u-boot/doc/device-tree-bindings. > > So please add one, thanks! Hello! Here is the patch for bindings: https://patchwork.ozlabs.org/project/uboot/patch/20220428185828.19513-1-p...@kernel.org/
Re: [PATCH] misc: atsha204a: Add support for atsha204 chip
Hello Pali,
On 21.04.22 11:40, Pali Rohár wrote:
> On Thursday 21 April 2022 06:11:11 Heiko Schocher wrote:
>> 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
>>> ---
>>>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?
>
> Hello! We are not using Linux kernel driver for atsha cryptochips (I was
> told that it decrease lifetime) but I can send also similar change to
> Linux.
See it, thanks!
>> And not forget, please add a documentation for the compatible string
>> in u-boot:/doc/device-tree-bindings/
>
> Currently I do not see any information about atsha in
> u-boot/doc/device-tree-bindings.
So please add one, thanks!
bye,
Heiko
>
>> 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: h...@denx.de
--
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: h...@denx.de
Re: [PATCH] misc: atsha204a: Add support for atsha204 chip
On Thursday 21 April 2022 11:40:47 Pali Rohár wrote:
> On Thursday 21 April 2022 06:11:11 Heiko Schocher wrote:
> > 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
> > > ---
> > >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?
>
> Hello! We are not using Linux kernel driver for atsha cryptochips (I was
> told that it decrease lifetime) but I can send also similar change to
> Linux.
Now I sent patch to Linux:
https://lore.kernel.org/linux-crypto/20220421134457.5867-1-p...@kernel.org/T/#u
> > And not forget, please add a documentation for the compatible string
> > in u-boot:/doc/device-tree-bindings/
>
> Currently I do not see any information about atsha 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: h...@denx.de
Re: [PATCH] misc: atsha204a: Add support for atsha204 chip
On Thursday 21 April 2022 06:11:11 Heiko Schocher wrote:
> 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
> > ---
> >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?
Hello! We are not using Linux kernel driver for atsha cryptochips (I was
told that it decrease lifetime) but I can send also similar change to
Linux.
> And not forget, please add a documentation for the compatible string
> in u-boot:/doc/device-tree-bindings/
Currently I do not see any information about atsha 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: h...@denx.de
Re: [PATCH] misc: atsha204a: Add support for atsha204 chip
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
> ---
>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: h...@denx.de
Re: [PATCH] misc: atsha204a: Add support for atsha204 chip
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
> > > > ---
> > > >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
>
Re: [PATCH] misc: atsha204a: Add support for atsha204 chip
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
---
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?
Don't get me wrong. I'm not blocking this change, just want to be sure
that it's really necessary.
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
Re: [PATCH] misc: atsha204a: Add support for atsha204 chip
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
> > ---
> > 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, so should have different compatible strings.
> 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
Re: [PATCH] misc: atsha204a: Add support for atsha204 chip
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
---
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? A quick grep
doesn't show this in any of the dts files, not in U-Boot and not in the
Kernel.
Just checking...
Thanks,
Stefan
[PATCH] misc: atsha204a: Add support for atsha204 chip
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
---
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" },
{ }
};
--
2.20.1
