mbox series

[v3,0/2] nvmem: lan9662-otpc: add support

Message ID 20220831064238.102267-1-horatiu.vultur@microchip.com
Headers show
Series nvmem: lan9662-otpc: add support | expand

Message

Horatiu Vultur Aug. 31, 2022, 6:42 a.m. UTC
Add support for lan9662 OTP controller that is available on lan9662.
The driver gives access to non-volatile memory. It allows both write
and read access to it.

v2->v3:
- fix dt-bindings, make sure that make dt_binding_check passes
- remove lan9662_writel/readl/clrbits/setbits and use writel/readl
- remove WARN_ON(offset >= OTP_MEM_SIZE) as it can't happen

v1->v2:
- remove unneed quotes for $ref: nvmem.yaml#
- rename lan966x to lan9662 as not to have any wildcards
- remove compatible string syscon

Horatiu Vultur (2):
  dt-bindings: lan9662-otpc: document Lan9662 OTPC
  nvmem: lan9662-otp: add support.

 .../nvmem/microchip,lan9662-otpc.yaml         |  45 ++++
 drivers/nvmem/Kconfig                         |   8 +
 drivers/nvmem/Makefile                        |   2 +
 drivers/nvmem/lan9662-otpc.c                  | 223 ++++++++++++++++++
 4 files changed, 278 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/microchip,lan9662-otpc.yaml
 create mode 100644 drivers/nvmem/lan9662-otpc.c

Comments

Krzysztof Kozlowski Aug. 31, 2022, 7:29 a.m. UTC | #1
On 31/08/2022 09:42, Horatiu Vultur wrote:

> +static const struct of_device_id lan9662_otp_match[] = {
> +	{ .compatible = "microchip,lan9662-otp", },
> +	{ .compatible = "microchip,lan9668-otp", },

This is still wrong, does not match your bindings at all and still
duplicates entries without driver data. One entry - 9662.

Best regards,
Krzysztof
Horatiu Vultur Aug. 31, 2022, 10:52 a.m. UTC | #2
The 08/31/2022 10:29, Krzysztof Kozlowski wrote:

Hi Krzysztof,

> 
> On 31/08/2022 09:42, Horatiu Vultur wrote:
> 
> > +static const struct of_device_id lan9662_otp_match[] = {
> > +     { .compatible = "microchip,lan9662-otp", },
> > +     { .compatible = "microchip,lan9668-otp", },
> 
> This is still wrong, does not match your bindings at all and still
> duplicates entries without driver data. One entry - 9662.

I have look at some other drivers, where I can see they don't have any
driver data. For example [1] and the bindings are here [2].

[1] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/ti/cpsw_new.c#L1832
[2] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/net/ti,cpsw-switch.yaml#L23

Is this also wrong, or I still can't understand how the bindings are
working?

If I put only one entry:
---
static const struct of_device_id lan9662_otp_match[] = {
     { .compatible = "microchip,lan9662-otp", },
---

Wouldn't be a problem that the binding mentions also lan9668?

> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski Aug. 31, 2022, 12:52 p.m. UTC | #3
On 31/08/2022 13:52, Horatiu Vultur wrote:
> The 08/31/2022 10:29, Krzysztof Kozlowski wrote:
> 
> Hi Krzysztof,
> 
>>
>> On 31/08/2022 09:42, Horatiu Vultur wrote:
>>
>>> +static const struct of_device_id lan9662_otp_match[] = {
>>> +     { .compatible = "microchip,lan9662-otp", },
>>> +     { .compatible = "microchip,lan9668-otp", },
>>
>> This is still wrong, does not match your bindings at all and still
>> duplicates entries without driver data. One entry - 9662.
> 
> I have look at some other drivers, where I can see they don't have any
> driver data. For example [1] and the bindings are here [2].
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/ti/cpsw_new.c#L1832
> [2] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/net/ti,cpsw-switch.yaml#L23

There are plenty of poor examples in Linux kernel code and it is not a
reason to re-use their patterns...

> Is this also wrong, or I still can't understand how the bindings are
> working?

The topic here is not that much related to the bindings, but device
matching in Linux kernel.

> 
> If I put only one entry:
> ---
> static const struct of_device_id lan9662_otp_match[] = {
>      { .compatible = "microchip,lan9662-otp", },
> ---
> 
> Wouldn't be a problem that the binding mentions also lan9668?

No. What could be the problem exactly, which you are afraid? Why
implementation should be a problem for a binding (which we try to be
mostly implementation independent)?

Best regards,
Krzysztof
Horatiu Vultur Aug. 31, 2022, 2:52 p.m. UTC | #4
The 08/31/2022 15:52, Krzysztof Kozlowski wrote:
> 
> On 31/08/2022 13:52, Horatiu Vultur wrote:
> > The 08/31/2022 10:29, Krzysztof Kozlowski wrote:
> >
> > Hi Krzysztof,
> >
> >>
> >> On 31/08/2022 09:42, Horatiu Vultur wrote:
> >>
> >>> +static const struct of_device_id lan9662_otp_match[] = {
> >>> +     { .compatible = "microchip,lan9662-otp", },
> >>> +     { .compatible = "microchip,lan9668-otp", },
> >>
> >> This is still wrong, does not match your bindings at all and still
> >> duplicates entries without driver data. One entry - 9662.
> >
> > I have look at some other drivers, where I can see they don't have any
> > driver data. For example [1] and the bindings are here [2].
> >
> > [1] https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/ti/cpsw_new.c#L1832
> > [2] https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/net/ti,cpsw-switch.yaml#L23
> 
> There are plenty of poor examples in Linux kernel code and it is not a
> reason to re-use their patterns...
> 
> > Is this also wrong, or I still can't understand how the bindings are
> > working?
> 
> The topic here is not that much related to the bindings, but device
> matching in Linux kernel.
> 
> >
> > If I put only one entry:
> > ---
> > static const struct of_device_id lan9662_otp_match[] = {
> >      { .compatible = "microchip,lan9662-otp", },
> > ---
> >
> > Wouldn't be a problem that the binding mentions also lan9668?
> 
> No. What could be the problem exactly, which you are afraid? Why
> implementation should be a problem for a binding (which we try to be
> mostly implementation independent)?

The implementation wouldn't be a problem for the binding.
The only thing was if the binding has more compatible strings than what
the driver supports. As an example, in the binding we mention about
lan9668 but nothing in the driver.

> 
> Best regards,
> Krzysztof
Krzysztof Kozlowski Aug. 31, 2022, 3:17 p.m. UTC | #5
On 31/08/2022 17:52, Horatiu Vultur wrote:
>>> If I put only one entry:
>>> ---
>>> static const struct of_device_id lan9662_otp_match[] = {
>>>      { .compatible = "microchip,lan9662-otp", },
>>> ---
>>>
>>> Wouldn't be a problem that the binding mentions also lan9668?
>>
>> No. What could be the problem exactly, which you are afraid? Why
>> implementation should be a problem for a binding (which we try to be
>> mostly implementation independent)?
> 
> The implementation wouldn't be a problem for the binding.
> The only thing was if the binding has more compatible strings than what
> the driver supports. As an example, in the binding we mention about
> lan9668 but nothing in the driver.

What is that "only thing"? What is the problem? It does not matter
really if the driver mentions or does not mention lan9668, because it's
not related...

Best regards,
Krzysztof