[v2,11/20] reset: npcm: using syscon instead of device data

Using syscon device tree property instead of
device data to handle the NPCM general control
registers.

Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
---
 drivers/reset/reset-npcm.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

On 08/06/2022 11:56, Tomer Maimon wrote:
> Using syscon device tree property instead of
> device data to handle the NPCM general control
> registers.
> 

Again ignored the comment.

> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> ---
>  drivers/reset/reset-npcm.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/reset/reset-npcm.c b/drivers/reset/reset-npcm.c
> index 2ea4d3136e15..312c3b594b8f 100644
> --- a/drivers/reset/reset-npcm.c
> +++ b/drivers/reset/reset-npcm.c
> @@ -138,8 +138,7 @@ static int npcm_reset_xlate(struct reset_controller_dev *rcdev,
>  }
>  
>  static const struct of_device_id npcm_rc_match[] = {
> -	{ .compatible = "nuvoton,npcm750-reset",
> -		.data = (void *)"nuvoton,npcm750-gcr" },
> +	{ .compatible = "nuvoton,npcm750-reset"},
>  	{ }
>  };
>  
> @@ -155,14 +154,10 @@ static int npcm_usb_reset(struct platform_device *pdev, struct npcm_rc_data *rc)
>  	u32 ipsrst1_bits = 0;
>  	u32 ipsrst2_bits = NPCM_IPSRST2_USB_HOST;
>  	u32 ipsrst3_bits = 0;
> -	const char *gcr_dt;
>  
> -	gcr_dt = (const char *)
> -	of_match_device(dev->driver->of_match_table, dev)->data;
> -
> -	gcr_regmap = syscon_regmap_lookup_by_compatible(gcr_dt);
> +	gcr_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "nuvoton,sysgcr");
>  	if (IS_ERR(gcr_regmap)) {
> -		dev_err(&pdev->dev, "Failed to find %s\n", gcr_dt);
> +		dev_err(&pdev->dev, "Failed to find gcr syscon");
>  		return PTR_ERR(gcr_regmap);

Comment still ignored.

There is no point in this review if you keep ignoring what we ask to fix.

If something is unclear, ask for clarification. Resending without
implementing the comment means that you ignore the review which is waste
of my time.

I am sorry, but this is not acceptable.

Best regards,
Krzysztof

Hi Krzysztof

Sorry but I didn't ignore your comment.

For not breaking exciting boards I add the following patch in V2
https://lore.kernel.org/linux-arm-kernel/20220608095623.22327-11-tmaimon77@gmail.com/

On Wed, 8 Jun 2022 at 13:08, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 08/06/2022 11:56, Tomer Maimon wrote:
> > Using syscon device tree property instead of
> > device data to handle the NPCM general control
> > registers.
> >
>
> Again ignored the comment.
>
> > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> > ---
> >  drivers/reset/reset-npcm.c | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/reset/reset-npcm.c b/drivers/reset/reset-npcm.c
> > index 2ea4d3136e15..312c3b594b8f 100644
> > --- a/drivers/reset/reset-npcm.c
> > +++ b/drivers/reset/reset-npcm.c
> > @@ -138,8 +138,7 @@ static int npcm_reset_xlate(struct reset_controller_dev *rcdev,
> >  }
> >
> >  static const struct of_device_id npcm_rc_match[] = {
> > -     { .compatible = "nuvoton,npcm750-reset",
> > -             .data = (void *)"nuvoton,npcm750-gcr" },
> > +     { .compatible = "nuvoton,npcm750-reset"},
> >       { }
> >  };
> >
> > @@ -155,14 +154,10 @@ static int npcm_usb_reset(struct platform_device *pdev, struct npcm_rc_data *rc)
> >       u32 ipsrst1_bits = 0;
> >       u32 ipsrst2_bits = NPCM_IPSRST2_USB_HOST;
> >       u32 ipsrst3_bits = 0;
> > -     const char *gcr_dt;
> >
> > -     gcr_dt = (const char *)
> > -     of_match_device(dev->driver->of_match_table, dev)->data;
> > -
> > -     gcr_regmap = syscon_regmap_lookup_by_compatible(gcr_dt);
> > +     gcr_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "nuvoton,sysgcr");
> >       if (IS_ERR(gcr_regmap)) {
> > -             dev_err(&pdev->dev, "Failed to find %s\n", gcr_dt);
> > +             dev_err(&pdev->dev, "Failed to find gcr syscon");
> >               return PTR_ERR(gcr_regmap);
>
> Comment still ignored.
>
> There is no point in this review if you keep ignoring what we ask to fix.
>
> If something is unclear, ask for clarification. Resending without
> implementing the comment means that you ignore the review which is waste
> of my time.
>
> I am sorry, but this is not acceptable.
>
> Best regards,
> Krzysztof

Best regards,

Tomer

On 09/06/2022 23:37, Tomer Maimon wrote:
> Hi Krzysztof
> 
> Sorry but I didn't ignore your comment.
> 
> For not breaking exciting boards I add the following patch in V2
> https://lore.kernel.org/linux-arm-kernel/20220608095623.22327-11-tmaimon77@gmail.com/

No, it does not solve it.
1. Patchset goes via separate trees (DTS are always separate), so it is
not bisectable. One of the branches/trees will have broken DTS.

2. All out of tree DTSes are broken. This is expressed as ABI and - with
some reasonable exceptions - you should not break it.
https://elixir.bootlin.com/linux/v5.19-rc1/source/Documentation/devicetree/bindings/ABI.rst

You have to keep backwards compatibility, so parse/handle both versions
of DTS.


Best regards,
Krzysztof

Hi Krzysztof,

I will make sure to add backward compatibility in the reset driver in
the next version.

Thanks,

Tomer


On Fri, 10 Jun 2022 at 12:53, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 09/06/2022 23:37, Tomer Maimon wrote:
> > Hi Krzysztof
> >
> > Sorry but I didn't ignore your comment.
> >
> > For not breaking exciting boards I add the following patch in V2
> > https://lore.kernel.org/linux-arm-kernel/20220608095623.22327-11-tmaimon77@gmail.com/
>
> No, it does not solve it.
> 1. Patchset goes via separate trees (DTS are always separate), so it is
> not bisectable. One of the branches/trees will have broken DTS.
>
> 2. All out of tree DTSes are broken. This is expressed as ABI and - with
> some reasonable exceptions - you should not break it.
> https://elixir.bootlin.com/linux/v5.19-rc1/source/Documentation/devicetree/bindings/ABI.rst
>
> You have to keep backwards compatibility, so parse/handle both versions
> of DTS.
>
>
> Best regards,
> Krzysztof