Re: [PATCH 17/21] power: reset: at91-poweroff: lookup for proper pmc dt node for sam9x7

[Conor Dooley]

Hey,

On Mon, Jun 05, 2023 at 03:04:34PM +0200, Nicolas Ferre wrote:
> On 05/06/2023 at 08:43, Krzysztof Kozlowski wrote:
> > On 03/06/2023 22:02, Varshini Rajendran wrote:
> > > Use sam9x7 pmc's compatible to lookup for in the SHDWC driver
> > > 
> > > Signed-off-by: Varshini Rajendran <varshini.rajendran@xxxxxxxxxxxxx>
> > > ---
> > >   drivers/power/reset/at91-sama5d2_shdwc.c | 1 +
> > >   1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/power/reset/at91-sama5d2_shdwc.c b/drivers/power/reset/at91-sama5d2_shdwc.c
> > > index d8ecffe72f16..d0f29b99f25e 100644
> > > --- a/drivers/power/reset/at91-sama5d2_shdwc.c
> > > +++ b/drivers/power/reset/at91-sama5d2_shdwc.c
> > > @@ -326,6 +326,7 @@ static const struct of_device_id at91_pmc_ids[] = {
> > >        { .compatible = "atmel,sama5d2-pmc" },
> > >        { .compatible = "microchip,sam9x60-pmc" },
> > >        { .compatible = "microchip,sama7g5-pmc" },
> > > +     { .compatible = "microchip,sam9x7-pmc" },
> > 
> > Why do you need new entry if these are compatible?
> 
> Yes, PMC is very specific to a SoC silicon. As we must look for it in the
> shutdown controller, I think we need a new entry here.

Copy-pasting this for a wee bit of context as I have two questions.

| static const struct of_device_id at91_shdwc_of_match[] = {
| 	{
| 		.compatible = "atmel,sama5d2-shdwc",
| 		.data = &sama5d2_reg_config,
| 	},
| 	{
| 		.compatible = "microchip,sam9x60-shdwc",
| 		.data = &sam9x60_reg_config,
| 	},
| 	{
| 		.compatible = "microchip,sama7g5-shdwc",
| 		.data = &sama7g5_reg_config,
| 	}, {
| 		/*sentinel*/
| 	}
| };
| MODULE_DEVICE_TABLE(of, at91_shdwc_of_match);
| 
| static const struct of_device_id at91_pmc_ids[] = {
| 	{ .compatible = "atmel,sama5d2-pmc" },
| 	{ .compatible = "microchip,sam9x60-pmc" },
| 	{ .compatible = "microchip,sama7g5-pmc" },
| 	{ .compatible = "microchip,sam9x7-pmc" },
| 	{ /* Sentinel. */ }
| };

If there's no changes made to the code, other than adding an entry to
the list of pmc compatibles, then either this has the same as an
existing SoC, or there is a bug in the patch, since the behaviour of
the driver will not have changed.

Secondly, this patch only updates the at91_pmc_ids and the dts patch
contains:
| shutdown_controller: shdwc@fffffe10 {
| 	compatible = "microchip,sam9x60-shdwc";
| 	reg = <0xfffffe10 0x10>;
| 	clocks = <&clk32k 0>;
| 	#address-cells = <1>;
| 	#size-cells = <0>;
| 	atmel,wakeup-rtc-timer;
| 	atmel,wakeup-rtt-timer;
| 	status = "disabled";
| };

...which would mean that the there's nothing different between the
programming models for the sam9x60 and sam9x7. If that's the case, the
dt-binding & dts should list the sam9x60 as a fallback for the sam9x7 &
there is no change required to the driver. If it's not the case, then
there's a bug in this patch and the dts one :)

In general, if things are the same as previous products, there's no need
to change the drivers at all & just add fallback compatibles to the
bindings and dts. IFF some difference pops up in the future, then the
sam9x7 compatible will already exist in the dts, and can then be added
to the driver.

Cheers,
Conor.

Attachment: signature.asc
Description: PGP signature