Re: [PATCH V1 04/10] watchdog: da9061: watchdog driver (RFC)

Re: [PATCH V1 04/10] watchdog: da9061: watchdog driver (RFC)

[Steve Twiss]

Hi Guenter,

Thanks for your reply.

On 06 October 2016 14:28, Guenter Roeck, wrote:

> To: Steve Twiss; LINUX-KERNEL; LINUX-WATCHDOG; Wim Van Sebroeck
> Subject: Re: [PATCH V1 04/10] watchdog: da9061: watchdog driver

[...]

> > @@ -180,6 +180,11 @@ static const struct watchdog_info da9062_watchdog_info = {
> >  	.identity = "DA9062 WDT",
> >  };
> >
> > +static const struct watchdog_info da9061_watchdog_info = {
> > +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> > +	.identity = "DA9061 WDT",
> 
> This adds a lot of complexity to the driver just to be able to display "DA9061".
> Why not just change the existing identity to "DA9061/DA9062 WDT" ?

This is true.
I am using the compatible string to pick a different configuration .data block:

{ .compatible = "dlg,da9062-watchdog", .data = &da9062_watchdog_info },
{ .compatible = "dlg,da9061-watchdog", .data = &da9061_watchdog_info },

when the only real difference between the DA9061 and DA9062 watchdog driver
is the name. Functionally they are identical in this case.
This was a similar comment in Dialog's internal review on this exact same point.
"Why not just report one thing?"

My answer to that was because it would allow to distinguish between different chips.
The watchdog driver would report the correct chip type, despite the driver always
being for DA9062.

This exact same thing would happen with da9063-onkey and da9062-thermal also.
For the ONKEY it is marginally confused by needing to support 63, but for 62 and 61
it is the same thing. Only the name is different.

I have TO:'d Dmitry Torokhov; Eduardo Valentin; Zhang Rui, for that reason.

But, it is just my opinion to keep the "name" different.
This will not be my decision if accepted into the Linux kernel, but I would like to 
at least be consistent for DA9061 and DA9062 so ... is this an issue?

The same question applies to thermal and onkey as well.

Regards,
Steve

Re: [PATCH V1 04/10] watchdog: da9061: watchdog driver (RFC)

[Guenter Roeck]

Hi Steve,

On Thu, Oct 06, 2016 at 04:28:14PM +0000, Steve Twiss wrote:
> Hi Guenter,
> 
> Thanks for your reply.
> 
> On 06 October 2016 14:28, Guenter Roeck, wrote:
> 
> > To: Steve Twiss; LINUX-KERNEL; LINUX-WATCHDOG; Wim Van Sebroeck
> > Subject: Re: [PATCH V1 04/10] watchdog: da9061: watchdog driver
> 
> [...]
> 
> > > @@ -180,6 +180,11 @@ static const struct watchdog_info da9062_watchdog_info = {
> > >  	.identity = "DA9062 WDT",
> > >  };
> > >
> > > +static const struct watchdog_info da9061_watchdog_info = {
> > > +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> > > +	.identity = "DA9061 WDT",
> > 
> > This adds a lot of complexity to the driver just to be able to display "DA9061".
> > Why not just change the existing identity to "DA9061/DA9062 WDT" ?
> 
> This is true.
> I am using the compatible string to pick a different configuration .data block:
> 
> { .compatible = "dlg,da9062-watchdog", .data = &da9062_watchdog_info },
> { .compatible = "dlg,da9061-watchdog", .data = &da9061_watchdog_info },
> 
> when the only real difference between the DA9061 and DA9062 watchdog driver
> is the name. Functionally they are identical in this case.
> This was a similar comment in Dialog's internal review on this exact same point.
> "Why not just report one thing?"
> 
> My answer to that was because it would allow to distinguish between different chips.
> The watchdog driver would report the correct chip type, despite the driver always
> being for DA9062.
> 
> This exact same thing would happen with da9063-onkey and da9062-thermal also.
> For the ONKEY it is marginally confused by needing to support 63, but for 62 and 61
> it is the same thing. Only the name is different.
> 
> I have TO:'d Dmitry Torokhov; Eduardo Valentin; Zhang Rui, for that reason.
> 
> But, it is just my opinion to keep the "name" different.
> This will not be my decision if accepted into the Linux kernel, but I would like to 
> at least be consistent for DA9061 and DA9062 so ... is this an issue?
> 

Yes, for me it is. The driver is still the same, and I don't see the point
of increasing code size and making the driver less readable just to be able
to report a slightly different driver identification string. And each time
a similar HW is added we would go through the same effort, again for no good
reason.

FWIW the driver doesn't really need to be updated in the first place.
A compatible statement listing both da9061 and da9062 would do it. Plus,
on top of that, even your change would not guarantee that the output is correct.
A DT entry with lists da9061 compatibility on a da9062 system would report the
"wrong" name. And still work. So I really don't see the point.

Guenter

RE: [PATCH V1 04/10] watchdog: da9061: watchdog driver (RFC)

[Steve Twiss]

On 06 October 2016 19:49, Guenter Roeck wrote:
> Subject: Re: [PATCH V1 04/10] watchdog: da9061: watchdog driver (RFC)
> 
> Hi Steve,

[...]

> On Thu, Oct 06, 2016 at 04:28:14PM +0000, Steve Twiss wrote:
> > I am using the compatible string to pick a different configuration .data block:
> >
> > { .compatible = "dlg,da9062-watchdog", .data = &da9062_watchdog_info },
> > { .compatible = "dlg,da9061-watchdog", .data = &da9061_watchdog_info },
> >
> > when the only real difference between the DA9061 and DA9062 watchdog driver
> > is the name. Functionally they are identical in this case.
[...]
> > This exact same thing would happen with da9063-onkey and da9062-thermal also.
> > For the ONKEY it is marginally confused by needing to support 63, but for 62 and 61
> > it is the same thing. Only the name is different.
> >
[...]
> > But, it is just my opinion to keep the "name" different.
> > This will not be my decision if accepted into the Linux kernel, but I would like to
> > at least be consistent for DA9061 and DA9062 so ... is this an issue?
> 
> 
> Yes, for me it is. The driver is still the same, and I don't see the point
> of increasing code size and making the driver less readable just to be able
> to report a slightly different driver identification string. And each time
> a similar HW is added we would go through the same effort, again for no
> good reason.
> 

My reason for doing this was to report the hardware identification, not the
driver name. But, there would certainly be a lot less to do if I was to make
DA9061 core use the DA9062 watchdog.
 
> FWIW the driver doesn't really need to be updated in the first place.
> A compatible statement listing both da9061 and da9062 would do it.

I will make the changes you requested: deprecate the existing compatibility
for da9062-watchdog and make a new compatibility string which combines both
da9061 and da9062.

Regards,
Stephen

Re: [PATCH V1 04/10] watchdog: da9061: watchdog driver (RFC)

[Guenter Roeck]

On Fri, Oct 07, 2016 at 02:56:17PM +0000, Steve Twiss wrote:
> On 06 October 2016 19:49, Guenter Roeck wrote:
> > Subject: Re: [PATCH V1 04/10] watchdog: da9061: watchdog driver (RFC)
> > 
> > Hi Steve,
> 
> [...]
> 
> > On Thu, Oct 06, 2016 at 04:28:14PM +0000, Steve Twiss wrote:
> > > I am using the compatible string to pick a different configuration .data block:
> > >
> > > { .compatible = "dlg,da9062-watchdog", .data = &da9062_watchdog_info },
> > > { .compatible = "dlg,da9061-watchdog", .data = &da9061_watchdog_info },
> > >
> > > when the only real difference between the DA9061 and DA9062 watchdog driver
> > > is the name. Functionally they are identical in this case.
> [...]
> > > This exact same thing would happen with da9063-onkey and da9062-thermal also.
> > > For the ONKEY it is marginally confused by needing to support 63, but for 62 and 61
> > > it is the same thing. Only the name is different.
> > >
> [...]
> > > But, it is just my opinion to keep the "name" different.
> > > This will not be my decision if accepted into the Linux kernel, but I would like to
> > > at least be consistent for DA9061 and DA9062 so ... is this an issue?
> > 
> > 
> > Yes, for me it is. The driver is still the same, and I don't see the point
> > of increasing code size and making the driver less readable just to be able
> > to report a slightly different driver identification string. And each time
> > a similar HW is added we would go through the same effort, again for no
> > good reason.
> > 
> 
> My reason for doing this was to report the hardware identification, not the
> driver name. But, there would certainly be a lot less to do if I was to make
> DA9061 core use the DA9062 watchdog.
>  
> > FWIW the driver doesn't really need to be updated in the first place.
> > A compatible statement listing both da9061 and da9062 would do it.
> 
> I will make the changes you requested: deprecate the existing compatibility
> for da9062-watchdog and make a new compatibility string which combines both
> da9061 and da9062.
> 
That is not what I asked for.

Guenter

RE: [PATCH V1 04/10] watchdog: da9061: watchdog driver (RFC)

[Steve Twiss]

On 07 October 2016 18:02, Guenter Roeck wrote:

> > On 06 October 2016 19:49, Guenter Roeck wrote:
> > > On Thu, Oct 06, 2016 at 04:28:14PM +0000, Steve Twiss wrote:
> > > > I am using the compatible string to pick a different configuration .data block:
> > > > { .compatible = "dlg,da9062-watchdog", .data = &da9062_watchdog_info },
> > > > { .compatible = "dlg,da9061-watchdog", .data = &da9061_watchdog_info },
> > > >
> > > > But, it is just my opinion to keep the "name" different.
> > > > This will not be my decision if accepted into the Linux kernel, but I would like to
> > > > at least be consistent for DA9061 and DA9062 so ... is this an issue?
> > >
> > > FWIW the driver doesn't really need to be updated in the first place.
> > > A compatible statement listing both da9061 and da9062 would do it.
> >
> > I will make the changes you requested: deprecate the existing compatibility
> > for da9062-watchdog and make a new compatibility string which combines both
> > da9061 and da9062.
> >
> That is not what I asked for.

Ok. Did you mean separate compatible statements with data sections pointing at 
the same structure?

Like this:
{ .compatible = "dlg,da9062-watchdog", .data = &da9062_watchdog_info },
{ .compatible = "dlg,da9061-watchdog", .data = &da9062_watchdog_info },

So this would be the only change needed in the device driver.

Re: [PATCH V1 04/10] watchdog: da9061: watchdog driver (RFC)

[Dmitry Torokhov]

On Fri, Oct 07, 2016 at 06:01:23PM +0000, Steve Twiss wrote:
> On 07 October 2016 18:02, Guenter Roeck wrote:
> 
> > > On 06 October 2016 19:49, Guenter Roeck wrote:
> > > > On Thu, Oct 06, 2016 at 04:28:14PM +0000, Steve Twiss wrote:
> > > > > I am using the compatible string to pick a different configuration .data block:
> > > > > { .compatible = "dlg,da9062-watchdog", .data = &da9062_watchdog_info },
> > > > > { .compatible = "dlg,da9061-watchdog", .data = &da9061_watchdog_info },
> > > > >
> > > > > But, it is just my opinion to keep the "name" different.
> > > > > This will not be my decision if accepted into the Linux kernel, but I would like to
> > > > > at least be consistent for DA9061 and DA9062 so ... is this an issue?
> > > >
> > > > FWIW the driver doesn't really need to be updated in the first place.
> > > > A compatible statement listing both da9061 and da9062 would do it.
> > >
> > > I will make the changes you requested: deprecate the existing compatibility
> > > for da9062-watchdog and make a new compatibility string which combines both
> > > da9061 and da9062.
> > >
> > That is not what I asked for.
> 
> Ok. Did you mean separate compatible statements with data sections pointing at 
> the same structure?
> 
> Like this:
> { .compatible = "dlg,da9062-watchdog", .data = &da9062_watchdog_info },
> { .compatible = "dlg,da9061-watchdog", .data = &da9062_watchdog_info },
> 
> So this would be the only change needed in the device driver.

If there is no change in IP block then I do not see why we need to
introduce new names at all. The dts can specify fallback compatible
stting. Note, it is called *compatible* not "model" or "device id" or
whatever. So you can just say in DTS:

	compatible = "dlg,da9061-watchdog", "dlg,da9062-watchdog";

and leave the driver alone. That goes for input part as well. You only
need to add new compatible to the driver when it in fact is
*incompatible* with the existing blocks.

Thanks.

-- 
Dmitry

Re: [PATCH V1 04/10] watchdog: da9061: watchdog driver (RFC)

[Guenter Roeck]

On 10/07/2016 04:35 PM, Dmitry Torokhov wrote:
> On Fri, Oct 07, 2016 at 06:01:23PM +0000, Steve Twiss wrote:
>> On 07 October 2016 18:02, Guenter Roeck wrote:
>>
>>>> On 06 October 2016 19:49, Guenter Roeck wrote:
>>>>> On Thu, Oct 06, 2016 at 04:28:14PM +0000, Steve Twiss wrote:
>>>>>> I am using the compatible string to pick a different configuration .data block:
>>>>>> { .compatible = "dlg,da9062-watchdog", .data = &da9062_watchdog_info },
>>>>>> { .compatible = "dlg,da9061-watchdog", .data = &da9061_watchdog_info },
>>>>>>
>>>>>> But, it is just my opinion to keep the "name" different.
>>>>>> This will not be my decision if accepted into the Linux kernel, but I would like to
>>>>>> at least be consistent for DA9061 and DA9062 so ... is this an issue?
>>>>>
>>>>> FWIW the driver doesn't really need to be updated in the first place.
>>>>> A compatible statement listing both da9061 and da9062 would do it.
>>>>
>>>> I will make the changes you requested: deprecate the existing compatibility
>>>> for da9062-watchdog and make a new compatibility string which combines both
>>>> da9061 and da9062.
>>>>
>>> That is not what I asked for.
>>
>> Ok. Did you mean separate compatible statements with data sections pointing at
>> the same structure?
>>
>> Like this:
>> { .compatible = "dlg,da9062-watchdog", .data = &da9062_watchdog_info },
>> { .compatible = "dlg,da9061-watchdog", .data = &da9062_watchdog_info },
>>
>> So this would be the only change needed in the device driver.
>
> If there is no change in IP block then I do not see why we need to
> introduce new names at all. The dts can specify fallback compatible
> stting. Note, it is called *compatible* not "model" or "device id" or
> whatever. So you can just say in DTS:
>
> 	compatible = "dlg,da9061-watchdog", "dlg,da9062-watchdog";
>
> and leave the driver alone. That goes for input part as well. You only
> need to add new compatible to the driver when it in fact is
> *incompatible* with the existing blocks.
>
Yes, exactly.

Thanks,
Guenter

RE: [PATCH V1 04/10] watchdog: da9061: watchdog driver (RFC)

[Steve Twiss]

On 08 October 2016 01:22, Guenter Roeck wrote:
> On 10/07/2016 04:35 PM, Dmitry Torokhov wrote:
> > On Fri, Oct 07, 2016 at 06:01:23PM +0000, Steve Twiss wrote:
> >> On 07 October 2016 18:02, Guenter Roeck wrote:
> >>
> >>>> On 06 October 2016 19:49, Guenter Roeck wrote:
> >>>>> On Thu, Oct 06, 2016 at 04:28:14PM +0000, Steve Twiss wrote:
> >>>>>> I am using the compatible string to pick a different configuration .data block:
> >>>>>> { .compatible = "dlg,da9062-watchdog", .data = &da9062_watchdog_info },
> >>>>>> { .compatible = "dlg,da9061-watchdog", .data = &da9061_watchdog_info },
> >>>>>>
> >>>>>> But, it is just my opinion to keep the "name" different.
> >>>>>> This will not be my decision if accepted into the Linux kernel, but I would like to
> >>>>>> at least be consistent for DA9061 and DA9062 so ... is this an issue?
> >>>>>
> >>>>> FWIW the driver doesn't really need to be updated in the first place.
> >>>>> A compatible statement listing both da9061 and da9062 would do it.
> >>>>
> >>>> I will make the changes you requested: deprecate the existing compatibility
> >>>> for da9062-watchdog and make a new compatibility string which combines both
> >>>> da9061 and da9062.
> >>>>
> >>> That is not what I asked for.
> >>
> >> Ok. Did you mean separate compatible statements with data sections pointing at
> >> the same structure?
> >>
> >> Like this:
> >> { .compatible = "dlg,da9062-watchdog", .data = &da9062_watchdog_info },
> >> { .compatible = "dlg,da9061-watchdog", .data = &da9062_watchdog_info },
> >>
> >> So this would be the only change needed in the device driver.
> >
> > If there is no change in IP block then I do not see why we need to
> > introduce new names at all. The dts can specify fallback compatible
> > stting. Note, it is called *compatible* not "model" or "device id" or
> > whatever. So you can just say in DTS:
> >
> > 	compatible = "dlg,da9061-watchdog", "dlg,da9062-watchdog";
> >
> > and leave the driver alone. That goes for input part as well. You only
> > need to add new compatible to the driver when it in fact is
> > *incompatible* with the existing blocks.
> >
> Yes, exactly.

Dmitry, Guenter,
Thanks for putting things straight on this.