Re: [PATCH RFC v6 6/6] ptp_ocp: implement DPLL ops

[Jiri Pirko <jiri@resnulli.us>]

Sat, Apr 01, 2023 at 01:28:29AM CEST, vadim.fedorenko@linux.dev wrote:
>On 15.03.2023 12:24, Jiri Pirko wrote:
>> Wed, Mar 15, 2023 at 01:10:33AM CET, vadim.fedorenko@linux.dev wrote:
>> > On 14/03/2023 10:05, Jiri Pirko wrote:
>> > > Sun, Mar 12, 2023 at 03:28:07AM CET, vadfed@meta.com wrote:

[...]


>> > > > +static int ptp_ocp_dpll_pin_to_sma(const struct ptp_ocp *bp, const struct dpll_pin *pin)
>> > > > +{
>> > > > +	int i;
>> > > > +
>> > > > +	for (i = 0; i < 4; i++) {
>> > > > +		if (bp->sma[i].dpll_pin == pin)
>> > > > +			return i;
>> > > 
>> > > Just pass &bp->sma[i] as a priv to dpll_pin_register().
>> > > and get that pointer directly in pin ops. No need for lookup and use of
>> > > sma_nr at all.
>> > 
>> > I'm still thinking about the change that you proposed to remove these
>> > _priv() helpers. I have to look into ice code to be sure we won't break
>> > any assumptions in it with moving to additional (void *) parameter.
>> 
>> There are basically 2 ways:
>> someop(struct subsystemobj *x, void *priv)
>> {
>> 	struct *mine = priv;
>> }
>> or:
>> someop(struct subsystemobj *x)
>> {
>> 	struct *mine = subsystem_get_priv(x);
>> }
>> 
>> Both are more or less equal. The first has benefit that the caller most
>> usually has direct access to the priv, so it can just easily pass it on.
>> Also, you as the driver write see right away there is a priv arg and
>> makes you want to use it and not figure out odd lookups to get to the
>> same priv.
>
>Thinking about this part. We have tons of parameters for some ops already.
>Adding void *priv to every one of them (and actually two for pins) makes them
>even more ugly. Let's stay with helpers if you don't have strong opinion
>against.

Well, with the patches I sent when 1 device/pin can be registered
multiple times with multiple privs, I believe it is much more feasable
to just pass the priv along in the arg list, because otherwise you would
have to do some odd lookup.

Please pass priv, would be nicer and easier and more simple for the
driver to implement. The arg list can handle that easily.

[...]