linux-kernel - Re: [PATCH v2 4/6] usb: typec: tipd: Add support for Apple CD321X

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YVF616Ga8jf2tWYJ@kuha.fi.intel.com>
Date:   Mon, 27 Sep 2021 11:03:35 +0300
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Sven Peter <sven@...npeter.dev>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Guido Günther <agx@...xcpu.org>,
        Bryan O'Donoghue <bryan.odonoghue@...aro.org>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Hector Martin <marcan@...can.st>,
        Mohamed Mediouni <mohamed.mediouni@...amail.com>,
        Stan Skowronek <stan@...ellium.com>,
        Mark Kettenis <mark.kettenis@...all.nl>,
        Alexander Graf <graf@...zon.com>,
        Alyssa Rosenzweig <alyssa@...enzweig.io>
Subject: Re: [PATCH v2 4/6] usb: typec: tipd: Add support for Apple CD321X

Hi Sven,

On Fri, Sep 24, 2021 at 04:58:52PM +0200, Sven Peter wrote:
> > Couldn't you just use the compatible property and local variable here?
> >
> >         irq_handler_t irq_handler = tps6598x_interrupt;
> >         struct device_node *np = client->dev.of_node;
> >
> >         if (np && of_device_is_compatible(np, "apple,cd321x")) {
> >                 /* CD321X chips have all interrupts masked initially */
> >                 ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
> >                                         APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
> >                                         APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
> >                                         APPLE_CD_REG_INT_PLUG_EVENT);
> >                 if (ret)
> >                         return ret;
> >
> >                 irq_handler = cd321x_interrupt;
> >         }
> >
> > 	ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> > 					irq_handler,
> > 					IRQF_SHARED | IRQF_ONESHOT,
> > 					dev_name(&client->dev), tps);
> >
> > I did not go over the whole series yet, so I may have missed
> > something.
> 
> Sure, that will work and get rid of the slightly awkward and redundant interrupt/enum
> combination. I'll wait for your comments for the rest of the series and do that for v3 then :)

The rest of the series look OK to me.

thanks,

-- 
heikki