> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: Friday, January 24, 2025 3:17 PM
> Subject: Re: [PATCH v4 1/9] soc: renesas: Add SYSC driver for Renesas RZ
> family
Hi Geert,
Thanks for your review.
>
> Hi John, Claudiu,
>
> On Thu, Jan 23, 2025 at 6:05 PM John Madieu
> <john.madieu.xa@xxxxxxxxxxxxxx> wrote:
> > From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> >
> > The RZ/G3S system controller (SYSC) has various registers that control
> > different functionalities. One of the exposed register offsers
> > information about the SoC identification.
> >
> > Add a driver that identifies the SoC. Later the driver will be
> > extended with other functionalities.
> >
> > Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
> > Signed-off-by: John Madieu <john.madieu.xa@xxxxxxxxxxxxxx>
> > ---
> > v4: No changes
> > v3: New patch
>
> Thanks for your patch!
>
> > --- /dev/null
> > +++ b/drivers/soc/renesas/rz-sysc.c
>
> > +static int rz_sysc_soc_init(struct rz_sysc *sysc, const struct
> > +of_device_id *match) {
> > + const struct rz_sysc_init_data *sysc_data = match->data;
> > + const struct rz_sysc_soc_id_init_data *soc_data = sysc_data-
> >soc_id_init_data;
> > + struct soc_device_attribute *soc_dev_attr;
> > + const char *soc_id_start, *soc_id_end;
> > + u32 val, revision, specific_id;
> > + struct soc_device *soc_dev;
> > + char soc_id[32] = {0};
> > + size_t size;
> > +
> > + soc_id_start = strchr(match->compatible, ',') + 1;
> > + soc_id_end = strchr(match->compatible, '-');
> > + size = soc_id_end - soc_id_start + 1;
> > + if (size > 32)
> > + size = sizeof(soc_id);
> > + strscpy(soc_id, soc_id_start, size);
> > +
> > + soc_dev_attr = devm_kzalloc(sysc->dev, sizeof(*soc_dev_attr),
> GFP_KERNEL);
> > + if (!soc_dev_attr)
> > + return -ENOMEM;
> > +
> > + soc_dev_attr->family = devm_kstrdup(sysc->dev,
> > + soc_data->family, GFP_KERNEL);
>
> Missing NULL-check
My bad. I missed it. I'll add it in v5.
>
> > + soc_dev_attr->soc_id = devm_kstrdup(sysc->dev, soc_id,
> GFP_KERNEL);
> > + if (!soc_dev_attr->soc_id)
> > + return -ENOMEM;
> > +
> > + val = readl(sysc->base + soc_data->offset);
> > + revision = field_get(soc_data->revision_mask, val);
> > + specific_id = field_get(soc_data->specific_id_mask, val);
> > + soc_dev_attr->revision = devm_kasprintf(sysc->dev, GFP_KERNEL,
> "%u", revision);
> > + if (!soc_dev_attr->revision)
> > + return -ENOMEM;
> > +
> > + if (soc_data->id && specific_id != soc_data->id) {
> > + dev_warn(sysc->dev, "SoC mismatch (product = 0x%x)\n",
> specific_id);
> > + return -ENODEV;
> > + }
> > +
> > + dev_info(sysc->dev, "Detected Renesas %s %s Rev %s\n",
> soc_dev_attr->family,
> > + soc_dev_attr->soc_id, soc_dev_attr->revision);
> > +
> > + soc_dev = soc_device_register(soc_dev_attr);
> > + if (IS_ERR(soc_dev))
> > + return PTR_ERR(soc_dev);
> > +
> > + return 0;
> > +}
>
> > --- /dev/null
> > +++ b/drivers/soc/renesas/rz-sysc.h
> > @@ -0,0 +1,37 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Renesas RZ System Controller
> > + *
> > + * Copyright (C) 2024 Renesas Electronics Corp.
> > + */
> > +
> > +#ifndef __SOC_RENESAS_RZ_SYSC_H__
> > +#define __SOC_RENESAS_RZ_SYSC_H__
> > +
> > +#include <linux/types.h>
> > +
> > +/**
> > + * struct rz_syc_soc_id_init_data - RZ SYSC SoC identification
> > +initialization data
> > + * @family: RZ SoC family
> > + * @id: RZ SoC expected ID
> > + * @offset: SYSC SoC ID register offset
> > + * @revision_mask: SYSC SoC ID revision mask
> > + * @specific_id_mask: SYSC SoC ID specific ID mask */ struct
> > +rz_sysc_soc_id_init_data {
> > + const char * const family;
> > + u32 id;
> > + u32 offset;
>
> .devid_offset?
Will rename it. Thanks.
>
> > + u32 revision_mask;
> > + u32 specific_id_mask;
> > +};
>
> Gr{oetje,eeting}s,
>
Best regards,
John
