RE: [PATCH v4 05/11] rtc: isl1208: Make similar I2C and DT-based matching table

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Geert,

Thanks for the feedback.

> -----Original Message-----
> From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Sent: Friday, May 19, 2023 8:43 PM
> To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> Cc: Alessandro Zummo <a.zummo@xxxxxxxxxxxx>; Alexandre Belloni
> <alexandre.belloni@xxxxxxxxxxx>; linux-rtc@xxxxxxxxxxxxxxx; Fabrizio
> Castro <fabrizio.castro.jz@xxxxxxxxxxx>; linux-renesas-
> soc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 05/11] rtc: isl1208: Make similar I2C and DT-
> based matching table
> 
> Hi Biju.
> 
> On Fri, May 19, 2023 at 6:08 PM Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> wrote:
> > > -----Original Message-----
> > > From: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> > > Sent: Friday, May 19, 2023 1:39 PM
> > > To: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > Cc: Alessandro Zummo <a.zummo@xxxxxxxxxxxx>; Alexandre Belloni
> > > <alexandre.belloni@xxxxxxxxxxx>; linux-rtc@xxxxxxxxxxxxxxx; Fabrizio
> > > Castro <fabrizio.castro.jz@xxxxxxxxxxx>; linux-renesas-
> > > soc@xxxxxxxxxxxxxxx
> > > Subject: Re: [PATCH v4 05/11] rtc: isl1208: Make similar I2C and DT-
> > > based matching table
> > >
> > > On Thu, May 18, 2023 at 1:37 PM Biju Das
> > > <biju.das.jz@xxxxxxxxxxxxxx>
> > > wrote:
> > > > The isl1208_id[].driver_data could store a pointer to the config,
> > > > like for DT-based matching, making I2C and DT-based matching more
> similar.
> > > >
> > > > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > ---
> > > > v4:
> > > >  * New patch.
> > >
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> > >
> > > > @@ -822,9 +822,9 @@ isl1208_probe(struct i2c_client *client)
> > > >         } else {
> > > >                 const struct i2c_device_id *id =
> > > > i2c_match_id(isl1208_id, client);
> > > >
> > > > -               if (id->driver_data >= ISL_LAST_ID)
> > > > +               if (!id)
> > > >                         return -ENODEV;
> > > > -               isl1208->config = &isl1208_configs[id-
> >driver_data];
> > > > +               isl1208->config = (struct isl1208_config
> > > > + *)id->driver_data;
> > >
> > > It's a pity there's no i2c_get_match_data() yet...
> >
> > You mean, introduce [1] and optimize ??
> >
> >         if (client->dev.of_node)
> >                 isl1208->config = of_device_get_match_data(&client-
> >dev);
> >         else
> >                 isl1208->config = i2c_get_match_data(isl1208_id,
> > client);
> >
> >         if (!isl1208->config)
> >                 return -ENODEV;
> 
> Exactly.  Might be better to do that later, to avoid stalling this
> series.

OK, will do it later.

> 
> >
> > [1]
> > const void * i2c_get_match_data(const struct i2c_device_id *id, const
> > struct i2c_client *client) {
> >         if (!(id && client))
> >                 return NULL;
> >
> >         while (id->name[0]) {
> >                 if (strcmp(client->name, id->name) == 0)
> >                         return id->driver_data;
> >                 id++;
> >         }
> >         return NULL;
> 
> Please reuse the existing i2c_match_id(), just like
> of_device_get_match_data() reuses of_match_device().

OK, Will send this as standalone patch, as it is enhancement.

Cheers,
Biju

> 
> > }
> > EXPORT_SYMBOL(i2c_get_match_data);
> >
> > Cheers,
> > Biju
> 
> 
> 
> --
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-
> m68k.org
> 
> In personal conversations with technical people, I call myself a hacker.
> But when I'm talking to journalists I just say "programmer" or something
> like that.
>                                 -- Linus Torvalds




[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux