Netdev List
 help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Sagi Maimon <maimon.sagi@gmail.com>
Cc: Vadim Fedorenko <vadim.fedorenko@linux.dev>,
	jonathan.lemon@gmail.com, richardcochran@gmail.com,
	andrew+netdev@lunn.ch, davem@davemloft.net, edumazet@google.com,
	pabeni@redhat.com, linux-kernel@vger.kernel.org,
	netdev@vger.kernel.org
Subject: Re: [PATCH v6] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1
Date: Tue, 28 Jul 2026 15:26:09 -0700	[thread overview]
Message-ID: <20260728152609.10b4c647@kernel.org> (raw)
In-Reply-To: <CAMuE1bHMe3BHz6YoAgXXtP+TpBEos6jaHFpBNPYjFHXKVxd4_g@mail.gmail.com>

On Tue, 28 Jul 2026 09:09:33 +0300 Sagi Maimon wrote:
> Thanks for the feedback.
> 
> My initial implementation actually exposed a real I2C bus to user
> space. However, following Vadim Fedorenko's review comments:
> 
> https://lore.kernel.org/all/c6aff5f7-e087-4bd9-b159-7adeb82e19f4@linux.dev/
> 
> I changed the design and switched to a sysfs-based interface for the
> ADVA device only.
> 
> From your comment, it sounds like neither of these approaches is
> acceptable in its current form. Could you please clarify what
> implementation you would consider appropriate for this use case?
> 
> Specifically, would you prefer:
> 
> exposing a proper I2C adapter/bus to user space, or
> implementing the required device operations directly in the driver and
> exposing only higher-level controls?
> 
> I'd like to understand the expected direction before respinning the patch.

Reminder: please don't top post.

I'm not sure what hangs on that bus. The commit message seemed 
to go into the topology but not into the "why" of the patch.

Assuming this is a fixed function bus within the device sysfs
attributes dedicated to what the user wants to do are likely
best. Again, hard to say for sure since I don't know what sits
on that bus.

Exposing what is effectively raw write over a custom attribute
is the worst possible choice. Neither does it use the standard i2c
infra, nor does it avoid raw access and per-vendor tooling.

Ideally you'd add dedicated attributes that are human-readable/writable
and have the driver translate the user intent into transactions on 
the bus. I'd assume this is what Vadim expected in the email you quoted.

      reply	other threads:[~2026-07-28 22:26 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-07-23 14:39 [PATCH v6] ptp: ocp: add CPLD ISP support for ADVA TimeCard X1 Sagi Maimon
2026-07-27 11:02 ` Vadim Fedorenko
2026-07-27 21:03   ` Jakub Kicinski
2026-07-28  6:09     ` Sagi Maimon
2026-07-28 22:26       ` Jakub Kicinski [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260728152609.10b4c647@kernel.org \
    --to=kuba@kernel.org \
    --cc=andrew+netdev@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=jonathan.lemon@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=maimon.sagi@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=richardcochran@gmail.com \
    --cc=vadim.fedorenko@linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox